sys/psa_crypto: ed25519 private key {ex,im}port by mguetschow · Pull Request #20334 · RIOT-OS/RIOT

mguetschow · 2024-02-02T14:33:56Z

Contribution description

This PR adds support for ed25519 private keys to psa_import_key() and adds the corresponding psa_export_key() function. For public asymmetric keys, psa_export_key() should behave like psa_export_public_key(). The backend implementations are used to derive the public key from the private key passed to psa_import_key().

Note that exporting and importing private keys from and to secure elements is not yet properly handled and will likely break for now. Maybe @Einhornhool could give a hint on where and how to test whether the key is stored on a secure element, to be able to report an error to the user.

Testing procedure

Two tests have been added to tests/sys/psa_crypto and tested successfully with hardware support on the nRF52840dk and using the software implementation on native.

Teufelchen1

Hey hey 🐭

just a short review, so this PR doesn't go stale 😃
I think @Einhornhool is going to review this thoroughly?

sys/psa_crypto/psa_crypto_algorithm_dispatch.c

sys/psa_crypto/include/psa_crypto_algorithm_dispatch.h

tests/sys/psa_crypto/main.c

Einhornhool · 2024-02-21T16:41:04Z

Hi! Thank you for the work!
I will review this more thoroughly, but here's a first thought:

Note that exporting and importing private keys from and to secure elements is not yet properly handled and will likely break for now. Maybe @Einhornhool could give a hint on where and how to test whether the key is stored on a secure element, to be able to report an error to the user.

This can be easily determined by the key location. There is a range of location values reserved for secure elements (either PSA_KEY_LOCATION_PRIMARY_SECURE_ELEMENT or a value between PSA_KEY_LOCATION_SE_MIN and PSA_KEY_LOCATION_SE_MAX). If a key has one of those locations, it is stored on an SE and can't be exported.

Einhornhool

Thank you for the work! I have some questions :)

sys/psa_crypto/psa_crypto.c

sys/psa_crypto/psa_crypto_algorithm_dispatch.c

tests/sys/psa_crypto/main.c

Einhornhool

LGTM

mguetschow · 2024-03-12T11:03:45Z

Note that exporting and importing private keys from and to secure elements is not yet properly handled and will likely break for now. Maybe @Einhornhool could give a hint on where and how to test whether the key is stored on a secure element, to be able to report an error to the user.

This can be easily determined by the key location. There is a range of location values reserved for secure elements (either PSA_KEY_LOCATION_PRIMARY_SECURE_ELEMENT or a value between PSA_KEY_LOCATION_SE_MIN and PSA_KEY_LOCATION_SE_MAX). If a key has one of those locations, it is stored on an SE and can't be exported.

I think this should still be adressed before proceeding to merging. Maybe you could try it out with a secure element and report on the current behavior.

Einhornhool · 2024-03-12T12:57:20Z

Note that exporting and importing private keys from and to secure elements is not yet properly handled and will likely break for now. Maybe @Einhornhool could give a hint on where and how to test whether the key is stored on a secure element, to be able to report an error to the user.

This can be easily determined by the key location. There is a range of location values reserved for secure elements (either PSA_KEY_LOCATION_PRIMARY_SECURE_ELEMENT or a value between PSA_KEY_LOCATION_SE_MIN and PSA_KEY_LOCATION_SE_MAX). If a key has one of those locations, it is stored on an SE and can't be exported.

I think this should still be adressed before proceeding to merging. Maybe you could try it out with a secure element and report on the current behavior.

Damn, sorry, I forgot about this.

psa_import_key returns PSA_ERROR_NOT_SUPPORTED, because the driver does not implement this function, yet, which is the way it's supposed to be (no changes required).

psa_export_key returns PSA_ERROR_INVALID_ARGUMENT, because

RIOT/sys/psa_crypto/psa_key_slot_mgmt/psa_crypto_slot_management.c

