From c9facef0bee07c11107e7b44dee92c6dd8e8419b Mon Sep 17 00:00:00 2001 From: v-abhishekbhaskar Date: Mon, 26 Oct 2026 17:35:03 -0500 Subject: [PATCH 1/2] allowlist registry redirect targets for private registries --- internal/handlers/egress_allowlist.go | 114 ++++++++++++++++++--- internal/handlers/egress_allowlist_test.go | 68 ++++++++++++ proxy.go | 1 + proxy_test.go | 63 ++++++++++++ 4 files changed, 234 insertions(+), 12 deletions(-) diff --git a/internal/handlers/egress_allowlist.go b/internal/handlers/egress_allowlist.go index 74c535e..4a27fd1 100644 --- a/internal/handlers/egress_allowlist.go +++ b/internal/handlers/egress_allowlist.go @@ -1,10 +1,12 @@ package handlers import ( + "net" "net/http" "path" "strconv" "strings" + "sync" "github.com/elazarl/goproxy" @@ -51,7 +53,17 @@ type EgressAllowlistHandler struct { // glob-free, so treating them as patterns (e.g. a configured "https://*.com") // would silently disable enforcement for every matching destination. dynamicHosts []string - metrics MetricSender + // redirectSources are the job's credential hosts that are not already on + // the static allowlist, i.e. the customer's own private registries. Only + // redirects issued by these hosts teach the allowlist a new target. + redirectSources []string + // redirectTargets are the exact hosts learned from redirects issued by a + // redirectSource, such as the per-instance S3 bucket a self-hosted + // Artifactory sends presigned downloads to. Guarded by redirectMu because + // requests are handled concurrently. + redirectMu sync.RWMutex + redirectTargets map[string]struct{} + metrics MetricSender } // NewEgressAllowlistHandler builds the allowlist from the always-allowed GitHub @@ -66,13 +78,24 @@ func NewEgressAllowlistHandler(cfg *config.Config, env config.ProxyEnvSettings, allowed = append(allowed, sharedRegistryDomains...) allowed = append(allowed, allEcosystemDomains...) - return &EgressAllowlistHandler{ - observe: cfg.Experiments.Enabled(egressObserveExperiment), - enforce: cfg.Experiments.Enabled(egressEnforceExperiment), - allowed: allowed, - dynamicHosts: dynamicHosts(cfg.Credentials), - metrics: metricSender, + h := &EgressAllowlistHandler{ + observe: cfg.Experiments.Enabled(egressObserveExperiment), + enforce: cfg.Experiments.Enabled(egressEnforceExperiment), + allowed: allowed, + dynamicHosts: dynamicHosts(cfg.Credentials), + redirectTargets: make(map[string]struct{}), + metrics: metricSender, } + // Public registries and GitHub hosts are excluded as redirect sources: they + // serve user-published content, so they are not trusted to vouch for an + // arbitrary destination. + for _, host := range credentialHosts(cfg.Credentials) { + host = strings.TrimSuffix(host, ".") + if host != "" && !h.isStaticallyAllowed(host) { + h.redirectSources = append(h.redirectSources, host) + } + } + return h } // HandleRequest logs and/or blocks requests whose host is not on the allowlist. @@ -125,15 +148,79 @@ func (h *EgressAllowlistHandler) recordHost(host string, allowed, blockEnforced }) } +// HandleResponse allows the exact host a private registry redirects to. +// +// Self-hosted registries such as Artifactory redirect downloads to a presigned +// URL on the customer's own object-storage bucket, whose name cannot be derived +// from the registry host. The registry is configured in the job's credentials, +// so the destination it chooses is trusted for the remainder of the job. +func (h *EgressAllowlistHandler) HandleResponse(rsp *http.Response, proxyCtx *goproxy.ProxyCtx) *http.Response { + if !h.observe && !h.enforce { + return rsp + } + if rsp == nil || proxyCtx == nil || proxyCtx.Req == nil || !isRedirectStatus(rsp.StatusCode) { + return rsp + } + source := strings.TrimSuffix(helpers.GetHost(proxyCtx.Req), ".") + if !h.isRedirectSource(source) { + return rsp + } + location := rsp.Header.Get("Location") + if location == "" { + return rsp + } + target, err := proxyCtx.Req.URL.Parse(location) + if err != nil || !strings.EqualFold(target.Scheme, "https") { + return rsp + } + host := strings.TrimSuffix(strings.ToLower(target.Hostname()), ".") + // IP literals are never learned: a redirect must not open a path to + // internal or link-local addresses. + if host == "" || net.ParseIP(host) != nil || h.isAllowed(host) { + return rsp + } + + h.redirectMu.Lock() + h.redirectTargets[host] = struct{}{} + h.redirectMu.Unlock() + logging.RequestLogf(proxyCtx, "* egress allowlisting redirect target %s from %s", host, source) + return rsp +} + +func isRedirectStatus(status int) bool { + switch status { + case http.StatusMovedPermanently, http.StatusFound, http.StatusSeeOther, + http.StatusTemporaryRedirect, http.StatusPermanentRedirect: + return true + } + return false +} + +func (h *EgressAllowlistHandler) isRedirectSource(host string) bool { + for _, source := range h.redirectSources { + if helpers.AreHostnamesEqual(host, source) { + return true + } + } + return false +} + +func (h *EgressAllowlistHandler) isStaticallyAllowed(host string) bool { + for _, entry := range h.allowed { + if hostMatchesAllowlistEntry(host, entry) { + return true + } + } + return false +} + func (h *EgressAllowlistHandler) isAllowed(host string) bool { // Normalize an absolute DNS name (trailing dot) so exact matches treat // "registry.npmjs.org." as equivalent to "registry.npmjs.org", consistent // with HostMatchesDomain's boundary handling for the suffix form. host = strings.TrimSuffix(host, ".") - for _, entry := range h.allowed { - if hostMatchesAllowlistEntry(host, entry) { - return true - } + if h.isStaticallyAllowed(host) { + return true } // Per-job credential hosts are matched exactly only (never as globs or // subdomain suffixes), so an untrusted credential value cannot widen the @@ -143,7 +230,10 @@ func (h *EgressAllowlistHandler) isAllowed(host string) bool { return true } } - return false + h.redirectMu.RLock() + _, learned := h.redirectTargets[strings.ToLower(host)] + h.redirectMu.RUnlock() + return learned } // hostMatchesAllowlistEntry reports whether host satisfies a single allowlist diff --git a/internal/handlers/egress_allowlist_test.go b/internal/handlers/egress_allowlist_test.go index fc77fea..a5540ad 100644 --- a/internal/handlers/egress_allowlist_test.go +++ b/internal/handlers/egress_allowlist_test.go @@ -12,6 +12,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/elazarl/goproxy" + "github.com/dependabot/proxy/internal/config" ) @@ -1606,3 +1608,69 @@ func TestEgressAllowlist_AnonymousCredentialDoesNotWidenBeyondExactHost(t *testi } } } + +// redirectFrom runs HandleResponse for a response to sourceURL with the given +// status and Location header. +func redirectFrom(t *testing.T, h *EgressAllowlistHandler, sourceURL string, status int, location string) { + t.Helper() + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, sourceURL, nil) + rsp := &http.Response{StatusCode: status, Header: http.Header{}, Request: req} + if location != "" { + rsp.Header.Set("Location", location) + } + require.Same(t, rsp, h.HandleResponse(rsp, &goproxy.ProxyCtx{Req: req})) +} + +func TestEgressAllowlist_PrivateRegistryRedirectTargetAllowed(t *testing.T) { + // A self-hosted Artifactory redirects downloads to a presigned URL on the + // customer's own per-instance bucket, which no static entry can cover. + const bucket = "jfrog-prod-b0fz94bdmu0ba.s3.amazonaws.com" + h := newEgressHandlerWithCreds(config.Credentials{ + {"type": "python_index", "index-url": "https://packages.example.com/artifactory/api/pypi/pypi/simple"}, + }) + require.NotNil(t, egressResult(t, h, "https://"+bucket+"/aol/filestore/9d/9dc0"), "blocked before the redirect") + + redirectFrom(t, h, "https://packages.example.com/artifactory/api/pypi/pypi/packages/x.whl", http.StatusFound, + "https://"+bucket+"/aol/filestore/9d/9dc0?X-Amz-Signature=abc") + + assert.Nil(t, egressResult(t, h, "https://"+bucket+"/aol/filestore/9d/9dc0?X-Amz-Signature=abc")) + for _, blocked := range []string{ + "https://other." + bucket + "/", + "https://jfrog-prod-other.s3.amazonaws.com/", + "https://s3.amazonaws.com/", + } { + assert.NotNil(t, egressResult(t, h, blocked), "only the exact redirect target is learned: "+blocked) + } +} + +func TestEgressAllowlist_RedirectTargetNotLearned(t *testing.T) { + const target = "https://attacker.example.net/file" + tests := []struct { + name string + source string + status int + location string + }{ + {"source is not a credential host", "https://unconfigured.example.com/x", http.StatusFound, target}, + {"source is a statically allowed credential host", "https://github.com/x", http.StatusFound, target}, + {"public registry credential host", "https://registry.npmjs.org/x", http.StatusFound, target}, + {"not a redirect status", "https://packages.example.com/x", http.StatusOK, target}, + {"304 is not a redirect", "https://packages.example.com/x", http.StatusNotModified, target}, + {"plain http target", "https://packages.example.com/x", http.StatusFound, "http://attacker.example.net/file"}, + {"IP literal target", "https://packages.example.com/x", http.StatusFound, "https://169.254.169.254/latest"}, + {"missing Location", "https://packages.example.com/x", http.StatusFound, ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + h := newEgressHandlerWithCreds(config.Credentials{ + {"type": "npm_registry", "registry": "packages.example.com", "token": "t"}, + {"type": "npm_registry", "registry": "registry.npmjs.org", "token": "t"}, + {"type": "git_source", "host": "github.com", "username": "x-access-token", "password": "p"}, + }) + redirectFrom(t, h, tt.source, tt.status, tt.location) + + assert.NotNil(t, egressResult(t, h, target)) + assert.NotNil(t, egressResult(t, h, "https://169.254.169.254/latest")) + }) + } +} diff --git a/proxy.go b/proxy.go index 79877ab..2163cf6 100644 --- a/proxy.go +++ b/proxy.go @@ -77,6 +77,7 @@ func newProxyWithCacheDir(envSettings config.ProxyEnvSettings, cfg *config.Confi egressAllowlistHandler := handlers.NewEgressAllowlistHandler(cfg, envSettings, metricsClient) proxy.OnRequest().DoFunc(egressAllowlistHandler.HandleRequest) + proxy.OnResponse().DoFunc(egressAllowlistHandler.HandleResponse) enableCache := os.Getenv("PROXY_CACHE") == "true" cacher, err := cache.New(enableCache, cacheDir) diff --git a/proxy_test.go b/proxy_test.go index d3d1cec..f626ab6 100644 --- a/proxy_test.go +++ b/proxy_test.go @@ -327,6 +327,69 @@ func TestProxyEgressAllowlistObserveAllows(t *testing.T) { "observe mode must log the non-allowlisted host") } +// A self-hosted Artifactory redirects downloads to a presigned URL on the +// customer's own bucket. Enforce mode must allow that exact redirect target +// while still blocking every other unlisted host. +func TestProxyEgressAllowlistEnforceAllowsPrivateRegistryRedirectTarget(t *testing.T) { + t.Setenv("PROXY_CACHE", "false") + const bucketURL = "https://bucket.example.com/filestore/9d/9dc0?X-Amz-Signature=abc" + upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Host { + case "artifactory.example.com": + assert.NotEmpty(t, r.Header.Get("Authorization"), "registry request is authenticated") + http.Redirect(w, r, bucketURL, http.StatusFound) + case "bucket.example.com": + assert.Empty(t, r.Header.Get("Authorization"), "presigned request must carry no credential") + _, _ = io.WriteString(w, "wheel") + default: + w.WriteHeader(http.StatusTeapot) + } + })) + defer upstream.Close() + + cfg := &config.Config{ + CA: testProxyConfig.CA, + Experiments: config.Experiments{"proxy-egress-enforce": true}, + Credentials: config.Credentials{{ + "type": "python_index", + "index-url": "https://artifactory.example.com/artifactory/api/pypi/pypi/simple", + "token": "user:secret", + }}, + } + client, proxy := testProxyServer(t, cfg, nil, upstream.Certificate()) + closeOnCleanup(t, proxy) + t.Cleanup(client.CloseIdleConnections) + proxyHandler := proxy.Handler.(*Proxy) + t.Cleanup(proxyHandler.Tr.CloseIdleConnections) + proxyHandler.Tr.Proxy = nil + proxyHandler.Tr.DialContext = func(ctx context.Context, network, address string) (net.Conn, error) { + return (&net.Dialer{}).DialContext(ctx, network, upstream.Listener.Addr().String()) + } + + get := func(rawURL string) (int, string) { + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, rawURL, nil) + require.NoError(t, err) + rsp, err := client.Do(req) + require.NoError(t, err) + defer func() { + require.NoError(t, rsp.Body.Close()) + }() + body, err := io.ReadAll(rsp.Body) + require.NoError(t, err) + return rsp.StatusCode, string(body) + } + + status, _ := get(bucketURL) + assert.Equal(t, http.StatusForbidden, status, "bucket is blocked before the registry redirects to it") + + status, body := get("https://artifactory.example.com/artifactory/api/pypi/pypi/packages/x.whl") + assert.Equal(t, http.StatusOK, status) + assert.Equal(t, "wheel", body) + + status, _ = get("https://other.example.com/") + assert.Equal(t, http.StatusForbidden, status, "other unlisted hosts stay blocked") +} + func TestProxyHTTPSMITMFixedLengthResponseFraming(t *testing.T) { upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Length", "5") From bb8f9bb5f86324673c040bc15aa607b4b24736a8 Mon Sep 17 00:00:00 2001 From: v-abhishekbhaskar Date: Mon, 26 Oct 2026 18:09:10 -0500 Subject: [PATCH 2/2] reject plain text source requests before allowing redirects --- internal/handlers/egress_allowlist.go | 5 ++ internal/handlers/egress_allowlist_test.go | 1 + proxy.go | 4 +- proxy_test.go | 57 ++++++++++++++++++++++ 4 files changed, 66 insertions(+), 1 deletion(-) diff --git a/internal/handlers/egress_allowlist.go b/internal/handlers/egress_allowlist.go index 4a27fd1..7166bfd 100644 --- a/internal/handlers/egress_allowlist.go +++ b/internal/handlers/egress_allowlist.go @@ -161,6 +161,11 @@ func (h *EgressAllowlistHandler) HandleResponse(rsp *http.Response, proxyCtx *go if rsp == nil || proxyCtx == nil || proxyCtx.Req == nil || !isRedirectStatus(rsp.StatusCode) { return rsp } + // A plaintext response could be forged by anything on the network path, so + // only a redirect received over TLS from the registry is trusted. + if !strings.EqualFold(proxyCtx.Req.URL.Scheme, "https") { + return rsp + } source := strings.TrimSuffix(helpers.GetHost(proxyCtx.Req), ".") if !h.isRedirectSource(source) { return rsp diff --git a/internal/handlers/egress_allowlist_test.go b/internal/handlers/egress_allowlist_test.go index a5540ad..6c57659 100644 --- a/internal/handlers/egress_allowlist_test.go +++ b/internal/handlers/egress_allowlist_test.go @@ -1652,6 +1652,7 @@ func TestEgressAllowlist_RedirectTargetNotLearned(t *testing.T) { location string }{ {"source is not a credential host", "https://unconfigured.example.com/x", http.StatusFound, target}, + {"plaintext source request", "http://packages.example.com/x", http.StatusFound, target}, {"source is a statically allowed credential host", "https://github.com/x", http.StatusFound, target}, {"public registry credential host", "https://registry.npmjs.org/x", http.StatusFound, target}, {"not a redirect status", "https://packages.example.com/x", http.StatusOK, target}, diff --git a/proxy.go b/proxy.go index 2163cf6..9b84782 100644 --- a/proxy.go +++ b/proxy.go @@ -77,7 +77,6 @@ func newProxyWithCacheDir(envSettings config.ProxyEnvSettings, cfg *config.Confi egressAllowlistHandler := handlers.NewEgressAllowlistHandler(cfg, envSettings, metricsClient) proxy.OnRequest().DoFunc(egressAllowlistHandler.HandleRequest) - proxy.OnResponse().DoFunc(egressAllowlistHandler.HandleResponse) enableCache := os.Getenv("PROXY_CACHE") == "true" cacher, err := cache.New(enableCache, cacheDir) @@ -152,6 +151,9 @@ func newProxyWithCacheDir(envSettings config.ProxyEnvSettings, cfg *config.Confi helmRegistryHandler := handlers.NewHelmRegistryHandler(cfg.Credentials, oidcClient) proxy.OnRequest().DoFunc(helmRegistryHandler.HandleRequest) + // Runs after every handler that can replace a response with an + // authentication retry, so a redirect returned by the retry is learned. + proxy.OnResponse().DoFunc(egressAllowlistHandler.HandleResponse) proxy.OnResponse().DoFunc(cacher.OnResponse) return &Proxy{ diff --git a/proxy_test.go b/proxy_test.go index f626ab6..dd0d0fe 100644 --- a/proxy_test.go +++ b/proxy_test.go @@ -390,6 +390,63 @@ func TestProxyEgressAllowlistEnforceAllowsPrivateRegistryRedirectTarget(t *testi assert.Equal(t, http.StatusForbidden, status, "other unlisted hosts stay blocked") } +// The git server handler retries an auth failure with an alternate credential +// and replaces the response. A redirect returned by that retry must still +// allowlist its target. +func TestProxyEgressAllowlistLearnsRedirectFromAuthRetry(t *testing.T) { + t.Setenv("PROXY_CACHE", "false") + const bucketURL = "https://bucket.example.com/archive.tar.gz?X-Amz-Signature=abc" + var rejected atomic.Int32 + upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.Host { + case "git.example.com": + if _, password, _ := r.BasicAuth(); password != "good" { + rejected.Add(1) + w.WriteHeader(http.StatusUnauthorized) + return + } + http.Redirect(w, r, bucketURL, http.StatusFound) + case "bucket.example.com": + _, _ = io.WriteString(w, "archive") + default: + w.WriteHeader(http.StatusTeapot) + } + })) + defer upstream.Close() + + cfg := &config.Config{ + CA: testProxyConfig.CA, + Experiments: config.Experiments{"proxy-egress-enforce": true}, + Credentials: config.Credentials{ + {"type": "git_source", "host": "git.example.com", "username": "x-access-token", "password": "bad"}, + {"type": "git_source", "host": "git.example.com", "username": "x-access-token", "password": "good"}, + }, + } + client, proxy := testProxyServer(t, cfg, nil, upstream.Certificate()) + closeOnCleanup(t, proxy) + t.Cleanup(client.CloseIdleConnections) + proxyHandler := proxy.Handler.(*Proxy) + t.Cleanup(proxyHandler.Tr.CloseIdleConnections) + proxyHandler.Tr.Proxy = nil + proxyHandler.Tr.DialContext = func(ctx context.Context, network, address string) (net.Conn, error) { + return (&net.Dialer{}).DialContext(ctx, network, upstream.Listener.Addr().String()) + } + + req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, "https://git.example.com/org/repo/archive/v1.tar.gz", nil) + require.NoError(t, err) + rsp, err := client.Do(req) + require.NoError(t, err) + defer func() { + require.NoError(t, rsp.Body.Close()) + }() + body, err := io.ReadAll(rsp.Body) + require.NoError(t, err) + + require.Positive(t, rejected.Load(), "the first credential must fail so the retry path is exercised") + assert.Equal(t, http.StatusOK, rsp.StatusCode) + assert.Equal(t, "archive", string(body)) +} + func TestProxyHTTPSMITMFixedLengthResponseFraming(t *testing.T) { upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Length", "5")