Allow MCPRemoteProxy to work without upstream or client auth#4168
Allow MCPRemoteProxy to work without upstream or client auth#4168aron-muon wants to merge 5 commits intostacklok:mainfrom
Conversation
e7fec58 to
1c8c7cd
Compare
There was a problem hiding this comment.
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 transformationAlternative:
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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4168 +/- ##
==========================================
+ Coverage 67.86% 67.91% +0.05%
==========================================
Files 610 610
Lines 62522 62556 +34
==========================================
+ Hits 42431 42488 +57
+ Misses 16910 16884 -26
- Partials 3181 3184 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
0aa7f12 to
68e79e9
Compare
Large PR justification has been provided. Thank you!
|
✅ Large PR justification has been provided. The size review has been dismissed and this PR can now proceed with normal review. |
68e79e9 to
1037865
Compare
1037865 to
5b110c0
Compare
5b110c0 to
010f7fc
Compare
010f7fc to
49e9690
Compare
|
@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 |
Very exciting! |
|
@claude review the changes and check if this overlaps with the MCPOIDCConfig CRD changes that have been added recently. |
|
I'll analyze this and get back to you. |
There was a problem hiding this comment.
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.
67d3ec4 to
3b7c6e1
Compare
|
Hey @aron-muon, thanks for keeping this rebased! I did a deep dive into the current state of Goal 2 (optional Goals 1 and 3 are still unresolved:
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 — 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. |
|
hi @aron-muon! 👋 just checking on this PR if there is any other help needed. Thanks! |
18ebe47 to
8195ba3
Compare
Updated with just goal 1 and 3! |
Ready for re-review, sorry for the delay! |
The embedded auth server always injected upstream IdP tokens into requests forwarded to backend MCP servers. This made it impossible to use the embedded auth server for client-facing OAuth flows when the upstream MCP server is public and doesn't require authentication — the injected token caused 401 rejections from the upstream. Add a `disableUpstreamTokenInjection` field to EmbeddedAuthServerConfig that skips the upstream swap middleware while keeping the embedded auth server running for client authentication. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three issues prevented MCPRemoteProxy from connecting to third-party upstream MCP servers that use HTTP redirects: 1. X-Forwarded-Host leaked the proxy's hostname to the upstream. The upstream used it to construct 307 redirect URLs pointing back to the proxy, creating a redirect loop. Fix: skip SetXForwarded() for remote upstreams (isRemote == true). 2. Go's http.Transport.RoundTrip does not follow redirects, but httputil.ReverseProxy uses Transport directly. Upstream 307/308 redirects (e.g. HTTPS→HTTP scheme changes, path canonicalization) were returned to the MCP client which cannot follow them through the proxy. Fix: add forwardFollowingRedirects that transparently follows up to 10 redirects, preserving method and body for 307/308 (RFC 7538). 3. When disableUpstreamTokenInjection is true, the client's ToolHive JWT was still forwarded to the upstream in the Authorization header. Fix: add strip-auth middleware that removes the Authorization header before forwarding. Also adds debug logging for outbound request headers and upstream response status codes to aid diagnosis of remote proxy issues. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Summary
MCPRemoteProxy always injected upstream IdP tokens when the embedded auth server was configured, and the transparent proxy always emitted
X-Forwarded-Hostto remote upstreams. Two consequences:X-Forwarded-Hostwould echo the proxy's hostname back, creating loops the transparent proxy then refused to follow ("refusing cross-host redirect from remote MCP server").This PR addresses both with two focused changes per @ChrisJBurns's review (#4168 (review)).
What changed (2 logical changes):
disableUpstreamTokenInjectionflag onEmbeddedAuthServerConfig. Whentrue, the embedded auth server still runs the OAuth flow for clients but skips upstream token swapping. Astrip-authmiddleware removes the client's ToolHive JWT from theAuthorizationheader before forwarding so the bearer token doesn't leak to the upstream either.Skip
SetXForwarded()for remote upstreams in the transparent proxy. PreventsX-Forwarded-Hostfrom leaking the proxy hostname to third-party servers. Adds debug logging on outbound requests and upstream responses.Dropped from earlier revisions of this PR:
Optional— @ChrisJBurns confirmed this is already resolved on main: bothoidcConfigOIDCConfigandOIDCConfigRefare pointer types andresolveAndAddOIDCConfigreturns(nil, nil)cleanly when both are absent. The OIDC resolver,AddOIDCConfigOptions, and controller validation all handle nil without error.Custom— main now has a more secure redirect-following implementation with cross-host and HTTPS-downgrade guards (addresses Claude Bot's earlier security finding on this PR about the Authorization header being cloned across hosts).forwardFollowingRedirectsRebased onto current main — clean cherry-pick after dropping Goal 2; conflicts resolved against the v1alpha1→v1beta1 rename.
Type of change
Test plan
go test ./cmd/thv-operator/... ./pkg/runner/... ./pkg/transport/proxy/transparent/... ./pkg/authserver/...) — all passgo build ./...) — cleango vet ./cmd/thv-operator/...— cleanAPI Compatibility
v1beta1API. The added field (disableUpstreamTokenInjection) is optional with afalsedefault, matching the existing behavior.Changes
cmd/thv-operator/api/v1beta1/mcpexternalauthconfig_types.goDisableUpstreamTokenInjectiontoEmbeddedAuthServerConfigcmd/thv-operator/pkg/controllerutil/authserver.goRunConfigcmd/thv-operator/pkg/controllerutil/authserver_test.gopkg/authserver/config.goDisableUpstreamTokenInjectionto runtimeRunConfigpkg/runner/middleware.gostrip-authmiddleware; skip upstream swap when injection disabledpkg/runner/middleware_test.gopkg/transport/proxy/transparent/transparent_proxy.goSetXForwarded()for remote upstreams; add debug loggingdeploy/charts/operator-crds/{files,templates}/...docs/operator/crd-api.md,docs/server/{docs.go,swagger.{json,yaml}}Does this introduce a user-facing change?
Yes. One new capability for
MCPExternalAuthConfig(typeembeddedAuthServer):spec.embeddedAuthServer.disableUpstreamTokenInjection: true— authenticates MCP clients via the embedded OAuth server but does not inject upstream IdP tokens or forward the client's ToolHive JWT to the backend. Useful for proxying public MCP servers (documentation, search, etc.) where you want client auth but no upstream auth.The X-Forwarded-Host change is a bug fix; no API surface change.
🤖 Generated with Claude Code