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.

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

nrf_security: drivers: cracen: adding support for ed25519 without sicrypto #19812

Merged
merged 1 commit into from
Feb 20, 2025
Merged

nrf_security: drivers: cracen: adding support for ed25519 without sicrypto #19812

merged 1 commit into from
Feb 20, 2025

Conversation

degjorva
Copy link
Contributor

@degjorva degjorva commented Jan 9, 2025 •
edited
Loading

Added implementation in cracenpsa of ed25519 and ed25519ph without using sicrypto
Removed the possibility of using the sicrypto implementation of ed25519 and ed25519ph through cracenpsa
Updated cracenpsa to support new implementation and
remove references to old

@degjorva degjorva added the DNM label Jan 9, 2025
@degjorva degjorva requested review from a team as code owners January 9, 2025 09:02
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 9, 2025
@degjorva degjorva removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 9, 2025
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Jan 9, 2025 •
edited
Loading

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 78

Inputs:

Sources:

sdk-nrf: PR head: db636a58569784f6ed4d7c7e21bad1f5be2fff0c

more details

sdk-nrf:

PR head: db636a58569784f6ed4d7c7e21bad1f5be2fff0c
merge base: 6d3f9f4257a34b9d6993ea267c256f8b24be8f9d
target head (main): 87fef230aa647b5dec2ad7770ef04e0c43f374bf
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (5) 
subsys
│  ├── nrf_security
│  │  ├── src
│  │  │  ├── drivers
│  │  │  │  ├── cracen
│  │  │  │  │  ├── cracenpsa
│  │  │  │  │  │  ├── cracenpsa.cmake
│  │  │  │  │  │  ├── include
│  │  │  │  │  │  │  │ cracen_psa.h
│  │  │  │  │  │  ├── src
│  │  │  │  │  │  │  ├── ed25519.c
│  │  │  │  │  │  │  ├── key_management.c
│  │  │  │  │  │  │  │ sign.c

Outputs:

Toolchain

Version: 342151af73
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:342151af73_bbe5b33786

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister
    • sdk-nrf test count: 1632
  • ✅ Integration tests
    • ✅ test-fw-nrfconnect-chip
    • ✅ test-fw-nrfconnect-nrf_crypto
    • ✅ test-fw-nrfconnect-tfm
    • ✅ test-sdk-find-my
    • ✅ test-sdk-sidewalk
    • ✅ test-sdk-dfu
    • ⚠️ test-fw-nrfconnect-nrf-iot_cloud
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test_ble_nrf_config
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-ble_samples
    • test-fw-nrfconnect-boot
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-proprietary_esb
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-thread
    • test-fw-nrfconnect-zigbee
    • test-low-level
    • test-sdk-audio
    • test-sdk-mcuboot
    • test-sdk-pmic-samples
    • test-sdk-wifi
    • test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publish GitHub Action.

@degjorva degjorva force-pushed the no-sicrypto-ED25519 branch from 0c0a5d5 to 933f5a8 Compare January 9, 2025 13:36
@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Jan 9, 2025
@degjorva degjorva force-pushed the no-sicrypto-ED25519 branch 3 times, most recently from 21feb07 to e54e7e5 Compare January 13, 2025 14:12
nordicjm
nordicjm requested changes Jan 13, 2025
View reviewed changes
subsys/nrf_security/src/drivers/cracen/cracenpsa/src/key_management.c Outdated
@@ -605,8 +608,10 @@ static psa_status_t export_ecc_public_key_from_keypair(const psa_key_attributes_
int si_status = 0;
psa_algorithm_t key_alg = psa_get_key_algorithm(attributes);
const struct sx_pk_ecurve *sx_curve;
#if CONFIG_PSA_NEED_NO_SI_CRYPTO_ED25519
Copy link
Contributor

Choose a reason for hiding this comment

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

as above

@degjorva degjorva force-pushed the no-sicrypto-ED25519 branch 13 times, most recently from 718033c to 66dcd8a Compare January 17, 2025 09:37
@degjorva degjorva force-pushed the no-sicrypto-ED25519 branch 2 times, most recently from 758768d to 0ace2dc Compare January 21, 2025 13:57
@degjorva degjorva requested a review from tomi-font February 13, 2025 09:00
@degjorva degjorva force-pushed the no-sicrypto-ED25519 branch 3 times, most recently from 4aee94c to c8b7ced Compare February 13, 2025 09:13
tomi-font
tomi-font requested changes Feb 13, 2025
View reviewed changes
nordicjm
nordicjm reviewed Feb 13, 2025
View reviewed changes
subsys/nrf_security/src/drivers/cracen/cracenpsa/include/cracen_psa.h Outdated
Comment on lines 367 to 374
size_t message_length);

