liboqs icon indicating copy to clipboard operation
liboqs copied to clipboard

Refactor OpenSSL Implementation of SHA3 SHAKE to use new Squeeze API

Open Eddy-M-K opened this issue 1 year ago • 5 comments

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.)

Eddy-M-K avatar Feb 12 '24 05:02 Eddy-M-K

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

beldmit avatar Feb 12 '24 06:02 beldmit

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

beldmit avatar Feb 12 '24 11:02 beldmit

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

Eddy-M-K avatar Feb 13 '24 18:02 Eddy-M-K

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]")

SWilson4 avatar Feb 13 '24 22:02 SWilson4

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).

baentsch avatar Feb 14 '24 06:02 baentsch

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

Eddy-M-K avatar Apr 05 '24 04:04 Eddy-M-K