Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 625
fix(auth): validate discovered metadata issuer#996
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1876,14 +1876,89 @@ impl AuthorizationManager { | ||
| } | ||
| match serde_json::from_slice::<AuthorizationMetadata>(response.body()) { | ||
| Ok(metadata) => Ok(Some(metadata)), | ||
| Ok(metadata) => { | ||
| Self::validate_authorization_metadata_issuer(discovery_url, &metadata)?; | ||
| Ok(Some(metadata)) | ||
| } | ||
| Err(err) => { | ||
| debug!("Failed to parse metadata for {}: {}", discovery_url, err); | ||
| Ok(None) // malformed JSON ⇒ try next candidate | ||
| } | ||
| } | ||
| } | ||
| fn expected_issuer_for_authorization_metadata_url(discovery_url: &Url) -> Option<String> { | ||
| let path = discovery_url.path(); | ||
| let oauth_prefix = "/.well-known/oauth-authorization-server"; | ||
| let oidc_prefix = "/.well-known/openid-configuration"; | ||
| let issuer_path = if path == oauth_prefix || path == oidc_prefix { | ||
| "" | ||
| } else if let Some(suffix) = path.strip_prefix(&format!("{oauth_prefix}/")) { | ||
| // RFC 8414 path-insertion form | ||
| suffix | ||
| } else if let Some(suffix) = path.strip_prefix(&format!("{oidc_prefix}/")) { | ||
| // MCP-required OpenID Connect path-insertion compatibility form | ||
| suffix | ||
| } else if let Some(prefix) = path.strip_suffix(oidc_prefix) { | ||
| // OpenID Connect path-appended form | ||
| prefix.trim_start_matches('/') | ||
jamadeo marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } else { | ||
| return None; | ||
| }; | ||
| let mut issuer = discovery_url.clone(); | ||
| issuer.set_query(None); | ||
| issuer.set_fragment(None); | ||
| if issuer_path.is_empty() { | ||
| issuer.set_path(""); | ||
| } else { | ||
| issuer.set_path(&format!("/{issuer_path}")); | ||
| } | ||
| Some(issuer.to_string()) | ||
| } | ||
| fn issuer_identifiers_match(received_issuer: &str, expected_issuer: &str) -> bool { | ||
| if received_issuer == expected_issuer { | ||
| return true; | ||
| } | ||
| let trim_root_slash = |issuer: &str| -> String { | ||
| issuer | ||
| .strip_suffix('/') | ||
| .filter(|without_slash| { | ||
| Url::parse(without_slash) | ||
| .map(|url| url.path().is_empty() || url.path() == "/") | ||
| .unwrap_or(false) | ||
| }) | ||
| .unwrap_or(issuer) | ||
| .to_string() | ||
| }; | ||
| trim_root_slash(received_issuer) == trim_root_slash(expected_issuer) | ||
| } | ||
| fn validate_authorization_metadata_issuer( | ||
| discovery_url: &Url, | ||
| metadata: &AuthorizationMetadata, | ||
| ) -> Result<(), AuthError> { | ||
| let Some(expected_issuer) = | ||
| Self::expected_issuer_for_authorization_metadata_url(discovery_url) | ||
Contributor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would inline the logic of this, as it is only called once | ||
| else { | ||
| return Ok(()); | ||
| }; | ||
| let Some(received_issuer) = metadata.issuer.as_deref() else { | ||
| return Err(AuthError::AuthorizationServerMissingIssuer { expected_issuer }); | ||
| }; | ||
| if !Self::issuer_identifiers_match(received_issuer, &expected_issuer) { | ||
| return Err(AuthError::AuthorizationServerMismatch { | ||
| expected_issuer, | ||
| received_issuer: received_issuer.to_string(), | ||
| }); | ||
| } | ||
| Ok(()) | ||
| } | ||
| async fn discover_oauth_server_via_resource_metadata( | ||
| &self, | ||
| ) -> Result<Option<AuthorizationMetadata>, AuthError> { | ||
| @@ -3367,6 +3442,7 @@ mod tests { | ||
| http_response( | ||
| 200, | ||
| serde_json::json!({ | ||
| "issuer": "https://auth.example.com", | ||
| "authorization_endpoint": "https://auth.example.com/authorize", | ||
| "token_endpoint": "https://auth.example.com/token" | ||
| }), | ||
| @@ -3467,6 +3543,125 @@ mod tests { | ||
| ); | ||
| } | ||
| #[tokio::test] | ||
| async fn authorization_metadata_rejects_mismatched_issuer() { | ||
| let client = RecordingOAuthHttpClient::with_responses(vec![ | ||
| empty_response(401), | ||
| http_response( | ||
| 200, | ||
| serde_json::json!({ | ||
| "resource": "https://mcp.example.com/", | ||
| "authorization_servers": ["https://auth.example.com/tenant1"] | ||
| }), | ||
| ), | ||
| http_response( | ||
| 200, | ||
| serde_json::json!({ | ||
| "resource": "https://mcp.example.com/", | ||
| "authorization_servers": ["https://auth.example.com/tenant1"] | ||
| }), | ||
| ), | ||
| http_response( | ||
| 200, | ||
| serde_json::json!({ | ||
| "issuer": "https://evil.example.com/tenant1", | ||
| "authorization_endpoint": "https://evil.example.com/tenant1/authorize", | ||
| "token_endpoint": "https://evil.example.com/tenant1/token" | ||
| }), | ||
| ), | ||
| ]); | ||
| let manager = AuthorizationManager::new_with_oauth_http_client( | ||
| "https://mcp.example.com/", | ||
| Arc::new(client), | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
| let error = manager.discover_metadata().await.unwrap_err(); | ||
| assert!( | ||
| matches!( | ||
| error, | ||
| AuthError::AuthorizationServerMismatch { | ||
| ref expected_issuer, | ||
| ref received_issuer | ||
| } if expected_issuer == "https://auth.example.com/tenant1" | ||
| && received_issuer == "https://evil.example.com/tenant1" | ||
| ), | ||
| "expected authorization server issuer mismatch, got: {error:?}" | ||
| ); | ||
| } | ||
| #[test] | ||
| fn authorization_metadata_accepts_oidc_path_appended_issuer() { | ||
| let discovery_url = | ||
| Url::parse("https://auth.example.com/tenant1/.well-known/openid-configuration") | ||
| .unwrap(); | ||
| let metadata = AuthorizationMetadata { | ||
| issuer: Some("https://auth.example.com/tenant1".to_string()), | ||
| authorization_endpoint: "https://auth.example.com/tenant1/authorize".to_string(), | ||
| token_endpoint: "https://auth.example.com/tenant1/token".to_string(), | ||
| ..Default::default() | ||
| }; | ||
| AuthorizationManager::validate_authorization_metadata_issuer(&discovery_url, &metadata) | ||
| .unwrap(); | ||
| } | ||
| #[test] | ||
| fn authorization_metadata_allows_only_root_trailing_slash_equivalence() { | ||
| assert!(AuthorizationManager::issuer_identifiers_match( | ||
| "https://auth.example.com/", | ||
| "https://auth.example.com" | ||
| )); | ||
| assert!(!AuthorizationManager::issuer_identifiers_match( | ||
| "https://auth.example.com/tenant1/", | ||
| "https://auth.example.com/tenant1" | ||
| )); | ||
| } | ||
| #[test] | ||
| fn authorization_metadata_accepts_oidc_path_inserted_issuer() { | ||
| let discovery_url = | ||
| Url::parse("https://auth.example.com/.well-known/openid-configuration/tenant1") | ||
| .unwrap(); | ||
| let metadata = AuthorizationMetadata { | ||
| issuer: Some("https://auth.example.com/tenant1".to_string()), | ||
| authorization_endpoint: "https://auth.example.com/tenant1/authorize".to_string(), | ||
| token_endpoint: "https://auth.example.com/tenant1/token".to_string(), | ||
| ..Default::default() | ||
| }; | ||
| AuthorizationManager::validate_authorization_metadata_issuer(&discovery_url, &metadata) | ||
| .unwrap(); | ||
| } | ||
| #[test] | ||
| fn authorization_metadata_rejects_missing_issuer_for_standard_discovery_url() { | ||
| let discovery_url = | ||
| Url::parse("https://auth.example.com/.well-known/openid-configuration/tenant1") | ||
| .unwrap(); | ||
| let metadata = AuthorizationMetadata { | ||
| issuer: None, | ||
| authorization_endpoint: "https://auth.example.com/tenant1/authorize".to_string(), | ||
| token_endpoint: "https://auth.example.com/tenant1/token".to_string(), | ||
| ..Default::default() | ||
| }; | ||
| let error = | ||
| AuthorizationManager::validate_authorization_metadata_issuer(&discovery_url, &metadata) | ||
| .unwrap_err(); | ||
| assert!( | ||
| matches!( | ||
| error, | ||
| AuthError::AuthorizationServerMissingIssuer { ref expected_issuer } | ||
| if expected_issuer == "https://auth.example.com/tenant1" | ||
| ), | ||
| "expected missing issuer error, got: {error:?}" | ||
| ); | ||
| } | ||
| #[tokio::test] | ||
| async fn protected_resource_metadata_supports_custom_location_and_oidc_path_append() { | ||
| let challenge = oauth2::http::Response::builder() | ||
| @@ -3672,6 +3867,7 @@ mod tests { | ||
| http_response( | ||
| 200, | ||
| serde_json::json!({ | ||
| "issuer": "https://auth.example.com", | ||
| "authorization_endpoint": "https://auth.example.com/authorize", | ||
| "token_endpoint": "https://auth.example.com/token" | ||
| }), | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.