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

Refactor OpenSSL Implementation of SHA3 SHAKE to use new Squeeze API #1694

Merged
merged 6 commits into from
Apr 7, 2024

Conversation

Eddy-M-K
Copy link
Contributor

Refactor OpenSSL Implementation of SHA3 SHAKE to use new EVP_DigestSqueeze() allowing for multiple calls to squeeze

Resolves #1539

Please correct me if I'm wrong but it looks like n_out of intrn_shake*_inc_ctxs are used solely to keep track of the byte number of the input and are unnecessary with the new Squeeze API. The test_sha3 test ran fine when I removed all instances of n_out so I'm curious if this is no longer necessary.

If n_out is indeed unnecessary, then perhaps we can also remove the wrapper structs intrn_shake128_inc_ctx and intrn_shake256_inc_ctx and instead point state->ctx directly to an instance of EVP_MD_CTX.

  • Does this PR change the input/output behaviour of a cryptographic algorithm (i.e., does it change known answer test values)? (If so, a version bump will be required from x.y.z to x.(y+1).0.)
  • Does this PR change the list of algorithms available -- either adding, removing, or renaming? Does this PR otherwise change an API? (If so, PRs in fully supported downstream projects dependent on these, i.e., oqs-provider and OQS-OpenSSH will also need to be ready for review and merge by the time this is merged.)

@beldmit
Copy link
Contributor

beldmit commented Feb 12, 2024

May I ask you to put this changes under some #ifdef? OpenSSL 3.0/3.1 are still available and these branches don't have the Squeeze API

Copy link
Member

@baentsch baentsch left a comment

Choose a reason for hiding this comment

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

Thanks @Eddy-M-K for putting this PR together. Conceptually this looks OK. The comment explains why this CI test is failing (it enables "OQS_USE_SHA3_OPENSSL" on a downlevel OpenSSL): You may simply wrap the new code in a suitable #ifdef OPENSSL_VERSION_NUMBER >= ... (do you happen to know the exact value that would be right, @beldmit ?)

You may also consider adding "-DOQS_USE_SHA3_OPENSSL=ON" to this CI test which is using 3.0.2 and maybe some other "OpenSSL master" test to test-drive the OpenSSL SHA3 code more regularly (as "OQS_USE_SHA3_OPENSSL" is OFF by default).

@beldmit
Copy link
Contributor

beldmit commented Feb 12, 2024

According to man pages EVP_DigestSqueeze() function was added in OpenSSL 3.3.

@Eddy-M-K
Copy link
Contributor Author

Oh right, completely forgot about backward compatibility with the current version. Thank you @beldmit and @baentsch - will fix this.

@SWilson4
Copy link
Member

SWilson4 commented Feb 13, 2024

You may also consider adding "-DOQS_USE_SHA3_OPENSSL=ON" to this CI test which is using 3.0.2 and maybe some other "OpenSSL master" test to test-drive the OpenSSL SHA3 code more regularly (as "OQS_USE_SHA3_OPENSSL" is OFF by default).

@baentsch Do you happen to know which jobs (if any) use OpenSSL master? It looks to me based on CI logs like all of our GitHub Actions runs are using either 1.1.1 or 3.0.2. If there indeed aren't any, should one be added? Or is that better left for downstream testing with the provider? (i.e., by calling "[trigger downstream]")

@baentsch
Copy link
Member

Do you happen to know which jobs (if any) use OpenSSL master?

Good question! I don't (recall us ever having had that goal), so the answer probably is "None". Until this PR I would have said "so what" as liboqs never used any unreleased openssl features. This PR apparently changes this, so maybe should also introduce openssl "master" tests. But then again, I'd argue against targeting "master" as that may stall liboqs CI for PRs based on things this project does not control. Also "master"-based testing would increase CI time substantially. I'd therefore argue for tests (particularly targeting this code) using a specific openssl commit in the "3.3.0-dev" range -- and replacing that with a tagged "3.3.0" build as and when that is released. This way, the openssl build can be cached in CI and will run fast(er than a "master" build always created from scratch/code).

@Eddy-M-K Eddy-M-K force-pushed the ek-ossl-sha3-squeeze branch from 8482bbd to cef780e Compare March 5, 2024 19:46
@Eddy-M-K Eddy-M-K force-pushed the ek-ossl-sha3-squeeze branch from e9ce69d to f5248ac Compare April 1, 2024 03:40
@SWilson4 SWilson4 marked this pull request as ready for review April 4, 2024 17:17
@SWilson4 SWilson4 requested a review from dstebila as a code owner April 4, 2024 17:17
Copy link
Member

@SWilson4 SWilson4 left a comment

Choose a reason for hiding this comment

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

Looks good to me, with one minor suggestion. Thanks, Eddy!

.github/workflows/unix.yml Outdated Show resolved Hide resolved
@SWilson4 SWilson4 requested a review from baentsch April 4, 2024 17:25
Copy link
Member

@dstebila dstebila left a comment

Choose a reason for hiding this comment

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

Thanks Eddy!

@Eddy-M-K Eddy-M-K force-pushed the ek-ossl-sha3-squeeze branch from c270962 to 64d806d Compare April 5, 2024 04:17
@Eddy-M-K Eddy-M-K requested a review from bhess as a code owner April 5, 2024 04:17
@Eddy-M-K Eddy-M-K force-pushed the ek-ossl-sha3-squeeze branch from 64d806d to c270962 Compare April 5, 2024 04:23
Signed-off-by: Eddy Kim <[email protected]>
Co-authored-by: Spencer Wilson <[email protected]>
@Eddy-M-K Eddy-M-K force-pushed the ek-ossl-sha3-squeeze branch from c270962 to 4d9c4a5 Compare April 5, 2024 04:28
@Eddy-M-K
Copy link
Contributor Author

Eddy-M-K commented Apr 5, 2024

Had to make a couple force pushes due to a failed attempt to sign off the last commit 😰

@baentsch baentsch merged commit cfc41f7 into open-quantum-safe:main Apr 7, 2024
44 checks passed
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.

Update OpenSSL integration
5 participants