-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Support BoringSSL as alternative to OpenSSL #25890
Comments
Conversation on this from 2015-2016: #1860 @nodejs/crypto |
@nornagon handles the majority of the work on the Electron fork of Node.js in this direction, so his opinion will also be very insightful. Essentially, IMO, it shouldn't be a lot of work to make this happen as pointed out by @hashseed, especially since this could easily be done by checking how Electron's fork handles these edge cases and wrapping that behavior in a series of However, I am not sure that I am completely onboard with the idea since that'll increase the overhead of adding features and functionalities to the crypto API, even though that overhead will be as minimal as it possibly could be. Essentially, it's a bit much to expect the crypto team to walk on eggshells since everyone will have to cross-check functionality to make sure if both libraries are on the same page regarding the behavior in question and if not, handle the said edge case gracefully. |
My intention is to not maintain a fork of Node, but rather be able to pull upstream Node as direct dependency. I'm sure there is some interest in interop from electron as well. If something that works with OpenSSL but breaks with BoringSSL in an unobvious way, that would be a potential security issue for electron. |
But Google does not recommend using it:
There are no API or ABI stability guarantees for BoringSSL, that essentially means that compilation with it can not be supported for Node.js. It could be done in a "technically works, but not supported and no guarantees" kinda way, but that'll tend to break, I think? Also, that would place all the maintenance burden on the @nodejs/crypto team, because BoringSSL doesn't do that. |
@ChALkeR very true. I doubt there's someone from @nodejs/crypto who would not agree to that. |
I don't have any objection in principal to tweeking node's crypto/tls code so it compiles with boringssl. I don't see us making any comittment to support it, so it might continuously break, and those who care would have to PR fixes. Whether any specific change is worth the overhead of accepting would depend on that change and how intrusive it is. WRT specifically to set_ciphersuites, I'm about to PR deeper dependence on it. The current call is just a backwards compatability fix (see openssl/openssl#7759 (comment) for some background). It could be ifdefd based on its existence, but soon a larger change that will depend on using set_ciphersuites to support TS1.3 ciphers is going to be PRed (by me :-), and it is possible that boringssl support would be fairly involved after that change, and might even effect the js code. But, all this is just speculation. I suggest you PR the actual changes that would make consuming node easier for you, and see what comments it gets. |
Thanks for the comments! Currently BoringSSL support is not a top priority for me, but I'll consider this at some point. |
FWIW, in Electron we are patching Node and BoringSSL (& working with upstream boringssl) to provide this support already. See e.g. https://boringssl-review.googlesource.com/c/boringssl/+/33984, https://boringssl-review.googlesource.com/c/boringssl/+/33984, https://boringssl-review.googlesource.com/c/boringssl/+/32525. |
Interesting. So next time I try linking against BoringSSL it may just work :) |
@hashseed "Interesting. So next time I try linking against BoringSSL it may just work :)". |
For other people who find this thread, the nodejs patches that can make it work are currently part of the electron repo. For example: https://github.com/electron/electron/blob/main/patches/node/fix_handle_boringssl_and_openssl_incompatibilities.patch I don't think there's a "clean" patchset anywhere. So the answer to "how can I try to link Node to BoringSSL" is currently "find the patches, apply them, and write custom build files". |
So any updates? |
@SwapnilSoni1999 The BoringSSL website still says (emphasis added):
So we likely won't be switching to BoringSSL any time soon. If you want to contribute patches that allow linking against BoringSSL, maybe they would be considered. (Electron has contributed a few patches to Node.js to make it simpler to link against BoringSSL, but I don't think those are sufficient.) Other than that, I don't think anything has changed since #25890 (comment) and #25890 (comment). |
FWIW, running Electron with |
The thing is I see cloudflare is using JA3 fingerprinting. To protect site api endpoints |
I've stumbled on this issue because I was looking for a way for 2 native addons in nodejs to share the same tls library. For example suppose you have a native addon for websockets, another native addon for QUIC. Both must use a TLS library. Both can statically compile their own boringssl library and produce @nornagon I was reading https://www.electronjs.org/blog/electron-internals-using-node-as-a-library, is there a way to take the nodejs out of electron and use it as compiled executable? And does making nodejs a shared object enable the usecase I described above? |
BoringSSL is a fork of OpenSSL maintained by the Chromium project. It shares a lot of the same API.
There are use cases where it it is preferable to use BoringSSL over OpenSSL:
Currently Node.js does not link against BoringSSL because it uses some APIs that BoringSSL does not implement. One example is
SSL_CTX_set_ciphersuites
. BoringSSL supportsSSL_CTX_set_cipher_list
though, so it would be as simple as an#ifdef
to make this work.I'm trying to gauge the interest in this work. I would be willing to invest the time and create necessary PRs.
The text was updated successfully, but these errors were encountered: