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

Discussion: OpenSSL 1.1.0 planning #4270

Closed
rvagg opened this issue Dec 14, 2015 · 41 comments
Closed

Discussion: OpenSSL 1.1.0 planning #4270

rvagg opened this issue Dec 14, 2015 · 41 comments
Labels
openssl Issues and PRs related to the OpenSSL dependency.

Comments

@rvagg
Copy link
Member

rvagg commented Dec 14, 2015

The first Alpha of OpenSSL 1.1.0 is out now so we could be experimenting with integration on a dedicated branch if someone dares to make a start.

The current list of changes is here: https://www.openssl.org/news/openssl-1.1.0-notes.html

It looks like there's some nice cleanup going on with some needed removals, there's also some interesting additions that are worth discussing (also, no header symlinking!).

The planned timing for 1.1.0 is here. The awkward part is that it's not due to be final until late April, the date roughly coincides with a projected V8 5.0 release (rough but educated guess on my part) but falls later than what would be ideal for inclusion in Node.js v6 which will go on to be LTS. I doubt it's something we can include in a semver-minor so it has to be in v6 or not from the beginning. There's some discussion going on regarding V8 and Node.js v6 timing over at nodejs/Release#62 that's relevant to this.

Regarding OpenSSL support, we'd be covered by their support schedule if we opted to stay with OpenSSL 1.0.2 as it's not due to be phased out until the end of 2019 and Node.js v6 LTS would end support in April 2019.

Aside from questions of timing, the following questions stand out to me as worthy of discussion:

  • If we shipped a Node.js v6 without extended master secret support, will we be regretting it shortly thereafter, perhaps this will become a must-have for TLS soon?
  • Does the addition of CCM and/or OCB mode mean we may need new core APIs to expose the functionality or does it fit in to what we have?
  • Is the asynchronous functionality useful for us at all, can we use it to retire some of our own code?

/cc @nodejs/crypto @nodejs/lts

@indutny
Copy link
Member

indutny commented Dec 14, 2015

ChaCha!

@ChALkeR ChALkeR added the openssl Issues and PRs related to the OpenSSL dependency. label Dec 14, 2015
@indutny
Copy link
Member

indutny commented Dec 14, 2015

Answering to your questions:

  • IMO, we should not regret it. It seems to be safer to go without it
  • It will be clear later, when we will start working on this, but I think it probably add new APIs
  • What do you mean? We already use cert callback in place of our old code...

@Fishrock123 Fishrock123 added the discuss Issues opened for discussions and feedbacks. label Dec 14, 2015
@jasnell
Copy link
Member

jasnell commented Dec 14, 2015

Overall, I'd say that this is a better justification for postponing v6 just a bit to get this landed than the possibility of a V8 upgrade. If we can get this in without the schedule slipping past the first week of May, then I'd say let's do it.

@shigeki
Copy link
Contributor

shigeki commented Dec 16, 2015

I had some time to look around the features of openssl-1.1.0 today and can answer some of questions.

  • extended master secret (RFC7627):
    This feature is for against to the triple handshake attack pointed out in https://www.secure-resumption.com/. There described two attack scenarios, one is renegotiation with client auth and the other is channel id with tls-unique.
    The latter does not affect Node since it is not supported. I'm not confident that the former is safe for Node. We do not check the certificate change after renegotiation as Chrome does in https://chromium.googlesource.com/chromium/src.git/+/master/net/socket/ssl_client_socket_openssl.cc#1924 . But I think It is very complicated attack so its severity seems to be low.
    The extended master secret has already been enabled in Chrome stable and it just began to be enabled in Firefox. Unless a new attack comes out, I think we need not to be in a hurry to support it.
  • OCB and ChaCha20-Poly1305:
    Using AES-OCB with openssl api is nearly the same as that of AES-GCM so that I think we will need not change the crypto API. AES-CCM has a difference as discussed in crypto: API changes needed for AES Counter with CBC-MAC (CCM) support #2383.
    In TLS, we cannot use AES-OCB yet while ChaCha20-Poly1305 has already had pre-assigned cipher suites and can be used in 1.1.0-alpha. ChaCha20-Poly1305 is to be included as MIT ciphers in the forthcoming TLS1.3 so I think it is worth while to test it.
  • ASYNC_JOB in libcrypto:
    It seems to be a something like uv_queue_work to use crypto features in openssl. I'm not sure it has a benefit to use it in Node instead of libuv.

