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

tls: secureConnect event will not be emitted if TLSSocket is created via constructor instead of tls.connect #10555

Closed
nbdd0121 opened this issue Dec 31, 2016 · 7 comments
Labels
doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem.

Comments

@nbdd0121
Copy link

  • Version: 7.3
  • Platform: Any
  • Subsystem: Any

In current TLS doc, secureConnect is documented as an event of TLSSocket. However, the event will only be emitted if the socket is created via tls.connect. Basically there is no sensible event to listen on for the 'connect' event if the socket is created directly via new tls.TLSSocket.

@addaleax addaleax added the tls Issues and PRs related to the tls subsystem. label Dec 31, 2016
@sam-github sam-github added the doc Issues and PRs related to the documentations. label Jan 16, 2017
@sam-github
Copy link
Contributor

Confirmed. I will document.

@sam-github
Copy link
Contributor

PTAL at #10846

sam-github added a commit to sam-github/node that referenced this issue Feb 20, 2017
Direct use of tls.TLSSocket to start a TLS session over an existing TCP connection was documented.

However, to use this connection securely it is necessary to validate and
authenticate the peer's certificate, and the documented events and
properties are implemented only for TLSSockets returned by
tls.connect(). In order to create secure connections, additional
undocumented APIs must be used, and these APIs are being called right
now by npm modules.

Fix: nodejs#10555
Fix: nodejs#11467
@Trott
Copy link
Member

Trott commented Jul 16, 2017

@sam-github Should this issue remain open?

@sam-github
Copy link
Contributor

Yes, our documentation is incorrect, and parts of the TLS API are unuseable now (at least, not useable with documented methods and events). I tried to fix, but #10846 was rejected.

@BridgeAR
Copy link
Member

I guess #10846 would be a good starting point to fix this. It was just closed due to being stalled and did not get rejected.

@jasnell
Copy link
Member

jasnell commented Oct 19, 2018

Ping @sam-github ... would you be interested in taking another run at #10846?

jasnell added a commit to jasnell/node that referenced this issue Nov 1, 2018
deprecate the legacy undocumented `.ssl` alias for the
`TLSSocket._handle` and document alternatives. Document
how to properly use the `TLSSocket` constructor directly.

Updated take on nodejs#10846

Fixes: nodejs#10555
jasnell added a commit to jasnell/node that referenced this issue Dec 31, 2020
Fixes: nodejs#10555
Refs: nodejs#10846

The `new tls.TLSSocket()` constructor does not set up all of the
necessary lifecycle management or event handlers necessary for
proper use. The `tls.connect()` method really should be the way
that all `tls.TLSSocket()` instances are created. This commit
begins the eventual phasing out of the `new tls.TLSSocket()`
constructor with a doc-only deprecation.

Signed-off-by: James M Snell <[email protected]>
jasnell added a commit to jasnell/node that referenced this issue Apr 28, 2021
@targos targos closed this as completed in 99099f9 May 2, 2021
targos pushed a commit that referenced this issue May 3, 2021
Fixes: #10555
Signed-off-by: James M Snell <[email protected]>
Refs: #10846

PR-URL: #38447
Reviewed-By: Alba Mendez <[email protected]>
Reviewed-By: Adrian Estrada <[email protected]>
@Antonius-S
Copy link

I wonder why on Earth this underwater stone exists. IMHO it only increases WTF-per-minute value.

targos pushed a commit that referenced this issue May 30, 2021
Fixes: #10555
Signed-off-by: James M Snell <[email protected]>
Refs: #10846

PR-URL: #38447
Reviewed-By: Alba Mendez <[email protected]>
Reviewed-By: Adrian Estrada <[email protected]>
targos pushed a commit that referenced this issue Jun 5, 2021
Fixes: #10555
Signed-off-by: James M Snell <[email protected]>
Refs: #10846

PR-URL: #38447
Reviewed-By: Alba Mendez <[email protected]>
Reviewed-By: Adrian Estrada <[email protected]>
targos pushed a commit that referenced this issue Jun 5, 2021
Fixes: #10555
Signed-off-by: James M Snell <[email protected]>
Refs: #10846

PR-URL: #38447
Reviewed-By: Alba Mendez <[email protected]>
Reviewed-By: Adrian Estrada <[email protected]>
targos pushed a commit that referenced this issue Jun 11, 2021
Fixes: #10555
Signed-off-by: James M Snell <[email protected]>
Refs: #10846

PR-URL: #38447
Reviewed-By: Alba Mendez <[email protected]>
Reviewed-By: Adrian Estrada <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. tls Issues and PRs related to the tls subsystem.
Projects
None yet
7 participants