-
Notifications
You must be signed in to change notification settings - Fork 4.7k
fetch: strip IPv6 brackets before TLS cert verification #30674
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
Open
robobun
wants to merge
3
commits into
main
Choose a base branch
from
farm/1cba0bed/ipv6-tls-checkserveridentity
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+109
−4
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| // Regression test for https://github.com/oven-sh/bun/issues/30668 | ||
| // | ||
| // The URL parser keeps the surrounding `[`/`]` on IPv6 hostnames. That bracketed | ||
| // form must NOT leak into TLS certificate verification: | ||
| // - strings::is_ip_address("[::1]") is false, so the native fast path would | ||
| // take the DNS-name branch and skip the IP-SAN match on the cert. | ||
| // - node:tls.checkServerIdentity uses net.isIP("[::1]") === 0 and likewise | ||
| // falls through to CN matching and emits ERR_TLS_CERT_ALTNAME_INVALID. | ||
| // | ||
| // Node.js strips brackets in urlToHttpOptions before either verification path | ||
| // runs. get_tls_hostname in src/http/lib.rs now mirrors that contract so every | ||
| // TLS consumer (native cert check, SNI's is_ip_address check, and the | ||
| // user-supplied checkServerIdentity callback) sees the bare address `::1`. | ||
| // | ||
| // This test lives in its own file rather than fetch.tls.test.ts because the | ||
| // subprocess approach is sensitive to environment setup (HTTP_PROXY, | ||
| // NO_PROXY) and we want a clean top-level env rather than sharing one with | ||
| // other concurrent tests in that file. | ||
|
|
||
| import { expect, it } from "bun:test"; | ||
| import { bunEnv, bunExe, isIPv6, tls as validTls } from "harness"; | ||
|
|
||
| // Skipped on Buildkite Linux — those AWS instances don't have IPv6 set up | ||
| // (see `isIPv6` in harness.ts). Matches the gating pattern already used by | ||
| // the directly analogous valkey-tls-verify.test.ts:177. | ||
| it.skipIf(!isIPv6())("fetch with IPv6 literal hostname verifies the certificate", async () => { | ||
| using server = Bun.serve({ | ||
| port: 0, | ||
| tls: validTls, | ||
| fetch() { | ||
| return new Response("Hello World"); | ||
| }, | ||
| }); | ||
| const port = server.port; | ||
|
|
||
| // Drop HTTP_PROXY/HTTPS_PROXY so the container-level egress proxy doesn't | ||
| // intercept a request to [::1] (NO_PROXY doesn't match the bracketed form | ||
| // in some fetch paths). | ||
| const { HTTP_PROXY, HTTPS_PROXY, http_proxy, https_proxy, ...cleanEnv } = bunEnv; | ||
| await using proc = Bun.spawn({ | ||
| cmd: [ | ||
| bunExe(), | ||
| "-e", | ||
| ` | ||
| const tls = require("node:tls"); | ||
| const cert = ${JSON.stringify(validTls.cert)}; | ||
| const url = "https://[::1]:${port}/"; | ||
| // Native fast path — no user-supplied checkServerIdentity. Native | ||
| // check_x509_server_identity must see "::1" (not "[::1]") so that | ||
| // strings::is_ip_address() picks the IP-SAN branch. | ||
| { | ||
| const res = await fetch(url, { | ||
| keepalive: false, | ||
| tls: { ca: cert, rejectUnauthorized: true }, | ||
| }); | ||
| console.log("native:", await res.text()); | ||
| } | ||
| // User-supplied checkServerIdentity path — the hostname handed to | ||
| // the callback must be bracket-stripped so that | ||
| // node:tls.checkServerIdentity (net.isIP) accepts it, matching | ||
| // Node.js's urlToHttpOptions behavior. | ||
| { | ||
| let observed; | ||
| const res = await fetch(url, { | ||
| keepalive: false, | ||
| tls: { | ||
| ca: cert, | ||
| rejectUnauthorized: true, | ||
| checkServerIdentity(hostname, cert) { | ||
| observed = hostname; | ||
| return tls.checkServerIdentity(hostname, cert); | ||
| }, | ||
| }, | ||
| }); | ||
| console.log("callback hostname:", observed); | ||
| console.log("js:", await res.text()); | ||
| } | ||
| `, | ||
| ], | ||
| env: cleanEnv, | ||
| stdout: "pipe", | ||
| stderr: "pipe", | ||
| }); | ||
| const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); | ||
| // Asserting stdout/stderr before exitCode produces a more useful failure | ||
| // message when the subprocess crashes unexpectedly. | ||
| expect(stdout).toBe("native: Hello World\ncallback hostname: ::1\njs: Hello World\n"); | ||
| expect(stderr).toBe(""); | ||
| expect(exitCode).toBe(0); | ||
| }); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🟣 Pre-existing, not introduced here — but
ProxyTunnel::on_open(src/http/ProxyTunnel.rs:248) computes the inner-TLS SNI hostname viathis.hostname.unwrap_or(this.url.hostname)instead of callingget_tls_hostname, so a proxiedfetch('https://[::1]:PORT/')still passes"[::1]"tobun_core::is_ip_address(which returnsfalsefor bracketed input) and sends it as the SNI server_name. Cert verification on that path is fixed by this PR (it routes throughcheck_server_identity→get_tls_hostname), so #30668's user-visible symptom is resolved either way — but if you want the "single chokepoint" claim to actually hold for SNI, swapping line 248 forget_tls_hostname(this, false)would also give the proxy-tunnel path port-stripping andtls_props.server_nameprecedence for free.Extended reasoning...
What this is
The PR description says
getTlsHostnameis the "single chokepoint" so that "every fetch TLS consumer (native cert check, SNI'sisIPAddresscheck, and the user-suppliedcheckServerIdentitycallback) now sees the bare address::1". For the direct (non-proxied) connection path that's true —get_tls_hostnameis called atsrc/http/lib.rs:1592and the result feeds the SNIis_ip_addressgate. But the proxy-tunnel inner-TLS handshake has its own, older copy of the hostname-selection logic that bypasses the chokepoint.src/http/ProxyTunnel.rs:248:This does not call
get_tls_hostname, so it picks up neither the existingstrip_port_from_hostnor the newstrip_ipv6_brackets(nor thetls_props.server_nameoverride).Step-by-step
fetch('https://[::1]:9443/', { proxy: 'http://proxyhost:8080', tls: { ca, rejectUnauthorized: true } }).CONNECT [::1]:9443establishes the tunnel.ProxyTunnel::on_openfires to start the inner TLS handshake to the origin.this.url.hostname == "[::1]"(the URL parser keeps brackets, persrc/url/url.zig:429-432and the PR description itself).bun_core::is_ip_address("[::1]")→false. It callsares_inet_pton(AF_INET, ...)thenares_inet_pton(AF_INET6, ...)directly on the input (src/bun_core/lib.rs:2240-2251);inet_ptondoes not accept bracketed IPv6, so both fail.elsebranch, NUL-terminates"[::1]", and passes it as theserver_nametoconfigure_http_client_with_alpn— an RFC 6066 §3 violation (literal IP addresses are not permitted in SNIHostName).What is and isn't fixed on this path
ProxyTunnel's identity check goes throughcheck_server_identity(..., allow_proxy_url=false)→get_tls_hostnameatsrc/http/lib.rs:1487, which now strips brackets. SoERR_TLS_CERT_ALTNAME_INVALIDis gone for proxied IPv6 origins too.on_openis missed, because it duplicates the hostname-selection logic locally.grepconfirmsget_tls_hostnamehas exactly two call sites (lib.rs:1487cert check,lib.rs:1592direct-path SNI);ProxyTunnel.rs:248is a third hostname consumer that doesn't go through it.Why file it at all (addressing the "no observable effect" objection)
It's true that most TLS servers ignore an unrecognized/garbage
server_namerather than aborting the handshake, and that for an IP-literal origin the "correct" behaviour is to send no SNI anyway — so the practical blast radius is small. I'm flagging it as pre-existing, not a blocker, for three reasons:is_ip_address), in a directly adjacent code path — not an unrelated drive-by.get_tls_hostnameboth name "SNI'sis_ip_addresscheck" as a covered consumer; the proxy-tunnel SNI'sis_ip_addresscheck atProxyTunnel.rs:252is not covered, so the claim is slightly overstated.get_tls_hostname(this, false)would also give the proxy-tunnel inner handshakestrip_port_from_hostandtls_props.server_nameprecedence, which it currently lacks relative to the direct path atlib.rs:1592.Suggested follow-up
(make
get_tls_hostnamepub(crate)if it isn't already). That makes the chokepoint claim hold for all three fetch-TLS hostname consumers.