feat: protectedResourceMetadata.resource from the request instead of requiring it to be configured - #2691
feat: protectedResourceMetadata.resource from the request instead of requiring it to be configured#2691aish1331 wants to merge 17 commits into
Conversation
The OAuth Protected Resource Metadata document (RFC 9728) was served as a static HTTPRouteFilter direct response, so its "resource" identifier had to be known when the HTTPRoute was generated. That forced operators to configure protectedResourceMetadata.resource by hand, duplicating the gateway's own external URL, and made the field impossible to omit correctly: the control plane cannot see the scheme, authority or port a client will actually use. Serve the document from the MCP proxy instead. The well-known route rule now forwards to the shared MCP proxy Backend with the same x-ai-eg-mcp-route header the main rule sets, so the proxy resolves the route's OAuth configuration and computes the identifier from the request: X-Forwarded-Proto for the scheme, the preserved downstream authority for host and port, and the original path. The 403 insufficient_scope challenge is built the same way. resource becomes optional, and an explicitly configured value still wins, for deployments fronted by something that rewrites the external URL without forwarding headers. The 401 challenge is the one value the proxy cannot produce, since Envoy's JWT filter rejects the request before it is proxied. It uses an Envoy substitution format string, which Envoy Gateway passes through to the local response policy's response_headers_to_add. Signed-off-by: Aishwarya <aishraimule@gmail.com>
Unit tests for the derivation itself (scheme from the forwarded protocol, host and port from the authority, path from the request), for an explicitly configured resource still winning, and for the metadata document the MCP proxy now serves. One config is exercised against several hosts to pin down the property the old static document could not have: the same MCPRoute advertises the right identifier on every address it is reachable on. Also covers the upgrade path, where a stale HTTPRouteFilter left by an older controller is removed, and asserts that a client cannot steer the proxy by supplying x-ai-eg-original-path itself, since nothing strips that header from inbound requests. The e2e OAuth fixture now omits resource, so it exercises the derived path end to end: it reaches the gateway through a port-forward on a port chosen at run time, which no statically configured identifier could have matched. That also puts the one value this change cannot compute in Go, the 401 challenge Envoy expands from a substitution format string, under test in CI. mcp_route_authorization.yaml keeps resource pinned to cover the override path. Examples and the MCP capability docs drop the hand-written identifier, which in the Keycloak example was a hardcoded http://127.0.0.1:1975/mcp. Signed-off-by: Aishwarya <aishraimule@gmail.com>
✅ Deploy Preview for theagentrouter canceled.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Direction is right, and the 401 substitution works end to end per the e2e log. Three things before this can land:
|
| remoteJWKS: | ||
| uri: http://localhost:8080/realms/master/protocol/openid-connect/certs | ||
| protectedResourceMetadata: | ||
| resource: "http://127.0.0.1:1975/mcp" |
There was a problem hiding this comment.
nit: do not remove here, and add a new example?
| // The OAuth protected resource metadata document used to be served by an HTTPRouteFilter | ||
| // direct response. It is now served by the MCP proxy, which can derive the resource | ||
| // identifier from the request, so delete the filter left behind by an older version. | ||
| if delErr := c.deleteOAuthProtectedResourceMetadataHRF(ctx, mcpRoute); delErr != nil { | ||
| return fmt.Errorf("failed to delete legacy HTTPRouteFilter: %w", delErr) |
There was a problem hiding this comment.
nit: can add a todo to remove this block after 2.0?
| // When set, the MCP proxy serves the protected resource metadata document and includes | ||
| // a resource_metadata challenge in WWW-Authenticate headers it emits. | ||
| OAuth *MCPRouteOAuth `json:"oauth,omitempty"` |
There was a problem hiding this comment.
if this represents only PRM, can we rename it to ProtectedResourceMetadata, since the present name sounds confusing
| // * https://datatracker.ietf.org/doc/html/rfc9728#name-www-authenticate-response | ||
| func buildWWWAuthenticateHeaderValue(metadata *aigv1b1.ProtectedResourceMetadata) string { | ||
| resourceMetadataURL := buildResourceMetadataURL(metadata) | ||
| func buildWWWAuthenticateHeaderValue(metadata *aigv1b1.ProtectedResourceMetadata, servingPath string) string { | ||
| resourceMetadataURL := envoyDerivedResourceMetadataURL(servingPath) | ||
| if metadata.Resource != "" { | ||
| resourceMetadataURL = buildResourceMetadataURL(metadata.Resource) | ||
| } | ||
| headerValue := `Bearer error="invalid_token", error_description="The access token is missing or invalid"` |
There was a problem hiding this comment.
we already have the PRM metadata, so we should prefer metadata.Resource and only fall back to the derived URL when it's empty
| // Reference: https://www.envoyproxy.io/docs/envoy/latest/configuration/observability/access_log/usage#command-operators | ||
| func envoyDerivedResourceMetadataURL(servingPath string) string { | ||
| return fmt.Sprintf("%%REQ(X-FORWARDED-PROTO)%%://%%REQ(:AUTHORITY)%%%s%s", | ||
| oauthWellKnownProtectedResourceMetadataPath, strings.TrimSuffix(servingPath, "/")) | ||
| } |
There was a problem hiding this comment.
qq: if we already have the url info here, why not hardcode the same in the HRF without the mcpproxy impl this PR suggests?
is it because clients would then get the literal string:
%REQ(X-FORWARDED-PROTO)%://%REQ(:AUTHORITY)%/.well-known/oauth-protected-resource/mcp
inside the JSON of PRM response and envoy does not expand command operators in that body?
There was a problem hiding this comment.
Yes, that's exactly it.
Envoy expands command operators only where the config field is a format string: access log
formats, request_headers_to_add / response_headers_to_add values, and
local_reply_config.body_format. The 401 challenge works because it lands in the second of
those — Envoy Gateway funnels a ResponseOverride header value into the local response
policy's response_headers_to_add.
A direct response body is not in that set. HTTPRouteFilter.directResponse.body.inline
becomes direct_response.body, which is a DataSource (an opaque inline string), so
%REQ(:AUTHORITY)% would be served to the client verbatim inside the JSON.
Two further reasons it wouldn't work even if the body were templated:
-
The derivation has to exist in Go regardless. The 403
insufficient_scopechallenge is
emitted by the MCP proxy, not by Envoy. It used to read aResourceMetadataURLprecomputed
at config time from the pinnedresource; once the identifier is no longer static, the
proxy has to compute it from the request it is handling. SoresourceMetadataURL()exists
either way — serving the document from the proxy reuses it instead of maintaining a second,
templated copy of the same logic in the HTTPRouteFilter. -
Escaping. Splicing
%REQ(:AUTHORITY)%into a JSON string literal is an injection
surface: a quote inHostterminates the literal and the attacker picks the next JSON key —
authorization_servers, say. Envoy's JSON-safe interpolation lives in
SubstitutionFormatString.json_format, whichdirect_responsecannot use. On the other hand, building the
document in Go meansjson.Marshalescapes it. The e2e test withHost: evil"injected
pins this for both surfaces.
There was a problem hiding this comment.
Example for the JSON Injection Surface:
Concretely. The template the HTTPRouteFilter would carry (keys sorted, as json.Marshal of a
map emits them, with the identifier templated):
{"authorization_servers":["https://auth.example.com"],"bearer_methods_supported":["header"],"resource":"%REQ(X-FORWARDED-PROTO)%://%REQ(:AUTHORITY)%/mcp","resource_name":"example-resource","scopes_supported":["echo"]}An attacker sends:
GET /.well-known/oauth-protected-resource/mcp HTTP/1.1
Host: victim.example.com","authorization_servers":["https://evil.example.com"],"x":"
%REQ(:AUTHORITY)% is substituted literally, so the body on the wire becomes:
{"authorization_servers":["https://auth.example.com"],"bearer_methods_supported":["header"],"resource":"http://victim.example.com","authorization_servers":["https://evil.example.com"],"x":"/mcp","resource_name":"example-resource","scopes_supported":["echo"]}That is still syntactically valid JSON, which is what makes it nasty — nothing downstream
errors out. It just has authorization_servers twice, and every mainstream parser (Go's
encoding/json, JSON.parse, Python's json) is last-wins, so the client reads
["https://evil.example.com"] and runs its authorization-code flow against the attacker's
server.
resource sorting after authorization_servers is the whole reason the second declaration
wins. So whether this is exploitable depends on Go's map key ordering and on the parser's
duplicate-key policy — accidents, not defenses.
Building the document in Go (MCP Proxy Code) removes the question. The same Host produces:
{"resource":"http://victim.example.com\",\"authorization_servers\":[\"https://evil.example.com\"],\"x\":\"/mcp", ...}one string value, escaped by json.Marshal, no structural change.
NOTE: To be clear about today's state: Envoy rejects an authority containing " long before any of
this, so the attack does not land against either implementation — that is what the
Host: evil"injected e2e test asserts.
The point of doing it in Go is that Envoy's authority validation stops being the only thing standing between a request header and the contents of the discovery document.
| if r.Method == http.MethodOptions { | ||
| writeProtectedResourceMetadataCORSHeaders(w.Header()) | ||
| w.WriteHeader(http.StatusNoContent) | ||
| return | ||
| } |
There was a problem hiding this comment.
qq: this is purely for browser clients like mcp inspector?
| # resource is deliberately omitted: the gateway derives the identifier from the | ||
| # scheme, authority and path of each request. This test reaches the gateway through a | ||
| # port-forward on a port picked at run time, so no statically configured value could | ||
| # ever match. mcp_route_authorization.yaml pins resource to cover the override path. | ||
| resourceName: "example-resource" | ||
| scopesSupported: |
There was a problem hiding this comment.
nit: can let this be, and create another tc without resource url?
Signed-off-by: Aishwarya <aishraimule@gmail.com>
011dce7 to
76a0d25
Compare
Serving the protected resource metadata from the MCP proxy changed four things a client can observe, none of them intended. An MCPRoute that pins protectedResourceMetadata.resource must behave exactly as it did when the document was an HTTPRouteFilter direct response. CORS: ensureCORSHeaders advertised "GET" and "mcp-protocol-version". The handler had narrowed these to "GET, OPTIONS" and "content-type", which would reject the preflight of a browser MCP client sending mcp-protocol-version. Methods: the rule matched on path alone, so Envoy answered every method with the document. The handler had added a method gate returning 405, and a separate 204 path for OPTIONS. Trailing slash: the direct response emitted the configured value unchanged. The document now does the same. The challenge URL still normalizes it, as buildResourceMetadataURL always has, so the two agree on the resource while the document stays byte-identical to what the field was set to. Vary is added only when the identifier is derived from the request, since that is the only case where the body varies by Host and X-Forwarded-Proto. A pinned resource yields a request-independent response, exactly as before, and a shared cache in front of it stays correct without the header. Signed-off-by: Aishwarya <aishraimule@gmail.com>
…metadata The logic for setting the Vary header in the response for OAuth protected resource metadata has been refined. The Vary header now only includes "X-Forwarded-Proto" when the resource is not pinned, as the Host is already part of the effective request URI used for caching. This change ensures that the response remains consistent with previous behavior while improving cache efficiency. Additionally, the obsolete derivesFromRequest function has been removed to streamline the code. Signed-off-by: Aishwarya <aishraimule@gmail.com>
…into derive-resource-url
… routes This commit introduces a new test case to ensure that the gateway correctly rejects Host headers that could break out of the expected syntax. The test checks both the 401 challenge and the metadata document for potential injection vulnerabilities. A helper function, rawRequestWithHost, is also added to facilitate sending requests with specific Host headers while capturing the raw response for validation. Signed-off-by: Aishwarya <aishraimule@gmail.com>
…ix documentation to resolve comments Signed-off-by: Aishwarya <aishraimule@gmail.com>
Signed-off-by: Aishwarya <aishraimule@gmail.com>
…rive-resource-url Signed-off-by: Aishwarya <aishraimule@gmail.com>
Signed-off-by: Aishwarya <aishraimule@gmail.com>
…into derive-resource-url
Description
Problem
The OAuth Protected Resource Metadata document (RFC 9728) was served as a static
HTTPRouteFilterdirect response, so the resource identifier it advertises had to be knownwhen the HTTPRoute was generated. That forced
protectedResourceMetadata.resourceto be arequired field, with operators hand-writing their own external URL into the MCPRoute and
keeping it in sync by hand. It also made the field impossible to get right in ordinary cases:
the control plane cannot see the scheme, authority or port a client will actually use, so one
configured value is wrong as soon as the route is reachable on a second hostname, sits behind
a non-standard port, or is served over plain HTTP. The Keycloak example had to hardcode
resource: "http://127.0.0.1:1975/mcp"for exactly this reason.Change
protectedResourceMetadata.resourcebecomes optional. When omitted, the identifier isderived per request from the forwarded scheme, the authority and the path the client used, so
one MCPRoute stays correct on every hostname and port it is reachable on. An explicitly
configured value still wins, for deployments fronted by something that rewrites the external
URL without forwarding headers.
Three places advertise the identifier, and each derives it where it can:
X-Forwarded-Proto+Host+ request path403 insufficient_scopechallengeresourceMetadataURL401 invalid_tokenchallengeBackendTrafficPolicyresponse overrideThe 401 is the one value the proxy cannot compute: Envoy's JWT filter rejects the request
before it is ever proxied, so the
ResponseOverrideheader carries%REQ(X-FORWARDED-PROTO)%://%REQ(:AUTHORITY)%/.well-known/..., which Envoy Gateway passesthrough to the local response policy's
response_headers_to_add.Also in this PR
modifyMCPGatewayGeneratedClusterno longer matches on the/rule/0suffix. The newclusterTargetsMCPProxyBackendparses the cluster name and matches the name segment only.The endpoint cannot be inspected instead: a Backend with static IPs becomes an EDS cluster
with no inline load assignment. The authn-filter stripping in
maybeUpdateMCPRoutesstayskeyed on
rule/0specifically — widening it would re-apply JWT auth to the metadata endpointand break unauthenticated discovery.
filterapi.MCPRouteAuthorization.ResourceMetadataURLis replaced by a route-levelProtectedResourceMetadata, populated wheneversecurityPolicy.oauthis set, since thedocument is served independently of whether authorization rules exist.
resourcegets a byte-identical document, trailingslash included; the challenge URL keeps normalizing it as
buildResourceMetadataURLalwaysdid. The CORS headers and the "answer any method" behaviour of the old direct response are
preserved.
Vary: X-Forwarded-Protois set only in the derived case, since a pinned responseis request-independent.
that an RFC 8707 client sends the advertised identifier as the
resourceparameter, so aderived identifier has to agree with a statically configured
audienceslist.Related Issues/PRs (if applicable)
Fixes #2690
Related PR: #2676
Special notes for reviewers (if applicable)
Upgrade path: a cluster reconciled by an older controller has an
HTTPRouteFilterservingthe metadata as a direct response, and the route rule no longer references it.
ensureOAuthResourcesdeletes that superseded filter rather than leaving it dangling. There isa test for this, and the delete costs one cached Get per reconcile once nothing is left to
remove.
Please scrutinise the trust framing. Deriving the identifier from
HostandX-Forwarded-Protomeans the advertised value depends on request headers. Envoy overwritesX-Forwarded-Protofrom the downstream connection, but it does not sanitizeHost, so on alistener with no
hostnamethe advertised identifier reflects whatever authority the clientsent; even with a
hostname, Envoy ignores the port when matching and forwardsHostintact.The docs now say this explicitly and point operators at listener
hostname/ routehostnamesor at pinning
resource. Both interpolation sites rest on Envoy rejecting an authority thatcould break out of the surrounding syntax, so an e2e test sends a raw
Host: evil"injectedandasserts the gateway rejects it rather than reflecting it into the challenge or the JSON
document. Worth confirming this is the framing the project wants to commit to.
The 401
WWW-Authenticateheader is the only part of this change that cannot be verified by aunit test. It relies on Envoy expanding
%REQ(X-FORWARDED-PROTO)%and%REQ(:AUTHORITY)%ina header value that Envoy Gateway funnels into the local response policy's
response_headers_to_add. A unit test pins the exact string the controller emits, but only e2eproves the expansion happens, so please give that job's result particular weight. If the
expansion does not work, the header contains the literal format string rather than a URL, which
fails loudly rather than silently.
externalPathintentionally readsr.URL.Pathrather than thex-ai-eg-original-path/x-envoy-original-pathheaders this codebase normally uses for "the path the client sent."Those headers are only trustworthy outbound, where we set them; nothing strips them off
inbound requests, so honoring them would let a client pick which document the proxy serves and
what identifier that document advertises. They would also be redundant: the generated MCP route
rules carry no
URLRewritefilter, so the routed path is already the client's path.TestExternalPathasserts client-supplied values are ignored.tests/e2e/testdata/mcp_route_oauth.yamlnow deliberately omitsresource, so the e2e suiteexercises the derived path end to end: it reaches the gateway through a port-forward on a port
chosen at run time, which no statically configured identifier could ever have matched.
tests/e2e/testdata/mcp_route_authorization.yamlkeepsresourcepinned, so the override pathstays covered.