tls: coalesce buffered records into one read

SSL_read() returns at most one TLS record, and ClearOut() emitted a
separate read for each one. Peers that use small records, such as
nginx with its default 4 KiB ssl_buffer_size or servers doing dynamic
record sizing, made every record pay for its own buffer allocation
and call into JavaScript.

While the next buffered record is application data, keep reading into
the same 16 KiB buffer before emitting it, so reads never get larger
than one full record already makes them. Batching stops before any
other record type, so that e.g. a TLS 1.2 renegotiation is still
processed after the data in front of it has been emitted. If
SSL_read() fails in the middle of a batch, the error is captured
first and reported after the data.

test-tls-max-send-fragment relied on 'data' chunks matching records,
so it now checks the record sizes on the wire.

Signed-off-by: Robert Nagy <ronagy@icloud.com>
Assisted-by: claude:opus-5.5

Author: ronag

benchmark/tls/throughput-s2c.js

@@ -6,6 +6,7 @@ const bench = common.createBenchmark(main, {
   sendchunklen: [256, 32 * 1024, 128 * 1024, 16 * 1024 * 1024],
   recvbuflen: [0, 64 * 1024, 1024 * 1024],
   recvbufgenfn: ['true', 'false'],
+  recordlen: [1024, 4096, 16384],
 });
 
 const fixtures = require('../../test/common/fixtures');
@@ -14,7 +15,8 @@ let recvbuf;
 let received = 0;
 const tls = require('tls');
 
