TLS: improve compliance with shutdown standard

RFC 5246 section-7.2.1 requires that the implementation must immediately
stop using the stream, as it is no longer TLS-encrypted. The stream is
permitted to still pump events (and errors) to other users, but those
are now unencrypted, so we should not process them here. But therefore,
we do not want to stop the underlying stream, as there could be another
user of it, but we do remove ourselves as a listener.

The section also states that the application must destroy the stream
immediately (discarding any pending writes, and sending a close_notify
response back), but we leave that to the upper layer of the application
here, as it should be sufficient to permit standards compliant usage
just to be ignoring read events.

Fixes: nodejs/node#35904
Closes: nodejs/node#35946
Co-authored-by: Momtchil Momtchev <[email protected]>

Author: vtjnash

lib/_http_client.js

@@ -409,11 +409,6 @@ function socketCloseListener() {
   const req = socket._httpMessage;
   debug('HTTP socket close');
 
-  // Pull through final chunk, if anything is buffered.
-  // the ondata function will handle it properly, and this
-  // is a no-op if no final chunk remains.
-  socket.read();
-
   // NOTE: It's important to get parser here, because it could be freed by
   // the `socketOnData`.
   const parser = socket.parser;

lib/internal/stream_base_commons.js

@@ -207,6 +207,9 @@ function onStreamRead(arrayBuffer) {
     return;
   }
 
+  // note: handle should never deliver any more read events after this point.
+  // (equivalently, it should have called readStop on itself alread).
+
   if (nread !== UV_EOF) {
     // CallJSOnreadMethod expects the return value to be a buffer.
     // Ref: https://github.com/nodejs/node/pull/34375
@@ -222,20 +225,6 @@ function onStreamRead(arrayBuffer) {
     if (stream[kMaybeDestroy])
       stream.on('end', stream[kMaybeDestroy]);
 
-    // TODO(ronag): Without this `readStop`, `onStreamRead`
-    // will be called once more (i.e. after Readable.ended)
-    // on Windows causing a ECONNRESET, failing the
-    // test-https-truncate test.
-    if (handle.readStop) {
-      const err = handle.readStop();
-      if (err) {
-        // CallJSOnreadMethod expects the return value to be a buffer.
-        // Ref: https://github.com/nodejs/node/pull/34375
-        stream.destroy(errnoException(err, 'read'));
-        return;
-      }
-    }
-
     // Push a null to signal the end of data.
     // Do it before `maybeDestroy` for correct order of events:
     // `end` -> `close`

src/crypto/crypto_tls.cc

@@ -808,6 +808,11 @@ void TLSWrap::ClearOut() {
   int flags = SSL_get_shutdown(ssl_.get());
   if (!eof_ && flags & SSL_RECEIVED_SHUTDOWN) {
     eof_ = true;
+    // FIXME: rfc5246#section-7.2.1 says we should call Destroy
+    // here, but Shutdown and Destroy in this class also affect the
+    // underlying stream. That behavior is permitted by the
+    // standard of the application protocol, but not of nodejs at
+    // the protocol layer in this file.
     EmitRead(UV_EOF);
   }
 
@@ -921,7 +926,7 @@ bool TLSWrap::IsClosing() {
 
 int TLSWrap::ReadStart() {
   Debug(this, "ReadStart()");
-  if (underlying_stream() != nullptr)
+  if (underlying_stream() != nullptr && !eof_)
     return underlying_stream()->ReadStart();
   return 0;
 }
@@ -1080,14 +1085,18 @@ uv_buf_t TLSWrap::OnStreamAlloc(size_t suggested_size) {
 
 void TLSWrap::OnStreamRead(ssize_t nread, const uv_buf_t& buf) {
   Debug(this, "Read %zd bytes from underlying stream", nread);
+
+  // Ignore everything after close_notify (rfc5246#section-7.2.1)
+  // TODO: unregister our interest in read events
+  if (eof_)
+    return;
+
   if (nread < 0)  {
     // Error should be emitted only after all data was read
     ClearOut();
 
-    // Ignore EOF if received close_notify
     if (nread == UV_EOF) {
-      if (eof_)
-        return;
+      // underlying stream already should have also called ReadStop on itself
       eof_ = true;
     }
 

test/parallel/test-https-agent-jssocket-close.js

@@ -0,0 +1,57 @@
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+const https = require('https');
+const key = fixtures.readKey('agent1-key.pem');
+const cert = fixtures.readKey('agent1-cert.pem');
+const net = require('net');
+const { Duplex } = require('stream');
+
+class CustomAgent extends https.Agent {
+  createConnection(options, cb) {
+    const realSocket = net.createConnection(options);
+    const stream = new Duplex({
+      emitClose: false,
+      read(n) {
+        (function retry() {
+          const data = realSocket.read();
+          if (data === null)
+            return realSocket.once('readable', retry);
+          stream.push(data);
+        })();
+      },
+      write(chunk, enc, callback) {
+        realSocket.write(chunk, enc, callback);
+      },
+    });
+    realSocket.on('end', () => {
+        stream.push(null);
+    });
+
+    stream.on('end', common.mustCall());
+    return tls.connect({ ...options, socket: stream });
+  }
+}
+
+const httpsServer = https.createServer({
+  key,
+  cert,
+}, (req, res) => {
+  httpsServer.close();
+  res.end('hello world!');
+});
+httpsServer.listen(0, 'localhost', () => {
+  const agent = new CustomAgent();
+  https.get({
+    host: 'localhost',
+    port: httpsServer.address().port,
+    agent,
+    headers: { Connection: 'close' },
+    rejectUnauthorized: false
+  }, (res) => {
+    res.resume();
+  });
+});