-
Notifications
You must be signed in to change notification settings - Fork 29k
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 ALPNCallback server option for dynamic ALPN negotiation #45190
Conversation
Review requested:
|
5c59acf
to
b818494
Compare
b818494
to
f0293c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to add tests for the ERR_TLS_ALPN_CALLBACK_WITH_PROTOCOLS
exception?
b35d907
to
58af789
Compare
@juanarbol Done 👍 |
bump @nodejs/crypto @bnoordhuis |
#45056 didn't really reach a conclusion. I believe I was okay with the general approach but I don't want to single-handedly make that decision. |
Bump - I'd love to get some thoughts on this. I've just found another motivating example, after updating to Node v20 myself: the ALPN changes from v19 break all usage of Node.js proxies that use ALPN with https-proxy-agent, because the ALPN identifier there is slightly wrong and fails negotiation, and with v19 that's now a blocking issue with no automatic fallback option available. I'm sure this won't be the last client doing ALPN slightly wrong - would be very nice to have this option available so people can easily ignore all that. |
doc/api/tls.md
Outdated
@@ -2042,6 +2045,17 @@ 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.) | |||
* `ALPNCallback(params)`: {Function} If set, this will be called when a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For naming, alpnCallback
would be more conventional, also remove the (params)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(params)
now removed.
Renaming from ALPNCallback
to alpnCallback
would make this inconsistent with the existing ALPNProtocols
option though, which feels odd. Is that OK?
@@ -53,6 +52,15 @@ function runTest(clientsOptions, serverOptions, cb) { | |||
cb(results); | |||
}); | |||
} | |||
} | |||
|
|||
const client = tls.connect(opt, function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrap the callback with common.mustCall(...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the other callbacks in this file now in the latest commit, but I think this one isn't actually a mustCall
. This is a function (runTest
) is called by quite a few tests in here with different options each time, and some of them expect this callback to run while others test failures where it should never run.
All of them check the results
values that get set within the callbacks here though - I think that's probably sufficient?
@jasnell thanks for the review! I've updated everything as suggested, except the two points I've replied to above. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but let's have others take a look also. Perhaps @bnoordhuis ?
PR-URL: #45190 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>
Notable changes: crypto: * update root certificates to NSS 3.90 (Node.js GitHub Bot) #48416 doc: * add vmoroz to collaborators (Vladimir Morozov) #48527 * add kvakil to collaborators (Keyhan Vakil) #48449 fs, stream: * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) #48518 test_runner: * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 PR-URL: TODO
Notable changes: crypto: * update root certificates to NSS 3.90 (Node.js GitHub Bot) nodejs#48416 doc: * add vmoroz to collaborators (Vladimir Morozov) nodejs#48527 * add kvakil to collaborators (Keyhan Vakil) nodejs#48449 fs, stream: * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) nodejs#48518 test_runner: * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs#47775 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs#45190 PR-URL: nodejs#48643
Notable changes: crypto: * update root certificates to NSS 3.90 (Node.js GitHub Bot) nodejs#48416 doc: * add vmoroz to collaborators (Vladimir Morozov) nodejs#48527 * add kvakil to collaborators (Keyhan Vakil) nodejs#48449 fs, stream: * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) nodejs#48518 test_runner: * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs#47775 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs#45190 PR-URL: nodejs#48643
Notable changes: crypto: * update root certificates to NSS 3.90 (Node.js GitHub Bot) #48416 doc: * add vmoroz to collaborators (Vladimir Morozov) #48527 * add kvakil to collaborators (Keyhan Vakil) #48449 fs, stream: * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) #48518 test_runner: * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 PR-URL: #48643
Notable changes: crypto: * update root certificates to NSS 3.90 (Node.js GitHub Bot) nodejs#48416 doc: * add vmoroz to collaborators (Vladimir Morozov) nodejs#48527 * add kvakil to collaborators (Keyhan Vakil) nodejs#48449 fs, stream: * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) nodejs#48518 test_runner: * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs#47775 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs#45190 PR-URL: nodejs#48643
Notable changes: crypto: * update root certificates to NSS 3.90 (Node.js GitHub Bot) #48416 doc: * add vmoroz to collaborators (Vladimir Morozov) #48527 * add kvakil to collaborators (Keyhan Vakil) #48449 fs, stream: * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) #48518 test_runner: * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 PR-URL: #48643
PR-URL: nodejs#45190 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>
Notable changes: crypto: * update root certificates to NSS 3.90 (Node.js GitHub Bot) nodejs#48416 doc: * add vmoroz to collaborators (Vladimir Morozov) nodejs#48527 * add kvakil to collaborators (Keyhan Vakil) nodejs#48449 fs, stream: * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) nodejs#48518 test_runner: * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs#47775 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs#45190 PR-URL: nodejs#48643
PR-URL: nodejs#45190 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>
Notable changes: crypto: * update root certificates to NSS 3.90 (Node.js GitHub Bot) nodejs#48416 doc: * add vmoroz to collaborators (Vladimir Morozov) nodejs#48527 * add kvakil to collaborators (Keyhan Vakil) nodejs#48449 fs, stream: * initial `Symbol.dispose` and `Symbol.asyncDispose` support (Moshe Atlow) nodejs#48518 test_runner: * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs#47775 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs#45190 PR-URL: nodejs#48643
This commit does not land cleanly on |
PR-URL: #45190 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>
Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908 * (SEMVER-MINOR) upgrade npm to 10.0.0 (npm team) #49423 doc: * add new TSC members (Michael Dawson) #48841 * move and rename loaders section (Geoffrey Booth) #49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) #50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869 * unflag import.meta.resolve (Guy Bedford) #49028 * move hook execution to separate thread (Jacob Smith) #44710 * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) #49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141 wasi: * (SEMVER-MINOR) updates required for latest uvwasi version (Michael Dawson) #49908 PR-URL: TODO
Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) #50531 doc: * move and rename loaders section (Geoffrey Booth) #49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) #50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869 * unflag import.meta.resolve (Guy Bedford) #49028 * move hook execution to separate thread (Jacob Smith) #44710 * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) #49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141 PR-URL: #50953
Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) #49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) #50531 doc: * move and rename loaders section (Geoffrey Booth) #49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) #50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) #49869 * unflag import.meta.resolve (Guy Bedford) #49028 * move hook execution to separate thread (Jacob Smith) #44710 * leverage loaders when resolving subsequent loaders (Maël Nison) #43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) #46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) #44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) #45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) #49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) #48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) #48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) #47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) #49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) #45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) #50141 PR-URL: #50953
PR-URL: nodejs/node#45190 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>
Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs/node#49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) nodejs/node#50531 doc: * move and rename loaders section (Geoffrey Booth) nodejs/node#49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs/node#49869 * unflag import.meta.resolve (Guy Bedford) nodejs/node#49028 * move hook execution to separate thread (Jacob Smith) nodejs/node#44710 * leverage loaders when resolving subsequent loaders (Maël Nison) nodejs/node#43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) nodejs/node#46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) nodejs/node#44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) nodejs/node#45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) nodejs/node#49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs/node#49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs/node#49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs/node#48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs/node#48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs/node#47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs/node#49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs/node#45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50141 PR-URL: nodejs/node#50953
PR-URL: nodejs/node#45190 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Debadree Chatterjee <[email protected]>
Notable changes: deps: * (SEMVER-MINOR) update uvwasi to 0.0.19 (Node.js GitHub Bot) nodejs/node#49908 * (SEMVER-MINOR) upgrade npm to 10.2.3 (npm team) nodejs/node#50531 doc: * move and rename loaders section (Geoffrey Booth) nodejs/node#49261 esm: * use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50140 * --experimental-default-type flag to flip module defaults (Geoffrey Booth) nodejs/node#49869 * unflag import.meta.resolve (Guy Bedford) nodejs/node#49028 * move hook execution to separate thread (Jacob Smith) nodejs/node#44710 * leverage loaders when resolving subsequent loaders (Maël Nison) nodejs/node#43772 lib: * (SEMVER-MINOR) add api to detect whether source-maps are enabled (翠 / green) nodejs/node#46391 * (SEMVER-MINOR) add tracing channel to diagnostics_channel (Stephen Belanger) nodejs/node#44943 src: * (SEMVER-MINOR) add cjs_module_lexer_version base64_version (Jithil P Ponnan) nodejs/node#45629 stream: * use bitmap in readable state (Benjamin Gruenbaum) nodejs/node#49745 test_runner: * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs/node#49753 * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs/node#49614 * (SEMVER-MINOR) expose location of tests (Colin Ihrig) nodejs/node#48975 * (SEMVER-MINOR) add shards support (Raz Luvaton) nodejs/node#48639 * (SEMVER-MINOR) add initial draft for fakeTimers (Erick Wendel) nodejs/node#47775 test_runner, cli: * (SEMVER-MINOR) add --test-concurrency flag (Colin Ihrig) nodejs/node#49996 tls: * (SEMVER-MINOR) add ALPNCallback server option for dynamic ALPN negotiation (Tim Perry) nodejs/node#45190 vm: * (SEMVER-MINOR) use import attributes instead of import assertions (Antoine du Hamel) nodejs/node#50141 PR-URL: nodejs/node#50953
This is an alternate fix for #45056 (which replaces #45075).
I'd definitely appreciate careful reviewing of the native parts of this, I'm not familiar with a lot of the standard approaches and concerns there and this is definitely more complex than the other approach.
A few open questions:
I've usedResolved in favour ofserverName
as the field for the server name that's passed to the callback. Some of the existing API usesservername
instead, but that casing seems very odd. Which do we prefer?servername
ALPNProtocols
behaviour, but is that correct? We could relax this with some other special return value, either now or in future, e.g. "returnundefined
if no ALPN values match to reject the handshake, orfalse
to skip ALPN and send no handshake at all" (or some other pair of non-string values). I'm not sure if anybody will need this, but it's perfectly legitimate behaviour (servers aren't required to send ALPN responses AFAICT) so it's probably good to at least leave space for this in the API design in case we want it later.