int cracen_ed25519_verify(const uint8_t *pubkey, const char *message, size_t message_length,
const char *signature);

int cracen_ed25519ph_sign(const uint8_t *ed25519, char *signature, const uint8_t *message,
size_t message_length, int ismessage);

int cracen_ed25519ph_verify(const uint8_t *pubkey, const char *message, size_t message_length,
const char *signature, int ismessage);
Copy link
Contributor

Choose a reason for hiding this comment

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

alignment on second lines is wrong

@degjorva degjorva force-pushed the no-sicrypto-ED25519 branch from c8b7ced to e3cfb62 Compare February 13, 2025 12:45
Vge0rge
Vge0rge reviewed Feb 13, 2025
View reviewed changes
subsys/nrf_security/src/drivers/cracen/cracenpsa/src/sign.c Outdated
&privkey->hashalg, alg, digestsz);
}
}
if (alg == PSA_ALG_PURE_EDDSA || alg == PSA_ALG_ED25519PH) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was that intentional to remove the IS_ENABLED statement which requires the relevant PSA_NEED to be enabled here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but actually this statement is not needed either anymore. cracen_signature_prepare_ec_prvkey is never called while using eddsa, as cracen_signature_prepare_ec_prvkey is a support function for sicrypto

subsys/nrf_security/src/drivers/cracen/cracenpsa/src/ed25519.c
if (status != SX_OK)
return status;

memcpy(pub_key, pub_key_A, SX_ED25519_SZ);
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to clear the scalar stored in the first part of the digest buffer here?

Maybe the workbuf in the other functions is also worth considering to clear before exiting 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.

That does make sense

@degjorva degjorva force-pushed the no-sicrypto-ED25519 branch 3 times, most recently from 9c701f7 to f9ea2ff Compare February 13, 2025 14:09
@degjorva degjorva force-pushed the no-sicrypto-ED25519 branch 2 times, most recently from a55a562 to 833596b Compare February 13, 2025 14:59
tomi-font
tomi-font requested changes Feb 14, 2025
View reviewed changes
@degjorva degjorva force-pushed the no-sicrypto-ED25519 branch 4 times, most recently from fb96622 to 6ca35d7 Compare February 14, 2025 14:18
@degjorva degjorva requested a review from tomi-font February 14, 2025 14:20
@degjorva degjorva force-pushed the no-sicrypto-ED25519 branch from 6ca35d7 to c3ab354 Compare February 14, 2025 14:38
@degjorva
nrf_security: drivers: cracen: Implement Ed25519 in cracenpsa
db636a5 
Add support for Ed25519 and Ed25519ph in cracenpsa
directly using silexpk/sxsymcrypt.
This bypasses sicrypto, which saves on flash usage
Remove sicrypto implementation of Ed25519 from
being accessible from cracenpsa.

Signed-off-by: Dag Erik Gjørvad <[email protected]>
@degjorva degjorva force-pushed the no-sicrypto-ED25519 branch from c3ab354 to db636a5 Compare February 14, 2025 14:42
tomi-font
tomi-font approved these changes Feb 20, 2025
View reviewed changes
Copy link
Contributor

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

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

Nice! Just #19812 (comment) remains which you could address if you push to this PR at some point but it can go in without that.

@degjorva
Copy link
Contributor Author

@nordicjm Please review when you have the chance

@nordicjm nordicjm merged commit 8f5b058 into nrfconnect:main Feb 20, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomi-font tomi-font tomi-font approved these changes

@Vge0rge Vge0rge Vge0rge approved these changes

@nordicjm nordicjm nordicjm approved these changes

Assignees
No one assigned
Labels
changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@degjorva @NordicBuilder @Vge0rge @nordicjm @tomi-font