The API and ABI compatibilities between 1.1.0-pre and 1.0.2e are very low as shown in http://abi-laboratory.pro/tracker/objects_report/openssl/1.0.2e/1.1.0-pre1/report.html . The deployment of openssl-1.1.0 in OS distributions will be slow. As in #2783, those who is using shared openssl library bundled in OS would want to stick 1.0.2 even in the next LTS. We should discuss the timing to upgrade when 1.1.0 nearly comes to be official release.

@jbergstroem
Copy link
Member

@shigeki that (shared library upstream support) would be a perfect discussion for the build group to have with upstream packagers. I'll try to get that mail going during holidays.

@shigeki
Copy link
Contributor

shigeki commented Dec 16, 2015

@jbergstroem That's good to know that. Another concern about shared library is that we would have an issue if we applied a floating patch to openssl that leads inconsistent behavior. It should be minimum but we did it before for an unavoidable reason as in #923.

@jbergstroem
Copy link
Member

@shigeki then upstream will at least know why if/why it fails. In general, I think most of us agree that the less floating patches we have the better :)

@rvagg rvagg removed the ctc-agenda label Dec 23, 2015
@rvagg
Copy link
Member Author

rvagg commented Dec 23, 2015

CTC meeting discussion roughly concluded that we should not hold up v6 for the OpenSSL upgrade and upgrading to 1.1.0 so soon after its release would require someone making a very good case for doing so. There doesn't appear to be a strong appetite for going ahead with this upgrade with any haste given the amount of API breakage for the little feature gain. The incompatibilities caused with distribution versions is also a factor in the negative for this.

I suggest we leave this issue open for discussion because we'll upgrade eventually.

@jasnell jasnell added this to the 7.0.0 milestone May 1, 2016
@Fishrock123
Copy link
Contributor

What's the status of this?

@rvagg
Copy link
Member Author

rvagg commented May 31, 2016

Given that:

Version 1.1.0 will be supported until 2018-04-30.
Version 1.0.2 will be supported until 2019-12-31 (LTS).

I think we're just going to continue kicking this can down the road. It's more of a thing that we should probably experiment with rather than actually ship any time soon. There may be Linux distros out there that end up shipping with 1.1.0 and it'd be good to know what kind of hoops we need to jump through to get that working.

@kroeckx
Copy link

kroeckx commented Jun 27, 2016

As the Debian maintainer of openssl I will be moving to 1.1.0 soon after it's released, which shouldn't take long anymore.

@bricss
Copy link

bricss commented Aug 25, 2016

OpenSSL v1.1.0 officially landed -> https://www.openssl.org/news/openssl-1.1.0-notes.html

@indutny
Copy link
Member

indutny commented Aug 25, 2016

Did it?

@indutny
Copy link
Member

indutny commented Aug 25, 2016

Looks like it did! Let's update :) It has a fix for TLS session cache that we have here.

@kroeckx
Copy link

kroeckx commented Sep 1, 2016

So will you need any help with this?

@indutny
Copy link
Member

indutny commented Sep 1, 2016

@kroeckx at this point it is hard to tell. I suppose there may be some difficulties with regards to our use of some now-private fields.

@kroeckx
Copy link

kroeckx commented Sep 1, 2016 via email

@indutny
Copy link
Member

indutny commented Sep 1, 2016

@kroeckx oh, we'd appreciate your help and contributions on this! Please let me know if I can help you! 😉

@bnoordhuis
Copy link
Member

We had a lot of complaints from people that link against the system openssl when we upgraded from 1.0.1 to 1.0.2 (because their copy of openssl was at 1.0.1 and it broke their builds.) I don't think that is a reason not to upgrade but some fallout and griping should be expected.

