Repository navigation
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 40 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 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
aish1331
changed the title
missBerg
added
enhancement
|
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?
There was a problem hiding this comment.
Done. Both mcp_oauth_keycloak.yaml and mcp_oauth_example.yaml keep their pinned resource values as on main. The Keycloak example now also has a second MCPRoute, mcp-route-derived-resource on /mcp-derived, with resource omitted, so the same aigw run shows the derived identifier next to the pinned one. The README mentions the new route.
| // 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
| // 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.
There was a problem hiding this comment.
This format string has no safe degradation, and it is the only path unauthenticated clients ever see. The 401 is what every client hits before it has a token, so a failure here breaks discovery completely rather than partially.
Two ways it degrades badly:
- No
X-Forwarded-Proto. Envoy's substitution formatter renders an absent header as-, so the client receivesresource_metadata="-://host/.well-known/...". Note the Go path ininternal/mcpproxy/oauth.go:45-63has ar.TLSfallback for exactly this; the Envoy path has nothing equivalent. - No expansion. If the value is not run through a substitution formatter, the client receives the literal
%REQ(X-FORWARDED-PROTO)%://%REQ(:AUTHORITY)%/.... The path does exist in the pinned EG version (envoyproxy/gateway@v1.9.0 internal/xds/translator/custom_response.go:586→route.go:695 buildXdsAddedHeaders→LocalResponsePolicy.ResponseHeadersToAdd), but this repo supports several EG versions and only a running Envoy proves it, as you say in the PR body.
The e2e assertion on fwd.Address() (the 401 challenge subtest under resource identifier derived from the request) is the right guard and should be kept running against every supported EG version, not just the newest.
There was a problem hiding this comment.
The format string now uses Envoy's alternate-header syntax, %REQ(X-FORWARDED-PROTO?:SCHEME)%, so if X-Forwarded-Proto is missing the scheme comes from the request's :scheme pseudo-header and never renders as -. That brings the Envoy path in line with the r.TLS fallback in externalScheme on the Go side. In practice the header should always be there: Envoy's connection manager sets X-Forwarded-Proto on the request before the filter chain runs, so the JWT filter's 401 already sees it. The fallback is a backstop.
On expansion across versions: the 401 challenge subtest under resource identifier derived from the request checks the expanded resource_metadata against fwd.Address(), has no version skip, and the E2E matrix runs it on Envoy Gateway latest, v1.9.2 and v1.8.5. If any supported version stopped expanding the operators, the client would see the literal format string and that job would fail.
| 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
| // 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.
This format string has no safe degradation, and it is the only path unauthenticated clients ever see. The 401 is what every client hits before it has a token, so a failure here breaks discovery completely rather than partially.
Two ways it degrades badly:
- No
X-Forwarded-Proto. Envoy's substitution formatter renders an absent header as-, so the client receivesresource_metadata="-://host/.well-known/...". Note the Go path ininternal/mcpproxy/oauth.go:45-63has ar.TLSfallback for exactly this; the Envoy path has nothing equivalent. - No expansion. If the value is not run through a substitution formatter, the client receives the literal
%REQ(X-FORWARDED-PROTO)%://%REQ(:AUTHORITY)%/.... The path does exist in the pinned EG version (envoyproxy/gateway@v1.9.0 internal/xds/translator/custom_response.go:586→route.go:695 buildXdsAddedHeaders→LocalResponsePolicy.ResponseHeadersToAdd), but this repo supports several EG versions and only a running Envoy proves it, as you say in the PR body.
The e2e assertion on fwd.Address() (the 401 challenge subtest under resource identifier derived from the request) is the right guard and should be kept running against every supported EG version, not just the newest.
| if metadata.Resource != "" { | ||
| resourceMetadataURL = buildResourceMetadataURL(metadata.Resource) | ||
| } else { | ||
| resourceMetadataURL = envoyDerivedResourceMetadataURL(servingPath) |
There was a problem hiding this comment.
Three code paths now choose between a pinned and a derived identifier, each with its own copy of the Resource != "" test and its own URL-splicing logic: here, resourceIdentifier (internal/mcpproxy/oauth.go:79-87) and resourceMetadataURL (internal/mcpproxy/oauth.go:93-110). The splicing in buildResourceMetadataURL above and in resourceMetadataURL is the same algorithm written twice, in two packages.
Currently the controller cannot import mcpproxy and one of the three has to emit Envoy operators rather than a URL, but it means a future change to how the well-known path is spliced has to be made in two places, and the tests that pin them (Test_buildWWWAuthenticateHeaderValue and TestResourceMetadataURL) live in different packages and could drift apart silently. A shared helper in a package both can import, or at minimum a comment in each pointing at the other, would make the coupling visible.
There was a problem hiding this comment.
moved the common code to a helper in internalapi package
| // filter exists, so it costs a single cached Get per reconcile after an upgrade. | ||
| func (c *MCPRouteController) deleteOAuthProtectedResourceMetadataHRF(ctx context.Context, mcpRoute *aigv1b1.MCPRoute) error { | ||
| var httpRouteFilter egv1a1.HTTPRouteFilter | ||
| name := oauthProtectedResourceMetadataName(mcpRoute.Name) |
There was a problem hiding this comment.
[nit] This is upgrade-path code that will run forever. The comment is right that it costs one cached Get per reconcile, which is cheap, but nothing records when it can be deleted. Can you just add a TODO comment to remove it in version X, or even mark the method as deprecated so we notice it in future releases and remember to remove?
There was a problem hiding this comment.
I have added a code comment to deprecate this after 2.0. This is tricky because if user upgrades from <=1.2 to 2.1, they will end up with a stale HRF resource. The resource is harmless in itself since it is not referenced. But deprecating after 2.0 release (which should be atleast a few months away) is alright since most active users would have run atleast one upgrade by then. Wdyt?
| // Resource, when non-empty, pins the resource identifier instead of deriving it from the | ||
| // request. Set it only when the externally visible URL cannot be recovered from the | ||
| // request, e.g. behind a proxy that rewrites the authority without forwarding headers. | ||
| Resource string `json:"resource,omitempty"` |
There was a problem hiding this comment.
compiledAuthorization.same no longer compares ResourceMetadataURL (internal/mcpproxy/authorization.go:47-53), and prm is not compared by sameTools (internal/mcpproxy/config.go:121-149) either. sameTools drives the notifications/tools/list_changed signal, so leaving OAuth metadata out of it is correct. A changed issuer is not a tool change. But it does mean nothing in the reload path compares prm at all. That is fine today because LoadConfig replaces the route wholesale (config.go:287-291), so the new value takes effect regardless; it is worth a one-line comment on prm saying so, since the neighbouring fields are all part of a comparison and this one deliberately is not.
There was a problem hiding this comment.
done, added a comment
|
|
||
| // TestServePOST_InitializeRequest_NegotiatesProtocolVersion verifies that the gateway | ||
| // returns max(protocolVersion20250618, min(clientVersion, min(backends))) end-to-end. | ||
| func TestServePOST_InitializeRequest_NegotiatesProtocolVersion(t *testing.T) { |
There was a problem hiding this comment.
This test is present at the merge-base (7d7c07ff) and on current upstream/main, and nothing in this PR touches protocol negotiation. mergedProtocolVersion, protocolVersion20260728 and the initialize handshake are all untouched here.
This is almost certainly a mis-resolved merge from main: the branch carries several Merge branch 'main' commits. The remaining coverage in internal/mcpproxy/modern_test.go:1213,1315,1375 exercises mergedProtocolVersion as a unit, but not the end-to-end servePOST path this test covered. So the deletion is a silent, unrelated coverage regression that CI will not flag.
Restore both the test and the BackendSelectorDenied doc comment, and re-check the rest of the diff for other collateral from the same merge.
There was a problem hiding this comment.
reverted and verified there is no collateral from that merge
| // byte-identical document before and after this change. | ||
| return prm.Resource | ||
| } | ||
| identifier := externalScheme(r) + "://" + r.Host + resourcePath |
There was a problem hiding this comment.
r.Host goes into the advertised RFC 9728 resource identifier with no validation and no allow-list. The doc update is honest about this (site/docs/capabilities/mcp/index.md:368: Envoy does not sanitize Host, ignores the port when matching a listener, and a wildcard hostname admits any subdomain), and an operator can pin resource. But the code has no defence of its own, so the safe configuration is entirely a documentation contract.
The new e2e (identifier tracks the request authority) sends well-formed foreign authorities (api.example.com, tenant-b.example.com:8443) and asserts a 200 that advertises them, so it pins this as intended behaviour. Concretely: on a listener with no hostname, Host: attacker.example.com yields a 200 document advertising resource: "https://attacker.example.com/mcp" from an endpoint that requires no token to reach.
What that buys an attacker is narrow. authorization_servers still comes from config, so the AS cannot be redirected, and a spec-conforming client rejects an identifier that does not match the URL it used. But it is unauthenticated, attacker-controlled content in a security document, and there is a cheap fix: when prm.Resource is empty, check the request authority against the MCPRoute's hostnames (already carried onto the HTTPRoute at internal/controller/mcp_route.go:484) and fall back to 404 or to the configured host on a mismatch. At minimum, add a case asserting what happens for a Host outside the MCPRoute's hostnames, so the behaviour is a deliberate decision rather than an untested side effect.
There was a problem hiding this comment.
The well-known rule is on the same generated HTTPRoute as the MCP endpoint and gets spec.hostnames, so Envoy's host matching already rejects a Host outside the list before the proxy sees it. The new e2e test 'derived identifier is limited to the route hostnames' shows this. A route without hostnames is reachable on every host its listener accepts, and the identifier reflects whichever one was used; the Resource field docs now say so.
| // Both mean there is no metadata document to serve for it. | ||
| m.l.Debug("no OAuth configuration for MCP route, not serving protected resource metadata", | ||
| "route", routeName, "path", externalPath(r)) | ||
| http.NotFound(w, r) |
There was a problem hiding this comment.
This 404 is a new failure mode. Previously the document was an Envoy direct response: once the HTTPRoute was programmed it was served unconditionally, with no dependency on the extproc process or on filter-config propagation. Now the endpoint returns 404 whenever m.routes[routeName] is not yet populated. During an extproc restart, or in the window between the HTTPRoute being programmed and LoadConfig receiving the config (internal/mcpproxy/config.go:287-291).
A client that gets a 404 from /.well-known/oauth-protected-resource treats the resource as having no metadata and abandons the OAuth flow; it does not retry the way it would on a 503. Returning 503 for "route unknown to this proxy instance" and reserving 404 for "route exists but configures no OAuth" would let clients back off correctly, and the two cases are already distinguishable here. m.routes[routeName] == nil versus route.prm == nil.
This is worth a line in the release notes regardless: the discovery endpoint's availability is now coupled to the extproc sidecar, which it was not before.
route := m.routes[routeName]
if route == nil {
// Config not loaded yet (or no longer). Tell the client to retry rather than
// letting it conclude the resource has no metadata.
http.Error(w, "service unavailable", http.StatusServiceUnavailable)
return
}| // honoring it would let a client choose which document the proxy serves and which path the | ||
| // advertised resource identifier names. | ||
| func externalPath(r *http.Request) string { | ||
| return r.URL.Path |
There was a problem hiding this comment.
r.URL.Path is the percent-decoded path, and it is spliced verbatim into the resource value the client is required to compare for exact string equality. A client that reached /.well-known/oauth-protected-resource/ten%61nt/mcp is handed back .../tenant/mcp and the comparison fails on its side.
In practice Envoy exact-matches the well-known path (internal/controller/mcp_route.go:358-361) so the decoded and raw forms normally coincide. But r.URL.EscapedPath() reproduces the bytes the client sent and removes the coupling between what isProtectedResourceMetadataRequest matched and what strings.TrimPrefix later slices. Worth doing while the function is one line.
func externalPath(r *http.Request) string {
return r.URL.EscapedPath()
}| // needs no Vary: it is already part of the effective request URI a cache keys on. | ||
| // Omitted when resource is pinned: that response is request-independent, exactly as | ||
| // the static direct response was, and the header would itself be a behaviour change. | ||
| h.Set("Vary", "X-Forwarded-Proto") |
There was a problem hiding this comment.
The reasoning that Host needs no Vary is correct for an RFC 9111 cache, which keys on the effective request URI. It does not hold for the deployment shape this feature is aimed at: a CDN or shared front-door in front of a single-hostname gateway commonly keys /.well-known/* on path alone, and this response body now varies by authority where the direct response it replaced did not.
Since the identifier is derived precisely so that one config serves many hostnames, the multi-hostname case is the intended one, and a poisoned entry hands every client an identifier naming someone else's host. Cache-Control: no-store on the derived branch would be the cheaper and more definitive answer than reasoning about cache key semantics. The document is trivial to generate and there is nothing to gain from caching it.
…rive-resource-url
…th internal API constant Signed-off-by: Aishwarya <aishraimule@gmail.com>
Signed-off-by: Aishwarya <aishraimule@gmail.com>
…ient Signed-off-by: Aishwarya <aishraimule@gmail.com>
…into derive-resource-url
…dvertised list, set Cache-Control to no-store in response to avoid caching where it is indexed by path Signed-off-by: Aishwarya <aishraimule@gmail.com>
…into derive-resource-url
Signed-off-by: Aishwarya <aishraimule@gmail.com>
Signed-off-by: Aishwarya <aishraimule@gmail.com>
|
/retest |
Signed-off-by: Aishwarya <aishraimule@gmail.com>
…into derive-resource-url
|
/retest |
Signed-off-by: Aishwarya <aishraimule@gmail.com>
…into derive-resource-url
|
/retest |
|
/retest |
1 similar comment
|
/retest |
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.resourceis optional on v1alpha1 and v1beta1 (CRD and API docsregenerated). When it is omitted, the identifier is derived per request, so one MCPRoute stays
correct on every hostname and port it is reachable on. An explicit value still wins, for a
deployment whose external URL is not recoverable from the forwarded headers.
Three places advertise the identifier:
resourceis omittedHost+ MCP path (URL.EscapedPath, well-known prefix removed)403 insufficient_scopechallengeresourceMetadataURL401 invalid_tokenchallenge%REQ(X-FORWARDED-PROTO?:SCHEME)%://%REQ(:AUTHORITY)%plus the well-known path and the route's serving path, in theBackendTrafficPolicyresponse overrideThe proxy's scheme is the first hop of
X-Forwarded-Proto. If that header is absent, it fallsback to
httpswhen the connection into the proxy is TLS and tohttpotherwise. The 401 uses?:SCHEMEbecause a missingX-Forwarded-Protoexpands to-, which would advertise-://host/.... A pinnedresourceskips derivation: the document emits the configured stringunchanged, trailing slash included, and the 401 is a static URL from
internalapi.OAuthProtectedResourceMetadataURL. The 403 uses that same helper, so thecontroller and the proxy advertise one metadata-URL shape.
The 401 is the one value the proxy cannot compute. Envoy's JWT filter rejects the request
before it is proxied, so the
ResponseOverrideheader carries the format string above, whichEnvoy Gateway passes through to the local response policy's
response_headers_to_add. The pathin that string is the route's serving path (
spec.path, default/mcp), not the request path.The well-known rule on the main HTTPRoute forwards to the MCP proxy with
x-ai-eg-mcp-routeset, the same way MCP traffic does. The proxy looks the route up from that header and writes
the document.
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, filled bymcpRouteOAuthwheneversecurityPolicy.oauthis set.The document is served independently of whether authorization rules exist.
resourcegets a byte-identical document, trailingslash included, and does not gain
Cache-Control. The challenge URL still drops a trailingslash, as
buildResourceMetadataURLdid; that helper is nowinternalapi.OAuthProtectedResourceMetadataURL, shared by the controller and the proxy.CORS headers and the old direct response's "answer any method" behaviour are preserved.
When
resourceis omitted,Cache-Control: no-storeis set so a cache keyed only on thepath cannot replay one host's document to another. The path comes from
r.URL.EscapedPath(),so a percent-encoded segment is advertised as the client sent it.
Host. The generated HTTPRoute copiesspec.hostnames, and the well-known rule is on that route, so Envoy rejects a Host thatdoes not match before the proxy can reflect it. With no hostnames, every authority the
listener accepts is reflected. An e2e covers this on
mcp-route-oauth-hostnames:mcp.example.comis served, and a foreign Host (including the port-forward address) is not.audiencesis whatever the authorization server puts inaud. It has to agree with aderived identifier only when that server honours RFC 8707 and the client sends the advertised
identifier as the
resourceparameter. The API godoc says the same. The Keycloak examplekeeps
resourcepinned onmcp-routeand addsmcp-route-derived-resourceat/mcp-derivedfor the derived case.
Behavior change
The OAuth Protected Resource Metadata endpoint (
/.well-known/oauth-protected-resource/...) isnow served by the MCP proxy in the extproc sidecar, instead of an Envoy direct response. Its
availability depends on the sidecar being up and having loaded the route named by
x-ai-eg-mcp-route. While that route is not loaded yet (for example during an extprocrestart), the endpoint returns 503 so clients retry. It returns 404 only for a loaded route
that does not configure OAuth.
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.cleanupOAuthResourcesdeletes it too, so a route that drops OAuth before the upgradedcontroller first reconciles it does not keep the stale filter. There is a test for the delete,
and it costs one cached Get per reconcile once nothing is left to remove. A TODO marks the
helper for removal after
v2.0. Call out in the release notes that a user who upgrades from<=1.2 straight to a version after that removal must delete the stale HTTPRouteFilter by hand.
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 proxy does not apply its own allow-list; it relies on that HTTPRoute match. The docs say
this explicitly and point operators at listener
hostname/ routehostnamesor at pinningresource. Both interpolation sites rest on Envoy rejecting an authority that could break outof the surrounding syntax, so an e2e test sends a raw
Host: evil"injectedand asserts thegateway rejects it rather than reflecting it into the 401 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?:SCHEME)%and%REQ(:AUTHORITY)%in a header value that Envoy Gateway funnels into the local responsepolicy's
response_headers_to_add. A unit test pins the exact string the controller emits, butonly e2e proves 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.EscapedPath()rather 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 and that a percent-encodedsegment is preserved.
tests/e2e/testdata/mcp_route_oauth.yamlkeepsresourcepinned onmcp-route-oauthand addsmcp-route-oauth-derived, which omits it, plusmcp-route-oauth-hostnames, which omits it andsets
hostnames. The derived route is what the port-forward exercises: the gateway is reachedon a port chosen at run time, which no statically configured identifier could have matched.
tests/e2e/testdata/mcp_route_authorization.yamllikewise keeps a pinned route and addsmcp-route-authorization-derived, so the 403insufficient_scopechallenge — the oneresource_metadataURL the proxy builds rather than Envoy — is covered for both a pinned and aderived identifier.
Testing
The following MCPRoutes were tested with Cursor MCP Client. We could connect with the MCP Server completing OAuth handshake. List Tools and Tool calls to this MCP Server verified.
MCPRoute without protected resource configured
Main HTTPRoute for the MCPRoute ai-eg-mcp-main-mcp-oauth-test-no-prm
BTP ai-eg-mcp-mcp-oauth-test-no-prm-oauth-protected-resource-metadata
Cursor MCP Config
3 Scenarios when PRM URL or PRM Doc is returned
Insufficient Scope
--- response ---
HTTP/1.1 403 Forbidden
content-type: text/plain; charset=utf-8
www-authenticate: Bearer error="insufficient_scope", scope="get_me",resource_metadata="https:///.well-known/oauth-protected-resource/mcp/mcp-oauth-test-no-prm", error_description="The token is missing required scopes"
date: Sat, 24 Oct 2026 07:34:01 GMT
content-length: 13
connection: close
access denied
Unauthenticated MCP initialize
Protected resource metadata discovery
404, if Authority doesn't belong to Hostnames
Similarly, responses returned by all other paths in the Main HTTPRoute were verified.
Also tested MCPRoute with Protected Resource Metadata configured with Cursor MCP Client.