From eb8868397ee91057ee1680bd9268d5e7806da412 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Sun, 3 May 2026 21:53:36 +0800 Subject: [PATCH] fix: address security review follow-ups (#2781) --- crates/ecstore/src/rpc/http_auth.rs | 44 ++++++++++++++++++++++----- rustfs/src/admin/handlers/user.rs | 47 +++++++++++++++++++++++++++-- 2 files changed, 81 insertions(+), 10 deletions(-) diff --git a/crates/ecstore/src/rpc/http_auth.rs b/crates/ecstore/src/rpc/http_auth.rs index c494b54c7..b11d9b1d0 100644 --- a/crates/ecstore/src/rpc/http_auth.rs +++ b/crates/ecstore/src/rpc/http_auth.rs @@ -58,21 +58,37 @@ fn get_shared_secret() -> std::io::Result { }) } -/// Generate HMAC-SHA256 signature for the given data -fn generate_signature(secret: &str, url: &str, method: &Method, timestamp: i64) -> String { +/// Build the canonical payload covered by the RPC HMAC. +fn signature_payload(url: &str, method: &Method, timestamp: i64) -> String { let uri: Uri = url.parse().expect("Invalid URL"); let path_and_query = uri.path_and_query().unwrap(); let url = path_and_query.to_string(); - let data = format!("{url}|{method}|{timestamp}"); + format!("{url}|{method}|{timestamp}") +} + +/// Generate HMAC-SHA256 signature for the given data +fn generate_signature(secret: &str, url: &str, method: &Method, timestamp: i64) -> String { + let data = signature_payload(url, method, timestamp); let mut mac = HmacSha256::new_from_slice(secret.as_bytes()).expect("HMAC can take key of any size"); mac.update(data.as_bytes()); let result = mac.finalize(); general_purpose::STANDARD.encode(result.into_bytes()) } +fn verify_signature(secret: &str, url: &str, method: &Method, timestamp: i64, signature: &str) -> bool { + let Ok(signature) = general_purpose::STANDARD.decode(signature) else { + return false; + }; + + let data = signature_payload(url, method, timestamp); + let mut mac = HmacSha256::new_from_slice(secret.as_bytes()).expect("HMAC can take key of any size"); + mac.update(data.as_bytes()); + mac.verify_slice(&signature).is_ok() +} + /// Build headers with authentication signature pub fn build_auth_headers(url: &str, method: &Method, headers: &mut HeaderMap) -> std::io::Result<()> { let auth_headers = gen_signature_headers(url, method)?; @@ -126,12 +142,10 @@ pub fn verify_rpc_signature(url: &str, method: &Method, headers: &HeaderMap) -> return Err(std::io::Error::other("Request timestamp expired")); } - // Generate expected signature + // Verify signature with constant-time HMAC comparison. let secret = get_shared_secret()?; - let expected_signature = generate_signature(&secret, url, method, timestamp); - // Compare signatures - if signature != expected_signature { + if !verify_signature(&secret, url, method, timestamp, signature) { error!( "verify_rpc_signature: Invalid signature: url {}, method {}, timestamp {}, signature_len {}", url, @@ -355,6 +369,22 @@ mod tests { assert_eq!(error.to_string(), "Invalid signature"); } + #[test] + fn test_verify_signature_uses_hmac_verification() { + let secret = "test-secret"; + let url = "http://example.com/api/test"; + let method = Method::GET; + let timestamp = 1640995200; + let signature = generate_signature(secret, url, &method, timestamp); + let mut tampered = general_purpose::STANDARD.decode(&signature).unwrap(); + tampered[0] ^= 1; + let tampered_signature = general_purpose::STANDARD.encode(tampered); + + assert!(verify_signature(secret, url, &method, timestamp, &signature)); + assert!(!verify_signature(secret, url, &method, timestamp, &tampered_signature)); + assert!(!verify_signature(secret, url, &method, timestamp, "invalid-signature")); + } + #[test] fn test_invalid_signature_log_contract_excludes_secrets() { ensure_test_rpc_secret(); diff --git a/rustfs/src/admin/handlers/user.rs b/rustfs/src/admin/handlers/user.rs index cd46a1112..873e253c9 100644 --- a/rustfs/src/admin/handlers/user.rs +++ b/rustfs/src/admin/handlers/user.rs @@ -140,6 +140,7 @@ fn imported_service_account_status(status: &str) -> Option { } const SERVICE_ACCOUNT_PARENT_SCOPE_ERROR: &str = "service account parent is outside requester scope"; +const SERVICE_ACCOUNT_ACCESS_KEY_MISMATCH_ERROR: &str = "service account access key does not match import entry"; fn imported_service_account_parent_allowed(parent: &str, requester: &Credentials, owner: bool) -> bool { if parent.is_empty() { @@ -169,6 +170,13 @@ fn imported_service_account_parent_scope_failure( }) } +fn imported_service_account_access_key_failure(entry_access_key: &str, payload_access_key: &str) -> Option { + (entry_access_key != payload_access_key).then(|| IAMErrEntity { + name: entry_access_key.to_string(), + error: SERVICE_ACCOUNT_ACCESS_KEY_MISMATCH_ERROR.to_string(), + }) +} + pub struct AddUser {} #[async_trait::async_trait] impl Operation for AddUser { @@ -1014,6 +1022,11 @@ impl Operation for ImportIam { return Err(s3_error!(InvalidArgument, "has space be {ak}")); } + if let Some(err) = imported_service_account_access_key_failure(&ak, &req.access_key) { + failed.service_accounts.push(err); + continue; + } + if let Some(err) = imported_service_account_parent_scope_failure(&ak, &req.parent, &cred, owner) { failed.service_accounts.push(err); continue; @@ -1021,7 +1034,7 @@ impl Operation for ImportIam { let mut update = true; - if let Err(e) = iam_store.get_service_account(&req.access_key).await { + if let Err(e) = iam_store.get_service_account(&ak).await { if !matches!(e, rustfs_iam::error::Error::NoSuchServiceAccount(_)) { return Err(s3_error!(InvalidArgument, "failed to get service account {ak} {e}")); } @@ -1029,7 +1042,7 @@ impl Operation for ImportIam { } if update { - iam_store.delete_service_account(&req.access_key, true).await.map_err(|e| { + iam_store.delete_service_account(&ak, true).await.map_err(|e| { S3Error::with_message( S3ErrorCode::InternalError, format!("failed to delete service account {ak} {e}"), @@ -1251,7 +1264,8 @@ impl Operation for ImportIam { #[cfg(test)] mod tests { use super::{ - GROUP_POLICY_MAPPING_USER_TYPE, SERVICE_ACCOUNT_PARENT_SCOPE_ERROR, imported_service_account_parent_allowed, + GROUP_POLICY_MAPPING_USER_TYPE, SERVICE_ACCOUNT_ACCESS_KEY_MISMATCH_ERROR, SERVICE_ACCOUNT_PARENT_SCOPE_ERROR, + imported_service_account_access_key_failure, imported_service_account_parent_allowed, imported_service_account_parent_scope_failure, imported_service_account_status, should_check_deny_only, should_reject_group_import_name, should_restore_group_as_disabled, }; @@ -1399,6 +1413,33 @@ mod tests { ); } + #[test] + fn test_service_account_import_rejects_payload_access_key_mismatch() { + let payload = r#"{ + "svcalpha": { + "parent": "useralpha", + "accessKey": "svcbeta", + "secretKey": "svcAlphaSecret123", + "groups": [], + "claims": {}, + "sessionPolicy": null, + "status": "on", + "name": "uploaderKey", + "description": "alpha upload key", + "expiration": "1970-01-01T00:00:00Z" + } + }"#; + + let svc_accts: HashMap = serde_json::from_str(payload).unwrap(); + let req = svc_accts.get("svcalpha").unwrap(); + let err = imported_service_account_access_key_failure("svcalpha", &req.access_key) + .expect("mismatched service account access keys must be rejected"); + + assert_eq!(err.name, "svcalpha"); + assert_eq!(err.error, SERVICE_ACCOUNT_ACCESS_KEY_MISMATCH_ERROR); + assert!(imported_service_account_access_key_failure("svcalpha", "svcalpha").is_none()); + } + #[test] fn test_import_service_account_parent_allows_owner_restore() { let requester = Credentials {