Remove dead TLS code, and fix some misnamed variables

lib/_stream_wrap.js

@@ -1,3 +1,3 @@
 'use strict';
 
-module.exports = require('internal/wrap_js_stream');
+module.exports = require('internal/js_stream_socket');

lib/_tls_wrap.js

@@ -29,7 +29,7 @@ const net = require('net');
 const tls = require('tls');
 const util = require('util');
 const common = require('_tls_common');
-const { StreamWrap } = require('_stream_wrap');
+const JSStreamSocket = require('internal/js_stream_socket');
 const { Buffer } = require('buffer');
 const debug = util.debuglog('tls');
 const { TCP, constants: TCPConstants } = internalBinding('tcp_wrap');
@@ -214,7 +214,7 @@ function requestOCSPDone(socket) {
 }
 
 
-function onnewsession(key, session) {
+function onnewsession(sessionId, session) {
   const owner = this[owner_symbol];
 
   if (!owner.server)
@@ -238,7 +238,7 @@ function onnewsession(key, session) {
   };
 
   owner._newSessionPending = true;
-  if (!owner.server.emit('newSession', key, session, done))
+  if (!owner.server.emit('newSession', sessionId, session, done))
     done();
 }
 
@@ -271,15 +271,17 @@ function onerror(err) {
   }
 }
 
-function initRead(tls, wrapped) {
+// Used by both client and server TLSSockets to start data flowing from _handle,
+// read(0) causes a StreamBase::ReadStart, via Socket._read.
+function initRead(tls, socket) {
   // If we were destroyed already don't bother reading
   if (!tls._handle)
     return;
 
   // Socket already has some buffered data - emulate receiving it
-  if (wrapped && wrapped.readableLength) {
+  if (socket && socket.readableLength) {
     var buf;
-    while ((buf = wrapped.read()) !== null)
+    while ((buf = socket.read()) !== null)
       tls._handle.receive(buf);
   }
 
@@ -308,12 +310,14 @@ function TLSSocket(socket, opts) {
   this.authorizationError = null;
   this[kRes] = null;
 
-  // Wrap plain JS Stream into StreamWrap
   var wrap;
   if ((socket instanceof net.Socket && socket._handle) || !socket) {
     wrap = socket;
   } else {
-    wrap = new StreamWrap(socket);
+    // TLS expects to interact from C++ with a net.Socket that has a C++ stream
+    // handle, but a JS stream doesn't have one. Wrap it up to make it look like
+    // a socket.
+    wrap = new JSStreamSocket(socket);
     wrap.once('close', () => this.destroy());
   }
 
@@ -1225,7 +1229,7 @@ exports.connect = function connect(...args) {
 
   const context = options.secureContext || tls.createSecureContext(options);
 
-  var socket = new TLSSocket(options.socket, {
+  var tlssock = new TLSSocket(options.socket, {
     pipe: !!options.path,
     secureContext: context,
     isServer: false,
@@ -1236,12 +1240,14 @@ exports.connect = function connect(...args) {
     requestOCSP: options.requestOCSP
   });
 
-  socket[kConnectOptions] = options;
+  tlssock[kConnectOptions] = options;
 
   if (cb)
-    socket.once('secureConnect', cb);
+    tlssock.once('secureConnect', cb);
 
   if (!options.socket) {
+    // If user provided the socket, its their responsibility to manage its
+    // connectivity. If we created one internally, we connect it.
     const connectOpt = {
       path: options.path,
       port: options.port,
@@ -1250,13 +1256,13 @@ exports.connect = function connect(...args) {
       localAddress: options.localAddress,
       lookup: options.lookup
     };
-    socket.connect(connectOpt, socket._start);
+    tlssock.connect(connectOpt, tlssock._start);
   }
 
-  socket._releaseControl();
+  tlssock._releaseControl();
 
   if (options.session)
-    socket.setSession(options.session);
+    tlssock.setSession(options.session);
 
   if (options.servername) {
     if (!ipServernameWarned && net.isIP(options.servername)) {
@@ -1268,14 +1274,14 @@ exports.connect = function connect(...args) {
       );
       ipServernameWarned = true;
     }
-    socket.setServername(options.servername);
+    tlssock.setServername(options.servername);
   }
 
   if (options.socket)
-    socket._start();
+    tlssock._start();
 
-  socket.on('secure', onConnectSecure);
-  socket.once('end', onConnectEnd);
+  tlssock.on('secure', onConnectSecure);
+  tlssock.once('end', onConnectEnd);
 
-  return socket;
+  return tlssock;
 };

lib/internal/http2/core.js

@@ -22,7 +22,7 @@ const util = require('util');
 
 const { kIncomingMessage } = require('_http_common');
 const { kServerResponse } = require('_http_server');
-const { StreamWrap } = require('_stream_wrap');
+const JSStreamSocket = require('internal/js_stream_socket');
 
 const {
   defaultTriggerAsyncIdScope,
@@ -935,7 +935,7 @@ class Http2Session extends EventEmitter {
     super();
 
     if (!socket._handle || !socket._handle._externalStream) {
-      socket = new StreamWrap(socket);
+      socket = new JSStreamSocket(socket);
     }
 
     // No validation is performed on the input parameters because this

lib/internal/wrap_js_stream.js → lib/internal/js_stream_socket.js

@@ -5,7 +5,7 @@ const util = require('util');
 const { Socket } = require('net');
 const { JSStream } = internalBinding('js_stream');
 const uv = internalBinding('uv');
-const debug = util.debuglog('stream_wrap');
+const debug = util.debuglog('stream_socket');
 const { owner_symbol } = require('internal/async_hooks').symbols;
 const { ERR_STREAM_WRAP } = require('internal/errors').codes;
 
@@ -29,17 +29,17 @@ function onwrite(req, bufs) { return this[owner_symbol].doWrite(req, bufs); }
  * can skip going through the JS layer and let TLS access the raw C++ handle
  * of a net.Socket. The flipside of this is that, to maintain composability,
  * we need a way to create "fake" net.Socket instances that call back into a
- * "real" JavaScript stream. JSStreamWrap is exactly this.
+ * "real" JavaScript stream. JSStreamSocket is exactly this.
  */
-class JSStreamWrap extends Socket {
+class JSStreamSocket extends Socket {
   constructor(stream) {
     const handle = new JSStream();
     handle.close = (cb) => {
       debug('close');
       this.doClose(cb);
     };
     // Inside of the following functions, `this` refers to the handle
-    // and `this[owner_symbol]` refers to this JSStreamWrap instance.
+    // and `this[owner_symbol]` refers to this JSStreamSocket instance.
     handle.isClosing = isClosing;
     handle.onreadstart = onreadstart;
     handle.onreadstop = onreadstop;
@@ -88,9 +88,10 @@ class JSStreamWrap extends Socket {
     this.read(0);
   }
 
-  // Legacy
+  // Allow legacy requires in the test suite to keep working:
+  //   const { StreamWrap } = require('internal/js_stream_socket')
   static get StreamWrap() {
-    return JSStreamWrap;
+    return JSStreamSocket;
   }
 
   isClosing() {
@@ -223,4 +224,4 @@ class JSStreamWrap extends Socket {
   }
 }
 
-module.exports = JSStreamWrap;
+module.exports = JSStreamSocket;

node.gyp

@@ -126,6 +126,7 @@
       'lib/internal/fs/watchers.js',
       'lib/internal/http.js',
       'lib/internal/inspector_async_hook.js',
+      'lib/internal/js_stream_socket.js',
       'lib/internal/linkedlist.js',
       'lib/internal/modules/cjs/helpers.js',
       'lib/internal/modules/cjs/loader.js',
@@ -188,7 +189,6 @@
       'lib/internal/streams/state.js',
       'lib/internal/streams/pipeline.js',
       'lib/internal/streams/end-of-stream.js',
-      'lib/internal/wrap_js_stream.js',
       'deps/v8/tools/splaytree.js',
       'deps/v8/tools/codemap.js',
       'deps/v8/tools/consarray.js',

src/node_crypto.cc

@@ -1510,20 +1510,20 @@ int SSLWrap<Base>::NewSessionCallback(SSL* s, SSL_SESSION* sess) {
     return 0;
 
   // Serialize session
-  Local<Object> buff = Buffer::New(env, size).ToLocalChecked();
-  unsigned char* serialized = reinterpret_cast<unsigned char*>(
-      Buffer::Data(buff));
-  memset(serialized, 0, size);
-  i2d_SSL_SESSION(sess, &serialized);
+  Local<Object> session = Buffer::New(env, size).ToLocalChecked();
+  unsigned char* session_data = reinterpret_cast<unsigned char*>(
+      Buffer::Data(session));
+  memset(session_data, 0, size);
+  i2d_SSL_SESSION(sess, &session_data);
 
   unsigned int session_id_length;
-  const unsigned char* session_id = SSL_SESSION_get_id(sess,
-                                                       &session_id_length);
-  Local<Object> session = Buffer::Copy(
+  const unsigned char* session_id_data = SSL_SESSION_get_id(sess,
+                                                            &session_id_length);
+  Local<Object> session_id = Buffer::Copy(
       env,
-      reinterpret_cast<const char*>(session_id),
+      reinterpret_cast<const char*>(session_id_data),
       session_id_length).ToLocalChecked();
-  Local<Value> argv[] = { session, buff };
+  Local<Value> argv[] = { session_id, session };
   w->new_session_wait_ = true;
   w->MakeCallback(env->onnewsession_string(), arraysize(argv), argv);
 
@@ -1559,9 +1559,6 @@ void SSLWrap<Base>::OnClientHello(void* arg,
   hello_obj->Set(context,
                  env->tls_ticket_string(),
                  Boolean::New(env->isolate(), hello.has_ticket())).FromJust();
-  hello_obj->Set(context,
-                 env->ocsp_request_string(),
-                 Boolean::New(env->isolate(), hello.ocsp_request())).FromJust();
 
   Local<Value> argv[] = { hello_obj };
   w->MakeCallback(env->onclienthello_string(), arraysize(argv), argv);

src/node_crypto_clienthello-inl.h

@@ -48,7 +48,6 @@ inline void ClientHelloParser::Reset() {
   tls_ticket_ = nullptr;
   servername_size_ = 0;
   servername_ = nullptr;
-  ocsp_request_ = 0;
 }
 
 inline void ClientHelloParser::Start(ClientHelloParser::OnHelloCb onhello_cb,

src/node_crypto_clienthello.cc

@@ -112,7 +112,6 @@ void ClientHelloParser::ParseHeader(const uint8_t* data, size_t avail) {
   hello.session_id_ = session_id_;
   hello.session_size_ = session_size_;
   hello.has_ticket_ = tls_ticket_ != nullptr && tls_ticket_size_ != 0;
-  hello.ocsp_request_ = ocsp_request_;
   hello.servername_ = servername_;
   hello.servername_size_ = static_cast<uint8_t>(servername_size_);
   onhello_cb_(cb_arg_, hello);
@@ -149,18 +148,6 @@ void ClientHelloParser::ParseExtension(const uint16_t type,
         }
       }
       break;
-    case kStatusRequest:
-      // We are ignoring any data, just indicating the presence of extension
-      if (len < kMinStatusRequestSize)
-        return;
-
-      // Unknown type, ignore it
-      if (data[0] != kStatusRequestOCSP)
-        break;
-
-      // Ignore extensions, they won't work with caching on backend anyway
-      ocsp_request_ = 1;
-      break;
     case kTLSSessionTicket:
       tls_ticket_size_ = len;
       tls_ticket_ = data + len;

src/node_crypto_clienthello.h

@@ -41,15 +41,13 @@ class ClientHelloParser {
     inline bool has_ticket() const { return has_ticket_; }
     inline uint8_t servername_size() const { return servername_size_; }
     inline const uint8_t* servername() const { return servername_; }
-    inline int ocsp_request() const { return ocsp_request_; }
 
    private:
     uint8_t session_size_;
     const uint8_t* session_id_;
     bool has_ticket_;
     uint8_t servername_size_;
     const uint8_t* servername_;
-    int ocsp_request_;
 
     friend class ClientHelloParser;
   };
@@ -69,7 +67,6 @@ class ClientHelloParser {
   static const size_t kMaxTLSFrameLen = 16 * 1024 + 5;
   static const size_t kMaxSSLExFrameLen = 32 * 1024;
   static const uint8_t kServernameHostname = 0;
-  static const uint8_t kStatusRequestOCSP = 1;
   static const size_t kMinStatusRequestSize = 5;
 
   enum ParseState {
@@ -93,7 +90,6 @@ class ClientHelloParser {
 
   enum ExtensionType {
     kServerName = 0,
-    kStatusRequest = 5,
     kTLSSessionTicket = 35
   };
 
@@ -115,7 +111,6 @@ class ClientHelloParser {
   const uint8_t* session_id_ = nullptr;
   uint16_t servername_size_ = 0;
   const uint8_t* servername_ = nullptr;
-  uint8_t ocsp_request_ = 0;
   uint16_t tls_ticket_size_ = -1;
   const uint8_t* tls_ticket_ = nullptr;
 };

test/parallel/test-stream-wrap-drain.js

@@ -2,12 +2,12 @@
 'use strict';
 const common = require('../common');
 const assert = require('assert');
-const { StreamWrap } = require('_stream_wrap');
+const { StreamWrap } = require('internal/js_stream_socket');
 const { Duplex } = require('stream');
 const { internalBinding } = require('internal/test/binding');
 const { ShutdownWrap } = internalBinding('stream_wrap');
 
-// This test makes sure that when an instance of JSStreamWrap is waiting for
+// This test makes sure that when a wrapped stream is waiting for
 // a "drain" event to `doShutdown`, the instance will work correctly when a
 // "drain" event emitted.
 {

test/parallel/test-stream-wrap-encoding.js

@@ -1,7 +1,8 @@
+// Flags: --expose-internals
 'use strict';
 const common = require('../common');
 
-const StreamWrap = require('_stream_wrap');
+const StreamWrap = require('internal/js_stream_socket');
 const Duplex = require('stream').Duplex;
 
 {

test/parallel/test-stream-wrap.js

@@ -4,7 +4,7 @@ const common = require('../common');
 const assert = require('assert');
 
 const { internalBinding } = require('internal/test/binding');
-const StreamWrap = require('_stream_wrap');
+const StreamWrap = require('internal/js_stream_socket');
 const { Duplex } = require('stream');
 const { ShutdownWrap } = internalBinding('stream_wrap');
 

test/parallel/test-wrap-js-stream-destroy.js

@@ -1,7 +1,8 @@
+// Flags: --expose-internals
 'use strict';
 
 const common = require('../common');
-const StreamWrap = require('_stream_wrap');
+const StreamWrap = require('internal/js_stream_socket');
 const net = require('net');
 
 // This test ensures that when we directly call `socket.destroy()` without

test/parallel/test-wrap-js-stream-duplex.js

@@ -1,7 +1,8 @@
+// Flags: --expose-internals
 'use strict';
 const common = require('../common');
 const assert = require('assert');
-const StreamWrap = require('_stream_wrap');
+const StreamWrap = require('internal/js_stream_socket');
 const { PassThrough } = require('stream');
 const { Socket } = require('net');
 

test/parallel/test-wrap-js-stream-exceptions.js

@@ -2,7 +2,7 @@
 'use strict';
 const common = require('../common');
 const assert = require('assert');
-const JSStreamWrap = require('internal/wrap_js_stream');
+const JSStreamWrap = require('internal/js_stream_socket');
 const { Duplex } = require('stream');
 
 process.once('uncaughtException', common.mustCall((err) => {