Repository navigation
Check upstream destinations sent through a forward proxy - #577
Conversation
The upstream client refuses loopback, private and link-local addresses when it dials, but with HTTP_PROXY or HTTPS_PROXY set it dials the forward proxy, so only the forward proxy's address was checked. A URL taken from upstream metadata, such as a Helm chart URL, a Composer dist URL, a token realm or a redirect, could then reach 169.254.169.254 or an internal host through the forward proxy. The transport's proxy function now resolves the destination of every request it sends to a forward proxy and applies the same rules, including upstream.allow_loopback and upstream.allow_private_hosts, before handing it over. A name that cannot be resolved locally is still passed on, since some networks only resolve public names at the forward proxy. Direct requests are unchanged. Git fetches and token requests use the same transport, so they are covered too.
andrew
left a comment
andrew
left a comment
There was a problem hiding this comment.
The bypass is real, but the gap is in safehttp: newClient keeps ProxyFromEnvironment from http.DefaultTransport and only gates DialContext, so every consumer of it has the same hole. Please move the destination check into safehttp in git-pkgs/registries (git-pkgs/registries#115), wrapping transport.Proxy in newClient next to the dial gate. The approach here carries over as is: resolve the destination, apply the same checks, and pass names that can't be resolved locally through to the forward proxy. This PR then becomes a dependency bump plus the docs paragraph.
|
Thanks, agreed, the gap belongs in safehttp. I've moved the destination check there in git-pkgs/registries#116, wrapping |
registries' safehttp now checks the destination of requests sent through a forward proxy (git-pkgs/registries#116), so the proxy's own guard is removed and newUpstreamClient is back to its earlier form. go.mod points at a pseudo-version of registries main until git-pkgs#116 is released.
# Conflicts: # go.mod # go.sum
simonchrz
added
the
breaking
Hardens the upstream address check for setups that use a forward proxy.
The upstream client refuses loopback, private, CGNAT and link-local addresses when it dials. With
HTTP_PROXYorHTTPS_PROXYset it dials the forward proxy instead of the destination, so only the forward proxy's address was checked, and URLs taken from upstream metadata (Helm chart URLs, Composer dist URLs, token realms, redirects) were passed through to whatever they named.The check now lives in registries'
safehttp(git-pkgs/registries#116): its transport resolves the destination of each request sent to a forward proxy and applies the same rules, includingupstream.allow_loopbackandupstream.allow_private_hosts, before handing it over. Link-local addresses stay blocked. A name that can't be resolved locally is still passed on, since some networks only resolve public names at the forward proxy. Direct requests are unchanged. Git fetches and OCI token requests go through the same client, so they are covered too.This PR bumps registries and documents how the rules apply with a forward proxy. The proxy-side guard from the first commit is removed again in the second, so
newUpstreamClientis unchanged against main; squashing gives just the bump and the docs.go.mod points at a pseudo-version of registries main until #116 is released. I'll switch it to the tag then.