fix(ssrf): avoid DNS resolution at construction time in NewSSRFSafeTransport#3540
Conversation
HTTP_PROXY/HTTPS_PROXY/ALL_PROXY hostnames were synchronously resolved during transport construction, blocking callers (e.g. NewLocalRuntime) for up to 5 s per proxy env var. The allowlist now stores literal pre-resolution host:port strings; matching moves to DialContext, which receives the proxy's literal address before the OS resolves it. Assisted-By: Claude (Anthropic)
…ctors Assisted-By: Claude (Anthropic)
docker-agent
left a comment
docker-agent
left a comment
There was a problem hiding this comment.
Assessment: 🟡 NEEDS ATTENTION
PR: fix(ssrf): avoid DNS resolution at construction time in NewSSRFSafeTransport
Author: dgageot
The fix correctly eliminates the blocking DNS call at construction time by storing proxy host:port strings verbatim and matching at dial time — the core goal is achieved and the implementation is sound for the common case. One edge case in the allowlist-bypass logic warrants attention before merge.
| Severity | Category | Finding |
|---|---|---|
| 🟡 MEDIUM | security | Verbatim hostname allowlist can bypass SSRF guard for direct connections when NO_PROXY excludes the proxy host |
Summary: When HTTP_PROXY=http://proxy.internal:3128 and NO_PROXY=proxy.internal are both set (common to prevent proxy-of-proxy loops), requests to http://proxy.internal:3128/ are dialed directly — DialContext receives proxy.internal:3128, the allowlist matches, and SSRFDialControl is skipped entirely via the unguarded trusted dialer. The TestNewSSRFSafeTransport_AllowlistFrozenAtConstruction test implicitly confirms that DialContext is called with the target host:port for direct connections. The scenario requires the attacker to influence a URL pointing to the exact proxy address, so exploitation is constrained — but it is a real bypass path introduced by this PR's design.
| // transport dials the proxy's literal host:port, so the allowlist can | ||
| // be matched by string without ever resolving the proxy hostname. | ||
| t.DialContext = func(ctx context.Context, network, address string) (net.Conn, error) { | ||
| if _, ok := proxies[canonicalHostPort(address)]; ok { |
There was a problem hiding this comment.
[MEDIUM/LIKELY] Verbatim hostname allowlist can bypass SSRF guard for direct connections when NO_PROXY excludes the proxy host
The new allowlist stores proxy entries as verbatim host:port strings (e.g. proxy.internal:3128), and DialContext checks this allowlist before deciding whether to use the unguarded trusted dialer or the SSRF-protected guarded dialer. This is correct for the proxied-connection case — Go's http.Transport calls DialContext with the proxy's literal address when routing through a proxy.
However, DialContext is also called with the target's host:port for direct connections (when NO_PROXY or the absence of a proxy causes http.Transport to bypass the proxy). The test TestNewSSRFSafeTransport_AllowlistFrozenAtConstruction directly demonstrates this: a request to http://10.0.0.1:3128/ goes through DialContext with address="10.0.0.1:3128" and is blocked by the SSRF guard — which means if that address were in the allowlist, the block would be skipped.
Attack scenario (requires two concurrent conditions):
HTTP_PROXY=http://proxy.internal:3128is set (addsproxy.internal:3128to allowlist)NO_PROXY=proxy.internalis also set — common practice to prevent proxy-of-proxy loops — causing Go's transport to dialproxy.internaldirectly, bypassing the proxy- Attacker influences a request URL to point to
http://proxy.internal:3128/
The result: DialContext is called with proxy.internal:3128, the allowlist matches, trusted.DialContext is used (no SSRFDialControl), and the connection succeeds unchecked.
The old IP-based approach had an analogous structural issue (if the proxy IP matched a target IP, SSRF was bypassed), so the attack surface class is not entirely new, but this PR introduces the verbatim string-based allowlist mechanism that makes the bypass deterministic and predictable for hostname-based proxies.
Suggested mitigation: Track whether the dial is to the proxy itself (via a transport-level Proxy func wrapper that records which addresses are being used as proxies for the current request) or check NO_PROXY exclusions when building the allowlist — e.g. if a proxy host is also in NO_PROXY, do not add it to the trusted allowlist.
3 participants
NewSSRFSafeTransportis reached early — at runtime construction, not at request time — viaNewLocalRuntimewhen thehttp_posthook builtin sets up its default client. IfHTTP_PROXY,HTTPS_PROXY, orALL_PROXYpointed at a hostname rather than a bare IP, the constructor synchronously resolved each one with a 5-secondLookupIPAddrcall per env var. On machines without a fast or reliable DNS path this made agent startup noticeably slow, and on machines where DNS is unavailable it silently failed to populate the allowlist, breaking proxy support entirely.The fix changes how the proxy allowlist is built and matched. Instead of resolving hostnames eagerly and storing IP addresses, the allowlist now stores each proxy's
host:portstring verbatim.Transport.DialContextalready receives the proxy's pre-resolved literal address, so a simple string comparison at dial time is sufficient. Trusted proxy dials bypass the SSRF hook dialer; everything else is still checked against the post-resolution SSRF control as before.A new test (
TestProxyDialAllowlist_NoDNSAtConstruction) asserts the constructor returns instantly even when the proxy env vars name an unresolvable host, and the existing suite was updated to match the new matching semantics. A companion lint cop extends the existingConstructorNetworkIOcheck to flagnet.Lookup*/Resolver.Lookup*calls inNew*constructors going forward, since this case escaped detection both because DNS lookups were not in the flagged-call list and because the cop only inspects the constructor body directly, not transitive helpers.