test: Use seclevel=0 unconditionally on OpenSSL 3 #49885
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
OpenSSL 3.1 forces TLS v1.1 and less to security level 0 so the security level was lowered in the tests to pass them. There is no need to conditionally lower the limit on OpenSSL 3.1 since the same can be done on 3.0. Both OpenSSL share the same ABI so it nodejs can be compiled again 3.0 and run the tests against 3.1. Remove 3.1 special case and use it unconditionally on the 3 series. Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
nodejs-github-bot
added
needs-ci
|
I don't think this is something we want to merge. You're right that it can be done but that doesn't mean it's desirable. Being as strict as possible is preferable because it helps catch unintentional changes. Your patch makes the test suite laxer under openssl 3.0, if I read it correctly. |
bnoordhuis
added
tls
|
On 2023-09-26 15:12:03 [-0700], Ben Noordhuis wrote:
I don't think this is something we want to merge. You're right that it
_can_ be done but that doesn't mean it's _desirable_.
Being as strict as possible is preferable because it helps catch
unintentional changes. Your patch makes the test suite laxer under
openssl 3.0, if I read it correctly.
The question is if this is an unintentional change you want to see.
Not supporting certain protocol levels can be still disabled by the
system administrator. However, thesting that the error code are returned
as expected during the testsuite is needed regardless. From that point
of view I don't see anything wrong with unconditionally lowering the
security level if a lower protocol is forced by the test.
Using TLSv1.2 for some of the tests (session resumption test for
instance) would be a work around for some but there is no work around if
testing of TLSv1.1 and less connections is needed.
Sebastian
|
|
Not sure I follow your line of reasoning. Tests get updated and expanded from time to time. Lowering the seclevel makes it more likely for questionable behavior to slip in. That's undesirable. |
|
On 2023-09-28 13:53:28 [-0700], Ben Noordhuis wrote:
Not sure I follow your line of reasoning. Tests get updated and
expanded from time to time. Lowering the seclevel makes it more likely
for questionable behavior to slip in. That's undesirable.
Right. Looking at the seclevel it looks not needed. However if you
insist in keeping it there is not much I can do.
Sebastian
|
|
Thanks for the pull request but I'll make the judgment call of not merging it. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
OpenSSL 3.1 forces TLS v1.1 and less to security level 0 so the security level was lowered in the tests to pass them.
There is no need to conditionally lower the limit on OpenSSL 3.1 since the same can be done on 3.0. Both OpenSSL share the same ABI so it nodejs can be compiled again 3.0 and run the tests against 3.1. The latter is used in Debian CI to ensure smooth transition without recompiling.
Remove 3.1 special case and use it unconditionally on the 3 series.