Validate the authorization server metadata issuer on every discovery path - #3398
Validate the authorization server metadata issuer on every discovery path#3398maxisbey wants to merge 2 commits into
Conversation
📚 Documentation preview
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also examined the sibling failure path at src/mcp/client/auth/oauth2.py:711 — when all protected-resource-metadata fetches fail, credentials bound to a different issuer are discarded before any metadata is fetched. That is the deliberate SEP-2352 fail-closed behavior (the flow re-registers on the next pass), so it is not the same transient-wipe problem as the ASM-metadata case flagged inline.
Extended reasoning...
Two confirmed findings are posted inline (the ASM-rediscovery metadata wipe at oauth2.py:737 and the identity-assertion step-up scope union), so approval is ruled out. Of the three ruled-out candidates, two duplicate the confirmed findings; the third (oauth2.py:711, PRM-failure credential discard) is genuinely distinct and adjacent to the line-737 finding, so recording that it was checked and judged intentional adds information a reader would otherwise have to re-derive. I verified against the diff that _discard_credentials_bound_elsewhere runs before metadata fetch by design, with an in-code comment stating credentials bound to another server are dropped "whatever the rest of discovery does" per SEP-2352, and that a configured-issuer mismatch raises OAuthFlowError before any discard.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/mcp/client/auth/extensions/identity_assertion.py— pre-existing: the step-up union drops previously escalated grants because it never folds in the granted token's scope. On a 403 insufficient_scope,scope_to_request = union_scopes(self._scope, challenged)uses only the constructor scope, andself._scopeis never updated after an exchange, so a client whose ops need disjoint step-up scopes thrashes: each step-up mints a token missing the other op's earlier escalation, re-triggering a 403/exchange on every alternate call (worse after restart, where the stored token's scope is the only record). oauth2.py fixes exactly this by foldingcurrent_tokens.scopeinto the SEP-2350 union (lines 673, 752-753); mirror it here by unioningself._tokens.scope(recorded at lines 210-212) intoscope_to_request.Extended reasoning...
Path: IdentityAssertionOAuthProvider.async_auth_flow. First step-up: op A 403s with scope="extra1"; line 179 requests union(constructor "read", "extra1") -> token granted read extra1; lines 210-215 record scope on the token and persist it, but self._scope stays "read". Second step-up: op B 403s with scope="extra2"; line 179 again unions from self._scope only -> requests "read extra2", and the newly minted token no longer carries extra1. Op A now 403s again, re-minting a token without extra2, and so on: every alternate call is a full jwt-bearer exchange (assertion_provider call + token POST). After a process restart the same line makes the stored token's escalations unrecoverable in one step. This PR added the equivalent safeguard to OAuthClientProvider (src/mcp/client/auth/oauth2.py lines 671-673 read granted_scope from current_tokens before discovery and lines 748-753 fold it into the step-up union, per the SEP-2350 comment) but did not apply it to identity_assertion.py, which the diff otherwise touches (lines 196-205). Fix: include self._tokens.scope (when self._tokens
Verification: pre-existing — src/mcp/client/auth/extensions/identity_assertion.py:179
scope_to_request = union_scopes(self._scope, extract_scope_from_www_auth(response))unions only the constructor scope (self._scope, set once at line 126 and never updated) with the challenged scope; the granted token's scope (self._tokens.scope, recorded at lines 210-212 and persisted at line 215) is never folded in. So
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="docs/client/identity-assertion.md">
<violation number="1" location="docs/client/identity-assertion.md:57">
P2: When `issuer` includes a path, this instruction sends users to the root well-known URL, but discovery uses a path-aware URL and then finds no metadata. Document the issuer-derived RFC 8414 well-known URL instead.</violation>
<violation number="2" location="docs/client/identity-assertion.md:60">
P2: When metadata uses an accepted noncanonical spelling, `OAuthMetadata` parses it before the callback runs, so the callback does not receive the metadata’s literal issuer. Describe this as the normalized parsed value, or preserve the raw metadata string before promising the ID-JAG uses the server’s spelling.</violation>
</file>
<file name="src/mcp/client/auth/extensions/client_credentials.py">
<violation number="1" location="src/mcp/client/auth/extensions/client_credentials.py:30">
P2: When `issuer` contains a query or fragment, `_configure_issuer` accepts an issuer that cannot be used consistently for RFC 8414 discovery because discovery drops those components. Reject query and fragment components during construction so configuration fails immediately instead of producing a late metadata mismatch or binding to a different discovery URL.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
d85452b to
41bcd5a
Compare
There was a problem hiding this comment.
Beyond the inline findings, I also examined whether the new issuer= pin is bypassed on the token-refresh path, since _refresh_token in oauth2.py never calls _require_metadata_for_configured_issuer: a refresh token only reaches storage after a successful exchange that the pin already gated against the configured issuer, so refresh cannot replay credentials obtained from a non-pinned server, and the metadata-less refresh against the default /token endpoint is pre-existing base-provider behavior unchanged by this PR — so I did not raise it as a finding.
Extended reasoning...
The bug hunt reached its findings cap (exit reason max_bugs), so approval is off the table, and five confirmed findings are being posted inline on src/mcp/client/auth/oauth2.py and src/mcp/client/auth/extensions/client_credentials.py. This note records one additional concrete candidate investigated and ruled out this run: the token-refresh path bypassing the new issuer pin. I traced it independently — async_auth_flow (oauth2.py:605-607) builds a refresh via the base _refresh_token (oauth2.py:499-527) without the pin check, but stored tokens (and thus refresh tokens) for a pinned provider can only originate from an exchange that _require_metadata_for_configured_issuer already validated, and a refused issuer drops in-memory metadata and tokens; falling back to the default /token on the resource origin when no metadata is held is base-class behavior that predates this PR. This is new information not covered by my prior review comment (which concerned metadata overwrite on transient ASM failure) and is not restated in any inline finding.
41bcd5a to
0c244f6
Compare
The discovery step now knows which issuer the authorization server metadata must carry before fetching it: the PRM-advertised server, or on the 2025-03-26 no-PRM fallback the resource server's origin, which is what that well-known URL is built from (RFC 8414 section 3.3). The metadata issuer check runs on both paths instead of only when PRM was found (on the no-PRM path a root issuer rendered with its trailing slash still names the origin, and the origin is written the way metadata issuers are, so host case or an explicit default port in `server_url` do not matter). The SEP-2352 stored-credential binding is evaluated once against that same value before metadata discovery, so it also applies when no metadata is served and the default endpoints are used, and the special case that re-evaluated it against the served issuer goes away; newly registered clients are bound to it when metadata for it was found. When no protected resource metadata location answers and one of them failed with a server error (or 429), the flow now stops rather than reading that as "no such metadata", so a transient failure cannot send a client with a bound registration down the legacy path. A 403 insufficient_scope step-up takes the same path as a 401. Until now it re-authorized with whatever metadata was in memory, and with none after a restart, in which case it used the 2025-03-26 default endpoints on the resource server's origin regardless of what the server advertises and never consulted the binding. It now discovers first when no metadata is held (extract_resource_metadata_from_www_auth also reads the `resource_metadata` hint from a 403 challenge), then re-authorizes with the SEP-2350 scope union, keeping the granted scope in the union even if discovery dropped the old token; metadata already discovered in this process is reused as before. A 403 that is not a scope challenge is handed back to the caller instead of being retried unchanged, as IdentityAssertionOAuthProvider already does. That root-slash allowance is one helper, issuers_match, which credentials_match_issuer uses as well, so records stamped in either form keep matching on both paths. This brings the no-PRM path in line with the TypeScript client, which applies the section 3.3 check on every discovery path (that client also offers an opt-out; this one does not, as on the PRM path since 2.0).
ClientCredentialsOAuthProvider and PrivateKeyJWTOAuthProvider take an optional issuer keyword: the issuer identifier of the authorization server the fixed client_id (and secret) were issued by. When set, token requests, the client_credentials exchange and any refresh, are only built from discovered authorization server metadata whose issuer matches it; if discovery yields metadata for another server, or none at all, the flow stops with OAuthFlowError before the secret is attached or an assertion is minted, and the metadata and tokens held are dropped so the next request starts discovery again. When the resource advertises several authorization servers the one matching the configured issuer is used; the comparison is issuers_match (exact, root slash aside); a value that is not an http(s) URL is a ValueError. Omitting it keeps the current behaviour. This is the same "the authorization server is configuration" model that IdentityAssertionOAuthProvider already uses, made available to the two older machine-to-machine providers without changing their defaults.
0c244f6 to
11baaf3
Compare
There was a problem hiding this comment.
I re-reviewed the latest push and this run found no new bugs, but since it rewrites the client's OAuth discovery and issuer-validation logic end to end, a maintainer's review is still worthwhile.
What was reviewed:
- The reworked 401/403 flow in
src/mcp/client/auth/oauth2.py: the PRM loop now tries all SEP-985 fallback URLs before raising on a 5xx/429, and the up-front_expected_issuer()/ SEP-2352 discard ordering matches the PR description. - The new
issuer=gate inclient_credentials.py: construction-time URL validation, the_select_authorization_server()preference among advertised servers, and root-slash tolerance viaissuers_matchall line up with the docs and tests. - Ruled out an
issuer=bypass via token refresh: the gate's refusal clears cached metadata and tokens, so a later request starts discovery afresh rather than refreshing against a stale endpoint. - My earlier open thread about Step 2 treating an AS-metadata 5xx as "no metadata" (falling through to origin-default endpoints) still applies to this push.
Extended reasoning...
Overview
This PR reworks the client OAuth flow in src/mcp/client/auth/oauth2.py (issuer decided before metadata fetch, PRM 5xx/429 guard, 403 step-up folded into the 401 discovery path with SEP-2350 scope union), adds issuers_match to utils.py, and gives the two machine-to-machine providers in extensions/client_credentials.py an optional issuer= pin with a metadata gate and an AS-selection hook. Docs and roughly 550 lines of new tests accompany the change. This is the fourth reviewed push; the 17:15 commits visibly addressed several of my earlier inline findings: the PRM discovery loop now records a 5xx/429 and continues through the remaining fallback URLs, raising only in the for-else when no metadata was found; _checked_issuer rejects non-http(s) issuers at construction; _select_authorization_server prefers the advertised server matching the configured issuer; and _require_metadata_for_configured_issuer compares via issuers_match, tolerating the root-slash spelling.
Security risks
The whole diff is security-sensitive: it decides which server receives client secrets, registration data, and browser redirects. The direction is hardening (issuer validated on the legacy path per RFC 8414 §3.3, credential binding evaluated before the fetch, transient PRM failures no longer demoting to the legacy path), and I found no regression in this run. The one residual gap is the subject of my still-open inline thread from the previous review: Step 2 still breaks on an AS-metadata 5xx with oauth_metadata left None, so the flow proceeds to resource-origin default endpoints even when PRM named a different AS — pre-existing behaviour carried over from main, now partially mitigated only for providers configured with issuer=.
Level of scrutiny
High. Auth code, non-trivial control-flow restructuring (a large re-indent under a new condition), and several spec-compliance judgment calls (root-slash issuer equivalence, when to hard-fail discovery, what a 403 without insufficient_scope should do) that a maintainer should weigh as deliberate observable-behaviour changes under the repo's v2 compatibility contract. Approval is ruled out both by that sensitivity and by my own unresolved thread on the Step 2 5xx handling.
Other factors
The bug hunt exited on a dry streak over this push and ruled out the one candidate it raised (the refresh-path bypass of the issuer= gate — refusal clears metadata and tokens, so there is nothing to refresh against, and the new tests exercise exactly that). Test coverage is extensive and the repo enforces 100% branch coverage; the author has been responsive, with each prior round of findings answered by replies and follow-up commits. A short deferral acknowledging the progress and pointing at the one remaining open thread seemed more useful to the author than silence, given the PR changed substantively since my last review.
1 participant
The OAuth client now decides which issuer it expects before it fetches any authorization server metadata, and checks what follows against that one value: the metadata's
issuer(RFC 8414 section 3.3) on the legacy no-PRM path as well as the PRM path, the SEP-2352 binding of stored credentials, and the issuer recorded on a new registration. A 403 scope step-up goes through the same discovery when it holds no metadata yet, and the two machine-to-machine providers gain an optionalissuer=so pre-provisioned credentials can be tied to their authorization server.Motivation and Context
issuerwas not checked, and the credential binding was re-evaluated afterwards against whatever issuer the document named. RFC 8414 section 3.3 says such metadata must not be used, and the TypeScript client applies the check on every path. Discovery now computes the expected issuer once (the PRM-advertised server, else the resource server's origin written the way metadata issuers are), validates metadata against it unconditionally (a root issuer served with its trailing slash still names the origin), and evaluates the binding against it before the metadata is fetched, which also covers the case where no metadata is served at all and the default endpoints are used. The post-fetch special case goes away. When none of the resource metadata locations answers and one of them failed with a server error (or 429), the flow now stops instead of reading that as "no resource metadata", so a transient failure cannot send a client with a bound registration down the legacy path.403 insufficient_scopere-authorized with whatever metadata was in memory, and with none after a restart, in which case it used the default/authorizeand/tokenon the resource server's origin even for servers that publish metadata, and never consulted the binding. It now takes the 401 path: discover first when nothing is held (reading aresource_metadatahint from the challenge too), then re-authorize with the SEP-2350 union; metadata already discovered in the process is reused as before. A 403 that is not a scope challenge is returned to the caller instead of being retried unchanged, asIdentityAssertionOAuthProvideralready does.issuer=onClientCredentialsOAuthProviderandPrivateKeyJWTOAuthProvider. These providers carry a fixedclient_id(and a secret, or a signed assertion) and used whichever authorization server discovery produced; nothing else in the flow can say which server issued those credentials. The optional keyword names it: when the resource advertises several servers the matching one is used, and the token request is only built from metadata for that issuer; otherwise the flow stops withOAuthFlowErrorand drops the metadata and token it was holding, so the next request starts discovery afresh rather than refreshing against them. Same model asIdentityAssertionOAuthProvider; omitting it keeps today's behaviour.How Has This Been Tested?
New and updated tests in
tests/client/test_auth.pyandtests/client/auth/extensions/test_client_credentials.py: legacy metadata naming another issuer is refused; aserver_urlwith an upper-case host or explicit port, and a root issuer served with a trailing slash, still match the origin; a registration bound elsewhere is dropped before the metadata request and also when no metadata is served, from a 401 or a 403 challenge; a 5xx/429 on the resource metadata request stops the flow with stored credentials intact; the root-slash tolerance and its limit; a step-up after a restart discovers first and targets the advertised server; a non-challenge 403 ends the flow;issuer=on both providers (matching metadata with either root spelling, other issuer, no metadata, the configured server listed second, a non-URL value, and that a refusal leaves nothing behind to refresh with). Each new flow test fails againstmain'ssrc/. I also drove the provider throughhttpx2.AsyncClient(auth=...)andstreamable_http_clientagainst local test servers (PRM and legacy paths, with and without metadata, 401-first and 403-first, client-credentials with and withoutissuer=). Full suite, coverage,strict-no-cover, pyright and ruff pass locally.Breaking Changes
No API removals or signature changes; the new keyword is optional and last, and
mcp.client.auth.utilsgainsissuers_match. These are fixes that bring the client in line with specified behaviour; the observable differences:issuerother than its own origin now getsOAuthFlowError: Authorization server metadata issuer mismatch. As on the PRM path since 2.0 there is no client-side override (the TypeScript client has one; adding it here would be a separate decision).OAuthFlowErrorinstead of moving on to the legacy path.credentials_match_issuertreatshttps://hostandhttps://host/as the same issuer, so a record stamped in either form keeps matching on both paths.insufficient_scopeis no longer retried.Types of changes
Checklist
help wanted, or I'm a maintainer)Additional context
Two commits: the discovery change (expected issuer, binding before the fetch, step-up), then the
issuer=keyword. The discovery block inasync_auth_flowis re-indented under one new condition, so the whitespace-insensitive view is the easier read there. Two things are unchanged frommainand left for a separate change: how discovery state is carried between passes (metadata held from an earlier pass while a later one selects a different server), and the fallback to the 2025-03-26 default endpoints when an advertised authorization server's metadata cannot be fetched.AI Disclaimer