Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34.3k
[v11.x backport] TLS1.3#26951
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
sam-github commented Mar 27, 2019 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
nodejs-github-bot commented Mar 27, 2019
sam-github commented Mar 28, 2019
@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 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 commented Mar 28, 2019
Remaining work (that I know about!):
|
sam-github commented Mar 28, 2019
@rvagg And I forgot to answer
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 commented Mar 28, 2019
sam-github commented Mar 28, 2019
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 commented Mar 28, 2019
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 commented Mar 29, 2019
Does this mean v11.x will no longer build against shared OpenSSL 1.1.0? |
sam-github commented Mar 29, 2019
@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 commented Mar 29, 2019
Uh oh!
There was an error while loading. Please reload this page.
refack commented Apr 15, 2019
Seems like this and #27132 were the only PRs affected 😌. |
refack commented Apr 15, 2019
If anyone is interested https://gist.github.com/refack/1ec020607baa346e9265554646274de5 is a |
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)
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 commented Apr 26, 2019
WIP backport: #27432 |
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
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
Do not allow the minimum protocol level to be set higher than the max protocol level. See: nodejs#26951, 109c097
Switch added in v11.x, add it to master/12.x for consistency and compatibility. See: nodejs#26951, commit bf2c283
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]>
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]>
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]>
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]>
PRs included in this backort:
Update openssl1.1.1b: Update openssl1.1.1b #26327, backported because openssl updates don't merge well, and always need backportingTLS1.3 support: TLS1.3 support #26209 (WIP), had to do some minor fixups to cherry-picktls: revert default max to TLSv1.2: this is new, it reverts the semver-major change introduced in the previous committls: add debugging to native TLS code: tls: add debugging to native TLS code #26843, cherry-picked clean on top ofTLS1.3 supportdoc: describe tls.DEFAULT_MIN_VERSION/_MAX_VERSION: doc: describe tls.DEFAULT_MIN_VERSION/_MAX_VERSION #26821, had to fix a minor conflict caused bytls: revert default max to TLSv1.2, which changed the default min/max from the values on mastertls: support shared openssl 1.1.0: this is new, it adds back support for openssl 1.1.0tls: add --tls-min-v1.2 CLI switch: this is new, it adds a CLI switch to change the min protocol to v1.2Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes