feat(auth): bind DCR client credentials to issuing authorization server (SEP-2352)#998
feat(auth): bind DCR client credentials to issuing authorization server (SEP-2352)#998alexhancock wants to merge 1 commit into
Conversation
github-actions
Bot
added
T-core
| token_response: Some(token_result.clone()), | ||
| granted_scopes, | ||
| token_received_at: Some(Self::now_epoch_secs()), | ||
| issuer: self.metadata_issuer(), |
There was a problem hiding this comment.
We now stamp issuer: self.metadata_issuer() here, but nothing reads it back. After a migration, what stops a static pre-registered credential from being silently sent to the new AS, the case where SEP-2352 says clients "SHOULD surface an error"?
| token_response: Some(credentials), | ||
| granted_scopes, | ||
| token_received_at: Some(AuthorizationManager::now_epoch_secs()), | ||
| issuer: None, |
There was a problem hiding this comment.
Since discover_metadata() runs right after the save, the issuer is actually available in this function. Was keeping the save before discovery intentional, or could the stamp be filled in after discovery succeeds?
| if let (Some(stored_issuer), Some(current_issuer)) = | ||
| (stored.issuer.as_deref(), self.metadata_issuer().as_deref()) |
There was a problem hiding this comment.
SEP-2352 explicitly says CIMD client IDs "are portable across authorization servers … No re-registration is required." The mismatch check here doesn't distinguish credential types.
| token_response, | ||
| granted_scopes, | ||
| token_received_at, | ||
| issuer: None, |
There was a problem hiding this comment.
How can external CredentialStore implementors set issuer?
| if stored_issuer != current_issuer { | ||
| self.credential_store.clear().await?; | ||
| return Ok(false); | ||
| } |
There was a problem hiding this comment.
Clearing stored credentials is destructive. Should we add a warning log here?
2 participants
Motivation and Context
Per modelcontextprotocol/modelcontextprotocol#2352 clients SHOULD maintain separate registration state per auth server and MUST NOT assume cross-AS credential validity
How Has This Been Tested?
Conformance tests
Breaking Changes
None.
StoredCredentialsis #[non_exhaustive] and has anewconstructorTypes of changes
Checklist
Additional context
Closes #879