From 9ffedc0e2b48764d10b99bf54fe3d7d2fa633d8b Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Wed, 8 Jul 2026 18:31:48 +0800 Subject: [PATCH] fix(keystone): fail closed on EC2 auth and apply configured timeout (#4464) --- crates/keystone/src/auth.rs | 58 ++++++++++----- crates/keystone/src/client.rs | 70 +++++++------------ crates/keystone/src/config.rs | 16 +++++ crates/keystone/src/error.rs | 7 ++ crates/keystone/src/identity.rs | 1 + crates/keystone/src/lib.rs | 16 +---- crates/keystone/src/middleware.rs | 1 + .../tests/integration/middleware_tests.rs | 2 + rustfs/src/auth_keystone.rs | 1 + 9 files changed, 94 insertions(+), 78 deletions(-) diff --git a/crates/keystone/src/auth.rs b/crates/keystone/src/auth.rs index b687b38e8..7673fc105 100644 --- a/crates/keystone/src/auth.rs +++ b/crates/keystone/src/auth.rs @@ -120,24 +120,14 @@ impl KeystoneAuthProvider { } /// Convert EC2 credential to Keystone token - async fn ec2_to_keystone_token(&self, ec2_cred: &EC2Credential) -> Result { - // In a real implementation, you'd need to: - // 1. Use admin credentials to get user/project details - // 2. Or maintain a mapping table - // For simplicity, construct a minimal token - - Ok(KeystoneToken { - token: String::new(), - user_id: ec2_cred.user_id.clone(), - username: ec2_cred.user_id.clone(), // Use user_id as username - project_id: ec2_cred.project_id.clone(), - project_name: ec2_cred.project_id.clone(), - domain_id: None, - domain_name: None, - roles: vec!["Member".to_string()], // Default role - expires_at: time::OffsetDateTime::now_utc() + time::Duration::hours(24), - issued_at: time::OffsetDateTime::now_utc(), - }) + /// + /// Fails closed. A correct implementation must resolve the full identity + /// (user, project, roles, expiry) from Keystone using admin credentials or + /// a trusted mapping. It must never fabricate roles or derive identity from + /// client-supplied input. Until that is modeled against a live server, this + /// path refuses to mint a token. + async fn ec2_to_keystone_token(&self, _ec2_cred: &EC2Credential) -> Result { + Err(KeystoneError::Ec2AuthUnsupported) } /// Convert Keystone token to RustFS credentials @@ -236,6 +226,7 @@ mod tests { None, "Default".to_string(), true, + Duration::from_secs(30), ); let provider = KeystoneAuthProvider::new(client, 100, Duration::from_secs(60), true); @@ -275,6 +266,7 @@ mod tests { None, "Default".to_string(), true, + Duration::from_secs(30), ); let provider = KeystoneAuthProvider::new(client, 100, Duration::from_secs(60), true); @@ -291,4 +283,34 @@ mod tests { cred.groups = Some(vec!["Admin".to_string()]); assert!(provider.is_admin(&cred)); } + + #[tokio::test] + async fn test_ec2_authentication_fails_closed() { + // The EC2/SigV4 path must never fabricate an identity from client input. + // It must fail closed rather than produce credentials. + let client = KeystoneClient::new( + "http://localhost:5000".to_string(), + crate::KeystoneVersion::V3, + None, + None, + None, + "Default".to_string(), + true, + Duration::from_secs(30), + ); + + let provider = KeystoneAuthProvider::new(client, 100, Duration::from_secs(60), true).without_cache(); + + // A `:`-bearing access key previously shaped user_id/project_id. It must + // no longer produce any credentials. + let result = provider + .authenticate_with_ec2("attacker:admin-project", "sig", "string-to-sign") + .await; + + assert!(result.is_err(), "EC2 auth must not return fabricated credentials"); + assert!( + matches!(result, Err(KeystoneError::Ec2AuthUnsupported)), + "EC2 auth must fail closed with Ec2AuthUnsupported" + ); + } } diff --git a/crates/keystone/src/client.rs b/crates/keystone/src/client.rs index 9453aabed..ffb5fcdd4 100644 --- a/crates/keystone/src/client.rs +++ b/crates/keystone/src/client.rs @@ -33,6 +33,8 @@ pub struct KeystoneClient { admin_domain: String, #[allow(dead_code)] verify_ssl: bool, + /// Request timeout applied to the underlying HTTP client. + timeout: std::time::Duration, } #[derive(Clone)] @@ -49,6 +51,7 @@ impl AdminToken { impl KeystoneClient { /// Create new Keystone client + #[allow(clippy::too_many_arguments)] pub fn new( auth_url: String, version: KeystoneVersion, @@ -57,6 +60,7 @@ impl KeystoneClient { admin_project: Option, admin_domain: String, verify_ssl: bool, + timeout: std::time::Duration, ) -> Self { if !verify_ssl { warn!( @@ -67,7 +71,7 @@ impl KeystoneClient { let client = Client::builder() .danger_accept_invalid_certs(!verify_ssl) - .timeout(std::time::Duration::from_secs(30)) + .timeout(timeout) .build() .unwrap(); @@ -81,9 +85,15 @@ impl KeystoneClient { admin_project, admin_domain, verify_ssl, + timeout, } } + /// Request timeout applied to the underlying HTTP client. + pub fn timeout(&self) -> std::time::Duration { + self.timeout + } + /// Validate a Keystone token pub async fn validate_token(&self, token: &str) -> Result { match self.version { @@ -209,53 +219,22 @@ impl KeystoneClient { } /// Validate EC2 credentials + /// + /// This path is intentionally not implemented and fails closed. A correct + /// implementation must derive the identity (user, project, roles, expiry) + /// from the Keystone `/v3/ec2tokens` response, not from client-supplied + /// input. The previous implementation discarded the response body and + /// fabricated an identity by splitting the client access key on `:`, which + /// let a caller shape their own user_id/project_id. Until the response + /// schema is modeled against a live server, refuse to produce credentials. pub async fn validate_ec2_credentials( &self, - access_key: &str, - signature: &str, - string_to_sign: &str, + _access_key: &str, + _signature: &str, + _string_to_sign: &str, ) -> Result { - let url = format!("{}/v3/ec2tokens", self.auth_url); - - debug!("Validating EC2 credentials: access_key={}", access_key); - - let payload = json!({ - "auth": { - "identity": { - "methods": ["ec2"], - "ec2": { - "access": access_key, - "signature": signature, - "data": string_to_sign - } - } - } - }); - - let response = self - .client - .post(&url) - .json(&payload) - .send() - .await - .map_err(|e| KeystoneError::HttpError(e.to_string()))?; - - if !response.status().is_success() { - return Err(KeystoneError::InvalidCredentials); - } - - let _body: serde_json::Value = response.json().await.map_err(|e| KeystoneError::ParseError(e.to_string()))?; - - // Parse access key to extract user_id and project_id - let (user_id, project_id) = EC2Credential::parse_access_key(access_key).unwrap_or_else(|| (access_key.to_string(), None)); - - Ok(EC2Credential { - access: access_key.to_string(), - secret: String::new(), // Secret not returned in validation - user_id, - project_id, - trust_id: None, - }) + warn!("EC2/SigV4 credential authentication is not supported; refusing to fabricate identity from client input"); + Err(KeystoneError::Ec2AuthUnsupported) } /// Get EC2 credentials for a user @@ -436,6 +415,7 @@ mod tests { Some("admin".to_string()), "Default".to_string(), true, + std::time::Duration::from_secs(30), ); assert_eq!(client.auth_url, "http://keystone:5000"); diff --git a/crates/keystone/src/config.rs b/crates/keystone/src/config.rs index 85f06686c..d675a55a2 100644 --- a/crates/keystone/src/config.rs +++ b/crates/keystone/src/config.rs @@ -268,6 +268,22 @@ mod tests { assert!(!config.enable_tenant_prefix); assert!(!config.implicit_tenants); assert_eq!(config.timeout_seconds, 99); + + // The configured timeout must actually be forwarded to the + // client, not silently dropped in favour of a hardcoded value. + assert_eq!(config.get_timeout(), Duration::from_secs(99)); + + let client = crate::KeystoneClient::new( + config.auth_url.clone(), + KeystoneVersion::V3, + config.admin_user.clone(), + config.admin_password.clone(), + config.admin_project.clone(), + config.get_admin_domain(), + config.verify_ssl, + config.get_timeout(), + ); + assert_eq!(client.timeout(), Duration::from_secs(99)); }, ); } diff --git a/crates/keystone/src/error.rs b/crates/keystone/src/error.rs index 501996a06..835e3dc89 100644 --- a/crates/keystone/src/error.rs +++ b/crates/keystone/src/error.rs @@ -51,6 +51,13 @@ pub enum KeystoneError { #[error("Unsupported Keystone version")] UnsupportedVersion, + /// EC2/SigV4 credential authentication is not supported + /// + /// The EC2 path deliberately fails closed rather than fabricating an + /// identity from client-supplied input. + #[error("EC2 credential authentication is not supported")] + Ec2AuthUnsupported, + /// Project not found #[error("Project not found")] ProjectNotFound, diff --git a/crates/keystone/src/identity.rs b/crates/keystone/src/identity.rs index 93a99d647..96e45de17 100644 --- a/crates/keystone/src/identity.rs +++ b/crates/keystone/src/identity.rs @@ -248,6 +248,7 @@ mod tests { None, "Default".to_string(), true, + std::time::Duration::from_secs(30), ); KeystoneIdentityMapper::new(Arc::new(client), true) } diff --git a/crates/keystone/src/lib.rs b/crates/keystone/src/lib.rs index 1b6d2b35f..a6227040b 100644 --- a/crates/keystone/src/lib.rs +++ b/crates/keystone/src/lib.rs @@ -40,6 +40,7 @@ //! config.admin_project.clone(), //! config.get_admin_domain(), //! config.verify_ssl, +//! config.get_timeout(), //! ); //! //! let auth_provider = KeystoneAuthProvider::new( @@ -141,21 +142,6 @@ pub struct EC2Credential { pub trust_id: Option, } -impl EC2Credential { - /// Parse access key to extract user_id and project_id - /// - /// Format: "user_id:project_id" or "user_id" - pub fn parse_access_key(access_key: &str) -> Option<(String, Option)> { - if access_key.contains(':') { - let parts: Vec<&str> = access_key.split(':').collect(); - if parts.len() == 2 { - return Some((parts[0].to_string(), Some(parts[1].to_string()))); - } - } - Some((access_key.to_string(), None)) - } -} - /// Token cache for performance optimization #[derive(Clone)] pub struct TokenCache { diff --git a/crates/keystone/src/middleware.rs b/crates/keystone/src/middleware.rs index 807dec096..769e9edd7 100644 --- a/crates/keystone/src/middleware.rs +++ b/crates/keystone/src/middleware.rs @@ -242,6 +242,7 @@ mod tests { None, "Default".to_string(), true, + Duration::from_secs(30), ); let provider = KeystoneAuthProvider::new(client, 100, Duration::from_secs(60), true); let layer = KeystoneAuthLayer::new(Some(Arc::new(provider))); diff --git a/crates/keystone/tests/integration/middleware_tests.rs b/crates/keystone/tests/integration/middleware_tests.rs index 459eccf9c..ea16f808b 100644 --- a/crates/keystone/tests/integration/middleware_tests.rs +++ b/crates/keystone/tests/integration/middleware_tests.rs @@ -28,6 +28,7 @@ fn create_test_auth_provider() -> Arc { Some("admin".to_string()), "Default".to_string(), false, // Don't verify SSL for tests + std::time::Duration::from_secs(30), ); Arc::new(KeystoneAuthProvider::new(client, 1000, std::time::Duration::from_secs(300), true)) @@ -256,6 +257,7 @@ fn test_auth_provider_configuration() { Some("test-project".to_string()), "TestDomain".to_string(), true, + std::time::Duration::from_secs(30), ); // Test with caching enabled diff --git a/rustfs/src/auth_keystone.rs b/rustfs/src/auth_keystone.rs index 812c3d061..9c7b3159c 100644 --- a/rustfs/src/auth_keystone.rs +++ b/rustfs/src/auth_keystone.rs @@ -66,6 +66,7 @@ pub async fn init_keystone_auth(config: KeystoneConfig) -> Result<(), Box