We could do it for v7 but what is the plan for v6 LTS? Support-wise we're good with 1.0.2 but I bet the addition of chacha20-poly1305 is going to make a lot of people happy. I know it's been discussed some but I'm curious if perhaps people changed their minds in the mean time.

@kroeckx
Copy link

kroeckx commented Sep 2, 2016

I understand that you make binary releases that include
openssl binaries. Note that the plan is just to make it able to
build with the 1.1.0 version, that doesn't mean you should
change your distributed files to 1.0.2, but you could. The
ChaCha20-Poly1305 support can clearly be a motivation.

I'm not sure what broke when going from 1.0.1 to 1.0.2. They
really should have been compatible. The only thing I can imagine
is that it was build against a 1.0.2 version but they tried to run
it with a 1.0.1 version, you added support for some new functions
in 1.0.2 and their binaries didn't have those new functions.

With 1.1.0 the soname changed, and I think for windows the name of
the DLLs changed. That would mean that if it was build against
1.1.0 and they still had some 1.0.X version, but not the new
files, that they'd get an error that the library wasn't found
instead.

But I think it's kind of problematic they might not get new
openssl version.

Kurt

@AaronHarris
Copy link

As someone who was affected by builds not working when Node upgraded to OpenSSL 1.0.2, I can say that building on OpenSSL 1.1.0 will make things equally as broken, in a good way - relying on the system OpenSSL was generally not a good idea, and now that people have (hopefully) fixed their builds, moving to OpenSSL 1.1.0 should be not much different from the changes people had to make when moving to 1.0.2.

