From c5bf4d115040080d7acc44c8c8b79f98c939dc7b Mon Sep 17 00:00:00 2001 From: Kamil Bukum Date: Thu, 2 Apr 2026 18:41:52 -0500 Subject: [PATCH 1/8] Add OIDCRegistry type for collision-free OIDC credential storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce OIDCRegistry in the oidc package as the foundation for fixing OIDC credential collisions when multiple same-ecosystem registries share a host (dependabot-updates#13014). Key design decisions: - Store credentials in a map[hostname][]entry (not map[hostname]*cred) so two registries on the same host with different paths coexist - Parse URLs at registration time, storing host/path/port separately - Match requests via O(1) host lookup + path prefix check - Provide Register(), RegisterURL(), and TryAuth() as the handler API This is Phase 1 (additive only) — no handler changes yet. Existing code and tests are unaffected. --- internal/oidc/oidc_registry.go | 181 +++++++++++++++++ internal/oidc/oidc_registry_test.go | 299 ++++++++++++++++++++++++++++ 2 files changed, 480 insertions(+) create mode 100644 internal/oidc/oidc_registry.go create mode 100644 internal/oidc/oidc_registry_test.go diff --git a/internal/oidc/oidc_registry.go b/internal/oidc/oidc_registry.go new file mode 100644 index 0000000..4387fb0 --- /dev/null +++ b/internal/oidc/oidc_registry.go @@ -0,0 +1,181 @@ +package oidc + +import ( + "fmt" + "net/http" + "strings" + "sync" + + "github.com/elazarl/goproxy" + + "github.com/dependabot/proxy/internal/config" + "github.com/dependabot/proxy/internal/helpers" + "github.com/dependabot/proxy/internal/logging" +) + +// OIDCRegistry stores OIDC credentials indexed by host, with path-based +// matching within each host bucket. This structure provides O(1) host lookup +// and avoids key collisions when multiple registries share a host with +// different paths. +type OIDCRegistry struct { + byHost map[string][]oidcEntry + mutex sync.RWMutex +} + +type oidcEntry struct { + path string // URL path prefix, e.g. "/org/_packaging/feed-A/npm/registry" + port string // port, defaults to "443" + credential *OIDCCredential +} + +// NewOIDCRegistry creates an empty registry. +func NewOIDCRegistry() *OIDCRegistry { + return &OIDCRegistry{ + byHost: make(map[string][]oidcEntry), + } +} + +// Register attempts to create an OIDC credential from the config and store it. +// urlFields are checked in order for a URL (preserving host + path); +// falls back to cred.Host() (hostname only) as last resort. +// +// Returns: +// - (credential, key, true) if an OIDC credential was created and registered +// - (nil, "", false) if the credential is not OIDC-configured +func (r *OIDCRegistry) Register( + cred config.Credential, + urlFields []string, + registryType string, +) (*OIDCCredential, string, bool) { + oidcCredential, _ := CreateOIDCCredential(cred) + if oidcCredential == nil { + return nil, "", false + } + + // Resolve the key: prefer URL fields (preserves path), fall back to host + var key string + for _, field := range urlFields { + if v := cred.GetString(field); v != "" { + key = v + break + } + } + if key == "" { + key = cred.Host() + } + if key == "" { + return oidcCredential, "", false + } + + r.addEntry(key, oidcCredential) + logging.RequestLogf(nil, "registered %s OIDC credentials for %s: %s", oidcCredential.Provider(), registryType, key) + + return oidcCredential, key, true +} + +// RegisterURL adds an already-created credential under a URL. +// Used by nuget to register HTTP-discovered resource URLs that +// should share the same OIDC credential as the primary feed URL. +func (r *OIDCRegistry) RegisterURL(url string, cred *OIDCCredential, registryType string) { + if url == "" || cred == nil { + return + } + r.addEntry(url, cred) + logging.RequestLogf(nil, "registered %s OIDC credentials for %s: %s", cred.Provider(), registryType, url) +} + +// TryAuth finds the matching OIDC credential for the request and +// sets the appropriate auth header. +// +// Lookup: +// 1. Find the host bucket via map lookup (exact hostname match) +// 2. Within that bucket, find the entry whose stored path is a +// prefix of the request path +// +// Returns true if the request was authenticated, false otherwise. +func (r *OIDCRegistry) TryAuth(req *http.Request, ctx *goproxy.ProxyCtx) bool { + host := helpers.GetHost(req) + reqPort := req.URL.Port() + if reqPort == "" { + reqPort = "443" + } + + r.mutex.RLock() + entries := r.byHost[host] + r.mutex.RUnlock() + + if len(entries) == 0 { + return false + } + + // Find the matching entry: host is already matched, check port + path prefix + var matched *OIDCCredential + for i := range entries { + e := &entries[i] + if e.port != reqPort { + continue + } + if strings.HasPrefix(req.URL.Path, e.path) { + matched = e.credential + break + } + } + + if matched == nil { + return false + } + + token, err := GetOrRefreshOIDCToken(matched, req.Context()) + if err != nil { + logging.RequestLogf(ctx, "* failed to get %s token via OIDC for %s: %v", matched.Provider(), host, err) + return false + } + + switch matched.parameters.(type) { + case *CloudsmithOIDCParameters: + logging.RequestLogf(ctx, "* authenticating request with OIDC API key (host: %s)", host) + req.Header.Set("X-Api-Key", token) + default: + logging.RequestLogf(ctx, "* authenticating request with OIDC token (host: %s)", host) + req.Header.Set("Authorization", fmt.Sprintf("Bearer %s", token)) + } + + return true +} + +// addEntry parses a URL or hostname string and adds a credential entry +// to the appropriate host bucket. +func (r *OIDCRegistry) addEntry(urlOrHost string, cred *OIDCCredential) { + host, path, port := parseRegistryURL(urlOrHost) + if host == "" { + return + } + + entry := oidcEntry{ + path: path, + port: port, + credential: cred, + } + + r.mutex.Lock() + r.byHost[host] = append(r.byHost[host], entry) + r.mutex.Unlock() +} + +// parseRegistryURL extracts host, path, and port from a URL or hostname string. +// For hostname-only input, path is empty and port defaults to "443". +func parseRegistryURL(urlOrHost string) (host, path, port string) { + parsed, err := helpers.ParseURLLax(urlOrHost) + if err != nil { + return "", "", "" + } + + host = strings.ToLower(parsed.Hostname()) + path = strings.TrimRight(parsed.Path, "/") + port = parsed.Port() + if port == "" { + port = "443" + } + + return host, path, port +} diff --git a/internal/oidc/oidc_registry_test.go b/internal/oidc/oidc_registry_test.go new file mode 100644 index 0000000..3667ead --- /dev/null +++ b/internal/oidc/oidc_registry_test.go @@ -0,0 +1,299 @@ +package oidc + +import ( + "net/http/httptest" + "os" + "testing" + + "github.com/jarcoal/httpmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/dependabot/proxy/internal/config" +) + +func setupOIDCEnv(t *testing.T) { + t.Helper() + t.Setenv(envActionsIDTokenRequestURL, "https://token.actions.example.com") + t.Setenv(envActionsIDTokenRequestToken, "sometoken") +} + +func mockAzureOIDC(t *testing.T, tenantID, token string) { + t.Helper() + httpmock.RegisterResponder("GET", "https://token.actions.example.com", + httpmock.NewStringResponder(200, `{"count": 1, "value": "sometoken"}`)) + httpmock.RegisterResponder("POST", + "https://login.microsoftonline.com/"+tenantID+"/oauth2/v2.0/token", + httpmock.NewStringResponder(200, `{ + "access_token": "`+token+`", + "expires_in": 3600, + "token_type": "Bearer" + }`)) +} + +func azureCred(tenantID, clientID string) config.Credential { + return config.Credential{ + "type": "test_registry", + "tenant-id": tenantID, + "client-id": clientID, + } +} + +func azureCredWithURL(tenantID, clientID, url string) config.Credential { + cred := azureCred(tenantID, clientID) + cred["url"] = url + return cred +} + +func azureCredWithRegistry(tenantID, clientID, registry string) config.Credential { + cred := azureCred(tenantID, clientID) + cred["registry"] = registry + return cred +} + +func TestOIDCRegistry_Register_SingleCredential(t *testing.T) { + setupOIDCEnv(t) + r := NewOIDCRegistry() + + cred := azureCredWithURL("tenant-1", "client-1", "https://registry.example.com/packages") + oidcCred, key, ok := r.Register(cred, []string{"url"}, "test registry") + + assert.True(t, ok, "should register successfully") + assert.NotNil(t, oidcCred) + assert.Equal(t, "https://registry.example.com/packages", key) +} + +func TestOIDCRegistry_Register_URLFieldPriority(t *testing.T) { + setupOIDCEnv(t) + r := NewOIDCRegistry() + + cred := config.Credential{ + "type": "test_registry", + "tenant-id": "tenant-1", + "client-id": "client-1", + "registry": "https://registry.example.com/from-registry", + "url": "https://registry.example.com/from-url", + } + + _, key, ok := r.Register(cred, []string{"registry", "url"}, "test registry") + + assert.True(t, ok, "should register successfully") + assert.Equal(t, "https://registry.example.com/from-registry", key, "should prefer first urlField") +} + +func TestOIDCRegistry_Register_FallsBackToHost(t *testing.T) { + setupOIDCEnv(t) + r := NewOIDCRegistry() + + cred := config.Credential{ + "type": "test_registry", + "tenant-id": "tenant-1", + "client-id": "client-1", + "host": "registry.example.com", + } + + _, key, ok := r.Register(cred, []string{"url"}, "test registry") + + assert.True(t, ok, "should register with host fallback") + assert.Equal(t, "registry.example.com", key) +} + +func TestOIDCRegistry_Register_NotOIDC(t *testing.T) { + // Don't set OIDC env vars — CreateOIDCCredential will return nil + os.Unsetenv(envActionsIDTokenRequestURL) + os.Unsetenv(envActionsIDTokenRequestToken) + + r := NewOIDCRegistry() + cred := config.Credential{ + "type": "test_registry", + "url": "https://registry.example.com", + } + + oidcCred, key, ok := r.Register(cred, []string{"url"}, "test registry") + + assert.False(t, ok) + assert.Nil(t, oidcCred) + assert.Empty(t, key) +} + +func TestOIDCRegistry_Register_NoKeyAvailable(t *testing.T) { + setupOIDCEnv(t) + r := NewOIDCRegistry() + + // Credential with OIDC params but no URL or host + cred := config.Credential{ + "type": "test_registry", + "tenant-id": "tenant-1", + "client-id": "client-1", + } + + oidcCred, key, ok := r.Register(cred, []string{"url"}, "test registry") + + assert.False(t, ok, "should not register without a key") + assert.NotNil(t, oidcCred, "credential was created but couldn't be stored") + assert.Empty(t, key) +} + +func TestOIDCRegistry_TryAuth_SingleCredential(t *testing.T) { + setupOIDCEnv(t) + httpmock.Activate() + defer httpmock.DeactivateAndReset() + mockAzureOIDC(t, "tenant-1", "__test_token__") + + r := NewOIDCRegistry() + cred := azureCredWithURL("tenant-1", "client-1", "https://registry.example.com/packages") + r.Register(cred, []string{"url"}, "test registry") + + req := httptest.NewRequest("GET", "https://registry.example.com/packages/some-package", nil) + ok := r.TryAuth(req, nil) + + assert.True(t, ok, "should authenticate") + assert.Equal(t, "Bearer __test_token__", req.Header.Get("Authorization")) +} + +func TestOIDCRegistry_TryAuth_SameHostDifferentPaths_NoCollision(t *testing.T) { + setupOIDCEnv(t) + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + mockAzureOIDC(t, "tenant-A", "token-feed-A") + mockAzureOIDC(t, "tenant-B", "token-feed-B") + + r := NewOIDCRegistry() + + // Two registries on the same host with different paths + credA := azureCredWithURL("tenant-A", "client-A", + "https://pkgs.dev.azure.com/org/_packaging/feed-A/npm/registry/") + credB := azureCredWithURL("tenant-B", "client-B", + "https://pkgs.dev.azure.com/org/_packaging/feed-B/npm/registry/") + + _, keyA, okA := r.Register(credA, []string{"url"}, "test registry") + _, keyB, okB := r.Register(credB, []string{"url"}, "test registry") + + require.True(t, okA, "feed-A should register") + require.True(t, okB, "feed-B should register") + assert.NotEqual(t, keyA, keyB, "keys should be different") + + // Request to feed-A should get feed-A's token + reqA := httptest.NewRequest("GET", + "https://pkgs.dev.azure.com/org/_packaging/feed-A/npm/registry/@scope/package", nil) + ok := r.TryAuth(reqA, nil) + assert.True(t, ok, "feed-A request should be authenticated") + assert.Equal(t, "Bearer token-feed-A", reqA.Header.Get("Authorization"), + "feed-A request should get feed-A's token") + + // Request to feed-B should get feed-B's token + reqB := httptest.NewRequest("GET", + "https://pkgs.dev.azure.com/org/_packaging/feed-B/npm/registry/@scope/package", nil) + ok = r.TryAuth(reqB, nil) + assert.True(t, ok, "feed-B request should be authenticated") + assert.Equal(t, "Bearer token-feed-B", reqB.Header.Get("Authorization"), + "feed-B request should get feed-B's token") +} + +func TestOIDCRegistry_TryAuth_HostOnlyMatchesAnyPath(t *testing.T) { + setupOIDCEnv(t) + httpmock.Activate() + defer httpmock.DeactivateAndReset() + mockAzureOIDC(t, "tenant-1", "__test_token__") + + r := NewOIDCRegistry() + + // Register with host only (no path) + cred := config.Credential{ + "type": "test_registry", + "tenant-id": "tenant-1", + "client-id": "client-1", + "host": "registry.example.com", + } + r.Register(cred, []string{"url"}, "test registry") + + // Should match any path on that host + req := httptest.NewRequest("GET", "https://registry.example.com/any/path/here", nil) + ok := r.TryAuth(req, nil) + + assert.True(t, ok, "host-only credential should match any path") + assert.Equal(t, "Bearer __test_token__", req.Header.Get("Authorization")) +} + +func TestOIDCRegistry_TryAuth_NoMatch(t *testing.T) { + setupOIDCEnv(t) + r := NewOIDCRegistry() + + cred := azureCredWithURL("tenant-1", "client-1", "https://registry.example.com/packages") + r.Register(cred, []string{"url"}, "test registry") + + // Request to a different host + req := httptest.NewRequest("GET", "https://other.example.com/packages/something", nil) + ok := r.TryAuth(req, nil) + + assert.False(t, ok, "should not match different host") + assert.Empty(t, req.Header.Get("Authorization")) +} + +func TestOIDCRegistry_TryAuth_WrongPathNoMatch(t *testing.T) { + setupOIDCEnv(t) + r := NewOIDCRegistry() + + cred := azureCredWithURL("tenant-1", "client-1", + "https://pkgs.dev.azure.com/org/_packaging/feed-A/npm/registry/") + r.Register(cred, []string{"url"}, "test registry") + + // Request to same host but different feed path + req := httptest.NewRequest("GET", + "https://pkgs.dev.azure.com/org/_packaging/feed-B/npm/registry/@scope/pkg", nil) + ok := r.TryAuth(req, nil) + + assert.False(t, ok, "should not match different path") + assert.Empty(t, req.Header.Get("Authorization")) +} + +func TestOIDCRegistry_RegisterURL(t *testing.T) { + setupOIDCEnv(t) + httpmock.Activate() + defer httpmock.DeactivateAndReset() + mockAzureOIDC(t, "tenant-1", "__test_token__") + + r := NewOIDCRegistry() + + // Register primary URL + cred := azureCredWithURL("tenant-1", "client-1", "https://nuget.example.com/v3/index.json") + oidcCred, _, ok := r.Register(cred, []string{"url"}, "nuget feed") + require.True(t, ok) + + // Register discovered URL (like nuget does) + r.RegisterURL("https://nuget.example.com/v3/package-content", oidcCred, "nuget resource") + + // Request to discovered URL should be authenticated + req := httptest.NewRequest("GET", + "https://nuget.example.com/v3/package-content/some-package/1.0.0", nil) + ok = r.TryAuth(req, nil) + + assert.True(t, ok, "discovered URL should be authenticated") + assert.Equal(t, "Bearer __test_token__", req.Header.Get("Authorization")) +} + +func TestOIDCRegistry_TryAuth_PortMismatch(t *testing.T) { + setupOIDCEnv(t) + r := NewOIDCRegistry() + + cred := azureCredWithURL("tenant-1", "client-1", "https://registry.example.com:8443/packages") + r.Register(cred, []string{"url"}, "test registry") + + // Request on default port (443) should not match cred on port 8443 + req := httptest.NewRequest("GET", "https://registry.example.com/packages/something", nil) + ok := r.TryAuth(req, nil) + + assert.False(t, ok, "should not match different port") +} + +func TestOIDCRegistry_Register_RegistryField(t *testing.T) { + setupOIDCEnv(t) + r := NewOIDCRegistry() + + cred := azureCredWithRegistry("tenant-1", "client-1", "ghcr.io") + _, key, ok := r.Register(cred, []string{"registry"}, "docker registry") + + assert.True(t, ok) + assert.Equal(t, "ghcr.io", key) +} From c277a3bf12c95f6f7aba101d113534130949b6d2 Mon Sep 17 00:00:00 2001 From: Kamil Bukum Date: Thu, 2 Apr 2026 18:55:40 -0500 Subject: [PATCH 2/8] Fix review: case-insensitive host, longest-prefix matching, doc comment - Lowercase request host in TryAuth to match stored entries - Select longest matching path prefix instead of first match - Document the (credential, "", false) return case in Register - Add tests: PathSpecificBeatsHostOnly, LongestPathPrefixWins, CaseInsensitiveHost --- internal/oidc/oidc_registry.go | 14 ++++-- internal/oidc/oidc_registry_test.go | 70 +++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 4 deletions(-) diff --git a/internal/oidc/oidc_registry.go b/internal/oidc/oidc_registry.go index 4387fb0..c8e877c 100644 --- a/internal/oidc/oidc_registry.go +++ b/internal/oidc/oidc_registry.go @@ -41,6 +41,7 @@ func NewOIDCRegistry() *OIDCRegistry { // // Returns: // - (credential, key, true) if an OIDC credential was created and registered +// - (credential, "", false) if OIDC-configured but no URL or host could be resolved // - (nil, "", false) if the credential is not OIDC-configured func (r *OIDCRegistry) Register( cred config.Credential, @@ -94,7 +95,7 @@ func (r *OIDCRegistry) RegisterURL(url string, cred *OIDCCredential, registryTyp // // Returns true if the request was authenticated, false otherwise. func (r *OIDCRegistry) TryAuth(req *http.Request, ctx *goproxy.ProxyCtx) bool { - host := helpers.GetHost(req) + host := strings.ToLower(helpers.GetHost(req)) reqPort := req.URL.Port() if reqPort == "" { reqPort = "443" @@ -108,16 +109,21 @@ func (r *OIDCRegistry) TryAuth(req *http.Request, ctx *goproxy.ProxyCtx) bool { return false } - // Find the matching entry: host is already matched, check port + path prefix + // Find the most specific matching entry: host is already matched, + // select the longest path prefix among entries with the same port. var matched *OIDCCredential + bestPathLen := -1 for i := range entries { e := &entries[i] if e.port != reqPort { continue } - if strings.HasPrefix(req.URL.Path, e.path) { + if !strings.HasPrefix(req.URL.Path, e.path) { + continue + } + if len(e.path) > bestPathLen { matched = e.credential - break + bestPathLen = len(e.path) } } diff --git a/internal/oidc/oidc_registry_test.go b/internal/oidc/oidc_registry_test.go index 3667ead..9e1bfc9 100644 --- a/internal/oidc/oidc_registry_test.go +++ b/internal/oidc/oidc_registry_test.go @@ -297,3 +297,73 @@ func TestOIDCRegistry_Register_RegistryField(t *testing.T) { assert.True(t, ok) assert.Equal(t, "ghcr.io", key) } + +func TestOIDCRegistry_TryAuth_PathSpecificBeatsHostOnly(t *testing.T) { + setupOIDCEnv(t) + httpmock.Activate() + defer httpmock.DeactivateAndReset() + mockAzureOIDC(t, "tenant-1", "__host_only_token__") + mockAzureOIDC(t, "tenant-2", "__path_specific_token__") + + r := NewOIDCRegistry() + + hostOnlyCred := config.Credential{ + "type": "test_registry", + "tenant-id": "tenant-1", + "client-id": "client-1", + "host": "registry.example.com", + } + pathSpecificCred := azureCredWithURL("tenant-2", "client-2", "https://registry.example.com/packages/private") + + // Register the less specific match first to verify the most specific wins + r.Register(hostOnlyCred, []string{"url"}, "test registry") + r.Register(pathSpecificCred, []string{"url"}, "test registry") + + req := httptest.NewRequest("GET", "https://registry.example.com/packages/private/module.tgz", nil) + ok := r.TryAuth(req, nil) + + assert.True(t, ok, "path-specific credential should match request") + assert.Equal(t, "Bearer __path_specific_token__", req.Header.Get("Authorization")) +} + +func TestOIDCRegistry_TryAuth_LongestPathPrefixWins(t *testing.T) { + setupOIDCEnv(t) + httpmock.Activate() + defer httpmock.DeactivateAndReset() + mockAzureOIDC(t, "tenant-1", "__short_prefix_token__") + mockAzureOIDC(t, "tenant-2", "__long_prefix_token__") + + r := NewOIDCRegistry() + + shortPrefixCred := azureCredWithURL("tenant-1", "client-1", "https://registry.example.com/packages") + longPrefixCred := azureCredWithURL("tenant-2", "client-2", "https://registry.example.com/packages/private") + + // Register the shorter prefix first to verify specificity over insertion order + r.Register(shortPrefixCred, []string{"url"}, "test registry") + r.Register(longPrefixCred, []string{"url"}, "test registry") + + req := httptest.NewRequest("GET", "https://registry.example.com/packages/private/module.tgz", nil) + ok := r.TryAuth(req, nil) + + assert.True(t, ok, "longer path prefix should match request") + assert.Equal(t, "Bearer __long_prefix_token__", req.Header.Get("Authorization")) +} + +func TestOIDCRegistry_TryAuth_CaseInsensitiveHost(t *testing.T) { + setupOIDCEnv(t) + httpmock.Activate() + defer httpmock.DeactivateAndReset() + mockAzureOIDC(t, "tenant-1", "__test_token__") + + r := NewOIDCRegistry() + + cred := azureCredWithURL("tenant-1", "client-1", "https://Registry.Example.COM/packages") + r.Register(cred, []string{"url"}, "test registry") + + // Request with different casing should still match + req := httptest.NewRequest("GET", "https://REGISTRY.EXAMPLE.COM/packages/something", nil) + ok := r.TryAuth(req, nil) + + assert.True(t, ok, "host matching should be case-insensitive") + assert.Equal(t, "Bearer __test_token__", req.Header.Get("Authorization")) +} From a204e4831e7bea2557d418dfa772d35587d786e7 Mon Sep 17 00:00:00 2001 From: Kamil Bukum Date: Thu, 2 Apr 2026 19:05:37 -0500 Subject: [PATCH 3/8] Fix review: propagate addEntry failure to callers addEntry can fail silently when parseRegistryURL cannot extract a hostname. Return bool from addEntry so Register returns ok=false and RegisterURL skips the success log when nothing was stored. --- internal/oidc/oidc_registry.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/internal/oidc/oidc_registry.go b/internal/oidc/oidc_registry.go index c8e877c..883bcd1 100644 --- a/internal/oidc/oidc_registry.go +++ b/internal/oidc/oidc_registry.go @@ -68,7 +68,9 @@ func (r *OIDCRegistry) Register( return oidcCredential, "", false } - r.addEntry(key, oidcCredential) + if !r.addEntry(key, oidcCredential) { + return oidcCredential, "", false + } logging.RequestLogf(nil, "registered %s OIDC credentials for %s: %s", oidcCredential.Provider(), registryType, key) return oidcCredential, key, true @@ -81,7 +83,9 @@ func (r *OIDCRegistry) RegisterURL(url string, cred *OIDCCredential, registryTyp if url == "" || cred == nil { return } - r.addEntry(url, cred) + if !r.addEntry(url, cred) { + return + } logging.RequestLogf(nil, "registered %s OIDC credentials for %s: %s", cred.Provider(), registryType, url) } @@ -150,11 +154,11 @@ func (r *OIDCRegistry) TryAuth(req *http.Request, ctx *goproxy.ProxyCtx) bool { } // addEntry parses a URL or hostname string and adds a credential entry -// to the appropriate host bucket. -func (r *OIDCRegistry) addEntry(urlOrHost string, cred *OIDCCredential) { +// to the appropriate host bucket. Returns false if the URL could not be parsed. +func (r *OIDCRegistry) addEntry(urlOrHost string, cred *OIDCCredential) bool { host, path, port := parseRegistryURL(urlOrHost) if host == "" { - return + return false } entry := oidcEntry{ @@ -166,6 +170,8 @@ func (r *OIDCRegistry) addEntry(urlOrHost string, cred *OIDCCredential) { r.mutex.Lock() r.byHost[host] = append(r.byHost[host], entry) r.mutex.Unlock() + + return true } // parseRegistryURL extracts host, path, and port from a URL or hostname string. From 1c3a4e21cafeeb832bca9b97b47c9e2cd5a3a8dd Mon Sep 17 00:00:00 2001 From: Kamil Bukum Date: Thu, 2 Apr 2026 19:11:21 -0500 Subject: [PATCH 4/8] Add tests for Cloudsmith, index-url, protocol-less URLs, multi-RegisterURL Cover remaining handler-driven scenarios: - Cloudsmith OIDC sets X-Api-Key instead of Authorization - index-url field priority (Python pattern) - URL without protocol handled by ParseURLLax - Multiple RegisterURL calls on same host (NuGet pattern) --- internal/oidc/oidc_registry_test.go | 95 +++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/internal/oidc/oidc_registry_test.go b/internal/oidc/oidc_registry_test.go index 9e1bfc9..cdc21da 100644 --- a/internal/oidc/oidc_registry_test.go +++ b/internal/oidc/oidc_registry_test.go @@ -367,3 +367,98 @@ func TestOIDCRegistry_TryAuth_CaseInsensitiveHost(t *testing.T) { assert.True(t, ok, "host matching should be case-insensitive") assert.Equal(t, "Bearer __test_token__", req.Header.Get("Authorization")) } + +func mockCloudsmithOIDC(t *testing.T, namespace, token string) { + t.Helper() + httpmock.RegisterResponder("GET", "https://token.actions.example.com", + httpmock.NewStringResponder(200, `{"count": 1, "value": "sometoken"}`)) + httpmock.RegisterResponder("POST", + "https://api.cloudsmith.io/openid/"+namespace+"/", + httpmock.NewStringResponder(200, `{"token": "`+token+`"}`)) +} + +func cloudsmithCred(namespace, serviceSlug, audience, url string) config.Credential { + return config.Credential{ + "type": "test_registry", + "oidc-namespace": namespace, + "oidc-service-slug": serviceSlug, + "oidc-audience": audience, + "url": url, + } +} + +func TestOIDCRegistry_TryAuth_Cloudsmith_UsesAPIKey(t *testing.T) { + setupOIDCEnv(t) + httpmock.Activate() + defer httpmock.DeactivateAndReset() + mockCloudsmithOIDC(t, "my-org", "__cs_token__") + + r := NewOIDCRegistry() + + cred := cloudsmithCred("my-org", "my-service", "https://cloudsmith.io", "https://dl.cloudsmith.io/basic/my-org/my-repo") + r.Register(cred, []string{"url"}, "test registry") + + req := httptest.NewRequest("GET", "https://dl.cloudsmith.io/basic/my-org/my-repo/some-package", nil) + ok := r.TryAuth(req, nil) + + assert.True(t, ok, "cloudsmith OIDC should authenticate") + assert.Equal(t, "__cs_token__", req.Header.Get("X-Api-Key"), "cloudsmith should use X-Api-Key") + assert.Empty(t, req.Header.Get("Authorization"), "cloudsmith should not set Authorization") +} + +func TestOIDCRegistry_Register_IndexURLField(t *testing.T) { + setupOIDCEnv(t) + r := NewOIDCRegistry() + + cred := azureCred("tenant-1", "client-1") + cred["index-url"] = "https://pkgs.dev.azure.com/org/_packaging/feed/pypi/simple" + + _, key, ok := r.Register(cred, []string{"index-url", "url"}, "python index") + + assert.True(t, ok) + assert.Equal(t, "https://pkgs.dev.azure.com/org/_packaging/feed/pypi/simple", key) +} + +func TestOIDCRegistry_TryAuth_URLWithoutProtocol(t *testing.T) { + setupOIDCEnv(t) + httpmock.Activate() + defer httpmock.DeactivateAndReset() + mockAzureOIDC(t, "tenant-1", "__test_token__") + + r := NewOIDCRegistry() + + cred := azureCred("tenant-1", "client-1") + cred["url"] = "registry.example.com/packages" + r.Register(cred, []string{"url"}, "test registry") + + req := httptest.NewRequest("GET", "https://registry.example.com/packages/something", nil) + ok := r.TryAuth(req, nil) + + assert.True(t, ok, "URL without protocol should be handled by ParseURLLax") + assert.Equal(t, "Bearer __test_token__", req.Header.Get("Authorization")) +} + +func TestOIDCRegistry_RegisterURL_MultipleOnSameHost(t *testing.T) { + setupOIDCEnv(t) + httpmock.Activate() + defer httpmock.DeactivateAndReset() + mockAzureOIDC(t, "tenant-1", "__test_token__") + + r := NewOIDCRegistry() + + cred := azureCredWithURL("tenant-1", "client-1", "https://nuget.example.com/v3/index.json") + oidcCred, _, ok := r.Register(cred, []string{"url"}, "nuget feed") + require.True(t, ok) + + // Register additional discovered resource URLs (nuget pattern) + r.RegisterURL("https://nuget.example.com/v3/package-content", oidcCred, "nuget resource") + r.RegisterURL("https://nuget.example.com/v3/registrations", oidcCred, "nuget resource") + + // All three paths should authenticate + for _, path := range []string{"/v3/index.json", "/v3/package-content/Some.Package/1.0.0", "/v3/registrations/some.package/index.json"} { + req := httptest.NewRequest("GET", "https://nuget.example.com"+path, nil) + ok := r.TryAuth(req, nil) + assert.True(t, ok, "should authenticate: "+path) + assert.Equal(t, "Bearer __test_token__", req.Header.Get("Authorization")) + } +} From 3ee39a6efc6abe17dbe7104963aca27110840206 Mon Sep 17 00:00:00 2001 From: Kamil Bukum Date: Thu, 2 Apr 2026 19:13:37 -0500 Subject: [PATCH 5/8] Prevent duplicate entries in OIDCRegistry addEntry now checks for existing entries with the same path and port before appending. This keeps storage bounded regardless of how many times the same URL is registered. --- internal/oidc/oidc_registry.go | 20 +++++++++++++------- internal/oidc/oidc_registry_test.go | 18 ++++++++++++++++++ 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/internal/oidc/oidc_registry.go b/internal/oidc/oidc_registry.go index 883bcd1..156422a 100644 --- a/internal/oidc/oidc_registry.go +++ b/internal/oidc/oidc_registry.go @@ -154,22 +154,28 @@ func (r *OIDCRegistry) TryAuth(req *http.Request, ctx *goproxy.ProxyCtx) bool { } // addEntry parses a URL or hostname string and adds a credential entry -// to the appropriate host bucket. Returns false if the URL could not be parsed. +// to the appropriate host bucket. Skips duplicates with the same path and port. +// Returns false if the URL could not be parsed. func (r *OIDCRegistry) addEntry(urlOrHost string, cred *OIDCCredential) bool { host, path, port := parseRegistryURL(urlOrHost) if host == "" { return false } - entry := oidcEntry{ + r.mutex.Lock() + defer r.mutex.Unlock() + + for _, e := range r.byHost[host] { + if e.path == path && e.port == port { + return true + } + } + + r.byHost[host] = append(r.byHost[host], oidcEntry{ path: path, port: port, credential: cred, - } - - r.mutex.Lock() - r.byHost[host] = append(r.byHost[host], entry) - r.mutex.Unlock() + }) return true } diff --git a/internal/oidc/oidc_registry_test.go b/internal/oidc/oidc_registry_test.go index cdc21da..7702d58 100644 --- a/internal/oidc/oidc_registry_test.go +++ b/internal/oidc/oidc_registry_test.go @@ -462,3 +462,21 @@ func TestOIDCRegistry_RegisterURL_MultipleOnSameHost(t *testing.T) { assert.Equal(t, "Bearer __test_token__", req.Header.Get("Authorization")) } } + +func TestOIDCRegistry_Register_NoDuplicateEntries(t *testing.T) { + setupOIDCEnv(t) + + r := NewOIDCRegistry() + + cred1 := azureCredWithURL("tenant-1", "client-1", "https://registry.example.com/packages") + cred2 := azureCredWithURL("tenant-2", "client-2", "https://registry.example.com/packages") + + r.Register(cred1, []string{"url"}, "test registry") + r.Register(cred2, []string{"url"}, "test registry") + + r.mutex.RLock() + entries := r.byHost["registry.example.com"] + r.mutex.RUnlock() + + assert.Equal(t, 1, len(entries), "duplicate path+port should not create a second entry") +} From b63c337e05f38e21945e3bec485b665fae5a93d8 Mon Sep 17 00:00:00 2001 From: Kamil Bukum Date: Thu, 2 Apr 2026 19:22:44 -0500 Subject: [PATCH 6/8] Fix test: replace os.Unsetenv with t.Setenv for proper cleanup os.Unsetenv doesn't participate in test cleanup and could pollute subsequent tests. Use t.Setenv with empty string instead. --- internal/oidc/oidc_registry_test.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/internal/oidc/oidc_registry_test.go b/internal/oidc/oidc_registry_test.go index 7702d58..c7372e4 100644 --- a/internal/oidc/oidc_registry_test.go +++ b/internal/oidc/oidc_registry_test.go @@ -2,7 +2,6 @@ package oidc import ( "net/http/httptest" - "os" "testing" "github.com/jarcoal/httpmock" @@ -99,9 +98,9 @@ func TestOIDCRegistry_Register_FallsBackToHost(t *testing.T) { } func TestOIDCRegistry_Register_NotOIDC(t *testing.T) { - // Don't set OIDC env vars — CreateOIDCCredential will return nil - os.Unsetenv(envActionsIDTokenRequestURL) - os.Unsetenv(envActionsIDTokenRequestToken) + // Ensure OIDC env vars are not set — CreateOIDCCredential will return nil + t.Setenv(envActionsIDTokenRequestURL, "") + t.Setenv(envActionsIDTokenRequestToken, "") r := NewOIDCRegistry() cred := config.Credential{ From 5328230092f5f615a75d03163e8e99481131275d Mon Sep 17 00:00:00 2001 From: Kamil Bukum Date: Thu, 2 Apr 2026 20:03:50 -0500 Subject: [PATCH 7/8] Retrigger CI From 4138ff981d019e4b4265cd2c07a934ec7f483f61 Mon Sep 17 00:00:00 2001 From: Kamil Bukum Date: Fri, 3 Apr 2026 17:58:30 -0500 Subject: [PATCH 8/8] Enhance OIDCRegistry: Generalize addEntry method and improve logging for duplicate entries --- internal/oidc/oidc_registry.go | 24 +++++++++++++--------- internal/oidc/oidc_registry_test.go | 31 +++++++++++++++++++++++++++-- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/internal/oidc/oidc_registry.go b/internal/oidc/oidc_registry.go index 156422a..18019e9 100644 --- a/internal/oidc/oidc_registry.go +++ b/internal/oidc/oidc_registry.go @@ -68,10 +68,9 @@ func (r *OIDCRegistry) Register( return oidcCredential, "", false } - if !r.addEntry(key, oidcCredential) { + if !r.addEntry(key, oidcCredential, registryType) { return oidcCredential, "", false } - logging.RequestLogf(nil, "registered %s OIDC credentials for %s: %s", oidcCredential.Provider(), registryType, key) return oidcCredential, key, true } @@ -83,15 +82,15 @@ func (r *OIDCRegistry) RegisterURL(url string, cred *OIDCCredential, registryTyp if url == "" || cred == nil { return } - if !r.addEntry(url, cred) { - return - } - logging.RequestLogf(nil, "registered %s OIDC credentials for %s: %s", cred.Provider(), registryType, url) + r.addEntry(url, cred, registryType) } // TryAuth finds the matching OIDC credential for the request and // sets the appropriate auth header. // +// Callers are responsible for scheme and method checks (e.g. HTTPS-only, +// GET/HEAD only) before calling TryAuth. +// // Lookup: // 1. Find the host bucket via map lookup (exact hostname match) // 2. Within that bucket, find the entry whose stored path is a @@ -154,11 +153,14 @@ func (r *OIDCRegistry) TryAuth(req *http.Request, ctx *goproxy.ProxyCtx) bool { } // addEntry parses a URL or hostname string and adds a credential entry -// to the appropriate host bucket. Skips duplicates with the same path and port. -// Returns false if the URL could not be parsed. -func (r *OIDCRegistry) addEntry(urlOrHost string, cred *OIDCCredential) bool { +// to the appropriate host bucket. Returns true if the credential is stored +// (either newly added or already present from a prior registration). +// Returns false only when the URL cannot be parsed. +// Duplicates with the same path and port are skipped (first-wins). +func (r *OIDCRegistry) addEntry(urlOrHost string, cred *OIDCCredential, registryType string) bool { host, path, port := parseRegistryURL(urlOrHost) if host == "" { + logging.RequestLogf(nil, "failed to parse OIDC credential URL: %s", urlOrHost) return false } @@ -167,6 +169,9 @@ func (r *OIDCRegistry) addEntry(urlOrHost string, cred *OIDCCredential) bool { for _, e := range r.byHost[host] { if e.path == path && e.port == port { + // First-wins: the credential already stored for this path+port + // will be used; the new one is discarded. + logging.RequestLogf(nil, "skipping duplicate OIDC credential for %s (path: %s)", host, path) return true } } @@ -177,6 +182,7 @@ func (r *OIDCRegistry) addEntry(urlOrHost string, cred *OIDCCredential) bool { credential: cred, }) + logging.RequestLogf(nil, "registered %s OIDC credentials for %s: %s", cred.Provider(), registryType, urlOrHost) return true } diff --git a/internal/oidc/oidc_registry_test.go b/internal/oidc/oidc_registry_test.go index c7372e4..0e2e327 100644 --- a/internal/oidc/oidc_registry_test.go +++ b/internal/oidc/oidc_registry_test.go @@ -1,7 +1,11 @@ package oidc import ( + "bytes" + "log" "net/http/httptest" + "os" + "strings" "testing" "github.com/jarcoal/httpmock" @@ -470,12 +474,35 @@ func TestOIDCRegistry_Register_NoDuplicateEntries(t *testing.T) { cred1 := azureCredWithURL("tenant-1", "client-1", "https://registry.example.com/packages") cred2 := azureCredWithURL("tenant-2", "client-2", "https://registry.example.com/packages") - r.Register(cred1, []string{"url"}, "test registry") - r.Register(cred2, []string{"url"}, "test registry") + var logBuf bytes.Buffer + log.SetOutput(&logBuf) + defer log.SetOutput(os.Stderr) + + oidcCred1, key1, ok1 := r.Register(cred1, []string{"url"}, "test registry") + oidcCred2, key2, ok2 := r.Register(cred2, []string{"url"}, "test registry") + + // Both should return ok=true (credential is registered or already present) + assert.True(t, ok1, "first registration should succeed") + assert.True(t, ok2, "duplicate registration should still return ok=true") + assert.NotNil(t, oidcCred1) + assert.NotNil(t, oidcCred2) + assert.Equal(t, key1, key2, "both should resolve to the same key") r.mutex.RLock() entries := r.byHost["registry.example.com"] r.mutex.RUnlock() assert.Equal(t, 1, len(entries), "duplicate path+port should not create a second entry") + + // First-wins: the stored credential should be from tenant-1 + assert.Equal(t, "tenant-1", + entries[0].credential.parameters.(*AzureOIDCParameters).TenantID, + "first credential should be retained (first-wins)") + + // Verify logging: "registered" only once, "skipping duplicate" for the second + logOutput := logBuf.String() + assert.Equal(t, 1, strings.Count(logOutput, "registered azure OIDC credentials for test registry"), + "should log 'registered' only once") + assert.Contains(t, logOutput, "skipping duplicate OIDC credential", + "should log that duplicate was skipped") }