Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v11.x backport] TLS1.3 #26951

Closed
wants to merge 13 commits into from
Closed

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Mar 27, 2019

PRs included in this backort:

  • tls: support shared openssl 1.1.0: this is new, it adds back support for openssl 1.1.0
  • tls: add --tls-min-v1.2 CLI switch: this is new, it adds a CLI switch to change the min protocol to v1.2
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. tools Issues and PRs related to the tools directory. labels Mar 27, 2019
@sam-github sam-github changed the base branch from master to v11.x-staging March 28, 2019 00:02
@sam-github
Copy link
Contributor Author

@targos, do you have any suggestions as to how to backport a set of dependent PRs to 11.x?

TLS1.3 depends on openssl1.1.1b, as well as the #25093 and #24729, see #24729 (comment)

Can I just pick them all onto this branch as I find necessary, or will that make things hard for you?

I could also do the backports in series, but that is made difficult unless they are pretty promptly cherry-picked onto v11.x-staging, because I have to float the commits in a stand-alone PR-branch, but I also need them in this PR-branch so I can keep working.

I'm not sure what the normal way of doing this is.

Btw, the openssl1.1.1b part of this PR could be cherry-picked to #26949 right now, I believe. Should I PR that immediately? Its only a backport because we don't merge openssl updates back, we re-do the source commit and the config regeneration step from scratch.

@rvagg
Copy link
Member

rvagg commented Mar 28, 2019

@sam-github is this just "Drafting" because it doesn't have those dependent pieces? Other than that it's a straight backport of 1.3?

@sam-github
Copy link
Contributor Author

Remaining work (that I know about!):

  1. Fix the test failures, I'm working through them. 7 of the tests are because of the missing .code property, it looks like, thus the dependent backports. Still need to look at the other 3.
  2. Once the tests pass, I need to push one more commit, to change the TLS default max back to TLS1.2, and then make sure the tests still pass.

@sam-github
Copy link
Contributor Author

@rvagg And I forgot to answer

Other than that it's a straight backport of 1.3?

So far, yes. Some conflict resolution during cherry pick, but nothing remarkable. The change to the default TLS max is the only thing I expect to be different, and I hope it won't affect the test suite too much.

@nodejs-github-bot
Copy link
Collaborator

@sam-github sam-github marked this pull request as ready for review March 28, 2019 18:50
@sam-github
Copy link
Contributor Author

Passes locally, both with TLS1.3 as the default max, and with TLS1.2 as the default max. Lets see what CI thinks.

Depends on:

@targos
Copy link
Member

targos commented Mar 28, 2019

@sam-github

@targos, do you have any suggestions as to how to backport a set of dependent PRs to 11.x?

Whatever is easier for you. I like when multiple backports are bundled in the same PR, so from my PoV you could close the other backport PRs and we land everything when this is ready.

@mscdex
Copy link
Contributor

mscdex commented Mar 29, 2019

Does this mean v11.x will no longer build against shared OpenSSL 1.1.0?

@sam-github
Copy link
Contributor Author

@mscdex ATM it does, but I suspect losing support for shared openssl 1.1.0 is not acceptable as semver-minor, would you agree?

I'm going to try to fix that, I assume it will take only a few ifdefs in c++, and then a whole lot more checks in the test suites for TLS1.3 support.

Actually attempting to use 1.1.1 features when node.js when its built against 1.1.0 will not go well, which is expected (I guess) and we don't document what features are 1.1.1 specific. Is there any way to give a heads up to distributors? Or (@refack?) is there a way at ./configure time to warn that the --shared-openssl options are being used, but the shared openssl version is < openssl 1.1.1b, just so people know that the build will not be fully functional?

I'm not sure why it is that we ever allowed building against a shared library that is not the same version as the currently included openssl version. I'd speculate that 1.1.0 and 1.0.2 were so similar it didn't matter much, though even there I'd think that 1.1.0 would have had something that a node API user would have noticed.

@nodejs-github-bot
Copy link
Collaborator

@refack
Copy link
Contributor

refack commented Apr 15, 2019

Seems like this and #27132 were the only PRs affected 😌.
Sorry for the noise.

@refack refack mentioned this pull request Apr 15, 2019
2 tasks
@refack
Copy link
Contributor

refack commented Apr 15, 2019

If anyone is interested https://gist.github.com/refack/1ec020607baa346e9265554646274de5 is a pre-push hook to block git from creating or deleting branches on a /nodejs/ repo.

@sam-github sam-github deleted the tls1.3-v11.x branch April 15, 2019 16:51
@codebytere codebytere mentioned this pull request Apr 19, 2019
codebytere added a commit that referenced this pull request Apr 19, 2019
Notable changes:

* deps: add s390 asm rules for OpenSSL-1.1.1 (Shigeki Ohtsu) [#19794](#19794)
* src: add .code and SSL specific error properties (Sam Roberts) [#25093](#25093)
* tls:
  * add --tls-min-v1.2 CLI switch (Sam Roberts) [#26951](#26951)
  * supported shared openssl 1.1.0 (Sam Roberts) [#26951](#26951)
  * revert default max toTLSv1.2 (Sam Roberts) [#26951](#26951)
  * revert change to invalid protocol error type (Sam Roberts) [#26951](#26951)
  * support TLSv1.3 (Sam Roberts) [#26209](#26209)
  * add code for ERR\_TLS\_INVALID\_PROTOCOL\_METHOD (Sam Roberts) [#24729](#24729)
codebytere added a commit that referenced this pull request Apr 19, 2019
Notable changes:

* deps: add s390 asm rules for OpenSSL-1.1.1 (Shigeki Ohtsu) [#19794](#19794)
* src: add .code and SSL specific error properties (Sam Roberts) [#25093](#25093)
* tls:
  * add --tls-min-v1.2 CLI switch (Sam Roberts) [#26951](#26951)
  * supported shared openssl 1.1.0 (Sam Roberts) [#26951](#26951)
  * revert default max toTLSv1.2 (Sam Roberts) [#26951](#26951)
  * revert change to invalid protocol error type (Sam Roberts) [#26951](#26951)
  * support TLSv1.3 (Sam Roberts) [#26209](#26209)
  * add code for ERR\_TLS\_INVALID\_PROTOCOL\_METHOD (Sam Roberts) [#24729](#24729)
@sam-github sam-github restored the tls1.3-v11.x branch April 26, 2019 18:44
@sam-github
Copy link
Contributor Author

WIP backport: #27432

@sam-github sam-github deleted the tls1.3-v11.x branch April 26, 2019 19:58
codebytere added a commit that referenced this pull request Apr 30, 2019
Notable changes:

* deps: add s390 asm rules for OpenSSL-1.1.1 (Shigeki Ohtsu) [#19794](#19794)
* src: add .code and SSL specific error properties (Sam Roberts) [#25093](#25093)
* tls:
  * add --tls-min-v1.2 CLI switch (Sam Roberts) [#26951](#26951)
  * supported shared openssl 1.1.0 (Sam Roberts) [#26951](#26951)
  * revert default max toTLSv1.2 (Sam Roberts) [#26951](#26951)
  * revert change to invalid protocol error type (Sam Roberts) [#26951](#26951)
  * support TLSv1.3 (Sam Roberts) [#26209](#26209)
  * add code for ERR\_TLS\_INVALID\_PROTOCOL\_METHOD (Sam Roberts) [#24729](#24729)

PR-URL: #27314
codebytere added a commit that referenced this pull request Apr 30, 2019
Notable changes:

* deps: add s390 asm rules for OpenSSL-1.1.1 (Shigeki Ohtsu) [#19794](#19794)
* src: add .code and SSL specific error properties (Sam Roberts) [#25093](#25093)
* tls:
  * add --tls-min-v1.2 CLI switch (Sam Roberts) [#26951](#26951)
  * supported shared openssl 1.1.0 (Sam Roberts) [#26951](#26951)
  * revert default max toTLSv1.2 (Sam Roberts) [#26951](#26951)
  * revert change to invalid protocol error type (Sam Roberts) [#26951](#26951)
  * support TLSv1.3 (Sam Roberts) [#26209](#26209)
  * add code for ERR\_TLS\_INVALID\_PROTOCOL\_METHOD (Sam Roberts) [#24729](#24729)

PR-URL: #27314
sam-github added a commit to sam-github/node that referenced this pull request May 2, 2019
Do not allow the minimum protocol level to be set higher than the max
protocol level.

See: nodejs#26951, 109c097
sam-github added a commit to sam-github/node that referenced this pull request May 3, 2019
Switch added in v11.x, add it to master/12.x for consistency and
compatibility.

See: nodejs#26951, commit bf2c283
sam-github added a commit that referenced this pull request May 3, 2019
Do not allow the minimum protocol level to be set higher than the max
protocol level.

See: #26951, 109c097

PR-URL: #27521
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request May 4, 2019
Do not allow the minimum protocol level to be set higher than the max
protocol level.

See: #26951, 109c097

PR-URL: #27521
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Trott pushed a commit to Trott/io.js that referenced this pull request May 5, 2019
Switch added in v11.x, add it to master/12.x for consistency and
compatibility.

See: nodejs#26951, commit bf2c283

PR-URL: nodejs#27520
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
targos pushed a commit that referenced this pull request May 6, 2019
Switch added in v11.x, add it to master/12.x for consistency and
compatibility.

See: #26951, commit bf2c283

PR-URL: #27520
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. semver-minor PRs that contain new features and should be released in the next minor version. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants