@shigeki, how does this approach sound to you?

/cc @nodejs/crypto

/cc @nodejs/tsc
@jasnell should we deprecate any of the above mentioned bits in 9.x?

@davidben do you have an insight into if there is planned fips support for openssl 1.1.0? I know that has been a reason for us to not upgrade in the past

You'll have to ask OpenSSL folks about that one. But I don't think it makes sense to wait for it before supporting it at all, maybe just before shipping it in the bundled copy. Switching to 1.1.0 is a large change with deprecation consequences. You're better off getting things in sooner rather than later so you can plan for it before 1.0.2 goes EOL. (I intentionally did not switch the bundled copy in this PR.)

Moreover, folks like Linux distributions may switch before you do. They may just pick up the old PR which would be problematic as it didn't work right.

@MylesBorins I think we are at the point where regardless of FIPs validation status for 1.1.0 we will need to upgrade. I believe that version 8.x was the last one where openssl 1.0.2 would be supported long enough to use it in an LTS release.

@davidben thanks for putting this together. It is important that we have a path to 1.1.0 sooner than later. Based on the size of change, this may take a bit of time for the reviewers to be able to comment.

node 8.x and openssl 1.0.2 both go out of support in Dec, 2019

Interestingly, openssl 1.1.0 goes out of support in Aug, 2018, before 1.0.2.

We can use openssl 1.1.0 for node 9.x, because 9.x will be short-lived.

For node 10.x, which is LTS for us, we will want to use an openssl release that will be supported for its lifetime, I assume. They haven't released one yet, and I can't find anywhere where they commit to a release date for the next LTS openssl, though they say this:

We may designate a release as a Long Term Support (LTS) release. LTS releases will be supported for at least five years and we will specify one at least every four years.

Since 1.0.2 was released in jan 2015, if I understand correctly, there should be a LTS release of openssl by jan 2019. But hopefully they won't wait to the last minute, because node 10.x goes LTS in october 2018.

What would be most awkward for us would be if openssl 1.1.0 goes out of support while we are using it for 10.x and getting ready for LTS, and there is no replacement yet. What would we do, go back to openssl 1.0.2?

I strongly suspect that won't happen. That either 1.1.0 will become LTS by then, or that there will be a 1.2.0 by then and it will be called LTS, but I'm speculating.

cf. https://www.openssl.org/policies/releasestrat.html

I'm adding the TSC-Agenda label. Specifically I think the TSC should create a interest group specifically focused around coming up with some suggestions around how we can deal with this. We only have 6 months before the 10.x cut

I can't speak for OpenSSL, but I am sure either 1.1.1 will be released before 1.1.0 goes out of support or they will extend support for 1.1.0. It wouldn't make much sense for 1.0.2 to be the only supported OpenSSL release at any point. :-)

OpenSSL 1.1.1's release is essentially waiting on a final version of the TLS 1.3 specification from the IETF. There is no hard deadline on this happening, but I agree with @davidben that it would be silly for 1.0.2 to be the only supported OpenSSL release.
With respect to the FIPS module, I believe the status is that the old FIPS module is not great code and intentionally is not/will not be compatible with OpenSSL 1.1.x, but there is a partially funded endeavor to do a from-scratch rewrite for a new FIPS module that is compatible with the OpenSSL 1.1.x series. See, e.g., https://www.openssl.org/blog/blog/2017/07/25/fips/

/cc @danbev

An important issue we need to consider here is how packagers are dynamically linking Node to their own versions of OpenSSL. Maximising the options for them to do that is going to be in our interests because we're not going to stop them from doing this (dynamic linking and separating out dependant packages is strict policy in Debian-land for instance) and they're already shipping Node in ways that are not strictly compatible with core since we do some patching of OpenSSL in our own statically linked version that is never picked up downstream.

So, supporting both 1.0.2 and 1.1.0 as soon as possible makes a lot of sense, so I'd be keen to see this PR move forward even while we're tied primarily to 1.0.2. Packagers can then opt to compile against 1.1.0 if it suits their needs, or stick to 1.0.2 which is still going to be more common for Linux distros for a while yet. i.e. there's going to be an extended period where our downstream packagers are going to be (either forced to, or preferring to) opting for 1.1.x or 1.0.2 in their linking regardless of what we would prefer them to do and the less we help them the more they're going to be doing a hacky job of it.

Given that we're about to hit 9.x, we're unlikely to get any breaking changes in there so the ideal might look something like this:

