scripts/imgtool.py: Add PKCS#11 ECDSA P384 support #2444
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fundakol
suggested changes
Sep 5, 2025
| def __init__(self, uri, env=None): | ||
| if env is None: | ||
| env = os.environ | ||
| if not 'PKCS11_PIN' in env.keys(): |
There was a problem hiding this comment.
Suggested change
| if not 'PKCS11_PIN' in env.keys(): | |
| if 'PKCS11_PIN' not in env: |
|
|
||
| def unquote_to_bytes(urlencoded_string): | ||
| """Replace %xx escapes by their single-character equivalent, | ||
| using the “iso-8859-1” encoding to decode all 8-bit values. |
There was a problem hiding this comment.
Add indentation with 4 spaces.
| if not 'PKCS11_PIN' in env.keys(): | ||
| raise RuntimeError("Environment variable PKCS11_PIN not set. Set it to the user PIN.") | ||
| params = get_pkcs11_uri_params(uri) | ||
| assert b'serial' in params.keys() |
There was a problem hiding this comment.
Suggested change
| assert b'serial' in params.keys() | |
| assert b'serial' in params |
| raise RuntimeError("Environment variable PKCS11_PIN not set. Set it to the user PIN.") | ||
| params = get_pkcs11_uri_params(uri) | ||
| assert b'serial' in params.keys() | ||
| assert b'id' in params.keys() or b'label' in params.keys() |
There was a problem hiding this comment.
Suggested change
| assert b'id' in params.keys() or b'label' in params.keys() | |
| assert b'id' in params or b'label' in params |
Comment on lines
76
to
77
| lib = '' | ||
| try: | ||
| lib = pkcs11.lib(pkcs11_module_path) | ||
| except RuntimeError: | ||
| pass # happens if lib does not exist or is corrupt | ||
| if '' == lib: | ||
| raise RuntimeError(f"PKCS11 module {pkcs11_module_path} not loaded.") |
There was a problem hiding this comment.
I guess pkcs11.lib(pkcs11_module_path) does not return an empty string.
Suggested change
| lib = '' | |
| try: | |
| lib = pkcs11.lib(pkcs11_module_path) | |
| except RuntimeError: | |
| pass # happens if lib does not exist or is corrupt | |
| if '' == lib: | |
| raise RuntimeError(f"PKCS11 module {pkcs11_module_path} not loaded.") | |
| try: | |
| lib = pkcs11.lib(pkcs11_module_path) | |
| except RuntimeError: | |
| raise RuntimeError(f"PKCS11 module {pkcs11_module_path} not loaded.") |
| # signatures to be padded to a fixed length. Because the DER | ||
| # encoding is done with signed integers, the size of the | ||
| # signature will vary depending on whether the high bit is set | ||
| # in each value. This padding was done in a |
There was a problem hiding this comment.
Comment was copied from here:
https://github.com/mcu-tools/mcuboot/blob/main/scripts/imgtool/keys/ecdsa.py#L161
But sure, will fix this and address all the other feedback, thanks!
|
|
||
| def sig_len(self): | ||
| # Early versions of MCUboot (< v1.5.0) required ECDSA | ||
| # signatures to be padded to a fixed length. Because the DER |
| # not-easily-reversible way (by just adding zeros). | ||
| # | ||
| # The signing code no longer requires this padding, and newer | ||
| # versions of MCUboot don't require it. But, continue to |
| sys.path.insert(0, os.path.abspath( | ||
| os.path.join(os.path.dirname(__file__), '../..'))) | ||
|
|
||
| from datetime import datetime |
There was a problem hiding this comment.
Imports should be grouped in the following order:
- Standard library imports.
- Related third party imports.
- Local application/library specific imports.
| ) | ||
| pubkey.destroy() | ||
|
|
||
|
|
There was a problem hiding this comment.
Keep single line separation between methods.
Based on work submitted in relation to: Issue mcu-tools#599 ref: mcu-tools#599 particularly these commits: grandcentrix/mcuboot@82441bd4286 grandcentrix/mcuboot@010ea89f rretanubun@33c6400a40 Updated and modified to support ECDSA P384 keys. Tests also updated and fixed, tested with SoftHSMv2. Signed-off-by: Ian Jamison <[email protected]> Co-authored-by: Michael Zimmermann <[email protected]> Co-authored-by: Nils Dagsson Moskopp <[email protected]> Co-authored-by: Richard Retanubun <[email protected]> Change-Id: I175b710834bd20a868961634483d43b459959769
IanJ-Arm
force-pushed
the
imgtool-pkcs11-signing-flat
branch
from
21627be to
2474bf7
Compare
|
@fundakol Pushed a revised set which hopefully addresses all your points. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on work submitted in relation to:
Issue #599 ref: #599
particularly these commits:
grandcentrix/mcuboot@82441bd4286
grandcentrix/mcuboot@010ea89f
rretanubun@33c6400a40
Updated and modified to support ECDSA P384 keys.
Tests also updated and fixed, tested with SoftHSMv2.