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.

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

Expose SHAKE through the EVP API #1199

Merged
merged 36 commits into from
Oct 9, 2023
Merged

Conversation

WillChilds-Klein
Copy link
Contributor

@WillChilds-Klein WillChilds-Klein commented Sep 19, 2023

Issues

Resolves CryptoAlg-2034

Notes

This commit exposes our extant SHAKE implementation through the EVP digest API and incorporates that implementation into the FIPS service indicator and ACVP tooling. SHAKE differs from traditional digest functions in that it does not have a fixed output digest size. When using the incremental EVP API, the output size is specified on finalization through EVP_DigestFinalXOF. This additional parameter percolates throughout the EVP and test code, as the function signature of EVP_DigestFinalXOF is different from that of EVP_DigestFinal or EVP_DigestFinal_ex.

We also add SHA3 to the self tests. As noted below, while the self-tests don't include actual tests for every approved digest algorithm we support, they do cover the core implementation of every approved digest. SHA224 is covered by the SHA256 self-test; SHA384, SHA512-224, and SHA512-256 are covered by the SHA512 self-test; SHAKE-128, SHAKE-256, and all the SHA3 variants are covered by the SHA3-512 self-test.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.

@WillChilds-Klein WillChilds-Klein changed the title Shake Expose SHAKE through the EVP API Sep 19, 2023
@WillChilds-Klein WillChilds-Klein force-pushed the shake branch 3 times, most recently from d2d298b to 48be6d7 Compare September 20, 2023 20:41
@samuel40791765
Copy link
Contributor

Only reviewed 48be6d7, but hte service indicator changes/tests lgtm 👍

crypto/fipsmodule/digest/internal.h Show resolved Hide resolved
util/fipstools/acvp/acvptool/test/expected/SHAKE-128.bz2 Outdated Show resolved Hide resolved
crypto/fipsmodule/digest/digest.c Outdated Show resolved Hide resolved
crypto/fipsmodule/digest/digest.c Outdated Show resolved Hide resolved
crypto/fipsmodule/sha/internal.h Outdated Show resolved Hide resolved
Copy link
Contributor

@darylmartin100 darylmartin100 left a comment

Choose a reason for hiding this comment

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

Since you are marking this as an approved algorithm, do we need to add a corresponding self test in self_check.c?

@WillChilds-Klein
Copy link
Contributor Author

WillChilds-Klein commented Oct 2, 2023

Since you are marking this as an approved algorithm, do we need to add a corresponding self test in self_check.c?

@darylmartin100 --That's a good question. Right now, we're missing coverage in self_check.c for the following approved hash algorithms:

  • SHA2: SHA224, SHA384, SHA512-224, SHA512-256
  • SHA3: SHA3-224, SHA3-256, SHA3-384, SHA3-512
  • SHAKE: SHAKE-128, SHAKE-256

I'm not sure what the decision criterion is for adding an algorithm to self_check.c, but it seems like if we add SHAKE, we should add the other missing ones as well.

@dkostic
Copy link
Contributor

dkostic commented Oct 2, 2023

one general question: can the finalXOF function be called more than once on the same context object and what's the expected behavior in that case?

@WillChilds-Klein
Copy link
Contributor Author

one general question: can the finalXOF function be called more than once on the same context object and what's the expected behavior in that case?

@dkostic -- I don't think we should allow that (neither does OpenSSL). For regular digests, we technically allow multiple calls to EVP_DigestFinal_ex, but advise against it in our documentation. EVP_DigestFinal cleans up the ctx, so it can't be called again without re-initialization.

I've added a test case to digest_test.cc to assert this behavior.

crypto/fipsmodule/digest/digest.c Outdated Show resolved Hide resolved
crypto/fipsmodule/digest/digest.c Outdated Show resolved Hide resolved
@WillChilds-Klein WillChilds-Klein enabled auto-merge (squash) October 9, 2023 21:47
@WillChilds-Klein WillChilds-Klein merged commit bb1aaba into aws:main Oct 9, 2023
4 of 5 checks passed
@WillChilds-Klein WillChilds-Klein deleted the shake branch October 9, 2023 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants