fetch: pool keep-alive connections for requests with checkServerIdentity

[Jarred-Sumner]

What

Fixes #40308. Since 1.4.0 (#33072), a fetch whose tls options include a checkServerIdentity callback never entered or took from the keep-alive pool, so every request paid a full TCP+TLS handshake. 1.3.x pooled these.

This restores pooling while keeping the property #33072 was after — a peer accepted by a JS callback is not silently inherited by a request that relies on the native hostname check (and vice versa) — by keeping them in separate pools:

• Each pooled socket / CONNECT tunnel / h2 session records a PeerVerification: None (rejectUnauthorized: false), Callback (chain verified, identity approved by checkServerIdentity), or Native (chain + native hostname check). This replaces the established_with_reject_unauthorized: bool that already separated lax from strict.
• A request only takes a pooled connection verified the way it would verify a fresh one (rejectUnauthorized: false may take any). The level is sticky, so a lax request borrowing a strict connection doesn't downgrade it when re-pooling.
• The callback runs once per connection, when it is established — same as 1.3 and Node's https.Agent (whose pool key doesn't include checkServerIdentity either). Two different callbacks for the same host + TLS config share the Callback pool; a request that needs its callback consulted every time can pass keepalive: false.

I considered re-running the callback against the pooled peer certificate on every reuse; it works, but parks an idle keep-alive socket for a JS round-trip before the first byte is written, which turns ordinary server idle-closes / unsolicited 408s during that window into request failures (or double callback invocations on retry). Per-connection semantics avoid that and match prior behavior.

Issue repro (5 sequential fetches through a counting TCP proxy):

                              1.3.14   1.4.1   this PR
ca[] only                     1        1       1
ca[] + checkServerIdentity    1        5       1

Tests

test/js/web/fetch/fetch.tls.test.ts:

• reuses one connection across callback-bearing requests (fresh closure each time); callback ran once
• callback-approved and natively-verified connections stay in separate pools and each keeps being reused
• a callback request never takes a connection established under NODE_TLS_REJECT_UNAUTHORIZED=0 (fails its own chain verification instead)

test/js/bun/http/proxy.test.ts: same for a CONNECT tunnel through an HTTP proxy.

The first two and the proxy test fail on 1.4.1. Also ran proxy.test.ts, proxy-stress-*.test.ts, fetch-keepalive.test.ts, fetch-http2-*.test.ts, node-http2.test.js, fetch-tls-cert.test.ts, node-tls-cert.test.ts, fetch.tls.wildcard.test.ts locally (debug build), all passing. One of ~8 full proxy.test.ts runs reported "2 errors" between tests that I could not reproduce afterwards; flagging in case CI sees it.

[coderabbitai]

Warning

Review limit reached

• Run on-demand review

_{On-demand reviews are free for the next 27 days. After that, they cost $0.25 per reviewed file.}

Or wait 12 minutes for your next included review.

View limit details

Limit details: You’ve used the included review currently available. Your 72 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d10e6977-746a-466a-badb-5f4a0cf16c1b

📥 Commits

Reviewing files that changed from the base of the PR and between 29b958f and 5db7ee9.

📒 Files selected for processing (7)

• src/http/HTTPContext.rs
• src/http/ProxyTunnel.rs
• src/http/h2_client/ClientSession.rs
• src/http/h2_client/PendingConnect.rs
• src/http/lib.rs
• test/js/bun/http/proxy.test.ts
• test/js/web/fetch/fetch.tls.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

I reviewed this PR and didn't find any bugs. Because it changes TLS connection-pool eligibility around checkServerIdentity — a security-sensitive path — and encodes a design decision (distinct callbacks share the Callback pool, per-connection rather than per-request semantics), a human look would still be worthwhile.

Checked: PeerVerification::admits correctly isolates Native ↔ Callback in both directions and keeps None from satisfying either; pooled_socket_verification().max() prevents a lax borrower from downgrading a strict socket on re-pool; can_offer_h2() still gates on CertErrors so a callback request never actually enters the h2 coalescing path; the did_have_handshaking_error_while_reject_unauthorized_is_false guard against required_for_target > None is behaviorally equivalent to the old reject_unauthorized check.

Extended reasoning...

Overview

This PR restores keep-alive pooling for fetch() requests that supply a checkServerIdentity callback (regressed in 1.4.0 via #33072), by replacing the boolean established_with_reject_unauthorized on pooled sockets / CONNECT tunnels / h2 sessions with a three-state PeerVerification enum (None / Callback / Native) and matching pool takers to connections verified the same way. It touches src/http/{HTTPContext,ProxyTunnel,lib}.rs, src/http/h2_client/{ClientSession,PendingConnect}.rs, and adds four tests across fetch.tls.test.ts and proxy.test.ts.

Security risks

The change lives directly in the TLS connection reuse guard — the code that decides whether a request may inherit a previously-verified connection instead of running its own hostname/identity check. The admits() predicate is small and the invariants check out (None may take any; Callback and Native only take exact matches; the sticky max() on re-pool prevents downgrade), and the third new test proves a Callback request under NODE_TLS_REJECT_UNAUTHORIZED=0 still fails chain verification rather than inheriting the lax socket. But the PR also encodes a policy choice: two different checkServerIdentity closures for the same host+TLS-config share one pool, and the callback runs once per connection rather than per request. The description argues this matches 1.3 and Node's https.Agent, and offers keepalive: false as the escape hatch — that reasoning is sound, but it's the kind of security-semantics decision a maintainer should sign off on.

Level of scrutiny

High. This is not a mechanical refactor: it removes a guard (CertErrors from is_keep_alive_possible) that was deliberately added in #33072, and replaces it with a finer-grained partition. The implementation is careful, well-commented, and the test coverage is targeted (direct HTTPS, CONNECT tunnel, and the lax→strict isolation case). I verified can_offer_h2() still short-circuits on CertErrors, so the h2-side verification plumbing is defensive consistency rather than a live path for callback requests; and the required_for_target > PeerVerification::None rewrite of the handshaking-error guard reduces to the same boolean as before.

Other factors

The PR description flags a non-reproducible "2 errors" observed once in ~8 local proxy.test.ts runs; worth watching in CI. No prior human reviews on the timeline. Given the security surface and the embedded design decision, deferring to a human reviewer rather than auto-approving.