diff --git a/internal/handlers/docker_registry.go b/internal/handlers/docker_registry.go index 26cc62b..67d0de1 100644 --- a/internal/handlers/docker_registry.go +++ b/internal/handlers/docker_registry.go @@ -33,9 +33,10 @@ type getECRClient func(ctx context.Context, region, keyID, secretKey string, cli // DockerRegistryHandler handles requests to Docker registries, adding auth. type DockerRegistryHandler struct { - credentials []*dockerRegistryCredentials - transport http.RoundTripper - oidcRegistry *oidc.OIDCRegistry + credentials []*dockerRegistryCredentials + oidcCredentials []*dockerOIDCCredential + transport http.RoundTripper + oidcRegistry *oidc.OIDCRegistry } // NewDockerRegistryHandler returns a new DockerRegistryHandler. @@ -70,6 +71,11 @@ func NewDockerRegistryHandler(creds config.Credentials, client *http.Client, get // OIDC credentials are not used as static credentials. oidcMatchingCred, urlFields := dockerOIDCMatchingCredential(cred, matchField, matchURL) if oidcCred, _, _ := handler.oidcRegistry.Register(oidcMatchingCred, urlFields, "docker registry"); oidcCred != nil { + handler.oidcCredentials = append(handler.oidcCredentials, &dockerOIDCCredential{ + matchURL: matchURL, + path: path, + credential: oidcCred, + }) continue } } @@ -146,7 +152,10 @@ func (h *DockerRegistryHandler) HandleRequest(req *http.Request, proxyCtx *gopro if oidcCredential != nil { return req, nil } - h.authenticateStaticCredential(req, proxyCtx, true) + if h.authenticateStaticCredential(req, proxyCtx, true) { + return req, nil + } + h.authenticateHostFallback(req, proxyCtx) return req, nil } @@ -169,32 +178,86 @@ func (h *DockerRegistryHandler) authenticateStaticCredential( continue } - if cred.getECRCredentials(req.Context(), proxyCtx) { - logging.RequestLogf(proxyCtx, "* authenticating docker ecr request (host: %s)", req.URL.Hostname()) - helpers.SetBasicAuthorization(req, cred.ecrUsername, cred.ecrPassword) - } else { - logging.RequestLogf(proxyCtx, "* authenticating docker registry request (host: %s)", req.URL.Hostname()) - transport := ®istry.BasicTransport{ - Transport: ®istry.TokenTransport{ - Transport: h.transport, - Username: cred.getUsername(), - Password: cred.getPassword(), - }, - URL: (&url.URL{Scheme: req.URL.Scheme, Host: req.URL.Host}).String() + matchedPath, - Username: cred.getUsername(), - Password: cred.getPassword(), - } - proxyCtx.RoundTripper = &dockerRegistryRoundTripper{ - transport: transport, - } - } - + h.authenticateWithCredential(req, proxyCtx, cred, matchedPath) return true } return false } +// authenticateHostFallback authenticates registry API requests that no +// credential path matched with the first explicit credential for the host, as +// the proxy did before credential paths were matched. Configured paths are +// often web context paths, such as Artifactory's /artifactory/, +// that never appear in registry API requests. +// +// The fallback is limited to /v2, so other ecosystems on a shared host keep +// their own credentials outside the registry API. Helm OCI requests also use +// /v2, so a static fallback credential is sent with them instead of a Helm +// credential, as a path-less Docker credential is. +func (h *DockerRegistryHandler) authenticateHostFallback(req *http.Request, proxyCtx *goproxy.ProxyCtx) { + matchedPath, ok := helpers.MatchingPathPrefix(req.URL.EscapedPath(), "/v2") + if !ok { + return + } + + var oidcCredential *dockerOIDCCredential + for _, cred := range h.oidcCredentials { + if helpers.CredentialURLMatchesRequest(req, cred.matchURL, false) { + oidcCredential = cred + break + } + } + var staticCredential *dockerRegistryCredentials + for _, cred := range h.credentials { + if !cred.proxyOnly && helpers.CredentialURLMatchesRequest(req, cred.matchURL, false) { + staticCredential = cred + break + } + } + + if oidcCredential != nil { + logging.RequestLogf(proxyCtx, "* no docker registry credential path matched, trying host fallback OIDC credential (host: %s, credential path: %s)", + req.URL.Hostname(), oidcCredential.path) + if h.oidcRegistry.TryAuthCredential(req, proxyCtx, oidcCredential.credential) { + return + } + } + if staticCredential != nil { + logging.RequestLogf(proxyCtx, "* no docker registry credential path matched, using host fallback static credential (host: %s, credential path: %s)", + req.URL.Hostname(), staticCredential.path) + h.authenticateWithCredential(req, proxyCtx, staticCredential, matchedPath) + } +} + +func (h *DockerRegistryHandler) authenticateWithCredential( + req *http.Request, + proxyCtx *goproxy.ProxyCtx, + cred *dockerRegistryCredentials, + matchedPath string, +) { + if cred.getECRCredentials(req.Context(), proxyCtx) { + logging.RequestLogf(proxyCtx, "* authenticating docker ecr request (host: %s)", req.URL.Hostname()) + helpers.SetBasicAuthorization(req, cred.ecrUsername, cred.ecrPassword) + return + } + + logging.RequestLogf(proxyCtx, "* authenticating docker registry request (host: %s)", req.URL.Hostname()) + transport := ®istry.BasicTransport{ + Transport: ®istry.TokenTransport{ + Transport: h.transport, + Username: cred.getUsername(), + Password: cred.getPassword(), + }, + URL: (&url.URL{Scheme: req.URL.Scheme, Host: req.URL.Host}).String() + matchedPath, + Username: cred.getUsername(), + Password: cred.getPassword(), + } + proxyCtx.RoundTripper = &dockerRegistryRoundTripper{ + transport: transport, + } +} + func dockerCredentialMatchesRequest( req *http.Request, cred *dockerRegistryCredentials, @@ -346,6 +409,12 @@ func defaultGetECRClient(ctx context.Context, region, keyID, secretKey string, c return ecr.NewFromConfig(cfg), nil } +type dockerOIDCCredential struct { + matchURL string + path string + credential *oidc.OIDCCredential +} + type dockerRegistryCredentials struct { matchURL string registry string diff --git a/internal/handlers/docker_registry_test.go b/internal/handlers/docker_registry_test.go index 169d7ca..0efabe2 100644 --- a/internal/handlers/docker_registry_test.go +++ b/internal/handlers/docker_registry_test.go @@ -1,6 +1,7 @@ package handlers import ( + "bytes" "context" "encoding/base64" "io" @@ -18,6 +19,7 @@ import ( "github.com/stretchr/testify/require" "github.com/dependabot/proxy/internal/config" + "github.com/dependabot/proxy/internal/testhelpers" ) type ecrContextKey struct{} @@ -100,14 +102,22 @@ func TestDockerRegistryHandler(t *testing.T) { for _, requestURL := range []string{ "https://example.com:443/v2/dependabot/core-attacker/manifests/latest", "https://example.com/v2/dependabot%2Fcore/manifests/latest", - "https://example.com/v2/dependabot/core/%2e%2e%2fattacker/manifests/latest", } { req = httptest.NewRequestWithContext(t.Context(), "GET", requestURL, nil) proxyCtx = &goproxy.ProxyCtx{} _ = handleRequestAndClose(handler, req, proxyCtx) - assert.Nil(t, proxyCtx.RoundTripper, "URL credential path scope") + rt, ok = proxyCtx.RoundTripper.(*dockerRegistryRoundTripper) + require.True(t, ok, "other repositories on the host use the host fallback") + trans = rt.transport.(*registry.BasicTransport) + assert.Equal(t, hubUser, trans.Username, "correct username is set") + assert.Equal(t, "https://"+req.URL.Host+"/v2", trans.URL, "host fallback is not scoped to the configured repository") } + req = httptest.NewRequestWithContext(t.Context(), "GET", "https://example.com/v2/dependabot/core/%2e%2e%2fattacker/manifests/latest", nil) + proxyCtx = &goproxy.ProxyCtx{} + _ = handleRequestAndClose(handler, req, proxyCtx) + assert.Nil(t, proxyCtx.RoundTripper, "ambiguous dot segments are not authenticated") + // Different private registry req = httptest.NewRequestWithContext(t.Context(), "GET", "https://docker.bigco.com/v2/their-repo/manifests/latest", nil) proxyCtx = &goproxy.ProxyCtx{} @@ -404,6 +414,204 @@ func TestDockerRegistryHandlerURLCredentialUsesMatchedRequestOrigin(t *testing.T assert.Equal(t, "https://example.com/v2/dependabot/core", transport.URL) } +// Registries such as Artifactory are commonly configured with a web context +// path, like /artifactory/, that never appears in Docker +// Registry API requests. +func TestDockerRegistryHandlerPathScopedCredentialFallsBackToHost(t *testing.T) { + client := &http.Client{Transport: &recordingECRTransport{}, Timeout: testOIDCClient.Timeout} + handler := NewDockerRegistryHandler(config.Credentials{ + // Org-level credentials list anonymous registries before those with a + // secret. The host fallback must skip them. + { + "type": "docker_registry", + "url": "https://artifactory.example.com/artifactory/docker-public", + }, + { + "type": "docker_registry", + "url": "https://artifactory.example.com/artifactory/docker-remote", + "username": "remote-user", + "password": "remote-password", + }, + { + "type": "docker_registry", + "url": "https://artifactory.example.com/artifactory/docker-local", + "username": "local-user", + "password": "local-password", + }, + }, client, nil) + + tests := []struct { + name string + requestURL string + username string + transportURL string + }{ + { + name: "repository path", + requestURL: "https://artifactory.example.com/v2/docker-remote/library/alpine/tags/list", + username: "remote-user", + transportURL: "https://artifactory.example.com/v2", + }, + { + name: "matching configured path takes precedence", + requestURL: "https://artifactory.example.com/v2/artifactory/docker-local/app/manifests/latest", + username: "local-user", + transportURL: "https://artifactory.example.com/v2/artifactory/docker-local", + }, + { + name: "outside the registry API", + requestURL: "https://artifactory.example.com/artifactory/api/npm/npm-remote/lodash", + }, + { + name: "registry API prefix confusion", + requestURL: "https://artifactory.example.com/v2-attacker/app/manifests/latest", + }, + { + name: "different port", + requestURL: "https://artifactory.example.com:8443/v2/docker-remote/app/manifests/latest", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, test.requestURL, nil) + proxyCtx := &goproxy.ProxyCtx{} + req = handleRequestAndClose(handler, req, proxyCtx) + + if test.username == "" { + assert.Nil(t, proxyCtx.RoundTripper, "no docker registry transport") + assertUnauthenticated(t, req, "no docker registry credential applies") + return + } + rt, ok := proxyCtx.RoundTripper.(*dockerRegistryRoundTripper) + require.True(t, ok, "request is assigned a docker registry transport") + transport := rt.transport.(*registry.BasicTransport) + assert.Equal(t, test.username, transport.Username) + assert.Equal(t, test.transportURL, transport.URL) + }) + } +} + +func TestDockerRegistryHandlerPathScopedOIDCCredentialFallsBackToHost(t *testing.T) { + const tokenURL = "https://token.actions.example.com" //nolint:gosec // test URL + const jfrogTokenURL = "https://jfrog.example.com/access/api/v1/oidc/token" //nolint:gosec // test URL + t.Setenv("ACTIONS_ID_TOKEN_REQUEST_URL", tokenURL) + t.Setenv("ACTIONS_ID_TOKEN_REQUEST_TOKEN", "request-token") + + httpmock.Activate() + defer httpmock.DeactivateAndReset() + httpmock.RegisterResponder(http.MethodGet, tokenURL, + httpmock.NewStringResponder(http.StatusOK, `{"count":1,"value":"github-token"}`)) + + newHandler := func() *DockerRegistryHandler { + return NewDockerRegistryHandler(config.Credentials{ + { + "type": "docker_registry", + "url": "https://jfrog.example.com/artifactory/docker-local", + "username": "static-user", + "password": "static-password", + }, + { + "type": "docker_registry", + "registry": "jfrog.example.com", + "url": "https://jfrog.example.com/artifactory/docker-remote", + "jfrog-oidc-provider-name": "proxy-test", + }, + }, testOIDCClient, nil) + } + + t.Run("OIDC credential is preferred", func(t *testing.T) { + httpmock.RegisterResponder(http.MethodPost, jfrogTokenURL, + httpmock.NewStringResponder(http.StatusOK, `{"access_token":"jfrog-token","expires_in":3600}`)) + handler := newHandler() + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, + "https://jfrog.example.com/v2/docker-remote/library/alpine/tags/list", nil) + proxyCtx := &goproxy.ProxyCtx{} + req = handleRequestAndClose(handler, req, proxyCtx) + assertHasTokenAuth(t, req, "Bearer", "jfrog-token", "OIDC host fallback") + assert.Nil(t, proxyCtx.RoundTripper, "OIDC host fallback") + + for _, requestURL := range []string{ + "https://other.example.com/v2/docker-remote/library/alpine/tags/list", + "https://jfrog.example.com:8443/v2/docker-remote/library/alpine/tags/list", + } { + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, requestURL, nil) + proxyCtx := &goproxy.ProxyCtx{} + req = handleRequestAndClose(handler, req, proxyCtx) + assertUnauthenticated(t, req, "host fallback is limited to the credential origin: "+requestURL) + assert.Nil(t, proxyCtx.RoundTripper, "host fallback is limited to the credential origin: "+requestURL) + } + }) + + t.Run("failed OIDC credential", func(t *testing.T) { + var logs bytes.Buffer + testhelpers.CaptureStandardLog(t, &logs) + httpmock.RegisterResponder(http.MethodPost, jfrogTokenURL, + httpmock.NewStringResponder(http.StatusInternalServerError, "failed")) + handler := newHandler() + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, + "https://jfrog.example.com/v2/docker-remote/library/alpine/tags/list", nil) + proxyCtx := &goproxy.ProxyCtx{} + req = handleRequestAndClose(handler, req, proxyCtx) + assertUnauthenticated(t, req, "failed OIDC token is not set") + rt, ok := proxyCtx.RoundTripper.(*dockerRegistryRoundTripper) + require.True(t, ok, "static host fallback configures challenge transport") + transport := rt.transport.(*registry.BasicTransport) + assert.Equal(t, "static-user", transport.Username) + assert.Equal(t, "https://jfrog.example.com/v2", transport.URL) + assert.Contains(t, logs.String(), "trying host fallback OIDC credential (host: jfrog.example.com, credential path: /v2/artifactory/docker-remote)") + assert.Contains(t, logs.String(), "using host fallback static credential (host: jfrog.example.com, credential path: /v2/artifactory/docker-local)") + + // An OIDC credential that matched the request path fails closed. + req = httptest.NewRequestWithContext(t.Context(), http.MethodGet, + "https://jfrog.example.com/v2/artifactory/docker-remote/app/manifests/latest", nil) + proxyCtx = &goproxy.ProxyCtx{} + req = handleRequestAndClose(handler, req, proxyCtx) + assertUnauthenticated(t, req, "failed path-matched OIDC credential") + assert.Nil(t, proxyCtx.RoundTripper, "failed path-matched OIDC credential does not use the host fallback") + }) +} + +// Helm OCI charts are pulled through the same /v2 registry API as Docker +// images, so on a host with both credentials the Docker host fallback +// credential is sent with them, as a path-less Docker credential is. +func TestDockerRegistryHandlerHostFallbackOnHostSharedWithHelm(t *testing.T) { + creds := config.Credentials{ + { + "type": "docker_registry", + "url": "https://artifactory.example.com/artifactory/docker-remote", + "username": "docker-user", + "password": "docker-password", + }, + { + "type": "helm_registry", + "registry": "artifactory.example.com", + "username": "helm-user", + "password": "helm-password", + }, + } + transport := &recordingECRTransport{} + client := &http.Client{Transport: transport, Timeout: testOIDCClient.Timeout} + + // The proxy runs request handlers in registration order, then sends the + // request through the proxy context's RoundTripper. + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, + "https://artifactory.example.com/v2/helm-oci-local/mychart/manifests/1.0.0", nil) + proxyCtx := &goproxy.ProxyCtx{} + req = handleRequestAndClose(NewDockerRegistryHandler(creds, client, nil), req, proxyCtx) + req = handleRequestAndClose(NewHelmRegistryHandler(creds, client), req, proxyCtx) + assertHasBasicAuth(t, req, "helm-user", "helm-password", "Helm handler sets its credential") + + require.NotNil(t, proxyCtx.RoundTripper, "Docker host fallback configures the registry transport") + resp, err := proxyCtx.RoundTripper.RoundTrip(req, proxyCtx) + require.NoError(t, err) + require.NoError(t, resp.Body.Close()) + require.NotNil(t, transport.request) + assertHasBasicAuth(t, transport.request, "docker-user", "docker-password", "Docker credential is sent") +} + func TestDockerRegistryHandlerRejectsConflictingCredentialLocations(t *testing.T) { tests := []struct { name string diff --git a/proxy_test.go b/proxy_test.go index e9635c9..d3d1cec 100644 --- a/proxy_test.go +++ b/proxy_test.go @@ -195,6 +195,78 @@ func TestProxyDependabotAPIHost(t *testing.T) { } } +// An org-level Docker registry credential can carry a web context path, like +// Artifactory's /artifactory/, that Docker Registry API +// requests never contain. The proxy must still answer the registry's challenge +// with that credential. +func TestProxyDockerRegistryCredentialWithWebContextPath(t *testing.T) { + t.Setenv("PROXY_CACHE", "false") + cfg := &config.Config{ + CA: testProxyConfig.CA, + Credentials: config.Credentials{{ + "type": "docker_registry", + "url": "https://artifactory.example.com/artifactory/docker-remote", + "username": "docker-user", + "password": "docker-password", + }}, + } + + tests := []struct { + name string + challenge string + acceptsBasic bool + }{ + { + name: "token challenge", + challenge: `Bearer realm="https://artifactory.example.com/v2/token",service="artifactory.example.com",scope="repository:team/app:pull"`, + }, + { + name: "basic challenge", + challenge: `Basic realm="Artifactory Realm"`, + acceptsBasic: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + username, password, _ := r.BasicAuth() + hasCredential := username == "docker-user" && password == "docker-password" + switch { + case r.URL.Path == "/v2/token" && hasCredential: + _, _ = io.WriteString(w, `{"token":"registry-token"}`) + case r.Header.Get("Authorization") == "Bearer registry-token", tt.acceptsBasic && hasCredential: + w.WriteHeader(http.StatusOK) + default: + w.Header().Set("WWW-Authenticate", tt.challenge) + w.WriteHeader(http.StatusUnauthorized) + } + })) + defer upstream.Close() + + 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://artifactory.example.com/v2/docker-remote/team/app/manifests/latest", nil) + require.NoError(t, err) + resp, err := client.Do(req) + require.NoError(t, err) + defer func() { + require.NoError(t, resp.Body.Close()) + }() + + assert.Equal(t, http.StatusOK, resp.StatusCode) + }) + } +} + func TestProxyEgressAllowlistEnforceBlocks(t *testing.T) { upstream := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.WriteHeader(http.StatusOK)