Skip to content

Skip registering credentials for anonymous registries for egress allowlist - #303

Merged
v-sachin-sandhu merged 2 commits into
mainfrom
abhishekbhaskar/handle-no-auth-registries
Oct 7, 2026
Merged

v-sachin-sandhu merged 2 commits into
mainfrom
abhishekbhaskar/handle-no-auth-registries

Conversation

@v-abhishekbhaskar

Copy link
Copy Markdown
Contributor

What are you trying to accomplish?

The egress allowlist builds its per-job allowlist from the job's credentials (credentialHosts in egress_dynamic_hosts.go), so a private registry that needs no authentication has no way onto the allowlist and every request to it is rejected with 403 * egress not allowlisted <host>.

Supporting an anonymous registry means the credential must still be present in the job config, since that is the only route to a dynamic allowlist entry. But once present, the static-credential handlers attach an empty Authorization header — Bearer with no value, or Basic base64(":") — which Nexus and Artifactory reject outright instead of serving anonymously. The customer would simply trade a 403 from the proxy for a 401 from their registry.

This change makes the proxy recognise a credential that names a registry but carries nothing to authenticate with, and skip registering it. The request is forwarded untouched, while the host stays allowlisted.

Anything you want to highlight for special attention from reviewers?

Only six handlers needed changing. maven_repository, goproxy_server, cargo_registry, terraform_registry, pub_repository, hex_organization, hex_repository, and nuget_feed already skip credential-free entries. The six that didn't are npm_registry, composer, helm_registry, docker_registry, python_index, and rubygems_server.

A username with no password is deliberately not anonymous. That's a misconfiguration rather than a declared anonymous registry, and treating it as anonymous would silently discard an intended credential. username is therefore included in credentialSecretKeys, and there's a test pinning this.

token: ":" is treated as absent. That's what dependabot-api's token_from_token_or_username_and_password emits when neither username nor password is set, and it decodes to an empty username and password. This incidentally fixes the existing latent bug for URL-only pip, Maven, and NuGet entries, which dependabot-api already accepts today.

Placement is after OIDC registration in every constructor, matching the existing skip guards, so a credential that mints its token at request time is never mistaken for an anonymous one.

ECR is unaffected — it passes its AWS key pair through username/password, so those credentials are never anonymous. An ECR entry configured with neither was already broken before this change, since the SDK is handed empty static credentials.

The existing proxy-only guards are left in place; the new check is purely additive, so no proxy-only path changes behaviour.

How will you know you've accomplished your goal?

TestAnonymousCredentialsAreNotAuthenticated covers all six handlers, asserting no Authorization header is added at all. Verified it's meaningful: commenting out the skip in npm_registry.go fails the npm subtest with Request's auth header should be empty.

TestEgressAllowlist_AnonymousCredentialAllowlistsHost proves the other half — a credential carrying only a registry location still contributes its host to the allowlist, across npm_registry, docker_registry, python_index, maven_repository, and nuget_feed.

TestEgressAllowlist_AnonymousCredentialDoesNotWidenBeyondExactHost holds the dynamic-host exact-match invariant, with the evil.<host> child probe this repository requires plus a sibling probe and a suffix-appending lookalike.

TestAnonymousCredentialsStillAuthenticateWhenSecretPresent guards against the skip being too eager, and TestCredentialIsAnonymous pins the classification itself, including both empty-secret shapes and the username-only case.

End-to-end validation against a real anonymous-read Nexus with proxy-egress-enforce enabled is still pending; it cannot be covered by unit tests.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@v-abhishekbhaskar v-abhishekbhaskar self-assigned this Oct 7, 2026
Copilot AI balanced review requested due to automatic review settings October 7, 2026 21:34
@v-abhishekbhaskar
v-abhishekbhaskar requested a review from a team as a code owner October 7, 2026 21:34

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

NuGet colon-token credentials remain authenticated, while colon-valued non-token secrets are incorrectly classified as anonymous.

1 open finding
What changed in this PR

Adds anonymous private-registry support while retaining dynamic egress allowlisting.

Changes:

  • Skips static authentication registration for anonymous credentials across six handlers.
  • Adds shared anonymous-credential classification.
  • Adds authentication and exact-host allowlist regression tests.
File Description
internal/​handlers/​credentials.go Adds anonymous credential detection.
internal/​handlers/​npm_registry.go Skips anonymous npm credentials.
internal/​handlers/​composer.go Skips anonymous Composer credentials.
internal/​handlers/​helm_registry.go Skips anonymous Helm credentials.
internal/​handlers/​docker_registry.go Skips anonymous Docker credentials.
internal/​handlers/​python_index.go Skips anonymous Python credentials.
internal/​handlers/​rubygems_server.go Skips anonymous RubyGems credentials.
internal/​handlers/​anonymous_credentials_test.go Tests anonymous and authenticated behavior.
internal/​handlers/​egress_allowlist_test.go Tests dynamic exact-host allowlisting.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/handlers/credentials.go
@v-sachin-sandhu
v-sachin-sandhu merged commit 20763b1 into main Oct 7, 2026
112 checks passed
@v-sachin-sandhu
v-sachin-sandhu deleted the abhishekbhaskar/handle-no-auth-registries branch October 7, 2026 22:57
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.

3 participants