Line 479 in 03c05f7

    
           size_t psa_get_key_data_from_key_slot(const psa_key_slot_t *slot, uint8_t **key_data,

returns a nullpointer, which is caught by the psa_builtin_export_key function.
It would be better to check the key location before calling psa_get_key_data_from_key_slot and then returning PSA_ERROR_NOT_SUPPORTED or PSA_ERROR_NOT_PERMITTED.

mguetschow · 2024-03-14T10:43:43Z

Thanks for testing! See this diff for the changes to fix the psa_export_key behavior.

On the way I noticed that I had actually broken the key import in previous fixup commits (I guess that's what the tests are for :P). Fixed that and also the TEST_ASSERT_PSA macro from going into an infinite loop if called within the cleanup part of the code.

May I squash?

riot-ci · 2024-03-14T10:45:56Z

Murdock results

✔️ PASSED

5f08f74 sys/psa_crypto: ed25519 private key {ex,im}port

Success	Failures	Total	Runtime
10065	0	10066	13m:38s

Artifacts

Documentation preview

mguetschow · 2024-03-14T16:42:48Z

Wow that is a difficult one 🤔 I actually expect the other tests/sys/psa_crypto_* tests to fail too because of boards missing in Makefile.ci.

I also found a related issue along the way which is not a regression and therefore out of scope for this PR. Opened #20468 as a follow-up for it.

mguetschow · 2024-03-15T09:23:22Z

I actually expect the other tests/sys/psa_crypto_* tests to fail too because of boards missing in Makefile.ci.

They were not since they symlink to examples/psa_crypto instead of tests/sys/psa_crypto.

@Einhornhool this should now be ready for final review, then I will squash all the fixups together.

Einhornhool · 2024-04-03T14:17:15Z

Thanks for testing! See this diff for the changes to fix the psa_export_key behavior.

On the way I noticed that I had actually broken the key import in previous fixup commits (I guess that's what the tests are for :P). Fixed that and also the TEST_ASSERT_PSA macro from going into an infinite loop if called within the cleanup part of the code.

May I squash?

Here's the new output for psa_export_key with a secure element:

2024-04-03 16:15:05,170 # main(): This is RIOT! (Version: 2024.01-devel-732-gecfcf-HEAD)
2024-04-03 16:15:05,257 # ECDSA took 85735 us
2024-04-03 16:15:05,261 # ECDSA failed: PSA_ERROR_NOT_SUPPORTED
2024-04-03 16:15:05,262 # Tests failed...

So for me this is fine!

mguetschow · 2024-04-03T14:25:42Z

Thanks for the renewed review! I've just squashed and rebased onto current master.

Maybe @Teufelchen1 could have a look at the documentation discussion above and provide a proxy-ack?

sys/psa_crypto: ed25519 private key {ex,im}port

mguetschow requested review from MichelRottleuthner, aabadie and leandrolanzieri as code owners February 2, 2024 14:33

mguetschow requested review from Einhornhool and removed request for MichelRottleuthner, aabadie and leandrolanzieri February 2, 2024 14:34

github-actions bot added Area: tests Area: tests and testing framework Area: pkg Area: External package ports Area: sys Area: System labels Feb 2, 2024

benpicco requested review from PeterKietzmann and leandrolanzieri February 3, 2024 11:10

Teufelchen1 requested changes Feb 6, 2024

View reviewed changes

sys/psa_crypto/psa_crypto_algorithm_dispatch.c Outdated Show resolved Hide resolved

sys/psa_crypto/include/psa_crypto_algorithm_dispatch.h Show resolved Hide resolved

tests/sys/psa_crypto/main.c Outdated Show resolved Hide resolved

Einhornhool reviewed Feb 27, 2024

View reviewed changes

sys/psa_crypto/psa_crypto.c Outdated Show resolved Hide resolved

sys/psa_crypto/psa_crypto.c Show resolved Hide resolved

sys/psa_crypto/psa_crypto_algorithm_dispatch.c Outdated Show resolved Hide resolved

sys/psa_crypto/psa_crypto_algorithm_dispatch.c Outdated Show resolved Hide resolved

Einhornhool suggested changes Feb 27, 2024

View reviewed changes

tests/sys/psa_crypto/main.c Outdated Show resolved Hide resolved

Einhornhool approved these changes Mar 12, 2024

View reviewed changes

mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 14, 2024

mguetschow mentioned this pull request Mar 14, 2024

examples/psa_crypto: key_bits usage doesn't match specification #20468

Closed

mguetschow force-pushed the psa-import-key branch from ecfcf76 to e787d42 Compare April 3, 2024 14:23

Teufelchen1 approved these changes Apr 3, 2024

View reviewed changes

benpicco added this pull request to the merge queue Apr 15, 2024

maribu removed this pull request from the merge queue due to a manual request Apr 16, 2024

github-merge-queue bot pushed a commit that referenced this pull request Apr 16, 2024

Merge pull request #20334 from mguetschow/psa-import-key

f281c00

sys/psa_crypto: ed25519 private key {ex,im}port

maribu added this pull request to the merge queue Apr 16, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 16, 2024

sys/psa_crypto: ed25519 private key {ex,im}port

5f08f74

mguetschow force-pushed the psa-import-key branch from e787d42 to 5f08f74 Compare April 17, 2024 10:42

mguetschow enabled auto-merge April 17, 2024 10:43

mguetschow added this pull request to the merge queue Apr 17, 2024

Merged via the queue into RIOT-OS:master with commit ad9d501 Apr 17, 2024

mguetschow deleted the psa-import-key branch April 18, 2024 06:57

mguetschow added this to the Release 2024.07 milestone Jul 5, 2024

Conversation

mguetschow commented Feb 2, 2024

Contribution description

Testing procedure

Uh oh!

Teufelchen1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Einhornhool commented Feb 21, 2024

Uh oh!

Einhornhool left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Einhornhool left a comment

Choose a reason for hiding this comment

Uh oh!

mguetschow commented Mar 12, 2024

Uh oh!

Einhornhool commented Mar 12, 2024

Uh oh!

mguetschow commented Mar 14, 2024

Uh oh!

riot-ci commented Mar 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Murdock results

Artifacts

Uh oh!

mguetschow commented Mar 14, 2024

Uh oh!

mguetschow commented Mar 15, 2024

Uh oh!

Einhornhool commented Apr 3, 2024

Uh oh!

mguetschow commented Apr 3, 2024

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

riot-ci commented Mar 14, 2024 •

edited

Loading