sys/psa_crypto: sha3 support#20698
Conversation
github-actions
bot
added
Area: doc
Wunderbaeumchen99817
requested review from
MichelRottleuthner,
aabadie and
leandrolanzieri
as code owners
|
Cool! Could you clean up your commits? |
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
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/psa_crypto/include/psa_hashes.h
Outdated
| #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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 :)
mguetschow
added
the
CI: ready for build
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
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).
Wunderbaeumchen99817
force-pushed
the
sha3_implementation
branch
from
5b947ca to
d1b8c2e
Compare
Wunderbaeumchen99817
requested review from
MrKevinWeiss,
PeterKietzmann,
benpicco,
biboc,
dylad,
jia200x,
kaspar030,
maribu,
miri64 and
vincent-d
as code owners
github-actions
bot
removed
Area: pkg
Wunderbaeumchen99817
force-pushed
the
sha3_implementation
branch
from
bee93a7 to
d27ff50
Compare
mguetschow
removed request for
Einhornhool,
MichelRottleuthner,
dylad and
gschorcht
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
@Wunderbaeumchen99817 sorry for the back and forth, but could you try to split the three parts in separate (pseudo)modules and add your changes as separate commits for easier review?
|
There are still two failing static checks that you should be able to fix (a trailing whitespace and running I'm not sure why the third check failed, though. |
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
Looks mostly good, just some more minor nitpicks :)
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
Looks good to me now, please squash @Wunderbaeumchen99817 :)
Wunderbaeumchen99817
force-pushed
the
sha3_implementation
branch
from
f56cc83 to
7a4d388
Compare
github-actions
bot
added
Platform: native
|
@mguetschow done :) |
7 participants
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:
Issues/PRs references
N/A