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

[mguetschow]

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?

[Einhornhool]

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 :)

[Einhornhool]

LGTM

[mguetschow]

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]

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]

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]

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]

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]

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]

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]

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?