-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Conversation
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.) |
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. |
@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 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 |
I added Also I squashed commits into one and fixed commit message because the tests were still complaining about the missing prefix. |
Thank you @OttoHollmann, that's great! For the commit message check to pass, you'll have to change it to @nodejs/crypto @nodejs/build I assume it is unrealistic to add OpenSSL 3.1 to our infra? |
Changed the commit message and wrapped offending lines so they don't exceed 120 characters. |
I think it's something we will end up doing, probably in addition to the existing dynamic linking to OpenSSL 3.0. |
That's great, thank you @richardlau. |
FWIW I've started work on adding testing with Node.js dynamically linked to OpenSSL 3.1 to our CI.
|
Landed in 5f28372 |
PR-URL: #47859 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: nodejs#47859 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
This broke tests when pulling into v18.x-staging, so it will need a backport. |
PR-URL: nodejs#47859 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: nodejs#47859 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: nodejs#47859 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: nodejs#47859 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
@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/ |
PR-URL: nodejs#47859 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: nodejs#47859 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: #47859 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
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). |
PR-URL: nodejs/node#47859 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
PR-URL: nodejs/node#47859 Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Richard Lau <rlau@redhat.com>
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