Add generalized OIDCRegistry for collision-free credential storage#78
Add generalized OIDCRegistry for collision-free credential storage#78
Conversation
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.
There was a problem hiding this comment.
Pull request overview
Adds a new generalized OIDCRegistry in internal/oidc to store and match OIDC credentials without host-only key collisions (e.g., same host with different registry paths), intended as a Phase 1 additive building block before migrating handlers.
Changes:
- Introduces
OIDCRegistrywith host-bucketed storage and path-prefix matching for OIDC credentials. - Adds
Register,RegisterURL, andTryAuthAPIs to support both configured and discovered registry URLs. - Adds unit tests covering registration behavior, collision avoidance by path, and port handling.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/oidc/oidc_registry.go | Implements the new OIDC credential registry and request-time auth application. |
| internal/oidc/oidc_registry_test.go | Adds unit tests validating registration and matching behaviors for the registry. |
- 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
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.
…erURL 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)
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.
os.Unsetenv doesn't participate in test cleanup and could pollute subsequent tests. Use t.Setenv with empty string instead.
|
This looks like a good solution. The only thing I noticed was it assumes port 443 when port isn't specified and doesn't check the scheme of the url. I think this is reasonably safe as I can't imagine there are many situations where you'd have OIDC auth but no TLS. But I thought it worth mentioning all the same. |
@microblag |
…for duplicate entries
What
Adds
OIDCRegistrytype to theoidcpackage — a generalized credential registry that replaces the per-handlermap[string]*OIDCCredentialpattern.Why
Fixes #87. The current OIDC map is keyed by hostname only. When two registries share a host with different paths (e.g., two Azure DevOps feeds on
pkgs.dev.azure.com), the second credential silently overwrites the first.How
OIDCRegistrystores credentials in amap[hostname][]entrystructure — each entry holds the parsed path, port, and credential. Lookup is O(1) by host, then longest path-prefix match within the bucket.Handlers use two methods:
Register(cred, urlFields, registryType)— at construction timeTryAuth(req, ctx)— at request timeRegisterURL(url, cred, registryType)— for nuget HTTP-discovered URLsThis PR is Phase 1 (additive only). No handler changes — each handler is migrated in its own follow-up PR.
Follow-up PRs
RegisterURLfor discovered URLs; consistent log formatCleanup PR (removing old
TryAuthOIDCRequestWithPrefix) will follow after all handlers are merged.Testing
Acknowledgment
The host+path keying approach used by
OIDCRegistrywas inspired by @microblag's earlier work in #72, which identified and fixed the same-host OIDC collision issue for npm specifically.