Repository: nodejs/node
Pull request: test: deflake sequential/test-tls-session-timeout

This patch:

- Splits the validation tests into a separate file and keep the
  test focus on functional test of the sessionTimeout option.
- Increase the testing timeout to 5 seconds in case it takes too
  long for the first connection to complete and the session is
  already expired when the second connection is started.
- Use a specific `sessionIdContext` to ensure stable session ID.
- Fix the s_client arguments by specifying CA file and server name.
- Do not use the serialized session ticket for the first connection.
  That was genearted years ago and may not work in different OpenSSL
  versions. Let the first fresh connection generate the ticket.
- Use random port instead of the common port.
- Add a timeout before the second connection to ensure session ticket
  is properly written.
- Log information to faciliate debugging.

Fixes: https://github.com/nodejs/node/issues/26839

---

<del>I have no idea if this actually fixes the test because I can't reproduce it locally. I just noticed that it's missing the `-reconnect` option which is theoratically necessary if you actually want session reuse and I figured adding it back might do something. This test has been failing pretty badly (failing 28 PRs out of the last 100 CI runs! https://github.com/nodejs/reliability/issues/1282) so any attempt seems useful  ¯\\_(ツ)_/¯. Another idea is probably adding a setTimeout after the first connection. </del>


<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: test/parallel/test-tls-session-timeout-errors.js
patch: @@ -0,0 +1,36 @@
+'use strict';
+// This tests validation of sessionTimeout option in TLS server.
+const common = require('../common');
+
+if (!common.hasCrypto) {
+  common.skip('missing crypto');
+}
+
+const tmpdir = require('../common/tmpdir');
+tmpdir.refresh();
+
+const assert = require('assert');
+const tls = require('tls');
+const fixtures = require('../common/fixtures');
+
+const key = fixtures.readKey('rsa_private.pem');
+const cert = fixtures.readKey('rsa_cert.crt');
+
+// Node.js should not allow setting negative timeouts since new versions of
+// OpenSSL do not handle those as users might expect
+
+for (const sessionTimeout of [-1, -100, -(2 ** 31)]) {
+  assert.throws(() => {
+    tls.createServer({
+      key: key,
+      cert: cert,
+      ca: [cert],
+      sessionTimeout,
+      maxVersion: 'TLSv1.2',
+    });
+  }, {
+    code: 'ERR_OUT_OF_RANGE',
+    message: 'The value of "options.sessionTimeout" is out of range. It ' +
+              `must be >= 0 && <= ${2 ** 31 - 1}. Received ${sessionTimeout}`,
+  });
+}

filename: test/sequential/test-tls-session-timeout.js
patch: @@ -35,34 +35,13 @@ const assert = require('assert');
 const tls = require('tls');
 const fixtures = require('../common/fixtures');
 
-const key = fixtures.readKey('rsa_private.pem');
-const cert = fixtures.readKey('rsa_cert.crt');
-
-{
-  // Node.js should not allow setting negative timeouts since new versions of
-  // OpenSSL do not handle those as users might expect
-
-  for (const sessionTimeout of [-1, -100, -(2 ** 31)]) {
-    assert.throws(() => {
-      tls.createServer({
-        key: key,
-        cert: cert,
-        ca: [cert],
-        sessionTimeout,
-        maxVersion: 'TLSv1.2',
-      });
-    }, {
-      code: 'ERR_OUT_OF_RANGE',
-      message: 'The value of "options.sessionTimeout" is out of range. It ' +
-               `must be >= 0 && <= ${2 ** 31 - 1}. Received ${sessionTimeout}`,
-    });
-  }
-}
-
 if (!opensslCli) {
   common.skip('node compiled without OpenSSL CLI.');
 }
 
+const key = fixtures.readKey('rsa_private.pem');
+const cert = fixtures.readKey('rsa_cert.crt');
+
 doTest();
 
 // This test consists of three TLS requests --
@@ -77,40 +56,34 @@ function doTest() {
   const fs = require('fs');
   const spawn = require('child_process').spawn;
 
-  const SESSION_TIMEOUT = 1;
+  const SESSION_TIMEOUT = 5;
 
   const options = {
     key: key,
     cert: cert,
     ca: [cert],
     sessionTimeout: SESSION_TIMEOUT,
     maxVersion: 'TLSv1.2',
+    sessionIdContext: 'test-session-timeout',
   };
 
-  // We need to store a sample session ticket in the fixtures directory because
-  // `s_client` behaves incorrectly if we do not pass in both the `-sess_in`
-  // and the `-sess_out` flags, and the `-sess_in` argument must point to a
-  // file containing a proper serialization of a session ticket.
-  // To avoid a source control diff, we copy the ticket to a temporary file.
-
-  const sessionFileName = (function() {
-    const ticketFileName = 'tls-session-ticket.txt';
-    const tmpPath = tmpdir.resolve(ticketFileName);
-    fs.writeFileSync(tmpPath, fixtures.readSync(ticketFileName));
-    return tmpPath;
-  }());
-
-  // Expects a callback -- cb(connectionType : enum ['New'|'Reused'])
-
-  function Client(cb) {
+  const sessionFileName = tmpdir.resolve('tls-session-ticket.txt');
+  // Expects a callback -- cb()
+  function Client(port, sessIn, sessOut, expectedType, cb) {
     const flags = [
       's_client',
-      '-connect', `localhost:${common.PORT}`,
-      '-sess_in', sessionFileName,
-      '-sess_out', sessionFileName,
+      '-connect', `localhost:${port}`,
+      '-CAfile', fixtures.path('keys', 'rsa_cert.crt'),
+      '-servername', 'localhost',
     ];
+    if (sessIn) {
+      flags.push('-sess_in', sessIn);
+    }
+    if (sessOut) {
+      flags.push('-sess_out', sessOut);
+    }
     const client = spawn(opensslCli, flags, {
-      stdio: ['ignore', 'pipe', 'ignore']
+      stdio: ['ignore', 'pipe', 'inherit']
     });
 
     let clientOutput = '';
@@ -119,6 +92,20 @@ function doTest() {
     });
     client.on('exit', (code) => {
       let connectionType;
+      // Log the output for debugging purposes. Don't remove them or otherwise
+      // the CI output is useless when this test flakes.
+      console.log(' ----- [COMMAND] ---');
+      console.log(`${opensslCli}, ${flags.join(' ')}`);
+      console.log(' ----- [STDOUT] ---');
+      console.log(clientOutput);
+      console.log(' ----- [SESSION FILE] ---');
+      try {
+        const stat = fs.statSync(sessionFileName);
+        console.log(`Session file size: ${stat.size} bytes`);
+      } catch (err) {
+        console.log('Error reading session file:', err);
+      }
+
       const grepConnectionType = (line) => {
         const matches = line.match(/(New|Reused), /);
         if (matches) {
@@ -131,6 +118,7 @@ function doTest() {
         throw new Error('unexpected output from openssl client');
       }
       assert.strictEqual(code, 0);
+      assert.strictEqual(connectionType, expectedType);
       cb(connectionType);
     });
   }
@@ -143,18 +131,18 @@ function doTest() {
     cleartext.end();
   });
 
-  server.listen(common.PORT, () => {
-    Client((connectionType) => {
-      assert.strictEqual(connectionType, 'New');
-      Client((connectionType) => {
-        assert.strictEqual(connectionType, 'Reused');
-        setTimeout(() => {
-          Client((connectionType) => {
-            assert.strictEqual(connectionType, 'New');
-            server.close();
-          });
-        }, (SESSION_TIMEOUT + 1) * 1000);
-      });
+  server.listen(0, () => {
+    const port = server.address().port;
+    Client(port, undefined, sessionFileName, 'New', () => {
+      setTimeout(() => {
+        Client(port, sessionFileName, sessionFileName, 'Reused', () => {
+          setTimeout(() => {
+            Client(port, sessionFileName, sessionFileName, 'New', () => {
+              server.close();
+            });
+          }, (SESSION_TIMEOUT + 1) * 1000);
+        });
+      }, 100);  // Wait a bit to ensure the session ticket is saved.
     });
   });
 }