Skip to content

Migrate maven handler to OIDCRegistry#82

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

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

Conversation

@kbukum1
Copy link
Copy Markdown
Contributor

@kbukum1 kbukum1 commented Apr 3, 2026

What

Migrate the Maven repository 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).

Maven was storing OIDC credentials keyed by hostname only. Two Maven repositories on the same host with different paths would collide. The OIDCRegistry preserves the full URL (via url field), fixing this.

Key behavior change

OIDC registration key changes from hostname-only to full URL, reflected in updated expected log lines in the OIDC integration test.

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.

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

This PR migrates the Maven repository handler from a per-handler OIDC credential map (keyed by hostname) to the shared oidc.OIDCRegistry, addressing OIDC credential collisions when multiple Maven registries share the same host but differ by path.

Changes:

  • Replace Maven’s handler-local map[string]*OIDCCredential + sync.RWMutex with oidc.OIDCRegistry.
  • Update OIDC integration test expectations to reflect the new registration/log key (full URL instead of hostname).

Reviewed changes

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

File Description
internal/handlers/maven_repository.go Switch Maven OIDC storage/matching to OIDCRegistry and use TryAuth for request authentication.
internal/handlers/oidc_handling_test.go Update expected log lines for Maven OIDC registration to include full URL keys.

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 2 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 04:38
@kbukum1 kbukum1 force-pushed the kamil/oidc-migrate-maven branch from a458dfa to 3e81650 Compare April 3, 2026 05:12
@kbukum1 kbukum1 requested a review from Copilot April 3, 2026 05:34
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 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

internal/handlers/oidc_handling_test.go:635

  • The Maven OIDC migration is meant to fix same-host/different-path collisions via longest path-prefix matching, but the handler-level integration test cases here still cover only a single Maven OIDC credential at a time. Consider adding a regression case with two maven_repository OIDC credentials sharing the same host but different URL paths (e.g., /feed-A/... and /feed-B/...) and assert that a request under feed-A is authenticated with the feed-A credential (and similarly for feed-B). This will catch regressions where the handler registers with the wrong key fields or the match selection stops being deterministic.
		{
			name:     "Maven",
			provider: "aws",
			handlerFactory: func(creds config.Credentials) oidcHandler {
				return NewMavenRepositoryHandler(creds)
			},
			credentials: config.Credentials{
				config.Credential{
					"type":         "maven_repository",
					"url":          "https://maven.example.com/packages",
					"aws-region":   testRegion,
					"account-id":   "123456789012",
					"role-name":    "MyRole",
					"domain":       "my-domain",
					"domain-owner": "9876543210",
				},
			},
			urlMocks: []mockHttpRequest{},
			expectedLogLines: []string{
				"registered aws OIDC credentials for maven repository: https://maven.example.com/packages",
			},
			urlsToAuthenticate: []string{
				"https://maven.example.com/packages/some-package",
			},
		},

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. OIDC key changes from hostname-only to full URL
(via url field), fixing credential collisions when multiple Maven
repositories share a host with different paths.
@kbukum1 kbukum1 force-pushed the kamil/oidc-migrate-maven branch from 3e81650 to 9e48329 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