Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: Add support for ALPN fallback when no ALPN protocol matches #45075

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -2698,6 +2698,17 @@ This error represents a failed test. Additional information about the failure
is available via the `cause` property. The `failureType` property specifies
what the test was doing when the failure occurred.

<a id="ERR_TLS_ALPN_FALLBACK_WITHOUT_PROTOCOLS"></a>

### `ERR_TLS_ALPN_FALLBACK_WITHOUT_PROTOCOLS`

This error is thrown when creating a `TLSServer` if the TLS options sets
`allowALPNFallback` to `true` without providing an `ALPNProtocols` argument.

When `ALPNProtocols` is not provided, ALPN is skipped entirely, so the fallback
would not be functional. To enable ALPN for all protocols, using the fallback
in all cases, set `ALPNProtocols` to an empty array instead.

<a id="ERR_TLS_CERT_ALTNAME_FORMAT"></a>

### `ERR_TLS_CERT_ALTNAME_FORMAT`
Expand Down
8 changes: 8 additions & 0 deletions doc/api/tls.md
Original file line number Diff line number Diff line change
Expand Up @@ -2012,6 +2012,9 @@ where `secureSocket` has the same API as `pair.cleartext`.
<!-- YAML
added: v0.3.2
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/45075
description: The `options` parameter can now include `allowALPNFallback`.
- version: v19.0.0
pr-url: https://github.com/nodejs/node/pull/44031
description: If `ALPNProtocols` is set, incoming connections that send an
Expand Down Expand Up @@ -2042,6 +2045,11 @@ changes:
e.g. `0x05hello0x05world`, where the first byte is the length of the next
protocol name. Passing an array is usually much simpler, e.g.
`['hello', 'world']`. (Protocols should be ordered by their priority.)
* `allowALPNFallback`: {boolean} If `true`, if the `ALPNProtocols` option was
set and the client uses ALPN, but requests only protocols that are not do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awkwardly structured sentence / grammar here.

not match the server's `ALPNProtocols`, the server will fall back to sending
an ALPN response accepting the first protocol the client suggested, instead
of closing the connection immediately with a fatal alert.
* `clientCertEngine` {string} Name of an OpenSSL engine which can provide the
client certificate.
* `enableTrace` {boolean} If `true`, [`tls.TLSSocket.enableTrace()`][] will be
Expand Down
7 changes: 7 additions & 0 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ const {
ERR_INVALID_ARG_VALUE,
ERR_MULTIPLE_CALLBACK,
ERR_SOCKET_CLOSED,
ERR_TLS_ALPN_FALLBACK_WITHOUT_PROTOCOLS,
ERR_TLS_DH_PARAM_SIZE,
ERR_TLS_HANDSHAKE_TIMEOUT,
ERR_TLS_INVALID_CONTEXT,
Expand Down Expand Up @@ -716,6 +717,7 @@ TLSSocket.prototype._init = function(socket, wrap) {
ssl.onnewsession = onnewsession;
ssl.lastHandshakeTime = 0;
ssl.handshakes = 0;
ssl.allowALPNFallback = !!options.allowALPNFallback;

if (this.server) {
if (this.server.listenerCount('resumeSession') > 0 ||
Expand Down Expand Up @@ -1100,6 +1102,7 @@ function tlsConnectionListener(rawSocket) {
rejectUnauthorized: this.rejectUnauthorized,
handshakeTimeout: this[kHandshakeTimeout],
ALPNProtocols: this.ALPNProtocols,
allowALPNFallback: this.allowALPNFallback,
SNICallback: this[kSNICallback] || SNICallback,
enableTrace: this[kEnableTrace],
pauseOnConnect: this.pauseOnConnect,
Expand Down Expand Up @@ -1198,6 +1201,10 @@ function Server(options, listener) {
this._contexts = [];
this.requestCert = options.requestCert === true;
this.rejectUnauthorized = options.rejectUnauthorized !== false;
this.allowALPNFallback = options.allowALPNFallback === true;
if (this.allowALPNFallback && !options.ALPNProtocols) {
throw new ERR_TLS_ALPN_FALLBACK_WITHOUT_PROTOCOLS();
}

if (options.sessionTimeout)
this.sessionTimeout = options.sessionTimeout;
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1612,6 +1612,9 @@ E('ERR_TEST_FAILURE', function(error, failureType) {
this.cause = error;
return msg;
}, Error);
E('ERR_TLS_ALPN_FALLBACK_WITHOUT_PROTOCOLS',
'The allowALPNFallback TLS option cannot be set without ALPNProtocols',
TypeError);
E('ERR_TLS_CERT_ALTNAME_FORMAT', 'Invalid subject alternative name string',
SyntaxError);
E('ERR_TLS_CERT_ALTNAME_INVALID', function(reason, host, cert) {
Expand Down
23 changes: 19 additions & 4 deletions src/crypto/crypto_tls.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ using v8::Array;
using v8::ArrayBuffer;
using v8::ArrayBufferView;
using v8::BackingStore;
using v8::Boolean;
using v8::Context;
using v8::DontDelete;
using v8::Exception;
Expand Down Expand Up @@ -237,6 +238,13 @@ int SelectALPNCallback(
if (UNLIKELY(alpn_buffer.IsEmpty()) || !alpn_buffer->IsArrayBufferView())
return SSL_TLSEXT_ERR_NOACK;

bool alpn_fallback_allowed =
w->object()
->Get(env->context(), env->allow_alpn_fallback_string())
.ToLocalChecked()
.As<Boolean>()
->Value();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @bnoordhuis mentions, this is going to be slow. Definitely should avoid uses of ->Get(...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very familiar with the JS/C++ interface. What's the preferred way to set a value from the JS TLS wrap such that it's directly accessible here?

If I create a new private field & setter on the C++ wrap, call that from JS in setup, and then read w->allow_alpn_fallback here instead of these calls, is that a) idiomatic and b) sufficiently performant?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes and yes. :-)


ArrayBufferViewContents<unsigned char> alpn_protos(alpn_buffer);
int status = SSL_select_next_proto(
const_cast<unsigned char**>(out),
Expand All @@ -249,10 +257,17 @@ int SelectALPNCallback(
// Previous versions of Node.js returned SSL_TLSEXT_ERR_NOACK if no protocol
// match was found. This would neither cause a fatal alert nor would it result
// in a useful ALPN response as part of the Server Hello message.
// We now return SSL_TLSEXT_ERR_ALERT_FATAL in that case as per Section 3.2
// of RFC 7301, which causes a fatal no_application_protocol alert.
return status == OPENSSL_NPN_NEGOTIATED ? SSL_TLSEXT_ERR_OK
: SSL_TLSEXT_ERR_ALERT_FATAL;

// By default, we now return SSL_TLSEXT_ERR_ALERT_FATAL in that case as per
// Section 3.2 of RFC 7301, which causes a fatal no_application_protocol
// alert, unless the allowALPNFallback option has been set, which enables
// OpenSSL's default protocol selection behavior, falling back to the client's
// first preference.
if (status == OPENSSL_NPN_NEGOTIATED || alpn_fallback_allowed) {
return SSL_TLSEXT_ERR_OK;
} else {
return SSL_TLSEXT_ERR_ALERT_FATAL;
}
}

int TLSExtStatusCallback(SSL* s, void* arg) {
Expand Down
1 change: 1 addition & 0 deletions src/env_properties.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
V(ack_string, "ack") \
V(address_string, "address") \
V(aliases_string, "aliases") \
V(allow_alpn_fallback_string, "allowALPNFallback") \
V(args_string, "args") \
V(asn1curve_string, "asn1Curve") \
V(async_ids_stack_string, "async_ids_stack") \
Expand Down
60 changes: 60 additions & 0 deletions test/parallel/test-tls-alpn-server-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,66 @@ function TestFatalAlert() {
}
}));
}));

TestALPNFallback();
}

function TestALPNFallback() {
const serverOptions = {
ALPNProtocols: ['b'],
allowALPNFallback: true
};

const clientOptions = [{
ALPNProtocols: ['a', 'b']
}, {
ALPNProtocols: ['a']
}];

runTest(clientOptions, serverOptions, function(results) {
// 'b' is selected by ALPN if available
checkResults(results[0],
{ server: { ALPN: 'b' },
client: { ALPN: 'b' } });

// 'a' is selected by ALPN if not, due to fallback
checkResults(results[1],
{ server: { ALPN: 'a' },
client: { ALPN: 'a' } });
});

TestEmptyALPNFallback();
}

function TestEmptyALPNFallback() {
const serverOptions = {
ALPNProtocols: [],
allowALPNFallback: true
};

const clientOptions = [{
ALPNProtocols: ['a', 'b']
}];

runTest(clientOptions, serverOptions, function(results) {
// 'a' is selected by ALPN, due to fallback with empty array
checkResults(results[0],
{ server: { ALPN: 'a' },
client: { ALPN: 'a' } });
});

TestFallbackWithoutProtocols();
}

function TestFallbackWithoutProtocols() {
assert.throws(() => {
tls.createServer({
allowALPNFallback: true
});
}, {
code: 'ERR_TLS_ALPN_FALLBACK_WITHOUT_PROTOCOLS',
message: 'The allowALPNFallback TLS option cannot be set without ALPNProtocols'
});
}

Test1();