Skip to content

Bind client credentials to their authorization server (SEP-2352)#2933

Merged
maxisbey merged 2 commits into
mainfrom
sep-2352-as-binding
Jun 20, 2026
Merged

Bind client credentials to their authorization server (SEP-2352)#2933
maxisbey merged 2 commits into
mainfrom
sep-2352-as-binding

Conversation

@Kludex

@Kludex Kludex commented Jun 20, 2026

Copy link
Copy Markdown
Member

Implements SEP-2352 (the last client-side item of #2902): bind OAuth client credentials to their authorization server and re-register when it changes.

What changed

Persisted client credentials are now bound to the issuer that registered them. OAuthClientInformationFull gains an SDK-owned issuer field, set after DCR/CIMD. Before reusing stored credentials, the provider compares their recorded issuer to the authorization server discovered from protected resource metadata; on a mismatch it discards the credentials (and the old tokens) and re-registers, instead of presenting one server's client_id to another.

  • CIMD (URL-based client IDs): portable across servers, always match - no re-registration.
  • No recorded issuer (pre-registered, or stored before this change): no binding to enforce, left as-is.
  • No TokenStorage protocol change - the issuer round-trips through the existing get_client_info/set_client_info, so the 8 in-repo implementations and downstream users are unaffected.

Follows the Go SDK's approach (issuer field on the client-info, proactive comparison). The TS SDK takes a reactive invalidate-on-error path that doesn't fit our httpx auth-generator cleanly.

Conformance

The auth/authorization-server-migration scenario's no-reuse-on-as-change and no-cross-as-credential-reuse checks pass (the client never presents the old AS's client_id to the new AS). The reregister-on-as-change check is not yet reached: the scenario runs only at 2026-07-28, where client.py's 2025 stateful lifecycle is rejected (400 on initialize) before the migration 401 fires. The scenario therefore stays baselined in both expected-failures files, re-annotated to point at the stateless-lifecycle gap (the same gap blocking tools_call, scope-step-up, etc. on the 2026 leg) rather than SEP-2352. An in-process interaction test covers the migration end to end.

Both conformance legs pass with no stale or unexpected entries.

#2902 status

This completes the implementation of all five client-side items (SEP-2207, SEP-2468, SEP-837, SEP-2350, SEP-2352). The AS-migration conformance scenario will go green once the 2026 stateless client lifecycle lands.

AI Disclaimer

This PR was developed with the assistance of either Claude or Codex. I've reviewed and verified the changes.

Kludex added 2 commits June 20, 2026 19:26
Persisted client credentials are now bound to the issuer that registered them:
OAuthClientInformationFull records an issuer, set by the SDK after DCR/CIMD.
When protected resource metadata points at a different authorization server,
the client discards the bound credentials and old tokens and re-registers,
instead of presenting one server's client_id to another.

URL-based client IDs (CIMD) are portable and always match; credentials with no
recorded issuer (pre-registered, or stored before this change) carry no binding
to enforce and are left as-is. No TokenStorage protocol change - the issuer
round-trips through the existing get_client_info/set_client_info.

Follows the Go SDK's approach. The auth/authorization-server-migration
conformance scenario's re-register check is satisfied in spirit (no-reuse and
no-cross-AS checks pass) but the scenario stays baselined: it runs at
2026-07-28, where client.py's 2025 lifecycle is rejected before the migration
401 fires. It unblocks with the 2026 stateless client lifecycle.
- Stamp the bound issuer from the discovered oauth_metadata.issuer when PRM did
  not advertise an authorization server (legacy no-PRM path), instead of leaving
  it None — otherwise migrated resources could reuse the old DCR client_id.
- Detect CIMD portability by the client_id equaling the configured
  client_metadata_url, not by URL shape, so a registration server that issues a
  URL-shaped client_id is still treated as bound to its issuer.
@Kludex

Kludex commented Jun 20, 2026

Copy link
Copy Markdown
Member Author

Ran a codex review pass, which raised two valid P2 edge cases — both fixed in 523b32b:

  1. Legacy no-PRM DCR persisted issuer=None. When PRM discovery fails but AS-metadata discovery succeeds, auth_server_url is None, so DCR creds were stamped with no issuer — a later PRM pointing elsewhere would reuse them. Now the bound issuer falls back to the discovered oauth_metadata.issuer.
  2. CIMD inferred from URL shape. A registration server returning a URL-shaped client_id was misclassified as portable CIMD. Portability is now keyed on the client_id equaling the configured client_metadata_url, not URL syntax. Added a regression test (test_credentials_match_issuer_url_shaped_dcr_id_is_not_portable).

Full suite + 100% coverage + both conformance legs still green.

@maxisbey maxisbey merged commit 4472428 into main Jun 20, 2026
32 checks passed
@maxisbey maxisbey deleted the sep-2352-as-binding branch June 20, 2026 17:47

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't find any bugs in this change, but it modifies the OAuth client credential-handling path (issuer binding, credential/token invalidation, and re-registration logic), so it warrants a human security-minded review rather than an automated approval.

Extended reasoning...

Overview

The PR implements SEP-2352: OAuthClientInformationFull gains an SDK-owned issuer field, the OAuth client provider stamps it after DCR/CIMD registration, and a new credentials_match_issuer helper drops stored credentials (and tokens) when the discovered authorization server no longer matches the recorded issuer. Supporting changes include conformance expected-failure annotations, migration-doc text, unit tests for the matching helper, and an end-to-end interaction test covering the migration scenario.

Security risks

This is security-relevant code by nature: it governs whether persisted client credentials and tokens are reused or discarded across authorization servers. The logic looks sound — CIMD portability is keyed on the client_id equaling the configured client_metadata_url (not URL shape), legacy credentials with no recorded issuer are left alone, and the no-PRM fallback binds to the discovered metadata issuer. The main residual considerations are policy ones (e.g. leaving unbound legacy credentials reusable across AS changes is an intentional compatibility choice, and the issuer comparison is a simple string compare per RFC 3986 §6.2.1), which are reasonable but worth a maintainer's eyes.

Level of scrutiny

OAuth/auth client code in the SDK warrants a higher bar than a mechanical change; per my approval criteria I do not auto-approve security-sensitive paths even when no bugs are found. The change is moderate in size and well-scoped, but it alters when credentials and tokens are invalidated mid-flow, which can affect existing users with persisted storage.

Other factors

Test coverage is good: targeted unit tests for credentials_match_issuer (including the URL-shaped DCR id regression case) and an interaction test asserting the stale client_id never reaches the authorize/token endpoints and that re-registration occurs. The author also already addressed two edge cases raised by a prior automated review pass (legacy no-PRM issuer fallback, CIMD misclassification) in 523b32b. Conformance baselines are updated with clear annotations rather than silently dropped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants