From f993fca4e476c514f5a97f8a61287fc7f94df26f Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 11 Aug 2025 19:36:30 +0200 Subject: [PATCH] test: deflake sequential/test-tls-session-timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. PR-URL: https://github.com/nodejs/node/pull/59423 Fixes: https://github.com/nodejs/node/issues/26839 Reviewed-By: Juan José Arboleda Reviewed-By: Luigi Pinca Reviewed-By: Jacob Smith Reviewed-By: Stefan Stojanovic --- .../test-tls-session-timeout-errors.js | 36 +++++++ test/sequential/test-tls-session-timeout.js | 102 ++++++++---------- 2 files changed, 81 insertions(+), 57 deletions(-) create mode 100644 test/parallel/test-tls-session-timeout-errors.js diff --git a/test/parallel/test-tls-session-timeout-errors.js b/test/parallel/test-tls-session-timeout-errors.js new file mode 100644 index 00000000000..6e5646127c8 --- /dev/null +++ b/test/parallel/test-tls-session-timeout-errors.js @@ -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}`, + }); +} diff --git a/test/sequential/test-tls-session-timeout.js b/test/sequential/test-tls-session-timeout.js index a93cdc793a2..14baabd7a64 100644 --- a/test/sequential/test-tls-session-timeout.js +++ b/test/sequential/test-tls-session-timeout.js @@ -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,7 +56,7 @@ function doTest() { const fs = require('fs'); const spawn = require('child_process').spawn; - const SESSION_TIMEOUT = 1; + const SESSION_TIMEOUT = 5; const options = { key: key, @@ -85,32 +64,26 @@ function doTest() { 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. }); }); }