sys: PSA Crypto API implementation#18547
Conversation
Einhornhool
requested review from
MrKevinWeiss,
PeterKietzmann,
aabadie,
haukepetersen,
jia200x and
leandrolanzieri
as code owners
github-actions
bot
added
Area: boards
|
Nice work! But a single 20kSLOC commit is a bit hard to handle - can you please split this up into more manageable chunks? Ideally even multiple PRs since there seem to be several things in there that could be merged independently. |
|
Thank you! |
|
You can of course do this in whatever time scale suits you best, but splitting this into multiple commits would be a good first step independent of whether the PR should be split or not. |
|
Alright, I will do that |
Einhornhool
force-pushed
the
pr/psa-crypto-implementation
branch
from
24395e3 to
03407f8
Compare
maribu
left a comment
maribu
left a comment
There was a problem hiding this comment.
Lots of style nitpicks. I didn't look at all and event failed to grasp the high level picture yet, as it is really hard to read code on a mobile phone :( I will provide better feadback one I am back at a keyboard.
MrKevinWeiss
left a comment
MrKevinWeiss
left a comment
There was a problem hiding this comment.
Maybe it would be nice to add a simple automatic test to the CI can test it as well?
Details
diff --git a/examples/psa_crypto/tests/01-run.py b/examples/psa_crypto/tests/01-run.py
new file mode 100755
index 0000000000..25257b8ca8
--- /dev/null
+++ b/examples/psa_crypto/tests/01-run.py
@@ -0,0 +1,13 @@
+#!/usr/bin/env python3
+
+import sys
+from testrunner import run
+
+
+def testfunc(child):
+ child.expect_exact('All Done')
+ print("[TEST PASSED]")
+
+
+if __name__ == "__main__":
+ sys.exit(run(testfunc))
pkg/driver_cryptocell/Makefile
Outdated
| @@ -0,0 +1,14 @@ | |||
| PKG_NAME=driver_cryptocell | |||
| PKG_URL=https://github.com/Einhornhool/RIOT-cryptocell-driver.git | |||
There was a problem hiding this comment.
This package points to an archive (.a) only, with headers repository. That's quite limited: only nrf52840 cpu is supported. Is there a possibility and a plan to provide an open-source implementation ?
Also note that the package is too much limited IMHO: it can only be built for nrf52840dk board but I think any nrf52840 CPU based board should work with it.
There was a problem hiding this comment.
Unfortunately this driver is closed source and there don't seem to be any plans to make it open source.
I got this from the Nordic SDK, where they only provide the archives and header files. There is a Git Repositiory (nrfxlib), which also provides archive files, but only headers with SHA 256 and ECC P256 support.
The limitation is my mistake, I did not realize that the nrf52840 also had an accelerator, I will fix that.
github-actions
bot
added
Area: cpu
kfessel
left a comment
kfessel
left a comment
There was a problem hiding this comment.
wow this is a great PR
since you already said you want to split it, I just had a first glance and found some small nit picks.
| #define CFG_I2C_DEFAULT_H | ||
|
|
||
| #include "periph_cpu.h" | ||
| #include "kernel_defines.h" |
There was a problem hiding this comment.
are the "kernel_defines.h" needed in this header? (cant see anything being added that requires them)
There was a problem hiding this comment.
No resolved here... Please remove
|
bors try |
tryBuild failed: |
|
I don't why codespell isn't picking it up the ignore. I did notice that in the docker static tests, our requirements are specifying codespell as a minimum, not an exact. This means, if codespell updates, new, unrelated errors may occur. |
|
All other words in that ignore list are spelled lower case (even if the target word is upper case). Maybe that is the problem? |
|
tested locally... That was it. lowercase please :) |
|
Now that things are good do you have any final remarks @Teufelchen1 ? |
|
There are still around 5ish recent (> 01.01.23) conversations unresolved. They seem to be minor, I would like to see them fixed before merging. Also, take a look at murdock? Missing stdio.h something something |
|
I think I'm done. |
|
bors try |
|
just to see everything :) |
tryBuild failed: |
|
Just one ATmega being too small for this :-) |
MrKevinWeiss
left a comment
MrKevinWeiss
left a comment
There was a problem hiding this comment.
If everything works I would give it my ACK. It is just a matter of @Teufelchen1 to finish checking the changes!
|
If everyone is happy then. |
|
bors merge |
|
🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set. |
|
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
|
Congrats @Einhornhool !! |
|
🎉 |
13 participants
Contribution description
This adds an implementation of the ARM PSA Crypto API specification to RIOT.
It is a cryptographic API that supports software and hardware backends as well as the use of multiple secure elements, which can be configured with Kconfig.
It integrates indirect, identifier based key management to support persistent storage of key material in local memory and devices with protected key storage.
A description of the implementation design and an evaluation of the processing time and memory overhead in RIOT has been published here: Usable Security for an IoT OS: Integrating the Zoo of Embedded Crypto Components Below a Common API
Implementation status
So far this implementation supports the following operations:
The following backends are supported so far:
Other operations and backends as well as persistent key storage can and will be implemented by me and anyone who wants to contribute in the future.
Testing procedure
So far there is a show case application in
examples/psa_cryptoto demonstrate the usage and configuration of different backends of the API (refer to the application README for more information).