diff --git a/crates/rmcp/src/transport/auth.rs b/crates/rmcp/src/transport/auth.rs index 759044b0c..fb55530cb 100644 --- a/crates/rmcp/src/transport/auth.rs +++ b/crates/rmcp/src/transport/auth.rs @@ -224,6 +224,9 @@ const DEFAULT_APPLICATION_TYPE: &str = "native"; #[non_exhaustive] pub struct StoredCredentials { pub client_id: String, + /// Client authentication material required for token refresh after restart. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub client_secret: Option, pub token_response: Option, #[serde(default)] pub granted_scopes: Vec, @@ -237,6 +240,10 @@ impl std::fmt::Debug for StoredCredentials { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { f.debug_struct("StoredCredentials") .field("client_id", &self.client_id) + .field( + "client_secret", + &self.client_secret.as_ref().map(|_| "[REDACTED]"), + ) .field( "token_response", &self.token_response.as_ref().map(|_| "[REDACTED]"), @@ -258,6 +265,7 @@ impl StoredCredentials { ) -> Self { Self { client_id, + client_secret: None, token_response, granted_scopes, token_received_at, @@ -265,6 +273,12 @@ impl StoredCredentials { } } + /// Retain client authentication with the token grant. Empty secrets denote public clients. + pub fn with_client_secret(mut self, secret: Option) -> Self { + self.client_secret = secret.filter(|value| !value.secret().is_empty()); + self + } + pub fn with_issuer(mut self, issuer: Option) -> Self { self.issuer = issuer; self @@ -1105,6 +1119,7 @@ pub struct AuthorizationManager { refresh_redirect_policy: OAuthHttpRedirectPolicy, metadata: Option, oauth_client: Option, + client_secret: Option, credential_store: Arc, state_store: Arc, base_url: Url, @@ -1352,6 +1367,7 @@ impl AuthorizationManager { refresh_redirect_policy, metadata: None, oauth_client: None, + client_secret: None, credential_store: Arc::new(InMemoryCredentialStore::new()), state_store: Arc::new(InMemoryStateStore::new()), base_url, @@ -1464,7 +1480,9 @@ impl AuthorizationManager { } } - self.configure_client_id(&stored.client_id)?; + let mut config = OAuthClientConfig::new(&stored.client_id, self.base_url.to_string()); + config.client_secret = stored.client_secret.map(|secret| secret.secret().clone()); + self.configure_client(config)?; return Ok(true); } Ok(false) @@ -1612,6 +1630,11 @@ impl AuthorizationManager { Ok((client_id.to_string(), token_response)) } + /// Return the secret needed to persist this client registration. + pub fn client_secret(&self) -> Option<&ClientSecret> { + self.client_secret.as_ref() + } + /// configure oauth2 client with client credentials pub fn configure_client(&mut self, config: OAuthClientConfig) -> Result<(), AuthError> { if self.metadata.is_none() { @@ -1640,8 +1663,12 @@ impl AuthorizationManager { .set_token_uri(token_url) .set_redirect_uri(redirect_url); - if let Some(secret) = config.client_secret { - client_builder = client_builder.set_client_secret(ClientSecret::new(secret)); + let client_secret = config + .client_secret + .filter(|secret| !secret.is_empty()) + .map(ClientSecret::new); + if let Some(secret) = &client_secret { + client_builder = client_builder.set_client_secret(secret.clone()); } let uses_secret_post = metadata @@ -1662,6 +1689,7 @@ impl AuthorizationManager { } self.oauth_client = Some(client_builder); + self.client_secret = client_secret; Ok(()) } /// validate authorization server metadata before starting authorization. @@ -2162,6 +2190,7 @@ impl AuthorizationManager { let client_id = oauth_client.client_id().to_string(); let stored = StoredCredentials { client_id, + client_secret: self.client_secret.clone(), token_response: Some(token_result.clone()), granted_scopes, token_received_at: Some(Self::now_epoch_secs()), @@ -2319,6 +2348,7 @@ impl AuthorizationManager { let client_id = oauth_client.client_id().to_string(); let stored = StoredCredentials { client_id, + client_secret: self.client_secret.clone(), token_response: Some(token_result.clone()), granted_scopes, token_received_at: Some(Self::now_epoch_secs()), @@ -3143,6 +3173,7 @@ impl AuthorizationManager { let client_id = config.client_id().to_string(); let stored = StoredCredentials { client_id, + client_secret: None, token_response: Some(token_result.clone()), granted_scopes, token_received_at: Some(Self::now_epoch_secs()), @@ -3264,6 +3295,7 @@ impl AuthorizationManager { let stored = StoredCredentials { client_id: client_id.clone(), + client_secret: None, token_response: Some(token_result.clone()), granted_scopes, token_received_at: Some(Self::now_epoch_secs()), @@ -3669,6 +3701,15 @@ impl OAuthState { } } + /// Return registration authentication material for external credential storage. + pub fn client_secret(&self) -> Option<&ClientSecret> { + match self { + Self::Unauthorized(manager) | Self::Authorized(manager) => manager.client_secret(), + Self::Session(session) => session.auth_manager.client_secret(), + Self::AuthorizedHttpClient(client) => client.auth_manager.client_secret(), + } + } + /// Manually set credentials and move into authorized state /// Useful if you're caching credentials externally and wish to reuse them pub async fn set_credentials( @@ -3697,6 +3738,7 @@ impl OAuthState { let stored = StoredCredentials { client_id: client_id.to_string(), + client_secret: None, token_response: Some(credentials), granted_scopes, token_received_at: Some(AuthorizationManager::now_epoch_secs()), @@ -6258,6 +6300,7 @@ mod tests { ); let creds = StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: Some(token_response), granted_scopes: vec![], token_received_at: None, @@ -6453,6 +6496,7 @@ mod tests { store .save(StoredCredentials { client_id: "dcr-client".to_string(), + client_secret: None, token_response: Some(make_token_response("old-token", Some(3600))), granted_scopes: vec![], token_received_at: Some(AuthorizationManager::now_epoch_secs()), @@ -7391,6 +7435,7 @@ mod tests { let manager = AuthorizationManager::new("http://localhost").await.unwrap(); let stored = StoredCredentials { client_id: "test".to_string(), + client_secret: None, token_response: Some(make_token_response("my-access-token", Some(3600))), granted_scopes: vec![], token_received_at: Some(AuthorizationManager::now_epoch_secs()), @@ -7409,6 +7454,7 @@ mod tests { let stored = StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: Some(make_token_response("stale-token", Some(3600))), granted_scopes: vec![], token_received_at: Some(AuthorizationManager::now_epoch_secs() - 7200), @@ -7428,6 +7474,7 @@ mod tests { let manager = AuthorizationManager::new("http://localhost").await.unwrap(); let stored = StoredCredentials { client_id: "test".to_string(), + client_secret: None, token_response: Some(make_token_response("no-expiry-token", None)), granted_scopes: vec![], token_received_at: None, @@ -7446,6 +7493,7 @@ mod tests { let stored = StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: Some(make_token_response("almost-expired", Some(3600))), granted_scopes: vec![], token_received_at: Some(AuthorizationManager::now_epoch_secs() - 3590), @@ -7465,6 +7513,7 @@ mod tests { let manager = AuthorizationManager::new("http://localhost").await.unwrap(); let stored = StoredCredentials { client_id: "test".to_string(), + client_secret: None, token_response: Some(make_token_response("stale-token", Some(3600))), granted_scopes: vec![], token_received_at: Some(AuthorizationManager::now_epoch_secs() - 7200), @@ -7878,6 +7927,7 @@ mod tests { .credential_store .save(StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: Some(make_token_response_with_refresh( "old-token", "my-refresh-token", @@ -7910,6 +7960,7 @@ mod tests { let stored = StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: None, granted_scopes: vec![], token_received_at: None, @@ -7931,6 +7982,7 @@ mod tests { let stored = StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: Some(make_token_response("old-token", Some(3600))), granted_scopes: vec![], token_received_at: Some(AuthorizationManager::now_epoch_secs()), @@ -8019,6 +8071,7 @@ mod tests { .credential_store .save(StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: Some(make_token_response_with_refresh( "old-token", "my-refresh-token", @@ -8225,6 +8278,7 @@ mod tests { let stored = StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: Some(make_token_response_with_refresh( "old-token", "my-refresh-token", @@ -8263,6 +8317,7 @@ mod tests { let stored = StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: Some(make_token_response_with_refresh( "old-token", "my-refresh-token", @@ -8301,6 +8356,7 @@ mod tests { let stored = StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: Some(make_token_response_with_refresh( "old-token", "my-refresh-token", @@ -8339,6 +8395,7 @@ mod tests { let stored = StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: Some(make_token_response_with_refresh( "old-token", "my-refresh-token", @@ -8378,6 +8435,7 @@ mod tests { let stored = StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: Some(make_token_response_with_refresh( "old-token", "my-refresh-token", @@ -8442,6 +8500,7 @@ mod tests { let stored = StoredCredentials { client_id: "my-client".to_string(), + client_secret: None, token_response: Some(make_token_response_with_refresh( "old-token", "my-refresh-token", @@ -8805,3 +8864,7 @@ mod tests { assert!(store.lock.try_lock().is_ok()); } } + +#[cfg(test)] +#[path = "auth/client_secret_tests.rs"] +mod client_secret_tests; diff --git a/crates/rmcp/src/transport/auth/client_secret_tests.rs b/crates/rmcp/src/transport/auth/client_secret_tests.rs new file mode 100644 index 000000000..c46c2e362 --- /dev/null +++ b/crates/rmcp/src/transport/auth/client_secret_tests.rs @@ -0,0 +1,173 @@ +use std::sync::atomic::{AtomicUsize, Ordering}; + +use super::*; + +#[test] +fn legacy_credentials_remain_public_and_debug_redacts_secrets() { + let mut stored: StoredCredentials = serde_json::from_value(serde_json::json!({ + "client_id": "public", "token_response": null + })) + .unwrap(); + assert!(stored.client_secret.is_none()); + stored = stored.with_client_secret(Some(ClientSecret::new("synthetic-secret".into()))); + let encoded = serde_json::to_string(&stored).unwrap(); + let restored: StoredCredentials = serde_json::from_str(&encoded).unwrap(); + assert_eq!(restored.client_secret.unwrap().secret(), "synthetic-secret"); + assert!(!format!("{stored:?}").contains("synthetic-secret")); + assert!( + stored + .with_client_secret(Some(ClientSecret::new(String::new()))) + .client_secret + .is_none() + ); +} + +#[tokio::test] +async fn issuer_change_discards_the_registered_secret_before_restoration() { + for client_id in ["registered-client", "https://client.example/metadata.json"] { + let mut manager = AuthorizationManager::new("https://resource.example") + .await + .unwrap(); + manager.set_metadata(AuthorizationMetadata { + authorization_endpoint: "https://new.example/authorize".into(), + token_endpoint: "https://new.example/token".into(), + issuer: Some("https://new.example".into()), + ..Default::default() + }); + let stored = serde_json::from_value(serde_json::json!({ + "client_id": client_id, "client_secret": "synthetic-secret", + "issuer": "https://old.example", + "token_response": {"access_token":"old-access", "token_type":"Bearer"} + })) + .unwrap(); + manager.credential_store.save(stored).await.unwrap(); + assert!(!manager.initialize_from_store().await.unwrap()); + assert!(manager.client_secret().is_none()); + if let Some(saved) = manager.credential_store.load().await.unwrap() { + assert!(saved.client_secret.is_none()); + assert!(saved.token_response.is_none()); + } + } +} + +#[tokio::test] +async fn restored_clients_authenticate_refresh_and_keep_rotating_credentials() { + use std::collections::HashMap; + + use axum::{Router, body::Bytes, http::HeaderMap, routing::post}; + use base64::Engine; + + for method in ["client_secret_basic", "client_secret_post", "none"] { + let calls = Arc::new(AtomicUsize::new(0)); + let observed = calls.clone(); + let app = Router::new().route( + "/token", + post(move |headers: HeaderMap, body: Bytes| { + let calls = observed.clone(); + async move { + let form: HashMap<_, _> = + url::form_urlencoded::parse(&body).into_owned().collect(); + let count = calls.fetch_add(1, Ordering::SeqCst); + assert_eq!(form["grant_type"], "refresh_token"); + assert_eq!( + form["refresh_token"], + if count == 0 { + "initial-refresh" + } else { + "rotated-refresh" + } + ); + assert!(form["resource"].starts_with("http://127.0.0.1:")); + match method { + "client_secret_basic" => { + let value = base64::engine::general_purpose::STANDARD + .encode("test-client:synthetic-secret"); + assert_eq!(headers["authorization"], format!("Basic {value}")); + assert!(!form.contains_key("client_secret")); + } + "client_secret_post" => { + assert_eq!(form["client_id"], "test-client"); + assert_eq!(form["client_secret"], "synthetic-secret"); + assert!(!headers.contains_key("authorization")); + } + _ => { + assert_eq!(form["client_id"], "test-client"); + assert!(!form.contains_key("client_secret")); + assert!(!headers.contains_key("authorization")); + } + } + let mut response = serde_json::json!({ + "access_token": "new-access", "token_type": "Bearer", "expires_in": 3600 + }); + if count == 0 { + response["refresh_token"] = "rotated-refresh".into(); + } + axum::http::Response::builder() + .header("content-type", "application/json") + .body(axum::body::Body::from(response.to_string())) + .unwrap() + } + }), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let base = format!("http://{}", listener.local_addr().unwrap()); + let server = tokio::spawn(async move { axum::serve(listener, app).await.unwrap() }); + let mut stored: StoredCredentials = serde_json::from_value(serde_json::json!({ + "client_id": "test-client", + "token_response": {"access_token":"old-access", "token_type":"Bearer", "expires_in":1, "refresh_token":"initial-refresh"}, + "token_received_at": 0, + "issuer": base + })).unwrap(); + if method != "none" { + stored = stored.with_client_secret(Some(ClientSecret::new("synthetic-secret".into()))); + } + for _ in 0..2 { + // Only serialized credentials survive between manager instances. + let encoded = serde_json::to_vec(&stored).unwrap(); + let mut manager = AuthorizationManager::new(&base).await.unwrap(); + let mut metadata = AuthorizationMetadata { + authorization_endpoint: format!("{base}/authorize"), + token_endpoint: format!("{base}/token"), + issuer: Some(base.clone()), + ..Default::default() + }; + metadata.additional_fields.insert( + "token_endpoint_auth_methods_supported".into(), + serde_json::json!([method]), + ); + manager.set_metadata(metadata); + manager + .credential_store + .save(serde_json::from_slice(&encoded).unwrap()) + .await + .unwrap(); + assert!(manager.initialize_from_store().await.unwrap()); + manager.refresh_token().await.unwrap(); + stored = manager.credential_store.load().await.unwrap().unwrap(); + assert_eq!( + stored + .client_secret + .as_ref() + .map(|secret| secret.secret().as_str()), + if method == "none" { + None + } else { + Some("synthetic-secret") + } + ); + assert_eq!( + stored + .token_response + .as_ref() + .unwrap() + .refresh_token() + .unwrap() + .secret(), + "rotated-refresh" + ); + assert!(!format!("{stored:?}").contains("synthetic-secret")); + } + assert_eq!(calls.load(Ordering::SeqCst), 2); + server.abort(); + } +}