Skip to content
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.

Sign up for GitHub

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

Jump to bottom

sys/psa_crypto: sha3 support #20698

Merged
merged 1 commit into from
Jul 15, 2024
Merged

sys/psa_crypto: sha3 support #20698

merged 1 commit into from
Jul 15, 2024

Conversation

Wunderbaeumchen99817
Copy link
Contributor

@Wunderbaeumchen99817 Wunderbaeumchen99817 commented May 27, 2024 •
edited
Loading

Contribution description
This PR adds a new pseudomodule for using the existing sha3-hashings-algorithms in a simpler way.
More specific: It adds gluecode for the PSA Crypto API to access the RIOT implementation of SHA3.

Testing procedure
The corresponding tests are located in tests/sys/psa_crypto_hashes/example_sha3_glue.c

You can run them the following way:

  1. go to tests/sys/psa_crypto_hashes/
  2. run "make compile-commands"
  3. run "make flash test"

Issues/PRs references
N/A

@github-actions github-actions bot added Area: doc Area: Documentation Area: tests Area: tests and testing framework Area: build system Area: Build system Area: sys Area: System Area: examples Area: Example Applications labels May 27, 2024
@Wunderbaeumchen99817 Wunderbaeumchen99817 marked this pull request as ready for review May 27, 2024 10:45
@mguetschow mguetschow changed the title Sha3 implementation sys/psa_crypto: sha3 support May 27, 2024
@Teufelchen1
Copy link
Contributor

Cool! Could you clean up your commits?

mguetschow
mguetschow requested changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! 🎉

Some rather minor suggestions for improvement below. Please add them first in separate commits and only squash/rebase after we've had a look at the new changes.

sys/hashes/psa_riot_hashes/sha_3.c Outdated Show resolved Hide resolved
sys/include/hashes/psa/riot_hashes.h Outdated Show resolved Hide resolved
sys/psa_crypto/include/psa_hashes.h Outdated
Comment on lines 344 to 352
#if IS_USED(MODULE_PSA_HASH_SHA_3) || defined(DOXYGEN)
/**
* @brief Low level wrapper function to call a driver for a SHA3-256 hash setup
* See @ref psa_hash_setup()
*
* @param ctx
* @return psa_status_t
*/
psa_status_t psa_hashes_sha3_256_setup(psa_hashes_sha3_ctx_t *ctx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have different psa modules for SHA3-256/384/512 or is the increase in code size if including all three per default negligible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mguetschow The increase in code-size is indeed negligible since they are all based on the same keccak-code (which is the biggest part of the underlying code), which needs to be compiled anyway with every variant of the sha3 algorithm.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, then it sounds reasonable to me to keep it as is. But maybe @Einhornhool has a different opinion and would argue in favor of consistency with the SHA-2 PSA modules?

Copy link
Contributor

@Einhornhool Einhornhool Jun 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, for consistency it would be better to separate them. BUT we separated the SHA-2 modules for the purpose of being modular, since some hardware backends we saw only supported SHA-256 and we wanted to be able to build software backends for the others.
As far as I can see there's only one implementation for SHA-3 available (RIOT Hashes) which supports all three variants. So there's no need to build two different backends.

For me it would be totally fine to keep them together for now and only separate them once we integrate a backend that does not support all of them.
It would be good to document this, though.

Update: Just checked again and there are some other libraries with SHA-3 support. HACL also supports 256/384/512 and Cifra additionally supports 224. I don't see a reason to mix different software backends, though, so I think separation is only needed once we get a hardware accelerator that only supports a subset of them.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think separation is only needed once we get a hardware accelerator that only supports a subset of them.

but that would be an API change then

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered this and I agree. This would be an API change, which makes it more prone to errors.
Also it would require people who add drivers in the future to not only add wrappers, but also to change the module structure.
That makes it less friendly to develop.

I'm sorry for changing my mind now, but I think it would be better to keep the structure consistent with the other modules :)

tests/sys/psa_crypto_hashes/main.c Outdated Show resolved Hide resolved
tests/sys/psa_crypto_hashes/main.c Outdated Show resolved Hide resolved
tests/sys/psa_crypto_hashes/main.c Outdated Show resolved Hide resolved
tests/sys/psa_crypto_hashes/sha3.c Outdated Show resolved Hide resolved
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 4, 2024
@riot-ci
Copy link

riot-ci commented Jun 4, 2024 •
edited
Loading

Murdock results

✔️ PASSED

9b50202 sys/psa_crypto: added sha3 glue code

Success Failures Total Runtime
10177 0 10178 16m:40s

Artifacts

mguetschow
mguetschow approved these changes Jun 4, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your fixups, I'm now happy with the changeset! You may rebase and squash everything together in one or two commits (in case you want to split implementation + test in separate commits).

@github-actions github-actions bot added Area: tools Area: Supplementary tools Area: boards Area: Board ports Area: SAUL Area: Sensor/Actuator Uber Layer Area: CoAP Area: Constrained Application Protocol implementations Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration labels Jul 14, 2024
@github-actions github-actions bot removed Platform: native Platform: This PR/issue effects the native platform Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: network Area: Networking Area: core Area: RIOT kernel. Handle PRs marked with this with care! Area: pkg Area: External package ports Area: drivers Area: Device drivers Area: CI Area: Continuous Integration of RIOT components Area: timers Area: timer subsystems Area: tools Area: Supplementary tools Area: boards Area: Board ports Area: SAUL Area: Sensor/Actuator Uber Layer Area: CoAP Area: Constrained Application Protocol implementations Area: cpu Area: CPU/MCU ports Area: examples Area: Example Applications Area: Kconfig Area: Kconfig integration labels Jul 14, 2024
@Wunderbaeumchen99817
Copy link
Contributor Author

@mguetschow done :)

mguetschow
mguetschow approved these changes Jul 15, 2024
View reviewed changes
Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, nice work!

@mguetschow mguetschow enabled auto-merge July 15, 2024 10:31
@mguetschow mguetschow added this pull request to the merge queue Jul 15, 2024
Merged via the queue into RIOT-OS:master with commit 9ca9696 Jul 15, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@darthdrannel darthdrannel darthdrannel left review comments

@Einhornhool Einhornhool Einhornhool left review comments

@Teufelchen1 Teufelchen1 Teufelchen1 left review comments

@mguetschow mguetschow mguetschow approved these changes

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@kaspar030 kaspar030 Awaiting requested review from kaspar030

@jia200x jia200x Awaiting requested review from jia200x

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss

@miri64 miri64 Awaiting requested review from miri64

@PeterKietzmann PeterKietzmann Awaiting requested review from PeterKietzmann

@maribu maribu Awaiting requested review from maribu

@benpicco benpicco Awaiting requested review from benpicco

@vincent-d vincent-d Awaiting requested review from vincent-d

@biboc biboc Awaiting requested review from biboc

@keestux keestux Awaiting requested review from keestux

@bergzand bergzand Awaiting requested review from bergzand

@kYc0o kYc0o Awaiting requested review from kYc0o

@chrysn chrysn Awaiting requested review from chrysn

Assignees
No one assigned
Labels
Area: build system Area: Build system Area: doc Area: Documentation Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2024.07
Development

Successfully merging this pull request may close these issues.
7 participants
@Wunderbaeumchen99817 @Teufelchen1 @riot-ci @mguetschow @benpicco @Einhornhool @darthdrannel