feat(mcp): add entra_obo profile to the token_exchange (OBO) arm - #31983
Conversation
Greptile SummaryThis PR adds an
Confidence Score: 5/5Safe to merge; the entra_obo dialect is additive, rfc8693 behavior is unchanged, and the Conditional Access claims flow is well-guarded against header injection. The profile dispatch is exhaustive (match + assert_never), the entra_obo form correctly omits subject_token/subject_token_type/audience, profile is part of the cache key so a dialect flip forces a fresh exchange, and claims from the IdP body are base64-encoded before they reach the WWW-Authenticate header. All load paths (YAML, DB, management API) thread token_exchange_profile correctly. Tests are mock-only, cover the exact form dict, cache-key isolation, and the step-up claims end-to-end path. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/_experimental/mcp_server/outbound_credentials/token_exchanger.py | Core OBO exchanger renamed and extended with exhaustive profile dispatch, entra_obo precondition check, and claims threading through SubjectTokenRejected; cache key correctly includes profile. |
| litellm/proxy/_experimental/mcp_server/outbound_credentials/adapter.py | Profile normalization (unknown → rfc8693), raise_token_exchange_challenge extended with claims base64-encoding and correct insufficient_claims error; header injection prevented by fixed alphabet. |
| litellm/proxy/_experimental/mcp_server/outbound_credentials/token_exchange_provider.py | _oauth_error_fields extended to return (error, claims) tuple; SubjectTokenRejected now carries claims from IdP rejection body; gateway-fault errors correctly take precedence over claims. |
| litellm/proxy/_experimental/mcp_server/outbound_credentials/types.py | Unauthorized gains claims field; CredError.of_unauthorized gains claims parameter; TokenExchangeConfig gains profile field with rfc8693 default; all backward-compatible. |
| litellm/types/mcp.py | MCPCredentials gains token_exchange_profile: Optional[str] so the management API can persist the entra_obo profile without Pydantic silently dropping it. |
| litellm/proxy/_experimental/mcp_server/mcp_server_manager.py | YAML and DB credential paths both thread token_exchange_profile; preflight and egress challenge paths forward claims from err.unauthorized.claims. |
| tests/test_litellm/proxy/_experimental/mcp_server/outbound_credentials/test_token_exchanger.py | Comprehensive mock-only tests: entra_obo form assertion (exact dict + absence of rfc8693 fields), scope precondition, profile as part of cache key, step-up claims threading; all Rfc8693TokenExchanger references updated to OboTokenExchanger. |
| tests/test_litellm/proxy/_experimental/mcp_server/outbound_credentials/test_adapter.py | Tests for profile mapping, unknown profile normalization, static challenge byte-identity, and insufficient_claims challenge format; all mock-only. |
| tests/test_litellm/proxy/_experimental/mcp_server/outbound_credentials/test_token_exchange_provider.py | Tests for step-up error+claims threading, gateway-fault priority over claims, and None claims on plain rejection; mock-only. |
| tests/test_litellm/proxy/_experimental/mcp_server/test_mcp_server_manager.py | DB round-trip test for entra_obo profile, preflight challenge with step-up claims, and aggregate list absorbing a CA-challenged server; all mock-only. |
| tests/test_litellm/proxy/management_endpoints/test_mcp_management_endpoints.py | Tests that NewMCPServerRequest and UpdateMCPServerRequest preserve token_exchange_profile in the credentials dict through pydantic validation. |
Reviews (4): Last reviewed commit: "fix(mcp): use error=insufficient_claims ..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This comment was marked as outdated.
This comment was marked as outdated.
dfe69f5 to
8b56f66
Compare
5662285 to
aea3a68
Compare
fcdd52d to
c64b9c7
Compare
329c4b6 to
9c72739
Compare
Microsoft Entra On-Behalf-Of uses the RFC 7523 jwt-bearer grant rather than RFC 8693, so the existing token_exchange arm cannot mint tokens against Entra. This adds an entra_obo profile on TokenExchangeConfig that switches the request form to Entra's jwt-bearer OBO dialect (the inbound token as assertion, the target resource carried in scope, and requested_token_use=on_behalf_of), while reusing the shared caching, single-flight, TTL, and fail-closed machinery. The exchanger is renamed from Rfc8693TokenExchanger to OboTokenExchanger since it now serves both dialects The profile is threaded through the config-load path, the DB credentials blob, and the MCPCredentials request schema, so an operator can select it from YAML or the management API. It rides in the existing credentials JSON blob, so there is no new auth_type and no DB migration Resolves LIT-4163
8b56f66 to
2176477
Compare
…n the OBO 401 An entra_obo exchange that the IdP rejects for Conditional Access returns a 4xx with error=interaction_required and a claims blob the client must satisfy to step up. The arm dropped both and emitted a static RFC 9728 challenge, so a CA-protected Entra upstream was unreachable through the gateway. The provider now reads the RFC 6749 error code and the claims string off the rejection body (error_description is still never carried; it can leak IdP internals), threads them through SubjectTokenRejected -> CredError.unauthorized, and the challenge builder folds them into WWW-Authenticate: the machine error only when it is a plain OAuth token (guards against header injection from a hostile body) and the claims base64-encoded in a claims parameter, the convention MSAL-family clients decode. With neither field the header is byte-identical to the static challenge. The multi-server aggregate still absorbs a step-up 401 to an empty listing; only single-server routes surface it
Per Microsoft's claims-challenge format, a WWW-Authenticate carrying a claims challenge must set error=insufficient_claims (the value MSAL-family clients key on to recognize the challenge and replay the claims), not the raw token-endpoint code. The challenge now sets insufficient_claims whenever a claims blob is present and keeps invalid_token otherwise, with a step-up-accurate error_description in the claims case. The presence of claims now drives the error value, so the raw oauth_error no longer needs threading from the provider through CredError to the edge; that plumbing is removed (the provider still reads the error code for its gateway-fault classification). Both the error value and the base64 claims are fixed-alphabet, so nothing from the IdP body reaches the header unescaped. Cross-checked field-by-field against Microsoft Learn; a bogus jwt-bearer OBO POST to the real login.microsoftonline.com/common endpoint confirmed Entra recognizes the grant and returns the error shape the parser reads. Follow-up: also emit authorization_uri alongside resource_metadata for strict non-MCP MSAL clients (RFC 9728 resource_metadata already serves MCP)
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
bugbot run |
|
note @tin-berri I'm just testing out coderabbit and comparing it with bugbot. Don't consider the coderabbit/bugbot comments blocking |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 5df7203. Configure here.
| # IdP will reject. | ||
| return Error( | ||
| CredError.of_misconfigured("entra_obo token exchange requires a scope (e.g. api://<app-id>/.default)") | ||
| ) |
There was a problem hiding this comment.
Whitespace scopes bypass entra guard
Medium Severity
For entra_obo, the preflight uses not config.scopes, so a non-empty scopes tuple that only contains blank strings (e.g. from YAML scopes: [""]) skips the misconfigured path and still POSTs to Entra with an empty or whitespace scope, instead of failing closed before any IdP call as documented.
Reviewed by Cursor Bugbot for commit 5df7203. Configure here.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR adds multi-profile support ( ChangesEntra OBO Token Exchange Profile Support
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MCPServerManager
participant Adapter
participant OboTokenExchanger
participant IdP
Client->>MCPServerManager: request tool call
MCPServerManager->>Adapter: resolve credentials (v2)
Adapter->>OboTokenExchanger: exchange(config, profile)
OboTokenExchanger->>IdP: POST token endpoint (rfc8693/entra_obo form)
IdP-->>OboTokenExchanger: 4xx + claims (step-up required)
OboTokenExchanger-->>Adapter: CredError.of_unauthorized(claims)
Adapter->>Adapter: raise_token_exchange_challenge(claims)
Adapter-->>Client: 401 WWW-Authenticate insufficient_claims
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
mateo-berri
left a comment
There was a problem hiding this comment.
LGTM; thanks!
Nonblocking but it would be a lot of assurance if you could check if the bugbot issue is legit or nbd
…ils closed
The DB-build path already runs YAML/DB scopes through _extract_scopes (which drops blanks), but the
config-load path read server_config["scopes"] raw. A YAML `scopes: [""]` therefore reached the
exchanger as a ("",) tuple: non-empty, so the entra_obo `not config.scopes` precondition skipped its
fail-closed misconfigured path and the form builder POSTed an empty scope to the IdP instead of
failing before any network call. Config-load now filters blanks the same way, so an all-blank list
normalizes to None and the precondition fails closed. Regression test loads a blank-scope entra_obo
server and asserts the exchange returns misconfigured without POSTing
…riAI#31983) * feat(mcp): add entra_obo profile to the token_exchange (OBO) arm Microsoft Entra On-Behalf-Of uses the RFC 7523 jwt-bearer grant rather than RFC 8693, so the existing token_exchange arm cannot mint tokens against Entra. This adds an entra_obo profile on TokenExchangeConfig that switches the request form to Entra's jwt-bearer OBO dialect (the inbound token as assertion, the target resource carried in scope, and requested_token_use=on_behalf_of), while reusing the shared caching, single-flight, TTL, and fail-closed machinery. The exchanger is renamed from Rfc8693TokenExchanger to OboTokenExchanger since it now serves both dialects The profile is threaded through the config-load path, the DB credentials blob, and the MCPCredentials request schema, so an operator can select it from YAML or the management API. It rides in the existing credentials JSON blob, so there is no new auth_type and no DB migration Resolves LIT-4163 * feat(mcp): propagate the Entra Conditional Access step-up challenge on the OBO 401 An entra_obo exchange that the IdP rejects for Conditional Access returns a 4xx with error=interaction_required and a claims blob the client must satisfy to step up. The arm dropped both and emitted a static RFC 9728 challenge, so a CA-protected Entra upstream was unreachable through the gateway. The provider now reads the RFC 6749 error code and the claims string off the rejection body (error_description is still never carried; it can leak IdP internals), threads them through SubjectTokenRejected -> CredError.unauthorized, and the challenge builder folds them into WWW-Authenticate: the machine error only when it is a plain OAuth token (guards against header injection from a hostile body) and the claims base64-encoded in a claims parameter, the convention MSAL-family clients decode. With neither field the header is byte-identical to the static challenge. The multi-server aggregate still absorbs a step-up 401 to an empty listing; only single-server routes surface it * fix(mcp): use error=insufficient_claims for the Entra step-up challenge Per Microsoft's claims-challenge format, a WWW-Authenticate carrying a claims challenge must set error=insufficient_claims (the value MSAL-family clients key on to recognize the challenge and replay the claims), not the raw token-endpoint code. The challenge now sets insufficient_claims whenever a claims blob is present and keeps invalid_token otherwise, with a step-up-accurate error_description in the claims case. The presence of claims now drives the error value, so the raw oauth_error no longer needs threading from the provider through CredError to the edge; that plumbing is removed (the provider still reads the error code for its gateway-fault classification). Both the error value and the base64 claims are fixed-alphabet, so nothing from the IdP body reaches the header unescaped. Cross-checked field-by-field against Microsoft Learn; a bogus jwt-bearer OBO POST to the real login.microsoftonline.com/common endpoint confirmed Entra recognizes the grant and returns the error shape the parser reads. Follow-up: also emit authorization_uri alongside resource_metadata for strict non-MCP MSAL clients (RFC 9728 resource_metadata already serves MCP) * fix(mcp): filter blank scopes on the config-load path so entra_obo fails closed The DB-build path already runs YAML/DB scopes through _extract_scopes (which drops blanks), but the config-load path read server_config["scopes"] raw. A YAML `scopes: [""]` therefore reached the exchanger as a ("",) tuple: non-empty, so the entra_obo `not config.scopes` precondition skipped its fail-closed misconfigured path and the form builder POSTed an empty scope to the IdP instead of failing before any network call. Config-load now filters blanks the same way, so an all-blank list normalizes to None and the precondition fails closed. Regression test loads a blank-scope entra_obo server and asserts the exchange returns misconfigured without POSTing


Relevant issues
Stacked on #31762 (OBO token-endpoint discovery), the top of the OBO stack #31526 -> #31622 -> #31762. Base is
litellm_mcp_v2_obo_endpoint_discovery, so the diff here is only the entra_obo work; it retargets up the stack as the parents mergeLinear ticket
Resolves LIT-4163
Pre-Submission checklist
What this is
The v2
token_exchange(OBO) arm speaks only RFC 8693 today. Microsoft Entra ID's On-Behalf-Of flow is not RFC 8693; it is the RFC 7523jwt-bearergrant with a Microsoftrequested_token_use=on_behalf_ofextension, so an Entra upstream cannot mint tokens through the existing arm. This adds anentra_oboprofile that switches the request form to Entra's dialect while reusing everything below the formTokenExchangeConfiggains aprofile: Literal["rfc8693", "entra_obo"](defaultrfc8693, so existing servers are unchanged). The exchanger's form builder dispatches on it with an exhaustivematchplusassert_never, the same pattern the package already uses forCredErrorand the auth-config union, so basedpyright proves the dialects stay in sync. Underentra_obothe caller's inbound token is sent asassertion(itsaudmust be the gateway's own client), the target resource is carried inscopeasapi://<app-id>/.defaultsince Entra has no audience parameter, andrequested_token_use=on_behalf_ofis added;subject_token_typeandaudienceare not sent. The cache key folds in the profile so a dialect flip re-exchanges rather than serving a token minted for the other form. The shared caching, single-flight, tenant keying, TTL, discovery, RFC 9728 challenge, and fail-closed contract are all untouchedBecause the concrete exchanger now serves both dialects,
Rfc8693TokenExchangeris renamed toOboTokenExchangerso the name is not a lie; theTokenExchangerprotocol and the resolver arm are unchanged, so the rename is contained to the class and its two call sitesFail-closed behavior follows the v2 no-silent-fallback rule. A missing endpoint is a 412 precondition, missing client credentials is a misconfigured 5xx, and an
entra_oboserver with no scope is misconfigured before any IdP call since Entra cannot resolve a target without one. An Entra 4xx such asAADSTS65001(no admin consent) maps to a 401 challenge through the existingSubjectTokenRejectededge so the caller re-authenticates, while a 5xx or transport failure stays a retryable 503Certificate client authentication (Entra's
client_assertioncase, RFC 7523 private_key_jwt) is out of scope here and slots into the sameentra_obobranch as a later follow-upOperator config for an Entra upstream:
Screenshots / Proof of Fix
A full end-to-end run against a real Entra tenant needs the reviewer's Azure app registrations (a gateway app with a client secret and an admin-consented permission to the downstream
api://app), so the automated proof below drives the real production egress exchanger (build_token_exchanger(), which POSTs through the real httpx_post_exchange_endpoint) against a local stand-in Entra token endpoint that records the exact form it receives. This exercises the real HTTP path and the real form builder, not mocked internals. The stand-in stands in for Entra the same way the parent PRs used KeycloakTo verify against a live Entra tenant on a running proxy, point
token_urlat your tenant, setclient_id/client_secretto a gateway app that has admin-consented permission to theapi://<target>app, present a user access token whoseaudis the gateway app, and call a tool on theentra_oboserver; the upstream receives the minted token withaud=api://<target>and the same useroid/sub, and a caller with no consent gets a 401 challenge rather than a 503 loopThe form builder, cache-key, and adapter mapping are pinned by mutation-grade unit tests in
test_token_exchanger.pyandtest_adapter.py(the entra form asserts the exact dict and thatsubject_token/subject_token_type/audienceare absent; the profile is verified to be part of the cache key by a count assertion that fails if it is dropped). The fulloutbound_credentialssuite is green (210) andmcp_server_manageris green (214); ruff, ruff format, and basedpyright strict add zero new findingsType
New Feature
Changes
TokenExchangeConfiggainsprofile; the exchanger form builder dispatches on it with an exhaustive match and is renamedOboTokenExchanger; the adapter mapstoken_exchange_profilefrom the server, normalizing an unknown value torfc8693. TheMCPServermodel gainstoken_exchange_profile, and both the config-load and DB build sites thread it.MCPCredentialsalso gainstoken_exchange_profileso the management API (NewMCPServerRequest/UpdateMCPServerRequest) validates and persists it into the credentials blob rather than pydantic silently dropping the unknown key. No newauth_typeand no DB migration, since the field rides in the existing credentials JSON blobConditional Access step-up challenge (follow-up commit)
Entra returns a 4xx with
error=interaction_requiredand aclaimsblob when a Conditional Access policy demands step-up (MFA, compliant device, etc.); the client must replay those claims to Entra to satisfy the policy, then retry. The arm previously dropped the code and claims and emitted a static RFC 9728 challenge, so a CA-protected Entra upstream was unreachable through the gateway. This threads the step-up through end to end.The provider reads the RFC 6749
errorcode and theclaimsstring off the rejection body (theerror_descriptionis still never read; it can carry IdP internals), carries them onSubjectTokenRejectedintoCredError.unauthorized, and the challenge builder folds them intoWWW-Authenticate: the machine error is embedded only when it is a plain OAuth token, guarding against header injection from a hostile IdP body, and the claims travel base64-encoded in aclaimsparameter, the convention MSAL-family clients decode. With neither field present the header is byte-identical to the prior static challenge, so the rfc8693 path is unchanged. The multi-server aggregate still absorbs a step-up 401 to an empty listing; only single-server routes surface it as a challenge.Proof of Fix (live, real proxy egress)
Driven against a stand-in Entra token endpoint that records the exact form it receives and, on demand, returns Entra's CA rejection; the real egress exchanger (
build_token_exchanger()-> real httpx_post_exchange_endpoint) POSTs to it, the same way the parent PRs used Keycloak.Happy path: the gateway POSTs the exact Entra OBO form and forwards the minted token:
Conditional Access rejection: single-server connect returns 401 with the propagated code and the base64 claims, and no AADSTS/error_description leak:
Aggregate graceful degradation is preserved:
/mcpwith the CA-rejected server present still returns200and lists the healthy servers, rather than a blanket 401.Unit coverage pins each hop: the provider extracts
error+claimsand never the description; a gateway-fault code still wins over a present claims blob; the exchanger carries both onto the unauthorizedCredError; the challenge builder folds them in, rejects a non-token error code back toinvalid_token, and stays byte-identical without them; the preflight surfaces them on the single-server 401 while the aggregate list still absorbs.To verify against a live Entra tenant, point
token_exchange_endpointat your tenant, setclient_id/client_secretto a gateway app admin-consented to theapi://<target>app, present a user access token whoseaudis the gateway app, and enable a Conditional Access policy on the target; the CA-blocked call returns a 401 whoseclaimsthe client replays to Entra to step up, then retries.Field-by-field cross-check against Microsoft Learn, plus a real-Entra probe
The request form and the challenge were checked field-by-field against the Microsoft OBO doc (
v2-oauth2-on-behalf-of-flow) and the claims-challenge format doc (claims-challenge). The request form matches verbatim (grant_type=jwt-bearer,assertion,scope=.../.default,requested_token_use=on_behalf_of, client auth in the body or Basic; nosubject_token/audience). Theclaimsvalue is base 64 encoded exactly as the spec's own example, and the directive name isclaims. The one correction the cross-check surfaced is folded into this PR: a claims challenge must useerror=insufficient_claims(not the raw token-endpoint code), which is what MSAL-family clients key on.To validate the failure contract against the live server without a tenant, a bogus
jwt-bearerOBO POST was sent to the realhttps://login.microsoftonline.com/common/oauth2/v2.0/token:Entra recognized the grant (it reached assertion validation rather than returning
unsupported_grant_type), and its error body carries exactly the fields the parser reads (error,error_codes, andclaimson a CA rejection), whileerror_descriptionand the trace IDs are the internals we deliberately never forward. This does not exercise the happy path (which needs app registrations to mint a token) and is not a substitute for a full real-tenant run; it confirms the wire dialect and error contract against the live endpoint.Follow-up (out of scope here)
For strict non-MCP MSAL clients, also emit
authorization_uri(the tenant/authorizeendpoint) alongside the RFC 9728resource_metadatain the challenge. MCP clients discover the IdP viaresource_metadata, so it is not needed for this gateway's clients; it is a compatibility add for raw MSAL tooling.Note
Medium Risk
Changes OAuth token exchange and 401 challenge behavior on a security-sensitive MCP egress path, though defaults stay RFC 8693 and behavior is fail-closed with broad unit coverage.
Overview
Extends the v2 MCP token exchange (OBO) arm with a configurable wire dialect via
token_exchange_profile(rfc8693default,entra_obofor Microsoft Entra). The profile is persisted onMCPServer, config YAML, DB credentials JSON, and management APIMCPCredentials, and maps intoTokenExchangeConfig.profile. Unknown profile values normalize torfc8693.Rfc8693TokenExchangeris renamedOboTokenExchangerand builds either the RFC 8693 token-exchange form or Entra’s RFC 7523jwt-bearerform (assertion, requiredscope,requested_token_use=on_behalf_of). Cache keys includeprofile;entra_obowithout scope fails as misconfigured before any IdP POST.When Entra rejects exchange with a step-up
claimsblob (Conditional Access), the HTTP provider readserror+claims(noterror_description), surfaces them onSubjectTokenRejected/CredError.unauthorized, andraise_token_exchange_challengeemitserror=insufficient_claimswith base64claimsinWWW-Authenticate(unchanged static challenge when no claims). Preflight and credential resolution passclaimsthrough to that challenge.Reviewed by Cursor Bugbot for commit 5df7203. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests