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

Externalise tls.DEFAULT_CIPHERS please! #46462

Closed
andreas-ibm opened this issue Feb 1, 2023 · 2 comments · Fixed by #46482
Closed

Externalise tls.DEFAULT_CIPHERS please! #46462

andreas-ibm opened this issue Feb 1, 2023 · 2 comments · Fixed by #46482
Labels
feature request Issues that request new features to be added to Node.js.

Comments

@andreas-ibm
Copy link
Contributor

What is the problem this feature will solve?

We would like to modify the default ciphers on startup, unless the user has overridden with --tls-cipher-list. So for example on startup, we'll compare tls.DEFAULT_CIPHERS against crypto.constants.defaultCoreCipherList and if they're the same (the user hasn't overridden on command line) then we might want to

  tls.DEFAULT_CIPHERS = tls.DEFAULT_CIPHERS+'!AES128-SHA:!AES128-SHA256:!AES256-SHA:!AES256-SHA256:!ECDHE-RSA-AES128-SHA:!ECDHE-RSA-AES128-SHA256:!ECDHE-RSA-AES256-SHA:!ECDHE-RSA-AES256-SHA384:!TLS_AES_128_GCM_SHA256:!TLS_AES_256_GCM_SHA384:!TLS_CHACHA20_POLY1305_SHA256:!ECDHE-ECDSA-AES128-SHA:!ECDHE-ECDSA-AES128-SHA256:!ECDHE-ECDSA-AES256-SHA:!ECDHE-ECDSA-AES256-SHA384:!kRSA'

to remove some older / weaker ciphers.

This also means we can customise the modification to remove more things, but still get the benefit of any new, stronger ciphers added.

What is the feature you are proposing to solve the problem?

We can do it today using the above method, but because tls.DEFAULT_CIPHERS isn't documented it's implied that you may remove it in the future, I'm asking for it to be documented.

What alternatives have you considered?

We could build and set crypto.constants.defaultCoreCipherList ourselves, but when we then upgrade we would need to re-review the cipher list to add in any new ciphers to our "blessed" list. Using the above method is more dynamic and easier to change (i.e. we don't need to rebuild).

@andreas-ibm andreas-ibm added the feature request Issues that request new features to be added to Node.js. label Feb 1, 2023
@bnoordhuis
Copy link
Member

When you say "externalise" you mean "document"? PR welcome, I suppose.

[..] when we then upgrade we would need to re-review the cipher list to add in any new ciphers [..]

Wouldn't you need to do that anyway as good engineering practice?

@andreas-ibm
Copy link
Contributor Author

I'll see if I can find the right place to chance for a PR :-) I just wanted to check it wouldn't cause problems, like "no, don't do that, if people start using that variable it'll cause them to do this other Bad Thing".

Yes, we review approximately every 3 months which is more often than we (currently) rebuild node.js, though this may change. I'd also like to be able to handle the case where the customer provides the node.js instance so being able to handle it from within the JS would be nice!

Right, I'll look at that PR then, cheers.

andreas-ibm added a commit to andreas-ibm/node that referenced this issue Feb 3, 2023
The DEFAULT_CIPHERS already exists, this change shows how to use it.

Fixes: nodejs#46462
nodejs-github-bot pushed a commit that referenced this issue Feb 23, 2023
The DEFAULT_CIPHERS already exists, this change shows how to use it.

Fixes: #46462
PR-URL: #46482
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this issue Mar 13, 2023
The DEFAULT_CIPHERS already exists, this change shows how to use it.

Fixes: #46462
PR-URL: #46482
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
danielleadams pushed a commit that referenced this issue Apr 11, 2023
The DEFAULT_CIPHERS already exists, this change shows how to use it.

Fixes: #46462
PR-URL: #46482
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants