-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
crypto: don't call SSL_CTX_set_ciphersuites on boringssl #26365
Conversation
41e1ec6
to
0fbfc86
Compare
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.
OK by me, but be aware that this code is going away soon.
See #26209, whee both js and C++ rely on this API, so it won't be such a simple "one extra arg to an ifdef" to work around.
If TLS1.3 gets more popular, then set_ciphersuites() is going to get called more, and BoringSSL is going to have to resolve its differences.
@nornagon if you have any suggestions for easing the burden for dual-support of BoringSSL and OpenSSL maybe you want to chime in: openssl/openssl#8000 |
In the past we've just ignored BoringSSL, partly on advice of the authors who talk about their own API instability and its use primarily as an internal Google tool. Compatibility will be like chasing the wind so my take would be that if you really want BoringSSL support then it's on you downstream and not for nodejs/node to be cluttered with changing |
Having said that, in the policy (that I wrote, approved but not quite merged), it does say we'll take compatibility contributions as long as they are unobtrusive: https://github.com/nodejs/TSC/pull/479/files#diff-e3dc80180e02cdd28ed2f79b48ad8bb9R80 |
For reference, this patch is required to build Electron, which uses BoringSSL. I'm totally fine with doing the work to maintain compatibility, but I thought I might as well upstream it where I can so that others can take advantage of the work :) |
To be clear, I'm +1 because I think this is unobtrusive. |
PR-URL: nodejs#26365 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Landed in 6e3af4d 🎉 |
PR-URL: nodejs#26365 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
BoringSSL doesn't have a different function to set cipher-suites in TLS 1.3.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes