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>

Author: sam-github

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_);
  }