Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nrf fromlist] imgtool: Add pure signature support #342

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

de-nordic
Copy link
Contributor

@de-nordic de-nordic commented Oct 1, 2024

Adds PureEdDSA signature support.

The change includes implementation of SIG_PURE TLV that, when present, indicates the signature that is present is Pure type.

Upstream PR: mcu-tools/mcuboot#2063

Rebased on top of: #344

Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not going to work, the compression one will be merged which will then cause a merge conflict and the only way to avoid that is to rebase this original PR once the compression PR is merged upstream, and david did not seem ok with that when I asked him to review it the other day "where are the mcuboot changes?", but irrespective this will have a merge conflict and will not be able to get merged

@de-nordic
Copy link
Contributor Author

This is not going to work, the compression one will be merged which will then cause a merge conflict and the only way to avoid that is to rebase this original PR once the compression PR is merged upstream, and david did not seem ok with that when I asked him to review it the other day "where are the mcuboot changes?", but irrespective this will have a merge conflict and will not be able to get merged

Conflicts are unavoidable then; the missing change is in draft now: mcu-tools/mcuboot#2080

@de-nordic de-nordic force-pushed the img-pure branch 3 times, most recently from 5322612 to 5857311 Compare October 4, 2024 17:13
@@ -20,7 +20,15 @@
Image signing and management.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commit 35c9291fcafafe8608722e0ec3c801178884f0ef does not exist

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rebased on NCS and it seems that the additional commits, revert and bring new, are no longer needed, so all that is left is the commit with pure support.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot be applied until compression support is merged into upstream and your upstream PR is rebased and this cherry-picked again:

git cherry-pick e872745275fe6920494caf297b1ce996c59ea21f
Auto-merging scripts/imgtool/image.py
CONFLICT (content): Merge conflict in scripts/imgtool/image.py
Auto-merging scripts/imgtool/main.py
CONFLICT (content): Merge conflict in scripts/imgtool/main.py
error: could not apply e8727452... imgtool: Add pure signature support
hint: After resolving the conflicts, mark them with
hint: "git add/rm <pathspec>", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
hint: Disable this message with "git config advice.mergeConflict false"

I will ping David to take a look today at the tech call later

@de-nordic
Copy link
Contributor Author

@nordicjm Please take a look

@nordicjm
Copy link
Contributor

nordicjm commented Oct 9, 2024

This needs someone that knows python to take a look, I have no idea what I'm looking at

Copy link

@fundakol fundakol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, although create method is very complex and hard to understand the business logic.

@@ -203,11 +211,17 @@ def key_and_user_sha_to_alg_and_tlv(key, user_sha):
# If key is not None, then we have to filter hash to only allowed
allowed = None
try:
allowed = ALLOWED_KEY_SHA[type(key)]
if is_pure:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: It is fine, but it is better to keep only single call between try..except. Code can look like so:

allowed_key_ssh = ALLOWED_PURE_KEY_SHA if is_pure else ALLOWED_KEY_SHA
try:
    allowed = allowed_key_ssh[type(key)]
except KeyError:
    ...

message = digest;
tlv.add(hash_tlv, digest)
self.image_hash = digest
if not is_pure:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would replace if not is_pure: with:

if is_pure:
    ...
else:
    ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is no longer there.

@de-nordic de-nordic added this to the ncs-2.8.0 milestone Oct 10, 2024
@de-nordic
Copy link
Contributor Author

Looks good to me, although create method is very complex and hard to understand the business logic.

Thanks. I am generally working on re-factoring some of the code here, I mean unrelated to pure thing, but that will take me a moment to sort out, and I do not want to do it in this PR.

Adds PureEdDSA signature support.

The change includes implementation of SIG_PURE TLV that, when present,
indicates the signature that is present is Pure type.

Upstream PR: mcu-tools/mcuboot#2063

Signed-off-by: Dominik Ermel <dominik.ermel@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants