Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 77def91

Browse filesBrowse files
tniessentniessen
authored
tls,http2: send fatal alert on ALPN mismatch
To comply with RFC 7301, make TLS servers send a fatal alert during the TLS handshake if both the client and the server are configured to use ALPN and if the server does not support any of the protocols advertised by the client. This affects HTTP/2 servers. Until now, applications could intercept the 'unknownProtocol' event when the client either did not advertise any protocols or if the list of protocols advertised by the client did not include HTTP/2 (or HTTP/1.1 if allowHTTP1 was true). With this change, only the first case can be handled, and the 'unknownProtocol' event will not be emitted in the second case because the TLS handshake fails and no secure connection is established. PR-URL: #44031 Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent 938212f commit 77def91
Copy full SHA for 77def91

File tree

Expand file treeCollapse file tree

7 files changed

+107
-19
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

7 files changed

+107
-19
lines changed
Open diff view settings
Collapse file

‎doc/api/http2.md‎

Copy file name to clipboardExpand all lines: doc/api/http2.md
+14Lines changed: 14 additions & 0 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -2275,6 +2275,11 @@ a given number of milliseconds set using `http2secureServer.setTimeout()`.
22752275

22762276
<!-- YAML
22772277
added: v8.4.0
2278+
changes:
2279+
- version: REPLACEME
2280+
pr-url: https://github.com/nodejs/node/pull/44031
2281+
description: This event will only be emitted if the client did not transmit
2282+
an ALPN extension during the TLS handshake.
22782283
-->
22792284

22802285
* `socket` {stream.Duplex}
@@ -2284,6 +2289,15 @@ negotiate an allowed protocol (i.e. HTTP/2 or HTTP/1.1). The event handler
22842289
receives the socket for handling. If no listener is registered for this event,
22852290
the connection is terminated. A timeout may be specified using the
22862291
`'unknownProtocolTimeout'` option passed to [`http2.createSecureServer()`][].
2292+
2293+
In earlier versions of Node.js, this event would be emitted if `allowHTTP1` is
2294+
`false` and, during the TLS handshake, the client either does not send an ALPN
2295+
extension or sends an ALPN extension that does not include HTTP/2 (`h2`). Newer
2296+
versions of Node.js only emit this event if `allowHTTP1` is `false` and the
2297+
client does not send an ALPN extension. If the client sends an ALPN extension
2298+
that does not include HTTP/2 (or HTTP/1.1 if `allowHTTP1` is `true`), the TLS
2299+
handshake will fail and no secure connection will be established.
2300+
22872301
See the [Compatibility API][].
22882302

22892303
#### `server.close([callback])`
Collapse file

‎doc/api/tls.md‎

Copy file name to clipboardExpand all lines: doc/api/tls.md
+7-2Lines changed: 7 additions & 2 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -683,8 +683,8 @@ is set to describe how authorization failed. Depending on the settings
683683
of the TLS server, unauthorized connections may still be accepted.
684684

685685
The `tlsSocket.alpnProtocol` property is a string that contains the selected
686-
ALPN protocol. When ALPN has no selected protocol, `tlsSocket.alpnProtocol`
687-
equals `false`.
686+
ALPN protocol. When ALPN has no selected protocol because the client or the
687+
server did not send an ALPN extension, `tlsSocket.alpnProtocol` equals `false`.
688688

689689
The `tlsSocket.servername` property is a string containing the server name
690690
requested via SNI.
@@ -2012,6 +2012,11 @@ where `secureSocket` has the same API as `pair.cleartext`.
20122012
<!-- YAML
20132013
added: v0.3.2
20142014
changes:
2015+
- version: REPLACEME
2016+
pr-url: https://github.com/nodejs/node/pull/44031
2017+
description: If `ALPNProtocols` is set, incoming connections that send an
2018+
ALPN extension with no supported protocols are terminated with
2019+
a fatal `no_application_protocol` alert.
20152020
- version: v12.3.0
20162021
pr-url: https://github.com/nodejs/node/pull/27665
20172022
description: The `options` parameter now supports `net.createServer()`
Collapse file

‎lib/internal/http2/core.js‎

Copy file name to clipboardExpand all lines: lib/internal/http2/core.js
+1-1Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3055,7 +3055,7 @@ function connectionListener(socket) {
30553055
// going on in a format that they *might* understand.
30563056
socket.end('HTTP/1.0 403 Forbidden\r\n' +
30573057
'Content-Type: text/plain\r\n\r\n' +
3058-
'Unknown ALPN Protocol, expected `h2` to be available.\n' +
3058+
'Missing ALPN Protocol, expected `h2` to be available.\n' +
30593059
'If this is a HTTP request: The server was not ' +
30603060
'configured with the `allowHTTP1` option or a ' +
30613061
'listener for the `unknownProtocol` event.\n');
Collapse file

‎src/crypto/crypto_tls.cc‎

Copy file name to clipboardExpand all lines: src/crypto/crypto_tls.cc
+7-7Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -246,13 +246,13 @@ int SelectALPNCallback(
246246
in,
247247
inlen);
248248

249-
// According to 3.2. Protocol Selection of RFC7301, fatal
250-
// no_application_protocol alert shall be sent but OpenSSL 1.0.2 does not
251-
// support it yet. See
252-
// https://rt.openssl.org/Ticket/Display.html?id=3463&user=guest&pass=guest
253-
return status == OPENSSL_NPN_NEGOTIATED
254-
? SSL_TLSEXT_ERR_OK
255-
: SSL_TLSEXT_ERR_NOACK;
249+
// Previous versions of Node.js returned SSL_TLSEXT_ERR_NOACK if no protocol
250+
// match was found. This would neither cause a fatal alert nor would it result
251+
// in a useful ALPN response as part of the Server Hello message.
252+
// We now return SSL_TLSEXT_ERR_ALERT_FATAL in that case as per Section 3.2
253+
// of RFC 7301, which causes a fatal no_application_protocol alert.
254+
return status == OPENSSL_NPN_NEGOTIATED ? SSL_TLSEXT_ERR_OK
255+
: SSL_TLSEXT_ERR_ALERT_FATAL;
256256
}
257257

258258
int TLSExtStatusCallback(SSL* s, void* arg) {
Collapse file

‎test/parallel/test-http2-https-fallback.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http2-https-fallback.js
+13-3Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ function onSession(session, next) {
126126
const { port } = server.address();
127127
const origin = `https://localhost:${port}`;
128128

129-
const cleanup = countdown(3, () => server.close());
129+
const cleanup = countdown(4, () => server.close());
130130

131131
// HTTP/2 client
132132
connect(
@@ -149,12 +149,22 @@ function onSession(session, next) {
149149

150150
function testWrongALPN() {
151151
// Incompatible ALPN TLS client
152-
let text = '';
153152
tls(Object.assign({ port, ALPNProtocols: ['fake'] }, clientOptions))
153+
.on('error', common.mustCall((err) => {
154+
strictEqual(err.code, 'ECONNRESET');
155+
cleanup();
156+
testNoALPN();
157+
}));
158+
}
159+
160+
function testNoALPN() {
161+
// TLS client does not send an ALPN extension
162+
let text = '';
163+
tls(Object.assign({ port }, clientOptions))
154164
.setEncoding('utf8')
155165
.on('data', (chunk) => text += chunk)
156166
.on('end', common.mustCall(() => {
157-
ok(/Unknown ALPN Protocol, expected `h2` to be available/.test(text));
167+
ok(/Missing ALPN Protocol, expected `h2` to be available/.test(text));
158168
cleanup();
159169
}));
160170
}
Collapse file

‎test/parallel/test-http2-server-unknown-protocol.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-http2-server-unknown-protocol.js
+17-3Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ const fixtures = require('../common/fixtures');
88
if (!common.hasCrypto)
99
common.skip('missing crypto');
1010

11+
const assert = require('assert');
1112
const h2 = require('http2');
1213
const tls = require('tls');
1314

@@ -18,16 +19,29 @@ const server = h2.createSecureServer({
1819
allowHalfOpen: true
1920
});
2021

21-
server.on('connection', (socket) => {
22+
server.on('secureConnection', common.mustCall((socket) => {
2223
socket.on('close', common.mustCall(() => {
2324
server.close();
2425
}));
25-
});
26+
}));
2627

2728
server.listen(0, function() {
29+
// If the client does not send an ALPN connection, and the server has not been
30+
// configured with allowHTTP1, then the server should destroy the socket
31+
// after unknownProtocolTimeout.
2832
tls.connect({
2933
port: server.address().port,
3034
rejectUnauthorized: false,
31-
ALPNProtocols: ['bogus']
3235
});
36+
37+
// If the client sends an ALPN extension that does not contain 'h2', the
38+
// server should send a fatal alert to the client before a secure connection
39+
// is established at all.
40+
tls.connect({
41+
port: server.address().port,
42+
rejectUnauthorized: false,
43+
ALPNProtocols: ['bogus']
44+
}).on('error', common.mustCall((err) => {
45+
assert.strictEqual(err.code, 'ECONNRESET');
46+
}));
3347
});
Collapse file

‎test/parallel/test-tls-alpn-server-client.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-tls-alpn-server-client.js
+48-3Lines changed: 48 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ if (!common.hasCrypto)
55
common.skip('missing crypto');
66

77
const assert = require('assert');
8+
const { spawn } = require('child_process');
89
const tls = require('tls');
910
const fixtures = require('../common/fixtures');
1011

@@ -68,7 +69,7 @@ function Test1() {
6869
}, {
6970
ALPNProtocols: ['c', 'b', 'e'],
7071
}, {
71-
ALPNProtocols: ['first-priority-unsupported', 'x', 'y'],
72+
ALPNProtocols: ['x', 'y', 'c'],
7273
}];
7374

7475
runTest(clientsOptions, serverOptions, function(results) {
@@ -82,8 +83,8 @@ function Test1() {
8283
client: { ALPN: 'b' } });
8384
// Nothing is selected by ALPN
8485
checkResults(results[2],
85-
{ server: { ALPN: false },
86-
client: { ALPN: false } });
86+
{ server: { ALPN: 'c' },
87+
client: { ALPN: 'c' } });
8788
// execute next test
8889
Test2();
8990
});
@@ -161,6 +162,50 @@ function Test4() {
161162
{ server: { ALPN: false },
162163
client: { ALPN: false } });
163164
});
165+
166+
TestFatalAlert();
167+
}
168+
169+
function TestFatalAlert() {
170+
const server = tls.createServer({
171+
ALPNProtocols: ['foo'],
172+
key: loadPEM('agent2-key'),
173+
cert: loadPEM('agent2-cert')
174+
}, common.mustNotCall());
175+
176+
server.listen(0, serverIP, common.mustCall(() => {
177+
const { port } = server.address();
178+
179+
// The Node.js client will just report ECONNRESET because the connection
180+
// is severed before the TLS handshake completes.
181+
tls.connect({
182+
host: serverIP,
183+
port,
184+
rejectUnauthorized: false,
185+
ALPNProtocols: ['bar']
186+
}, common.mustNotCall()).on('error', common.mustCall((err) => {
187+
assert.strictEqual(err.code, 'ECONNRESET');
188+
189+
// OpenSSL's s_client should output the TLS alert number, which is 120
190+
// for the 'no_application_protocol' alert.
191+
const { opensslCli } = common;
192+
if (opensslCli) {
193+
const addr = `${serverIP}:${port}`;
194+
let stderr = '';
195+
spawn(opensslCli, ['s_client', '--alpn', 'bar', addr], {
196+
stdio: ['ignore', 'ignore', 'pipe']
197+
}).stderr
198+
.setEncoding('utf8')
199+
.on('data', (chunk) => stderr += chunk)
200+
.on('close', common.mustCall(() => {
201+
assert.match(stderr, /SSL alert number 120/);
202+
server.close();
203+
}));
204+
} else {
205+
server.close();
206+
}
207+
}));
208+
}));
164209
}
165210

166211
Test1();

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.