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 both OpenSSL 1.1.0 and 1.0.2 #16130

Closed
wants to merge 26 commits into
base: master
from

Conversation

@davidben
Contributor

davidben commented Oct 10, 2017

WARNING: This is a rather large change.

In a previous PR, it was suggested that Node was uninterested in incremental changes for OpenSSL 1.1.0 support, with the worry that OPENSSL_VERSION_NUMBER ifdefs in the code will appear as if you support 1.1.0 when you don't. However, doing it all in one commit is also not feasible as one cannot possibly review such a change. I've instead attempted a middle ground where there is one PR, but it is made up of many many individual commits. It's recommended that you review each commit individually and not use Github's combined diff UI.

Let me know if you would like the branch presented differently. (Go back to separate PRs, etc.)

This was partially adapted from PR #8491 but fixes some issues with it. This PR does not switch Node to OpenSSL 1.1.0, nor does it break support with OpenSSL 1.0.2. Two notes:

  • OpenSSL needs openssl/openssl#4384 applied. This means OpenSSL 1.1.0g or later, which is not yet released. Since it would otherwise compile but not work, I've added a OPENSSL_VERSION_NUMBER check to catch this.

  • test-http2-create-client-connect does not work. I've fixed an issue with the test itself, but it still doesn't pass in 1.1.0. The HTTP/2 code bypasses the usual JS afterWrite code which, combined with some weird OpenSSL behaviors, causes the error-handling to be a little off. It actually affects 1.0.2, just not on that particular test's input. I'll file a separate ticket for all that, but since it affects 1.0.2 too, I figure that should be separate to this PR. I'll put together a report with details later.

Additionally, switching to OpenSSL 1.1.0 comes with a number of removals. OpenSSL has documentation here:

https://www.openssl.org/news/openssl-1.1.0-notes.html
https://www.openssl.org/news/changelog.html#x7

I've described what I did notice below. Where it was feasible, I added a change to retain compatibility in Node's API as much as possible. I'll let you all decide exactly which of those you wish to include. For instance, one could imagine adding 1.1.0 support (so it doesn't bitrot), but still shipping with 1.0.2 for a deprecation cycle, and then switching to 1.1.0 with all the breakages that entails. That would let you drop some of the commits in this branch.

DSS1

In OpenSSL 1.0.2, the digest name "DSS1" was a DSA alias for SHA-1, part of the EVP_Sign* legacy. This has been removed in OpenSSL 1.1.0. In crypto: add compatibility logic for "DSS1" and "dss1", I've added a compatibility knob for it, but it may be better to skip this commit and instead deprecate the name, since there are other unavoidable removals anyway.

Weak crypto

OpenSSL 1.1.0 disables a number of weak ciphers like RC4 or anonymous ciphers by default. You can build OpenSSL with enable-weak-ssl-ciphers if you wish to restore them. If you do, you probably want a deprecation cycle.

OpenSSL 1.1.0 also rejects RSA keys smaller than 1024 bits by default and other settings. See https://www.openssl.org/docs/man1.1.0/ssl/SSL_CTX_get_security_level.html

SHA-0

SHA-0 was removed in OpenSSL 1.1.0. This is an insecure and virtually unused early variant of SHA-1, but Node did nominally expose it via crypto.createHash('sha'). Short of adding a compatibility reimplementation of the hash, I think this is an unavoidable removal in OpenSSL 1.1.0.

tlsSocket.getCipher().version

The version field returned by tlsSocket.getCipher() did not do what was documented in 1.0.2 and always returned the string "TLSv1/SSLv3". I've retained that behavior in Node and fixed the documentation, but you all may wish to go through a deprecation cycle with that field. It's not useful anyway.

See the commit crypto: hard-code tlsSocket.getCipher().version for details.

Error messages

Some of the errors OpenSSL emits are slightly differnent. I had to fix a few strings in tests. Dunno how much you care about this sort of behavior change.

ecdhCurve

The ecdhCurve server setting in the TLS module is defined to disable ECDH. This is a remnant of an implementation quirk in OpenSSL where it lacked curve negotiation logic. That meant there exists an ECDH-less state for OpenSSL servers separate from the cipher list. (Note no such mode exists on the client.) They fixed this and added this "ecdh_auto" opt-in knob in 1.0.2. In 1.1.0, this opt-in knob was made always on. Unfortunately, Node exposed this state by setting ecdhCurve to false.

I've hacked it back in in the comment crypto: fix setting ecdhCurve to false with OpenSSL 1.1.0 by appending to the user's cipher list. Your call whether you wish to keep this. Since everything in TLS without ECDHE is insecure anyway, I would suggest taking a deprecation cycle to remove this instead and dropping the commit.

Ticket keys

OpenSSL 1.0.2 used a 16 byte key name, 16 byte AES-128-CBC key, and 16 byte HMAC-SHA256 key for session tickets. OpenSSL 1.1.0 switches this to a 16 byte key name, 32 byte AES-256-CBC key, and 32 byte HMAC-SHA256 key. One can query the size of the ticket key with SSL_CTX_get_tlsext_ticket_keys to avoid the hard-coded 48, but that doesn't really solve the problem. That ticket keys are 48 bytes are exposed in Node's public APIs.

In crypto: emulate OpenSSL 1.0.x ticket scheme in 1.1.x, I've implemented the callback to use the old scheme in OpenSSL 1.1.0. This works, but you may wish to think about alternate APIs (maybe expose a constant somewhere for the ticket key size) that allow the ticket scheme to change.

SSL_METHOD and protocol versions

OpenSSL's old SSL_METHODs are kind of a mess. You had methods like TLSv1_1_client_method which locked you to TLS 1.1 and only worked for clients. Then you had methods like TLSv1_1_method which were exactly the same as TLSv1_1_client_method except you could use them for either clients or server. Then you had SSLv23_method which was actually version-flexible and misnamed. Then, to configure versions, you had toggles like SSL_OP_NO_TLSv1_1.

This is a mess. In OpenSSL 1.1.0, things are much better. SSLv23_method is renamed to TLS_method and then you have APIs like SSL_CTX_set_min_proto_version and SSL_CTX_set_max_proto_version. TLS_client_method and TLS_server_method sadly still exist and are approximately as pointless as they always were. :-)

Unfortunately, Node's APIs thoroughly expose this and has the user specify the string name of the method to use, rather than min/max version configurations. OpenSSL 1.1.0 retains the old APIs but marks them as deprecated, so this still works but you have a lot oc compile warnings. I have not attempted to remove the deprecated APIs in this branch as I think you all need a better API first.

My suggestion:

  1. Deprecate the secureProtocol option.
  2. Introduce option like, perhaps, minVersion and maxVersion.
  3. Once secureProtocol is completely removed, stop using the version-locked methods and tripping the deprecation warnings.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

crypto

@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Oct 10, 2017

Contributor

@shigeki, how does this approach sound to you?

Contributor

davidben commented Oct 10, 2017

@shigeki, how does this approach sound to you?

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex
Contributor

mscdex commented Oct 10, 2017

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 11, 2017

Member

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

Member

MylesBorins commented Oct 11, 2017

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

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 11, 2017

Member

@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

Member

MylesBorins commented Oct 11, 2017

@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

@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Oct 11, 2017

Contributor

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.

Contributor

davidben commented Oct 11, 2017

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.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Oct 11, 2017

Member

@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.

Member

mhdawson commented Oct 11, 2017

@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.

@mhdawson

This comment has been minimized.

Show comment
Hide comment
@mhdawson

mhdawson Oct 11, 2017

Member

@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.

Member

mhdawson commented Oct 11, 2017

@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.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Oct 11, 2017

Member

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

Member

sam-github commented Oct 11, 2017

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

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Oct 11, 2017

Member

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

Member

MylesBorins commented Oct 11, 2017

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

@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Oct 11, 2017

Contributor

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

Contributor

davidben commented Oct 11, 2017

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

@kaduk

This comment has been minimized.

Show comment
Hide comment
@kaduk

kaduk Oct 12, 2017

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/

kaduk commented Oct 12, 2017

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/

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax
Member

addaleax commented Oct 14, 2017

/cc @danbev

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Oct 20, 2017

Member

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!

Member

rvagg commented Oct 20, 2017

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!

@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Oct 20, 2017

Contributor

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.

Contributor

davidben commented Oct 20, 2017

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.

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Oct 21, 2017

Member

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

Member

rvagg commented Oct 21, 2017

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

@bnoordhuis

Mostly LGTM. I agree on deprecating ecdhCurve and that there probably isn't much risk in changing the ticket keys API (and defensible for v9.x because it's ultimately openssl that drives this change.)

X509_STORE* store = SSL_CTX_get_cert_store(ctx);
X509_STORE_CTX store_ctx;
ret = X509_STORE_CTX_init(&store_ctx, store, nullptr, nullptr);

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Can you explain in the commit log why this no longer works? X509_STORE_CTX_init() and X509_STORE_CTX_cleanup() still exist in 1.1.0, don't they?

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Can you explain in the commit log why this no longer works? X509_STORE_CTX_init() and X509_STORE_CTX_cleanup() still exist in 1.1.0, don't they?

This comment has been minimized.

@davidben

davidben Oct 21, 2017

Contributor

Yeah, I'm not really sure why they still exist TBH. Some weird form of reset I guess. The reason this is needed is X509_STORE_CTX is opaque and thus cannot be stack-allocated. (I can add a note in the commit message to clarify this.)

@davidben

davidben Oct 21, 2017

Contributor

Yeah, I'm not really sure why they still exist TBH. Some weird form of reset I guess. The reason this is needed is X509_STORE_CTX is opaque and thus cannot be stack-allocated. (I can add a note in the commit message to clarify this.)

Show outdated Hide outdated src/node_crypto_bio.cc Outdated
Show outdated Hide outdated src/node_crypto_bio.cc Outdated
Show outdated Hide outdated src/node_crypto.cc Outdated
Show outdated Hide outdated src/node_crypto.cc Outdated
assert.ok(
/SSL routines:SSL23_GET_CLIENT_HELLO:unknown protocol/.test(e.message),
/SSL routines:[^:]*:(unknown protocol|wrong version number)/.test(
e.message),

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Tiny nit: can you indent by four spaces here and below? (Line continuation.)

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Tiny nit: can you indent by four spaces here and below? (Line continuation.)

This comment has been minimized.

@davidben

davidben Oct 21, 2017

Contributor

eslint gets unhappy and seems to really want this indent.

@davidben

davidben Oct 21, 2017

Contributor

eslint gets unhappy and seems to really want this indent.

This comment has been minimized.

@apapirovski

apapirovski Oct 21, 2017

Member

2 spaces is the standard line continuation for our JS style guide. This seems fine to me but if you really, really wanted to make it a bit less awkward because of the length then maybe just put the RegExp into a variable... ?

@apapirovski

apapirovski Oct 21, 2017

Member

2 spaces is the standard line continuation for our JS style guide. This seems fine to me but if you really, really wanted to make it a bit less awkward because of the length then maybe just put the RegExp into a variable... ?

@@ -1084,8 +1084,10 @@ void SecureContext::SetECDHCurve(const FunctionCallbackInfo<Value>& args) {
node::Utf8Value curve(env->isolate(), args[0]);
#if OPENSSL_VERSION_NUMBER < 0x10100000L
SSL_CTX_set_options(sc->ctx_, SSL_OP_SINGLE_ECDH_USE);
SSL_CTX_set_ecdh_auto(sc->ctx_, 1);

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Somewhat amusingly, we only started doing that last month. Is there a better way of doing that with openssl 1.0.2? I couldn't find one.

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Somewhat amusingly, we only started doing that last month. Is there a better way of doing that with openssl 1.0.2? I couldn't find one.

This comment has been minimized.

@davidben

davidben Oct 21, 2017

Contributor

Nah, that's the way to do it. They added and deprecated APIs one release after another. :-) Both of those calls are no-ops in 1.1.0. The ifdef isn't needed, but it silences some build warnings, so I figured I'd clear them.

@davidben

davidben Oct 21, 2017

Contributor

Nah, that's the way to do it. They added and deprecated APIs one release after another. :-) Both of those calls are no-ops in 1.1.0. The ifdef isn't needed, but it silences some build warnings, so I figured I'd clear them.

Show outdated Hide outdated test/parallel/test-tls-econnreset.js Outdated
@@ -7,20 +7,22 @@ if (!common.hasIPv6)
common.skip('no IPv6 support');
const assert = require('assert');
const fixtures = require('../common/fixtures');

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Not needed, available as common.fixtures

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Not needed, available as common.fixtures

This comment has been minimized.

@davidben

davidben Oct 21, 2017

Contributor

Hrm. That doesn't seem to work. I also don't see common.fixtures in any other test. There's common.fixturesDir, though that just points to the directory. Is that plus fs.readFileSync plus path-munging preferred over fixtures.readKey?

@davidben

davidben Oct 21, 2017

Contributor

Hrm. That doesn't seem to work. I also don't see common.fixtures in any other test. There's common.fixturesDir, though that just points to the directory. Is that plus fs.readFileSync plus path-munging preferred over fixtures.readKey?

This comment has been minimized.

@apapirovski

apapirovski Oct 21, 2017

Member

This seems fine as is. It matches what people were instructed to use in the latest Code & Learn.

@apapirovski

apapirovski Oct 21, 2017

Member

This seems fine as is. It matches what people were instructed to use in the latest Code & Learn.

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Sorry, you're both right. I thought (don't know why) that common.js re-exported everything from fixtures.js.

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Sorry, you're both right. I thought (don't know why) that common.js re-exported everything from fixtures.js.

Show outdated Hide outdated test/parallel/test-https-agent-session-eviction.js Outdated
@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Oct 21, 2017

Contributor

I've addressed the comments, rebased to master, and removed the original ecdhCurve commit. Instead, I've added fd6658e 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?

Contributor

davidben commented Oct 21, 2017

I've addressed the comments, rebased to master, and removed the original ecdhCurve commit. Instead, I've added fd6658e 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?

@bnoordhuis

Thanks, basically LGTM pending two questions.

I'm not sure how to make that as deprecated, so I'll probably need some help from one of you all there.

Untested but this diff shows the basic principle:

diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
index 75c5c0feb5..62c0a7a0ce 100644
--- a/doc/api/deprecations.md
+++ b/doc/api/deprecations.md
@@ -737,6 +737,13 @@ Type: Runtime
 internal mechanics of the `REPLServer` itself, and is therefore not
 necessary in user space.

+<a id="DEP0083"></a>
+### DEP0083: Disabling ECDH with `{ ecdhCurve: false }`
+
+Type: Runtime
+
+Blurb goes here.
+

 [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
 [`Buffer.from(array)`]: buffer.html#buffer_class_method_buffer_from_array
diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index d7e349b239..ff2fa178d8 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -919,6 +919,15 @@ Server.prototype.setTicketKeys = function setTicketKeys(keys) {
 };


+function ecdhCurveWarning() {
+  if (ecdhCurveWarning.emitted) return;
+  process.emitWarning('{ ecdhCurve: false } is a deprecated no-op.',
+                      'DeprecationWarning',
+                      'DEP0083');
+  ecdhCurveWarning.emitted = true;
+}
+ecdhCurveWarning.emitted = false;
+
 Server.prototype.setOptions = function(options) {
   this.requestCert = options.requestCert === true;
   this.rejectUnauthorized = options.rejectUnauthorized !== false;
@@ -931,8 +940,10 @@ Server.prototype.setOptions = function(options) {
   if (options.secureProtocol) this.secureProtocol = options.secureProtocol;
   if (options.crl) this.crl = options.crl;
   if (options.ciphers) this.ciphers = options.ciphers;
-  if (options.ecdhCurve !== undefined)
+  if (options.ecdhCurve !== undefined) {
     this.ecdhCurve = options.ecdhCurve;
+    if (this.ecdhCurve === false) ecdhCurveWarning();
+  }
   if (options.dhparam) this.dhparam = options.dhparam;
   if (options.sessionTimeout) this.sessionTimeout = options.sessionTimeout;
   if (options.ticketKeys) this.ticketKeys = options.ticketKeys;

I also realize now I missed the "DSS1" one... shall I remove that workaround in favor of a deprecation + test suppression like ecdhCurve?

No strong opinion. It doesn't cost much to keep around and logging deprecation warnings from C++ is kind of cumbersome because you have to thread through the node::Environment. The function you are looking for is ProcessEmitWarning() if you want to pursue that.

sc->ticket_key_aes_, iv) <= 0 ||
HMAC_Init_ex(hctx, sc->ticket_key_hmac_, sizeof(sc->ticket_key_hmac_),
EVP_sha256(), nullptr) <= 0) {
return -1;

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Doesn't this leak the resources allocated by EVP_EncryptInit_ex() when HMAC_Init_ex() fails?

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Doesn't this leak the resources allocated by EVP_EncryptInit_ex() when HMAC_Init_ex() fails?

This comment has been minimized.

@davidben

davidben Nov 2, 2017

Contributor

It does not. hctx is owned by the caller, so it's meant to get cleaned up after this function returns.

@davidben

davidben Nov 2, 2017

Contributor

It does not. hctx is owned by the caller, so it's meant to get cleaned up after this function returns.

This comment has been minimized.

@bnoordhuis

bnoordhuis Nov 3, 2017

Member

Ah, I found the corresponding logic in t1_lib.c and s3_srvr.c. Okay, thanks for clearing that up.

@bnoordhuis

bnoordhuis Nov 3, 2017

Member

Ah, I found the corresponding logic in t1_lib.c and s3_srvr.c. Okay, thanks for clearing that up.

if (EVP_DecryptInit_ex(ectx, EVP_aes_128_cbc(), nullptr, sc->ticket_key_aes_,
iv) <= 0 ||
HMAC_Init_ex(hctx, sc->ticket_key_hmac_, sizeof(sc->ticket_key_hmac_),
EVP_sha256(), nullptr) <= 0) {

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Same question.

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Same question.

This comment has been minimized.

@davidben

davidben Nov 2, 2017

Contributor

(ditto.)

@davidben

davidben Nov 2, 2017

Contributor

(ditto.)

Show outdated Hide outdated src/node_crypto.cc Outdated
@@ -7,20 +7,22 @@ if (!common.hasIPv6)
common.skip('no IPv6 support');
const assert = require('assert');
const fixtures = require('../common/fixtures');

This comment has been minimized.

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Sorry, you're both right. I thought (don't know why) that common.js re-exported everything from fixtures.js.

@bnoordhuis

bnoordhuis Oct 21, 2017

Member

Sorry, you're both right. I thought (don't know why) that common.js re-exported everything from fixtures.js.

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Oct 22, 2017

Member

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

Member

rvagg commented Oct 22, 2017

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

@rvagg rvagg referenced this pull request Oct 23, 2017

Closed

test: pass process.env to child processes #16405

3 of 3 tasks complete
@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Oct 23, 2017

Member

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:

  • #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.

Member

rvagg commented Oct 23, 2017

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:

  • #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.

@mhdawson mhdawson referenced this pull request Oct 23, 2017

Open

test issue #2

@davidben

This comment has been minimized.

Show comment
Hide comment
@davidben

davidben Oct 23, 2017

Contributor

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

Contributor

davidben commented Oct 23, 2017

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

@mhdawson mhdawson referenced this pull request Oct 23, 2017

Open

test issue #3

gibfahn added a commit that referenced this pull request Feb 18, 2018

crypto: remove locking callbacks for OpenSSL 1.1.0
The callbacks are all no-ops in OpenSSL 1.1.0. This isn't necessary (the
functions still exist for compatibility), but silences some warnings and
avoids allocating a few unused mutexes.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

