TLS cleanups

lib/_tls_wrap.js

@@ -61,6 +61,8 @@ const noop = () => {};
 
 let ipServernameWarned = false;
 
+// Server side times how long a handshake is taking to protect against slow
+// handshakes being used for DoS.
 function onhandshakestart(now) {
   debug('onhandshakestart');
 
@@ -120,13 +122,19 @@ function loadSession(hello) {
       return owner.destroy(new ERR_SOCKET_CLOSED());
 
     owner._handle.loadSession(session);
+    // Session is loaded. End the parser to allow handshaking to continue.
     owner._handle.endParser();
   }
 
   if (hello.sessionId.length <= 0 ||
       hello.tlsTicket ||
       owner.server &&
       !owner.server.emit('resumeSession', hello.sessionId, onSession)) {
+    // Sessions without identifiers can't be resumed.
+    // Sessions with tickets can be resumed directly from the ticket, no server
+    // session storage is necessary.
+    // Without a call to a resumeSession listener, a session will never be
+    // loaded, so end the parser to allow handshaking to continue.
     owner._handle.endParser();
   }
 }
@@ -215,13 +223,17 @@ function requestOCSPDone(socket) {
 
 
 function onnewsession(sessionId, session) {
+  debug('onnewsession');
   const owner = this[owner_symbol];
 
+  // XXX(sam) no server to emit the event on, but handshake won't continue
+  // unless newSessionDone() is called, should it be?
   if (!owner.server)
     return;
 
   var once = false;
   const done = () => {
+    debug('onnewsession done');
     if (once)
       return;
     once = true;
@@ -273,19 +285,19 @@ function onerror(err) {
 
 // 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) {
+function initRead(tlsSocket, socket) {
   // If we were destroyed already don't bother reading
-  if (!tls._handle)
+  if (!tlsSocket._handle)
     return;
 
   // Socket already has some buffered data - emulate receiving it
   if (socket && socket.readableLength) {
     var buf;
     while ((buf = socket.read()) !== null)
-      tls._handle.receive(buf);
+      tlsSocket._handle.receive(buf);
   }
 
-  tls.read(0);
+  tlsSocket.read(0);
 }
 
 /**
@@ -312,8 +324,12 @@ function TLSSocket(socket, opts) {
 
   var wrap;
   if ((socket instanceof net.Socket && socket._handle) || !socket) {
+    // 1. connected socket
+    // 2. no socket, one will be created with net.Socket().connect
     wrap = socket;
   } else {
+    // 3. socket has no handle so is js not c++
+    // 4. unconnected sockets are wrapped
     // 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.
@@ -333,7 +349,7 @@ function TLSSocket(socket, opts) {
   });
 
   // Proxy for API compatibility
-  this.ssl = this._handle;
+  this.ssl = this._handle;  // C++ TLSWrap object
 
   this.on('error', this._tlsError);
 
@@ -429,8 +445,8 @@ TLSSocket.prototype._wrapHandle = function(wrap) {
   const res = tls_wrap.wrap(externalStream,
                             context.context,
                             !!options.isServer);
-  res._parent = handle;
-  res._parentWrap = wrap;
+  res._parent = handle;  // C++ "wrap" object: TCPWrap, JSStream, ...
+  res._parentWrap = wrap;  // JS object: net.Socket, JSStreamSocket, ...
   res._secureContext = context;
   res.reading = handle.reading;
   this[kRes] = res;
@@ -480,8 +496,8 @@ TLSSocket.prototype._init = function(socket, wrap) {
 
   this.server = options.server;
 
-  // For clients, we will always have either a given ca list or be using
-  // default one
+  // Clients (!isServer) always request a cert, servers request a client cert
+  // only on explicit configuration.
   const requestCert = !!options.requestCert || !options.isServer;
   const rejectUnauthorized = !!options.rejectUnauthorized;
 
@@ -502,6 +518,7 @@ TLSSocket.prototype._init = function(socket, wrap) {
     if (this.server) {
       if (this.server.listenerCount('resumeSession') > 0 ||
           this.server.listenerCount('newSession') > 0) {
+        // Also starts the client hello parser as a side effect.
         ssl.enableSessionCallbacks();
       }
       if (this.server.listenerCount('OCSPRequest') > 0)
@@ -709,7 +726,7 @@ TLSSocket.prototype.getCipher = function(err) {
 // TODO: support anonymous (nocert) and PSK
 
 
-function onSocketSecure() {
+function onServerSocketSecure() {
   if (this._requestCert) {
     const verifyError = this._handle.verifyError();
     if (verifyError) {
@@ -760,7 +777,7 @@ function tlsConnectionListener(rawSocket) {
     SNICallback: this[kSNICallback] || SNICallback
   });
 
-  socket.on('secure', onSocketSecure);
+  socket.on('secure', onServerSocketSecure);
 
   socket[kErrorEmitted] = false;
   socket.on('close', onSocketClose);

src/node_crypto.cc

@@ -2648,47 +2648,19 @@ int SSLWrap<Base>::SetCACerts(SecureContext* sc) {
 }
 
 int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx) {
-  // Quoting SSL_set_verify(3ssl):
+  // From https://www.openssl.org/docs/man1.1.1/man3/SSL_verify_cb:
   //
-  //   The VerifyCallback function is used to control the behaviour when
-  //   the SSL_VERIFY_PEER flag is set. It must be supplied by the
-  //   application and receives two arguments: preverify_ok indicates,
-  //   whether the verification of the certificate in question was passed
-  //   (preverify_ok=1) or not (preverify_ok=0). x509_ctx is a pointer to
-  //   the complete context used for the certificate chain verification.
-  //
-  //   The certificate chain is checked starting with the deepest nesting
-  //   level (the root CA certificate) and worked upward to the peer's
-  //   certificate.  At each level signatures and issuer attributes are
-  //   checked.  Whenever a verification error is found, the error number is
-  //   stored in x509_ctx and VerifyCallback is called with preverify_ok=0.
-  //   By applying X509_CTX_store_* functions VerifyCallback can locate the
-  //   certificate in question and perform additional steps (see EXAMPLES).
-  //   If no error is found for a certificate, VerifyCallback is called
-  //   with preverify_ok=1 before advancing to the next level.
-  //
-  //   The return value of VerifyCallback controls the strategy of the
-  //   further verification process. If VerifyCallback returns 0, the
-  //   verification process is immediately stopped with "verification
-  //   failed" state. If SSL_VERIFY_PEER is set, a verification failure
-  //   alert is sent to the peer and the TLS/SSL handshake is terminated. If
-  //   VerifyCallback returns 1, the verification process is continued. If
+  //   If VerifyCallback returns 1, the verification process is continued. If
   //   VerifyCallback always returns 1, the TLS/SSL handshake will not be
-  //   terminated with respect to verification failures and the connection
-  //   will be established. The calling process can however retrieve the
-  //   error code of the last verification error using
-  //   SSL_get_verify_result(3) or by maintaining its own error storage
-  //   managed by VerifyCallback.
-  //
-  //   If no VerifyCallback is specified, the default callback will be
-  //   used.  Its return value is identical to preverify_ok, so that any
-  //   verification failure will lead to a termination of the TLS/SSL
-  //   handshake with an alert message, if SSL_VERIFY_PEER is set.
+  //   terminated with respect to verification failures and the connection will
+  //   be established. The calling process can however retrieve the error code
+  //   of the last verification error using SSL_get_verify_result(3) or by
+  //   maintaining its own error storage managed by VerifyCallback.
   //
-  // Since we cannot perform I/O quickly enough in this callback, we ignore
-  // all preverify_ok errors and let the handshake continue. It is
-  // imperative that the user use Connection::VerifyError after the
-  // 'secure' callback has been made.
+  // Since we cannot perform I/O quickly enough with X509_STORE_CTX_ APIs in
+  // this callback, we ignore all preverify_ok errors and let the handshake
+  // continue. It is imperative that the user use Connection::VerifyError after
+  // the 'secure' callback has been made.
   return 1;
 }
 

src/tls_wrap.cc

@@ -210,10 +210,9 @@ void TLSWrap::SSLInfoCallback(const SSL* ssl_, int where, int ret) {
   if (!(where & (SSL_CB_HANDSHAKE_START | SSL_CB_HANDSHAKE_DONE)))
     return;
 
-  // Be compatible with older versions of OpenSSL. SSL_get_app_data() wants
-  // a non-const SSL* in OpenSSL <= 0.9.7e.
+  // SSL_renegotiate_pending() should take `const SSL*`, but it does not.
   SSL* ssl = const_cast<SSL*>(ssl_);
-  TLSWrap* c = static_cast<TLSWrap*>(SSL_get_app_data(ssl));
+  TLSWrap* c = static_cast<TLSWrap*>(SSL_get_app_data(ssl_));
   Environment* env = c->env();
   HandleScope handle_scope(env->isolate());
   Context::Scope context_scope(env->context());
@@ -349,14 +348,14 @@ Local<Value> TLSWrap::GetSSLError(int status, int* err, std::string* msg) {
     case SSL_ERROR_WANT_READ:
     case SSL_ERROR_WANT_WRITE:
     case SSL_ERROR_WANT_X509_LOOKUP:
-      break;
+      return Local<Value>();
+
     case SSL_ERROR_ZERO_RETURN:
       return scope.Escape(env()->zero_return_string());
-      break;
-    default:
-      {
-        CHECK(*err == SSL_ERROR_SSL || *err == SSL_ERROR_SYSCALL);
 
+    case SSL_ERROR_SSL:
+    case SSL_ERROR_SYSCALL:
+      {
         unsigned long ssl_err = ERR_peek_error();  // NOLINT(runtime/int)
         BIO* bio = BIO_new(BIO_s_mem());
         ERR_print_errors(bio);
@@ -409,8 +408,11 @@ Local<Value> TLSWrap::GetSSLError(int status, int* err, std::string* msg) {
 
         return scope.Escape(exception);
       }
+
+    default:
+      UNREACHABLE();
   }
-  return Local<Value>();
+  UNREACHABLE();
 }
 
 
@@ -768,7 +770,7 @@ void TLSWrap::SetVerifyMode(const FunctionCallbackInfo<Value>& args) {
   if (wrap->is_server()) {
     bool request_cert = args[0]->IsTrue();
     if (!request_cert) {
-      // Note reject_unauthorized ignored.
+      // If no cert is requested, there will be none to reject as unauthorized.
       verify_mode = SSL_VERIFY_NONE;
     } else {
       bool reject_unauthorized = args[1]->IsTrue();
@@ -777,7 +779,9 @@ void TLSWrap::SetVerifyMode(const FunctionCallbackInfo<Value>& args) {
         verify_mode |= SSL_VERIFY_FAIL_IF_NO_PEER_CERT;
     }
   } else {
-    // Note request_cert and reject_unauthorized are ignored for clients.
+    // Servers always send a cert if the cipher is not anonymous (anon is
+    // disabled by default), so use VERIFY_NONE and check the cert after the
+    // handshake has completed.
     verify_mode = SSL_VERIFY_NONE;
   }
 

src/tls_wrap.h

@@ -57,16 +57,19 @@ class TLSWrap : public AsyncWrap,
                          v8::Local<v8::Context> context,
                          void* priv);
 
-  int GetFD() override;
+  // Implement StreamBase:
   bool IsAlive() override;
   bool IsClosing() override;
-
-  // JavaScript functions
-  int ReadStart() override;
-  int ReadStop() override;
-
+  bool IsIPCPipe() override;
+  int GetFD() override;
   ShutdownWrap* CreateShutdownWrap(
       v8::Local<v8::Object> req_wrap_object) override;
+  AsyncWrap* GetAsyncWrap() override;
+
+
+  // Implement StreamResource:
+  int ReadStart() override;  // Exposed to JS
+  int ReadStop() override;   // Exposed to JS
   int DoShutdown(ShutdownWrap* req_wrap) override;
   int DoWrite(WriteWrap* w,
               uv_buf_t* bufs,
@@ -77,14 +80,18 @@ class TLSWrap : public AsyncWrap,
   // Reset error_ string to empty. Not related to "clear text".
   void ClearError() override;
 
+
+  // Called by the done() callback of the 'newSession' event.
   void NewSessionDoneCb();
 
+  // Implement MemoryRetainer:
   void MemoryInfo(MemoryTracker* tracker) const override;
-
   SET_MEMORY_INFO_NAME(TLSWrap)
   SET_SELF_SIZE(TLSWrap)
 
  protected:
+  // Alternative to StreamListener::stream(), that returns a StreamBase instead
+  // of a StreamResource.
   inline StreamBase* underlying_stream() {
     return static_cast<StreamBase*>(stream_);
   }
@@ -136,13 +143,11 @@ class TLSWrap : public AsyncWrap,
     }
   }
 
-  AsyncWrap* GetAsyncWrap() override;
-  bool IsIPCPipe() override;
-
-  // Resource implementation
-  void OnStreamAfterWrite(WriteWrap* w, int status) override;
+  // Implement StreamListener:
+  // Returns buf that points into enc_in_.
   uv_buf_t OnStreamAlloc(size_t size) override;
   void OnStreamRead(ssize_t nread, const uv_buf_t& buf) override;
+  void OnStreamAfterWrite(WriteWrap* w, int status) override;
 
   v8::Local<v8::Value> GetSSLError(int status, int* err, std::string* msg);
 
@@ -153,7 +158,6 @@ class TLSWrap : public AsyncWrap,
   static void SetVerifyMode(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void EnableSessionCallbacks(
       const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void EnableTrace(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void EnableCertCb(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void DestroySSL(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void GetServername(const v8::FunctionCallbackInfo<v8::Value>& args);