Skip to content

fix(controller): recover MCP auth session from RequestExtra in tool handlers#1853

Closed
onematchfox wants to merge 1 commit into
kagent-dev:mainfrom
onematchfox:fix-mcp-auth
Closed

fix(controller): recover MCP auth session from RequestExtra in tool handlers#1853
onematchfox wants to merge 1 commit into
kagent-dev:mainfrom
onematchfox:fix-mcp-auth

Conversation

@onematchfox

@onematchfox onematchfox commented May 12, 2026

Copy link
Copy Markdown
Contributor

The Go MCP SDK detaches the HTTP request context before dispatching to tool handlers. From the SDK source:

// Pass req.Context() here, to allow middleware to add context values.
// The context is detached in the jsonrpc2 library when handling the
// long-running stream.

This means the auth session placed by AuthnMiddleware is not visible via auth.AuthSessionFrom(ctx) in tool handlers.

The SDK does preserve the original HTTP headers in RequestExtra.Header though.

Re-authenticate from those headers at the top of handleInvokeAgent so the A2A client's outbound request to the agent carries the user's JWT.

Copilot AI review requested due to automatic review settings May 12, 2026 14:00
@github-actions github-actions Bot added the bug Something isn't working label May 12, 2026
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels May 12, 2026
@onematchfox onematchfox changed the title fix(mcp): recover auth session from RequestExtra in tool handlers fix(controller): recover MCP auth session from RequestExtra in tool handlers May 12, 2026
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels May 12, 2026

Copilot AI left a comment

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.

Pull request overview

This PR addresses an auth-context propagation gap in the Go MCP Streamable HTTP handling path: tool handlers lose the original HTTP request context (and thus the auth.Session), so invoke_agent re-authenticates using the preserved request headers in CallToolRequest.Extra to ensure downstream A2A calls can forward the user’s JWT.

Changes:

  • Recover auth.Session inside handleInvokeAgent by re-authenticating from RequestExtra.Header, then attach it to the handler context.
  • Add unit tests validating Authorization propagation to the A2A backend when an MCP client supplies (or omits) an Authorization header.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
go/core/internal/mcp/mcp_handler.go Re-authenticates from RequestExtra.Header to restore the session in tool-handler context for downstream A2A auth propagation.
go/core/internal/mcp/mcp_handler_test.go Adds end-to-end-ish unit tests covering auth propagation behavior through MCP → A2A.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +186 to +194
// The Go MCP SDK detaches the HTTP request context when dispatching to
// tool handlers, so auth.AuthSessionFrom(ctx) returns nothing. Recover
// the auth session from the HTTP headers preserved in RequestExtra so
// that the A2A client's outbound request to the agent carries the user's JWT.
if extra := req.GetExtra(); extra != nil {
if session, err := h.authenticator.Authenticate(ctx, extra.Header, nil); err == nil {
ctx = auth.AuthSessionTo(ctx, session)
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

RequestExtra doesn't carry query params

Comment thread go/core/internal/mcp/mcp_handler_test.go
Comment thread go/core/internal/mcp/mcp_handler_test.go Outdated
… handlers

The Go MCP SDK detaches the HTTP request context before dispatching to tool handlers. From the [SDK source](https://github.com/modelcontextprotocol/go-sdk/blob/v1.5.0/mcp/streamable.go#L485-L487):

>  // Pass req.Context() here, to allow middleware to add context values.
>  // The context is detached in the jsonrpc2 library when handling the
>  // long-running stream.

This means the auth session placed by `AuthnMiddleware` is not visible via `auth.AuthSessionFrom(ctx)` in tool handlers.

The SDK does preserve the original HTTP headers in [RequestExtra.Header](https://github.com/modelcontextprotocol/go-sdk/blob/v1.5.0/mcp/streamable.go#L1155-L1158) though.

Re-authenticate from those headers at the top of handleInvokeAgent so the A2A client's outbound request to the agent carries the user's JWT.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>

@supreme-gg-gg supreme-gg-gg left a comment

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.

I tried testing the MCP handler before and after the change, but both seem to work fine with propagating auth context to a2a agent invocations (trusted proxy mode)

I asked copilot to take a look at the mcp go code and he told me this:

The conclusion is wrong because “detached context” here does not mean context values are dropped.

The SDK comment says it passes req.Context() specifically “to allow middleware to add context values”:


streamable.go
Lines 485-488
// Pass req.Context() here, to allow middleware to add context values.
// The context is detached in the jsonrpc2 library when handling the
// long-running stream.
session, err := server.Connect(req.Context(), transport, connectOpts)

The actual detach wrapper preserves values:

conn.go
Lines 758-767
type notDone struct{ ctx context.Context }
func (ic notDone) Value(key any) any {
	return ic.ctx.Value(key)
}
func (notDone) Done() <-chan struct{}       { return nil }
func (notDone) Err() error                  { return nil }
func (notDone) Deadline() (time.Time, bool) { return time.Time{}, false }

Also in: https://github.com/modelcontextprotocol/go-sdk/blob/v1.5.0/internal/xcontext/xcontext.go#L14-L15

So AuthnMiddleware context values should remain visible to tool handlers. The current test was misleading because it mounted MCPHandler directly, bypassing the real /mcp auth middleware path.

If this has been a valid issue can you lmk how was this not working before the change or how did you set this up?

mcpHandler, err := NewMCPHandler(nil, backend.server.URL, authProvider, 5*time.Second)
require.NoError(t, err)

mcpServer := httptest.NewServer(mcpHandler)

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.

These tests were passing without your re-authenticate change if you add the auth middleware

mcpServer := httptest.NewServer(auth.AuthnMiddleware(authProvider)(mcpHandler))

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hey @supreme-gg-gg - thanks for pointing this out. You're completely right - the original root cause was wrong. I originally ran into this when trying to set up access to the agents MCP via agentgateway but I think the actual underlying issue in my case was most likely #1855 since the token used to access the MCP gateway domain (which is then passed on to the container) is not valid for accessing KAgent controller (via oauth2-proxy) which is what ended up happening without the change in #1855.

This code was my first attempt to fix this and I thought the tests proved the issue but as you correctly point out, the tests didn't attach the auth middleware, which meant they were testing a code path that doesn't exist in reality and, were passing (or rather failing 😄) for the wrong reasons.

I'm going to go ahead and close this out...

@onematchfox onematchfox deleted the fix-mcp-auth branch May 25, 2026 09:40
EItanya added a commit that referenced this pull request May 27, 2026
Replace the HTTP round-trip through the controller's own A2A listener
with direct invocation via a new `AgentClientRegistry`. The registry is
owned by `A2ARegistrar`, which already maintains an `A2AClient` per
agent for its HTTP mux — the registry gives the MCP handler access to
those same clients without an extra network hop.

The old approach routed through the controller's public A2A endpoint,
meaning requests could traverse the external network (and any ingress or
load-balancer in front of it) unnecessarily. The new path stays
in-process.

The old handler also cached its own `A2AClient` per agent in a
`sync.Map` with no eviction, so clients for deleted agents would remain
indefinitely. The registry is kept consistent by the registrar's
add/update/delete lifecycle, eliminating that staleness.

`A2ARegistrar.upsertAgentHandler` writes to both the HTTP mux (for
inbound /api/a2a/<ns>/<name>/ routing) and the registry (for direct
invocation). The registry is exposed via `ClientRegistry()` and passed
to `NewMCPHandler` in app.go.

~Note:~
~- currently includes changes from #1853 as well since this needs to
adjust the tests added in that PR.~

---------

Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Eitan Yarmush <eitan.yarmush@solo.io>
EItanya added a commit that referenced this pull request Jun 3, 2026
…1856)

Adds a `kagent://agents` [MCP
resource](https://modelcontextprotocol.io/specification/2025-06-18/server/resources)
that lists agents as JSON. Clients can read the resource directly or
subscribe to receive notifications when the agent list changes. I've
left the existing `list_agents` tool in place for now to not break
existing consumers although this can potentially be removed prior to the
next breaking version?

Notes: 
- Contains changes from both #1853 and #1855 at present. Diff will clean
up as those get reviewed.
- Post this change we can also adjust the implementation of list agents
to use the registry introduced in #1855 to avoid needing to query Kube
API every time someone calls the endpoint.

---------

Signed-off-by: Brian Fox <878612+onematchfox@users.noreply.github.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Eitan Yarmush <eitan.yarmush@solo.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants