Skip to content

Commit 7c189d4

Browse files
joyeecheungRafaelGSS
joyeecheung
authored and
RafaelGSS
committed
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. PR-URL: #59423 Fixes: #26839 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
1 parent 269cd16 commit 7c189d4

File tree

2 files changed

+81
-57
lines changed

2 files changed

+81
-57
lines changed

‎test/parallel/test-tls-session-timeout-errors.js

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
'use strict';
2+
// This tests validation of sessionTimeout option in TLS server.
3+
const common = require('../common');
4+
5+
if (!common.hasCrypto) {
6+
common.skip('missing crypto');
7+
}
8+
9+
const tmpdir = require('../common/tmpdir');
10+
tmpdir.refresh();
11+
12+
const assert = require('assert');
13+
const tls = require('tls');
14+
const fixtures = require('../common/fixtures');
15+
16+
const key = fixtures.readKey('rsa_private.pem');
17+
const cert = fixtures.readKey('rsa_cert.crt');
18+
19+
// Node.js should not allow setting negative timeouts since new versions of
20+
// OpenSSL do not handle those as users might expect
21+
22+
for (const sessionTimeout of [-1, -100, -(2 ** 31)]) {
23+
assert.throws(() => {
24+
tls.createServer({
25+
key: key,
26+
cert: cert,
27+
ca: [cert],
28+
sessionTimeout,
29+
maxVersion: 'TLSv1.2',
30+
});
31+
}, {
32+
code: 'ERR_OUT_OF_RANGE',
33+
message: 'The value of "options.sessionTimeout" is out of range. It ' +
34+
`must be >= 0 && <= ${2 ** 31 - 1}. Received ${sessionTimeout}`,
35+
});
36+
}

‎test/sequential/test-tls-session-timeout.js

Lines changed: 45 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -35,34 +35,13 @@ const assert = require('assert');
3535
const tls = require('tls');
3636
const fixtures = require('../common/fixtures');
3737

38-
const key = fixtures.readKey('rsa_private.pem');
39-
const cert = fixtures.readKey('rsa_cert.crt');
40-
41-
{
42-
// Node.js should not allow setting negative timeouts since new versions of
43-
// OpenSSL do not handle those as users might expect
44-
45-
for (const sessionTimeout of [-1, -100, -(2 ** 31)]) {
46-
assert.throws(() => {
47-
tls.createServer({
48-
key: key,
49-
cert: cert,
50-
ca: [cert],
51-
sessionTimeout,
52-
maxVersion: 'TLSv1.2',
53-
});
54-
}, {
55-
code: 'ERR_OUT_OF_RANGE',
56-
message: 'The value of "options.sessionTimeout" is out of range. It ' +
57-
`must be >= 0 && <= ${2 ** 31 - 1}. Received ${sessionTimeout}`,
58-
});
59-
}
60-
}
61-
6238
if (!opensslCli) {
6339
common.skip('node compiled without OpenSSL CLI.');
6440
}
6541

42+
const key = fixtures.readKey('rsa_private.pem');
43+
const cert = fixtures.readKey('rsa_cert.crt');
44+
6645
doTest();
6746

6847
// This test consists of three TLS requests --
@@ -77,40 +56,34 @@ function doTest() {
7756
const fs = require('fs');
7857
const spawn = require('child_process').spawn;
7958

80-
const SESSION_TIMEOUT = 1;
59+
const SESSION_TIMEOUT = 5;
8160

8261
const options = {
8362
key: key,
8463
cert: cert,
8564
ca: [cert],
8665
sessionTimeout: SESSION_TIMEOUT,
8766
maxVersion: 'TLSv1.2',
67+
sessionIdContext: 'test-session-timeout',
8868
};
8969

90-
// We need to store a sample session ticket in the fixtures directory because
91-
// `s_client` behaves incorrectly if we do not pass in both the `-sess_in`
92-
// and the `-sess_out` flags, and the `-sess_in` argument must point to a
93-
// file containing a proper serialization of a session ticket.
94-
// To avoid a source control diff, we copy the ticket to a temporary file.
95-
96-
const sessionFileName = (function() {
97-
const ticketFileName = 'tls-session-ticket.txt';
98-
const tmpPath = tmpdir.resolve(ticketFileName);
99-
fs.writeFileSync(tmpPath, fixtures.readSync(ticketFileName));
100-
return tmpPath;
101-
}());
102-
103-
// Expects a callback -- cb(connectionType : enum ['New'|'Reused'])
104-
105-
function Client(cb) {
70+
const sessionFileName = tmpdir.resolve('tls-session-ticket.txt');
71+
// Expects a callback -- cb()
72+
function Client(port, sessIn, sessOut, expectedType, cb) {
10673
const flags = [
10774
's_client',
108-
'-connect', `localhost:${common.PORT}`,
109-
'-sess_in', sessionFileName,
110-
'-sess_out', sessionFileName,
75+
'-connect', `localhost:${port}`,
76+
'-CAfile', fixtures.path('keys', 'rsa_cert.crt'),
77+
'-servername', 'localhost',
11178
];
79+
if (sessIn) {
80+
flags.push('-sess_in', sessIn);
81+
}
82+
if (sessOut) {
83+
flags.push('-sess_out', sessOut);
84+
}
11285
const client = spawn(opensslCli, flags, {
113-
stdio: ['ignore', 'pipe', 'ignore']
86+
stdio: ['ignore', 'pipe', 'inherit']
11487
});
11588

11689
let clientOutput = '';
@@ -119,6 +92,20 @@ function doTest() {
11992
});
12093
client.on('exit', (code) => {
12194
let connectionType;
95+
// Log the output for debugging purposes. Don't remove them or otherwise
96+
// the CI output is useless when this test flakes.
97+
console.log(' ----- [COMMAND] ---');
98+
console.log(`${opensslCli}, ${flags.join(' ')}`);
99+
console.log(' ----- [STDOUT] ---');
100+
console.log(clientOutput);
101+
console.log(' ----- [SESSION FILE] ---');
102+
try {
103+
const stat = fs.statSync(sessionFileName);
104+
console.log(`Session file size: ${stat.size} bytes`);
105+
} catch (err) {
106+
console.log('Error reading session file:', err);
107+
}
108+
122109
const grepConnectionType = (line) => {
123110
const matches = line.match(/(New|Reused), /);
124111
if (matches) {
@@ -131,6 +118,7 @@ function doTest() {
131118
throw new Error('unexpected output from openssl client');
132119
}
133120
assert.strictEqual(code, 0);
121+
assert.strictEqual(connectionType, expectedType);
134122
cb(connectionType);
135123
});
136124
}
@@ -143,18 +131,18 @@ function doTest() {
143131
cleartext.end();
144132
});
145133

146-
server.listen(common.PORT, () => {
147-
Client((connectionType) => {
148-
assert.strictEqual(connectionType, 'New');
149-
Client((connectionType) => {
150-
assert.strictEqual(connectionType, 'Reused');
151-
setTimeout(() => {
152-
Client((connectionType) => {
153-
assert.strictEqual(connectionType, 'New');
154-
server.close();
155-
});
156-
}, (SESSION_TIMEOUT + 1) * 1000);
157-
});
134+
server.listen(0, () => {
135+
const port = server.address().port;
136+
Client(port, undefined, sessionFileName, 'New', () => {
137+
setTimeout(() => {
138+
Client(port, sessionFileName, sessionFileName, 'Reused', () => {
139+
setTimeout(() => {
140+
Client(port, sessionFileName, sessionFileName, 'New', () => {
141+
server.close();
142+
});
143+
}, (SESSION_TIMEOUT + 1) * 1000);
144+
});
145+
}, 100); // Wait a bit to ensure the session ticket is saved.
158146
});
159147
});
160148
}

0 commit comments

Comments
 (0)