fix(auth): validate discovered metadata issuer#996
Conversation
github-actions
Bot
added
T-core
18fba5a to
5e5a2fe
Compare
| match serde_json::from_slice::<AuthorizationMetadata>(response.body()) { | ||
| Ok(metadata) => Ok(Some(metadata)), | ||
| Ok(metadata) => { | ||
| Self::validate_authorization_metadata_issuer(discovery_url, &metadata)?; |
There was a problem hiding this comment.
I see an issuer mismatch hard fails with ? here. So, it's not worth trying the remaining discovery URL candidates?
There was a problem hiding this comment.
The spec actually seems a bit ambiguous - it only says we must reject the metadata document if there's a mismatch, not necessarily the whole flow. I looked at https://github.com/modelcontextprotocol/typescript-sdk/blob/e81758caed29f6568ce8873f7f9a3bd65b017d9c/packages/client/src/client/auth.ts though, which also hard-fails on a mismatch, so I think this is probably the way we should go.
There was a problem hiding this comment.
Makes sense, agreed. Worth noting the TS SDK pairs this with an escape hatch though, skipIssuerValidation: true, documented as security-weakening for known-misconfigured servers. Might be worth a follow-up for parity, but no need to block this PR on it.
5e5a2fe to
f230960
Compare
alexhancock
left a comment
alexhancock
left a comment
There was a problem hiding this comment.
Code LGTM - left one recommendation
Also check into the failing checks
| metadata: &AuthorizationMetadata, | ||
| ) -> Result<(), AuthError> { | ||
| let Some(expected_issuer) = | ||
| Self::expected_issuer_for_authorization_metadata_url(discovery_url) |
There was a problem hiding this comment.
I would inline the logic of this, as it is only called once
|
@jamadeo Can you take care of the failing checks before merging? |
Validate authorization server metadata issuers against the issuer implied by standard discovery URLs before trusting advertised endpoints. fixes #983
f230960 to
e209d41
Compare
3 participants
Motivation and Context
Validate authorization server metadata issuers against the issuer implied by standard discovery URLs before trusting advertised endpoints. Reject mismatches while preserving compatibility for metadata that omits issuer.
fixes #983
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context