From 9500c5b5a44f7be47274da1f20a09a35739d8932 Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sat, 24 Oct 2026 08:07:13 +0200 Subject: [PATCH 1/3] Check upstream destinations sent through a forward proxy 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. --- docs/configuration.md | 2 + internal/server/forward_proxy.go | 61 +++++++++++++++ internal/server/forward_proxy_test.go | 105 ++++++++++++++++++++++++++ internal/server/server.go | 9 ++- 4 files changed, 174 insertions(+), 3 deletions(-) create mode 100644 internal/server/forward_proxy.go create mode 100644 internal/server/forward_proxy_test.go diff --git a/docs/configuration.md b/docs/configuration.md index fa77ec69..bc97ae90 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -293,6 +293,8 @@ Each upstream used by a built-in package route can be set in YAML or JSON under Private, ULA, CGNAT, and loopback addresses are rejected by default. Add each private upstream hostname or IP address to `upstream.allow_private_hosts`. The matching environment variable accepts a comma-separated list. Loopback upstreams also require `upstream.allow_loopback: true`. That setting permits upstream requests and redirects to reach any loopback address. +Upstream requests honour `HTTP_PROXY`, `HTTPS_PROXY` and `NO_PROXY`. When a request goes through a forward proxy, these rules apply twice: to the forward proxy's own address, which therefore has to be listed in `upstream.allow_private_hosts` if it is private, and to the request's destination, which the proxy resolves before handing the request over. Link-local addresses such as `169.254.169.254` are always refused. A destination the proxy cannot resolve itself is passed to the forward proxy, since some networks only resolve public names there; the forward proxy's own policy decides what such a name may reach. + ```yaml upstream: allow_private_hosts: diff --git a/internal/server/forward_proxy.go b/internal/server/forward_proxy.go new file mode 100644 index 00000000..66117a67 --- /dev/null +++ b/internal/server/forward_proxy.go @@ -0,0 +1,61 @@ +package server + +import ( + "context" + "net" + "net/http" + "net/url" + + "github.com/git-pkgs/registries/safehttp" +) + +// upstreamForwardProxy picks the forward proxy for an upstream request. It is +// a variable so tests can route through a test proxy without changing the +// environment, which net/http reads only once. +var upstreamForwardProxy = http.ProxyFromEnvironment + +// lookupUpstreamHost resolves a destination host for the forward proxy check. +var lookupUpstreamHost = net.DefaultResolver.LookupIPAddr + +// guardForwardProxy wraps proxy so that a request sent through a forward +// proxy is held to the same address rules as a direct one. The safehttp dial +// gate only sees the address it connects to, which for a proxied request is +// the forward proxy, so without this a URL taken from upstream metadata could +// reach a link-local or private address through the forward proxy. +// +// The destination is resolved here and every address must pass. A host that +// cannot be resolved locally is passed on, since setups that route all +// traffic through a forward proxy often cannot resolve public names +// themselves; the forward proxy resolves it. +func guardForwardProxy(proxy func(*http.Request) (*url.URL, error), opts safehttp.Options) func(*http.Request) (*url.URL, error) { + if proxy == nil { + return nil + } + checker := safehttp.NewHostIPChecker(opts) + return func(req *http.Request) (*url.URL, error) { + proxyURL, err := proxy(req) + if err != nil || proxyURL == nil { + return proxyURL, err + } + if err := checkProxiedDestination(req.Context(), checker, req.URL.Hostname()); err != nil { + return nil, err + } + return proxyURL, nil + } +} + +func checkProxiedDestination(ctx context.Context, checker *safehttp.HostIPChecker, host string) error { + if ip := net.ParseIP(host); ip != nil { + return checker.Check(host, ip) + } + addrs, err := lookupUpstreamHost(ctx, host) + if err != nil { + return nil //nolint:nilerr // the forward proxy resolves names this host cannot + } + for _, addr := range addrs { + if err := checker.Check(host, addr.IP); err != nil { + return err + } + } + return nil +} diff --git a/internal/server/forward_proxy_test.go b/internal/server/forward_proxy_test.go new file mode 100644 index 00000000..b5287f43 --- /dev/null +++ b/internal/server/forward_proxy_test.go @@ -0,0 +1,105 @@ +package server + +import ( + "context" + "errors" + "net" + "net/http" + "net/http/httptest" + "net/url" + "sync/atomic" + "testing" + + "github.com/git-pkgs/proxy/internal/config" +) + +// routeThroughTestProxy starts a forward proxy that answers every request +// itself, points upstream clients at it, and resolves hostnames from hosts. +func routeThroughTestProxy(t *testing.T, hosts map[string]string) *atomic.Int32 { + t.Helper() + var received atomic.Int32 + forward := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + received.Add(1) + _, _ = w.Write([]byte("via proxy")) + })) + t.Cleanup(forward.Close) + forwardURL, err := url.Parse(forward.URL) + if err != nil { + t.Fatal(err) + } + + oldProxy, oldLookup := upstreamForwardProxy, lookupUpstreamHost + upstreamForwardProxy = http.ProxyURL(forwardURL) + lookupUpstreamHost = func(_ context.Context, host string) ([]net.IPAddr, error) { + if ip, ok := hosts[host]; ok { + return []net.IPAddr{{IP: net.ParseIP(ip)}}, nil + } + return nil, &net.DNSError{Err: "no such host", Name: host, IsNotFound: true} + } + t.Cleanup(func() { upstreamForwardProxy, lookupUpstreamHost = oldProxy, oldLookup }) + return &received +} + +func TestUpstreamClientChecksDestinationBehindForwardProxy(t *testing.T) { + received := routeThroughTestProxy(t, map[string]string{ + "metadata.example.test": "169.254.169.254", + "intranet.example.test": "10.1.2.3", + "allowed.example.test": "10.1.2.4", + "public.example.test": "203.0.113.10", + }) + // The forward proxy listens on loopback, so the client has to be allowed + // to dial it. + client := newUpstreamClient(config.UpstreamConfig{ + AllowLoopback: true, + AllowPrivateHosts: []string{"allowed.example.test", "10.9.9.9"}, + }) + + for _, tt := range []struct { + url string + blocked bool + }{ + {"http://169.254.169.254/latest/meta-data/", true}, + {"https://169.254.169.254/latest/meta-data/", true}, + {"http://[fe80::1]/", true}, + {"http://metadata.example.test/", true}, + {"http://10.1.2.3/", true}, + {"http://intranet.example.test/", true}, + {"http://allowed.example.test/", false}, + {"http://10.9.9.9/", false}, + {"http://203.0.113.10/", false}, + {"http://public.example.test/", false}, + {"http://unresolvable.example.test/", false}, + } { + before := received.Load() + resp, err := client.Get(tt.url) + if err == nil { + _ = resp.Body.Close() + } + reached := received.Load() > before + if tt.blocked { + if err == nil || reached { + t.Errorf("GET %s: err = %v, reached forward proxy = %v; want refused before the forward proxy", tt.url, err, reached) + } + continue + } + if err != nil || !reached { + t.Errorf("GET %s: err = %v, reached forward proxy = %v; want it passed to the forward proxy", tt.url, err, reached) + } + } +} + +func TestGuardForwardProxyKeepsDirectRequests(t *testing.T) { + if guardForwardProxy(nil, upstreamSafeHTTPOptions(config.UpstreamConfig{})) != nil { + t.Error("a transport without a proxy func got one") + } + proxyErr := errors.New("bad proxy config") + guarded := guardForwardProxy(func(*http.Request) (*url.URL, error) { return nil, proxyErr }, upstreamSafeHTTPOptions(config.UpstreamConfig{})) + req := httptest.NewRequest(http.MethodGet, "http://169.254.169.254/", nil) + if _, err := guarded(req); !errors.Is(err, proxyErr) { + t.Errorf("err = %v, want the proxy func's own error", err) + } + direct := guardForwardProxy(func(*http.Request) (*url.URL, error) { return nil, nil }, upstreamSafeHTTPOptions(config.UpstreamConfig{})) + if proxyURL, err := direct(req); proxyURL != nil || err != nil { + t.Errorf("direct request: proxy = %v, err = %v; want nil, nil so the dial gate checks it", proxyURL, err) + } +} diff --git a/internal/server/server.go b/internal/server/server.go index 5b0ebf72..394a6998 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -611,14 +611,17 @@ func configureScanning(proxy *handler.Proxy, cfg config.ScanningConfig, baseURL } // newUpstreamClient builds the shared upstream client: a safehttp client whose -// transport keeps upstreamMaxIdleConnsPerHost idle connections per host and +// transport keeps upstreamMaxIdleConnsPerHost idle connections per host, // gives up after upstreamResponseHeaderTimeout when an upstream accepts a -// request but stalls before sending headers. +// request but stalls before sending headers, and checks the destination of +// requests sent through a forward proxy. func newUpstreamClient(upstream config.UpstreamConfig) *http.Client { - client := safehttp.New(nil, upstreamSafeHTTPOptions(upstream)) + opts := upstreamSafeHTTPOptions(upstream) + client := safehttp.New(nil, opts) if transport, ok := client.Transport.(*http.Transport); ok { transport.MaxIdleConnsPerHost = upstreamMaxIdleConnsPerHost transport.ResponseHeaderTimeout = upstreamResponseHeaderTimeout + transport.Proxy = guardForwardProxy(upstreamForwardProxy, opts) } return client } From 0b2fa835903be0d3567c2ea3feda9dd594139a3c Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Sat, 24 Oct 2026 19:00:57 +0200 Subject: [PATCH 2/3] Use registries' forward proxy check 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 #116 is released. --- go.mod | 2 +- go.sum | 4 +- internal/server/forward_proxy.go | 61 --------------- internal/server/forward_proxy_test.go | 105 -------------------------- internal/server/server.go | 9 +-- 5 files changed, 6 insertions(+), 175 deletions(-) delete mode 100644 internal/server/forward_proxy.go delete mode 100644 internal/server/forward_proxy_test.go diff --git a/go.mod b/go.mod index e2f860e0..3b8425e6 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,7 @@ require ( github.com/git-pkgs/integrity v0.2.0 github.com/git-pkgs/magic v0.5.0 github.com/git-pkgs/purl v0.1.23 - github.com/git-pkgs/registries v0.11.0 + github.com/git-pkgs/registries v0.11.1-0.20261024160437-36fa54623922 github.com/git-pkgs/sbom v0.1.7 github.com/git-pkgs/spdx v0.3.3 github.com/git-pkgs/vers v0.7.2 diff --git a/go.sum b/go.sum index f298c284..ed30d812 100644 --- a/go.sum +++ b/go.sum @@ -135,8 +135,8 @@ github.com/git-pkgs/pom v0.1.8 h1:mXCuWw6XkrKKZb6IurRZyIznYLPhV5AYg5ZO38Uj/l8= github.com/git-pkgs/pom v0.1.8/go.mod h1:sQBeYRzjCUn77fcABPn6I/a1aBHFxm55jTmkNxYtGtw= github.com/git-pkgs/purl v0.1.23 h1:RG7F73S4ZnbAnB/posWf2WS4OhdG0xUbyX8UIkEUhPw= github.com/git-pkgs/purl v0.1.23/go.mod h1:l8rCfvt5iXVrFIcoQ8+vCLOaHhrtEW2id0LENP+IzMw= -github.com/git-pkgs/registries v0.11.0 h1:kkB80NjO9UncyyxGfKmdPXWqUMNE/CkNw5Z/6gqW7Rs= -github.com/git-pkgs/registries v0.11.0/go.mod h1:bKxgdy/PwnzQXL2FnOktmSQHTKk6B85MA4VtvvYItPc= +github.com/git-pkgs/registries v0.11.1-0.20261024160437-36fa54623922 h1:sg69ptO+sEegScnAmS+iiEVOXYtz5Tb5H2AjZBPV4Vo= +github.com/git-pkgs/registries v0.11.1-0.20261024160437-36fa54623922/go.mod h1:bKxgdy/PwnzQXL2FnOktmSQHTKk6B85MA4VtvvYItPc= github.com/git-pkgs/sbom v0.1.7 h1:4eboy/Yed/SC9ZcC/xT50iywHPzWbmCU7azDb5T1RbA= github.com/git-pkgs/sbom v0.1.7/go.mod h1:KIkZX0v31l62SeFsi2qsQXcZRVrAXvIdPQ4x4en+G2M= github.com/git-pkgs/spdx v0.3.3 h1:xXlUoZzjDSkwxC6mWHfaW+kc8a0+LH6d+gGHuZWftqI= diff --git a/internal/server/forward_proxy.go b/internal/server/forward_proxy.go deleted file mode 100644 index 66117a67..00000000 --- a/internal/server/forward_proxy.go +++ /dev/null @@ -1,61 +0,0 @@ -package server - -import ( - "context" - "net" - "net/http" - "net/url" - - "github.com/git-pkgs/registries/safehttp" -) - -// upstreamForwardProxy picks the forward proxy for an upstream request. It is -// a variable so tests can route through a test proxy without changing the -// environment, which net/http reads only once. -var upstreamForwardProxy = http.ProxyFromEnvironment - -// lookupUpstreamHost resolves a destination host for the forward proxy check. -var lookupUpstreamHost = net.DefaultResolver.LookupIPAddr - -// guardForwardProxy wraps proxy so that a request sent through a forward -// proxy is held to the same address rules as a direct one. The safehttp dial -// gate only sees the address it connects to, which for a proxied request is -// the forward proxy, so without this a URL taken from upstream metadata could -// reach a link-local or private address through the forward proxy. -// -// The destination is resolved here and every address must pass. A host that -// cannot be resolved locally is passed on, since setups that route all -// traffic through a forward proxy often cannot resolve public names -// themselves; the forward proxy resolves it. -func guardForwardProxy(proxy func(*http.Request) (*url.URL, error), opts safehttp.Options) func(*http.Request) (*url.URL, error) { - if proxy == nil { - return nil - } - checker := safehttp.NewHostIPChecker(opts) - return func(req *http.Request) (*url.URL, error) { - proxyURL, err := proxy(req) - if err != nil || proxyURL == nil { - return proxyURL, err - } - if err := checkProxiedDestination(req.Context(), checker, req.URL.Hostname()); err != nil { - return nil, err - } - return proxyURL, nil - } -} - -func checkProxiedDestination(ctx context.Context, checker *safehttp.HostIPChecker, host string) error { - if ip := net.ParseIP(host); ip != nil { - return checker.Check(host, ip) - } - addrs, err := lookupUpstreamHost(ctx, host) - if err != nil { - return nil //nolint:nilerr // the forward proxy resolves names this host cannot - } - for _, addr := range addrs { - if err := checker.Check(host, addr.IP); err != nil { - return err - } - } - return nil -} diff --git a/internal/server/forward_proxy_test.go b/internal/server/forward_proxy_test.go deleted file mode 100644 index b5287f43..00000000 --- a/internal/server/forward_proxy_test.go +++ /dev/null @@ -1,105 +0,0 @@ -package server - -import ( - "context" - "errors" - "net" - "net/http" - "net/http/httptest" - "net/url" - "sync/atomic" - "testing" - - "github.com/git-pkgs/proxy/internal/config" -) - -// routeThroughTestProxy starts a forward proxy that answers every request -// itself, points upstream clients at it, and resolves hostnames from hosts. -func routeThroughTestProxy(t *testing.T, hosts map[string]string) *atomic.Int32 { - t.Helper() - var received atomic.Int32 - forward := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - received.Add(1) - _, _ = w.Write([]byte("via proxy")) - })) - t.Cleanup(forward.Close) - forwardURL, err := url.Parse(forward.URL) - if err != nil { - t.Fatal(err) - } - - oldProxy, oldLookup := upstreamForwardProxy, lookupUpstreamHost - upstreamForwardProxy = http.ProxyURL(forwardURL) - lookupUpstreamHost = func(_ context.Context, host string) ([]net.IPAddr, error) { - if ip, ok := hosts[host]; ok { - return []net.IPAddr{{IP: net.ParseIP(ip)}}, nil - } - return nil, &net.DNSError{Err: "no such host", Name: host, IsNotFound: true} - } - t.Cleanup(func() { upstreamForwardProxy, lookupUpstreamHost = oldProxy, oldLookup }) - return &received -} - -func TestUpstreamClientChecksDestinationBehindForwardProxy(t *testing.T) { - received := routeThroughTestProxy(t, map[string]string{ - "metadata.example.test": "169.254.169.254", - "intranet.example.test": "10.1.2.3", - "allowed.example.test": "10.1.2.4", - "public.example.test": "203.0.113.10", - }) - // The forward proxy listens on loopback, so the client has to be allowed - // to dial it. - client := newUpstreamClient(config.UpstreamConfig{ - AllowLoopback: true, - AllowPrivateHosts: []string{"allowed.example.test", "10.9.9.9"}, - }) - - for _, tt := range []struct { - url string - blocked bool - }{ - {"http://169.254.169.254/latest/meta-data/", true}, - {"https://169.254.169.254/latest/meta-data/", true}, - {"http://[fe80::1]/", true}, - {"http://metadata.example.test/", true}, - {"http://10.1.2.3/", true}, - {"http://intranet.example.test/", true}, - {"http://allowed.example.test/", false}, - {"http://10.9.9.9/", false}, - {"http://203.0.113.10/", false}, - {"http://public.example.test/", false}, - {"http://unresolvable.example.test/", false}, - } { - before := received.Load() - resp, err := client.Get(tt.url) - if err == nil { - _ = resp.Body.Close() - } - reached := received.Load() > before - if tt.blocked { - if err == nil || reached { - t.Errorf("GET %s: err = %v, reached forward proxy = %v; want refused before the forward proxy", tt.url, err, reached) - } - continue - } - if err != nil || !reached { - t.Errorf("GET %s: err = %v, reached forward proxy = %v; want it passed to the forward proxy", tt.url, err, reached) - } - } -} - -func TestGuardForwardProxyKeepsDirectRequests(t *testing.T) { - if guardForwardProxy(nil, upstreamSafeHTTPOptions(config.UpstreamConfig{})) != nil { - t.Error("a transport without a proxy func got one") - } - proxyErr := errors.New("bad proxy config") - guarded := guardForwardProxy(func(*http.Request) (*url.URL, error) { return nil, proxyErr }, upstreamSafeHTTPOptions(config.UpstreamConfig{})) - req := httptest.NewRequest(http.MethodGet, "http://169.254.169.254/", nil) - if _, err := guarded(req); !errors.Is(err, proxyErr) { - t.Errorf("err = %v, want the proxy func's own error", err) - } - direct := guardForwardProxy(func(*http.Request) (*url.URL, error) { return nil, nil }, upstreamSafeHTTPOptions(config.UpstreamConfig{})) - if proxyURL, err := direct(req); proxyURL != nil || err != nil { - t.Errorf("direct request: proxy = %v, err = %v; want nil, nil so the dial gate checks it", proxyURL, err) - } -} diff --git a/internal/server/server.go b/internal/server/server.go index 394a6998..5b0ebf72 100644 --- a/internal/server/server.go +++ b/internal/server/server.go @@ -611,17 +611,14 @@ func configureScanning(proxy *handler.Proxy, cfg config.ScanningConfig, baseURL } // newUpstreamClient builds the shared upstream client: a safehttp client whose -// transport keeps upstreamMaxIdleConnsPerHost idle connections per host, +// transport keeps upstreamMaxIdleConnsPerHost idle connections per host and // gives up after upstreamResponseHeaderTimeout when an upstream accepts a -// request but stalls before sending headers, and checks the destination of -// requests sent through a forward proxy. +// request but stalls before sending headers. func newUpstreamClient(upstream config.UpstreamConfig) *http.Client { - opts := upstreamSafeHTTPOptions(upstream) - client := safehttp.New(nil, opts) + client := safehttp.New(nil, upstreamSafeHTTPOptions(upstream)) if transport, ok := client.Transport.(*http.Transport); ok { transport.MaxIdleConnsPerHost = upstreamMaxIdleConnsPerHost transport.ResponseHeaderTimeout = upstreamResponseHeaderTimeout - transport.Proxy = guardForwardProxy(upstreamForwardProxy, opts) } return client } From 910dadc220f8868057dc60a37e3f521acaa4273f Mon Sep 17 00:00:00 2001 From: Andrew Nesbitt Date: Sun, 25 Oct 2026 10:17:37 +0000 Subject: [PATCH 3/3] Bump registries to v0.11.1 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 3b8425e6..d7a3d340 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,7 @@ require ( github.com/git-pkgs/integrity v0.2.0 github.com/git-pkgs/magic v0.5.0 github.com/git-pkgs/purl v0.1.23 - github.com/git-pkgs/registries v0.11.1-0.20261024160437-36fa54623922 + github.com/git-pkgs/registries v0.11.1 github.com/git-pkgs/sbom v0.1.7 github.com/git-pkgs/spdx v0.3.3 github.com/git-pkgs/vers v0.7.2 diff --git a/go.sum b/go.sum index ed30d812..77e04445 100644 --- a/go.sum +++ b/go.sum @@ -135,8 +135,8 @@ github.com/git-pkgs/pom v0.1.8 h1:mXCuWw6XkrKKZb6IurRZyIznYLPhV5AYg5ZO38Uj/l8= github.com/git-pkgs/pom v0.1.8/go.mod h1:sQBeYRzjCUn77fcABPn6I/a1aBHFxm55jTmkNxYtGtw= github.com/git-pkgs/purl v0.1.23 h1:RG7F73S4ZnbAnB/posWf2WS4OhdG0xUbyX8UIkEUhPw= github.com/git-pkgs/purl v0.1.23/go.mod h1:l8rCfvt5iXVrFIcoQ8+vCLOaHhrtEW2id0LENP+IzMw= -github.com/git-pkgs/registries v0.11.1-0.20261024160437-36fa54623922 h1:sg69ptO+sEegScnAmS+iiEVOXYtz5Tb5H2AjZBPV4Vo= -github.com/git-pkgs/registries v0.11.1-0.20261024160437-36fa54623922/go.mod h1:bKxgdy/PwnzQXL2FnOktmSQHTKk6B85MA4VtvvYItPc= +github.com/git-pkgs/registries v0.11.1 h1:AkVQ15Rnrho4tbTnocYuHQuzNQ1m9jEjWKTA8P3xnPI= +github.com/git-pkgs/registries v0.11.1/go.mod h1:bKxgdy/PwnzQXL2FnOktmSQHTKk6B85MA4VtvvYItPc= github.com/git-pkgs/sbom v0.1.7 h1:4eboy/Yed/SC9ZcC/xT50iywHPzWbmCU7azDb5T1RbA= github.com/git-pkgs/sbom v0.1.7/go.mod h1:KIkZX0v31l62SeFsi2qsQXcZRVrAXvIdPQ4x4en+G2M= github.com/git-pkgs/spdx v0.3.3 h1:xXlUoZzjDSkwxC6mWHfaW+kc8a0+LH6d+gGHuZWftqI=