tls: in-line comments and other cleanups

PR-URL: #25861
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Backport-PR-URL: #25968

Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

lib/_tls_wrap.js

@@ -59,6 +59,8 @@ const kSNICallback = Symbol('snicallback');
 
 const noop = () => {};
 
+// Server side times how long a handshake is taking to protect against slow
+// handshakes being used for DoS.
 function onhandshakestart(now) {
   debug('onhandshakestart');
 
@@ -118,13 +120,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();
   }
 }
@@ -219,13 +227,17 @@ function onnewsessionclient(sessionId, session) {
 }
 
 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;
@@ -316,8 +328,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 it 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.
@@ -337,7 +353,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);
 
@@ -433,8 +449,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;
@@ -484,8 +500,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;
 
@@ -506,6 +522,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)
@@ -728,7 +745,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) {
@@ -779,7 +796,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

@@ -2643,47 +2643,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

@@ -735,7 +735,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();
@@ -744,7 +744,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

@@ -80,6 +80,8 @@ 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:
@@ -88,6 +90,8 @@ class TLSWrap : public AsyncWrap,
   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_);
   }