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.

Sign up for GitHub

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

Jump to bottom

[v10.x backport] TLS1.3 (and dependent PRs) #27432

Closed
wants to merge 21 commits into from
Closed

[v10.x backport] TLS1.3 (and dependent PRs) #27432

wants to merge 21 commits into from

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Apr 26, 2019 •
edited
Loading

WIP to backport #26951 and any required deps.

Includes:

@nodejs-github-bot
Copy link
Collaborator

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3377

@nodejs-github-bot nodejs-github-bot added openssl Issues and PRs related to the OpenSSL dependency. v10.x labels Apr 26, 2019
@sam-github sam-github added v10.x and removed v10.x labels Apr 26, 2019
This was referenced Apr 26, 2019
Merged
Closed
@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 26, 2019
sam-github and others added 7 commits April 29, 2019 09:54
@sam-github
deps: update OpenSSL upgrade process
a868ebe 
Particularly, ensure that the commit messages are self-explanatory so
that reviewers can understand that the large commits are the result of a
simple repeatable process. This should make them easier to review.

See: nodejs#26327 (comment)

PR-URL: nodejs#26378
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@sam-github
deps: upgrade openssl sources to 1.1.1b
c80bff3 
This updates all sources in deps/openssl/openssl with openssl-1.1.1b.

PR-URL: nodejs#26327
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@sam-github
deps: openssl-1.1.1b no longer packages .gitignore
63aa831 
Since its not packaged, we don't have to delete it, and the Makefile
and update can become a (tiny) bit simpler.

PR-URL: nodejs#26327
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@shigeki @sam-github
deps: add ARM64 Windows support in openssl
1cea121 
This adds ARM64 Windows support in the OpenSSL build system.

Since OpenSSL's ARM64 Windows support does not have support for ASM--
that is, VC-WIN64-ARM inherits from VC-noCE-common which has no ASM
files--`openssl_no_asm.gypi` is always used for building. This
essentially forces the 'no-asm' Configure flag.

PR-URL: nodejs#26001
Fixes: nodejs#25998
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Signed-off-by: Beth Griggs <Bethany.Griggs@uk.ibm.com>
@shigeki @sam-github
deps: add s390 asm rules for OpenSSL-1.1.1
c2310c7 
This is a floating patch against OpenSSL-1.1.1 to generate asm files
with Makefile rules.

PR-URL: nodejs#25381
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Shigeki Ohtsu <ohtsu@ohtsu.org>
Backport-PR-URL: nodejs#25688
@sam-github
deps: update archs files for OpenSSL-1.1.1b
f54db0b 
`cd deps/openssl/config; make` updates all archs dependant files.

PR-URL: nodejs#26327
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@cjihrig @sam-github
tls: support changing credentials dynamically
f47e208 
This commit adds a setSecureContext() method to TLS servers. In
order to maintain backwards compatibility, the method takes the
options needed to create a new SecureContext, rather than an
instance of SecureContext.

Fixes: nodejs#4464
Refs: nodejs#10349
Refs: nodejs/help#603
Refs: nodejs#15115
PR-URL: nodejs#23644
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
sam-github and others added 12 commits April 29, 2019 11:21
@sam-github
tls: get the local certificate after tls handshake
5f5d3c9 
Add an API to get the local certificate chosen during TLS handshake from
the SSL context.

Fix: nodejs#24095

PR-URL: nodejs#24261
Fixes: nodejs#24095
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@sam-github
tls: fix initRead socket argument name
4a82835 
"wrapped" argument is the caller's "socket", not its "wrap", and its
referred to as "socket" in the comments, so call it that.

PR-URL: nodejs#25153
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@sam-github
tls: do not confuse session and session ID
78b42fc 
session ID was named session in C++ and key in JS, Name them after what
they are, as the 'newSession' event docs do.

PR-URL: nodejs#25153
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@sam-github
src: use consistent names for JSStream
a6635b2 
Its confusing to call a js class with a handle a "Wrap", usually it's
the C++ handle that is called a Wrap (tcp_wrap, tls_wrap, ...). Its
derived from Socket, and makes a JS stream look like a Socket, so call
it that. Also, remove use of lib/_stream_wrap.js so it can be deprecated
some time.

PR-URL: nodejs#25153
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@sam-github
tls: remove unused ocsp extension parsing
ae7c74c 
The OCSP info from parsing the TLS ClientHello has not been used since
550c263, remove it.

See: nodejs#1464

PR-URL: nodejs#25153
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@sam-github
src: in-source comments and minor TLS cleanups
6b327e5 
Renamed some internal C++ methods and properties for consistency, and
commented SSL I/O.

- Rename waiting_new_session_ after is_waiting_new_session(), instead of
  using reverse naming (new_session_wait_), and change "waiting" to
  "awaiting".
- Make TLSWrap::ClearIn() return void, the value is never used.
- Fix a getTicketKeys() cut-n-paste error. Since it doesn't use the
  arguments, remove them from the js wrapper.
- Remove call of setTicketKeys(getTicketKeys()), its a no-op.

PR-URL: nodejs#25713
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@sam-github
tls: introduce client 'session' event
2d25b65 
OpenSSL has supported async notification of sessions and tickets since
1.1.0 using SSL_CTX_sess_set_new_cb(), for all versions of TLS. Using
the async API is optional for TLS1.2 and below, but for TLS1.3 it will
be mandatory. Future-proof applications should start to use async
notification immediately. In the future, for TLS1.3, applications that
don't use the async API will silently, but gracefully, fail to resume
sessions and instead do a full handshake.

See: https://wiki.openssl.org/index.php/TLS1.3#Sessions

PR-URL: nodejs#25831
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
@addaleax @sam-github
tls: do not free cert in .getCertificate()
8c7406f 
The documentation of `SSL_get_certificate` states that it returns
an internal pointer that must not be freed by the caller.

Therefore, using a smart pointer to take ownership is incorrect.

Refs: https://man.openbsd.org/SSL_get_certificate.3
Refs: nodejs#24261
Fixes: https://github.com/nodejs-private/security/issues/217

PR-URL: nodejs#25490
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@sam-github
src: remove unused TLWrap::EnableTrace()
38838af 
Declaration is unused, it was added by mistake in 46c5c33.

PR-URL: nodejs#25861
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@sam-github
src: organize TLSWrap declarations by parent
d3c7020 
Make it clear which of the multiple interfaces a TLSWrap method is
implementing by grouping and commenting the related methods.

PR-URL: nodejs#25861
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@sam-github
tls: don't shadow the tls global with a local
1c3c9f3 
`tls` shadows the global `tls` require, and isn't indicative of the
arument type.

PR-URL: nodejs#25861
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@sam-github
src: const_cast is necessary for 1.1.1, not 0.9.7
750b906 
The const_cast used to be necessary for SSL_get_app_data() in OpenSSL
0.9.7, but node doesn't compile against OpenSSL versions that old.
However, now it's needed for the recently introduced
SSL_renegotiate_pending(), which is not const-correct as of 1.1.1a.

PR-URL: nodejs#25861
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@sam-github
src: refactor SSLError case statement
5febe41 
- Don't use both break and return simultaneously.
- Use case:/UNREACHABLE() to enforce that all cases are handled, instead
  of CHECK().

Backport-PR-URL: nodejs#25968
PR-URL: nodejs#25861
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@sam-github
tls: support "BEGIN TRUSTED CERTIFICATE" for ca:
1f65f18 
Support the same PEM certificate formats for the ca: option to
tls.createSecureContext() that are supported by openssl when loading a
CAfile.

Fixes: nodejs#24761

PR-URL: nodejs#24733
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@sam-github
Copy link
Contributor Author

I don't think the effort is worth it at this point. I'll wait until either more of the commit backlog has made it into 10.x, or there are users requesting TLS 1.3 specfically for 10.x.

@sam-github sam-github closed this May 1, 2019
@bnoordhuis
Copy link
Member

@sam-github How about back-porting some of the --tls-{min-max} switches? That would let people use them in startup scripts without having to worry about the Node.js version.

@bnoordhuis bnoordhuis mentioned this pull request May 7, 2019
Closed
@sam-github
Copy link
Contributor Author

sam-github commented May 7, 2019 via email

@bnoordhuis
Copy link
Member

I think erroring out is sufficient and appropriate. An error message that says something along the lines of "this version of Node.js doesn't support TLS v1.3 but Node.js v12.x does" might be nice but not essential.

@saurabh14nov
Copy link

hi @bnoordhuis / @sam-github , when will this fix be released and as part of which node version?

@sam-github
Copy link
Contributor Author

@saurabh14nov Which fix are you referring to?

TLS1.3 support isn't a fix, its a new feature. Its in 12.x and 11.x right now.

There is not currently a timeline or decision on TLS1.3 support in 10.x: #27432 (comment)

@saurabh14nov
Copy link

@sam-github, there was a discussion to support TLS1.0: bug #21088. I am facing the same with latest node version.

@sam-github
Copy link
Contributor Author

With latest version, use https://nodejs.org/api/cli.html#cli_tls_min_v1_0

@bnoordhuis bnoordhuis mentioned this pull request May 16, 2019
Closed
@sam-github sam-github mentioned this pull request May 29, 2019
Closed
4 tasks
@kissofire kissofire mentioned this pull request Jul 22, 2022
Open
@wedataintelligence wedataintelligence mentioned this pull request Jul 22, 2022
Open
@ron-a20 ron-a20 mentioned this pull request Jul 22, 2022
Open
@Dmarch28 Dmarch28 mentioned this pull request Jul 22, 2022
Open
@aliceUnhinged613 aliceUnhinged613 mentioned this pull request Jul 22, 2022
Open
@137717unity 137717unity mentioned this pull request Jul 22, 2022
Open
@baby636 baby636 mentioned this pull request Jul 22, 2022
Open
@molllyn1 molllyn1 mentioned this pull request Jul 23, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
openssl Issues and PRs related to the OpenSSL dependency. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sam-github @nodejs-github-bot @bnoordhuis @saurabh14nov @MylesBorins @shigeki @cjihrig @addaleax