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

build: explicitly set OpenSSL default TLS seclevel #54639

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

Conversation

richardlau
Copy link
Member

Explicitly set the default TLS seclevel for OpenSSL. OpenSSL 3.2 changed the default TLS seclevel from 1 (default in OpenSSL 3.0 and 3.1) to 2. This causes smaller key sizes to be rejected, as well as any cipher suite that uses RC4.

Since the End-of-Life date for OpenSSL 3.0 is before the End-of-Life date for Node.js 22, we anticipate that we will need to update OpenSSL to whatever the next (as yet unannounced) LTS version of OpenSSL will be. Fixing the seclevel will minimize ecosystem disruption when that update happens.

Even with the seclevel fixed at 1, updating from OpenSSL 3.1 would still result in a change -- OpenSSL 3.1 disabled SSLv3, TLS 1.0, TLS 1.1 and DTLS 1.0 at seclevel 1.

Refs: https://docs.openssl.org/3.0/man3/SSL_CTX_set_security_level/#default-callback-behaviour
Refs: https://docs.openssl.org/3.1/man3/SSL_CTX_set_security_level/#default-callback-behaviour
Refs: https://docs.openssl.org/3.2/man3/SSL_CTX_set_security_level/#default-callback-behaviour
Refs: https://github.com/nodejs/collaborators/discussions/202#discussioncomment-10477925


I'm opening this to start the conversation/see if this is something we want to do. It turns out that the change in OpenSSL 3.2 of the default seclevel from 1 to 2 is the cause of the majority of the test failures identified in #53382.

If we land this, then we will become responsible for setting the default TLS seclevel in Node.js (for the binaries we build) instead of leaving it to OpenSSL. We could opt to do nothing (retain the status quo) which potentially means updating to the next LTS OpenSSL (whatever that ends up being) in Node.js 22 could break users -- we do have an explicit exception in our release policy for breaking changes for security reasons (which we could argue the OpenSSL update would be).

cc @nodejs/security-wg @nodejs/tsc @nodejs/releasers

Explicitly set the default TLS seclevel for OpenSSL. OpenSSL 3.2 changed
the default TLS seclevel from 1 (default in OpenSSL 3.0 and 3.1) to 2.
This causes smaller key sizes to be rejected, as well as any cipher
suite that uses RC4.

Since the End-of-Life date for OpenSSL 3.0 is before the End-of-Life
date for Node.js 22, we anticipate that we will need to update OpenSSL
to whatever the next (as yet unannounced) LTS version of OpenSSL will
be. Fixing the seclevel will minimize ecosystem disruption when that
update happens.

Even with the seclevel fixed at 1, updating from OpenSSL 3.1 would still
result in a change -- OpenSSL 3.1 disabled SSLv3, TLS 1.0, TLS 1.1 and
DTLS 1.0 at seclevel 1.

Refs: https://docs.openssl.org/3.0/man3/SSL_CTX_set_security_level/#default-callback-behaviour
Refs: https://docs.openssl.org/3.1/man3/SSL_CTX_set_security_level/#default-callback-behaviour
Refs: https://docs.openssl.org/3.2/man3/SSL_CTX_set_security_level/#default-callback-behaviour
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Aug 29, 2024
@avivkeller avivkeller added the openssl Issues and PRs related to the OpenSSL dependency. label Aug 29, 2024
@mhdawson
Copy link
Member

I think we would only want to do this in versions which are already LTS

@richardlau
Copy link
Member Author

I think we would only want to do this in versions which are already LTS

I disagree -- if we decide to land this I think this should apply to all releases (i.e. we would avoid changing the seclevel during the lifetime of a release (even current)). Effectively changing the explicitly set seclevel would become semver-major.

@mhdawson
Copy link
Member

@richardlau my initial concern is that it would be easy to forget to change in Node.js when OpenSSL updates (ie we'd never notice), meaning that we'd end up with Node.js being a lower level than the default for a new SemVer major of Node.js. I think it would be safer to land a change like this as part of cutting a major. That would ensure it would never change during the life of the major, while also ensuring we don't miss an update to it.

@richardlau
Copy link
Member Author

I think it would be safer to land a change like this as part of cutting a major.

@nodejs/releasers Thoughts? It would be one more step to remember to do when cutting a major.

@RafaelGSS
Copy link
Member

I think doing it in a security release is a better approach. We'll have tracking and as mentioned by Richard, we can do it per policy if we believe that's the right thing to do.

@mcollina
Copy link
Member

If I understood the problem correctly, we also have the option to cut the v22 LTS short.

I don't see how we can avoid doing this for v22 if we want to keep the LTS schedule.

@adrien-n
Copy link

I'm not a node person but rather from a distribution background (Ubuntu, currently dealing with openssl). I've been looking at various related issues and thinking about them. I believe the best approach is to reduce the security level or system-wide configuration only during build/test time. That should be enough to avoid the issues mentioned here.

You can (maybe should) specify an appropriate openssl configuration file through environment variables during your testsuite. This will shield the testsuite from any change that distributions can and will do.

Also, consider python's ssl module which first changed defaults to stronger ones but didn't keep up afterwards and, after a few years, ended up with settings that were weaker than the defaults. Without even thinking about stronger/weaker settings, the issue is the one already pointed out: one more thing to maintain and one more thing that can be forgotten about until it becomes problematic.

PS: I don't have insider knowledge but I suspect that the rate of seclevel changes in openssl is going to slow down as the settings were a bit too old and are looking much better now IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants