Allowlist registry redirect targets for private registries

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,84 @@ 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
+	}
+	// 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
+	}
+	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 +235,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

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,70 @@ 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},
+		{"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},
+		{"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"))
+		})
+	}
+}

proxy.go

@@ -151,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{

proxy_test.go

@@ -327,6 +327,126 @@ 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")
+}
+
+// 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")