fix(mcp): derive OAuth discovery URLs from request path when opt-in - #35226
fix(mcp): derive OAuth discovery URLs from request path when opt-in#35226gym-cmd wants to merge 6 commits into
Conversation
RFC 9728 §3 requires the ``resource`` value in an OAuth protected-resource discovery document to exactly match the URL the client called. Today the MCP discovery-doc builders derive every URL from ``PROXY_BASE_URL`` (a scalar env var) alone, so a single LiteLLM pod that fronts more than one MCP origin mounted at distinct URL path prefixes emits a ``resource`` that matches at most one prefix; every other prefix's client aborts discovery before the first request fires. Add ``get_oauth_discovery_base_url()`` in oauth_utils, an opt-in helper that appends the client-visible URL path prefix (segments before ``/.well-known/``) to the resolved base when ``MCP_OAUTH_DISCOVERY_PATH_FROM_REQUEST=true``. Wire the four discovery- doc builders (per-server + aggregate; protected-resource + auth-server) plus the OpenID JWKS augmentation to use it. Default behaviour is unchanged — existing deployments emit an identical discovery document without touching config. Opt-in because a reverse proxy that rewrites the path before LiteLLM sees the request would produce an incorrect prefix; operators affirm their topology preserves the client-visible prefix by setting the env var. Tests: two backward-compat guards (default + root-mounted route), one prefix-derivation assertion on the per-server response, and two direct unit tests on the helper. All exercise the fixture shape existing MCP discovery tests already use.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThe PR adds opt-in request-path-aware MCP OAuth resource discovery and preserves the request prefix in aggregate and per-server authentication challenges
Confidence Score: 5/5The PR appears safe to merge No blocking failure remains; the previously reported endpoint-prefix and authentication-challenge issues are addressed by separating resource and authorization-server bases and preserving prefixes in challenge metadata URLs
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/oauth_utils.py | Adds opt-in path-prefix derivation and prefix-preserving protected-resource metadata URL helpers |
| litellm/proxy/_experimental/mcp_server/discoverable_endpoints.py | Applies the request-derived prefix only to protected-resource resource values while retaining root-relative authorization-server URLs |
| litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py | Uses the new aggregate metadata helper when constructing gateway authentication challenges |
| tests/test_litellm/proxy/_experimental/mcp_server/test_discoverable_endpoints.py | Adds focused regression coverage for discovery documents and authentication challenges across prefixed and default configurations |
Reviews (4): Last reviewed commit: "fix(mcp): preserve request path prefix o..." | Re-trigger Greptile
Address Greptile review on BerriAI#35226. Prefixing the authorization-server metadata document (authorization_endpoint, token_endpoint, registration_endpoint, jwks_uri) or the protected-resource authorization_servers list broke the default deployment: those handlers are mounted only at root-relative paths, so a prefixed advertisement would 404 the client mid-flow. Only the RFC 9728 §3 ``resource`` value is now derived from the request path prefix. Every other URL in the discovery documents stays on the un-prefixed base returned by ``get_request_base_url``. Split ``_build_oauth_protected_resource_response`` and ``_build_aggregate_protected_resource_response`` to compute both bases; revert ``_build_oauth_authorization_server_response``, ``_build_aggregate_authorization_server_response``, and the OpenID JWKS augmentation to the un-prefixed base entirely. Tests: change the per-server prefix assertion to check ``authorization_servers`` stays un-prefixed, add an aggregate-doc analogue, and add an auth-server regression guard that the opt-in must not leak into the AS metadata.
|
@greptile-apps review again |
…per-request-resource Resolve additive conflict in oauth_utils.py by keeping both the PR's opt-in discovery helpers (get_oauth_discovery_base_url and support) and upstream's shared passthrough challenge helpers (get_passthrough_resource_metadata_url / get_passthrough_www_authenticate) introduced in commit 79d4962.
The passthrough WWW-Authenticate helper built the resource_metadata URL
from get_request_base_url alone, so an anonymous client hitting a
path-prefixed per-server or aggregate MCP endpoint (e.g. /tenant-a/mcp/server_a)
was pointed at an unprefixed .well-known URL. The discovery builder then
saw no prefix on that follow-up request and returned an unprefixed
resource, tripping the RFC 9728 section 3 exact-match check and aborting
discovery on strict clients.
When MCP_OAUTH_DISCOVERY_PATH_FROM_REQUEST is enabled,
get_passthrough_resource_metadata_url now carries the reverse-proxy path
prefix into the challenge URL so the follow-up well-known request lands
on the same prefixed route the resource value will be derived from.
Route-shape detection switches from startswith to endswith so a
prefix-pushed legacy /tenant-a/{server}/mcp shape is recognised too;
without opt-in and without a prefix the emitted URL is byte-identical
to the previous behaviour.
Also refresh two tests that asserted per-server authorization_servers,
which upstream (LIT-4864) changed to the aggregate /mcp AS for
explicitly-named gateway-managed OAuth2 servers.
![veria-ai[bot]](https://avatars.githubusercontent.com/in/1723776?s=60&v=4){kind=small}
PR overviewAll previously flagged issues have been addressed. No open security concerns remain on this pull request. Security reviewNo open security issues remain on this pull request. Fixed/addressed: 2 · PR risk: 0/10 |
Reflow lines under the 120-column budget on oauth_utils.py and the matching test module; formatter-only, no behaviour change.
c5ed78b to
34f050a
Compare
|
Rebased on top of the current litellm_internal_staging (previous head had an ad-hoc merge that pulled in stale drift and left the PR conflicting; the branch now merges cleanly and the diff is exactly the three OAuth discovery files). Ruff format applied to the two touched files. The two open veria-ai comments hang off unchanged upstream code (#35146 auth entitlements, #35155 guardrail translation) that this PR no longer touches; replied on each thread. Full test module (317 tests) green locally. @greptileai review again |
The per-server passthrough challenge already round-trips the request prefix into its WWW-Authenticate URL, but the aggregate /mcp fallback in _gateway_dcr_challenge still built the challenge URL from the un-prefixed base. A client on /tenant-a/mcp was told to fetch an un-prefixed well-known route, the aggregate discovery builder read a request with no prefix, and the returned resource failed the RFC 9728 §3 exact-match against the original prefixed URL. Adds get_aggregate_resource_metadata_url (mirroring the per-server helper) and wires it into the aggregate branch of the DCR challenge, plus three unit tests on the helper (opt-in prefix, default no-op, root-mounted no-op) and two end-to-end tests through _gateway_dcr_challenge (opt-in prefixed, default un-prefixed).
|
Aggregate-endpoint fix landed in f4b2d86. @greptileai review again |
|
Thanks for digging into this. The part of your diff that fixes a bug we can reproduce is the route-shape detection: On the opt-in env var we are going to pass. The multi-prefix topology it targets is not routable today: For a single prefix per pod, which is what routing supports, Happy to be wrong on the routing point if you have a deployment where the prefixed |
Hey @tin-berri You're correct — my approach here was incorrect. Thanks for outlining the routing reality, tin, that saves us going further down this shape. The issue we're having. In our environment, distinct client-visible URL path prefixes route into the same LiteLLM pod from the ingress layer — e.g., /tenant-a/* and /tenant-b/* both terminating at the same pod. Discovery today derives every URL from PROXY_BASE_URL, which is a scalar per pod and can encode at most one prefix. One pod = one prefix that discovery renders correctly for; any second origin's client sees a resource value that doesn't match the URL it called and aborts on the RFC 9728 §3 exact-match check. What we want to achieve. One LiteLLM pod fronting multiple OAuth pass-through MCP origins on distinct URL path prefixes, each origin's discovery emitting a resource that matches the URL its client called. Why. Shared-pod economics. One Deployment to version, patch, and monitor rather than N. One replica floor rather than N × baseline. One config surface, one connection pool, one Redis. Adding a new origin becomes a config change rather than a new Kubernetes resource cohort (VirtualService + AuthorizationPolicy + WAF rule + Deployment + HPA + PDB). Faster onboarding, less drift risk between per-origin releases. Would you mind pointing me in the right direction to make this work upstream — or, if it'd unblock us, letting me raise a PR toward it? If per-request root_path is the primitive I should be targeting, rough shape of what I'd propose: Opt-in, off by default. Existing scalar SERVER_ROOT_PATH and path-carrying PROXY_BASE_URL paths untouched. Configured list of allowed prefixes (in the config file). LiteLLM matches the incoming path against the list, strips the matched prefix off scope["path"], populates scope["root_path"] for that request only. Routes stay registered root-relative. Every downstream URL constructor picks up the per-request prefix via request.base_url — no discovery-builder changes needed. 404 for any prefix not in the allowed list. Can you confirm — if I raise a PR in this shape, would that be acceptable? Happy to iterate on the mechanism (config-file vs. env vs. request-header driving the strip) if you'd prefer a different one, or to hear "keep the single-scalar model, push the multi-origin discovery rewrite out to the mesh" if that's the upstream preference. Also, if there's a config shape I've missed where the current single-scalar mechanism can actually serve multiple prefixes into one pod today, I'd want to know first — that would remove the need for the change entirely. Happy to close this PR and follow up on a fresh issue with a written-up proposal, plus the implementation PR after we align. #35576 covers the reproducible sub-path bug we hit and resolves the immediate breakage regardless. |
|
Follow-up as promised: the per-request It is exactly the shape from the comment above / the direction you pointed at: an opt-in Live before/after from a real proxy is in the PR body: one pod, @tin-berri @milan-berri whenever you get a chance 🙏 |
3 participants
TLDR
Problem this solves:
resourcein MCP OAuth discovery is derived fromPROXY_BASE_URLonlyresourcevalue that matches at most one prefixHow it solves it:
MCP_OAUTH_DISCOVERY_PATH_FROM_REQUEST=trueopts a deployment in/.well-known/) to the baseRelevant issues
Linear ticket
Pre-Submission checklist
Screenshots / Proof of Fix
Reproduction on this branch before the fix applied (test file present, code file at upstream tip). Commit SHA of upstream baseline:
4d54324515.Interpretation: with
PROXY_BASE_URL=https://mcp.example.comand the client hittinghttps://mcp.example.com/tenant-a/.well-known/oauth-protected-resource/mcp/server_a, the discovery response emitsresource=https://mcp.example.com/mcp/server_a— dropping the/tenant-aprefix. An RFC 9728 client that verifies exact match on the resource field will abort.After the fix (same test, this branch's HEAD
0ea9994c6d):Full
test_discoverable_endpoints.pymodule (308 tests) passes cleanly on this branch.Type
🐛 Bug Fix
Changes
Why
RFC 9728 §3 requires the
resourcevalue in an OAuth protected-resource discovery document to exactly match the URL the client called. LiteLLM's MCP discovery builders derive every URL fromget_request_base_url(request), whose resolution order isPROXY_BASE_URLfirst, X-Forwarded-* next,request.base_urllast. That's the correct policy for origin (host + scheme), but it drops the path prefix the client actually used — becausePROXY_BASE_URLis a scalar env baked in at pod startup.Consequence for multi-tenant setups: a single LiteLLM pod that fronts more than one MCP origin under distinct URL path prefixes (e.g.
/tenant-a/mcp/…and/tenant-b/mcp/…both routed to the same LiteLLM) can encode at most one prefix inPROXY_BASE_URL. Any origin whose prefix doesn't match sees a discovery response with the wrongresource; every RFC 9728-strict client aborts before the MCP request even fires. Operators end up forced into oneDeploymentper origin purely to encode a per-originPROXY_BASE_URL, even though the rest of the workload (DB, Redis, config) is homogeneous.What
oauth_utils.py: addget_oauth_discovery_base_url(request). Wrapsget_request_base_urland — whenMCP_OAUTH_DISCOVERY_PATH_FROM_REQUESTis truthy — appends the client-visible URL path prefix (segments before/.well-known/inrequest.url.path) to the resolved base.discoverable_endpoints.py: swap the four discovery-doc builders (per-server + aggregate; protected-resource + auth-server) plus the OpenID JWKS augmentation over to the new helper. Non-discovery callers ofget_request_base_urlare left untouched.Backward compatibility
Zero. The helper is a pass-through to
get_request_base_urlunless the new env var is explicitly set. Existing deployments emit an identical discovery document with no config change.Why opt-in
A reverse proxy that rewrites the path before LiteLLM sees the request (e.g. an Nginx that strips a prefix) would produce an incorrect prefix under this new mode. The env var makes the operator affirm that their topology preserves the client-visible prefix — which is trivially true for the common ALB / path-based ingress topology, but not something LiteLLM can auto-detect.
Alternatives considered
resource_base_urlconfig field. Requires operators to hardcode every origin's routing prefix intomcp_servers.<name>.resource_base_url. Works but is fragile: renaming a routing prefix now requires editing every server row that shares it. The request-URI-derived approach is dynamic and DRY.QA runbook
Local:
End-to-end (against a running proxy):
/tenant-a/*and/tenant-b/*to the same LiteLLM pod, both withPROXY_BASE_URL=https://mcp.example.com(host-only, no prefix).MCP_OAUTH_DISCOVERY_PATH_FROM_REQUEST=true.curl https://mcp.example.com/tenant-a/.well-known/oauth-protected-resource/mcp/<server>→ assertresource == "https://mcp.example.com/tenant-a/mcp/<server>"curl https://mcp.example.com/tenant-b/.well-known/oauth-protected-resource/mcp/<server>→ assertresource == "https://mcp.example.com/tenant-b/mcp/<server>"resource == "https://mcp.example.com/mcp/<server>", matching pre-existing behaviour.