sys/psa_crypto: Implement persistent key storage by Einhornhool · Pull Request #20099 · RIOT-OS/RIOT

Einhornhool · 2023-11-21T15:28:26Z

Contribution description

This is an implementation of persistent key storage in PSA Crypto. It uses VFS with littlefs2 and MTD.
PSA keys are encoded in CBOR, written to files and stored in flash (or emulated flash, depending on the MTD implementation).

So far this works on native and the nRF52840dk and it requires that the board supports MTD. This is why it is optional and must be enabled explicitly when building PSA Crypto.

Testing procedure

tests/sys/psa_crypto_persistent_storage and tests/sys/psa_crypto_cbor_encoder should pass successfully on the supported platforms.
examples/psa_crypto should still build and run without problems.

Issues/PRs references

Probably needs to be updated once #19992 is merged, since this does not yet include the module separation.

mguetschow

Nice job, thank you! 🎉

I've finally taken the time to do a partial review (excluding the test applications and without actually running the code). A few first comments below.

sys/psa_crypto/psa_crypto.c

sys/psa_crypto/psa_key_slot_mgmt/psa_crypto_slot_management.c

sys/psa_crypto/psa_persistent_storage/psa_crypto_cbor_encoder.c

sys/psa_crypto/psa_persistent_storage/psa_crypto_persistent_storage.c

mguetschow

Some more comments below on the tests. Thanks again for your work!

tests/sys/psa_crypto_cbor_encoder/test_psa_crypto_enc_dec_keypair.c

tests/sys/psa_crypto_cbor_encoder/tests_psa_cbor_enc_dec.h

tests/sys/psa_crypto_persistent_storage/Makefile

tests/sys/psa_crypto_persistent_storage/tests_psa_persistent_storage.h

tests/sys/psa_crypto_persistent_storage/test_psa_crypto_store_persistent_single_key.c

tests/sys/psa_crypto_persistent_storage/test_psa_crypto_store_persistent_asym_keypair.c

Einhornhool · 2024-01-03T16:13:54Z

@mguetschow thank you for the nice and detailed feedback! I didn't finish everything today, but I'm working on it =)

mguetschow · 2024-01-05T09:57:20Z

Two more things I thought about:

The keys are stored in plain-text on the (external) flash memory and could be easily read out by anyone getting their hands on the flash module. Securing this is out of scope for this PR, I'd say, but at least it should be mentioned in the documentation (maybe in a warning box?).
CONFIG_PSA_SINGLE_KEY_COUNT and friends only refers to in-memory key slots, this PR supports a virtually unlimited number of (additional) persistent keys, right? Maybe that should be explicitly mentioned in the docs, too.

mguetschow

Thanks for adapting to my feedback. The CBOR format and code is much more concise now, I like!

Second round of review below. It's quite an extensive PR, so I'd expect some more.

Just a small comment: Could you maybe avoid rebasing before pushing the changes so that Github can show me a nice diff of the actual new changes?

sys/psa_crypto/doc.txt

sys/psa_crypto/include/psa_crypto_cbor_encoder.h

sys/psa_crypto/include/psa_crypto_persistent_storage.h

sys/psa_crypto/psa_persistent_storage/psa_crypto_cbor_encoder.c

tests/sys/psa_crypto_cbor_encoder/Makefile

tests/sys/psa_crypto_persistent_storage/test_psa_crypto_store_persistent_asym_keypair.c

sys/psa_crypto/psa_key_slot_mgmt/psa_crypto_slot_management.c

tests/sys/psa_crypto_cbor_encoder/test_psa_crypto_enc_dec_keypair.c

Einhornhool · 2024-01-23T09:51:50Z

Just a small comment: Could you maybe avoid rebasing before pushing the changes so that Github can show me a nice diff of the actual new changes?

Sorry, I did a rebase, because the first version of this PR was still based on the code before the changes we made in #19992 and I wanted to have most current version. But I will consider this in the future =)

riot-ci · 2024-03-14T10:48:50Z

Murdock results

✔️ PASSED

cbadc4f sys/psa_crypto: implement persistent key storage

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

Artifacts

Documentation preview

mguetschow

~~I'm sorry that I didn't think of this before, but keys marked as persistent should really be stored to flash unconditionally, right?~~

Edit: Alright, scratch that, just had another look and found the call to persist the key in psa_finish_key_creation.

sys/psa_crypto/psa_persistent_storage/psa_crypto_persistent_storage.c

sys/psa_crypto/doc.txt

mguetschow

Thanks again for your work! LGTM :)

@Teufelchen1 are we allowed to merge this for the next release? 👉👈

mguetschow · 2024-04-17T11:35:14Z

Great, congrats! 🎉

Ollrogge · 2024-05-02T14:19:58Z

Hey, short question: Why does this only work on the nrf52840dk and native ?

mguetschow · 2024-05-03T11:35:29Z

I think™ the implementation should work on all boards supporting MTD, but has only been tested on native and nrf52840dk for now. @Einhornhool could correct me if I'm wrong, and you should be able to test it on a suitable board using USEMODULE += psa_persistent_storage.

Einhornhool · 2024-05-03T11:48:21Z

Hey, short question: Why does this only work on the nrf52840dk and native ?

As Mikolai said, it should work with Boards that support MTD. I just haven't tested it with all of them and can't guarantee that it works.
Of course you can try it.

Einhornhool requested review from MichelRottleuthner, MrKevinWeiss, aabadie, fjmolinas, jia200x, leandrolanzieri and smlng as code owners November 21, 2023 15:28

github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: sys Area: System Area: Kconfig Area: Kconfig integration labels Nov 21, 2023

Einhornhool force-pushed the feature/psa-crypto-persistent-storage branch 2 times, most recently from 4003957 to 29f2020 Compare November 21, 2023 15:57

mguetschow requested changes Dec 22, 2023

View reviewed changes

mguetschow requested changes Jan 2, 2024

View reviewed changes

Einhornhool force-pushed the feature/psa-crypto-persistent-storage branch from 29f2020 to 74f2b9a Compare January 9, 2024 15:17

mguetschow requested changes Jan 18, 2024

View reviewed changes

Einhornhool mentioned this pull request Jan 30, 2024

Change name of psa_key_slot_get_slot_number to be more clear #20316

Open

Einhornhool force-pushed the feature/psa-crypto-persistent-storage branch from 472e07f to bf6a943 Compare February 21, 2024 16:12

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 requested changes Mar 15, 2024

View reviewed changes

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

sys/psa_crypto/doc.txt Show resolved Hide resolved

Einhornhool force-pushed the feature/psa-crypto-persistent-storage branch from bf6a943 to 9b4228d Compare April 3, 2024 14:32

github-actions bot removed the Area: Kconfig Area: Kconfig integration label Apr 3, 2024

mguetschow approved these changes Apr 3, 2024

View reviewed changes

Teufelchen1 enabled auto-merge April 3, 2024 15:00

auto-merge was automatically disabled April 10, 2024 10:00
Head branch was pushed to by a user without write access

Einhornhool force-pushed the feature/psa-crypto-persistent-storage branch 2 times, most recently from 07faa14 to 9a1255b Compare April 10, 2024 15:25

sys/psa_crypto: implement persistent key storage

cbadc4f

Einhornhool force-pushed the feature/psa-crypto-persistent-storage branch from 9a1255b to cbadc4f Compare April 16, 2024 09:05

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

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

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

Conversation

Einhornhool commented Nov 21, 2023

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Einhornhool commented Jan 3, 2024

Uh oh!

mguetschow commented Jan 5, 2024

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Einhornhool commented Jan 23, 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 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mguetschow left a comment

Choose a reason for hiding this comment

Uh oh!

mguetschow commented Apr 17, 2024

Uh oh!

Ollrogge commented May 2, 2024

Uh oh!

mguetschow commented May 3, 2024

Uh oh!

Einhornhool commented May 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

riot-ci commented Mar 14, 2024 •

edited

Loading

mguetschow left a comment •

edited

Loading