• Land fully backward-compat 1.1.0 support some time during 9.x (is seems like this PR does that, but would need some careful confirmation of that)
• Move forward with some API adjustments that set us up better for 1.1.0, mostly as suggested in OP (they're all pretty good suggestions, comments & questions on some below)
• Consider a full switch to 1.1.x in 10.x due to LTS and support timeframe
• Consider leaving opt-in support for 1.0.2 for 10.x to leave that optionality for packagers since it's going to cause headaches on older distros to force 1.1.x support

We can fire up at least one dedicated 1.1.0 node in Jenkins like we have for FIPS now, it'd be great to be testing it multiple ways (ideally we'd also have dynamic-linking nodes in there too, we've talked about that for a while but never done it).

wrt to some of the specifics in OP:

• DSS1 - let's deprecate it during 9.x
• "Weak crypto" - we don't actually use any of these anywhere in core right, we just make some of them available to users, I'd be fine with deprecating during 9.x, however I can see a case for defaulting to enable-weak-ssl-ciphers and caveat emptor to users who wish to use them for whatever reason
• crypto.createHash('sha') / SHA-0 - deprecate during 9.x (potentially similar case to weak crypto above tho?)
• tlsSocket.getCipher().version - interesting to hear that it's basically useless as is now, let's deprecate during 9.x then since it's unlikely that anyone is using it for anything useful and if they are then they're ill-informed about what it does
• Error messages - maybe there's a case for combining it with our own new internal error messages stuff. I haven't looked at what 1.1.0 might have done wrt the internals of their error messages and dealing with them in 1.0.x is pretty disgusting, so maybe it'll be too hard to connect the two in a graceful way that lets us not be too worried about future error message breakage. @jasnell it'd be great if you had the headspace to look into this and advise?
• ecdhCurve:false - seems like a no-brainer to me to semver-major this asap, so deprecation in 9.x I suppose
• Ticket keys - I don't understand the implications of switching ticket key length, is there really going to be a problem going from 48 to 80 bytes? tbh I have no idea if/how that might break things in user-land, it doesn't sound like a problem to me but I'm far from expert on this topic.
• SSL_METHOD - I love the API suggestion here but we're going to need some userland metrics to try and get a handle on how/if it's used in the wild. Perhaps it'll be trivial to do a quick deprecate/semver-major cycle on this but my guess is that we'd want to be slow and therefore support old and new API for a little while.

Huge thanks to @davidben for this work and being so thorough in description and advice here!

To add to the distro thing, another reason to get 1.1.0 support in sooner rather than later is this way the support won't bitrot and you can test out all the ramifications while you still have time for the 9.x deprecation cycle. Everything I wrote in the OP was stuff I only realized working on this PR.

Land fully backward-compat 1.1.0 support some time during 9.x (is seems like this PR does that, but would need some careful confirmation of that)

Yup, the intent was that this PR has no user-visible changes when building against 1.0.2. (Though certainly this needs careful review to make sure I got it right!)

"Weak crypto" - we don't actually use any of these anywhere in core right, we just make some of them available to users

As far as I can tell, that's correct. The closest you all get to using some of it is in TLS tests used anonymous ciphers to avoid needing to configure certs. (This PR makes them configure certs instead.)

I haven't looked at what 1.1.0 might have done wrt the internals of their error messages

It's mostly that OpenSSL's errors are not very good at being stable. The most obvious change is that the function code switched from all caps to actually being the function code. And function codes existing at all mean renaming an internally OpenSSL function will change that part of the error string!

And then there are some minor differences just because some of the SSL code was reworked and invalid things are detected differently. (Version negotiation in particular goes through a very different codepath. OpenSSL 1.0.x's version negotiation logic was kind of weird. OpenSSL 1.1.0 does it better.)

Ticket key [...] is there really going to be a problem going from 48 to 80 bytes

Not a problem in itself but the server.setTicketKeys API expects the caller just know 48 is the magic number. It's documented as such and whatnot. There's also no other way to find out the expected key length, so I expect anyone using this API is indeed just hardcoding 48. (Unless they're querying some other context's server.getTicketKeys and passing it in, in which case they wouldn't care.)
https://nodejs.org/api/tls.html#tls_server_setticketkeys_keys

It's possible the answer is just exposing some constant[*] which queries the key length and asking callers to use that rather than a hardcoded 48? In that case, you probably could avoid the relevant commit in this patch and do a deprecation cycle? (I can adjust this PR to drop the "compatibility hack" commits you all don't end up needing. I erred on the side on including them here so it'd be easier to see what the options were.)

[*] Or something? The OpenSSL API to query it is per-SSL_CTX so, in theory, they could decide to make ticket key sizes differ based on SSL_METHOD or random configuration??? Though it's just a constant right now.

SSL_METHOD [...] my guess is that we'd want to be slow and therefore support old and new API for a little while.

Yeah, there's no rush here. You get a mess of deprecated API warnings, but it builds fine.

https://nodejs.org/api/tls.html#tls_server_setticketkeys_keys

Note: The key's Buffer should be 48 bytes long. See ticketKeys option in tls.createServer for more information on how it is used.

I see the problem now, we've hardwired it in our API and docs, that's a bit unfortunate. I'm not seeing an obvious best way around this. A constant could be backported to 8.x and maybe even 6.x so users could start querying it. The awkward part is that we throw if ! 48 (env->ThrowTypeError("Ticket keys length must be 48 bytes")) and it seems to me from your patch that it'd be too late to insert the callback at setTicketKeys() only if we get 48 bytes because that'd be the nicer way to approach this and then we could more slowly deprecate 48-byte keys. So, unless we want to be stuck with the old behaviour then we're going to have to force a break here. This is an API we're not going to get good ecosystem usage numbers because it's most likely used in applications rather than libraries. My hunch is that it's got very limited usage. IIRC it was PayPal that drove interest in this and I haven't seen much of anyone else talk about ticket keys than @indutny.

Next step would be some reviews by people with more of a clue. @indutny, @bnoordhuis, @shigeki, maybe @sam-github? Would you mind taking a look over the code? It's a surprisingly small diff and shouldn't take too long. I've had a look over it and will play more before I give a +1.

It'll also need some testing actually compiling against 1.1.0 across our platforms so we'll have to come up with a strategy that's a little more comprehensive than just adding a single 1.1.0 box into the mix.

@davidben would you mind rebasing off master so we can run this in CI? Currently having a problem with that: https://ci.nodejs.org/job/node-test-commit/13351/console

I've addressed the comments, rebased to master, and removed the original ecdhCurve commit. Instead, I've added fd6658e99db128f4879cefa70630b98fe57b28aa to suppress the test in OpenSSL 1.1.0. I'm not sure how to make that as deprecated, so I'll probably need some help from one of you all there. (Or should that be done separately?)

I've left the ticket one alone for the time being, since it sounds like that's still being discussed. I also realize now I missed the "DSS1" one... shall I remove that workaround in favor of a deprecation + test suppression like ecdhCurve?

FYI https://ci.nodejs.org/job/node-test-commit/13361/ CI is happy with this as is sans the linting problem in test/parallel/test-tls-ecdh-disable.js

I'm playing with 1.1.0 as a local (non-global) build to test this out and I have a few notes.

First, for @davidben, I'm getting 2 failures, not just parallel/test-http2-create-client-connect but also parallel/test-tls-close-notify on Linux and macOS. This is using the current HEAD of openssl/openssl:

    Error: 140737282147264:error:140E0197:SSL routines:SSL_shutdown:shutdown while in init:ssl/ssl_lib.c:1988:
    140737282147264:error:140E0197:SSL routines:SSL_shutdown:shutdown while in init:ssl/ssl_lib.c:1988:

Notes, mainly for me and for anyone else wanting to test this out for themselves without having to install OpenSSL globally:

• test: pass process.env to child processes #16405 is required to get make some of the tests happy (env not passed, so LD_LIBRARY_PATH doesn't work)
• Need to grab OpenSSL from source because 1.1.0g isn't released yet and this patch set requires it
• Using OpenSSL unpacked inside the Node directory as openssl-1.1.0g/
  • (cd openssl-1.1.0g && ./config --prefix=$(pwd)/out/ --openssldir=$(pwd)/out/ && make -j4 && make install)
  • ./configure --shared-openssl --shared-openssl-includes=$(pwd)/openssl-1.1.0g/out/include/ --shared-openssl-libpath=$(pwd)/openssl-1.1.0g/out/lib/ && make -j4
  • export LD_LIBRARY_PATH=$(pwd)/openssl-1.1.0g/out
  • export DYLD_LIBRARY_PATH=$(pwd)/openssl-1.1.0g/out (macOS)
  • export PATH=$(pwd)/openssl-1.1.0g/out/bin/:$PATH (for tests using the openssl CLI)
  • make test

My thinking here is that we could test in bulk across most of our CI by either checking in a openssl-1.1.0g/ source directory or just downloading it and unpacking it, then testing with a modified Makefile that does all of these steps. That wouldn't require any CI modification or special machines.

A step beyond that for a more permanent test-dynamically-linked-openssl might be to bake in these steps into CI somewhere so they can be run on any of our hosts without requiring a special host like we do for FIPS. Or perhaps it's sufficient to just have a sub-job that reuses some of the other hosts to do this work across a small subset of our platforms. I'll take this up with the Build WG because it'd be great to start testing dynamic linking in our CI. We could then conceivably extend to some of the other shared libs we support.

I'll try to get to the other comments later today or this week, but to answer the immediate test issues real quick:

So, OpenSSL master is slated to be 1.1.1, not 1.1.0g. I've been testing with the OpenSSL_1_1_0-stable branch, on the assumption that 1.1.1 adding TLS 1.3 might throw off others of your tests (though I see it's off by default) and dealing with that later would be simpler. But clearly I should have tested master too, given this new failure!

The immediate cause for this is openssl/openssl#4574, however I think ultimately it's Node that's misbehaving. You all are calling into user code within the OpenSSL info callback, which might re-entrantly call into more code. OpenSSL, 1.1.x or 1.0.x, is unlikely to behave well in all scenarios where SSL_read calls the handshake calls info_callback calls Node calls SSL_read again. TLSWrap too is also unlikely to be happy if random methods are called reentrantly from ClearOut or ClearIn.

Some part of the assembly leaking up to TLSSocket.prototype._finishInit calling this.emit('secure') should probably be deferred an event loop iteration. Though it would also be weird if some read or write callback runs just before the 'secure' event, rather than just after, so we may want to be a bit careful there.

(Yeah, parallel/test-http2-create-client-connect failing is a known issue. Sorry I haven't gotten to writing up the details there yet. Just finished a much-needed vacation the past week, so I've got quite a backlog. :-) )

Release team were -1 on backporting to 6.x, but cautiously +1 on backporting to 8.x.

It would be great to have feedback on:

• whether this will be helpful (e.g. for package maintainers)
• risk of backporting to 8.x
• whether anyone is willing to do the work to backport

For info on the backporting process, see the guide

There is a working set of patches for 8.9.3 here the order of which can be found in this file.
Notice the "fixups.patch" file which i did not care to find which commit it comes from, and the "s_client_tls12.patch" which i believe is debian-related.

@kapouer does this mean that Debian is already using this for Node 8.x. If so that seems like a good reason to backport.

Yes it does in debian/unstable and soon testing.

I'm working on similar patch for fedora, though test/parallel/test-tls-no-sslv3 always fails, even v9.x/master fails.

=== release test-tls-no-sslv3 ===                                              
Path: parallel/test-tls-no-sslv3
CONNECTED(00000003)
---
no peer certificate available
---
No client certificate CA names sent
---
SSL handshake has read 7 bytes and written 80 bytes
Verification: OK
---
New, (NONE), Cipher is (NONE)
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
SSL-Session:
    Protocol  : SSLv3
    Cipher    : 0000
    Session-ID: 
    Session-ID-ctx: 
    Master-Key: 
    PSK identity: None
    PSK identity hint: None
    SRP username: None
    Start Time: 1516204799
    Timeout   : 7200 (sec)
    Verify return code: 0 (ok)
    Extended master secret: no
---
140662277297984:error:14094410:SSL routines:ssl3_read_bytes:sslv3 alert handshake failure:ssl/record/rec_layer_s3.c:1399:SSL alert number 40
assert.js:42
  throw new errors.AssertionError({
  ^

AssertionError [ERR_ASSERTION]: false == true
    at process.<anonymous> (/root/node/test/parallel/test-tls-no-sslv3.js:50:5)
    at emitOne (events.js:121:20)
    at process.emit (events.js:211:7)
Command: out/Release/node /root/node/test/parallel/test-tls-no-sslv3.js

Here are my patches for Fedora Rawhide for v8.9.4

Thanks @kasicka. I had to do some manual work to get the backports to land (specifically where there were conflicts). I got a working backport together though and opened a PR #18326