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

crypto: don't call SSL_CTX_set_ciphersuites on boringssl #26365

Closed
wants to merge 1 commit into from

Conversation

nornagon
Copy link
Contributor

BoringSSL doesn't have a different function to set cipher-suites in TLS 1.3.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Feb 28, 2019
@nornagon nornagon changed the title fix: don't call SSL_CTX_set_ciphersuites on boringssl crypto: don't call SSL_CTX_set_ciphersuites on boringssl Feb 28, 2019
Copy link
Contributor

@sam-github sam-github left a 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.

@sam-github
Copy link
Contributor

@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

@rvagg
Copy link
Member

rvagg commented Feb 28, 2019

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 #ifdefs. Not a -1 but I'm not keen.

@rvagg
Copy link
Member

rvagg commented Feb 28, 2019

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

@nornagon
Copy link
Contributor Author

nornagon commented Mar 1, 2019

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 :)

@sam-github
Copy link
Contributor

To be clear, I'm +1 because I think this is unobtrusive.

@addaleax
Copy link
Member

addaleax commented Mar 2, 2019

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 5, 2019
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>
@BridgeAR
Copy link
Member

BridgeAR commented Mar 5, 2019

Landed in 6e3af4d 🎉

@BridgeAR BridgeAR closed this Mar 5, 2019
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Mar 12, 2019
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants