Skip to content

Conversation

@adrien-n
Copy link

This PR fixes two issues in the testsuite caused by changes in openssl 3.4:

  • using SHAKE128 and SHAKE256 requires explicitely setting an output length,
  • the error code for one of the failure mode in TLS 1.3 was off-spec.

In addition to #56160 , this should fix
the build of nodejs with openssl 3.4 for Ubuntu 25.04 (Plucky).

@nodejs-github-botnodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Dec 17, 2024
@codecov
Copy link

codecovbot commented Dec 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.19%. Comparing base (529b56e) to head (81bdf85).
Report is 26 commits behind head on main.

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 

see 49 files with indirect coverage changes

adrien-n added a commit to adrien-n/node that referenced this pull request Jan 10, 2025
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#56294Fixes: nodejs#56159 Refs: openssl/openssl@b911fef Refs: openssl/openssl@ad3f28c
adrien-n added a commit to adrien-n/node that referenced this pull request Jan 10, 2025
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#56294Fixes: nodejs#56159 Refs: openssl/openssl@b911fef Refs: openssl/openssl@ad3f28c
@adrien-n
Copy link
Author

Sorry, I wasn't around for a couple weeks.

Thanks for the review and suggestion!
I've updated the branch with the code you proposed, only slightly reformatted to be more amenable to future additional .filter() steps (though, hopefully, there won't be any other needed).

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.

Copy link
Member

@mhdawsonmhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawsonmhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2025
@github-actionsgithub-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2025
@nodejs-github-bot
Copy link
Collaborator

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#56294Fixes: nodejs#56159 Refs: openssl/openssl@b911fef Refs: openssl/openssl@ad3f28c
@adrien-n
Copy link
Author

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.

@aduh95
Copy link
Contributor

aduh95 commented Jan 13, 2025

Isn't it already covered by

if(method.startsWith('shake')&&common.hasOpenSSL(3,4))
continue;
?

@adrien-n
Copy link
Author

adrien-n commented Jan 13, 2025 via email

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-ciPRs that need a full CI run.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@adrien-n@nodejs-github-bot@aduh95@jasnell@lpinca@mhdawson