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

test: openssl 3.4 compatibility #56294

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

test: openssl 3.4 compatibility #56294

wants to merge 1 commit into from

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-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Dec 17, 2024
Copy link

codecov bot 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#56294

Fixes: 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#56294

Fixes: 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

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson mhdawson added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 13, 2025
@github-actions github-actions bot 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#56294

Fixes: 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 free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants