Skip to content

[nrf noup] Added BOOT_SIGNATURE_USING_ITS for ecdsa configuration #476

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.

Sign up for GitHub

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

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

[nrf noup] Added BOOT_SIGNATURE_USING_ITS for ecdsa configuration #476

wants to merge 1 commit into from

Conversation

ahasztag
Copy link
Contributor

This configuration has the purpose of using keys provisioned to the internal trusted storage (ITS). It makes use of the already existing parts of code for MCUBOOT_BUILTIN_KEY

@ahasztag
Copy link
Contributor Author

ahasztag commented Jul 24, 2025 •
edited
Loading

Note: I have not added nrf-squash! as I consider it to be a new feature and I do not see anything to squash it with. Please let me know if there is a commit that this should be squashed with

@ahasztag ahasztag marked this pull request as draft July 24, 2025 07:54
nordicjm
nordicjm requested changes Jul 24, 2025
View reviewed changes
boot/bootutil/include/bootutil/crypto/ecdsa.h Outdated
static psa_key_id_t builtin_key_ids[BOOT_SIGNATURE_BUILTIN_KEY_SLOTS] = {
0x40022100,
0x40022101,
// 0x40022102,
Copy link
Contributor

Choose a reason for hiding this comment

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

commented line can go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

boot/bootutil/include/bootutil/crypto/ecdsa.h Outdated
@@ -491,6 +495,61 @@ static inline int bootutil_ecdsa_verify(bootutil_ecdsa_context *ctx,
return (int) psa_verify_hash(ctx->key_id, PSA_ALG_ECDSA(ctx->required_algorithm),
hash, hlen, reformatted_signature, 2*ctx->curve_byte_count);
}
#else /* !CONFIG_BOOT_SIGNATURE_USING_ITS */

#define BOOT_SIGNATURE_BUILTIN_KEY_SLOTS 4
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#define BOOT_SIGNATURE_BUILTIN_KEY_SLOTS 4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

boot/bootutil/include/bootutil/crypto/ecdsa.h Outdated
psa_algorithm_t algorithm;
};

static psa_key_id_t builtin_key_ids[BOOT_SIGNATURE_BUILTIN_KEY_SLOTS] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
static psa_key_id_t builtin_key_ids[BOOT_SIGNATURE_BUILTIN_KEY_SLOTS] = {
static psa_key_id_t builtin_key_ids[] = {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@ahasztag ahasztag force-pushed the NCSDK-34470_ecdsa_with_its branch from 1214847 to 2f2be35 Compare July 24, 2025 07:55
@ahasztag ahasztag marked this pull request as ready for review July 24, 2025 07:56
@NordicBuilder NordicBuilder mentioned this pull request Jul 24, 2025
Open
@ahasztag ahasztag force-pushed the NCSDK-34470_ecdsa_with_its branch from 2f2be35 to 6a24060 Compare July 24, 2025 10:55
@ahasztag ahasztag requested a review from nordicjm July 24, 2025 10:56
@ahasztag ahasztag marked this pull request as draft July 24, 2025 11:27
@ahasztag ahasztag force-pushed the NCSDK-34470_ecdsa_with_its branch 3 times, most recently from 320e0de to 6b51018 Compare July 24, 2025 11:41
@ahasztag ahasztag marked this pull request as ready for review July 24, 2025 11:42
de-nordic
de-nordic requested changes Jul 24, 2025
View reviewed changes
Copy link
Contributor

@de-nordic de-nordic left a comment •
edited
Loading

Choose a reason for hiding this comment

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

image

boot/bootutil/include/bootutil/crypto/ecdsa.h Outdated
BOOT_LOG_ERR("ECDSA signature verification failed %d", status);
}

return (int) status;
Copy link
Contributor

Choose a reason for hiding this comment

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

No. Translate the status to something like 1 or 2, or whatever.
I do not think that PSA codes should leave the bootutil_ecdsa_verify, because they make no sense to MCUboot.
I guess that what we have outside is a mess, but we are sure to understand PSA statuses here, but not sure we do so outside the function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I probably just wanted to return similarly as the normal PSA implementation, now the function is returning 2 on error.

boot/bootutil/include/bootutil/crypto/ecdsa.h Outdated
static psa_key_id_t builtin_key_ids[] = {
0x40022100,
0x40022101,
0x40022102,
0x40022103
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this cause link errors that there is unused static builtin_key_ids defined in all the units that include the ecdsa.h?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be defined within the bootutil_ecdsa_verify.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't cause errors, as it is under CONFIG_BOOT_SIGNATURE_USING_ITS.

I think keeping it outside of the function seems a bit cleaner. I tried to mimic the implementation from CRACEN KMU as much as possible, where the array is kept outside the function. But I do not have any strong feelings about it, I can move it inside of the function if you prefer it.

In the future I'd like to move this to some sort of external hook, but I didn't want to complicate the commit too much currently, as the current version is OK for our current needs and I assume we will want to rework the ECDSA signing code in the near future anyway.

boot/bootutil/include/bootutil/crypto/ecdsa.h Outdated
Comment on lines 497 to 500
struct boot_ecdsa_key_info {
psa_key_id_t key_id;
psa_algorithm_t algorithm;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is this thing used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, it is a leftover from the development process, I have missed this, removed.

@ahasztag
[nrf noup] Added BOOT_SIGNATURE_USING_ITS for ecdsa configuration
8c4a36f 
This configuration has the purpose of using keys provisioned
to the internal trusted storage (ITS). It makes use of the
already existing parts of code for MCUBOOT_BUILTIN_KEY

Signed-off-by: Artur Hadasz <[email protected]>
@ahasztag ahasztag force-pushed the NCSDK-34470_ecdsa_with_its branch from 6b51018 to 8c4a36f Compare July 24, 2025 14:00
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@ahasztag ahasztag requested a review from de-nordic July 24, 2025 14:37
nordic-mik7
nordic-mik7 reviewed Jul 25, 2025
View reviewed changes
boot/zephyr/Kconfig
@@ -422,7 +422,14 @@ config BOOT_KMU_KEYS_REVOCATION
help
Enabling KMU key revocation backend.

if !BOOT_SIGNATURE_USING_KMU
config BOOT_SIGNATURE_USING_ITS
bool "Use KMU stored keys for signature verification"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is 'KMU' here intended ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nordic-mik7 nordic-mik7 nordic-mik7 left review comments

@tomchy tomchy tomchy approved these changes

@nordicjm nordicjm nordicjm approved these changes

@de-nordic de-nordic Awaiting requested review from de-nordic de-nordic is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ahasztag @tomchy @nordicjm @de-nordic @nordic-mik7