Skip to content

Migrate nuget handler to OIDCRegistry#92

Open
kbukum1 wants to merge 1 commit intomainfrom
kamil/oidc-migrate-nuget
Open

Migrate nuget handler to OIDCRegistry#92
kbukum1 wants to merge 1 commit intomainfrom
kamil/oidc-migrate-nuget

Conversation

@kbukum1
Copy link
Copy Markdown
Contributor

@kbukum1 kbukum1 commented Apr 3, 2026

What

Migrate the NuGet feed handler from manual OIDC credential map + mutex to the shared OIDCRegistry type introduced in #78.

Why

Part of the phased migration to fix OIDC credential collisions when multiple registries share a host (#87).

NuGet previously stored OIDC credentials by url-with-host-fallback. OIDCRegistry preserves the full URL with path-prefix matching, fixing potential collisions when multiple NuGet feeds share a host.

Key changes

  • Replace oidcCredentials map[string]*oidc.OIDCCredential + sync.RWMutex with *oidc.OIDCRegistry
  • Primary feed URL registered via Register(cred, ["url"], "nuget feed")
  • HTTP-discovered resource URLs registered via RegisterURL(discoveredURL, credential, "nuget resource") — this is the only handler that uses RegisterURL
  • Discovery request authenticated via oidcRegistry.TryAuth() instead of TryAuthOIDCRequestWithPrefix()
  • Test log lines updated: RegisterURL uses consistent format without the leading indent the old code used
  • Net -13 lines

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.TryAuth uses 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.ToLower instead of IDNA normalization. The old TryAuthOIDCRequestWithPrefix used helpers.AreHostnamesEqual (IDNA ToASCII), while OIDCRegistry.TryAuth uses 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.

  • OIDC auth restricted to HTTPS only. Previously TryAuthOIDCRequestWithPrefix had no scheme guard, so OIDC tokens could be sent over plaintext HTTP. Now gated with req.URL.Scheme == "https" to prevent credential leakage. Static credentials continue to work over both HTTP and HTTPS (existing nuget behavior).

  • OIDC discovery guarded with url != "". Host-only credentials (from CLI) are still registered for request-time matching, but feed index discovery is skipped when the URL field is empty since bare hostnames are not valid request URLs.

  • Response body leak fix. Both the OIDC and static credential discovery blocks now use a closure with defer rawRsp.Body.Close(), fixing a pre-existing bug where the body was leaked on io.ReadAll error or early-return status code paths.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Migrates the NuGet feed handler’s OIDC handling to the shared oidc.OIDCRegistry, aligning NuGet with the registry-based, path-prefix OIDC matching model introduced in #78 to avoid credential collisions when multiple feeds share a host.

Changes:

  • Replaced the NuGet handler’s per-handler OIDC map + RWMutex with *oidc.OIDCRegistry, and switched request-time auth to oidcRegistry.TryAuth().
  • Added registration of NuGet HTTP-discovered resource URLs via oidcRegistry.RegisterURL(...).
  • Updated OIDC log-line expectations in oidc_handling_test.go to match the new registration log format.

Reviewed changes

Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.

File Description
internal/handlers/nuget_feed.go Uses OIDCRegistry for OIDC credential storage/matching and registers discovered NuGet resource URLs via RegisterURL.
internal/handlers/oidc_handling_test.go Updates expected log lines for NuGet OIDC resource registration output.
Comments suppressed due to low confidence (1)

internal/handlers/nuget_feed.go:92

  • The log line on io.ReadAll failure drops the underlying error and the URL being fetched, which makes diagnosing feed discovery issues harder. Include both err and the key/URL in the message (and consider logging response status when relevant).
				body, err := io.ReadAll(rawRsp.Body)
				if err != nil {
					logging.RequestLogf(nil, "error reading http response body")
					continue

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 2 changed files in this pull request and generated 1 comment.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 2 changed files in this pull request and generated no new comments.

@kbukum1 kbukum1 marked this pull request as ready for review April 3, 2026 05:01
@kbukum1 kbukum1 force-pushed the kamil/oidc-migrate-nuget branch from c8bf1a1 to 65234e2 Compare April 3, 2026 05:30
@kbukum1 kbukum1 requested a review from Copilot April 3, 2026 05:33
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 2 changed files in this pull request and generated no new comments.

@kbukum1 kbukum1 requested a review from brettfo April 3, 2026 07:42
Base automatically changed from kamil/oidc-registry-generalized to main April 4, 2026 00:12
Replace manual OIDC credential map and mutex with the shared
OIDCRegistry type. Nuget uses Register() for the primary feed
URL and RegisterURL() for HTTP-discovered resource URLs.

The old code stored OIDC credentials by url-with-host-fallback.
OIDCRegistry preserves the full URL with path-prefix matching,
fixing potential collisions when multiple nuget feeds share a host.

Test log lines updated: RegisterURL uses consistent log format
without the leading indent that the old code used.
@kbukum1 kbukum1 force-pushed the kamil/oidc-migrate-nuget branch from 65234e2 to a63f4fd Compare April 4, 2026 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants