feat(http): per-client dialers with injectable dialer/resolver - #1572
feat(http): per-client dialers with injectable dialer/resolver#1572Coldwings wants to merge 5 commits into
Conversation
Coldwings
added
bugfix
Coldwings
changed the title
| virtual ISocketStream* dial(std::string_view host, uint16_t port, bool secure, | ||
| uint64_t timeout = -1ULL) = 0; | ||
| // dial to a Unix Domain Socket | ||
| virtual ISocketStream* dial(std::string_view uds_path, uint64_t timeout = -1ULL) = 0; |
There was a problem hiding this comment.
IDialer should take proxy access into account.
There was a problem hiding this comment.
Good catch — addressed in f5d5bc5, and the proxy headers / pooling follow-up in ab140a6 and 0457d89.
IDialer is now a single dial(const DialTarget&, timeout), and DialTarget describes the whole connection rather than just the origin:
struct DialTarget {
std::string_view host; // origin host, without port
uint16_t port = 0; // origin port
bool secure = false; // the origin speaks TLS
std::string_view uds_path; // if set, connect here instead of TCP
std::string_view proxy_host; // empty: connect to the origin directly
uint16_t proxy_port = 0;
bool proxy_secure = false; // the proxy itself speaks TLS
std::string_view proxy_auth; // Proxy-Authorization value, may be empty
// Headers for the proxy itself to read, and the part of the connection
// pool key that they imply. Both are produced by the client's
// ProxyAuthenticator, once per dial. Never shown to the origin.
const HeadersBase* proxy_headers = nullptr;
std::string_view proxy_pool_key;
bool via_proxy() const { return !proxy_host.empty(); }
// a TLS origin behind a proxy is reached by tunneling with CONNECT
bool need_tunnel() const { return via_proxy() && secure; }
};so an injected dialer can tell the two ways a proxy is used apart, instead of having proxying decided behind its back.
This overlaps #1466, which had already found and fixed the underlying bug: a TLS origin behind a proxy was reached by handing the proxy an absolute-URI https://... request over a plaintext connection, which can never work, as the answer would have to arrive as TLS. Happy to align on which PR ends up carrying it.
Either way, #1466's design fed into this one, and one detail earned its keep immediately — scoping a tunnel's pool key by the credentials that authenticated it. My first version keyed only on (proxy, origin), and a test written after reading #1466 showed a second set of credentials silently reusing the first's authenticated tunnel. It is now keyed on proxy, origin and auth. The distinction is that a CONNECT is authenticated once, for the tunnel, whereas a forwarded request carries Proxy-Authorization per request and may therefore share one connection to the proxy across credentials. @lihuiba's make_conditional_cat_list suggestion on that same key in #1466 is what the key builder here uses.
Proxy headers, and the pooling they imply
I first left these two open. They are needed now, so ab140a6 implements them — and doing so turned up something worse than a missing feature. @lihuiba's objection to a proxy_header on Operation points at the real defect: which headers a proxy gets to read is a property of one hop, not of a request. A redirect can turn a forwarded request into a tunneled one, and only the former is read by the proxy — inside a tunnel it is the origin that reads them. So a request object cannot hold them correctly, and Proxy-Authorization sitting there was already wrong in both directions. f5d5bc5 guarded it by the scheme of the first URL, which a redirect defeats:
http://origin, forwarded with the credentials (correctly), redirected to anhttps://one: the request keeps them while it travels inside the tunnel, so the origin is shown the proxy's credentials.https://origin, tunneled without them (correctly), redirected to anhttp://one: it is forwarded with no credentials at all, and is answered with 407.
Both are pinned by tests, the leak end to end: the TLS origin echoes back that it received Proxy-Authorization.
Nothing in a set of headers records who its reader was meant to be, so a redirect has nothing to separate the proxy's headers from the caller's own by. Filtering by name would not do it either — it cannot cover the headers a caller adds itself. And removing them after the fact is expensive: headers are appended in insertion order, so erasing one costs about as much as rebuilding the message. Hence:
Message::send_header(stream, extra)writesextraon the wire after its own headers without registering an entry for any of them. A redirect of that message cannot carry them along because there is nothing there to carry — the isolation is structural, not a cleanup step someone has to remember. TheCONNECTgets them this way too, as aRequestof its own.ProxyAuthenticator—Delegate<int, const DialTarget&, ProxyAuth&>, set byClient::set_proxy_authenticator()— is called once per dial through a proxy, just before connecting, so the headers may be refreshed and may differ from one request to the next while belonging to no request:
struct ProxyAuth {
CommonHeaders<4 * 1024 - 1> headers; // put into the CONNECT, or into a forwarded request
estring pool_key; // connections differing here are never shared
};- The
pool_keyis the caller-supplied component of the tunnel's key that I suspected would be needed. ACONNECTauthenticates a tunnel once, so only the identity that opened it may reuse it — but only the caller can tell which of its headers mean identity (a tenant, a route) and which are per-request noise (a trace id). Keying on all of them would open a tunnel per request; keying on none would let one identity ride another's tunnel.Proxy-Authorizationstays keyed automatically, and the caller's contribution goes last in the key, where the colons it may contain cannot be mistaken for the separators of the fields before it. - The outer leg of a tunnel is still not pooled on its own: a tunnel is reused as a whole.
Covered by http_client.proxy_request_line, connect_tunnel, connect_tunnel_auth, connect_tunnel_refused, connect_tunnel_distinct_credentials, proxy_authenticator_headers_and_pool_key, proxy_headers_do_not_follow_a_redirect_into_a_tunnel, proxy_headers_come_back_when_a_redirect_leaves_the_tunnel and ReqHeaders.connect_is_in_authority_form. The three new http_client ones were each checked by temporarily reverting the fix they cover, so they are known to fail without it. PTAL — @lihuiba too, since the shape of the authenticator and of the key is what you flagged.
There was a problem hiding this comment.
Update: the two points I had left open above are implemented in ab140a6, and my reply is edited accordingly — headers for the proxy now come from a ProxyAuthenticator called once per dial, and the tunnel's pool key admits a caller-supplied component.
Implementing it turned up a defect worth calling out on its own: because Proxy-Authorization was stored on the request, a plaintext request forwarded with credentials and then redirected to a TLS origin kept them while travelling inside the tunnel — the origin was shown the proxy's credentials. f5d5bc5 guarded that by the scheme of the first URL, which a redirect defeats; the mirror case failed the other way, with a forwarded hop after a tunnel carrying no credentials at all and being answered 407. Both are now pinned by tests, the leak end to end.
The fix is structural rather than a filter: Message::send_header(stream, extra) writes the proxy's headers on the wire without registering an entry for any of them, so a redirect has nothing to carry over. Details and reasoning in the edited reply and in the PR description.
…socket pool - Replace the process-wide thread_local PooledDialer (shared by all clients on a vCPU) with per-(client, vCPU) dialers owned by the client: deleting a client now tears down its connection pools deterministically, and clients no longer silently share the first client's TLS context. - Track dialers in a per-vCPU registry whose photon::fini() hook destroys whatever is left (leaked clients, or the fini+init cycle of a pthread_atfork handler), so no pool collector outlives its vCPU. - Add Client::set_dialer() / set_resolver() for dependency injection; the factory signature and default behavior remain source-compatible. - TCPSocketPool implements ResetHandle: the atfork child drops idle pooled connections, closing raw fds first so that stream destructors (e.g. TLS close_notify) cannot write into connections shared with the parent, and without touching the inherited epoll instance.
Revert the TCPSocketPool ResetHandle change and its fork test to keep this PR focused on the HTTP client dialer lifecycle; the pool fork-safety will be submitted as a separate PR.
df6d911 to
3d030a8
Compare
The IDialer added by this PR described only where the origin server is, so an injected dialer could not tell whether the connection had to go to a proxy instead. The built-in one, in turn, reached a TLS origin behind a proxy by handing it an absolute-URI request over a plaintext connection, which no proxy can answer with a TLS response. - Collapse the two dial() overloads into a single dial(const DialTarget&). The target carries the origin, the unix socket path, the proxy and its credentials, so an injected dialer sees the whole picture; via_proxy() and need_tunnel() name the two ways a proxy is used. - The built-in dialer now opens a CONNECT tunnel for a TLS origin behind a proxy and handshakes with the origin inside it, so the proxy sees no plaintext. A tunnel is pooled under the proxy it was opened through, the origin it leads to and the credentials that authenticated it, since none of the three may differ for it to be reusable. - Write a request in absolute-URI form only for a plaintext origin; a tunneled request is in origin-form, exactly like a direct one. - Honor the userinfo of a per-operation proxy URL as Proxy-Authorization, which used to be ignored, and keep those credentials out of the request that travels inside the tunnel. - Share one DNS cache process-wide instead of one per vCPU, so that each host is cold-resolved once rather than once per vCPU. A borrow spans a single resolver call; the owning vCPU unpublishes the cache in its fini hook and drains the borrowers before freeing it.
3d030a8 to
f5d5bc5
Compare
…el pool Which headers a proxy gets to read is a property of one hop, but the client stored them in the per-request object, where a redirect carries them along. The previous commit narrowed that to the scheme of the first URL, which is still wrong in both directions: a request that starts plaintext, is forwarded with Proxy-Authorization, and is then redirected to a TLS origin keeps those credentials while it travels inside the tunnel, so the origin is shown the proxy's credentials; one that starts TLS and is redirected to a plaintext origin is forwarded with no credentials at all, and is answered with 407. Nothing in a set of headers records who the reader was meant to be, so a redirect cannot separate the ones for the proxy from the caller's own. Rather than guess by name -- which cannot cover the headers a caller adds itself -- the headers for the proxy are now never entered into the request at all. - Add ProxyAuthenticator, called once per dial through a proxy, just before connecting. It returns the headers the proxy is to read and the pool key they imply, so they may be refreshed and may differ from one request to the next without being owned by any request. DialTarget carries both to the dialer. - Take the caller's pool key into the key of a tunnel. A tunnel is authenticated once, by the CONNECT that opened it, so only the identity that opened it may reuse it; only the caller can tell which of its headers mean identity and which are per-request noise. Keying on all of them would open a tunnel per request, on none of them would let one identity ride another's tunnel. - Send the proxy's headers as `extra` to Message::send_header(), which writes them on the wire after our own without registering an entry for any of them. A redirect of that message therefore cannot carry them to the next hop. - Build the CONNECT as a Request of its own instead of concatenating a string, so the authenticator's headers are deduplicated against Host and Proxy-Authorization rather than appended blindly. make_request_line() writes the authority-form target that CONNECT asks for. The response is still parsed by hand, since it must be read without consuming the bytes that follow it.
The leak in the other direction is covered end to end, but its mirror was not: an operation that starts inside a tunnel, where the origin reads the headers, and is redirected to a plaintext origin, which is forwarded -- so the proxy reads them again. Deciding by the scheme of the first URL leaves that second hop with no credentials at all, and a 407 in reply. The fake proxy learns to answer a forwarded request with a plain 200, next to the redirect it already served, and the test origin learns to redirect. Temporarily dropping the credentials of the forwarded hop fails the new test, so the assertion is load-bearing.
|
Two comments from GPT, for reference： 2，_users is shared across resolver generations. After at_photon_fini() unpublishes R1, another vCPU can publish R2 and increment the same _users. The owner of R1 will then wait for R2's borrowers as well, and may hit the drain timeout/leak path even though R1 itself has no borrowers left. |
3 participants
Summary
Rework the HTTP client's dialer on three fronts: replace the process-wide
thread_local PooledDialer(shared by all clients on a vCPU) with per-(client, vCPU)dialers owned by theClient; make dialing proxy-aware, so that a TLS origin behind a proxy is reached through a realCONNECTtunnel and the headers meant for the proxy are decided per hop rather than stored on the request; and share one DNS cache process-wide instead of one per vCPU. Addsset_dialer()/set_resolver()/set_proxy_authenticator()injection points; the existing API stays source-compatible.Motivation
dialer.init()latches on first use, so the first client's TLS context / bind IPs silently win over every later client on that vCPU (the old comment inclient.heven suggested "use separate std::threads for different TLS configurations" as a workaround).thread_localdestructors run at OS-thread exit, afterphoton::fini()has torn down the runtime — the existingat_photon_finihook is a patch over the wrong lifetime ownership, and deleting aClientnever releases its connection pools.Changes
Per-client dialers (lifetime fix)
PooledDialerbecomes owned by a(client, vCPU)pair, created lazily on first use per vCPU, tracked by a per-vCPUDialerRegistry.~ClientImpl(claiming it out of the registry; when deleted cross-vCPU, a helper photon thread is migrated to the owning vCPU), or thephoton::fini()hook of its vCPU (covers leaked clients and the fini+init cycle ofpthread_atforkhandlers). Ownership is decided by who unlinks the dialer from the registry, so the two paths cannot race.Proxy-aware dialing, and CONNECT tunneling for TLS origins
IDialeris now a singledial(const DialTarget&, timeout).DialTargetcarries the whole picture — the origin (host / port / secure), the unix socket path, the proxy and its credentials — andvia_proxy()/need_tunnel()name the two ways a proxy is used, so an injected dialer can serve both.CONNECTtunnel and handshaking with the origin inside it (SNI = origin), which is the point of tunneling rather than forwarding: the proxy sees no plaintext.CONNECTis authenticated once, for the tunnel, so a tunnel opened with one set of credentials must not be handed to a request carrying another — unlike a forwarded request, whoseProxy-Authorizationtravels per request and therefore permits sharing one connection to the proxy across credentials. Their outer legs are not pooled separately: a tunnel is reused as a whole, never its outer leg alone.CONNECTresponse fails withECONNREFUSED, which the round-trip already maps to a fast retry instead of an exponential backoff. Any bytes trailing the response terminator are rejected asEPROTO, because the client is the one that speaks first inside a tunnel.Proxy-Authorizationis unified: the userinfo of a per-operation proxy URL is now honored (it used to be ignored — only client-level credentials worked). Where those credentials go is decided per hop, together with the rest of the headers meant for the proxy — see the next section.Proxy headers, and the pooling they imply
Raised in review, and in the discussion around #1466: richer proxy headers are needed, the tunnel pool has to account for them, and neither belongs on the
Operation.Which headers a proxy gets to read is a property of one hop, not of a request: a redirect can turn a forwarded request into a tunneled one, and only the former is read by the proxy — inside a tunnel it is the origin that reads them. A request object is therefore the wrong home for them, and holding
Proxy-Authorizationthere is already wrong in both directions. The tunneling commit above guarded it by the scheme of the first URL, which a redirect defeats:http://origin, forwarded with the credentials (correctly), redirected to anhttps://one: the request keeps them while it travels inside the tunnel, so the origin is shown the proxy's credentials.https://origin, tunneled without them (correctly), redirected to anhttp://one: it is forwarded with no credentials at all, and is answered with 407.Nothing in a set of headers records who its reader was meant to be, so a redirect cannot separate the proxy's headers from the caller's own; guessing by name would not cover the headers a caller adds itself. Removing them afterwards is not the answer either — headers are stored in insertion order, so erasing one costs about as much as rebuilding the message. So the headers meant for the proxy are never entered into the request at all:
Message::send_header()takes an optionalextraset of headers, written on the wire after its own without an entry being registered for any of them. A redirect of that message cannot carry them along, because there is nothing there to carry. The isolation is structural rather than a cleanup step that has to be remembered.ProxyAuthenticator—Delegate<int, const DialTarget&, ProxyAuth&>, set byClient::set_proxy_authenticator()— is called once per dial that goes through a proxy, just before connecting. It returns the headers the proxy is to read and the pool key they imply, so they may be refreshed and may differ from one request to the next while belonging to no request.DialTargetcarries both to the dialer, injected ones included.CONNECTauthenticates a tunnel once, so only the identity that opened it may reuse it — but only the caller can tell which of its headers mean identity (a tenant, a route) and which are per-request noise (a trace id). Keying on all of them would open a tunnel per request; keying on none would let one identity ride another's tunnel.Proxy-Authorizationstays keyed automatically, as before.CONNECTis now built as aRequestof its own instead of being concatenated as a string, so the authenticator's headers are deduplicated againstHostandProxy-Authorizationrather than appended blindly.make_request_line()learns the authority-form target thatCONNECTasks for. Its response is still parsed by hand, since it has to be read without consuming the bytes that follow it.One DNS cache per process
photon::Timerand thus lives on the vCPU that created it. A borrow spans a single resolver call; the owning vCPU unpublishes the cache in its fini hook and drains the borrowers before freeing it — and, should draining ever time out, leaks it loudly rather than freeing it under a borrower. The next dial on a live vCPU publishes a fresh cache in its place.set_resolver()overrides this entirely, as before.Dependency injection (API addition, source-compatible)
IDialerplus three non-virtual setters:Client::set_dialer(IDialer*)— take over connection establishment / pooling entirely;Client::set_resolver(Resolver*)— supply a DNS cache of one's own;Client::set_proxy_authenticator(ProxyAuthenticator)— take over the headers sent to the proxy, and the pooling of the connections they authenticate.new_http_client()signature and default behavior are unchanged; existing code compiles and behaves as before, except that clients no longer accidentally share TLS contexts, and https-over-proxy now actually works — both bug fixes.Tests
http_client.per_client_dialer_lifecycle: per-client dialer isolation; deleting one client tears down only its own pools.http_client.dialer_injection/http_client.resolver_injection: the new setters.http_client.cross_vcpu_client_destruction: client used on a worker vCPU, deleted from the main vCPU while the worker is alive (migrate path).http_client.proxy_request_line: absolute-URI for a plaintext origin, origin-form for a TLS one.http_client.connect_tunnel: a fakeCONNECTproxy in front of a TLS origin — asserts the origin is reached, that the proxy was asked for the right authority, and that the tunnel is pooled (two requests, oneCONNECT).http_client.connect_tunnel_auth: the credentials reach the proxy and do not leak to the origin.http_client.connect_tunnel_refused: a 407 from the proxy fails the call.http_client.connect_tunnel_distinct_credentials: two users of the same proxy get a tunnel each, rather than the second borrowing the first's authenticated one.http_client.proxy_authenticator_headers_and_pool_key: the authenticator's headers reach the proxy and not the origin; a header it declares to be noise does not multiply tunnels, while one it declares to be an identity gets a tunnel of its own.http_client.proxy_headers_do_not_follow_a_redirect_into_a_tunnel: a forwarded request, answered by the proxy with a redirect to a TLS origin, continues as a tunneled one — and the origin receives none of the headers the proxy read on the first hop.http_client.proxy_headers_come_back_when_a_redirect_leaves_the_tunnel: the mirror case — a tunneled request redirected to a plaintext origin is forwarded, so the headers the proxy reads must be there again.ReqHeaders.connect_is_in_authority_form: aCONNECTnames a host and a port, with neither scheme nor path.http_clienttests above were each checked by temporarily reverting the fix they cover, so that they are known to fail without it.One existing assertion changed:
ReqHeaders.redirectrequired ahttps://URL withenable_proxy=trueto produce an absolute-URI request line. That is the unusable combination described in Motivation 4 (plaintext to the proxy, TLS expected back), so the test now covers the new rule instead — absolute-URI for a plaintext origin, origin-form for a TLS one.References
CONNECTtunneling as well, and found the underlying bug first; which PR ends up carrying it is still to be aligned. Its design informed this one concretely — most usefully the pooling key that scopes a tunnel by credentials, which caught a real bug in the first version of this commit.TCPSocketPoolfork-safe viaResetHandle, complementing the fini+initpthread_atforkpattern; split out of this PR to keep it focused.Transfer-Encodingparsing (case-insensitive, tolerant of OWS,chunkedrequired to be last) and response body decompression.