shigeki added a commit that referenced this issue Apr 9, 2018
After upgrading OpenSSL-1.1.0, header files depends on architectures
were changed. This fixes to copy all `deps/openssl/config/*.h' into
the install directory.

Fixes: #4270
PR-URL: #19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
shigeki added a commit that referenced this issue Apr 9, 2018
All version-specific methods were deprecated in OpenSSL 1.1.0 and
min/max versions explicitly need to be set.
This still keeps comptatible with JS and OpenSSL-1.0.2 APIs for now.

crypto, constants: add constant of OpenSSL-1.1.0

Several constants for OpenSSL-1.1.0 engine were removed and renamed in
OpenSSL-1.1.0. This added one renamed constant in order to have a
compatible feature with that of OpenSSL-1.0.2.
Other missed or new constants in OpenSSL-1.1.0 are not yet added.

crypto,tls,constants: remove OpenSSL1.0.2 support

This is semver-majar change so that we need not to have
compatibilities with older versions.

Fixes: #4270
PR-URL: #19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
shigeki added a commit that referenced this issue Apr 9, 2018
Fixes: #4270
PR-URL: #19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
shigeki pushed a commit that referenced this issue Apr 9, 2018
OpenSSL-1.1.0 requires the nasm assembler for building asm files on
Windows. This finds nasm at \Program Files\NASM\nasm.exe or
\ProgramFiles(x86)\NASM\nasm.exe in vcbuild.bat for users who did not
add its path in their enviroments.

Fixes: nodejs/build#1190
Fixes: #4270
PR-URL: #19794
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
shigeki added a commit to shigeki/node that referenced this issue Aug 14, 2018
This is a floating patch against OpenSSL-1.1.0 to generate asm files
with Makefile rules and it is to be submitted to the upstream.

Fixes: nodejs#4270
PR-URL: nodejs#19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
rvagg pushed a commit that referenced this issue Aug 15, 2018
This is a floating patch against OpenSSL-1.1.0 to generate asm files
with Makefile rules and it is to be submitted to the upstream.

Fixes: #4270
PR-URL: #19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
rvagg pushed a commit that referenced this issue Aug 16, 2018
This is a floating patch against OpenSSL-1.1.0 to generate asm files
with Makefile rules and it is to be submitted to the upstream.

Fixes: #4270
PR-URL: #19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
shigeki added a commit to shigeki/node that referenced this issue Nov 2, 2018
This is a floating patch against OpenSSL-1.1.0 to generate asm files
with Makefile rules and it is to be submitted to the upstream.

Fixes: nodejs#4270
PR-URL: nodejs#19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
sam-github pushed a commit to sam-github/node that referenced this issue Nov 20, 2018
This is a floating patch against OpenSSL-1.1.0 to generate asm files
with Makefile rules and it is to be submitted to the upstream.

Fixes: nodejs#4270
PR-URL: nodejs#19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
sam-github pushed a commit that referenced this issue Nov 22, 2018
This is a floating patch against OpenSSL-1.1.0 to generate asm files
with Makefile rules and it is to be submitted to the upstream.

Fixes: #4270
PR-URL: #19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

PR-URL: #24523
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
targos pushed a commit that referenced this issue Nov 24, 2018
This is a floating patch against OpenSSL-1.1.0 to generate asm files
with Makefile rules and it is to be submitted to the upstream.

Fixes: #4270
PR-URL: #19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

PR-URL: #24523
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
This is a floating patch against OpenSSL-1.1.0 to generate asm files
with Makefile rules and it is to be submitted to the upstream.

Fixes: nodejs#4270
PR-URL: nodejs#19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs#24523
Reviewed-By: Shigeki Ohtsu <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
sam-github pushed a commit that referenced this issue Mar 5, 2019
This is a floating patch against OpenSSL-1.1.1 to generate asm files
with Makefile rules.

PR-URL: #26327
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

Original:

Fixes: #4270
PR-URL: #19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
sam-github pushed a commit to sam-github/node that referenced this issue Apr 1, 2019
This is a floating patch against OpenSSL-1.1.1 to generate asm files
with Makefile rules.

PR-URL: nodejs#26327
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

Original:

Fixes: nodejs#4270
PR-URL: nodejs#19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
sam-github pushed a commit to sam-github/node that referenced this issue Apr 11, 2019
This is a floating patch against OpenSSL-1.1.1 to generate asm files
with Makefile rules.

PR-URL: nodejs#26327
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

Original:

Fixes: nodejs#4270
PR-URL: nodejs#19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BethGriggs pushed a commit that referenced this issue Apr 15, 2019
This is a floating patch against OpenSSL-1.1.1 to generate asm files
with Makefile rules.

Backport-PR-URL: #26951
PR-URL: #26327
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>

Original:

Fixes: #4270
PR-URL: #19794
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rod Vagg <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
BaochengSu added a commit to BaochengSu/node that referenced this issue Oct 22, 2020
Ported from
OpenSUSE:nodejs8-8.17.0-lp152.147.1:openssl_1_1_1.patch

Original commit message:

Backport OpenSSL 1.1.1 support, mostly be disabling TLS 1.3
Upstream commits:

commit 8dd8033
Author: Shigeki Ohtsu <[email protected]>
Date:   Wed Sep 12 17:34:24 2018 +0900

    tls: workaround handshakedone in renegotiation

    `SSL_CB_HANDSHAKE_START` and `SSL_CB_HANDSHAKE_DONE` are called
    sending HelloRequest in OpenSSL-1.1.1.
    We need to check whether this is in a renegotiation state or not.

    Backport-PR-URL: nodejs#26270
    PR-URL: nodejs#25381
    Reviewed-By: Daniel Bevenius <[email protected]>
    Reviewed-By: Shigeki Ohtsu <[email protected]>

commit 161dca7
Author: Sam Roberts <[email protected]>
Date:   Wed Nov 28 14:11:18 2018 -0800

    tls: re-define max supported version as 1.2

    Several secureProtocol strings allow any supported TLS version as the
    maximum, but our maximum supported protocol version is TLSv1.2 even if
    someone configures a build against an OpenSSL that supports TLSv1.3.

    Fixes: nodejs#24658

    PR-URL: nodejs#25024
    Reviewed-By: Richard Lau <[email protected]>
    Reviewed-By: Ben Noordhuis <[email protected]>
    Reviewed-By: Daniel Bevenius <[email protected]>
    Reviewed-By: Colin Ihrig <[email protected]>

Partial port, remain compatible with 1.0.2:

commit 970ce14
Author: Shigeki Ohtsu <[email protected]>
Date:   Wed Mar 14 14:26:55 2018 +0900

    crypto: remove deperecated methods of TLS version

    All version-specific methods were deprecated in OpenSSL 1.1.0 and
    min/max versions explicitly need to be set.
    This still keeps comptatible with JS and OpenSSL-1.0.2 APIs for now.

    crypto, constants: add constant of OpenSSL-1.1.0

    Several constants for OpenSSL-1.1.0 engine were removed and renamed in
    OpenSSL-1.1.0. This added one renamed constant in order to have a
    compatible feature with that of OpenSSL-1.0.2.
    Other missed or new constants in OpenSSL-1.1.0 are not yet added.

    crypto,tls,constants: remove OpenSSL1.0.2 support

    This is semver-majar change so that we need not to have
    compatibilities with older versions.

    Fixes: nodejs#4270
    PR-URL: nodejs#19794
    Reviewed-By: James M Snell <[email protected]>
    Reviewed-By: Rod Vagg <[email protected]>
    Reviewed-By: Michael Dawson <[email protected]>

Signed-off-by: Su Baocheng <[email protected]>
BaochengSu added a commit to BaochengSu/node that referenced this issue Jul 14, 2022
Ported from
OpenSUSE:nodejs8-8.17.0-lp152.147.1:openssl_1_1_1.patch

Original commit message:

Backport OpenSSL 1.1.1 support, mostly be disabling TLS 1.3
Upstream commits:

commit 8dd8033
Author: Shigeki Ohtsu <[email protected]>
Date:   Wed Sep 12 17:34:24 2018 +0900

    tls: workaround handshakedone in renegotiation

    `SSL_CB_HANDSHAKE_START` and `SSL_CB_HANDSHAKE_DONE` are called
    sending HelloRequest in OpenSSL-1.1.1.
    We need to check whether this is in a renegotiation state or not.

    Backport-PR-URL: nodejs#26270
    PR-URL: nodejs#25381
    Reviewed-By: Daniel Bevenius <[email protected]>
    Reviewed-By: Shigeki Ohtsu <[email protected]>

commit 161dca7
Author: Sam Roberts <[email protected]>
Date:   Wed Nov 28 14:11:18 2018 -0800

    tls: re-define max supported version as 1.2

    Several secureProtocol strings allow any supported TLS version as the
    maximum, but our maximum supported protocol version is TLSv1.2 even if
    someone configures a build against an OpenSSL that supports TLSv1.3.

    Fixes: nodejs#24658

    PR-URL: nodejs#25024
    Reviewed-By: Richard Lau <[email protected]>
    Reviewed-By: Ben Noordhuis <[email protected]>
    Reviewed-By: Daniel Bevenius <[email protected]>
    Reviewed-By: Colin Ihrig <[email protected]>

Partial port, remain compatible with 1.0.2:

commit 970ce14
Author: Shigeki Ohtsu <[email protected]>
Date:   Wed Mar 14 14:26:55 2018 +0900

    crypto: remove deperecated methods of TLS version

    All version-specific methods were deprecated in OpenSSL 1.1.0 and
    min/max versions explicitly need to be set.
    This still keeps comptatible with JS and OpenSSL-1.0.2 APIs for now.

    crypto, constants: add constant of OpenSSL-1.1.0

    Several constants for OpenSSL-1.1.0 engine were removed and renamed in
    OpenSSL-1.1.0. This added one renamed constant in order to have a
    compatible feature with that of OpenSSL-1.0.2.
    Other missed or new constants in OpenSSL-1.1.0 are not yet added.

    crypto,tls,constants: remove OpenSSL1.0.2 support

    This is semver-majar change so that we need not to have
    compatibilities with older versions.

    Fixes: nodejs#4270
    PR-URL: nodejs#19794
    Reviewed-By: James M Snell <[email protected]>
    Reviewed-By: Rod Vagg <[email protected]>
    Reviewed-By: Michael Dawson <[email protected]>

Signed-off-by: Su Baocheng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
openssl Issues and PRs related to the OpenSSL dependency.
Projects
None yet
Development

No branches or pull requests