feat(mcp): admit dcr_bridge oauth_delegate clients via a single envelope bearer - #32824
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Greptile SummaryWires the DCR-bridge
Confidence Score: 5/5Safe to merge — the new envelope admission arm is gated, fail-closed, and inert until the companion producer PR lands. All three previously flagged findings are addressed (assert_never import, nullable server_name, bare identity). The arm is fail-closed at every step: envelope crypto, live key reload, SCIM check, and centralized policy gate. The test suite drives every critical failure mode with real minted envelopes and asserts the correct HTTP status for each. No regressions are introduced to existing arms — the new elif branch is strictly additive and is skipped for any non-bridge, multi-target, or non-envelope request. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/auth/user_api_key_auth_mcp.py | Adds a new _single_dcr_bridge_delegate_target gate and _admit_dcr_bridge_delegate arm to process_mcp_request; new helpers reload the live key, enforce SCIM deactivation, and run the centralized policy gate. All prior review findings addressed: alias-first injection, live key reload replacing bare identity, assert_never from typing_extensions. |
| litellm/proxy/_experimental/mcp_server/outbound_credentials/envelope.py | Replaces user_id with key_hash in EnvelopeIdentity and _EnvelopeClaims; wire format now carries the hashed key reference for live-reload at admission rather than a frozen user identity. |
| tests/test_litellm/proxy/_experimental/mcp_server/auth/test_user_api_key_auth_mcp.py | Adds TestMCPDcrBridgeDelegateAdmission with 895 lines covering admission, revoked/blocked/expired keys, blocked teams/projects, SCIM gating, alias-priority injection, egress round-trip for all server shapes, status mapping (429/503/401/403), non-mutation contract, and fall-through guarantees. |
| tests/test_litellm/proxy/_experimental/mcp_server/outbound_credentials/test_bridge_credentials.py | Updates fixture identities from user_id to key_hash to match the new EnvelopeIdentity shape; existing test semantics unchanged. |
| tests/test_litellm/proxy/_experimental/mcp_server/outbound_credentials/test_envelope.py | Updates claim-layout and construction tests to assert key_hash in place of user_id; strengthens claim-set coverage by verifying both server_id and key_hash appear in the JWT claims. |
Reviews (10): Last reviewed commit: "fix(mcp): run proxy-wide pre-DB gates on..." | Re-trigger Greptile
b7302ad to
7c9b491
Compare
3d88cfa to
eb9e1ef
Compare
7c9b491 to
08963b7
Compare
b2fa98d to
9e72416
Compare
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: 5 · PR risk: 0/10 |
9e72416 to
70a4591
Compare
|
Addressed the injection-key divergence: the inner token is now keyed on server.server_name or server.alias only (the identifiers egress resolves via lookup_mcp_server_auth_in_headers), dropping the server.name fallback egress never looks up, and the target gate now fails closed when a bridge server has neither, so a request is never admitted with its forwarded token dropped under an unresolvable key. Tests cover the alias-only path and the no-name fail-closed path. On the spend/rate-limit context: this is intentional for this first version and not a correctness gap. The admitted UserAPIKeyAuth carries the envelope's user_id, so per-user attribution and per-user budgets apply; what it does not carry is a virtual-key identity, so per-key rate limits do not, because a bridge client authenticates with the envelope rather than a litellm key. Reconstructing full per-key context would mean sealing the litellm credential into the envelope, which changes the envelope contract in the merged module and puts that credential in the client-held bearer; that is a deliberate follow-up rather than something to fold in here. |
70a4591 to
5534930
Compare
|
bugbot run |
|
@greptileai rereview |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1cc611e. Configure here.
|
@greptileai rereview |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 488f57f. Configure here.
|
Addressed the remaining observation on the live-policy gate in a81ac2d. _enforce_admitted_live_policy no longer flattens every failure to 401; it mirrors UserAPIKeyAuthExceptionHandler, so an over-budget identity surfaces the standard 429, a sub-check's own HTTPException/ProxyException keeps its status, a transient database outage is a retryable 503, and only a genuinely unresolvable failure (blocked team/project, which raise a bare Exception exactly as the standard pipeline's fallback) stays the fail-closed 401. Flattening budget to 401 had told an over-budget but validly-authenticated caller their credential was invalid, which on a DCR client reads as broken auth and can trigger a re-authorize loop that cannot fix a budget problem, and it masked a DB outage as an auth error. Added four regression tests over the mapping (429 / 503 / 401 / 403); three fail against the old flatten-to-401 behavior. Also documented that the SCIM owner lookup is the one deliberately fail-open check, matching the standard builder The double user-object fetch (SCIM check plus the centralized gate) is left as-is; the second fetch is a cache hit and deduping would mean forking the shared common-checks function Separately, assert_never is now imported from typing_extensions rather than typing (488f57f), since the project supports Python 3.10 where typing.assert_never does not exist and the bad import broke MCP auth at module load |
… identity The bridge envelope sealed only user_id/server_id, and admission fabricated a UserAPIKeyAuth(user_id=...) with no object_permission, team_id, org_id, or key identity. Downstream MCP permission checks read the missing restrictions as unrestricted, so a caller holding a valid envelope for a restricted key could reach tools and servers that key was never granted, and a revoked key kept working until the envelope expired. Bind the hashed authorizing key into the envelope identity and reload the live UserAPIKeyAuth by it at admission via get_key_object, failing closed with a 401 when the key is missing, blocked, or expired. Authorization is resolved fresh per request instead of frozen at mint time, so current key/team/org and tool restrictions plus revocation are enforced.
…idge admission
Two follow-ups on the envelope admission arm flagged in review.
Team revocation bypass: _reload_admitted_key checked only the key's own
blocked/expires, so blocking a key's team left every envelope minted under it
live until expiry. Reload the team and reject a blocked team, mirroring
common_checks, so a team block revokes its envelopes immediately.
Caller-overridable upstream token: egress resolves the per-server auth header
alias-first, but injection keyed under server_name, so for a server with a
distinct alias a caller-forwarded x-mcp-{alias}-authorization sat at the
higher-priority slot and paired the admitted identity with an attacker's
upstream credential. Inject under alias-first so the sealed token owns the slot
egress resolves.
…and mirror the SCIM owner check
…ad of flattening to 401 Over-budget rendered 401 (should be 429), model-access and other typed failures collapsed to 401, and a transient DB outage was masked as an auth error. Mirror UserAPIKeyAuthExceptionHandler: budget maps to 429, a sub-check's own HTTPException/ProxyException keeps its status, a DB outage is a retryable 503, and only a genuinely unresolvable failure stays the fail-closed 401.
a81ac2d to
ea64ef7
Compare
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a81ac2d. Configure here.
|
@greptileai rereview |
get_key_object's raw transport error propagated uncaught out of _reload_admitted_key as an opaque 500; classify it via the shared _raise_503_if_db_unavailable helper (also used by the live-policy gate) so a database outage is a retryable 503, while a key-not-found ProxyException stays the fail-closed 401.
|
Handled the reload-path exception gap in f5f03cb. A DB transport error during the key reload previously propagated uncaught out of _reload_admitted_key as an opaque 500 (get_key_object only surfaced ProxyException/HTTPException, which mapped to 401). It now runs through a shared _raise_503_if_db_unavailable helper, the same classifier the live-policy gate uses, so a database outage is a retryable 503 while a genuine key-not-found stays the fail-closed 401. Added a regression test that drives a ConnectionError through the reload and asserts 503; without the fix the raw error escapes as a 500. The helper also de-duplicates the 503 logic that was inline in _enforce_admitted_live_policy |
…e enforced The envelope arm reloaded the identity and ran _run_centralized_common_checks but skipped RouteChecks.should_call_route, which the standard pipeline runs between the builder and common_checks. Because the centralized checks treat MCP as an inference route and never re-check allowed_routes, a key barred from MCP routes could mint an envelope at the token endpoint (not itself an MCP route) and replay it against MCP. Run the route gate before admitting, and clear the request-scoped budget_reservation, matching the wrapper's sequence; a disallowed route now surfaces the gate's own 403.
|
Fixed in 688f535. The arm now runs RouteChecks.should_call_route(route=route, valid_token=admitted, request=request) before admitting, matching the standard pipeline's sequence between the builder and common_checks, so a key whose allowed_routes exclude MCP is rejected with the route gate's own 403 rather than reaching tools through an envelope minted at the token endpoint. Regression test drives an envelope for a key restricted to /chat/completions and asserts 403; without the gate the request is admitted On using _run_centralized_common_checks properly: the wrapper's sequence is builder, then budget_reservation = None, then should_call_route, then _run_centralized_common_checks. The arm was doing the reload and the common checks but skipping the two middle steps; both are now in _enforce_admitted_live_policy, so it mirrors the wrapper. The post-common-checks steps (end-user id resolution, client-tag merge) are no-ops for an MCP request whose body carries no model, tags, or end_user, so nothing else is missing |
|
bugbot run |
The envelope arm bypasses user_api_key_auth, so it never ran pre_db_read_auth_checks (request-size and body-safety limits, the IP allowlist, and the general_settings route allowlist) that the normal MCP admission path runs before any key lookup. A caller blocked by IP or a disallowed proxy route could be admitted through an envelope where the same principal on the normal path is rejected. Run those gates before the envelope crypto, mirroring the pipeline's pre-DB ordering; a blocked IP or route surfaces its own 403.
|
Fixed in 1c3c1af, and audited the whole entry sequence so this closes the class of finding rather than one instance. The envelope arm bypasses user_api_key_auth, so it now runs the same gates in the same order the wrapper does: pre_db_read_auth_checks (request size, body safety, IP allowlist, general_settings route allowlist) runs before the envelope crypto, mirroring the pipeline's pre-DB ordering; a blocked IP or disallowed proxy route surfaces its own 403. Then the identity is reloaded (the DB-read equivalent). Then budget_reservation is cleared, RouteChecks.should_call_route enforces the key's allowed_routes and any disabled/admin route, and _run_centralized_common_checks enforces team-block, project-block, org, and budget. The steps the wrapper runs after common_checks (end-user id resolution, client-tag merge) are no-ops for an MCP request whose body carries no model, tags, or end_user, so nothing else is outstanding Regression test drives an envelope under a general_settings route allowlist that forbids MCP and asserts 403 before the identity is reloaded; without the pre-DB gate it is admitted |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1c3c1af. Configure here.
Relevant issues
Linear ticket
Part of LIT-4338 (based on the envelope consumer PR so the review diff is only this change; the producer that mints the envelope this admits lands in the follow-up)
Pre-Submission checklist
Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
This wires the envelope consumer into MCP admission but is inert until the producer mints envelopes (nothing issues an llm_env_ bearer yet), so the contract is pinned by unit tests driving process_mcp_request with real minted envelopes. The full interactive proof (Claude Desktop signing in, receiving an envelope, and calling tools that attribute to the litellm identity) lands with the producer PR that completes the loop
Type
🆕 New Feature
Changes
Wires the DCR-bridge oauth_delegate envelope consumer into MCP admission so a bridge client can authenticate with the single envelope bearer it holds, carrying its authorizing litellm key reference and the upstream OAuth token
process_mcp_request gains one arm, gated so every existing path stays byte-identical: it fires only when the request targets exactly one server that is both is_oauth_delegate and is_dcr_bridge (via single_dcr_bridge_delegate_target, which fails closed on multi-target, unresolved, or non-matching) AND the Authorization is envelope-shaped (a cheap keyless llm_env prefix check). The arm sits after the explicit x-litellm-api-key path so a real litellm key still wins, and a non-envelope bearer on a bridge server falls through to the normal oauth2 arm that 401s
_admit_dcr_bridge_delegate derives the envelope keys from master_key, opens the envelope bound to the target server_id (rejecting an envelope minted for another server), and admits the caller under the live key the envelope references. Authorization is resolved fresh at admission rather than trusted from the envelope: the sealed key_hash reloads the current UserAPIKeyAuth via get_key_object, so the key's present team, org, and object-permission restrictions and its revocation state gate the request instead of a snapshot frozen at mint time. A key that is missing, blocked, or expired fails closed with a 401. The inner upstream token is injected under the server's per-server auth-header key so egress forwards it through the PassthroughConfig override, while the envelope Authorization the LIT-3794 leak-defense strips never reaches the upstream. A new headers dict is returned rather than mutating the input, and an invalid, expired, wrong-key, or wrong-server envelope fails closed with a 401
The live-key reload closes a review finding on an earlier revision of this arm: it previously admitted under a bare identity fabricated from the envelope, with no object_permission, team_id, or org_id, so a caller holding a valid envelope for a restricted key could reach tools that key was never granted, and a revoked key kept working until the envelope expired. Binding the authorizing key's hash into the envelope (the producer PR seals it) and reloading the record here is what keeps every key, team, and tool restriction plus revocation enforced
Three follow-up review findings are also fixed. The sealed inner token is keyed alias-first to match egress (lookup_mcp_server_auth_in_headers resolves alias before server_name); keying under server_name left a caller-forwarded x-mcp-{alias}-authorization at the higher-priority slot, which would have paired the admitted identity with an attacker-chosen upstream credential, and a parametrized round-trip test now resolves the injected headers through the real egress lookup for each admissible server shape (alias only, server_name only, both) so the two key hierarchies cannot drift apart. Blocked teams and the account-revocation gap (a SCIM-deactivated owner or a blocked project kept working until envelope expiry) are enforced by routing the admitted identity through the standard pipeline's centralized policy gate: after the reload, _enforce_admitted_live_policy runs _run_centralized_common_checks, the same single authorization point user_api_key_auth applies after every builder path, so team, project, org, and budget state gate the envelope exactly as they gate the same key presented directly, and a policy dimension added to the standard pipeline applies here without this arm mirroring it one by one; the earlier hand-rolled team-block gate is deleted in favor of that call. The one check the shared gate cannot cover is the SCIM owner state, which the standard pipeline enforces inline in the builder rather than in common_checks, so the reload mirrors that single gate and rejects a key whose owning user carries scim_active false, making IdP offboarding revoke the user's already-minted envelopes immediately instead of at envelope expiry. A failed user lookup during that SCIM check is the one deliberately fail-open point in an otherwise fail-closed arm, matching how the standard builder treats the same lookup failure. Policy failures surface with the status the standard pipeline gives them rather than a flattened 401: _enforce_admitted_live_policy mirrors UserAPIKeyAuthExceptionHandler, so an over-budget identity is a 429, a sub-check that raised its own HTTPException or ProxyException keeps that status, a transient database outage is a retryable 503, and only a genuinely unresolvable failure (a blocked team or project raises a bare Exception, exactly as the standard pipeline's fallback) stays the fail-closed 401; collapsing everything to 401 had told an over-budget but validly-authenticated caller their credential was invalid and masked a DB outage as an auth error. A separate fix imports assert_never from typing_extensions rather than typing, since the project supports Python 3.10 where typing.assert_never does not exist and the bad import would break MCP auth at module load
Tests drive process_mcp_request with real minted envelopes covering live-key authorization-context admission (the reloaded key's team, org, and object-permission ride on the admitted auth), a revoked key that no longer resolves, a blocked key, an expired key record, a blocked team, a SCIM-deactivated owner, a SCIM-active owner that still admits, a blocked project enforced through the centralized gate, the alias-priority injection that overwrites a caller-forwarded upstream header, the injection/egress round-trip for every admissible server shape, plus inner-token injection, envelope expiry, wrong master key, wrong server binding, non-envelope fall-through, explicit-key precedence, non-bridge servers, multi-target scopes, and the no-mutation contract; mutations reverting admission to the old bare-identity behavior, dropping the centralized policy call or the SCIM gate, or restoring server_name-first injection are each caught by the corresponding test
Note
High Risk
Changes MCP authentication and authorization for a new bearer type, including live key reload and policy gates; mistakes could admit revoked users, wrong upstream credentials, or bypass route/IP restrictions.
Overview
Adds a DCR-bridge envelope admission path in MCP request handling so a client can authenticate with one
llm_env_bearer that carries both LiteLLM authorization and the upstream OAuth token. The new arm runs only when the request targets exactly one server that is bothoauth_delegateanddcr_bridge, andAuthorizationis envelope-shaped; explicitx-litellm-api-keystill wins, and plain bearers keep the existing oauth2 path.Envelope identity switches from sealing
user_idtokey_hash. At admission the gateway opens the envelope (bound toserver_id), reloads the currentUserAPIKeyAuthviaget_key_object, runs pre-DB checks, route allowlists, centralized common checks, and a SCIM owner gate, then injects the inner upstream token under the server alias-first per-server header key so egress can forward it without leaking the envelopeAuthorization.Policy failures now mirror the standard pipeline (403 route denial, 429 budget, 503 DB outages) instead of collapsing to 401. Extensive tests cover revocation, injection/egress alignment, and bypass cases.
Reviewed by Cursor Bugbot for commit 1c3c1af. Bugbot is set up for automated code reviews on this repo. Configure here.