Unify safe dialer use

[joniumGit]

This PR is co-authored by Copilot

What are you trying to accomplish?

I am trying to unify the safeDialer use in the codebase. I noticed that there were plenty of user input going into Http Clients in some parameters and them not using the safe dialer that the proxy explicitly creates in the beginning. This will prevent any config parameters from being used to reach unintended destinations. The way I did it is a bit ugly and ends up with the transport being added to all of the registry handler constructors.

Anything you want to highlight for special attention from reviewers?

Please check if this change is needed. I am not sure if the initial state of not using safe dialer in some of the registry handlers was intended.

How will you know you've accomplished your goal?

No user input should reach a raw HTTP Client.

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.

[Copilot]

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR threads a shared http.RoundTripper/http.Client through OIDC token acquisition and multiple registry handlers so outbound OIDC exchanges can use the proxy’s configured transport.

Changes:

• Add transport http.RoundTripper to many handler constructors and pass it from proxy.go.
• Extend OIDCRegistry and CreateOIDCCredential to carry/create an HTTP client using the provided transport.
• Update Actions OIDC token-exchange helpers to accept an injected *http.Client, and adjust tests accordingly.

Reviewed changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 10 comments.

Show a summary per file

File	Description
proxy.go	Passes the shared transport into more registry handlers.
internal/oidc/oidc_registry.go	Stores transport in the registry and uses it when creating OIDC credentials.
internal/oidc/oidc_registry_test.go	Updates registry construction for new transport parameter.
internal/oidc/oidc_credential.go	Adds an HTTP client to OIDC credentials and routes token fetches through it.
internal/oidc/oidc_credential_test.go	Updates credential creation calls for new transport parameter.
internal/oidc/actions_oidc.go	Injects *http.Client into token-exchange functions instead of creating local clients.
internal/oidc/actions_oidc_test.go	Passes http.DefaultClient to updated token-exchange signatures.
internal/handlers/terraform_registry.go	Adds transport param and passes it into OIDC registry.
internal/handlers/terraform_registry_test.go	Updates handler construction for new transport parameter.
internal/handlers/rubygems_server.go	Adds transport param and passes it into OIDC registry.
internal/handlers/rubygems_server_test.go	Updates handler construction for new transport parameter.
internal/handlers/python_index.go	Adds transport param and uses it for OIDC credential creation/registration.
internal/handlers/python_index_test.go	Updates handler construction for new transport parameter.
internal/handlers/pub_repository.go	Adds transport param and uses it for OIDC credential creation/registration.
internal/handlers/pub_repository_test.go	Updates handler construction for new transport parameter.
internal/handlers/oidc_handling_test.go	Updates many handler factories to pass the new transport argument.
internal/handlers/nuget_feed.go	Adds transport param, uses it in HTTP client and OIDC registry.
internal/handlers/nuget_feed_test.go	Updates handler construction for new transport parameter.
internal/handlers/npm_registry.go	Adds transport param and passes it into OIDC registry.
internal/handlers/npm_registry_test.go	Updates handler construction for new transport parameter.
internal/handlers/maven_repository.go	Adds transport param and passes it into OIDC registry.
internal/handlers/maven_repository_test.go	Updates handler construction for new transport parameter.
internal/handlers/hex_repository.go	Adds transport param and uses it for OIDC credential creation/registration.
internal/handlers/hex_repository_test.go	Updates handler construction for new transport parameter.
internal/handlers/helm_registry.go	Adds transport param and passes it into OIDC registry.
internal/handlers/helm_registry_test.go	Updates handler construction for new transport parameter.
internal/handlers/goproxy_server_handler.go	Adds transport param and passes it into OIDC registry.
internal/handlers/goproxy_server_handler_test.go	Updates handler construction for new transport parameter.
internal/handlers/docker_registry.go	Constructs OIDC registry with the provided transport.
internal/handlers/composer.go	Adds transport param and passes it into OIDC registry.
internal/handlers/composer_test.go	Updates handler construction for new transport parameter.
internal/handlers/cargo_registry.go	Adds transport param and uses it for OIDC credential creation/registration.
internal/handlers/cargo_registry_test.go	Updates handler construction for new transport parameter.

[jeffwidman]

@joniumGit 👋 Sorry this took a while for us to get to...

I started to review when I noticed merge conflicts and also a bunch of agent review comments...

If you can rebase and then take a look at those--don't have to agree, but at least take a look and if you disagree note why, after that I'd be happy to review.

[jeffwidman]

Agent note: This comment was produced by an agent after an adversarial review, guided by Jeff on the specific areas to examine. It is not Jeff speaking directly, so maintainers may hold these conclusions less strongly.

• The underlying problem is worth solving: OIDC exchanges can carry assertions or generated credentials, and bypassing the proxy's restricted transport weakens an intentional SSRF boundary. This is meaningful defense in depth, though currently narrow: production blocks cloud metadata destinations, not private networks generally, and exploitation appears to require influence over job credentials or OIDC configuration.
• The implementation is directionally sound, but incomplete. Provider token exchanges use the injected transport, while the initial GitHub Actions GetToken request still uses a raw client; the ECR SDK client also remains outside this boundary unless that is explicitly considered trusted.
• Construct one *http.Client at the composition root with the safe transport and the existing bounded timeout, then inject that client through the OIDC registry/credential path. Passing the client, rather than a nullable RoundTripper, keeps timeout and transport policy together and avoids silent fallback to http.DefaultTransport. Production constructors should reject or make nil impossible; tests should pass an explicit test client.
• Use that same client for every leg of the flow: the GitHub Actions GetToken request and each Azure/JFrog/AWS/Cloudsmith/GCP exchange. Redirects must remain on the safe transport as well. For ECR, supply the client through the AWS SDK HTTP-client option, or explicitly document why that endpoint is outside this boundary.
• Add focused regression coverage for the invariant rather than only adapting signatures with nil or http.DefaultClient: a recording RoundTripper should prove both OIDC legs use the injected client, and an integration-style test should prove direct and redirected requests to a blocked destination fail without making a connection. Retain an assertion for the 10-second timeout.
• The Cloudsmith audience validation looks reasonable, but is unrelated and would be clearer as a separate change.

Overall: preserve the idea, but make the restricted client a mandatory dependency, cover all outbound credential-bearing requests, and test the security boundary directly before merging.

[joniumGit]

I am thinking this could be just a nil check for the client

[joniumGit]

I agree with the review feedback.

Originally, I left the Github Actions and AWS SDK clients untouched as I was not sure if they need to reach some SafeDialer blocked destination. I also used the transport as a parameter instead of a client to minimize changes as that pattern was used already.

Anyways, I made Copilot rebase the changes, change the parameter to a HttpClient, and add tests that your review flagged. I can separate the audience validation to another PR, but I think it is a small change overall and could go as part of this as well, it is in a separate commit.

[joniumGit]

Oh right, the Copilot review comments. They are all about a nil check for the HttpClient in the OIDC code. I personally don't see it necessary to nil check that as I don't think there is any valid reason to pass null to those functions, but I am open to other opinions.