imgtool: formalize public-key-only PEM support#2702
imgtool: formalize public-key-only PEM support#2702JPHutchins wants to merge 1 commit intomcu-tools:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Formalizes and documents imgtool’s long-standing support for public-key-only PEMs (for getpub / getpubhash and Zephyr bootloader builds), while ensuring imgtool sign fails with a clear user-facing error when only a public key is provided.
Changes:
- Add parametrized tests verifying
getpub/getpubhashbehave identically when given a keypair PEM vs a public-key-only PEM. - Convert the
imgtool signfailure mode for public-only PEMs from an internalAttributeErrorto a descriptiveclick.UsageError, with a regression test. - Update docs to explicitly state that bootloader builds can consume public-only PEMs, but signing still requires a private key.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/tests/test_keys.py | Adds coverage for public-only PEM inputs for getpub/getpubhash and a clean-failure regression test for sign. |
| scripts/imgtool/main.py | Adds a click.UsageError guard when sign is invoked with a public-only PEM. |
| docs/signed_images.md | Documents that bootloader builds can use an exported public-key PEM (and notes signing still needs private key). |
| docs/readme-zephyr.md | Clarifies CONFIG_BOOT_SIGNATURE_KEY_FILE can point to keypair PEM or public-only PEM. |
| docs/imgtool.md | Updates getpub documentation to explicitly allow public-only PEM input. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if key is not None and not isinstance( | ||
| key, ( | ||
| keys.Ed25519, | ||
| keys.RSA, | ||
| keys.ECDSA256P1, | ||
| keys.ECDSA384P1, | ||
| keys.X25519 | ||
| ), | ||
| ): | ||
| raise click.UsageError( | ||
| "Cannot sign with a public-only PEM; signing requires the " | ||
| "private key." | ||
| ) |
There was a problem hiding this comment.
The new check treats “not one of these private-key wrapper classes” as synonymous with “public-only PEM”. That works with the current set of key wrappers, but it’s brittle and the error message would become misleading if another key type is added (or if load_key ever returns a different wrapper). Consider checking explicitly for the public-only wrapper types (keys.RSAPublic, keys.ECDSA256P1Public, keys.ECDSA384P1Public, keys.Ed25519Public, keys.X25519Public) (e.g., via type(key) is ...) and raising UsageError only in that case.
There was a problem hiding this comment.
I kind of agree with Copilot here, although I'm not sure either is actually all that great. What would be even better would be if there was an api on key that would just tell us if it had a signing key (or could be used for signing). Aside from that, I'm not sure if this not list, or a positive list based on the Public variants is really better. Both cases require the list to be updated when a new key type is added.
There was a problem hiding this comment.
I am investigating potential for a more maintainable approach.
There was a problem hiding this comment.
I've added static + runtime check using structural subtyping so that this line doesn't ever need to be updated when new key types are added:
if key is not None and not isinstance(key, (keys.PayloadSigner, keys.DigestSigner)):
raise click.UsageError(
"Cannot sign with a public-only PEM; signing requires the "
"private key."
)
d3zd3z
left a comment
There was a problem hiding this comment.
Just the Copilot issue to consider, but not really blockers.
b545836 to
5a15962
Compare
|
imgtool's keys.load() silently falls through to load_pem_public_key when the PEM contains no private material. As a result, `getpub`, `getpubhash`, and `verify` have long accepted public-key-only PEMs, which the Zephyr bootloader build relies on. This was neither tested nor documented, leaving the capability exposed to silent regression. Add parametrized tests covering getpub and getpubhash against pub-only PEMs for all supported key types and encodings, asserting byte equivalence with the private-key path. Replace the AttributeError raised when `imgtool sign` is invoked with a pub-only PEM with a descriptive click.UsageError, and add a regression test. This matters for dev/prod key-split workflows where the signing private key is held only by a release team. Update docs/imgtool.md, docs/signed_images.md, and docs/readme-zephyr.md to state that the bootloader build accepts pub-only PEMs, and that `imgtool sign` itself still requires the private key. Signed-off-by: JP Hutchins <jp@intercreate.io>
5a15962 to
d53bad3
Compare
imgtool's keys.load() silently falls through to load_pem_public_key when the PEM contains no private material. As a result,
getpub,getpubhash, andverifyhave long accepted public-key-only PEMs, which the Zephyr bootloader build relies on. This was neither tested nor documented, leaving the capability exposed to silent regression.Add parametrized tests covering getpub and getpubhash against pub-only PEMs for all supported key types and encodings, asserting byte equivalence with the private-key path.
Replace the AttributeError raised when
imgtool signis invoked with a pub-only PEM with a descriptive click.UsageError, and add a regression test. This matters for dev/prod key-split workflows where the signing private key is held only by a release team.Update docs/imgtool.md, docs/signed_images.md, and docs/readme-zephyr.md to state that the bootloader build accepts pub-only PEMs, and that
imgtool signitself still requires the private key.