security: Fix timing attack vulnerability in credential comparison (#1014)

Co-authored-by: Copilot AI <copilot@github.com>

.github/workflows/helm-package.yml

@@ -6,11 +6,16 @@ on:
     types: [completed]
 
 env:
-  new_version: ${{ github.ref_name }}
+  new_version: ${{ github.event.workflow_run.head_branch }}
 
 jobs:
   build-helm-package:
     runs-on: ubuntu-latest
+    # Only run on successful builds triggered by tag pushes (version format: x.y.z or x.y.z-suffix)
+    if: |
+      github.event.workflow_run.conclusion == 'success' && 
+      github.event.workflow_run.event == 'push' &&
+      contains(github.event.workflow_run.head_branch, '.')
 
     steps:
       - name: Checkout helm chart repo

Cargo.toml

@@ -152,6 +152,7 @@ rustls-pemfile = "2.2.0"
 rustls-pki-types = "1.13.1"
 sha1 = "0.11.0-rc.3"
 sha2 = "0.11.0-rc.3"
+subtle = "2.6"
 zeroize = { version = "1.8.2", features = ["derive"] }
 
 # Time and Date

rustfs/Cargo.toml

@@ -92,6 +92,7 @@ serde_urlencoded = { workspace = true }
 
 # Cryptography and Security
 rustls = { workspace = true }
+subtle = { workspace = true }
 
 # Time and Date
 chrono = { workspace = true }

rustfs/src/admin/handlers/group.rs

@@ -29,7 +29,7 @@ use tracing::warn;
 
 use crate::{
     admin::{auth::validate_admin_request, router::Operation, utils::has_space_be},
-    auth::{check_key_valid, get_session_token},
+    auth::{check_key_valid, constant_time_eq, get_session_token},
 };
 
 #[derive(Debug, Deserialize, Default)]
@@ -240,7 +240,7 @@ impl Operation for UpdateGroupMembers {
 
                     get_global_action_cred()
                         .map(|cred| {
-                            if cred.access_key == *member {
+                            if constant_time_eq(&cred.access_key, member) {
                                 return Err(S3Error::with_message(
                                     S3ErrorCode::MethodNotAllowed,
                                     format!("can't add root {member}"),

rustfs/src/admin/handlers/service_account.rs

@@ -13,7 +13,7 @@
 // limitations under the License.
 
 use crate::admin::utils::has_space_be;
-use crate::auth::{get_condition_values, get_session_token};
+use crate::auth::{constant_time_eq, get_condition_values, get_session_token};
 use crate::{admin::router::Operation, auth::check_key_valid};
 use http::HeaderMap;
 use hyper::StatusCode;
@@ -83,7 +83,7 @@ impl Operation for AddServiceAccount {
             return Err(s3_error!(InvalidRequest, "get sys cred failed"));
         };
 
-        if sys_cred.access_key == create_req.access_key {
+        if constant_time_eq(&sys_cred.access_key, &create_req.access_key) {
             return Err(s3_error!(InvalidArgument, "can't create user with system access key"));
         }
 
@@ -107,7 +107,7 @@ impl Operation for AddServiceAccount {
             return Err(s3_error!(InvalidRequest, "iam not init"));
         };
 
-        let deny_only = cred.access_key == target_user || cred.parent_user == target_user;
+        let deny_only = constant_time_eq(&cred.access_key, &target_user) || constant_time_eq(&cred.parent_user, &target_user);
 
         if !iam_store
             .is_allowed(&Args {

rustfs/src/admin/handlers/user.rs

@@ -14,7 +14,7 @@
 
 use crate::{
     admin::{auth::validate_admin_request, router::Operation, utils::has_space_be},
-    auth::{check_key_valid, get_session_token},
+    auth::{check_key_valid, constant_time_eq, get_session_token},
 };
 use http::{HeaderMap, StatusCode};
 use matchit::Params;
@@ -95,7 +95,7 @@ impl Operation for AddUser {
         }
 
         if let Some(sys_cred) = get_global_action_cred() {
-            if sys_cred.access_key == ak {
+            if constant_time_eq(&sys_cred.access_key, ak) {
                 return Err(s3_error!(InvalidArgument, "can't create user with system access key"));
             }
         }
@@ -162,7 +162,7 @@ impl Operation for SetUserStatus {
             return Err(s3_error!(InvalidRequest, "get cred failed"));
         };
 
-        if input_cred.access_key == ak {
+        if constant_time_eq(&input_cred.access_key, ak) {
             return Err(s3_error!(InvalidArgument, "can't change status of self"));
         }
 

rustfs/src/auth.rs

@@ -29,9 +29,37 @@ use s3s::auth::SimpleAuth;
 use s3s::s3_error;
 use serde_json::Value;
 use std::collections::HashMap;
+use subtle::ConstantTimeEq;
 use time::OffsetDateTime;
 use time::format_description::well_known::Rfc3339;
 
+/// Performs constant-time string comparison to prevent timing attacks.
+///
+/// This function should be used when comparing sensitive values like passwords,
+/// API keys, or authentication tokens. It ensures the comparison time is
+/// independent of the position where strings differ and handles length differences
+/// securely.
+///
+/// # Security Note
+/// This implementation uses the `subtle` crate to provide cryptographically
+/// sound constant-time guarantees. The function is resistant to timing side-channel
+/// attacks and suitable for security-critical comparisons.
+///
+/// # Example
+/// ```
+/// use rustfs::auth::constant_time_eq;
+///
+/// let secret1 = "my-secret-key";
+/// let secret2 = "my-secret-key";
+/// let secret3 = "wrong-secret";
+///
+/// assert!(constant_time_eq(secret1, secret2));
+/// assert!(!constant_time_eq(secret1, secret3));
+/// ```
+pub fn constant_time_eq(a: &str, b: &str) -> bool {
+    a.as_bytes().ct_eq(b.as_bytes()).into()
+}
+
 // Authentication type constants
 const JWT_ALGORITHM: &str = "Bearer ";
 const SIGN_V2_ALGORITHM: &str = "AWS ";
@@ -111,7 +139,7 @@ pub async fn check_key_valid(session_token: &str, access_key: &str) -> S3Result<
 
     let sys_cred = cred.clone();
 
-    if cred.access_key != access_key {
+    if !constant_time_eq(&cred.access_key, access_key) {
         let Ok(iam_store) = rustfs_iam::get() else {
             return Err(S3Error::with_message(
                 S3ErrorCode::InternalError,
@@ -146,7 +174,8 @@ pub async fn check_key_valid(session_token: &str, access_key: &str) -> S3Result<
 
     cred.claims = if !claims.is_empty() { Some(claims) } else { None };
 
-    let mut owner = sys_cred.access_key == cred.access_key || cred.parent_user == sys_cred.access_key;
+    let mut owner =
+        constant_time_eq(&sys_cred.access_key, &cred.access_key) || constant_time_eq(&cred.parent_user, &sys_cred.access_key);
 
     // permitRootAccess
     if let Some(claims) = &cred.claims {
@@ -225,7 +254,7 @@ pub fn get_condition_values(
     let principal_type = if !username.is_empty() {
         if claims.is_some() {
             "AssumedRole"
-        } else if sys_cred.access_key == username {
+        } else if constant_time_eq(&sys_cred.access_key, &username) {
             "Account"
         } else {
             "User"
@@ -1102,4 +1131,21 @@ mod tests {
 
         assert_eq!(auth_type, AuthType::Unknown);
     }
+
+    #[test]
+    fn test_constant_time_eq() {
+        assert!(constant_time_eq("test", "test"));
+        assert!(!constant_time_eq("test", "Test"));
+        assert!(!constant_time_eq("test", "test1"));
+        assert!(!constant_time_eq("test1", "test"));
+        assert!(!constant_time_eq("", "test"));
+        assert!(constant_time_eq("", ""));
+
+        // Test with credentials-like strings
+        let key1 = "AKIAIOSFODNN7EXAMPLE";
+        let key2 = "AKIAIOSFODNN7EXAMPLE";
+        let key3 = "AKIAIOSFODNN7EXAMPLF";
+        assert!(constant_time_eq(key1, key2));
+        assert!(!constant_time_eq(key1, key3));
+    }
 }