tls: coalesce buffered records into one read

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,57 @@ 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. This is limited to application data records that are too
+    // long to be empty: other record types can trigger callbacks, e.g. for
+    // renegotiation, that have to run after the preceding data has been
+    // emitted, and SSL_read() skips over empty records to whatever follows.
+    size_t pending;
+    const unsigned char* next = reinterpret_cast<const unsigned char*>(
+        NodeBIO::FromBIO(enc_in_)->Peek(&pending));
+    if (length < kClearOutChunkSize && pending >= SSL3_RT_HEADER_LENGTH &&
+        next[0] == SSL3_RT_APPLICATION_DATA &&
+        (next[3] << 8 | next[4]) > SSL3_RT_MAX_ENCRYPTED_OVERHEAD) {
+      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 +1000,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-double-tls-client.js

@@ -23,8 +23,15 @@ const server = tls.createServer({
 })
   .listen(client)
   .on('secureConnection', common.mustCall((serverTlsSock) => {
-    serverTlsSock.on('data', common.mustCallAtLeast((chunk) => {
-      assert.strictEqual(chunk.toString(), HEAD);
+    serverTlsSock.once('data', common.mustCall((chunk) => {
+      assert.strictEqual(chunk.toString('latin1', 0, HEAD.length), HEAD);
+      // The start of the inner handshake can arrive in the same chunk as
+      // HEAD. Put it back so that the inner TLSSocket, which reads whatever
+      // is buffered on the socket it wraps, receives it.
+      if (chunk.length > HEAD.length) {
+        serverTlsSock.pause();
+        serverTlsSock.unshift(chunk.subarray(HEAD.length));
+      }
       subserver.emit('connection', serverTlsSock);
     }));
   }));

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

@@ -0,0 +1,130 @@
+'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 { isBoringSSL } = require('../common/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.
+{
+  // Small enough to arrive in one read and to fit into one batch.
+  const data = payload.subarray(0, 8 * RECORD_SIZE);
+  const server = tls.createServer(serverOptions, common.mustCall((socket) => {
+    socket.once('data', common.mustCall(() => {
+      assert(socket.setMaxSendFragment(RECORD_SIZE));
+      socket.end(data);
+    }));
+  }));
+  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 data record (the one
+    // before 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 response = Buffer.concat(held);
+        let closeNotify = 0;
+        for (let i = 0; i < response.length; i += 5 + response.readUInt16BE(i + 3))
+          closeNotify = i;
+        response[closeNotify - 1] ^= 1;
+        socket.end(response);
+      }));
+    }));
+    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, isBoringSSL ?
+          'ERR_SSL_BAD_DECRYPT' :
+          'ERR_SSL_DECRYPTION_FAILED_OR_BAD_RECORD_MAC');
+        assert.deepStrictEqual(Buffer.concat(chunks),
+                               data.subarray(0, -RECORD_SIZE));
+        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')));
+      }));
+    }));
+  }));
+}));