fix: address security review follow-ups (#2781)

crates/ecstore/src/rpc/http_auth.rs

@@ -58,21 +58,37 @@ fn get_shared_secret() -> std::io::Result<String> {
     })
 }
 
-/// Generate HMAC-SHA256 signature for the given data
+/// Build the canonical payload covered by the RPC HMAC.
-fn generate_signature(secret: &str, url: &str, method: &Method, timestamp: i64) -> String {
+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 !verify_signature(&secret, url, method, timestamp, signature) {
-    if signature != expected_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();

rustfs/src/admin/handlers/user.rs

@@ -140,6 +140,7 @@ fn imported_service_account_status(status: &str) -> Option<String> {
 }
 
 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<IAMErrEntity> {
+    (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<String, SRSvcAccCreate> = 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 {