Conversation
There was a problem hiding this comment.
Pull request overview
Migrates the Terraform registry handler to use the shared oidc.OIDCRegistry so OIDC credentials can be matched collision-free (host bucket + longest path-prefix match), aligning Terraform with the registry-wide OIDC credential storage approach introduced in #78 / fixing #87 scenarios.
Changes:
- Replaced Terraform’s per-handler OIDC credential map + RWMutex with
oidc.OIDCRegistry. - Updated Terraform OIDC registration to use the credential’s
urlfield as the primary key (preserving path), with host fallback. - Adjusted OIDC handling tests to expect Terraform registration logs to include the full URL key.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/handlers/terraform_registry.go | Switches Terraform OIDC auth to OIDCRegistry and removes handler-level mutex/map storage. |
| internal/handlers/oidc_handling_test.go | Updates expected Terraform OIDC registration log lines to match new URL-based keys. |
kbukum1
force-pushed
the
kamil/oidc-migrate-terraform
branch
from
8bec9a4 to
46ef1d8
Compare
kbukum1
force-pushed
the
kamil/oidc-migrate-terraform
branch
from
46ef1d8 to
e8fd91c
Compare
| // OIDC credentials are not used as static credentials. | ||
| if oidcCred, _, _ := handler.oidcRegistry.Register(credential, []string{"url"}, "terraform registry"); oidcCred != nil { | ||
| continue |
There was a problem hiding this comment.
The result of oidcRegistry.Register includes a boolean indicating whether the OIDC credential was actually registered (i.e., a usable URL/host key was resolved and parsed). The current check only looks at oidcCred != nil, so an OIDC-configured credential that fails registration will still be skipped for static token setup, leaving it unusable with no handler-level signal. Consider checking the returned registered flag (and logging when it’s false), and only continue when registration succeeds.
Replace manual OIDC credential map and mutex with the shared OIDCRegistry type. OIDC key changes from hostname-only to full URL (via url field, with cred.Host() fallback), fixing credential collisions when multiple Terraform registries share a host.
kbukum1
force-pushed
the
kamil/oidc-migrate-terraform
branch
from
e8fd91c to
d6b157f
Compare
What
Migrate the Terraform registry handler from manual OIDC credential map + mutex to the shared
OIDCRegistrytype introduced in #78.Why
Part of the phased migration to fix OIDC credential collisions when multiple registries share a host (#87).
Terraform was storing OIDC credentials keyed by hostname only via
cred.Host(). TheOIDCRegistrynow preserves the full URL (viaurlfield, withcred.Host()fallback), fixing potential collisions.Behavior changes
Credential selection is now deterministic. The old code iterated over a Go map (
map[string]*OIDCCredential), so with multiple OIDC credentials on the same host, which one matched was nondeterministic.OIDCRegistry.TryAuthuses longest path-prefix matching, ensuring the most specific credential always wins. This is the core fix for OIDC credential collision when multiple registries share a host #87.Host matching uses
strings.ToLowerinstead of IDNA normalization. The oldTryAuthOIDCRequestWithPrefixusedhelpers.AreHostnamesEqual(IDNAToASCII), whileOIDCRegistry.TryAuthuses lowercase comparison. This is acceptable because all real OIDC registries (Azure DevOps, JFrog, AWS CodeArtifact, Cloudsmith) use ASCII hostnames — no package registry uses internationalized domain names.