tls: make deprecated tls.createSecurePair() use public API

Make the deprecated `tls.createSecurePair()` method use other public
APIs only (`TLSSocket` in particular).

Since `tls.createSecurePair()` has been runtime-deprecated only
since Node 8, it probably isn’t quite time to remove it yet,
but this patch removes almost all of the code complexity that
is retained by it.

The API, as it is documented, is retained. However, it is very likely
that some users have come to rely on parts of undocumented API
of the `SecurePair` class, especially since some of the existing
tests checked for those. Therefore, this should definitely be
considered a breaking change.

PR-URL: #17882
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Minwoo Jung <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

Author: addaleax

lib/_tls_legacy.js

@@ -0,0 +1,42 @@
+'use strict';
+const { Duplex } = require('stream');
+
+const kCallback = Symbol('Callback');
+const kOtherSide = Symbol('Other');
+
+class DuplexSocket extends Duplex {
+  constructor() {
+    super();
+    this[kCallback] = null;
+    this[kOtherSide] = null;
+  }
+
+  _read() {
+    const callback = this[kCallback];
+    if (callback) {
+      this[kCallback] = null;
+      callback();
+    }
+  }
+
+  _write(chunk, encoding, callback) {
+    this[kOtherSide][kCallback] = callback;
+    this[kOtherSide].push(chunk);
+  }
+
+  _final(callback) {
+    this[kOtherSide].on('end', callback);
+    this[kOtherSide].push(null);
+  }
+}
+
+class DuplexPair {
+  constructor() {
+    this.socket1 = new DuplexSocket();
+    this.socket2 = new DuplexSocket();
+    this.socket1[kOtherSide] = this.socket2;
+    this.socket2[kOtherSide] = this.socket1;
+  }
+}
+
+module.exports = DuplexPair;

lib/internal/streams/duplexpair.js

@@ -31,6 +31,8 @@ const net = require('net');
 const url = require('url');
 const binding = process.binding('crypto');
 const Buffer = require('buffer').Buffer;
+const EventEmitter = require('events');
+const DuplexPair = require('internal/streams/duplexpair');
 const canonicalizeIP = process.binding('cares_wrap').canonicalizeIP;
 
 // Allow {CLIENT_RENEG_LIMIT} client-initiated session renegotiations
@@ -230,6 +232,33 @@ exports.checkServerIdentity = function checkServerIdentity(host, cert) {
   }
 };
 
+
+class SecurePair extends EventEmitter {
+  constructor(secureContext = exports.createSecureContext(),
+              isServer = false,
+              requestCert = !isServer,
+              rejectUnauthorized = false,
+              options = {}) {
+    super();
+    const { socket1, socket2 } = new DuplexPair();
+
+    this.server = options.server;
+    this.credentials = secureContext;
+
+    this.encrypted = socket1;
+    this.cleartext = new exports.TLSSocket(socket2, Object.assign({
+      secureContext, isServer, requestCert, rejectUnauthorized
+    }, options));
+    this.cleartext.once('secure', () => this.emit('secure'));
+  }
+
+  destroy() {
+    this.cleartext.destroy();
+    this.encrypted.destroy();
+  }
+}
+
+
 exports.parseCertString = internalUtil.deprecate(
   internalTLS.parseCertString,
   'tls.parseCertString() is deprecated. ' +
@@ -243,5 +272,9 @@ exports.Server = require('_tls_wrap').Server;
 exports.createServer = require('_tls_wrap').createServer;
 exports.connect = require('_tls_wrap').connect;
 
-// Deprecated: DEP0064
-exports.createSecurePair = require('_tls_legacy').createSecurePair;
+exports.createSecurePair = internalUtil.deprecate(
+  function createSecurePair(...args) {
+    return new SecurePair(...args);
+  },
+  'tls.createSecurePair() is deprecated. Please use ' +
+  'tls.TLSSocket instead.', 'DEP0064');

lib/tls.js

@@ -69,7 +69,6 @@
       'lib/timers.js',
       'lib/tls.js',
       'lib/_tls_common.js',
-      'lib/_tls_legacy.js',
       'lib/_tls_wrap.js',
       'lib/tty.js',
       'lib/url.js',
@@ -140,6 +139,7 @@
       'lib/internal/streams/lazy_transform.js',
       'lib/internal/streams/async_iterator.js',
       'lib/internal/streams/BufferList.js',
+      'lib/internal/streams/duplexpair.js',
       'lib/internal/streams/legacy.js',
       'lib/internal/streams/destroy.js',
       'lib/internal/wrap_js_stream.js',

node.gyp

@@ -67,7 +67,6 @@ namespace node {
 
 #if HAVE_OPENSSL
 #define NODE_ASYNC_CRYPTO_PROVIDER_TYPES(V)                                   \
-  V(SSLCONNECTION)                                                            \
   V(PBKDF2REQUEST)                                                            \
   V(RANDOMBYTESREQUEST)                                                       \
   V(TLSWRAP)

src/async_wrap.h

@@ -193,14 +193,12 @@ class ModuleWrap;
   V(onheaders_string, "onheaders")                                            \
   V(onmessage_string, "onmessage")                                            \
   V(onnewsession_string, "onnewsession")                                      \
-  V(onnewsessiondone_string, "onnewsessiondone")                              \
   V(onocspresponse_string, "onocspresponse")                                  \
   V(ongoawaydata_string, "ongoawaydata")                                      \
   V(onpriority_string, "onpriority")                                          \
   V(onread_string, "onread")                                                  \
   V(onreadstart_string, "onreadstart")                                        \
   V(onreadstop_string, "onreadstop")                                          \
-  V(onselect_string, "onselect")                                              \
   V(onsettings_string, "onsettings")                                          \
   V(onshutdown_string, "onshutdown")                                          \
   V(onsignal_string, "onsignal")                                              \
@@ -224,15 +222,13 @@ class ModuleWrap;
   V(raw_string, "raw")                                                        \
   V(read_host_object_string, "_readHostObject")                               \
   V(readable_string, "readable")                                              \
-  V(received_shutdown_string, "receivedShutdown")                             \
   V(refresh_string, "refresh")                                                \
   V(regexp_string, "regexp")                                                  \
   V(rename_string, "rename")                                                  \
   V(replacement_string, "replacement")                                        \
   V(retry_string, "retry")                                                    \
   V(serial_string, "serial")                                                  \
   V(scopeid_string, "scopeid")                                                \
-  V(sent_shutdown_string, "sentShutdown")                                     \
   V(serial_number_string, "serialNumber")                                     \
   V(service_string, "service")                                                \
   V(servername_string, "servername")                                          \

src/env.h

@@ -328,7 +328,6 @@ class SSLWrap {
                                 void* arg);
   static int TLSExtStatusCallback(SSL* s, void* arg);
   static int SSLCertCallback(SSL* s, void* arg);
-  static void SSLGetter(const v8::FunctionCallbackInfo<v8::Value>& info);
 
   void DestroySSL();
   void WaitForCertCb(CertCb cb, void* arg);
@@ -364,87 +363,6 @@ class SSLWrap {
   friend class SecureContext;
 };
 
-// Connection inherits from AsyncWrap because SSLWrap makes calls to
-// MakeCallback, but SSLWrap doesn't store the handle itself. Instead it
-// assumes that any args.This() called will be the handle from Connection.
-class Connection : public AsyncWrap, public SSLWrap<Connection> {
- public:
-  ~Connection() override {
-#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
-    sniObject_.Reset();
-    servername_.Reset();
-#endif
-  }
-
-  static void Initialize(Environment* env, v8::Local<v8::Object> target);
-  void NewSessionDoneCb();
-
-#ifndef OPENSSL_NO_NEXTPROTONEG
-  v8::Persistent<v8::Object> npnProtos_;
-  v8::Persistent<v8::Value> selectedNPNProto_;
-#endif
-
-#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
-  v8::Persistent<v8::Object> sniObject_;
-  v8::Persistent<v8::String> servername_;
-#endif
-
-  size_t self_size() const override { return sizeof(*this); }
-
- protected:
-  static void New(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void EncIn(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void ClearOut(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void ClearPending(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void EncPending(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void EncOut(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void ClearIn(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void Start(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void Close(const v8::FunctionCallbackInfo<v8::Value>& args);
-
-#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
-  // SNI
-  static void GetServername(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void SetSNICallback(const v8::FunctionCallbackInfo<v8::Value>& args);
-  static int SelectSNIContextCallback_(SSL* s, int* ad, void* arg);
-#endif
-
-  static void OnClientHelloParseEnd(void* arg);
-
-  int HandleBIOError(BIO* bio, const char* func, int rv);
-
-  enum ZeroStatus {
-    kZeroIsNotAnError,
-    kZeroIsAnError
-  };
-
-  enum SyscallStatus {
-    kIgnoreSyscall,
-    kSyscallError
-  };
-
-  int HandleSSLError(const char* func, int rv, ZeroStatus zs, SyscallStatus ss);
-
-  void SetShutdownFlags();
-
-  Connection(Environment* env,
-             v8::Local<v8::Object> wrap,
-             SecureContext* sc,
-             SSLWrap<Connection>::Kind kind);
-
- private:
-  static void SSLInfoCallback(const SSL *ssl, int where, int ret);
-
-  BIO *bio_read_;
-  BIO *bio_write_;
-
-  uint8_t hello_data_[18432];
-  size_t hello_offset_;
-
-  friend class ClientHelloParser;
-  friend class SecureContext;
-};
-
 class CipherBase : public BaseObject {
  public:
   ~CipherBase() override {

src/node_crypto.cc

@@ -59,20 +59,10 @@ const UDP = process.binding('udp_wrap').UDP;
       crypto.SecureContext.prototype._external;
     }, TypeError);
 
-    assert.throws(() => {
-      crypto.Connection.prototype._external;
-    }, TypeError);
-
     assert.strictEqual(
       typeof Object.getOwnPropertyDescriptor(
         crypto.SecureContext.prototype, '_external'),
       'object'
     );
-
-    assert.strictEqual(
-      typeof Object.getOwnPropertyDescriptor(
-        crypto.Connection.prototype, '_external'),
-      'object'
-    );
   }
 }

src/node_crypto.h

@@ -40,7 +40,7 @@ assert.throws(() => tls.createServer({ ticketKeys: Buffer.alloc(0) }),
               /TypeError: Ticket keys length must be 48 bytes/);
 
 assert.throws(() => tls.createSecurePair({}),
-              /Error: First argument must be a tls module SecureContext/);
+              /TypeError: Second argument should be a SecureContext instance/);
 
 {
   const buffer = Buffer.from('abcd');

test/async-hooks/test-connection.ssl.js

@@ -7,7 +7,7 @@ if (!common.hasCrypto)
 
 const assert = require('assert');
 const { createSecureContext } = require('tls');
-const { createSecurePair } = require('_tls_legacy');
+const { createSecurePair } = require('tls');
 
 const before = process.memoryUsage().external;
 {
@@ -16,11 +16,13 @@ const before = process.memoryUsage().external;
   for (let i = 0; i < 1e4; i += 1)
     createSecurePair(context, false, false, false, options).destroy();
 }
-global.gc();
-const after = process.memoryUsage().external;
+setImmediate(() => {
+  global.gc();
+  const after = process.memoryUsage().external;
 
-// It's not an exact science but a SecurePair grows .external by about 45 kB.
-// Unless AdjustAmountOfExternalAllocatedMemory() is called on destruction,
-// 10,000 instances make it grow by well over 400 MB.  Allow for some slop
-// because objects like buffers also affect the external limit.
-assert(after - before < 25 << 20);
+  // It's not an exact science but a SecurePair grows .external by about 45 kB.
+  // Unless AdjustAmountOfExternalAllocatedMemory() is called on destruction,
+  // 10,000 instances make it grow by well over 400 MB.  Allow for some slop
+  // because objects like buffers also affect the external limit.
+  assert(after - before < 25 << 20);
+});

test/async-hooks/test-graph.connection.js

@@ -87,13 +87,6 @@ function testInitialized(req, ctor_name) {
 }
 
 
-if (common.hasCrypto) { // eslint-disable-line crypto-check
-  const tls = require('tls');
-  // SecurePair
-  testInitialized(tls.createSecurePair().ssl, 'Connection');
-}
-
-
 if (common.hasCrypto) { // eslint-disable-line crypto-check
   const crypto = require('crypto');