refactor(validator): remove hardcoded dummy key for timing protection (#18)

ssddOnTop · autofix-ci[bot] · web-flow · commit 4db6c9d72581 · 2025-12-13T15:22:14.000-05:00
Co-authored-by: autofix-ci[bot] &lt;114827586+autofix-ci[bot]@users.noreply.github.com&gt;
diff --git a/crates/api-keys-simplified/src/domain.rs b/crates/api-keys-simplified/src/domain.rs
@@ -1,4 +1,4 @@
-use crate::error::ConfigError;
+use crate::error::InitError;
 use crate::validator::KeyStatus;
 use crate::{
     config::{Environment, KeyConfig, KeyPrefix},
@@ -53,12 +53,17 @@ impl ApiKeyManagerV0 {
         prefix: impl Into<String>,
         config: KeyConfig,
         hash_config: HashConfig,
-    ) -> std::result::Result<Self, ConfigError> {
+    ) -> std::result::Result<Self, InitError> {
         let include_checksum = *config.checksum_length() != 0;
         let prefix = KeyPrefix::new(prefix)?;
-        let generator = KeyGenerator::new(prefix, config);
-        let validator = KeyValidator::new(&hash_config, include_checksum)?;
-        let hasher = KeyHasher::new(hash_config);
+        let generator = KeyGenerator::new(prefix, config)?;
+        let hasher = KeyHasher::new(hash_config.clone());
+
+        // Generate dummy key and its hash for timing attack protection
+        let dummy_key = generator.dummy_key().clone();
+        let dummy_hash = hasher.hash(&dummy_key)?;
+
+        let validator = KeyValidator::new(&hash_config, include_checksum, dummy_key, dummy_hash)?;
 
         Ok(Self {
             generator,
@@ -68,14 +73,12 @@ impl ApiKeyManagerV0 {
         })
     }
 
-    pub fn init_default_config(
-        prefix: impl Into<String>,
-    ) -> std::result::Result<Self, ConfigError> {
+    pub fn init_default_config(prefix: impl Into<String>) -> std::result::Result<Self, InitError> {
         Self::init(prefix, KeyConfig::default(), HashConfig::default())
     }
     pub fn init_high_security_config(
         prefix: impl Into<String>,
-    ) -> std::result::Result<Self, ConfigError> {
+    ) -> std::result::Result<Self, InitError> {
         Self::init(
             prefix,
             KeyConfig::high_security(),
diff --git a/crates/api-keys-simplified/src/error.rs b/crates/api-keys-simplified/src/error.rs
@@ -80,6 +80,19 @@ pub enum OperationError {
 
 pub type Result<T> = std::result::Result<T, Error>;
 
+/// Error type for initialization operations that can fail with either
+/// configuration errors or runtime operation errors.
+#[derive(Debug, Error)]
+pub enum InitError {
+    /// Configuration error during initialization
+    #[error(transparent)]
+    Config(#[from] ConfigError),
+
+    /// Operation error during initialization (i.e., dummy key generation)
+    #[error(transparent)]
+    Operation(#[from] Error),
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/crates/api-keys-simplified/src/generator.rs b/crates/api-keys-simplified/src/generator.rs
@@ -13,17 +13,38 @@ use zeroize::Zeroizing;
 // Prevent DoS: Validate input length before processing
 const MAX_KEY_LENGTH: usize = 512;
 const CHECKSUM_SEPARATOR: u8 = b'.';
-static DUMMY_KEY: &str = "dummy_key_for_timing_protection";
 
 #[derive(Clone)]
 pub struct KeyGenerator {
     prefix: KeyPrefix,
     config: KeyConfig,
+    /// Dummy key for timing attack protection
+    dummy_key: SecureString,
 }
 
 impl KeyGenerator {
-    pub fn new(prefix: KeyPrefix, config: KeyConfig) -> KeyGenerator {
-        Self { prefix, config }
+    pub fn new(prefix: KeyPrefix, config: KeyConfig) -> Result<KeyGenerator> {
+        // Generate a dummy key for timing attack protection
+        // This is used in verify_checksum when input is invalid
+        let dummy_generator = Self {
+            prefix: prefix.clone(),
+            config: config.clone(),
+            dummy_key: SecureString::from(String::new()), // Temporary placeholder
+        };
+
+        let dummy_key = dummy_generator.generate(Environment::Production, None)?;
+
+        Ok(Self {
+            prefix,
+            config,
+            dummy_key,
+        })
+    }
+
+    /// Returns a reference to the dummy key for timing attack protection.
+    /// This is used by KeyValidator to perform dummy work.
+    pub(crate) fn dummy_key(&self) -> &SecureString {
+        &self.dummy_key
     }
 
     fn generate_key(&self) -> Result<Zeroizing<Vec<u8>>> {
@@ -153,7 +174,7 @@ impl KeyGenerator {
         if key_bytes.len() > MAX_KEY_LENGTH {
             // Perform fake work to prevent timing side-channel attacks
             // This ensures rejection takes similar time as actual verification
-            let _ = self.compute_checksum(DUMMY_KEY, None);
+            let _ = self.compute_checksum(self.dummy_key.expose_secret(), None);
             return Err(Error::InvalidFormat);
         }
 
@@ -163,7 +184,7 @@ impl KeyGenerator {
             Ok((_, parts)) => parts,
             Err(_) => {
                 // If parsing fails, perform dummy work for timing consistency
-                let _ = self.compute_checksum(DUMMY_KEY, None);
+                let _ = self.compute_checksum(self.dummy_key.expose_secret(), None);
                 return Ok(false);
             }
         };
@@ -172,15 +193,15 @@ impl KeyGenerator {
         let checksum_bytes = match parts.checksum {
             Some(c) => c,
             None => {
-                let _ = self.compute_checksum(DUMMY_KEY, None);
+                let _ = self.compute_checksum(self.dummy_key.expose_secret(), None);
                 return Ok(false);
             }
         };
 
         let computed = match self.compute_checksum(parts.key, parts.expiry_b64) {
             Some(computed) => computed,
             None => {
-                let _ = self.compute_checksum(DUMMY_KEY, None);
+                let _ = self.compute_checksum(self.dummy_key.expose_secret(), None);
                 return Ok(false);
             }
         };
@@ -285,7 +306,7 @@ mod tests {
         let config = KeyConfig::default();
         let checksum_len = *config.checksum_length();
 
-        let generator = KeyGenerator::new(prefix, config);
+        let generator = KeyGenerator::new(prefix, config).unwrap();
         let key = generator.generate(env, None).unwrap();
         assert!(key.expose_secret().starts_with("sk-live-"));
 
@@ -332,7 +353,7 @@ mod tests {
         let env = Environment::Test;
         let config = KeyConfig::default();
 
-        let generator = KeyGenerator::new(prefix, config);
+        let generator = KeyGenerator::new(prefix, config).unwrap();
         let key = generator.generate(env, None).unwrap();
 
         // Verify checksum is separated by '.' (enabled by default)
@@ -375,11 +396,11 @@ mod tests {
         let env = Environment::Development;
 
         let config16 = KeyConfig::new().with_entropy(16).unwrap();
-        let generator16 = KeyGenerator::new(prefix.clone(), config16);
+        let generator16 = KeyGenerator::new(prefix.clone(), config16).unwrap();
         let key16 = generator16.generate(env.clone(), None).unwrap();
 
         let config32 = KeyConfig::new().with_entropy(32).unwrap();
-        let generator32 = KeyGenerator::new(prefix, config32);
+        let generator32 = KeyGenerator::new(prefix, config32).unwrap();
         let key32 = generator32.generate(env, None).unwrap();
 
         assert!(key32.len() > key16.len());
@@ -392,7 +413,7 @@ mod tests {
         let config = KeyConfig::default();
         let checksum_len = *config.checksum_length();
 
-        let generator = KeyGenerator::new(prefix, config);
+        let generator = KeyGenerator::new(prefix, config).unwrap();
         let key = generator.generate(env, None).unwrap();
 
         // With dash separator and checksum (default): test-live-data.checksum
@@ -432,15 +453,15 @@ mod tests {
 
         // Test with Slash
         let config_slash = KeyConfig::default().with_separator(Separator::Slash);
-        let generator_slash = KeyGenerator::new(prefix.clone(), config_slash);
+        let generator_slash = KeyGenerator::new(prefix.clone(), config_slash).unwrap();
         let key_slash = generator_slash.generate(env.clone(), None).unwrap();
         assert!(key_slash.expose_secret().contains('/'));
         assert!(!key_slash.expose_secret().contains('~'));
         assert!(generator_slash.verify_checksum(&key_slash).unwrap());
 
         // Test with Dash (default)
         let config_dash = KeyConfig::default().with_separator(Separator::Dash);
-        let generator_dash = KeyGenerator::new(prefix.clone(), config_dash);
+        let generator_dash = KeyGenerator::new(prefix.clone(), config_dash).unwrap();
         let key_dash = generator_dash.generate(env.clone(), None).unwrap();
         assert!(key_dash.expose_secret().contains('-'));
         // Checksum is always separated by dot
@@ -450,7 +471,7 @@ mod tests {
 
         // Test with Tilde
         let config_tilde = KeyConfig::default().with_separator(Separator::Tilde);
-        let generator_tilde = KeyGenerator::new(prefix, config_tilde);
+        let generator_tilde = KeyGenerator::new(prefix, config_tilde).unwrap();
         let key_tilde = generator_tilde.generate(env, None).unwrap();
         assert!(key_tilde.expose_secret().contains('~'));
         assert!(key_tilde.len() > 10);
diff --git a/crates/api-keys-simplified/src/lib.rs b/crates/api-keys-simplified/src/lib.rs
@@ -44,7 +44,7 @@ pub use config::{
     ChecksumAlgo, Environment, HashConfig, KeyConfig, KeyPrefix, KeyVersion, Separator,
 };
 pub use domain::{ApiKey, ApiKeyManagerV0, Hash, NoHash};
-pub use error::{ConfigError, Error, Result};
+pub use error::{ConfigError, Error, InitError, Result};
 pub use secure::{SecureString, SecureStringExt};
 pub use validator::KeyStatus;
 
diff --git a/crates/api-keys-simplified/src/validator.rs b/crates/api-keys-simplified/src/validator.rs
@@ -1,6 +1,6 @@
 use crate::error::{ConfigError, Error, Result};
 use crate::token_parser::{parse_token, Parts};
-use crate::HashConfig;
+use crate::SecureString;
 use argon2::{
     password_hash::{PasswordHash, PasswordVerifier},
     Argon2,
@@ -13,6 +13,8 @@ use password_hash::PasswordHashString;
 pub struct KeyValidator {
     hash: PasswordHashString,
     has_checksum: bool,
+    /// Dummy password for timing attack protection (should be a generated API key)
+    dummy_password: SecureString,
 }
 
 /// Represents the status of an API key after verification
@@ -31,14 +33,19 @@ impl KeyValidator {
     const MAX_HASH_LENGTH: usize = 512;
 
     pub fn new(
-        hash_config: &HashConfig,
+        _hash_config: &crate::HashConfig,
         has_checksum: bool,
+        dummy_key: SecureString,
+        dummy_hash: String,
     ) -> std::result::Result<KeyValidator, ConfigError> {
-        let dummy_hash = format!("$argon2id$v=19$m={},t={},p={}$0bJKH8iokgID0PWXnrsXvw$oef42xfOKBQMkCpvoQTeVHLhsYf+EQWMc2u4Ebn1MUo", hash_config.memory_cost(), hash_config.time_cost(), hash_config.parallelism());
         let hash =
             PasswordHashString::new(&dummy_hash).map_err(|_| ConfigError::InvalidArgon2Hash)?;
 
-        Ok(KeyValidator { hash, has_checksum })
+        Ok(KeyValidator {
+            hash,
+            has_checksum,
+            dummy_password: dummy_key,
+        })
     }
 
     fn verify_expiry(&self, parts: Parts) -> Result<KeyStatus> {
@@ -113,12 +120,12 @@ impl KeyValidator {
         // SECURITY: Perform dummy Argon2 verification to match timing of real verification
         // This prevents timing attacks that could distinguish between "invalid hash format"
         // and "valid hash but wrong password" errors
-        let dummy_password =
-            b"text-v1-test-okphUY-aqllb-qHoZDC9mVlm5sY9lvmm.AAAAAGk2Mvg.a54368d6331bf42dc18c";
-        parse_token(dummy_password, self.has_checksum).ok();
+        use crate::ExposeSecret;
+        let dummy_bytes = self.dummy_password.expose_secret().as_bytes();
+        parse_token(dummy_bytes, self.has_checksum).ok();
 
         Argon2::default()
-            .verify_password(dummy_password, &self.hash.password_hash())
+            .verify_password(dummy_bytes, &self.hash.password_hash())
             .ok();
     }
 }
@@ -129,13 +136,22 @@ mod tests {
     use crate::ExposeSecret;
     use crate::{config::HashConfig, hasher::KeyHasher, SecureString};
 
+    fn dummy_key_and_hash() -> (SecureString, String) {
+        let key = SecureString::from("sk-live-dummy123test".to_string());
+        let hasher = KeyHasher::new(HashConfig::default());
+        let hash = hasher.hash(&key).unwrap();
+        (key, hash)
+    }
+
     #[test]
     fn test_verification() {
         let key = SecureString::from("sk_live_testkey123".to_string());
         let hasher = KeyHasher::new(HashConfig::default());
         let hash = hasher.hash(&key).unwrap();
 
-        let validator = KeyValidator::new(&HashConfig::default(), true).unwrap();
+        let (dummy_key, dummy_hash) = dummy_key_and_hash();
+        let validator =
+            KeyValidator::new(&HashConfig::default(), true, dummy_key, dummy_hash).unwrap();
         assert_eq!(
             validator
                 .verify(key.expose_secret(), hash.as_ref())
@@ -150,7 +166,9 @@ mod tests {
 
     #[test]
     fn test_invalid_hash_format() {
-        let validator = KeyValidator::new(&HashConfig::default(), true).unwrap();
+        let (dummy_key, dummy_hash) = dummy_key_and_hash();
+        let validator =
+            KeyValidator::new(&HashConfig::default(), true, dummy_key, dummy_hash).unwrap();
         let result = validator.verify("any_key", "invalid_hash");
         // After timing oracle fix: invalid hash format returns Ok(Invalid) instead of Err
         // to prevent timing-based user enumeration attacks
@@ -165,7 +183,9 @@ mod tests {
         let hasher = KeyHasher::new(HashConfig::default());
         let hash = hasher.hash(&valid_key).unwrap();
 
-        let validator = KeyValidator::new(&HashConfig::default(), true).unwrap();
+        let (dummy_key, dummy_hash) = dummy_key_and_hash();
+        let validator =
+            KeyValidator::new(&HashConfig::default(), true, dummy_key, dummy_hash).unwrap();
         let result = validator.verify(&oversized_key, hash.as_ref());
         assert!(result.is_err());
         assert!(matches!(result.unwrap_err(), Error::InvalidFormat));
@@ -175,7 +195,9 @@ mod tests {
     fn test_oversized_hash_rejection() {
         let oversized_hash = "a".repeat(513); // Exceeds MAX_HASH_LENGTH
 
-        let validator = KeyValidator::new(&HashConfig::default(), true).unwrap();
+        let (dummy_key, dummy_hash) = dummy_key_and_hash();
+        let validator =
+            KeyValidator::new(&HashConfig::default(), true, dummy_key, dummy_hash).unwrap();
         let result = validator.verify("valid_key", &oversized_hash);
         assert!(result.is_err());
         assert!(matches!(result.unwrap_err(), Error::InvalidFormat));
@@ -187,7 +209,9 @@ mod tests {
         let hasher = KeyHasher::new(HashConfig::default());
         let hash = hasher.hash(&valid_key).unwrap();
 
-        let validator = KeyValidator::new(&HashConfig::default(), true).unwrap();
+        let (dummy_key, dummy_hash) = dummy_key_and_hash();
+        let validator =
+            KeyValidator::new(&HashConfig::default(), true, dummy_key, dummy_hash).unwrap();
 
         // Test at boundary (512 chars - should pass)
         let max_key = "a".repeat(512);
@@ -207,7 +231,9 @@ mod tests {
         let hasher = KeyHasher::new(HashConfig::default());
         let valid_hash = hasher.hash(&valid_key).unwrap();
 
-        let validator = KeyValidator::new(&HashConfig::default(), true).unwrap();
+        let (dummy_key, dummy_hash) = dummy_key_and_hash();
+        let validator =
+            KeyValidator::new(&HashConfig::default(), true, dummy_key, dummy_hash).unwrap();
 
         let result1 = validator.verify("wrong_key", valid_hash.as_ref());
         assert!(result1.is_ok());
