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: adapt tests for OpenSSL 3.1 #47859

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

OttoHollmann
Copy link
Contributor

From OpenSSL 3.1.0 protocols SSL 3, TLS 1.0, TLS 1.1, and DTLS 1.0 only work at security level 0. So if we still want to test it, we need to decrease security.
https://www.openssl.org/news/openssl-3.1-notes.html

@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 May 4, 2023
@tniessen
Copy link
Member

tniessen commented May 4, 2023

Thanks for the contribution @OttoHollmann. I think these additional options should only be added when actually testing a custom Node.js build that was built/linked against OpenSSL >= 3.1. (We are unlikely to officially switch to OpenSSL 3.1 due to their release timeline.)

@OttoHollmann OttoHollmann changed the title Adapt tests for OpenSSL 3.1 test: Adapt tests for OpenSSL 3.1 May 5, 2023
@OttoHollmann
Copy link
Contributor Author

I understand (and respect your decision) that you don't want to switch to OpenSSL 3.1 but from a distribution maintainer point of view it's a complication.

In our rolling release distribution openSUSE Tumbleweed we would like to switch to OpenSSL 3.1 but on the other hand we would like to avoid our specific out of tree patches.

Accepting this PR could be also beneficial for other distributions and individuals who want to build nodejs with latest OpenSSL. Especially when this PR doesn't harm tests with OpenSSL 3.0.

@tniessen
Copy link
Member

tniessen commented May 5, 2023

@OttoHollmann To clarify, when I say that we likely won't switch to OpenSSL 3.1, what I mean is that we won't upgrade our own copy in deps/openssl because of the reasons discussed in #47177.

Of course, our intention is to be compatible with OpenSSL >= 3.1 so that users are free to dynamically link against newer versions than 3.0, and that includes the tests.

My suggestion remains the same, that is, to only add the flags that are required only for OpenSSL >= 3.1 when the test is actually running against OpenSSL >= 3.1. For example, you are already checking common.hasOpenSSL3 in test-tls-min-max-version.js to only add the flag in OpenSSL >= 3.0, but my suggestion is to further restrict that to OpenSSL >= 3.1.

@OttoHollmann
Copy link
Contributor Author

I added hasOpenSSL31 flag and ciphers DEFAULT:@SECLEVEL=0 are set only if OpenSSL 3.1 or later is used. Otherwise DEFAULT ciphers are set.

Also I squashed commits into one and fixed commit message because the tests were still complaining about the missing prefix.

@tniessen
Copy link
Member

Thank you @OttoHollmann, that's great!

For the commit message check to pass, you'll have to change it to test: adapt tests for OpenSSL 3.1 (that is, lowercase adapt). Let me know if you'd like me to do so for you.

@nodejs/crypto @nodejs/build I assume it is unrealistic to add OpenSSL 3.1 to our infra?

@OttoHollmann
Copy link
Contributor Author

Changed the commit message and wrapped offending lines so they don't exceed 120 characters.

@tniessen tniessen changed the title test: Adapt tests for OpenSSL 3.1 test: adapt tests for OpenSSL 3.1 May 10, 2023
@richardlau
Copy link
Member

@nodejs/crypto @nodejs/build I assume it is unrealistic to add OpenSSL 3.1 to our infra?

I think it's something we will end up doing, probably in addition to the existing dynamic linking to OpenSSL 3.0.

@tniessen
Copy link
Member

That's great, thank you @richardlau.

@tniessen tniessen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels May 10, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 10, 2023
@nodejs-github-bot
Copy link
Collaborator

@richardlau
Copy link
Member

@nodejs/crypto @nodejs/build I assume it is unrealistic to add OpenSSL 3.1 to our infra?

I think it's something we will end up doing, probably in addition to the existing dynamic linking to OpenSSL 3.0.

FWIW I've started work on adding testing with Node.js dynamically linked to OpenSSL 3.1 to our CI.

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 1, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 1, 2023
@nodejs-github-bot nodejs-github-bot merged commit 5f28372 into nodejs:main Jun 1, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 5f28372

@richardlau richardlau added the lts-watch-v18.x PRs that may need to be released in v18.x. label Jun 3, 2023
targos pushed a commit that referenced this pull request Jun 4, 2023
PR-URL: #47859
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
@targos targos mentioned this pull request Jun 4, 2023
franciszek-koltuniuk-red pushed a commit to franciszek-koltuniuk-red/node that referenced this pull request Jun 7, 2023
PR-URL: nodejs#47859
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Jul 6, 2023
@danielleadams
Copy link
Contributor

This broke tests when pulling into v18.x-staging, so it will need a backport.

richardlau pushed a commit to richardlau/node-1 that referenced this pull request Jul 6, 2023
PR-URL: nodejs#47859
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Jul 6, 2023
PR-URL: nodejs#47859
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Jul 6, 2023
PR-URL: nodejs#47859
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
richardlau pushed a commit to richardlau/node-1 that referenced this pull request Jul 6, 2023
PR-URL: nodejs#47859
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
@richardlau
Copy link
Member

richardlau commented Jul 6, 2023

This broke tests when pulling into v18.x-staging, so it will need a backport.

@danielleadams Which tests broke? I cherry picked the commit for this PR onto the v18.x-staging branch as it was this morning (2edd0fd) and got a clean CI run https://ci.nodejs.org/job/node-test-commit/63239/. All tests also passed locally for me on Linux x64.

FWIW I've rebased onto the current v18.x-staging (2262653) + the cherry pick of this PR, and kicked off https://ci.nodejs.org/job/node-test-commit/63243/

Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#47859
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
PR-URL: nodejs#47859
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
@targos targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. lts-watch-v18.x PRs that may need to be released in v18.x. labels Oct 27, 2023
targos pushed a commit that referenced this pull request Oct 28, 2023
PR-URL: #47859
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
@richardlau
Copy link
Member

@nodejs/crypto @nodejs/build I assume it is unrealistic to add OpenSSL 3.1 to our infra?

I think it's something we will end up doing, probably in addition to the existing dynamic linking to OpenSSL 3.0.

FWIW I've started work on adding testing with Node.js dynamically linked to OpenSSL 3.1 to our CI.

I enabled testing with dynamically linked OpenSSL 3.1 earlier this week. It's now running as part of node-test-commit-linux-containered. Of course, OpenSSL 3.2 is out now, which is another version we could be testing with (but we'll need to figure out if more test adaptions would be needed for that version of OpenSSL before enabling it in the main CI workflow).

sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#47859
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#47859
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backported-to-v18.x PRs backported to the v18.x-staging branch. 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