Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 32 additions & 1 deletion pkg/vmcp/session/internal/backend/mcp_session.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,32 @@ func (i *identityRoundTripper) RoundTrip(req *http.Request) (*http.Response, err
return i.base.RoundTrip(req)
}

// claimInjectionRoundTripper injects authenticated user identity claims as HTTP headers
// so backend MCP servers can identify the user without OAuth token introspection.
//
// Headers injected when identity is present:
// - X-User-Sub: the authenticated user's subject claim (Google/OIDC sub)
// - X-User-Email: the user's email address (if present in token)
// - X-User-Name: the user's display name (if present in token)
type claimInjectionRoundTripper struct {
base http.RoundTripper
identity *auth.Identity
}

func (c *claimInjectionRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
cloned := req.Clone(req.Context())
if c.identity.Subject != "" {
cloned.Header.Set("X-User-Sub", c.identity.Subject)
}
if c.identity.Email != "" {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Email (and Name a few lines down) are PII. As written, this sends them to every backend unconditionally whenever an identity is present. A backend that only needs sub for authorization still gets the user's email — which then very likely ends up in that backend's request logs.

If the feature stays in this form, defaulting to sub only and requiring explicit opt-in for email/name would be much better data-minimization. If it moves to a per-backend strategy (see the other comment), the claim set should be configurable there anyway.

cloned.Header.Set("X-User-Email", c.identity.Email)
}
if c.identity.Name != "" {
cloned.Header.Set("X-User-Name", c.identity.Name)
}
return c.base.RoundTrip(cloned)
}

// Compile-time assertion: mcpSession must implement Session.
var _ Session = (*mcpSession)(nil)

Expand Down Expand Up @@ -259,7 +285,7 @@ func createMCPClient(

slog.Debug("Applied authentication strategy", "strategy", strategy.Name(), "backendID", target.WorkloadID)

// Build shared transport chain: auth → identity propagation.
// Build shared transport chain: auth → identity propagation → claim injection.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heads up — this comment is the inverse of the actual execution order. http.RoundTripper chains wrap inward, so the outermost wrapper runs first on the outgoing request. As wired below, an outgoing request goes claimInjectionRoundTripperidentityRoundTripperauthRoundTripperhttp.DefaultTransport. So the real order is "claim injection → identity propagation → auth", not the other way round.

Either flip the order in the comment, or add a sentence clarifying that wrap order is the reverse of execution order.

// The per-transport sections below may add a size-limiting wrapper on top.
base := http.RoundTripper(http.DefaultTransport)
base = &authRoundTripper{
Expand All @@ -269,6 +295,11 @@ func createMCPClient(
target: target,
}
base = &identityRoundTripper{base: base, identity: identity}
// Inject user identity as HTTP headers so backend MCP servers can read
// X-User-Sub / X-User-Email without needing their own /introspect calls.
if identity != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The commit message says "When no identity is present in context (e.g. anonymous mode), no headers are injected" — but anonymous identity isn't nil. pkg/auth/anonymous.go builds a real *Identity with Subject="anonymous", Email="anonymous@localhost", Name="Anonymous User". So this if identity != nil guard passes in anonymous mode and the backend ends up with X-User-Sub: anonymous, etc.

Worth fixing one way or the other — either gate explicitly here (e.g. add an IsAnonymous() helper on *Identity and check !identity.IsAnonymous()), or update the commit message. The combination of implicit network trust + anonymous user looking like a real principal at the backend is the bit that worries me.

base = &claimInjectionRoundTripper{base: base, identity: identity}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vmcp already has a per-backend outgoing-auth strategy registry — see the constants in pkg/vmcp/auth/types/types.go (unauthenticated, header_injection, token_exchange, upstream_inject, aws_sts) and the strategy resolved a few lines up at strategy, err := registry.GetStrategy(strategyName). This change is a parallel header-mutation path that runs unconditionally for every backend regardless of which strategy is selected.

Think about a setup with github-tools + atlassian-tools via upstream_inject and an internal-api via header_injection. With this code, the GitHub and Atlassian backends also get X-User-Sub / X-User-Email / X-User-Name, even though they're authenticating with a real upstream token and don't need (or really want) those headers — the headers describe the vmcp user, not the upstream service principal.

Could this be a new strategy variant instead? Either fold a fromClaim: source into header_injection, or add a separate claim_injection strategy. That way backends opt in and the claim → header mapping is configurable per backend.

}

var c *mcpclient.Client
switch target.TransportType {
Expand Down
88 changes: 88 additions & 0 deletions pkg/vmcp/session/internal/backend/roundtripper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -246,3 +246,91 @@ func TestIdentityRoundTripper_WithIdentity_ClonesRequest(t *testing.T) {
require.NotNil(t, base.received)
assert.NotSame(t, orig, base.received, "non-nil identity should clone the request")
}

// ---------------------------------------------------------------------------
// claimInjectionRoundTripper
// ---------------------------------------------------------------------------

func TestClaimInjectionRoundTripper_AllFields_InjectsHeaders(t *testing.T) {
t.Parallel()

identity := &auth.Identity{PrincipalInfo: auth.PrincipalInfo{
Subject: "108352771234567890",
Email: "user@example.com",
Name: "Test User",
}}
base := &okTransport{}
rt := &claimInjectionRoundTripper{base: base, identity: identity}

orig := newTestRequest(context.Background(), t)
resp, err := rt.RoundTrip(orig)

require.NoError(t, err)
assert.Equal(t, http.StatusOK, resp.StatusCode)

require.NotNil(t, base.received)
assert.Equal(t, "108352771234567890", base.received.Header.Get("X-User-Sub"))
assert.Equal(t, "user@example.com", base.received.Header.Get("X-User-Email"))
assert.Equal(t, "Test User", base.received.Header.Get("X-User-Name"))
}

func TestClaimInjectionRoundTripper_EmptyEmail_DoesNotInjectEmailHeader(t *testing.T) {
t.Parallel()

identity := &auth.Identity{PrincipalInfo: auth.PrincipalInfo{
Subject: "sub-only",
// Email and Name intentionally omitted.
}}
base := &okTransport{}
rt := &claimInjectionRoundTripper{base: base, identity: identity}

orig := newTestRequest(context.Background(), t)
_, err := rt.RoundTrip(orig)
require.NoError(t, err)

require.NotNil(t, base.received)
assert.Equal(t, "sub-only", base.received.Header.Get("X-User-Sub"), "X-User-Sub must be set")
assert.Empty(t, base.received.Header.Get("X-User-Email"), "X-User-Email must not be set when empty")
assert.Empty(t, base.received.Header.Get("X-User-Name"), "X-User-Name must not be set when empty")
}

func TestClaimInjectionRoundTripper_EmptySubject_DoesNotInjectSubHeader(t *testing.T) {
t.Parallel()

identity := &auth.Identity{PrincipalInfo: auth.PrincipalInfo{
// Subject intentionally omitted.
Email: "user@example.com",
}}
base := &okTransport{}
rt := &claimInjectionRoundTripper{base: base, identity: identity}

orig := newTestRequest(context.Background(), t)
_, err := rt.RoundTrip(orig)
require.NoError(t, err)

require.NotNil(t, base.received)
assert.Empty(t, base.received.Header.Get("X-User-Sub"), "X-User-Sub must not be set when subject is empty")
assert.Equal(t, "user@example.com", base.received.Header.Get("X-User-Email"))
}

func TestClaimInjectionRoundTripper_ClonesRequest_OriginalUnmodified(t *testing.T) {
t.Parallel()

identity := &auth.Identity{PrincipalInfo: auth.PrincipalInfo{
Subject: "clone-test",
Email: "clone@example.com",
}}
base := &okTransport{}
rt := &claimInjectionRoundTripper{base: base, identity: identity}

orig := newTestRequest(context.Background(), t)
_, err := rt.RoundTrip(orig)
require.NoError(t, err)

// The forwarded request must be a distinct clone, not the original.
require.NotNil(t, base.received)
assert.NotSame(t, orig, base.received, "claimInjectionRoundTripper must clone the request")

// The original request must not be mutated.
assert.Empty(t, orig.Header.Get("X-User-Sub"), "original request header must not be mutated")
}