feat(mcp): discover the OBO token endpoint via RFC 9728 -> RFC 8414 (no IdP guessing) - #31762
Conversation
Greptile SummaryThis PR adds deterministic token-endpoint discovery (RFC 9728 → RFC 8414) for
Confidence Score: 5/5Safe to merge; the discovery and preflight logic is correct, previous concerns about DB persistence and test coverage of the DB path are both addressed, and all new tests are mock-only with no real network calls. Discovery resolves exactly two authoritative sources (RFC 9728 PRM and RFC 8414 AS metadata) with no fallback to guessed origins for OBO, the write-back fires at most once per server and is best-effort, and the preflight correctly distinguishes IdP rejections (401 + WWW-Authenticate) from gateway faults (500). No issues found that would affect correctness at runtime. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | Adds _obo_needs_endpoint_discovery, preflight_token_exchange, _persist_discovered_obo_token_url, and allow_origin_fallback flag; all logic is correct and the new method signatures are backward-compatible. |
| litellm/proxy/_experimental/mcp_server/server.py | Adds preflight OBO exchange call inside _raise_preemptive_401_for_unauthenticated_servers for single-server routes; gating on len(mcp_servers or []) == 1 is correctly scoped. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py | Adds 11 new mock-only tests covering discovery decision, config-load OBO path, DB-path OBO path, DB write-back, and preflight scenarios; the one signature update to an existing test stub adds an assertion rather than weakening coverage. |
Reviews (2): Last reviewed commit: "fix(mcp): persist the discovered OBO tok..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
3dcb0c1 to
5662285
Compare
mateo-berri
left a comment
There was a problem hiding this comment.
Why is greptile 3/5? Just reping me once you've gotten 5/5 greptile with "No files need special attention", no veria concerns, and no bugbot concerns on the last commit. Or if there are any false positives, a response to each of them as to why it's a false positive
25218ed to
fe9f073
Compare
5662285 to
aea3a68
Compare
fe9f073 to
4c8729b
Compare
aea3a68 to
fcdd52d
Compare
4c8729b to
2d4f870
Compare
fcdd52d to
c64b9c7
Compare
2d4f870 to
c5e32f5
Compare
6eb44d3 to
679d65a
Compare
c5e32f5 to
67c7d32
Compare
b910aec to
329c4b6
Compare
…no IdP guessing) An oauth2_token_exchange server can now have its token endpoint discovered the same way the oauth2 (authorization_code) flow already does, instead of always requiring token_exchange_endpoint/token_url to be configured by hand. The existing _descovery_metadata chain (RFC 9728 protected-resource metadata -> RFC 8414 authorization-server metadata -> token_endpoint, SSRF-guarded via async_safe_get) is reused; both the config-load and DB-build paths gate on a new _obo_needs_endpoint_discovery so discovery runs only when no endpoint is configured, and an explicitly configured endpoint still wins and skips the round-trip. The discovered token endpoint lands on token_url, which _token_exchange_spec already reads, so no resolver change is needed. _resolve_oauth2_flow returns None for any non-oauth2 auth_type, so a discovered token_url on an OBO server is never mis-inferred as the M2M client_credentials flow. Discovery for OBO is authoritative only: the resolution order is explicitly configured endpoint, then RFC 9728 -> RFC 8414 advertisement, then fail closed (412, on the parent commit). The gateway never guesses the IdP. _descovery_metadata grows an allow_origin_fallback flag, kept True for the browser oauth2 flow (a human sees the redirect) but set False for token_exchange so the last-resort guess that treats the resource server's own origin as its authorization server is skipped; a subject token is never exchanged against an inferred endpoint.
…y tool list A token_exchange server whose exchange fails with a subject present used to open the MCP session anyway and mask the failure as an empty tools/list. Single-server routes now run the exchange preemptively at the transport edge, where a rejected subject raises the RFC 9728 challenge and a gateway fault its public status; the multi-server aggregate keeps absorbing per-server auth failures. The exchanger caches the preflight result, so the session's list/call reuses it with no extra IdP round-trip. Discovery now also debug-logs the authorization server's advertised issuer, grant types, and client auth methods
329c4b6 to
9c72739
Compare
A DB-backed oauth2_token_exchange server with no configured endpoint had its token_url resolved via RFC 9728 -> RFC 8414 only on the in-memory object returned from build_mcp_server_from_table; the row kept token_url=None, so every rebuild re-ran discovery and a transient upstream outage during a rebuild left the server with no endpoint until the next successful discovery. Write the discovered token_url back onto the row so the guard sees it on the next build. Best-effort and scoped to DB servers: config servers already persist in-memory, and the write-back never fires from a user connect (only from add/update/reload, all admin or system driven). Adds DB-path coverage for discovery firing when unset, skipping when the credentials endpoint is configured, the write-back, and its negative guards
|
@greptileai rereview |
Relevant issues
Stacked on #31622 (the OBO production-readiness PR). Base is that branch, so this diff is just the discovery work; it retargets up the stack as the parent merges
Linear ticket
N/A
What this is
A follow-up from the OBO behavior-contract audit. Today an
oauth2_token_exchange(OBO) server only works when an admin hand-configurestoken_exchange_endpoint/token_url. This adds the contract's deterministic token-endpoint discovery, reusing the chain theoauth2flow already runs, while keeping the gateway from ever guessing the IdP.Resolution order
Defined config first, then authoritative discovery, then fail closed. An explicitly configured
token_exchange_endpoint/token_urlalways wins and skips any network round-trip. When none is configured, the gateway follows RFC 9728 (read the upstream's protected-resource metadata for its advertisedauthorization_servers) then RFC 8414 (read that authorization server's metadata fortoken_endpoint), reusingMCPServerManager._descovery_metadata(SSRF-guarded viaasync_safe_get). The two existing discovery gates (config load and DB build) now also fire for OBO behind a new_obo_needs_endpoint_discovery, only when the endpoint is unset. The discovered endpoint lands ontoken_url, which_token_exchange_specalready reads, so the resolver and exchanger are untouched. If discovery yields nothing, the endpoint stays unset and the parent PR's 412 fail-closed takes over.No IdP guessing
Discovery for OBO is authoritative only.
_descovery_metadatagrows anallow_origin_fallbackflag; it stays True for the browseroauth2(authorization_code) flow, where a human sees the redirect, but is False for token_exchange. That disables the last-resort fallback that treats the resource server's own origin as its authorization server when nothing is advertised, so the gateway never POSTs a subject token to an endpoint it inferred rather than one explicitly configured or authoritatively advertised._resolve_oauth2_flowreturns None for any non-oauth2auth type, so a discoveredtoken_urlon an OBO server is never mis-inferred as the M2Mclient_credentialsflow.Discovery runs once at config-load / DB-build time and the result is persisted onto the
MCPServerrecord, so there is no per-request discovery latency.Screenshots / Proof of Fix
Verified live against a real Keycloak IdP (the same setup used to prove the OBO arm), with two extra upstreams to exercise discovery: one that advertises its authorization server via RFC 9728 protected-resource metadata, and one that advertises no PRM but whose origin does serve RFC 8414 authorization-server metadata, so it is a guessable origin. Loopback was added to
litellm_settings.user_url_allowed_hostsso the discovery fetches clear the SSRF guard.Discovery success.
obo_discoveredis a token_exchange server with notoken_exchange_endpointconfigured. Its upstream advertises the IdP:The gateway reads that, follows RFC 8414 to Keycloak's metadata for the
token_endpoint, and completes the exchange with no endpoint ever configured by hand. The upstream receivesazp=litellm-exchange aud=upstream-api(the minted, audience-scoped token), proving the endpoint came from discovery.No IdP guessing.
obo_guessablepoints at an upstream that returns 404 for its PRM but serves a valid token endpoint at its own origin's.well-known/oauth-authorization-server, so guessing the origin would "work". The upstream's request log shows exactly what the gateway probed during config-load discovery:It probed the RFC 9728 PRM and stopped. It made zero requests to
/.well-known/oauth-authorization-server, so it never guessed the origin as the IdP, and the endpoint stays unresolved. A call then fails closed:precondition required: token exchange endpoint is not configured for this server(412), with no token POSTed anywhere.The full battery:
Config-first precedence is unchanged: an OBO server with an explicit
token_exchange_endpointskips discovery entirely (proven on the parent PR, where the happy-path server used a configured endpoint and made no PRM fetch). Unit coverage pins the decision predicate, that config-load threads the discoveredtoken_endpointontotoken_urlwith the origin fallback disabled for OBO, and that_descovery_metadatadoes not guess the origin when the fallback is offType
🆕 New Feature
Changes
mcp_server_manager.py:_obo_needs_endpoint_discovery, the two widened discovery gates, and theallow_origin_fallbackflag on_descovery_metadata(False for OBO), with tests intest_mcp_server_manager.pyTwo more contract paths were proven live after the follow-up landed. A server with a correct endpoint but a wrong client secret (a real
unauthorized_clientrejection from Keycloak) fails the connect with an opaque 500 and no challenge, so the caller is never sent into a pointless re-login loop for a gateway-side fault. Revoking the minted token at the upstream mid-session was also exercised: the upstream 401'd the cached token once, the gateway invalidated it, re-exchanged with the same subject, and the retried call succeeded under a newjti, invisible to the callerDeliberately not implemented
Discovery extracts and debug-logs the authorization server's advertised
issuer,grant_types_supported, andtoken_endpoint_auth_methods_supportedbut enforces nothing from them; some IdPs under-advertise token exchange support ingrant_types_supported, so a metadata gate would wrongly reject working servers, and the exchange itself is the authoritative check. Onlytoken_endpointis threaded onto the server record. RFC 8707resourceis not part of discovery or the exchange request