crypto: make CipherBase 1.1.0-compatible
In OpenSSL 1.1.0, EVP_CIPHER_CTX must be heap-allocated. Once we're
heap-allocating them, there's no need in a separate initialised_ bit.
The presence of ctx_ is sufficient.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

crypto: make Hash 1.1.0-compatible
OpenSSL 1.1.0 requires EVP_MD_CTX be heap-allocated.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

crypto: make SignBase compatible with OpenSSL 1.1.0
1.1.0 requires EVP_MD_CTX be heap-allocated. In doing so, move the Init
and Update hooks to shared code because they are the same between Verify
and Sign.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

crypto: Make Hmac 1.1.0-compatible
OpenSSL 1.1.0 requries HMAC_CTX be heap-allocated.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

crypto: add compat logic for "DSS1" and "dss1"
In OpenSSL 1.1.0, EVP_dss1() is removed. These hash names were exposed
in Node's public API, so add compatibility hooks for them.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

crypto: hard-code tlsSocket.getCipher().version
This aligns the documentation with reality. This API never did what Node
claims it did.

The SSL_CIPHER_get_version function just isn't useful. In OpenSSL 1.0.2,
it always returned the string "TLSv1/SSLv3" for anything but SSLv2
ciphers, which Node does not support. Note how test-tls-multi-pfx.js
claims that ECDHE-ECDSA-AES256-GCM-SHA384 was added in TLSv1/SSLv3 which
is not true. That cipher is new as of TLS 1.2. The OpenSSL 1.0.2
implementation is:

char *SSL_CIPHER_get_version(const SSL_CIPHER *c)
{
    int i;

    if (c == NULL)
        return ("(NONE)");
    i = (int)(c->id >> 24L);
    if (i == 3)
        return ("TLSv1/SSLv3");
    else if (i == 2)
        return ("SSLv2");
    else
        return ("unknown");
}

In OpenSSL 1.1.0, SSL_CIPHER_get_version changed to actually behave as
Node documented it, but this changes the semantics of the function and
breaks tests. The cipher's minimum protocol version is not a useful
notion to return to the caller here, so just hardcode the string at
"TLSv1/SSLv3" and document it as legacy.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

test: update test expectations for OpenSSL 1.1.0
Some errors in the two versions are different. The test-tls-no-sslv3 one
because OpenSSL 1.1.x finally does version negotiation properly. 1.0.x's
logic was somewhat weird and resulted in very inconsistent errors for
SSLv3 in particular.

Also the function codes are capitalized differently, but function codes
leak implementation details, so don't assert on them to begin with.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

test: remove sha from test expectations
"sha" in OpenSSL refers to SHA-0 which was removed from OpenSSL 1.1.0
and is insecure. Replace it with SHA-256 which is present in both 1.0.2
and 1.1.0. Short of shipping a reimplementation in Node, this is an
unavoidable behavior change with 1.1.0.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

crypto: emulate OpenSSL 1.0 ticket scheme in 1.1
OpenSSL 1.0.x used a 48-byte ticket key, but OpenSSL 1.1.x made it
larger by using a larger HMAC-SHA256 key and using AES-256-CBC to
encrypt. However, Node's public API exposes the 48-byte key. Implement
the ticket key callback to restore the OpenSSL 1.0.x behavior.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

test: test with a larger RSA key
OpenSSL 1.1.0 rejects RSA keys smaller than 1024 bits by default. Fix
the tests to use larger ones. This test only cares that the PEM blob be
missing a trailing newline. Certificate adapted from
test/fixtures/cert.pem.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

test: revise test-tls-econnreset for OpenSSL 1.1.0
This test is testing what happens to the server if the client shuts off
the connection (so the server sees ECONNRESET), but the way it does it
is convoluted. It uses a static RSA key exchange with a tiny (384-bit)
RSA key. The server doesn't notice (since it is static RSA, the client
acts on the key first), so the client tries to encrypt a premaster and
fails:

  rsa routines:RSA_padding_add_PKCS1_type_2:data too large for key size
  SSL routines:ssl3_send_client_key_exchange:bad rsa encrypt

OpenSSL happens not to send an alert in this case, so we get ECONNRESET
with no alert. This is quite fragile and, notably, breaks in OpenSSL
1.1.0 now that small RSA keys are rejected by libssl. Instead, test by
just connecting a TCP socket and immediately closing it.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

crypto: remove deprecated ECDH calls w/ OpenSSL 1.1
These are both no-ops in OpenSSL 1.1.0.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

test: configure certs in tests
OpenSSL 1.1.0 disables anonymous ciphers unless building with
enable-weak-crypto. Avoid unnecessary dependencies on these ciphers in
tests.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

test: fix test-https-agent-session-eviction for 1.1
This test is testing the workaround for an OpenSSL 1.0.x bug, which was
fixed in 1.1.0. With the bug fixed, the test expectations need to change
slightly.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

crypto: make ALPN the same for OpenSSL 1.0.2 & 1.1.0
This is kind of hairy. OpenSSL 1.0.2 ignored the return value and always
treated everything as SSL_TLSEXT_ERR_NOACK (so the comment was wrong and
Node was never sending a warning alert). OpenSSL 1.1.0 honors
SSL_TLSEXT_ERR_NOACK vs SSL_TLSEXT_ERR_FATAL_ALERT and treats everything
unknown as SSL_TLSEXT_ERR_FATAL_ALERT.

Since this is a behavior change (tests break too), start by aligning
everything on SSL_TLSEXT_ERR_NOACK. If sending no_application_protocol
is desirable in the future, this can by changed to
SSL_TLSEXT_ERR_FATAL_ALERT with whatever deprecation process is
appropriate.

However, note that, contrary to
https://rt.openssl.org/Ticket/Display.html?id=3463#txn-54498,
SSL_TLSEXT_ERR_FATAL_ALERT is *not* useful to a server with no fallback
protocol. Even if such mismatches were rejected, such a server must
*still* account for the fallback protocol case when the client does not
advertise ALPN at all. Thus this may not be worth bothering.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Feb 18, 2018

crypto: clear some SSL_METHOD deprecation warnings
Fixing the rest will be rather involved. I think the cleanest option is
to deprecate the method string APIs which are weird to begin with.

PR-URL: #16130
Backport-PR-URL: #18622
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rod Vagg <rod@vagg.org>

gibfahn added a commit that referenced this pull request Mar 6, 2018

2018-03-06 Version 8.10.0 'Carbon' (LTS)
Notable changes:

* deps:
  * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413)
  * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413)
  * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260)
  * re land npm 5.6.0 (Myles Borins) [#18625](#18625)
  * ICU 60 bump (Steven R. Loomis) [#16876](#16876)
* crypto:
  * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130)
  * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566)
* async_hooks:
  * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004)
  * use typed array stack as fast path (Anna Henningsen) [#17780](#17780)
  * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273)
  * separate missing from default context (Andreas Madsen) [#17273](#17273)
  * rename initTriggerId (Andreas Madsen) [#17273](#17273)
  * deprecate undocumented API (Andreas Madsen) [#16972](#16972)
  * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998)
  * add trace events to async_hooks (Andreas Madsen) [#15538](#15538)
  * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003)
  * add provider types for net server (Andreas Madsen) [#17157](#17157)
* n-api:
  * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109)
* cli:
  * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495)
* console:
  * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033)
* module:
  * add builtinModules (Jon Moss) [#16386](#16386)
  * replace default paths in require.resolve() (cjihrig) [#17113](#17113)
* src:
  * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109)
  * add process.ppid (cjihrig) [#16839](#16839)
* http:
  * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267)
  * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672)
  * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772)
* net:
  * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662)
* process:
  * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463)
* tls:
  * unconsume stream on destroy (Anna Henningsen) [#17478](#17478)
* process:
  * improve unhandled rejection message (Madara Uchiha) [#17158](#17158)
* stream:
  * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860)
* trace_events:
  * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196)

PR-URL: #18336

gibfahn added a commit that referenced this pull request Mar 7, 2018

2018-03-06 Version 8.10.0 'Carbon' (LTS)
Notable changes:

* deps:
  * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413)
  * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413)
  * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260)
  * re land npm 5.6.0 (Myles Borins) [#18625](#18625)
  * ICU 60 bump (Steven R. Loomis) [#16876](#16876)
* crypto:
  * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130)
  * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566)
* async_hooks:
  * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004)
  * use typed array stack as fast path (Anna Henningsen) [#17780](#17780)
  * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273)
  * separate missing from default context (Andreas Madsen) [#17273](#17273)
  * rename initTriggerId (Andreas Madsen) [#17273](#17273)
  * deprecate undocumented API (Andreas Madsen) [#16972](#16972)
  * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998)
  * add trace events to async_hooks (Andreas Madsen) [#15538](#15538)
  * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003)
  * add provider types for net server (Andreas Madsen) [#17157](#17157)
* n-api:
  * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109)
* cli:
  * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495)
* console:
  * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033)
* module:
  * add builtinModules (Jon Moss) [#16386](#16386)
  * replace default paths in require.resolve() (cjihrig) [#17113](#17113)
* src:
  * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109)
  * add process.ppid (cjihrig) [#16839](#16839)
* http:
  * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267)
  * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672)
  * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772)
* net:
  * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662)
* process:
  * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463)
* tls:
  * unconsume stream on destroy (Anna Henningsen) [#17478](#17478)
* process:
  * improve unhandled rejection message (Madara Uchiha) [#17158](#17158)
* stream:
  * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860)
* trace_events:
  * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196)

PR-URL: #18336

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

2018-03-06 Version 8.10.0 'Carbon' (LTS)
Notable changes:

* deps:
  * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413)
  * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413)
  * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260)
  * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625)
  * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876)
* crypto:
  * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130)
  * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566)
* async_hooks:
  * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004)
  * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780)
  * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273)
  * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273)
  * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273)
  * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972)
  * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998)
  * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538)
  * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003)
  * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157)
* n-api:
  * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109)
* cli:
  * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495)
* console:
  * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033)
* module:
  * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386)
  * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113)
* src:
  * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109)
  * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839)
* http:
  * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267)
  * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672)
  * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772)
* net:
  * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662)
* process:
  * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463)
* tls:
  * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478)
* process:
  * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158)
* stream:
  * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860)
* trace_events:
  * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196)

PR-URL: nodejs#18336

tniessen added a commit to tniessen/node that referenced this pull request Sep 19, 2018

tls: fix DEP0083 after upgrading to OpenSSL 1.1.0
Setting ecdhCurve to false is already unsupported, so the deprecation
should already be EOL. The test was skipped ever since we upgraded to
OpenSSL 1.1.0.

Refs: nodejs#16130

tniessen added a commit to tniessen/node that referenced this pull request Sep 19, 2018

tls: fix DEP0083 after upgrading to OpenSSL 1.1.0
Setting ecdhCurve to false is already unsupported, so the deprecation
should already be EOL. The test was skipped ever since we upgraded to
OpenSSL 1.1.0.

Refs: nodejs#16130

@tniessen tniessen referenced this pull request Sep 19, 2018

Closed

tls: fix DEP0083 after upgrading to OpenSSL 1.1.0 #22953

4 of 4 tasks complete

danbev added a commit that referenced this pull request Sep 24, 2018

tls: fix DEP0083 after upgrading to OpenSSL 1.1.0
Setting ecdhCurve to false is already unsupported, so the deprecation
should already be EOL. The test was skipped ever since we upgraded to
OpenSSL 1.1.0.

PR-URL: #22953
Refs: #16130
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

targos added a commit that referenced this pull request Sep 24, 2018

tls: fix DEP0083 after upgrading to OpenSSL 1.1.0
Setting ecdhCurve to false is already unsupported, so the deprecation
should already be EOL. The test was skipped ever since we upgraded to
OpenSSL 1.1.0.

PR-URL: #22953
Refs: #16130
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment