Add passthroughHeaders to VirtualMCPServer for header forwarding#5466
Add passthroughHeaders to VirtualMCPServer for header forwarding#5466juancarlosm wants to merge 13 commits into
Conversation
juancarlosm
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
blkt,
jerm-dro,
jhrozek,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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 #5466 +/- ##
==========================================
+ Coverage 69.44% 69.47% +0.02%
==========================================
Files 638 639 +1
Lines 65022 65099 +77
==========================================
+ Hits 45155 45228 +73
- Misses 16540 16546 +6
+ Partials 3327 3325 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
juancarlosm
force-pushed
the
feat/vmcp-passthrough-headers
branch
from
0d98f3a to
508a72a
Compare
github-actions
Bot
added
size/XL
github-actions
Bot
dismissed
their stale review
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. |
github-actions
Bot
added
size/XL
github-actions
Bot
added
size/XL
github-actions
Bot
added
size/XL
github-actions
Bot
added
size/XL
Move the forwarded-header carrier off auth.Identity (and out of the auth factory) onto a typed context value in pkg/vmcp/headerforward, wired in the vMCP server chain via Config.PassthroughHeaders. Addresses @jerm-dro's review on stacklok#5466. Behavior unchanged; validated end-to-end on a live cluster. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
size/L
github-actions
Bot
added
size/L
github-actions
Bot
added
size/L
github-actions
Bot
added
size/L
jerm-dro
left a comment
jerm-dro
left a comment
There was a problem hiding this comment.
New direction LGTM — just a few minor notes inline.
| // PassthroughHeaders is an allowlist of incoming client request header names | ||
| // forwarded verbatim to all backends. Resolved per-request at the auth | ||
| // boundary and baked into the per-session backend client. Names must not be | ||
| // in the restricted set (Host, hop-by-hop, X-Forwarded-*, etc.). |
There was a problem hiding this comment.
suggestion: Doc comment is stale after the refactor. "Resolved per-request at the auth boundary" doesn't match the code anymore — headerforward.CaptureMiddleware is mounted outside the auth middleware (it runs before auth in execution order), and the forwarded map is consumed once at session creation in createMCPClient, not per-request. Suggested rewording:
// PassthroughHeaders is an allowlist of incoming client request header names
// forwarded verbatim to all backends. Captured at the vMCP incoming edge by
// headerforward.CaptureMiddleware and consumed once at session creation
// when the per-session backend client's HeaderForwardConfig is built. Names
// must not be in the restricted set (Host, hop-by-hop, X-Forwarded-*, etc.).| // mergeForwardedHeaders returns a HeaderForwardConfig that combines the static | ||
| // backend configuration (base) with any per-request forwarded headers captured | ||
| // from the caller's request (see headerforward.CaptureMiddleware). | ||
| // | ||
| // Rules (applied in order): | ||
| // 1. If forwarded is empty, base is returned unchanged (no allocation, same | ||
| // pointer). | ||
| // 2. A new HeaderForwardConfig is built from a shallow copy of base so the | ||
| // shared target.HeaderForward is never mutated. | ||
| // 3. Forwarded header names are canonicalized via http.CanonicalHeaderKey and | ||
| // checked against middleware.RestrictedHeaders; restricted names are silently | ||
| // dropped (defense-in-depth — they were already filtered upstream, but we | ||
| // guard here too). | ||
| // 4. A forwarded header does NOT overwrite a name already present as an explicit | ||
| // static value in base.AddPlaintextHeaders (static config wins). | ||
| func mergeForwardedHeaders(base *vmcp.HeaderForwardConfig, forwarded map[string]string) *vmcp.HeaderForwardConfig { | ||
| if len(forwarded) == 0 { | ||
| return base | ||
| } |
There was a problem hiding this comment.
suggestion: Drop the "static config wins" precedence and fail loudly on collision instead. A header name appearing in both cfg.PassthroughHeaders and a backend's static HeaderForward (plaintext or secret) is an admin misconfiguration, not a precedence question — silently dropping one side hides the conflict, and the current wins-by-overwrite-ordering for secret-backed collisions is a latent bug waiting on someone trusting the comment.
Suggested shape: have mergeForwardedHeaders return (*HeaderForwardConfig, error) and error when a forwarded canonical name appears in base.AddPlaintextHeaders or base.AddHeadersFromSecret. Propagate through createMCPClient — the first session creation per misconfigured backend fails with a clear "forwarded header X collides with backend Y's static header-forward config" message, and the operator fixes the config.
|
|
||
| canonical := http.CanonicalHeaderKey(name) | ||
|
|
||
| if middleware.RestrictedHeaders[canonical] { |
There was a problem hiding this comment.
suggestion: Add Authorization to pkg/transport/middleware/header_forward.go's RestrictedHeaders map. Backend bearer tokens have a dedicated header_injection outgoing-auth strategy; allowing Authorization in HeaderForward / passthroughHeaders is a footgun (it silently survives to backends with the unauthenticated strategy) with no documented use case.
There was a problem hiding this comment.
Agreed the footgun is real, but I'd push back on blocking Authorization outright — I have a concrete use case that depends on it. I run a vMCP fronting gmail/calendar/drive MCPs behind LiteLLM's Zero Trust MCP setup (docs).
LiteLLM Zero Trust MCP works like this: the LiteLLM gateway signs a short-lived JWT on every MCP call so the downstream servers can verify the request genuinely came from LiteLLM (and which user). Concretely, its mcp_jwt_signer guardrail sends that JWT as Authorization: Bearer <jwt> (issuer=litellm, aud=mcp, 300s TTL). The flow:
LiteLLM (signs JWT) → vMCP (incomingAuth: anonymous — transparent aggregator)
→ passthrough authorization verbatim → backends validate the JWT (issuer/aud/JWKS)
vMCP isn't the enforcement point here by design — the backends are. They run the unauthenticated outgoing strategy because the caller supplies the token. Blocking Authorization in passthrough removes the only mechanism that makes this work, and this is in production for me, not hypothetical.
I think the real problem is the silence, not the header:
- authed backend → forwarded
Authorizationis silently clobbered byauthRoundTripper - unauthenticated backend → it silently survives (intended here, surprising elsewhere)
So rather than blocking, what if we fail loud on an actual collision? It lines up with your mergeForwardedHeaders comment: error when a passthrough header is also owned by the backend — its static HeaderForward, or its outgoing-auth strategy. The misconfig becomes a clear error, while the "anonymous vMCP → unauthenticated backend" passthrough stays valid since there's nothing backend-side to collide with.
Happy to keep the static-HeaderForward collision check in this PR and move the strategy-side check to a follow-up if that's cleaner. No objection to keeping Host/hop-by-hop/X-Forwarded-* restricted — it's specifically Authorization I'd like to keep allowed. What do you think?
| // the handler silently returning wrong data. | ||
| assert.Contains(t, text, absentSentinel, | ||
| "non-allowlisted header slot should contain the absent sentinel") | ||
| } |
There was a problem hiding this comment.
suggestion: Add a second CallTool on the same client to lock in the documented "captured once at session creation, stable for session lifetime" behavior. Right now a regression that re-reads forwarded headers on every request (or, worse, drops them after the first call) would pass this test. A second call with a changed X-Test-Api-Key value asserting the backend still sees the original would lock that contract.
| // Auth middleware composes auth + MCP parser. | ||
| // Order (outermost → innermost): authMiddleware → parser. The parser is | ||
| // innermost so downstream middleware (audit, authz) sees both the identity | ||
| // and the parsed MCP request. | ||
| composedAuth := func(next http.Handler) http.Handler { | ||
| withParser := mcp.ParsingMiddleware(next) | ||
| return authMiddleware(withParser) | ||
| return authMiddleware(mcp.ParsingMiddleware(next)) | ||
| } |
There was a problem hiding this comment.
nitpick: Refactor was supposed to leave this file at its pre-PR shape, but the comment got rewritten and the two-line form collapsed. Behavior is identical; consider reverting verbatim to keep this PR's diff scoped to the passthrough feature.
2 participants
Summary
When a client calls vMCP with a header its backend needs — e.g.
x-mcp-api-keythat the backend resolves to a specific user — vMCP silently drops it. vMCP opens a fresh request to each backend and sends only its own headers, so the caller's header never arrives. Backends that authenticate per-user from a header therefore can't be used through vMCP today — only by hitting the backend directly.This adds
spec.passthroughHeaders: an allowlist of incoming header names that vMCP forwards, unchanged, to every backend it calls.A listed header is read from the client request and attached to that session's backend requests. Headers not listed are ignored. Restricted names (
Host,Authorization,X-Forwarded-*, hop-by-hop) are rejected.Approach discussed and agreed with @jerm-dro on #3958 (assigned to me there).
Closes #3958
Type of change
Test plan
test/integration/vmcp/passthrough_headers_test.go): real client → capture → backend; asserts allowlisted header forwarded, non-allowlisted droppedgolangci-lintclean; CRD/deepcopy/docs regenerated (no drift)Does this introduce a user-facing change?
Yes:
Special notes for reviewers
*auth.Identityat the incoming-auth boundary (cloned, not mutated) and injected by the per-session backend client — it flows as an explicit parameter, not a context value, so novmcp-anti-patterns§1/§7 coupling and the round-tripper is unchanged. This is the no-context-plumbing path discussed in vMCP: add header_passthrough outgoing auth strategy to forward dynamic incoming headers to backends #3958 once the session refactor landed; thevmcp-reviewaudit was clean.(iss, sub)tuple is persisted — forwarded headers aren't, same as bearer tokens. Low impact withsessionAffinity: ClientIP.Large PR Justification
Most of the diff is required tests (~700 lines: unit + an in-process e2e) and generated code that can't be split (CRD manifests in two API versions × two chart copies, deepcopy, API docs). Hand-written source is ~240 lines. Already trimmed from ~1157 to ~1030 added lines by de-duplicating tests (coverage unchanged); the remainder is cohesive single-feature code that wouldn't be safe to split.