Allow opt-in credential header passthrough in vMCP - #6523
Open
juancarlosm wants to merge 1 commit into
Open
Conversation
juancarlosm
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
blkt,
jerm-dro,
jhrozek,
rdimitrov,
reyortiz3 and
tgrunnagle
as code owners
September 7, 2026 10:51
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6523 +/- ##
=======================================
Coverage 78.97% 78.97%
=======================================
Files 782 782
Lines 78015 78049 +34
=======================================
+ Hits 61612 61643 +31
- Misses 16398 16401 +3
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
juancarlosm
added a commit
to ackstorm/ach
that referenced
this pull request
Sep 9, 2026
v0.8.4 forwarded the public hostname to LiteLLM, but a compliant MCP client still could not complete discovery through ACH. Header propagation cannot fix this, for two upstream reasons measured on LiteLLM v1.99.1 (#177). 1. `PROXY_BASE_URL` short-circuits the X-Forwarded path entirely. `get_request_base_url` returns it BEFORE any X-Forwarded handling, so on any deployment that sets it -- needed for the admin UI, OAuth callbacks and spend links, so most of them -- `use_x_forwarded_for`, `mcp_trusted_proxy_ranges` and `X-Forwarded-Host` are unreachable code. Unsetting it is not a workaround either: an Ingress behind a TLS-terminating L4 load balancer truthfully reports http, trading a wrong host for a wrong scheme. 2. LiteLLM composes the challenge itself. Its pre-session probe loop discards the upstream's WWW-Authenticate and synthesizes a replacement, so a backend that correctly selects its own `resource` cannot reach the client at all. The header also has only one reachable consumer, and it is not the backend: toolhive's transparent proxy calls SetXForwarded() unconditionally and vMCP rejects X-Forwarded-* by design (stacklok/toolhive#6523). So the forwarder rewrites the challenge on the way out. It consumes no header -- it uses ACH_BASE_URL, already validated http(s):// at process start and used as the JWT `iss` -- which is what makes it immune to both problems above. Two branches converge on one invariant: a challenge leaving ACH names an ACH document, ALWAYS. Rewrite swaps scheme+authority when a `resource_metadata` pointer is present; insert appends one when it is absent on a /mcp/{name} or /a2a/{name} route. Insert is not housekeeping -- rewrite alone silently no-ops the day LiteLLM stops discarding the backend's challenge, leaving the client to derive the URL under RFC 9728 §3.1, which is legal but not dependable and fails the same silent way this issue opened with. Deliberately narrow, because the client FOLLOWS this URL and a broader rewrite would be a redirect-injection primitive: 401/403 only, `resource_metadata` only, RFC 9728 metadata paths only, Bearer only. Other auth-params survive byte for byte. An empty ACH_BASE_URL leaves the hook uninstalled. ModifyResponse was nil, documented as deliberate for streaming pass-through. The constraint that comment encoded is that the BODY is never touched, and it still is not -- the hook is header-only and buffers nothing. `newDepsWithUpstream` set no BaseURL, so the existing pass-through and SSE streaming tests would have run with ModifyResponse nil and covered none of this; it now sets one, so those tests exercise the hook rather than passing vacuously. Docs corrected in the same commit: §1.4's measured table carried a stale claim (it holds only when PROXY_BASE_URL is unset) and its "why a backend needs this" section now scopes itself to directly-proxied backends. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vMCP rejects Authorization and Cookie in passthroughHeaders unconditionally, which blocks legitimate zero-trust chains where a trusted upstream mints a per-backend, audience-scoped token that the backend verifies itself. Operators with that topology have no way to relay the caller's credential. Add spec.allowCredentialHeaderPassthrough, default false. It unblocks those two names only; Host, hop-by-hop, Transfer-Encoding, Content-Length and X-Forwarded-* stay rejected, since those are smuggling and spoofing vectors rather than a credential policy question. Validation stays at config-parse time, so a bad config still fails at startup. When enabled, vMCP warns at startup naming the credential headers it will forward, and marks every audit event whose request carried one with metadata.extra.credential_header_passthrough. Header names only; values are never recorded. Per-backend behaviour comes from the existing transport chain: the header-forward stage is outermost and skips headers already present, the auth stage is innermost and sets unconditionally, so a backend on a real strategy receives its own token and never the caller's. That ordering is now load-bearing, so document it at both ends and on the field itself. Closes stacklok#6394 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
juancarlosm
force-pushed
the
vmcp-allow-credential-header-passthrough
branch
from
September 11, 2026 08:52
de7008d to
37fb136
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AuthorizationandCookieinpassthroughHeadersunconditionally. That blocks zero-trust chains where a trusted upstream mints a per-backend, audience-scoped token which the backend verifies itself (e.g. LiteLLM's MCP zero-trust pattern). Operators with that topology have no way to relay the caller's credential at all.spec.allowCredentialHeaderPassthrough, a boolean defaulting tofalse, following the design @blkt specified in vMCP: passthroughHeaders blocks Authorization/Cookie even for backends whose outgoing-auth strategy sets nothing (unauthenticated) #6394. It unblocks those two names only —Host, hop-by-hop,Transfer-Encoding,Content-LengthandX-Forwarded-*stay rejected unconditionally, since those are smuggling and spoofing vectors rather than a credential policy question.validatePassthroughHeaders, so a config listing a credential header without the opt-in still fails at startup. No validation moved intoDiscover.metadata.extra.credential_header_passthrough. Header names only — values are never recorded. No new event type.Closes #6394
Type of change
Test plan
task test)task lint-fix)task testhas two failures on this branch, both pre-existing onmainand unrelated to this change (verified by running the same two packages in a clean worktree atHEAD):pkg/skills/identitytoken—TestResolve/neither_a_file_nor_JWT-shapedpkg/vmcp/optimizer/internal/similarity—TestOpenAIClient_Live(needs a liveOPENAI_API_KEY)New tests:
TestValidator_ValidatePassthroughHeaders— both credential headers rejected without the opt-in, accepted with it, andX-Forwarded-For/Transfer-Encodingstill rejected with the opt-in on.TestConverter_AllowCredentialHeaderPassthrough— promotion from either level, and that the promoted field never disables the config-level one.TestAuditorCredentialPassthroughMetadata— the marker appears only when the request actually carried the header, matches non-canonical configuration, and never contains the credential value.API Compatibility
v1beta1API, OR theapi-break-allowedlabel is applied and the migration guidance is described above.Additive only: one new optional boolean field with a
falsezero value, which is the current behaviour.Changes
pkg/vmcp/config/validator.govmcpRestrictedHeaders→vmcpCredentialHeaders, split out of the unconditionalmiddleware.RestrictedHeaderscheck and gated on the new flag. AddsCredentialPassthroughHeaders().pkg/vmcp/config/config.goAllowCredentialHeaderPassthroughfield.cmd/thv-operator/api/v1beta1/virtualmcpserver_types.gospec.allowCredentialHeaderPassthrough.cmd/thv-operator/pkg/vmcpconfig/converter.goallowCredentialHeaderPassthrough(vmcp)— either level enables, neither disables the other.pkg/vmcp/server/server.gopkg/audit/auditor.go,pkg/audit/mcp_events.goAuditorOption+WithCredentialPassthroughHeaders;addMetadatarecords the marker.pkg/vmcp/headerforward/transport.go,pkg/vmcp/session/internal/backend/mcp_session.godocs/operator/virtualmcpserver-api.mdpassthroughHeaderssection, which saidAuthorizationis always rejected.docs/operator/crd-api.mdtask operator-manifests,task crdref-gen).Does this introduce a user-facing change?
Yes.
VirtualMCPServergainsspec.allowCredentialHeaderPassthrough(defaultfalse). With it set,AuthorizationandCookiemay be listed inspec.passthroughHeaders; without it they are rejected at startup exactly as today.The rejection message for those two headers changes from:
to:
The message for genuinely restricted headers is unchanged.
Special notes for reviewers
On the nesting invariant. Per @blkt's note, the correctness of this flag rests on
headerForwardRoundTripperbeing constructed last (outermost) and skipping headers already present, whileauthRoundTripperis constructed first (innermost) andSets unconditionally. A backend ontoken_exchangetherefore receives the exchanged token and never the caller's; only backends resolving tounauthenticatedreceive the forwarded value. That was an implementation detail before this PR and is load-bearing after it, so it is documented in three places: the construction site inmcp_session.go, the skip site intransport.go, and the field's own doc comment on both the CRD and the config struct. A refactor that reorders those stages, or that makes the forward tripper overwrite rather than skip, silently converts this flag into a credential leak to every backend.The runtime needed no changes.
middleware.RestrictedHeadersnever containedAuthorization/Cookie, andpkg/vmcp/headerforward/transport.goonly ever consulted that set.vmcpRestrictedHeadersin the validator was the sole gate, so gating it is the entire behavioural change.Two config levels. The field also exists at
spec.config.allowCredentialHeaderPassthrough. That is not a second surface by choice — the field has to live inpkg/vmcp/config.Configfor validation and for standalonethv vmcp serve, and the operator embeds that struct asspec.config. Same shapepassthroughHeadersalready has. The promoted field ORs with the embedded one so neither can silently switch the other off.Three
//nolintdirectives, each with existing precedent in the repo:lllon the struct tag line,gosecG101 on the audit metadata key name (credential_header_passthroughtrips the credential heuristic), and the OR moved into a small helper becauseConverter.Convertwas already at the gocyclo limit of 15.Not included, deliberately: no CEL admission rule for this field. It would turn a startup CrashLoopBackOff into an admission rejection, which is nicer, but @blkt asked that startup validation be preserved and it is out of scope here. Happy to follow up separately if wanted.
🤖 Generated with Claude Code