tls: add ALPNCallback server option for dynamic ALPN negotiation #45190

pimterry · 2022-10-26T12:53:07Z

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 used serverName as the field for the server name that's passed to the callback. Some of the existing API uses servername instead, but that casing seems very odd. Which do we prefer?~~ Resolved in favour of servername
The implementation here throws an error if the server callback returns a string that isn't in the client's offered protocols. Reading the RFC that's implicitly what you should do, but I don't think it's strictly required - it's weird but not 100% invalid as far as I can tell to send an ALPN response telling the client you're speaking a protocol they didn't ask for. I assume we're fine with being strict here, but worth considering.
The implementation here doesn't support returning no ALPN extension if you set a callback. If you've provided an ALPN callback, then when an ALPN-using client arrives you must either return an ALPN response or reject the connection entirely. That's equivalent to the current ALPNProtocols behaviour, but is that correct? We could relax this with some other special return value, either now or in future, e.g. "return undefined if no ALPN values match to reject the handshake, or false 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.

nodejs-github-bot · 2022-10-26T12:53:12Z

Review requested:

@nodejs/crypto

lib/_tls_wrap.js

nodejs-github-bot · 2022-10-28T18:50:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/47536/

juanarbol

Is it possible to add tests for the ERR_TLS_ALPN_CALLBACK_WITH_PROTOCOLS exception?

pimterry · 2022-10-31T13:50:15Z

Is it possible to add tests for the ERR_TLS_ALPN_CALLBACK_WITH_PROTOCOLS exception?

@juanarbol Done 👍

pimterry · 2022-11-07T12:07:17Z

Any other feedback on this? Happy to make any further changes that might be useful. It would be great to get this merged so we can close this PR, #45075 and #45056.

addaleax · 2022-12-22T15:24:06Z

bump @nodejs/crypto @bnoordhuis

bnoordhuis · 2022-12-22T15:46:12Z

#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.

pimterry · 2023-04-28T15:06:26Z

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.

jasnell · 2023-05-05T00:14:23Z

doc/api/tls.md

@@ -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


For naming, alpnCallback would be more conventional, also remove the (params)

(params) now removed.

Renaming from ALPNCallback to alpnCallback would make this inconsistent with the existing ALPNProtocols option though, which feels odd. Is that OK?

doc/api/tls.md

lib/_tls_wrap.js

jasnell · 2023-05-05T00:20:59Z

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

@@ -53,6 +52,15 @@ function runTest(clientsOptions, serverOptions, cb) {
          cb(results);
        });
      }
+    }
+
+    const client = tls.connect(opt, function() {


Wrap the callback with common.mustCall(...)

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?

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

pimterry · 2023-05-05T11:38:21Z

@jasnell thanks for the review! I've updated everything as suggested, except the two points I've replied to above.

lib/_tls_wrap.js

jasnell

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) #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

ruyadorno · 2023-09-10T03:43:26Z

This commit does not land cleanly on v18.x-staging and will need manual backport in case we want it in v18.

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

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

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 26, 2022

pimterry force-pushed the alpn-callback branch from 5c59acf to b818494 Compare October 26, 2022 13:02

pimterry mentioned this pull request Oct 26, 2022

Allow ALPN fallback #45056

Closed

VoltrexKeyva reviewed Oct 26, 2022

View reviewed changes

lib/_tls_wrap.js Outdated Show resolved Hide resolved

pimterry force-pushed the alpn-callback branch from b818494 to f0293c2 Compare October 27, 2022 10:03

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2022

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 28, 2022

juanarbol reviewed Oct 29, 2022

View reviewed changes

pimterry force-pushed the alpn-callback branch from b35d907 to 58af789 Compare October 31, 2022 13:49

lpinca added the review wanted PRs that need reviews. label Nov 7, 2022

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

jasnell reviewed May 5, 2023

View reviewed changes

doc/api/tls.md Show resolved Hide resolved

jasnell reviewed May 5, 2023

View reviewed changes

lib/_tls_wrap.js Show resolved Hide resolved

jasnell reviewed May 5, 2023

View reviewed changes

lib/_tls_wrap.js Outdated Show resolved Hide resolved

jasnell reviewed May 5, 2023

View reviewed changes

test/parallel/test-tls-alpn-server-client.js Outdated Show resolved Hide resolved

pimterry force-pushed the alpn-callback branch from d3440e7 to 0d3a2f7 Compare May 5, 2023 11:30

jasnell reviewed May 10, 2023

View reviewed changes

lib/_tls_wrap.js Outdated Show resolved Hide resolved

jasnell approved these changes May 10, 2023

View reviewed changes

This was referenced Jul 1, 2023

CI Reliability 2023-07-01 nodejs/reliability#604

Open

CI Reliability 2023-07-02 nodejs/reliability#605

Open

CI Reliability 2023-07-03 nodejs/reliability#606

Open

RafaelGSS mentioned this pull request Jul 3, 2023

v20.4.0 proposal #48643

Merged

github-actions bot mentioned this pull request Jul 4, 2023

CI Reliability 2023-07-04 nodejs/reliability#607

Open

22 tasks

github-actions bot mentioned this pull request Jul 5, 2023

CI Reliability 2023-07-05 nodejs/reliability#608

Open

20 tasks

ruyadorno added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Sep 10, 2023

targos added backported-to-v18.x PRs backported to the v18.x-staging branch. and removed backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. labels Oct 28, 2023

targos mentioned this pull request Nov 28, 2023

v18.19.0 release proposal #50953

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: add ALPNCallback server option for dynamic ALPN negotiation #45190

tls: add ALPNCallback server option for dynamic ALPN negotiation #45190

pimterry commented Oct 26, 2022 •

edited

Loading

nodejs-github-bot commented Oct 26, 2022

nodejs-github-bot commented Oct 28, 2022

juanarbol left a comment

pimterry commented Oct 31, 2022

pimterry commented Nov 7, 2022

addaleax commented Dec 22, 2022

bnoordhuis commented Dec 22, 2022

pimterry commented Apr 28, 2023

jasnell May 5, 2023

pimterry May 5, 2023 •

edited

Loading

jasnell May 5, 2023

pimterry May 5, 2023

pimterry commented May 5, 2023

jasnell left a comment

ruyadorno commented Sep 10, 2023

tls: add ALPNCallback server option for dynamic ALPN negotiation #45190

tls: add ALPNCallback server option for dynamic ALPN negotiation #45190

Conversation

pimterry commented Oct 26, 2022 • edited Loading

nodejs-github-bot commented Oct 26, 2022

nodejs-github-bot commented Oct 28, 2022

juanarbol left a comment

Choose a reason for hiding this comment

pimterry commented Oct 31, 2022

pimterry commented Nov 7, 2022

addaleax commented Dec 22, 2022

bnoordhuis commented Dec 22, 2022

pimterry commented Apr 28, 2023

jasnell May 5, 2023

Choose a reason for hiding this comment

pimterry May 5, 2023 • edited Loading

Choose a reason for hiding this comment

jasnell May 5, 2023

Choose a reason for hiding this comment

pimterry May 5, 2023

Choose a reason for hiding this comment

pimterry commented May 5, 2023

jasnell left a comment

Choose a reason for hiding this comment

ruyadorno commented Sep 10, 2023

pimterry commented Oct 26, 2022 •

edited

Loading

pimterry May 5, 2023 •

edited

Loading