-
Notifications
You must be signed in to change notification settings - Fork 2.1k
sys: PSA Crypto API implementation #18547
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.
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
Conversation
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 |
24395e3
to
03407f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it would be nice to add a simple automatic test to the CI can test it as well?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -21,6 +21,7 @@ | |||
#define CFG_I2C_DEFAULT_H | |||
|
|||
#include "periph_cpu.h" | |||
#include "kernel_defines.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are the "kernel_defines.h" needed in this header? (cant see anything being added that requires them)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
resolved in 18582
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No resolved here... Please remove
I think I'm done. |
a3f7d59
to
17da706
Compare
bors try |
just to see everything :) |
tryBuild failed: |
Just one ATmega being too small for this :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If everything works I would give it my ACK. It is just a matter of @Teufelchen1 to finish checking the changes!
17da706
to
a0ccbce
Compare
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 !! |
🎉 |
19954: sys/psa_crypto: Ed25519 (EdDSA) support r=benpicco a=mguetschow ### Contribution description - implement [`psa_sign_message()`](https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.psa_sign_message) and [`psa_verify_message()`](https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.psa_verify_message) for the two already supported [`PSA_ALG_ECDSA`](https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.PSA_ALG_ECDSA) algorithms, together with the CryptoCell and `micro-ecc` backends (*not* for the SE backend) - add support for [`PSA_ALG_PURE_EDDSA`](https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.PSA_ALG_PURE_EDDSA), together with the CryptoCell hardware and `c25519` software backend (*not* for the SE backend) - wipe private key data from stack for both ECDSA and EdDSA algorithms using `explicit_bzero()` (opinions from experienced Riot maintainers about usage of `goto` to avoid duplicating that function call before every `return`?) ### Testing procedure - `examples/psa_crypto` has been updated to include EdDSA - successfully tested configurations: - `nrf52840dk` with cryptocell (hardware) and `c25519` (software) backend - `native` with software backend ### Issues/PRs references Thanks `@Einhornhool` for the PSA Crypto framework implementation #18547 which is great to work with! 19966: sys/event: add event_is_queued() r=benpicco a=fabian18 Co-authored-by: Mikolai Gütschow <mikolai.guetschow@tu-dresden.de> Co-authored-by: Fabian Hüßler <fabian.huessler@ml-pa.com>
19954: sys/psa_crypto: Ed25519 (EdDSA) support r=miri64 a=mguetschow ### Contribution description - implement [`psa_sign_message()`](https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.psa_sign_message) and [`psa_verify_message()`](https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.psa_verify_message) for the two already supported [`PSA_ALG_ECDSA`](https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.PSA_ALG_ECDSA) algorithms, together with the CryptoCell and `micro-ecc` backends (*not* for the SE backend) - add support for [`PSA_ALG_PURE_EDDSA`](https://armmbed.github.io/mbed-crypto/html/api/ops/sign.html#c.PSA_ALG_PURE_EDDSA), together with the CryptoCell hardware and `c25519` software backend (*not* for the SE backend) - wipe private key data from stack for both ECDSA and EdDSA algorithms using `explicit_bzero()` (opinions from experienced Riot maintainers about usage of `goto` to avoid duplicating that function call before every `return`?) ### Testing procedure - `examples/psa_crypto` has been updated to include EdDSA - successfully tested configurations: - `nrf52840dk` with cryptocell (hardware) and `c25519` (software) backend - `native` with software backend ### Issues/PRs references Thanks `@Einhornhool` for the PSA Crypto framework implementation #18547 which is great to work with! Co-authored-by: Mikolai Gütschow <mikolai.guetschow@tu-dresden.de>
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_crypto
to demonstrate the usage and configuration of different backends of the API (refer to the application README for more information).