-function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn }) {
+function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn,
+                recordlen }) {
   if (isFinite(recvbuflen) && recvbuflen > 0)
     recvbuf = Buffer.alloc(recvbuflen);
 
@@ -74,6 +76,7 @@ function main({ dur, type, sendchunklen, recvbuflen, recvbufgenfn }) {
   }
 
   const server = tls.createServer(options, (socket) => {
+    socket.setMaxSendFragment(recordlen);
     socket.on('data', (buf) => {
       socket.on('drain', write);
       write();

src/crypto/crypto_tls.cc

@@ -880,20 +880,14 @@ void TLSWrap::ClearOut() {
   MarkPopErrorOnReturn mark_pop_error_on_return;
 
   char out[kClearOutChunkSize];
+  int length = 0;
   int read;
-  for (;;) {
-    {
-      SSLLibraryCallScope ssl_library_call_scope(this);
-      read = SSL_read(ssl_.get(), out, sizeof(out));
-    }
-    Debug(this, "Read %d bytes of cleartext output", read);
-
-    if (read <= 0)
-      break;
 
+  // Emits out[0, length). Returns false if JS destroyed the SSL context.
+  auto emit = [&]() {
     char* current = out;
-    while (read > 0) {
-      int avail = read;
+    while (length > 0) {
+      int avail = length;
 
       uv_buf_t buf = EmitAlloc(avail);
       if (static_cast<int>(buf.len) < avail)
@@ -906,26 +900,54 @@ void TLSWrap::ClearOut() {
       // check that ssl_ != nullptr afterwards.
       if (ssl_ == nullptr) {
         Debug(this, "Returning from read loop, ssl_ == nullptr");
-        return;
+        return false;
       }
 
-      read -= avail;
+      length -= avail;
       current += avail;
     }
+    return true;
+  };
+
+  for (;;) {
+    {
+      SSLLibraryCallScope ssl_library_call_scope(this);
+      read = SSL_read(ssl_.get(), out + length, kClearOutChunkSize - length);
+    }
+    Debug(this, "Read %d bytes of cleartext output", read);
+
+    if (read <= 0) break;
+    length += read;
+
+    // SSL_read() returns at most one record. If the next record is already
+    // buffered, decrypt it into the same chunk instead of emitting one read
+    // per record. Only application data is batched: other record types can
+    // trigger callbacks, e.g. for renegotiation, that have to run after the
+    // preceding data has been emitted.
+    size_t pending;
+    const char* next = NodeBIO::FromBIO(enc_in_)->Peek(&pending);
+    if (length < kClearOutChunkSize && pending > 0 &&
+        static_cast<unsigned char>(next[0]) == SSL3_RT_APPLICATION_DATA) {
+      continue;
+    }
+
+    if (!emit()) return;
   }
 
   // We need to check whether an error occurred or the connection was
   // shutdown cleanly (SSL_ERROR_ZERO_RETURN) even when read == 0.
   // See node#1642 and SSL_read(3SSL) for details. SSL_get_error must be
   // called immediately after SSL_read, without calling into JS, which may
   // change OpenSSL's error queue, modify ssl_, or even destroy ssl_
-  // altogether.
+  // altogether. Data that was read before the failure is emitted once the
+  // error has been captured, before it is reported.
   if (read <= 0) {
     HandleScope handle_scope(env()->isolate());
     Local<Value> error;
     int err = SSL_get_error(ssl_.get(), read);
     switch (err) {
       case SSL_ERROR_ZERO_RETURN:
+        if (!emit()) return;
         if (!flags_.eof) {
           flags_.eof = true;
           EmitRead(UV_EOF);
@@ -975,9 +997,12 @@ void TLSWrap::ClearOut() {
         break;
 
       default:
+        emit();
         return;
     }
 
+    if (!emit()) return;
+
     Debug(this, "Got SSL error (%d), calling onerror", err);
     // When TLS Alert are stored in wbio,
     // it should be flushed to socket before destroyed.

test/parallel/test-tls-coalesce-records.js

@@ -0,0 +1,119 @@
+'use strict';
+// TLS records that are already buffered are emitted as one chunk rather than
+// one chunk per record. The end of the stream, clean or not, must still be
+// reported after the data that precedes it.
+
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');
+const net = require('net');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+
+const RECORD_SIZE = 512;
+const RECORDS = 256;
+const payload = Buffer.alloc(RECORD_SIZE * RECORDS);
+for (let i = 0; i < RECORDS; i++)
+  payload.fill(i, i * RECORD_SIZE, (i + 1) * RECORD_SIZE);
+
+const serverOptions = {
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem'),
+};
+const clientOptions = {
+  ca: fixtures.readKey('ca1-cert.pem'),
+  servername: 'agent1',
+};
+
+function sendPayload(socket) {
+  assert(socket.setMaxSendFragment(RECORD_SIZE));
+  socket.end(payload);
+}
+
+// Small records are coalesced, and 'end' follows all of the data.
+{
+  const server = tls.createServer(serverOptions, common.mustCall(sendPayload));
+  server.listen(0, common.mustCall(() => {
+    const chunks = [];
+    const client = tls.connect({ ...clientOptions, port: server.address().port });
+    client.on('data', (chunk) => chunks.push(chunk));
+    client.on('end', common.mustCall(() => {
+      assert.deepStrictEqual(Buffer.concat(chunks), payload);
+      assert(chunks.length < RECORDS, `${chunks.length} chunks`);
+      assert(chunks.every((chunk) => chunk.length <= 16 * 1024));
+      server.close();
+    }));
+  }));
+}
+
+// An onread buffer smaller than a record still receives every byte.
+{
+  const server = tls.createServer(serverOptions, common.mustCall(sendPayload));
+  server.listen(0, common.mustCall(() => {
+    const chunks = [];
+    const client = tls.connect({
+      ...clientOptions,
+      port: server.address().port,
+      onread: {
+        buffer: Buffer.alloc(100),
+        callback(nread, buf) {
+          chunks.push(Buffer.from(buf.subarray(0, nread)));
+        },
+      },
+    });
+    client.on('end', common.mustCall(() => {
+      assert.deepStrictEqual(Buffer.concat(chunks), payload);
+      server.close();
+    }));
+  }));
+}
+
+// A bad record that arrives together with data is reported after the data.
+{
+  const server = tls.createServer(serverOptions, common.mustCall((socket) => {
+    socket.once('data', common.mustCall(() => sendPayload(socket)));
+  }));
+  server.listen(0, common.mustCall(() => {
+    // Passes the handshake through, then holds the response until the server
+    // ends and forwards it in one write, with the last record (close_notify)
+    // corrupted.
+    let holding = false;
+    const held = [];
+    const proxy = net.createServer(common.mustCall((socket) => {
+      const upstream = net.connect(server.address().port);
+      socket.pipe(upstream);
+      socket.on('error', () => {});
+      upstream.on('data', (chunk) => {
+        if (holding)
+          held.push(chunk);
+        else
+          socket.write(chunk);
+      });
+      upstream.on('end', common.mustCall(() => {
+        const data = Buffer.concat(held);
+        data[data.length - 1] ^= 1;
+        socket.end(data);
+      }));
+    }));
+    proxy.listen(0, common.mustCall(() => {
+      const chunks = [];
+      const client = tls.connect({
+        ...clientOptions,
+        port: proxy.address().port,
+      }, common.mustCall(() => {
+        holding = true;
+        client.write('go');
+      }));
+      client.on('data', (chunk) => chunks.push(chunk));
+      client.on('error', common.mustCall((err) => {
+        assert.strictEqual(err.code,
+                           'ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC');
+        assert.deepStrictEqual(Buffer.concat(chunks), payload);
+        proxy.close();
+        server.close();
+      }));
+    }));
+  }));
+}

test/parallel/test-tls-coalesce-renegotiation.js

@@ -0,0 +1,82 @@
+'use strict';
+const common = require('../common');
+if (!common.hasCrypto)
+  common.skip('missing crypto');
+const { isBoringSSL } = require('../common/crypto');
+if (isBoringSSL)
+  common.skip('BoringSSL does not support renegotiation');
+
+const assert = require('assert');
+const net = require('net');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+
+// Deliver application data and a TLS 1.2 ClientHello in one transport write.
+// The data handler must run before renegotiation starts, so it can disable it.
+const payload = Buffer.from('disable renegotiation');
+let holding = false;
+let client;
+
+const server = tls.createServer({
+  key: fixtures.readKey('agent1-key.pem'),
+  cert: fixtures.readKey('agent1-cert.pem'),
+  maxVersion: 'TLSv1.2',
+}, common.mustCall((socket) => {
+  socket.on('data', common.mustCall((data) => {
+    assert.deepStrictEqual(data, payload);
+    socket.disableRenegotiation();
+  }));
+  socket.on('secure', common.mustNotCall('renegotiation completed'));
+  socket.on('error', common.mustCall((err) => {
+    assert.strictEqual(err.code, 'ERR_TLS_RENEGOTIATION_DISABLED');
+    client.destroy();
+    socket.destroy();
+    server.close();
+    proxy.close();
+  }));
+}));
+
+const proxy = net.createServer(common.mustCall((socket) => {
+  const upstream = net.connect(server.address().port);
+  upstream.pipe(socket);
+  socket.on('end', () => upstream.end());
+  socket.on('close', () => upstream.destroy());
+  const held = [];
+  let pending = Buffer.alloc(0);
+  socket.on('data', (data) => {
+    if (!holding) {
+      upstream.write(data);
+      return;
+    }
+    held.push(data);
+    pending = Buffer.concat([pending, data]);
+    while (pending.length >= 5) {
+      const length = pending.readUInt16BE(3);
+      if (pending.length < 5 + length)
+        break;
+      if (pending[0] === 22) {  // handshake
+        holding = false;
+        upstream.write(Buffer.concat(held));
+        return;
+      }
+      pending = pending.subarray(5 + length);
+    }
+  });
+}));
+
+server.listen(0, common.mustCall(() => {
+  proxy.listen(0, common.mustCall(() => {
+    client = tls.connect({
+      port: proxy.address().port,
+      ca: fixtures.readKey('ca1-cert.pem'),
+      servername: 'agent1',
+      maxVersion: 'TLSv1.2',
+    }, common.mustCall(() => {
+      holding = true;
+      client.write(payload, common.mustCall(() => {
+        assert(client.renegotiate({},
+                                  common.mustNotCall('renegotiation completed')));
+      }));
+    }));
+  }));
+}));