-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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
Update doc/api/cli.md, add cipher DEFAULT@SECLEVEL=0 for TLSv1 #49236
Conversation
/cc @nodejs/crypto for reviews |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, yes this is indeed required for any kind of TLSv1 support with modern OpenSSL. This definitely makes sense in that context, and it's useful important info for people who need this.
That said, I don't think we should recommend this without a bit more context so people know what it means. Since setting SECLEVEL=0
reduces all sorts of other TLS constraints too. I'd suggest we:
- Briefly explain the consequences of this directly: basically it lowers the OpenSSL security level to 0, which can be useful for compatibility but allows also sorts of ciphers, small key sizes and other configurations that may be insecure in a modern environment.
- Link to https://www.openssl.org/docs/manmaster/man3/SSL_CTX_set_security_level.html for more information.
- Include this same info in the main TLS docs somewhere too.
I'd suggest we put that main explanation in the TLS docs somewhere, and then just link to that from here ("This may significantly reduce TLS security - see ... for more information").
Assuming that setting the same option at runtime with TLS APIs directly also works (i.e. using ciphers: ...
as a context option), we should also probably recommend doing that instead where possible. For most applications it's significantly safer to lower the security level only for the specific operations that need it, rather than lowering it globally.
@pimterry Thanks for the detailed review! I agree that |
I've done a quick bit of testing, this seems to work correctly: tls.connect({
host: 'tls-v1-0.badssl.com',
port: 1010,
ciphers: 'DEFAULT@SECLEVEL=0',
minVersion: 'TLSv1'
}).on('error', (e) => console.log(e.message)) The ciphers option effectively the same as the CLI option you suggest, but on a per-context basis rather than modifying the global default. Testing in Node v20.11.1 on my machine, without the cipher & minVersion option this fails with unsupported protocol and/or signature algorithm errors, but with those options it works (prints 'certificate is expired' annoyingly, because this test server is not well maintained, but I think that's unrelated). Can you test that in your setup? Assuming that does work, I think we should probably nudge people in this direction as a first solution, rather than reducing the process-wide security level. |
@pimterry Thank you for the code sample! I see you are trying to tune // Node.js v20.11.1
// seems like adding '--tls-min-v1.0' doesn't affect anything
var execSync = require('child_process').execSync
var fs = require('fs')
var tls = require('tls')
var port = 8000
var minVersion = 'TLSv1'
var maxVersion = 'TLSv1' // doesn't work
// var maxVersion = 'TLSv1.2' // works
// var maxVersion = 'TLSv1.3' // works
execSync('openssl req -x509 -newkey rsa:1024 -keyout key -out cert -nodes -subj "/C=US/CN=localhost"')
var key = fs.readFileSync('key')
var cert = fs.readFileSync('cert')
tls.createServer({key, cert, ciphers: 'DEFAULT@SECLEVEL=0', minVersion}, function (socket) {
console.log('it works!', socket.getProtocol())
socket.end()
this.close()
}).
listen(port, () => {
tls.connect(port, {ca: [cert], minVersion, maxVersion})
}) |
Yes, the same does apply to the server side, but both can use the same runtime-option solution. In your example, you've used the To fix this, if you add the Does that make sense? |
@pimterry Now it all makes sense, thanks! I didn't know // Node.js v20.11.1
var execSync = require('child_process').execSync
var fs = require('fs')
var tls = require('tls')
var port = 8000
var minVersion = 'TLSv1'
var maxVersion = 'TLSv1'
execSync('openssl req -x509 -newkey rsa:1024 -keyout key -out cert -nodes -subj "/C=US/CN=localhost"')
var key = fs.readFileSync('key')
var cert = fs.readFileSync('cert')
tls.createServer({key, cert, ciphers: 'DEFAULT@SECLEVEL=0', minVersion}, function (socket) {
console.log('it works!', socket.getProtocol())
socket.end()
this.close()
}).
listen(port, () => {
tls.connect(port, {ca: [cert], ciphers: 'DEFAULT@SECLEVEL=0', minVersion, maxVersion})
}) At this point I think the best would be to mention |
Yes that sounds great. How about:
|
Updated docs: for TLSv1 server to work you need to pass both
--tls-min-v1.0
and--tls-cipher-list=DEFAULT@SECLEVEL=0
(same for TLSv1.1).