From 4ca0e4c949307d7e93e797b1a2cb4c54e6af0dc5 Mon Sep 17 00:00:00 2001
From: James M Snell <jasnell@gmail.com>
Date: Fri, 26 Oct 2018 15:46:07 -0700
Subject: [PATCH] tls: deprecate undocumented .ssl property, add docs

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 https://github.com/nodejs/node/pull/10846

Fixes: https://github.com/nodejs/node/issues/10555
---
 doc/api/deprecations.md                       | 10 +++
 doc/api/tls.md                                | 73 ++++++++++++++++++-
 lib/_tls_wrap.js                              | 35 ++++++---
 .../test-tls-socket-default-options.js        |  2 +-
 .../test-tls-socket-ssl-deprecated.js         | 31 ++++++++
 test/parallel/test-tls-tlswrap-segfault.js    |  4 +-
 6 files changed, 141 insertions(+), 14 deletions(-)
 create mode 100644 test/parallel/test-tls-socket-ssl-deprecated.js

diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
index 681fac92deae99..7ea3139b181d9b 100644
--- a/doc/api/deprecations.md
+++ b/doc/api/deprecations.md
@@ -2292,6 +2292,16 @@ Type: Runtime
 
 Please use `Server.prototype.setSecureContext()` instead.
 
+<a id="DEP00XX"></a>
+### DEP00XX: TLSSocket.prototype.ssl
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/23915
+    description: Runtime deprecation.
+-->
+
+The undocumented `tlsSocket.ssl` property has been deprecated.
 
 [`--pending-deprecation`]: cli.html#cli_pending_deprecation
 [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
diff --git a/doc/api/tls.md b/doc/api/tls.md
index 8700839c400b38..c318d753661834 100644
--- a/doc/api/tls.md
+++ b/doc/api/tls.md
@@ -465,7 +465,8 @@ changes:
 * `socket` {net.Socket|stream.Duplex}
   On the server side, any `Duplex` stream. On the client side, any
   instance of [`net.Socket`][] (for generic `Duplex` stream support
-  on the client side, [`tls.connect()`][] must be used).
+  on the client side, [`tls.connect()`][] must be used). If the `socket`
+  is not provided, a new unconnected TCP socket or named pipe will be created.
 * `options` {Object}
   * `isServer`: The SSL/TLS protocol is asymmetrical, TLSSockets must know if
     they are to behave as a server or a client. If `true` the TLS socket will be
@@ -489,7 +490,24 @@ changes:
   * ...: [`tls.createSecureContext()`][] options that are used if the
     `secureContext` option is missing. Otherwise, they are ignored.
 
-Construct a new `tls.TLSSocket` object from an existing TCP socket.
+Construct a new `tls.TLSSocket` object from an existing `net.Socket` instance.
+Using the `tls.connect()` method is preferred when creating a new TLS session
+on top of a new `net.Socket` instance.
+
+Using the `tls.TLSSocket()` constructor directly is helpful when implementing
+protocols that can start off insecure (such as SMTP), then initiating a secure
+session after the socket is connected. It is important to remember, however,
+that it is the caller's responsibility to manage the lifecycle of the provided
+`net.Socket`, including establishing the connection and validating peer
+certificates and identity. See the [`'secure'`][] event.
+
+### Event: 'connect'
+<!-- YAML
+added: v0.11.4
+-->
+
+The `'connect'` event is emitted once the underlying `net.Socket` has been
+connected.
 
 ### Event: 'OCSPResponse'
 <!-- YAML
@@ -505,6 +523,30 @@ The listener callback is passed a single argument when called:
 Typically, the `response` is a digitally signed object from the server's CA that
 contains information about server's certificate revocation status.
 
+### Event: 'secure'
+<!-- YAML
+added: v0.11.3
+-->
+
+The `'secure'` event is emitted after the TLS handshake has been completed.
+
+Before using the connection, the user must verify that the peer certificate
+is valid (see [`tls.TLSSocket.verifyError()`][]) and that the peer certificate
+is for the expected host (see [`tls.checkServerIdentity()`][] and
+[`tls.TLSSocket.getPeerCertificate()`][]). If these checks are not performed,
+the connection should be considered insecure. When using the `tls.connect()`
+method to create a new `tls.TLSSocket`, these checks are performed
+automatically.
+
+```js
+tlsSocket.on('secure', function() {
+  const err = this.verifyError() ||
+    tls.checkServerIdentity(hostname, this.getPeerCertificate());
+  if (err)
+    this.destroy(err);
+});
+```
+
 ### Event: 'secureConnect'
 <!-- YAML
 added: v0.11.4
