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

Plans on incorporating BoringSSL #1860

Closed
tychotatitscheff opened this issue Jun 1, 2015 · 21 comments
Closed

Plans on incorporating BoringSSL #1860

tychotatitscheff opened this issue Jun 1, 2015 · 21 comments
Labels
crypto Issues and PRs related to the crypto subsystem. discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js.

Comments

@tychotatitscheff
Copy link

It won't be near future and I think that you have better things to do (in peculiar with 3.x release and v8 mess) but if #428 is delayed could BoringSSl be an option ?

In fact it :

Don't mind to close this if you find that this is a dumb question.

EDIT : md formatting and more informations.

@bnoordhuis
Copy link
Member

Not a stupid question but I speculate it's not something we'll undertake lightly. A lot of time and effort has been sunk in integrating OpenSSL.

For starters, someone (not me, I'm not volunteering) would have to GYP-ify BoringSSL so it can be integrated with the build system.

@tychotatitscheff
Copy link
Author

I never did this but I can try.
Correct me if I'm wrong, I need to :

@bnoordhuis
Copy link
Member

That would a good first step. You may want to take a look at openssl.gyp and openssl.gypi for the feature sets that would need to be supported.

For example, we try very hard to use the assembly versions of algorithms when available, the cli tools are built for use in the test suite, there are some Windows fix-ups, etc. It's also an open question how distro packagers that link to a shared openssl should deal with this.

Also, /cc @nodejs/crypto - it would be good to get consensus first before you waste a lot of time on something that ends up getting rejected.

EDIT: For the record, I'm neutral on this. I don't see compelling reasons pro or con boringssl vis-a-vis openssl. For any pro, you can probably come up with a con and vice versa.

@tychotatitscheff
Copy link
Author

Did further research:

  • it seem's that gyp version already exists (chromium maintans it)
  • As stated here it may require:
    • perl to build
    • ninja to build on windows
    • yasm to build on windows
    • go to test
      This was dependency from the CMAKE version so it may be not needed with Gyp but are this dependency not showstopers ?

@indutny
Copy link
Member

indutny commented Jun 1, 2015

I think OpenSSL is quite close to integrating ChaCha. Since we are on 1.0.2

  • it wouldn't be much problem to integrate these changes.

Don't get me wrong, Adam is a security expert, but I don't have much trust
in BoringSSL. Mostly because it doesn't have the git history of the initial
changes between it and OpenSSL.

Second thing - I am a bit afraid of how the CVEs will be made available for
the BoringSSL.

On Monday, June 1, 2015, Tycho Tatitscheff notifications@github.com wrote:

Did further research:


Reply to this email directly or view it on GitHub
#1860 (comment).

@tychotatitscheff
Copy link
Author

Also, /cc @nodejs/crypto - it would be good to get consensus first before you waste a lot of time on something that ends up getting rejected.

Sure I will wait from this but due to the fact that gyp version already exists, it would be not that hard to extarct crypto from chromium project (both C file under /src/crypto and thirdparty (assembly file and gyp) under third_party/boringssl.

@tychotatitscheff
Copy link
Author

@indutny seem's valide concern.

So:

  • pros:
    • integrate or will integrate libressl fix to openssl mess (this prez freaked me).
    • seem's to be (like libressl) less vulnerable to openssl vulnerabilities
    • integrate faster the news ciphers
    • used in webrtc
  • cons:
    • where to find the CVE
    • intial change review
    • meen to be use in google

Anyway, if you guys choose to give it a try I can start looking into this (forking, configuring gyp and starting the replacement).

@Fishrock123 Fishrock123 added crypto Issues and PRs related to the crypto subsystem. discuss Issues opened for discussions and feedbacks. labels Jun 1, 2015
@tychotatitscheff
Copy link
Author

Mostly because it doesn't have the git history of the initial
changes between it and OpenSSL.

Diffing first boringssl commit and last openssl commit before fork, even with suppressing boringssl go test file, open and boring ssl build file, docs and co and requesting minimal, there is still plenty changes.

https://gist.github.com/tychotatitscheff/52bbd3e003e9c9d97e0a

@shigeki
Copy link
Contributor

shigeki commented Jun 1, 2015

I thought of this some times ago and have a few comments.

  • ChaChe20 + Poly1305 for OpenSSL would not be introduced in openssl-1.0.2 branch, maybe next 1.1.0. Cloudfare provides a patch for OpenSSL in https://github.com/cloudflare/sslconfig/blob/master/patches/openssl__chacha20_poly1305_cf.patch that supports AVX/AVX2.
  • Looking at some APIs in BoringSSL, they are already differed from those of OpenSSL. I think it is very hard to support both API simultaneously unless we implement some wrappers.
  • FreeBSD and Solaris are not supported platform in BoringSSL while iojs supports them. Probably no-asm options needed for BoringSSL but not existed. For Windows, yasm is also needed to compile asms. But it is our another work to deprecate current masm support on Win32.
  • There seems no release step in BoringSSL. They keep the version of openssl to be 1.0.2 but change APIs. It would be very hard to be consistent with our semver release policy in iojs.

But anyway, I think it is very interesting to support BoringSSL in iojs. TLS1.3 is forth-coming and BoringSSL would be one of the early adopter to support it. It would be great if we can have alternate solutions for SSL library.

@indutny
Copy link
Member

indutny commented Jun 1, 2015

Oh, another cons from me (inspired by @shigeki's comment):

There are many user space addons that depend on OpenSSL API. If not a
concern, this should be considered anyway

On Monday, June 1, 2015, Shigeki Ohtsu notifications@github.com wrote:

I thought of this some times ago and have a few comments.

ChaChe20 + Poly1305 for OpenSSL would not be introduced in
openssl-1.0.2 branch, maybe next 1.1.0. Cloudfare provides a patch for
OpenSSL in
https://github.com/cloudflare/sslconfig/blob/master/patches/openssl__chacha20_poly1305_cf.patch
that supports AVX/AVX2.

Looking at some APIs in BoringSSL, they are already differed from
those of OpenSSL. I think it is very hard to support both API
simultaneously unless we implement some wrappers.

FreeBSD and Solaris are not supported platform in BoringSSL while iojs
supports them. Probably no-asm options needed for BoringSSL but not
existed. For Windows, yasm is also needed to compile asms. But it is our
another work to deprecate current masm support on Win32.

There seems no release step in BoringSSL. They keep the version of
openssl to be 1.0.2 but change APIs. It would be very hard to be consistent
with our semver release policy in iojs.

But anyway, I think it is very interesting to support BoringSSL in iojs.
TLS1.3 is forth-coming and BoringSSL would be one of the early adopter to
support it. It would be great if we can have alternate solutions for SSL
library.


Reply to this email directly or view it on GitHub
#1860 (comment).

@miketheprogrammer
Copy link

would'nt it be great to say

request({
uri: "https://google.com:443",
ssli: require('boringssl')
})

Where boring ssl subscribes to some fundamental wrapper definition of the minimum subset of functionality that needs to be exposed.

*edit
rather than complicate the "Small tight core in C, large user land in JS" philosophy that i so religiously subscribe to now.

@tychotatitscheff
Copy link
Author

FreeBSD and Solaris are not supported platform in BoringSSL while iojs supports them. Probably no-asm options needed for BoringSSL but not existed. For Windows, yasm is also needed to compile asms. But it is our another work to deprecate current masm support on Win32.

I will ping @agl about this.

I think it is very hard to support both API simultaneously unless we implement some wrappers.

After nan let's create sas(simple abstraction for SSL) ^^
More seriously it may have a benefits to make some js wrapper library out of the core. It would be huge work thought.

@brendanashworth brendanashworth added the feature request Issues that request new features to be added to Node.js. label Jun 18, 2015
@shouze
Copy link

shouze commented Nov 26, 2015

Hi guys, no news here since some time. I'm using nodejs on Scaleway C1 arm machines... and making https requests to some external APIs is very slow... I pay a minimum overhead of 200ms for each request... freaking no?

So... yes patched OpenSSL or using LibreSSL should be cool for example. I can help as benefits would be great for me.

@shigeki
Copy link
Contributor

shigeki commented Nov 26, 2015

I'm going to close this for now because BoringSSL clearly says that incorporating to the third parties is not recommended in README.

See https://boringssl.googlesource.com/boringssl/ as

BoringSSL is a fork of OpenSSL that is designed to meet Google’s needs.
Although BoringSSL is an open source project, it is not intended for general use, as OpenSSL is. We don’t recommend that third parties depend upon it. Doing so is likely to be frustrating because there are no guarantees of API or ABI stability.

@shigeki shigeki closed this as completed Nov 26, 2015
@bridiver
Copy link

bridiver commented Nov 8, 2016

we are building using node in a project that also uses Chrome so using boringssl looks like it will save a lot of build headaches. The changes are trivial. On the first pass I commented out OPENSSL_no_config and the section after it for loading a config file because that isn't supported by boringssl. After that it built without a hitch.

@shigeki
Copy link
Contributor

shigeki commented Nov 9, 2016

It was already included in https://boringssl.googlesource.com/boringssl/+/373a6a5a7d0b055f1bb4cfe7bb4254c49a28ab67 .

In the long run to incorporate BoringSSL, I'd like to have a comment from @agl .

@shigeki shigeki reopened this Nov 9, 2016
@bridiver
Copy link

bridiver commented Nov 9, 2016

that commit would seem to indicate a desire to keep it compatible despite the reference you linked to above

@agl
Copy link
Contributor

agl commented Nov 9, 2016

We try to keep the number of patches needed to build Node with BoringSSL to a minimum, and for those patches to be included in node/master where applicable. That's because we have some internal case where we build Node with BoringSSL.

So, while running Node with BoringSSL is likely to be relatively painless because of the above, it might need some tweaking from time-to-time. We don't currently have any plans to publish our patches to Node, although that's not inconceivable if useful.

@shigeki
Copy link
Contributor

shigeki commented Nov 9, 2016

@agl Thanks for your comments.

I'm very glad to have contributions to build Node with BoringSSL but I think that it's not yet officially supported use according to Google's plan. I'm going to close this again.

@shigeki shigeki closed this as completed Nov 9, 2016
@bridiver
Copy link

@agl we are interested in the patches and plan on moving forward with building against boringssl

@ssaroha
Copy link

ssaroha commented Nov 26, 2016

If somebody has node building with boringssl, we would be very much interested as well so that node and webrtc work together nicely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. discuss Issues opened for discussions and feedbacks. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

No branches or pull requests