-
Notifications
You must be signed in to change notification settings - Fork 31.1k
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
test: openssl 3.4 compatibility #56294
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #56294 +/- ##
==========================================
+ Coverage 89.16% 89.19% +0.02%
==========================================
Files 662 662
Lines 191746 191797 +51
Branches 36902 36922 +20
==========================================
+ Hits 170970 171065 +95
+ Misses 13632 13567 -65
- Partials 7144 7165 +21 |
OpenSSL 3.4 has intentionally broken EVP_DigestFinal for SHAKE128 and SHAKE256 when OSSL_DIGEST_PARAM_XOFLEN is not set because a) the default length used weakened them from their maximum strength and b) a static length does not fully make sense for XOFs (which SHAKE* are). Unfortunately, while crypto.createHash accepts an option argument that can be something like `{ outputLength: 128 }`, crypto.hash doesn't offer a similar API. Therefore there is little choice but to skip the test completely for shake128 and shake256 on openssl >= 3.4. PR-URL: nodejs#56294 Fixes: nodejs#56159 Refs: openssl/openssl@b911fef Refs: openssl/openssl@ad3f28c
OpenSSL 3.4 has intentionally broken EVP_DigestFinal for SHAKE128 and SHAKE256 when OSSL_DIGEST_PARAM_XOFLEN is not set because a) the default length used weakened them from their maximum strength and b) a static length does not fully make sense for XOFs (which SHAKE* are). Unfortunately, while crypto.createHash accepts an option argument that can be something like `{ outputLength: 128 }`, crypto.hash doesn't offer a similar API. Therefore there is little choice but to skip the test completely for shake128 and shake256 on openssl >= 3.4. PR-URL: nodejs#56294 Fixes: nodejs#56159 Refs: openssl/openssl@b911fef Refs: openssl/openssl@ad3f28c
Sorry, I wasn't around for a couple weeks. Thanks for the review and suggestion! The second patch in the series basically landed separately through #56420 (not from me but the code is probably exactly the same) so I've dropped it from here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
OpenSSL 3.4 has intentionally broken EVP_DigestFinal for SHAKE128 and SHAKE256 when OSSL_DIGEST_PARAM_XOFLEN is not set because a) the default length used weakened them from their maximum strength and b) a static length does not fully make sense for XOFs (which SHAKE* are). Unfortunately, while crypto.createHash accepts an option argument that can be something like `{ outputLength: 128 }`, crypto.hash doesn't offer a similar API. Therefore there is little choice but to skip the test completely for shake128 and shake256 on openssl >= 3.4. PR-URL: nodejs#56294 Fixes: nodejs#56159 Refs: openssl/openssl@b911fef Refs: openssl/openssl@ad3f28c
I fixed the two cosmetic issues raised in the GH CI. I'm didn't look at the nodejs.org one due to the permissions it was asking from my GH account and the expectation I wouldn't be able to log in anyway. Let me know if was wrong on these and I should look at it. |
Isn't it already covered by node/test/parallel/test-crypto-oneshot-hash.js Lines 35 to 36 in 93c15bf
|
On Mon, Jan 13, 2025, Antoine du Hamel wrote:
Isn't it already covered by https://github.com/nodejs/node/blob/93c15bfbc79c42244b11f99ca931b5745e508612/test/parallel/test-crypto-oneshot-hash.js#L35-L36?
It looks indeed possible that this covers it. Since the PR just got
merged a couple hours, I hadn't seen it and haven't checked it.
|
This PR fixes two issues in the testsuite caused by changes in openssl 3.4:
In addition to #56160 , this should fix
the build of nodejs with openssl 3.4 for Ubuntu 25.04 (Plucky).