@@ -520,6 +562,9 @@ determine if the server certificate was signed by one of the specified CAs. If
 `tlsSocket.alpnProtocol` property can be checked to determine the negotiated
 protocol.
 
+The `'secureConnect'` event is only emitted on `tls.TLSSocket` connections
+created using the `tls.connect()` method.
+
 ### tlsSocket.address()
 <!-- YAML
 added: v0.11.4
@@ -539,6 +584,9 @@ added: v0.11.4
 Returns the reason why the peer's certificate was not been verified. This
 property is set only when `tlsSocket.authorized === false`.
 
+The `tlsSocket.authorizationError` property is only set for `tls.TLSSocket`
+instances created using the `tls.connect()` method.
+
 ### tlsSocket.authorized
 <!-- YAML
 added: v0.11.4
@@ -549,6 +597,9 @@ added: v0.11.4
 Returns `true` if the peer certificate was signed by one of the CAs specified
 when creating the `tls.TLSSocket` instance, otherwise `false`.
 
+The `tlsSocket.authorized` property is only set for `tls.TLSSocket` instances
+created using the `tls.connect()` method.
+
 ### tlsSocket.disableRenegotiation()
 <!-- YAML
 added: v8.4.0
@@ -805,6 +856,21 @@ and their processing can be delayed due to packet loss or reordering. However,
 smaller fragments add extra TLS framing bytes and CPU overhead, which may
 decrease overall server throughput.
 
+### tlsSocket.verifyError()
+<!-- YAML
+added: v0.11.3
+-->
+
+* Returns: {Error} object if the peer's certificate fails validation.
+
+Validation contains many checks, including verification that the certificate
+is either trusted or can be chained to a trusted certificate authority
+(see the `ca` option of [`tls.createSecureContext()`][] for more information).
+
+Validation explicitly does *not* include any authentication of the identity.
+The [`tls.checkServerIdentity()`][] method can be used to authenticate the
+identity of the peer.
+
 ## tls.checkServerIdentity(hostname, cert)
 <!-- YAML
 added: v0.8.4
@@ -1389,6 +1455,7 @@ secureSocket = tls.TLSSocket(socket, options);
 
 where `secureSocket` has the same API as `pair.cleartext`.
 
+[`'secure'`]: #tls_event_secure
 [`'secureConnect'`]: #tls_event_secureconnect
 [`'secureConnection'`]: #tls_event_secureconnection
 [`SSL_CTX_set_timeout`]: https://www.openssl.org/docs/man1.1.0/ssl/SSL_CTX_set_timeout.html
@@ -1403,6 +1470,8 @@ where `secureSocket` has the same API as `pair.cleartext`.
 [`tls.Server`]: #tls_class_tls_server
 [`tls.TLSSocket.getPeerCertificate()`]: #tls_tlssocket_getpeercertificate_detailed
 [`tls.TLSSocket`]: #tls_class_tls_tlssocket
+[`tls.TLSSocket.verifyError()`]: #tls_tlssocket_verifyerror
+[`tls.checkServerIdentity()`]: #tls_tls_checkserveridentity_hostname_cert
 [`tls.connect()`]: #tls_tls_connect_options_callback
 [`tls.createSecureContext()`]: #tls_tls_createsecurecontext_options
 [`tls.createSecurePair()`]: #tls_tls_createsecurepair_context_isserver_requestcert_rejectunauthorized_options
diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index f0d86f3d870f09..bc16dfd6009112 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -56,6 +56,7 @@ const kErrorEmitted = Symbol('error-emitted');
 const kHandshakeTimeout = Symbol('handshake-timeout');
 const kRes = Symbol('res');
 const kSNICallback = Symbol('snicallback');
+const kSSL = Symbol('ssl');
 
 const noop = () => {};
 
@@ -325,7 +326,17 @@ function TLSSocket(socket, opts) {
   });
 
   // Proxy for API compatibility
-  this.ssl = this._handle;
+  this[kSSL] = this._handle;
+  Object.defineProperty(this, 'ssl', {
+    enumerable: true,
+    configurable: true,
+    get: util.deprecate(() => this[kSSL],
+                        'The tlsSocket.ssl property is deprecated.',
+                        'DEP00XX'),
+    set: util.deprecate((val) => this[kSSL] = val,
+                        'The tlsSocket.ssl property is deprecated.',
+                        'DEP00XX')
+  });
 
   this.on('error', this._tlsError);
 
@@ -366,8 +377,8 @@ for (var n = 0; n < proxiedMethods.length; n++) {
 tls_wrap.TLSWrap.prototype.close = function close(cb) {
   let ssl;
   if (this[owner_symbol]) {
-    ssl = this[owner_symbol].ssl;
-    this[owner_symbol].ssl = null;
+    ssl = this[owner_symbol][kSSL];
+    this[owner_symbol][kSSL] = null;
   }
 
   // Invoke `destroySSL` on close to clean up possibly pending write requests
@@ -457,13 +468,13 @@ function destroySSL(self) {
 }
 
 TLSSocket.prototype._destroySSL = function _destroySSL() {
-  if (!this.ssl) return;
-  this.ssl.destroySSL();
-  if (this.ssl._secureContext.singleUse) {
-    this.ssl._secureContext.context.close();
-    this.ssl._secureContext.context = null;
+  if (!this[kSSL]) return;
+  this[kSSL].destroySSL();
+  if (this[kSSL]._secureContext.singleUse) {
+    this[kSSL]._secureContext.context.close();
+    this[kSSL]._secureContext.context = null;
   }
-  this.ssl = null;
+  this[kSSL] = null;
 };
 
 TLSSocket.prototype._init = function(socket, wrap) {
@@ -586,6 +597,12 @@ TLSSocket.prototype.renegotiate = function(options, callback) {
   return true;
 };
 
+TLSSocket.prototype.verifyError = function verifyError(...args) {
+  if (this.destroyed)
+    return;
+  return this._handle.verifyError(...args);
+};
+
 TLSSocket.prototype.setMaxSendFragment = function setMaxSendFragment(size) {
   return this._handle.setMaxSendFragment(size) === 1;
 };
diff --git a/test/parallel/test-tls-socket-default-options.js b/test/parallel/test-tls-socket-default-options.js
index f086377b0faa20..afddc4514f3413 100644
--- a/test/parallel/test-tls-socket-default-options.js
+++ b/test/parallel/test-tls-socket-default-options.js
@@ -61,7 +61,7 @@ function test(client, callback) {
         this.end('hello');
       }))
       .on('secure', common.mustCall(function() {
-        callback(this.ssl.verifyError());
+        callback(this.verifyError());
       }));
   });
 }
diff --git a/test/parallel/test-tls-socket-ssl-deprecated.js b/test/parallel/test-tls-socket-ssl-deprecated.js
new file mode 100644
index 00000000000000..c50485ca96269e
--- /dev/null
+++ b/test/parallel/test-tls-socket-ssl-deprecated.js
@@ -0,0 +1,31 @@
+// Flags: --no-warnings
+'use strict';
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+
+const key = fixtures.readKey('agent2-key.pem');
+const cert = fixtures.readKey('agent2-cert.pem');
+
+const server = tls.createServer({ key, cert }, (socket) => {
+  socket.ssl;  // This should trigger a deprecation warning
+  socket.setEncoding('utf8');
+  socket.pipe(socket);
+});
+server.listen(0, () => {
+  const options = { rejectUnauthorized: false };
+  const socket =
+    tls.connect(server.address().port, options, common.mustCall(() => {
+      socket.end('hello');
+    }));
+  socket.resume();
+  socket.on('end', () => server.close());
+});
+
+common.expectWarning('DeprecationWarning',
+                     'The tlsSocket.ssl property is deprecated.',
+                     'DEP00XX');
diff --git a/test/parallel/test-tls-tlswrap-segfault.js b/test/parallel/test-tls-tlswrap-segfault.js
index a36016efa48a02..43a176a10955d3 100644
--- a/test/parallel/test-tls-tlswrap-segfault.js
+++ b/test/parallel/test-tls-tlswrap-segfault.js
@@ -31,8 +31,8 @@ const server = tls.createServer(options, function(s) {
 
 function putImmediate(client) {
   setImmediate(function() {
-    if (client.ssl) {
-      const fd = client.ssl.fd;
+    if (client._handle) {
+      const fd = client._handle.fd;
       assert(!!fd);
       putImmediate(client);
     } else {