src: in-source comments and minor TLS cleanups

Renamed some internal C++ methods and properties for consistency, and
commented SSL I/O.

- Rename waiting_new_session_ after is_waiting_new_session(), instead of
  using reverse naming (new_session_wait_), and change "waiting" to
  "awaiting".
- Make TLSWrap::ClearIn() return void, the value is never used.
- Fix a getTicketKeys() cut-n-paste error. Since it doesn't use the
  arguments, remove them from the js wrapper.
- Remove call of setTicketKeys(getTicketKeys()), its a no-op.

PR-URL: https://github.com/nodejs/node/pull/25713
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

8 files changed, 84 insertions, 39 deletions

diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index ef2e5e3a7b..816fcb6896 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -1003,8 +1003,6 @@ Server.prototype.setSecureContext = function(options) {
   if (options.ticketKeys) {
     this.ticketKeys = options.ticketKeys;
     this.setTicketKeys(this.ticketKeys);
-  } else {
-    this.setTicketKeys(this.getTicketKeys());
   }
 };
 
@@ -1021,8 +1019,8 @@ Server.prototype._setServerData = function(data) {
 };
 
 
-Server.prototype.getTicketKeys = function getTicketKeys(keys) {
-  return this._sharedCreds.context.getTicketKeys(keys);
+Server.prototype.getTicketKeys = function getTicketKeys() {
+  return this._sharedCreds.context.getTicketKeys();
 };
 
 
diff --git a/src/node_crypto.cc b/src/node_crypto.cc
index 01593914a1..0b6c6c8582 100644
--- a/src/node_crypto.cc
+++ b/src/node_crypto.cc
@@ -1532,7 +1532,7 @@ int SSLWrap<Base>::NewSessionCallback(SSL* s, SSL_SESSION* sess) {
       reinterpret_cast<const char*>(session_id_data),
       session_id_length).ToLocalChecked();
   Local<Value> argv[] = { session_id, session };
-  w->new_session_wait_ = true;
+  w->awaiting_new_session_ = true;
   w->MakeCallback(env->onnewsession_string(), arraysize(argv), argv);
 
   return 0;
@@ -2128,6 +2128,7 @@ void SSLWrap<Base>::Renegotiate(const FunctionCallbackInfo<Value>& args) {
 
   ClearErrorOnReturn clear_error_on_return;
 
+  // XXX(sam) Return/throw an error, don't discard the SSL error reason.
   bool yes = SSL_renegotiate(w->ssl_.get()) == 1;
   args.GetReturnValue().Set(yes);
 }
@@ -2161,7 +2162,7 @@ template <class Base>
 void SSLWrap<Base>::NewSessionDone(const FunctionCallbackInfo<Value>& args) {
   Base* w;
   ASSIGN_OR_RETURN_UNWRAP(&w, args.Holder());
-  w->new_session_wait_ = false;
+  w->awaiting_new_session_ = false;
   w->NewSessionDoneCb();
 }
 
diff --git a/src/node_crypto.h b/src/node_crypto.h
index 1b950846a7..32b6188259 100644
--- a/src/node_crypto.h
+++ b/src/node_crypto.h
@@ -218,7 +218,7 @@ class SSLWrap {
         kind_(kind),
         next_sess_(nullptr),
         session_callbacks_(false),
-        new_session_wait_(false),
+        awaiting_new_session_(false),
         cert_cb_(nullptr),
         cert_cb_arg_(nullptr),
         cert_cb_running_(false) {
@@ -234,7 +234,7 @@ class SSLWrap {
   inline void enable_session_callbacks() { session_callbacks_ = true; }
   inline bool is_server() const { return kind_ == kServer; }
   inline bool is_client() const { return kind_ == kClient; }
-  inline bool is_waiting_new_session() const { return new_session_wait_; }
+  inline bool is_awaiting_new_session() const { return awaiting_new_session_; }
   inline bool is_waiting_cert_cb() const { return cert_cb_ != nullptr; }
 
  protected:
@@ -325,7 +325,7 @@ class SSLWrap {
   SSLSessionPointer next_sess_;
   SSLPointer ssl_;
   bool session_callbacks_;
-  bool new_session_wait_;
+  bool awaiting_new_session_;
 
   // SSL_set_cert_cb
   CertCb cert_cb_;
diff --git a/src/node_crypto_bio.h b/src/node_crypto_bio.h
index 1c62fbbd35..b7f1d4f169 100644
--- a/src/node_crypto_bio.h
+++ b/src/node_crypto_bio.h
@@ -34,8 +34,8 @@ namespace node {
 namespace crypto {
 
 // This class represents buffers for OpenSSL I/O, implemented as a singly-linked
-// list of chunks. It can be used both for writing data from Node to OpenSSL
-// and back, but only one direction per instance.
+// list of chunks. It can be used either for writing data from Node to OpenSSL,
+// or for reading data back, but not both.
 // The structure is only accessed, and owned by, the OpenSSL BIOPointer
 // (a.k.a. std::unique_ptr<BIO>).
 class NodeBIO : public MemoryRetainer {
@@ -80,11 +80,12 @@ class NodeBIO : public MemoryRetainer {
   // Put `len` bytes from `data` into buffer
   void Write(const char* data, size_t size);
 
-  // Return pointer to internal data and amount of
-  // contiguous data available for future writes
+  // Return pointer to contiguous block of reserved data and the size available
+  // for future writes. Call Commit() once the write is complete.
   char* PeekWritable(size_t* size);
 
-  // Commit reserved data
+  // Specify how much data was written into the block returned by
+  // PeekWritable().
   void Commit(size_t size);
 
 
diff --git a/src/node_crypto_clienthello.h b/src/node_crypto_clienthello.h
index 2ced72c4e8..48f49771fd 100644
--- a/src/node_crypto_clienthello.h
+++ b/src/node_crypto_clienthello.h
@@ -30,6 +30,9 @@
 namespace node {
 namespace crypto {
 
+// Parse the client hello so we can do async session resumption. OpenSSL's
+// session resumption uses synchronous callbacks, see SSL_CTX_sess_set_get_cb
+// and get_session_cb.
 class ClientHelloParser {
  public:
   inline ClientHelloParser();
diff --git a/src/stream_wrap.cc b/src/stream_wrap.cc
index ae54b019fe..627bcf5949 100644
--- a/src/stream_wrap.cc
+++ b/src/stream_wrap.cc
@@ -232,7 +232,7 @@ void LibuvStreamWrap::OnUvRead(ssize_t nread, const uv_buf_t* buf) {
     type = uv_pipe_pending_type(reinterpret_cast<uv_pipe_t*>(stream()));
   }
 
-  // We should not be getting this callback if someone as already called
+  // We should not be getting this callback if someone has already called
   // uv_close() on the handle.
   CHECK_EQ(persistent().IsEmpty(), false);
 
diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc
index d86e31c6af..e467e2d167 100644
--- a/src/tls_wrap.cc
+++ b/src/tls_wrap.cc
@@ -115,6 +115,11 @@ void TLSWrap::InitSSL() {
 #endif  // SSL_MODE_RELEASE_BUFFERS
 
   SSL_set_app_data(ssl_.get(), this);
+  // Using InfoCallback isn't how we are supposed to check handshake progress:
+  //   https://github.com/openssl/openssl/issues/7199#issuecomment-420915993
+  //
+  // Note on when this gets called on various openssl versions:
+  //   https://github.com/openssl/openssl/issues/7199#issuecomment-420670544
   SSL_set_info_callback(ssl_.get(), SSLInfoCallback);
 
   if (is_server()) {
@@ -193,6 +198,9 @@ void TLSWrap::Start(const FunctionCallbackInfo<Value>& args) {
 
   // Send ClientHello handshake
   CHECK(wrap->is_client());
+  // Seems odd to read when when we want to send, but SSL_read() triggers a
+  // handshake if a session isn't established, and handshake will cause
+  // encrypted data to become available for output.
   wrap->ClearOut();
   wrap->EncOut();
 }
@@ -247,7 +255,7 @@ void TLSWrap::EncOut() {
     return;
 
   // Wait for `newSession` callback to be invoked
-  if (is_waiting_new_session())
+  if (is_awaiting_new_session())
     return;
 
   // Split-off queue
@@ -257,7 +265,7 @@ void TLSWrap::EncOut() {
   if (ssl_ == nullptr)
     return;
 
-  // No data to write
+  // No encrypted output ready to write to the underlying stream.
   if (BIO_pending(enc_out_) == 0) {
     if (pending_cleartext_input_.empty())
       InvokeQueued(0);
@@ -479,13 +487,13 @@ void TLSWrap::ClearOut() {
 }
 
 
-bool TLSWrap::ClearIn() {
+void TLSWrap::ClearIn() {
   // Ignore cycling data if ClientHello wasn't yet parsed
   if (!hello_parser_.IsEnded())
-    return false;
+    return;
 
   if (ssl_ == nullptr)
-    return false;
+    return;
 
   std::vector<uv_buf_t> buffers;
   buffers.swap(pending_cleartext_input_);
@@ -505,8 +513,9 @@ bool TLSWrap::ClearIn() {
 
   // All written
   if (i == buffers.size()) {
+    // We wrote all the buffers, so no writes failed (written < 0 on failure).
     CHECK_GE(written, 0);
-    return true;
+    return;
   }
 
   // Error or partial write
@@ -518,6 +527,8 @@ bool TLSWrap::ClearIn() {
   Local<Value> arg = GetSSLError(written, &err, &error_str);
   if (!arg.IsEmpty()) {
     write_callback_scheduled_ = true;
+    // XXX(sam) Should forward an error object with .code/.function/.etc, if
+    // possible.
     InvokeQueued(UV_EPROTO, error_str.c_str());
   } else {
     // Push back the not-yet-written pending buffers into their queue.
@@ -528,7 +539,7 @@ bool TLSWrap::ClearIn() {
                                     buffers.end());
   }
 
-  return false;
+  return;
 }
 
 
@@ -584,6 +595,7 @@ void TLSWrap::ClearError() {
 }
 
 
+// Called by StreamBase::Write() to request async write of clear text into SSL.
 int TLSWrap::DoWrite(WriteWrap* w,
                      uv_buf_t* bufs,
                      size_t count,
@@ -597,18 +609,26 @@ int TLSWrap::DoWrite(WriteWrap* w,
   }
 
   bool empty = true;
-
-  // Empty writes should not go through encryption process
   size_t i;
-  for (i = 0; i < count; i++)
+  for (i = 0; i < count; i++) {
     if (bufs[i].len > 0) {
       empty = false;
       break;
     }
+  }
+
+  // We want to trigger a Write() on the underlying stream to drive the stream
+  // system, but don't want to encrypt empty buffers into a TLS frame, so see
+  // if we can find something to Write().
+  // First, call ClearOut(). It does an SSL_read(), which might cause handshake
+  // or other internal messages to be encrypted. If it does, write them later
+  // with EncOut().
+  // If there is still no encrypted output, call Write(bufs) on the underlying
+  // stream. Since the bufs are empty, it won't actually write non-TLS data
+  // onto the socket, we just want the side-effects. After, make sure the
+  // WriteWrap was accepted by the stream, or that we call Done() on it.
   if (empty) {
     ClearOut();
-    // However, if there is any data that should be written to the socket,
-    // the callback should not be invoked immediately
     if (BIO_pending(enc_out_) == 0) {
       CHECK_NULL(current_empty_write_);
       current_empty_write_ = w;
@@ -628,7 +648,7 @@ int TLSWrap::DoWrite(WriteWrap* w,
   CHECK_NULL(current_write_);
   current_write_ = w;
 
-  // Write queued data
+  // Write encrypted data to underlying stream and call Done().
   if (empty) {
     EncOut();
     return 0;
@@ -647,17 +667,20 @@ int TLSWrap::DoWrite(WriteWrap* w,
   if (i != count) {
     int err;
     Local<Value> arg = GetSSLError(written, &err, &error_);
+
+    // If we stopped writing because of an error, it's fatal, discard the data.
     if (!arg.IsEmpty()) {
       current_write_ = nullptr;
       return UV_EPROTO;
     }
 
+    // Otherwise, save unwritten data so it can be written later by ClearIn().
     pending_cleartext_input_.insert(pending_cleartext_input_.end(),
                                     &bufs[i],
                                     &bufs[count]);
   }
 
-  // Try writing data immediately
+  // Write any encrypted/handshake output that may be ready.
   EncOut();
 
   return 0;
@@ -689,17 +712,20 @@ void TLSWrap::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
     return;
   }
 
-  // Only client connections can receive data
   if (ssl_ == nullptr) {
     EmitRead(UV_EPROTO);
     return;
   }
 
-  // Commit read data
+  // Commit the amount of data actually read into the peeked/allocated buffer
+  // from the underlying stream.
   crypto::NodeBIO* enc_in = crypto::NodeBIO::FromBIO(enc_in_);
   enc_in->Commit(nread);
 
-  // Parse ClientHello first
+  // Parse ClientHello first, if we need to. It's only parsed if session event
+  // listeners are used on the server side.  "ended" is the initial state, so
+  // can mean parsing was never started, or that parsing is finished. Either
+  // way, ended means we can give the buffered data to SSL.
   if (!hello_parser_.IsEnded()) {
     size_t avail = 0;
     uint8_t* data = reinterpret_cast<uint8_t*>(enc_in->Peek(&avail));
diff --git a/src/tls_wrap.h b/src/tls_wrap.h
index 13f2bc1c71..cd2701cd6d 100644
--- a/src/tls_wrap.h
+++ b/src/tls_wrap.h
@@ -72,7 +72,9 @@ class TLSWrap : public AsyncWrap,
               uv_buf_t* bufs,
               size_t count,
               uv_stream_t* send_handle) override;
+  // Return error_ string or nullptr if it's empty.
   const char* Error() const override;
+  // Reset error_ string to empty. Not related to "clear text".
   void ClearError() override;
 
   void NewSessionDoneCb();
@@ -105,11 +107,22 @@ class TLSWrap : public AsyncWrap,
 
   static void SSLInfoCallback(const SSL* ssl_, int where, int ret);
   void InitSSL();
-  void EncOut();
-  bool ClearIn();
-  void ClearOut();
+  // SSL has a "clear" text (unencrypted) side (to/from the node API) and
+  // encrypted ("enc") text side (to/from the underlying socket/stream).
+  // On each side data flows "in" or "out" of SSL context.
+  //
+  // EncIn() doesn't exist. Encrypted data is pushed from underlying stream into
+  // enc_in_ via the stream listener's OnStreamAlloc()/OnStreamRead() interface.
+  void EncOut();  // Write encrypted data from enc_out_ to underlying stream.
+  void ClearIn();  // SSL_write() clear data "in" to SSL.
+  void ClearOut();  // SSL_read() clear text "out" from SSL.
+
+  // Call Done() on outstanding WriteWrap request.
   bool InvokeQueued(int status, const char* error_str = nullptr);
 
+  // Drive the SSL state machine by attempting to SSL_read() and SSL_write() to
+  // it. Transparent handshakes mean SSL_read() might trigger I/O on the
+  // underlying stream even if there is no clear text to read or write.
   inline void Cycle() {
     // Prevent recursion
     if (++cycle_depth_ > 1)
@@ -118,6 +131,7 @@ class TLSWrap : public AsyncWrap,
     for (; cycle_depth_ > 0; cycle_depth_--) {
       ClearIn();
       ClearOut();
+      // EncIn() doesn't exist, it happens via stream listener callbacks.
       EncOut();
     }
   }
@@ -139,16 +153,18 @@ class TLSWrap : public AsyncWrap,
   static void SetVerifyMode(const v8::FunctionCallbackInfo<v8::Value>& args);
   static void EnableSessionCallbacks(
       const v8::FunctionCallbackInfo<v8::Value>& args);
-  static void EnableCertCb(
-      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);
   static void SetServername(const v8::FunctionCallbackInfo<v8::Value>& args);
   static int SelectSNIContextCallback(SSL* s, int* ad, void* arg);
 
   crypto::SecureContext* sc_;
-  BIO* enc_in_ = nullptr;
-  BIO* enc_out_ = nullptr;
+  // BIO buffers hold encrypted data.
+  BIO* enc_in_ = nullptr;   // StreamListener fills this for SSL_read().
+  BIO* enc_out_ = nullptr;  // SSL_write()/handshake fills this for EncOut().
+  // Waiting for ClearIn() to pass to SSL_write().
   std::vector<uv_buf_t> pending_cleartext_input_;
   size_t write_size_ = 0;
   WriteWrap* current_write_ = nullptr;