From 101d571ca8628ee9e2379565f08f96d8db1e9818 Mon Sep 17 00:00:00 2001 From: Jeff Widman Date: Sat, 15 Aug 2026 17:19:37 +0000 Subject: [PATCH] Reject ambiguous NuGet credential routes --- internal/handlers/nuget_feed.go | 108 +++++++++++++++++++++--- internal/handlers/nuget_feed_test.go | 117 ++++++++++++++++++++++++++ internal/oidc/oidc_credential.go | 7 ++ internal/oidc/oidc_credential_test.go | 10 +++ 4 files changed, 231 insertions(+), 11 deletions(-) diff --git a/internal/handlers/nuget_feed.go b/internal/handlers/nuget_feed.go index 400280b..9f827e4 100644 --- a/internal/handlers/nuget_feed.go +++ b/internal/handlers/nuget_feed.go @@ -7,6 +7,7 @@ import ( "io" "net/http" "net/url" + "slices" "strings" "sync" @@ -37,13 +38,15 @@ type nugetV3IndexResponse struct { // NugetFeedHandler handles requests to nuget feeds, adding auth. type NugetFeedHandler struct { - credentials []nugetFeedCredentials - credentialURLs map[string]struct{} - credentialsMutex sync.RWMutex - discoverySources []nugetDiscoveryAuth - discoverySourceURLs map[string]struct{} - discoveryMutex sync.RWMutex - oidcRegistry *oidc.OIDCRegistry + credentials []nugetFeedCredentials + credentialURLs map[string]struct{} + credentialsMutex sync.RWMutex + credentialClaims map[string]*nugetCredentialClaim + credentialClaimsMutex sync.RWMutex + discoverySources []nugetDiscoveryAuth + discoverySourceURLs map[string]struct{} + discoveryMutex sync.RWMutex + oidcRegistry *oidc.OIDCRegistry } type nugetFeedCredentials struct { @@ -62,11 +65,19 @@ type nugetDiscoveryAuth struct { authenticateURL bool } +type nugetCredentialClaim struct { + url string + auth nugetDiscoveryAuth + sourceURLs []string + conflicting bool +} + // NewNugetFeedHandler returns a new NugetFeedHandler. func NewNugetFeedHandler(creds config.Credentials) *NugetFeedHandler { handler := NugetFeedHandler{ credentials: []nugetFeedCredentials{}, credentialURLs: make(map[string]struct{}), + credentialClaims: make(map[string]*nugetCredentialClaim), discoverySourceURLs: make(map[string]struct{}), oidcRegistry: oidc.NewOIDCRegistry(), } @@ -86,11 +97,13 @@ func NewNugetFeedHandler(creds config.Credentials) *NugetFeedHandler { oidcCredential, _, ok := handler.oidcRegistry.Register(cred, []string{"url"}, "nuget feed") if ok { if url != "" { - handler.addDiscoverySource(nugetDiscoveryAuth{ + source := nugetDiscoveryAuth{ serviceIndexURL: url, oidc: oidcCredential, authenticateURL: true, - }) + } + handler.addDiscoverySource(source) + handler.claimCredentialRoute(url, source, nil) } continue } @@ -108,12 +121,14 @@ func NewNugetFeedHandler(creds config.Credentials) *NugetFeedHandler { } handler.addStaticCredential(feedCred) if url != "" && (token != "" || password != "") { - handler.addDiscoverySource(nugetDiscoveryAuth{ + source := nugetDiscoveryAuth{ serviceIndexURL: url, static: feedCred, hasStatic: true, authenticateURL: true, - }) + } + handler.addDiscoverySource(source) + handler.claimCredentialRoute(url, source, nil) } } @@ -221,6 +236,13 @@ func (h *NugetFeedHandler) HandleRequest(req *http.Request, proxyCtx *goproxy.Pr if (req.URL.Scheme != "http" && req.URL.Scheme != "https") || !helpers.MethodPermitted(req, "GET", "HEAD") { return req, nil } + // A first claim may remain in the static or OIDC registry after a later + // claim makes the route ambiguous. Check conflicts before every credential + // lookup so no registered NuGet credential can be used for that route. + if h.requestMatchesConflictingRoute(req) { + logging.RequestLogf(proxyCtx, "* refusing to inject credentials for request matching an ambiguous NuGet route: %s", req.URL.String()) + return req, nil + } // Try OIDC credentials first (HTTPS only to avoid leaking tokens over plaintext) if req.URL.Scheme == "https" { @@ -294,6 +316,9 @@ func (h *NugetFeedHandler) HandleResponse(resp *http.Response, proxyCtx *goproxy } for _, discoveredURL := range extraUrlsFromSourceResponse(body, discoveryAuth.serviceIndexURL) { + if !h.claimCredentialRoute(discoveredURL, discoveryAuth, proxyCtx) { + continue + } if discoveryAuth.oidc != nil { h.oidcRegistry.RegisterURL(discoveredURL, discoveryAuth.oidc, "nuget resource") continue @@ -342,6 +367,10 @@ func (h *NugetFeedHandler) registerServiceIndexRedirect(resp *http.Response, sou } if redirectedSource.authenticateURL { + if !h.claimCredentialRoute(redirectURL.String(), redirectedSource, proxyCtx) { + logging.RequestLogf(proxyCtx, " registered nuget service-index redirect: %s", redirectURL.String()) + return + } if source.oidc != nil { h.oidcRegistry.RegisterURL(redirectURL.String(), source.oidc, "nuget service-index redirect") } @@ -384,6 +413,63 @@ func (h *NugetFeedHandler) addDiscoverySource(source nugetDiscoveryAuth) bool { return true } +func (h *NugetFeedHandler) claimCredentialRoute(rawURL string, auth nugetDiscoveryAuth, proxyCtx *goproxy.ProxyCtx) bool { + key := nugetCredentialURLKey(rawURL) + h.credentialClaimsMutex.Lock() + defer h.credentialClaimsMutex.Unlock() + + claim, ok := h.credentialClaims[key] + if !ok { + h.credentialClaims[key] = &nugetCredentialClaim{ + url: rawURL, + auth: auth, + sourceURLs: []string{auth.serviceIndexURL}, + } + return true + } + + if !slices.Contains(claim.sourceURLs, auth.serviceIndexURL) { + claim.sourceURLs = append(claim.sourceURLs, auth.serviceIndexURL) + } + if equivalentNugetDiscoveryAuth(claim.auth, auth) { + return false + } + + claim.conflicting = true + logging.RequestLogf(proxyCtx, + "conflicting NuGet credentials for route %s claimed by service indexes %s; NuGet credentials will not be injected for matching requests", + claim.url, strings.Join(claim.sourceURLs, ", ")) + return false +} + +func (h *NugetFeedHandler) requestMatchesConflictingRoute(req *http.Request) bool { + h.credentialClaimsMutex.RLock() + defer h.credentialClaimsMutex.RUnlock() + for _, claim := range h.credentialClaims { + if claim.conflicting && helpers.UrlMatchesRequest(req, claim.url, true) { + return true + } + } + return false +} + +func equivalentNugetDiscoveryAuth(first, second nugetDiscoveryAuth) bool { + if first.hasStatic || second.hasStatic { + return first.hasStatic && second.hasStatic && staticNugetCredentialValue(first.static) == staticNugetCredentialValue(second.static) + } + return first.oidc.Equivalent(second.oidc) +} + +func staticNugetCredentialValue(credential nugetFeedCredentials) string { + if credential.token != "" { + return credential.token + } + if credential.password != "" { + return credential.username + ":" + credential.password + } + return "" +} + func markNugetDiscovery(proxyCtx *goproxy.ProxyCtx, auth nugetDiscoveryAuth) { if proxyCtx != nil { proxyctx.SetValue(proxyCtx, nugetDiscoveryCtxKey, auth) diff --git a/internal/handlers/nuget_feed_test.go b/internal/handlers/nuget_feed_test.go index 4e1b934..28b57c8 100644 --- a/internal/handlers/nuget_feed_test.go +++ b/internal/handlers/nuget_feed_test.go @@ -492,6 +492,123 @@ func TestNugetFeedHandlerConcurrentDiscoveryIsDeduplicated(t *testing.T) { assert.Len(t, handler.credentials, 2) } +func TestNugetFeedHandlerAllowsSharedRouteWithSameCredential(t *testing.T) { + handler := NewNugetFeedHandler(config.Credentials{ + config.Credential{"type": "nuget_feed", "url": "https://first.example/index.json", "token": "user:password"}, + config.Credential{"type": "nuget_feed", "url": "https://second.example/index.json", "username": "user", "password": "password"}, + }) + response := `{"version":"3.0.0","resources":[{"@id":"https://cdn.example.com/packages","@type":"PackageBaseAddress/3.0.0"}]}` + discoverNugetFeed(t, handler, "https://first.example/index.json", http.StatusOK, response) + discoverNugetFeed(t, handler, "https://second.example/index.json", http.StatusOK, response) + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "https://cdn.example.com/packages/example/index.json", nil) + req = handleRequestAndClose(handler, req, &goproxy.ProxyCtx{}) + assertHasBasicAuth(t, req, "user", "password", "shared route with same credential") +} + +func TestNugetFeedHandlerRejectsSharedRouteWithDifferentCredentials(t *testing.T) { + for _, order := range [][]string{ + {"https://first.example/index.json", "https://second.example/index.json"}, + {"https://second.example/index.json", "https://first.example/index.json"}, + } { + handler := NewNugetFeedHandler(config.Credentials{ + config.Credential{"type": "nuget_feed", "url": "https://first.example/index.json", "token": "first-token"}, + config.Credential{"type": "nuget_feed", "url": "https://second.example/index.json", "token": "second-token"}, + }) + response := `{"version":"3.0.0","resources":[{"@id":"https://cdn.example.com/packages","@type":"PackageBaseAddress/3.0.0"}]}` + var buf bytes.Buffer + log.SetOutput(&buf) + for _, sourceURL := range order { + discoverNugetFeed(t, handler, sourceURL, http.StatusOK, response) + } + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "https://cdn.example.com/packages/example/index.json", nil) + req = handleRequestAndClose(handler, req, &goproxy.ProxyCtx{}) + assertUnauthenticated(t, req, "shared route with different credentials") + assert.Contains(t, buf.String(), "conflicting NuGet credentials for route https://cdn.example.com/packages") + assert.Contains(t, buf.String(), "https://first.example/index.json") + assert.Contains(t, buf.String(), "https://second.example/index.json") + } +} + +func TestNugetFeedHandlerRejectsOIDCAndMixedCredentialConflicts(t *testing.T) { + t.Setenv("ACTIONS_ID_TOKEN_REQUEST_URL", "https://token.actions.example.com") + t.Setenv("ACTIONS_ID_TOKEN_REQUEST_TOKEN", "test-token") + oidcCredential := config.Credential{ + "type": "nuget_feed", "url": "https://first.example/index.json", + "tenant-id": "first-tenant", "client-id": "first-client", + } + testCases := []struct { + name string + credential config.Credential + conflicting bool + }{ + { + name: "equivalent OIDC credential", + credential: config.Credential{ + "type": "nuget_feed", "url": "https://second.example/index.json", + "tenant-id": "first-tenant", "client-id": "first-client", + }, + conflicting: false, + }, + { + name: "different OIDC credential", + credential: config.Credential{ + "type": "nuget_feed", "url": "https://second.example/index.json", + "tenant-id": "second-tenant", "client-id": "second-client", + }, + conflicting: true, + }, + { + name: "static credential", + credential: config.Credential{"type": "nuget_feed", "url": "https://second.example/index.json", "token": "static-token"}, + conflicting: true, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + handler := NewNugetFeedHandler(config.Credentials{oidcCredential, testCase.credential}) + responseBody := `{"version":"3.0.0","resources":[{"@id":"https://cdn.example.com/packages","@type":"PackageBaseAddress/3.0.0"}]}` + for _, sourceURL := range []string{"https://first.example/index.json", "https://second.example/index.json"} { + proxyCtx := &goproxy.ProxyCtx{} + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, sourceURL, nil) + handler.PrepareRequest(req, proxyCtx) + handler.HandleResponse(&http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(responseBody)), + }, proxyCtx) + } + + req := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "https://cdn.example.com/packages/example/index.json", nil) + assert.Equal(t, testCase.conflicting, handler.requestMatchesConflictingRoute(req)) + if testCase.conflicting { + req = handleRequestAndClose(handler, req, &goproxy.ProxyCtx{}) + assertUnauthenticated(t, req, "ambiguous OIDC route") + } + }) + } +} + +func TestNugetFeedHandlerKeepsDistinctMirrorRoutesIndependent(t *testing.T) { + handler := NewNugetFeedHandler(config.Credentials{ + config.Credential{"type": "nuget_feed", "url": "https://first.example/index.json", "token": "first-token"}, + config.Credential{"type": "nuget_feed", "url": "https://mirror.example/index.json", "token": "mirror-token"}, + }) + discoverNugetFeed(t, handler, "https://first.example/index.json", http.StatusOK, + `{"version":"3.0.0","resources":[{"@id":"https://first.example/packages","@type":"PackageBaseAddress/3.0.0"}]}`) + discoverNugetFeed(t, handler, "https://mirror.example/index.json", http.StatusOK, + `{"version":"3.0.0","resources":[{"@id":"https://mirror.example/packages","@type":"PackageBaseAddress/3.0.0"}]}`) + + firstReq := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "https://first.example/packages/example/index.json", nil) + firstReq = handleRequestAndClose(handler, firstReq, &goproxy.ProxyCtx{}) + assertHasTokenAuth(t, firstReq, "Bearer", "first-token", "first mirror route") + + mirrorReq := httptest.NewRequestWithContext(t.Context(), http.MethodGet, "https://mirror.example/packages/example/index.json", nil) + mirrorReq = handleRequestAndClose(handler, mirrorReq, &goproxy.ProxyCtx{}) + assertHasTokenAuth(t, mirrorReq, "Bearer", "mirror-token", "second mirror route") +} + func TestNugetFeedHandlerPrefersMostSpecificURLCredential(t *testing.T) { handler := NewNugetFeedHandler(config.Credentials{ config.Credential{ diff --git a/internal/oidc/oidc_credential.go b/internal/oidc/oidc_credential.go index d3d591a..a03d9ce 100644 --- a/internal/oidc/oidc_credential.go +++ b/internal/oidc/oidc_credential.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "net/url" + "reflect" "sync" "time" @@ -80,6 +81,12 @@ func (c *OIDCCredential) Provider() string { return c.parameters.Name() } +// Equivalent reports whether two credentials request tokens with the same +// provider parameters. Cached token state does not affect equivalence. +func (c *OIDCCredential) Equivalent(other *OIDCCredential) bool { + return c != nil && other != nil && reflect.DeepEqual(c.parameters, other.parameters) +} + func CreateOIDCCredential(cred config.Credential) (*OIDCCredential, error) { if !IsOIDCConfigured() { return nil, fmt.Errorf("OIDC is not configured") diff --git a/internal/oidc/oidc_credential_test.go b/internal/oidc/oidc_credential_test.go index b133c2a..9fabf32 100644 --- a/internal/oidc/oidc_credential_test.go +++ b/internal/oidc/oidc_credential_test.go @@ -18,6 +18,16 @@ import ( "github.com/dependabot/proxy/internal/config" ) +func TestOIDCCredentialEquivalent(t *testing.T) { + first := &OIDCCredential{parameters: &AzureOIDCParameters{TenantID: "tenant", ClientID: "client"}} + same := &OIDCCredential{parameters: &AzureOIDCParameters{TenantID: "tenant", ClientID: "client"}} + different := &OIDCCredential{parameters: &AzureOIDCParameters{TenantID: "tenant", ClientID: "other"}} + + assert.True(t, first.Equivalent(same)) + assert.False(t, first.Equivalent(different)) + assert.False(t, first.Equivalent(nil)) +} + func TestSuccessfulAuthenticationDoesNotMakeARepeatedRequest(t *testing.T) { // these variables are necessary os.Setenv(envActionsIDTokenRequestURL, "https://example.com/token")