diff --git a/Cargo.lock b/Cargo.lock index 15d6ffc12..d1fd9a370 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9460,6 +9460,7 @@ dependencies = [ "serde", "serde_json", "sha2 0.11.0", + "subtle", "temp-env", "tempfile", "thiserror 2.0.19", diff --git a/crates/kms/Cargo.toml b/crates/kms/Cargo.toml index 621f913f4..47b5c5fe7 100644 --- a/crates/kms/Cargo.toml +++ b/crates/kms/Cargo.toml @@ -45,6 +45,7 @@ chacha20poly1305 = { workspace = true } rand = { workspace = true, features = ["serde"] } base64 = { workspace = true } sha2 = { workspace = true } +subtle = { workspace = true } zeroize = { workspace = true, features = ["derive"] } # Configuration and storage diff --git a/crates/kms/src/backends/local.rs b/crates/kms/src/backends/local.rs index 755f23dcd..ada43f00b 100644 --- a/crates/kms/src/backends/local.rs +++ b/crates/kms/src/backends/local.rs @@ -35,7 +35,6 @@ use std::collections::HashMap; use std::path::{Component, Path, PathBuf}; use std::time::Duration; use tokio::fs; -use tokio::sync::RwLock; use tracing::{debug, warn}; /// Reject key identifiers that would not name a single file directly inside the key @@ -77,8 +76,6 @@ const LOCAL_KMS_ARGON2_P_COST: u32 = 1; /// Local KMS client that stores keys in local files pub struct LocalKmsClient { config: LocalConfig, - /// In-memory cache of loaded keys for performance - key_cache: RwLock>, /// Master encryption key for encrypting stored keys master_cipher: Option, /// Legacy pre-beta.9 master cipher for reading pre-Argon2 key files @@ -139,13 +136,14 @@ impl LocalKmsClient { (None, None) }; - Ok(Self { + let client = Self { config, - key_cache: RwLock::new(HashMap::new()), master_cipher, legacy_master_cipher, dek_crypto: AesDekCrypto::new(), - }) + }; + client.validate_existing_keys().await?; + Ok(client) } /// Derive a 256-bit key from the master key string using a persistent Argon2id salt. @@ -193,10 +191,35 @@ impl LocalKmsClient { let mut salt = [0u8; LOCAL_KMS_MASTER_KEY_SALT_LEN]; rand::rng().fill(&mut salt[..]); - fs::write(&salt_path, salt).await?; - Self::set_file_permissions(&salt_path, config.file_permissions).await?; - debug!(path = ?salt_path, "Local KMS master key salt created"); - Ok(salt) + let temp_path = config + .key_dir + .join(format!("{LOCAL_KMS_MASTER_KEY_SALT_FILE}.tmp-{}", uuid::Uuid::new_v4())); + fs::write(&temp_path, salt).await?; + Self::set_file_permissions(&temp_path, config.file_permissions).await?; + match fs::hard_link(&temp_path, &salt_path).await { + Ok(()) => { + if let Err(error) = fs::remove_file(&temp_path).await { + warn!(path = ?temp_path, %error, "Failed to remove Local KMS salt temporary file"); + } + debug!(path = ?salt_path, "Local KMS master key salt created"); + Ok(salt) + } + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => { + let _ = fs::remove_file(&temp_path).await; + let bytes = fs::read(&salt_path).await?; + bytes.try_into().map_err(|_| { + KmsError::configuration_error(format!( + "Local KMS master key salt at {} must be exactly {} bytes", + salt_path.display(), + LOCAL_KMS_MASTER_KEY_SALT_LEN + )) + }) + } + Err(error) => { + let _ = fs::remove_file(&temp_path).await; + Err(error.into()) + } + } } #[cfg(unix)] @@ -242,6 +265,12 @@ impl LocalKmsClient { let content = fs::read(&key_path).await?; let stored_key: StoredMasterKey = serde_json::from_slice(&content)?; + if stored_key.key_id != key_id { + return Err(KmsError::invalid_key(format!( + "Local KMS key file identity mismatch: expected {key_id:?}, found {:?}", + stored_key.key_id + ))); + } let encrypted_bytes = BASE64 .decode(&stored_key.encrypted_key_material) @@ -326,8 +355,43 @@ impl LocalKmsClient { /// Save a master key to disk async fn save_master_key(&self, master_key: &MasterKeyInfo, key_material: &[u8]) -> Result<()> { let key_path = self.master_key_path(&master_key.key_id)?; + let content = self.encode_master_key(master_key, key_material)?; + let temp_path = key_path.with_extension(format!("tmp-{}", uuid::Uuid::new_v4())); + fs::write(&temp_path, &content).await?; + Self::set_file_permissions(&temp_path, self.config.file_permissions).await?; + fs::rename(&temp_path, &key_path).await?; - // Encrypt key material if master cipher is available + debug!(key_id = %master_key.key_id, path = ?key_path, "Local KMS master key saved"); + Ok(()) + } + + async fn save_new_master_key(&self, master_key: &MasterKeyInfo, key_material: &[u8]) -> Result<()> { + let key_path = self.master_key_path(&master_key.key_id)?; + let content = self.encode_master_key(master_key, key_material)?; + let temp_path = key_path.with_extension(format!("tmp-{}", uuid::Uuid::new_v4())); + fs::write(&temp_path, &content).await?; + Self::set_file_permissions(&temp_path, self.config.file_permissions).await?; + + match fs::hard_link(&temp_path, &key_path).await { + Ok(()) => { + if let Err(error) = fs::remove_file(&temp_path).await { + warn!(path = ?temp_path, %error, "Failed to remove Local KMS key temporary file"); + } + debug!(key_id = %master_key.key_id, path = ?key_path, "Local KMS master key created"); + Ok(()) + } + Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => { + let _ = fs::remove_file(&temp_path).await; + Err(KmsError::key_already_exists(&master_key.key_id)) + } + Err(error) => { + let _ = fs::remove_file(&temp_path).await; + Err(error.into()) + } + } + } + + fn encode_master_key(&self, master_key: &MasterKeyInfo, key_material: &[u8]) -> Result> { let (encrypted_key_material, nonce, at_rest_protection) = if let Some(ref cipher) = self.master_cipher { let mut nonce_bytes = [0u8; 12]; rand::rng().fill(&mut nonce_bytes[..]); @@ -362,17 +426,7 @@ impl LocalKmsClient { at_rest_protection, }; - let content = serde_json::to_vec_pretty(&stored_key)?; - - // Write to temporary file first, then rename for atomicity - let temp_path = key_path.with_extension("tmp"); - fs::write(&temp_path, &content).await?; - Self::set_file_permissions(&temp_path, self.config.file_permissions).await?; - - fs::rename(&temp_path, &key_path).await?; - - debug!(key_id = %master_key.key_id, path = ?key_path, "Local KMS master key saved"); - Ok(()) + serde_json::to_vec_pretty(&stored_key).map_err(Into::into) } /// Get the actual key material for a master key @@ -381,6 +435,23 @@ impl LocalKmsClient { Ok(key_material) } + async fn validate_existing_keys(&self) -> Result<()> { + let mut entries = fs::read_dir(&self.config.key_dir).await?; + while let Some(entry) = entries.next_entry().await? { + let path = entry.path(); + if path.extension().is_none_or(|extension| extension != "key") { + continue; + } + + let key_id = path + .file_stem() + .and_then(|stem| stem.to_str()) + .ok_or_else(|| KmsError::configuration_error("Local KMS key file name must be valid UTF-8"))?; + self.decode_stored_key(key_id).await?; + } + Ok(()) + } + /// Encrypt data using a master key async fn encrypt_with_master_key(&self, key_id: &str, plaintext: &[u8]) -> Result<(Vec, Vec)> { // Load the actual master key material @@ -513,11 +584,7 @@ impl KmsClient for LocalKmsClient { let master_key = MasterKeyInfo::new_with_description(key_id.to_string(), algorithm.to_string(), Some(created_by), None); // Save to disk - self.save_master_key(&master_key, &key_material).await?; - - // Cache the key - let mut cache = self.key_cache.write().await; - cache.insert(key_id.to_string(), master_key.clone()); + self.save_new_master_key(&master_key, &key_material).await?; debug!(key_id, "Local KMS master key created"); Ok(master_key) @@ -526,23 +593,7 @@ impl KmsClient for LocalKmsClient { async fn describe_key(&self, key_id: &str, _context: Option<&OperationContext>) -> Result { debug!("Describing key: {}", key_id); - // Check cache first - { - let cache = self.key_cache.read().await; - if let Some(master_key) = cache.get(key_id) { - return Ok(master_key.clone().into()); - } - } - - // Load from disk let master_key = self.load_master_key(key_id).await?; - - // Update cache - { - let mut cache = self.key_cache.write().await; - cache.insert(key_id.to_string(), master_key.clone()); - } - Ok(master_key.into()) } @@ -602,10 +653,6 @@ impl KmsClient for LocalKmsClient { let key_material = self.get_key_material(key_id).await?; self.save_master_key(&master_key, &key_material).await?; - // Update cache - let mut cache = self.key_cache.write().await; - cache.insert(key_id.to_string(), master_key); - debug!(key_id, "Local KMS key enabled"); Ok(()) } @@ -621,10 +668,6 @@ impl KmsClient for LocalKmsClient { let key_material = self.get_key_material(key_id).await?; self.save_master_key(&master_key, &key_material).await?; - // Update cache - let mut cache = self.key_cache.write().await; - cache.insert(key_id.to_string(), master_key); - debug!(key_id, "Local KMS key disabled"); Ok(()) } @@ -646,10 +689,6 @@ impl KmsClient for LocalKmsClient { let key_material = self.get_key_material(key_id).await?; self.save_master_key(&master_key, &key_material).await?; - // Update cache - let mut cache = self.key_cache.write().await; - cache.insert(key_id.to_string(), master_key); - debug!(key_id, "Local KMS key deletion scheduled"); Ok(()) } @@ -665,31 +704,17 @@ impl KmsClient for LocalKmsClient { let key_material = self.get_key_material(key_id).await?; self.save_master_key(&master_key, &key_material).await?; - // Update cache - let mut cache = self.key_cache.write().await; - cache.insert(key_id.to_string(), master_key); - debug!(key_id, "Local KMS key deletion canceled"); Ok(()) } async fn rotate_key(&self, key_id: &str, _context: Option<&OperationContext>) -> Result { - debug!("Rotating key: {}", key_id); - - let mut master_key = self.load_master_key(key_id).await?; - master_key.version += 1; - master_key.rotated_at = Some(Zoned::now()); - - // Generate new key material - let key_material = generate_key_material(&master_key.algorithm)?; - self.save_master_key(&master_key, &key_material).await?; - - // Update cache - let mut cache = self.key_cache.write().await; - cache.insert(key_id.to_string(), master_key.clone()); - - debug!(key_id, "Local KMS key rotated"); - Ok(master_key) + if !fs::try_exists(self.master_key_path(key_id)?).await? { + return Err(KmsError::key_not_found(key_id)); + } + Err(KmsError::invalid_operation( + "Local KMS key rotation is unavailable until historical key versions can be retained", + )) } async fn health_check(&self) -> Result<()> { @@ -745,11 +770,6 @@ impl KmsBackend for LocalKmsBackend { async fn create_key(&self, request: CreateKeyRequest) -> Result { let key_id = request.key_name.unwrap_or_else(|| uuid::Uuid::new_v4().to_string()); - // `save_master_key` writes through a temp file and renames over the destination, so - // creating a key under an existing name would replace its material and silently - // destroy the ability to decrypt everything wrapped under it. The sibling - // `KmsClient::create_key` has always refused this; the backend path did not, and - // this is the path the admin API uses. if self.client.master_key_path(&key_id)?.exists() { return Err(KmsError::key_already_exists(&key_id)); } @@ -767,11 +787,8 @@ impl KmsBackend for LocalKmsBackend { request.description.clone(), ); - // Save to disk and cache - self.client.save_master_key(&master_key, &key_material).await?; - - let mut cache = self.client.key_cache.write().await; - cache.insert(key_id.clone(), master_key.clone()); + // Save to disk + self.client.save_new_master_key(&master_key, &key_material).await?; master_key }; @@ -888,10 +905,6 @@ impl KmsBackend for LocalKmsBackend { .await .map_err(|e| KmsError::internal_error(format!("Failed to delete key file: {e}")))?; - // Remove from cache - let mut cache = self.client.key_cache.write().await; - cache.remove(key_id); - debug!(key_id, "Local KMS key deleted immediately"); // Return success response for immediate deletion @@ -935,10 +948,6 @@ impl KmsBackend for LocalKmsBackend { self.client.save_master_key(&master_key, &existing_key_material).await?; - // Update cache - let mut cache = self.client.key_cache.write().await; - cache.insert(key_id.to_string(), master_key.clone()); - // Convert master_key to KeyMetadata for response let key_metadata = KeyMetadata { key_id: master_key.key_id.clone(), @@ -986,10 +995,6 @@ impl KmsBackend for LocalKmsBackend { self.client.save_master_key(&master_key, &existing_key_material).await?; - // Update cache - let mut cache = self.client.key_cache.write().await; - cache.insert(key_id.to_string(), master_key.clone()); - // Convert master_key to KeyMetadata for response let key_metadata = KeyMetadata { key_id: master_key.key_id.clone(), @@ -1189,6 +1194,18 @@ mod tests { .expect("stored encrypted key should deserialize"); assert_eq!(stored.at_rest_protection, StoredKeyProtection::EncryptedMasterKey); assert_eq!(stored.nonce.len(), 12); + + let wrong_master_error = match LocalKmsClient::new(LocalConfig { + key_dir: client.config.key_dir.clone(), + master_key: Some("wrong-master-key".to_string()), + file_permissions: Some(0o600), + }) + .await + { + Ok(_) => panic!("wrong master key must fail initialization"), + Err(error) => error, + }; + assert!(matches!(wrong_master_error, KmsError::CryptographicError { .. })); } #[tokio::test] @@ -1228,17 +1245,57 @@ mod tests { master_key: None, file_permissions: Some(0o600), }; - let client_without_master = LocalKmsClient::new(config) - .await - .expect("client without master key should still initialize in dev-mode tests"); - - let err = client_without_master - .describe_key("encrypted-key", None) - .await - .expect_err("encrypted key should require a master key to read"); + let err = match LocalKmsClient::new(config).await { + Ok(_) => panic!("initialization must reject an unreadable encrypted key"), + Err(error) => error, + }; assert!(err.to_string().contains("requires a configured master key")); } + #[tokio::test] + async fn local_key_rotation_is_rejected_without_overwriting_key_material() { + let (client, _temp_dir) = create_test_client().await; + let key_id = "rotation-key"; + client.create_key(key_id, "AES_256", None).await.expect("create key"); + let original_material = client.get_key_material(key_id).await.expect("load original material"); + + let error = client + .rotate_key(key_id, None) + .await + .expect_err("rotation must remain unavailable without historical key versions"); + + assert!(matches!(error, KmsError::InvalidOperation { .. })); + assert_eq!( + client.get_key_material(key_id).await.expect("reload original material"), + original_material + ); + } + + #[tokio::test] + async fn startup_rejects_key_file_with_mismatched_embedded_id() { + let (client, temp_dir) = create_dev_mode_client().await; + client.create_key("file-name", "AES_256", None).await.expect("create key"); + let key_path = client.master_key_path("file-name").expect("valid key id"); + let mut stored: serde_json::Value = + serde_json::from_slice(&fs::read(&key_path).await.expect("read key file")).expect("decode key file"); + stored["key_id"] = serde_json::json!("embedded-name"); + fs::write(&key_path, serde_json::to_vec_pretty(&stored).expect("encode mismatched key")) + .await + .expect("write mismatched key"); + + let error = match LocalKmsClient::new(LocalConfig { + key_dir: temp_dir.path().to_path_buf(), + master_key: None, + file_permissions: Some(0o600), + }) + .await + { + Ok(_) => panic!("mismatched key identity must fail initialization"), + Err(error) => error, + }; + assert!(matches!(error, KmsError::InvalidKey { .. })); + } + #[tokio::test] async fn test_load_master_key_accepts_legacy_rfc3339_timestamp() { let (client, _temp_dir) = create_dev_mode_client().await; @@ -1329,17 +1386,6 @@ mod tests { ) .await .expect("write beta.5 fixture"); - let mut explicit_protection = stored_key.clone(); - let explicit_object = explicit_protection.as_object_mut().expect("beta.5 fixture is a JSON object"); - explicit_object.insert("key_id".to_string(), serde_json::json!("beta5-explicit-key")); - explicit_object.insert("at_rest_protection".to_string(), serde_json::json!("encrypted-master-key")); - fs::write( - temp_dir.path().join("beta5-explicit-key.key"), - serde_json::to_vec_pretty(&explicit_protection).expect("serialize explicit-protection fixture"), - ) - .await - .expect("write explicit-protection fixture"); - let client = LocalKmsClient::new(LocalConfig { key_dir: temp_dir.path().to_path_buf(), master_key: Some("beta5-test-master-key".to_string()), @@ -1353,24 +1399,34 @@ mod tests { .await .expect("decrypt beta.5 SHA-256 protected key"); assert_eq!(material, vec![0x42; 32]); + + let mut explicit_protection = stored_key.clone(); + let explicit_object = explicit_protection.as_object_mut().expect("beta.5 fixture is a JSON object"); + explicit_object.insert("key_id".to_string(), serde_json::json!("beta5-explicit-key")); + explicit_object.insert("at_rest_protection".to_string(), serde_json::json!("encrypted-master-key")); + fs::write( + temp_dir.path().join("beta5-explicit-key.key"), + serde_json::to_vec_pretty(&explicit_protection).expect("serialize explicit-protection fixture"), + ) + .await + .expect("write explicit-protection fixture"); let explicit_error = client .get_key_material("beta5-explicit-key") .await .expect_err("explicit current protection must not fall back to the beta.5 KDF"); assert!(matches!(explicit_error, KmsError::CryptographicError { .. })); - let wrong_key_client = LocalKmsClient::new(LocalConfig { + let wrong_key_error = match LocalKmsClient::new(LocalConfig { key_dir: temp_dir.path().to_path_buf(), master_key: Some("wrong-beta5-master-key".to_string()), file_permissions: Some(0o600), }) .await - .expect("initialize local KMS with wrong beta.5 master key"); - let error = wrong_key_client - .get_key_material("beta5-key") - .await - .expect_err("wrong beta.5 master key must not decrypt the fixture"); - assert!(matches!(error, KmsError::CryptographicError { .. })); + { + Ok(_) => panic!("wrong beta.5 master key must fail initialization"), + Err(error) => error, + }; + assert!(matches!(wrong_key_error, KmsError::CryptographicError { .. })); } /// R03-CAN-072 / R03-CAN-073: key identifiers arrive from request input, so every path @@ -1429,9 +1485,7 @@ mod tests { } } - /// R07-CAN-103: `save_master_key` writes a temp file and renames over the destination, - /// so creating a key under an existing name would replace its material and silently - /// destroy the ability to decrypt anything wrapped under it. + /// R07-CAN-103: creating a duplicate key must preserve its original material. #[tokio::test] async fn backend_create_key_refuses_to_replace_existing_key_material() { let temp_dir = TempDir::new().expect("Failed to create temp dir"); @@ -1469,4 +1523,33 @@ mod tests { .expect("original key material must survive the refused create"); assert_eq!(original, after, "existing key material must not be replaced"); } + + #[tokio::test] + async fn concurrent_backend_create_allows_only_one_writer() { + let temp_dir = TempDir::new().expect("create key directory"); + let config = || LocalConfig { + key_dir: temp_dir.path().to_path_buf(), + master_key: Some("test-master-key".to_string()), + file_permissions: Some(0o600), + }; + let first = LocalKmsBackend { + client: LocalKmsClient::new(config()).await.expect("create first client"), + }; + let second = LocalKmsBackend { + client: LocalKmsClient::new(config()).await.expect("create second client"), + }; + let request = || CreateKeyRequest { + key_name: Some("concurrent-key".to_string()), + ..Default::default() + }; + + let (first_result, second_result) = tokio::join!(first.create_key(request()), second.create_key(request())); + assert_ne!(first_result.is_ok(), second_result.is_ok(), "exactly one create must succeed"); + let error = first_result + .err() + .or_else(|| second_result.err()) + .expect("one create must fail"); + assert!(matches!(error, KmsError::KeyAlreadyExists { .. })); + assert_eq!(first.client.get_key_material("concurrent-key").await.expect("load key").len(), 32); + } } diff --git a/crates/kms/src/lib.rs b/crates/kms/src/lib.rs index de97e5349..7806bbc7d 100644 --- a/crates/kms/src/lib.rs +++ b/crates/kms/src/lib.rs @@ -177,8 +177,8 @@ mod tests { let service1 = manager.get_encryption_service().await.expect("Service should be available"); // Reconfigure to new service (zero-downtime) - let temp_dir2 = TempDir::new().expect("Failed to create temp dir"); - let config2 = KmsConfig::local(temp_dir2.path().to_path_buf()).with_insecure_development_defaults(); + let mut config2 = config1; + config2.timeout = std::time::Duration::from_secs(45); manager.reconfigure(config2).await.expect("Reconfiguration should succeed"); // Verify version 2 diff --git a/crates/kms/src/service_manager.rs b/crates/kms/src/service_manager.rs index b3a37ca22..07b8628cf 100644 --- a/crates/kms/src/service_manager.rs +++ b/crates/kms/src/service_manager.rs @@ -20,10 +20,12 @@ use crate::error::{KmsError, Result}; use crate::manager::KmsManager; use crate::service::ObjectEncryptionService; use arc_swap::ArcSwap; +use sha2::{Digest, Sha256}; use std::sync::{ Arc, OnceLock, atomic::{AtomicU64, Ordering}, }; +use subtle::ConstantTimeEq; use tokio::sync::{Mutex, RwLock}; use tracing::{debug, error, info, warn}; @@ -31,6 +33,53 @@ const LOG_COMPONENT_KMS: &str = "kms"; const LOG_SUBSYSTEM_SERVICE: &str = "service"; const EVENT_KMS_SERVICE_STATE: &str = "kms_service_state"; +fn local_master_key_fingerprint(master_key: Option<&str>) -> [u8; 32] { + let mut digest = Sha256::new(); + digest.update([u8::from(master_key.is_some())]); + if let Some(master_key) = master_key { + digest.update(master_key.as_bytes()); + } + digest.finalize().into() +} + +fn validate_local_transition(current: Option<&KmsConfig>, new: &KmsConfig) -> Result<()> { + let Some(current) = current else { + return Ok(()); + }; + let BackendConfig::Local(current_local) = ¤t.backend_config else { + return Ok(()); + }; + let BackendConfig::Local(new_local) = &new.backend_config else { + return Err(KmsError::configuration_error("Local KMS backend cannot be changed after configuration")); + }; + + if current_local.key_dir != new_local.key_dir { + return Err(KmsError::configuration_error( + "Local KMS key directory cannot be changed after configuration", + )); + } + if current_local.file_permissions != new_local.file_permissions { + return Err(KmsError::configuration_error( + "Local KMS file permissions cannot be changed after configuration", + )); + } + if current.allow_insecure_dev_defaults != new.allow_insecure_dev_defaults { + return Err(KmsError::configuration_error( + "Local KMS development mode cannot be changed after configuration", + )); + } + + let current_master_key = local_master_key_fingerprint(current_local.master_key.as_deref()); + let new_master_key = local_master_key_fingerprint(new_local.master_key.as_deref()); + if !bool::from(current_master_key.ct_eq(&new_master_key)) { + return Err(KmsError::configuration_error( + "Local KMS master key cannot be changed after configuration", + )); + } + + Ok(()) +} + /// KMS service status #[derive(Debug, Clone, PartialEq, serde::Serialize, serde::Deserialize)] pub enum KmsServiceStatus { @@ -106,7 +155,12 @@ impl KmsServiceManager { /// Configure KMS with new configuration pub async fn configure(&self, new_config: KmsConfig) -> Result<()> { + let _guard = self.lifecycle_mutex.lock().await; new_config.validate()?; + { + let config = self.config.read().await; + validate_local_transition(config.as_ref(), &new_config)?; + } // Update configuration { @@ -254,11 +308,9 @@ impl KmsServiceManager { "KMS service reconfiguring" ); new_config.validate()?; - - // Configure with new config { - let mut config = self.config.write().await; - *config = Some(new_config.clone()); + let config = self.config.read().await; + validate_local_transition(config.as_ref(), &new_config)?; } // Create new service version without stopping old one @@ -268,6 +320,11 @@ impl KmsServiceManager { // Get old version for logging (lock-free read) let old_version = self.current_service.load().as_ref().as_ref().map(|sv| sv.version); + { + let mut config = self.config.write().await; + *config = Some(new_config); + } + // Atomically switch to new service version (lock-free, instant CAS operation) // This is a true atomic operation - no waiting for locks, instant switch // Old service will be dropped when no more Arc references exist @@ -304,8 +361,6 @@ impl KmsServiceManager { Err(e) => { let err_msg = format!("Failed to reconfigure KMS: {e}"); error!("{}", err_msg); - let mut status = self.status.write().await; - *status = KmsServiceStatus::Error(err_msg.clone()); Err(KmsError::backend_error(&err_msg)) } } @@ -477,4 +532,105 @@ mod tests { }; assert!(static_config.secret_key.is_empty()); } + + #[tokio::test] + async fn forbidden_local_master_key_change_preserves_running_config_and_service() { + use crate::types::{CreateKeyRequest, KeyUsage}; + use std::collections::HashMap; + use tempfile::TempDir; + + let key_dir = TempDir::new().expect("create local KMS directory"); + let config = |master_key: &str| { + let mut config = KmsConfig::local(key_dir.path().to_path_buf()); + let BackendConfig::Local(local) = &mut config.backend_config else { + panic!("local constructor must create local backend config"); + }; + local.master_key = Some(master_key.to_string()); + config.allow_insecure_dev_defaults = true; + config + }; + let manager = KmsServiceManager::new(); + manager + .configure(config("working-master-key")) + .await + .expect("configure local KMS"); + manager.start().await.expect("start local KMS"); + manager + .get_manager() + .await + .expect("running KMS manager") + .create_key(CreateKeyRequest { + key_name: Some("existing-key".to_string()), + key_usage: KeyUsage::EncryptDecrypt, + description: None, + policy: None, + tags: HashMap::new(), + origin: None, + }) + .await + .expect("create encrypted key"); + let service_version = manager.get_service_version().await; + + let error = manager + .reconfigure(config("wrong-master-key")) + .await + .expect_err("local master key change must be rejected"); + + assert!(error.to_string().contains("master key cannot be changed")); + assert_eq!(manager.get_status().await, KmsServiceStatus::Running); + assert_eq!(manager.get_service_version().await, service_version); + let current = manager.get_config().await.expect("working config must remain"); + let BackendConfig::Local(local) = current.backend_config else { + panic!("working config must remain local"); + }; + assert_eq!(local.master_key.as_deref(), Some("working-master-key")); + } + + #[tokio::test] + async fn configure_cannot_replace_existing_local_backend() { + use base64::Engine as _; + use tempfile::TempDir; + + let key_dir = TempDir::new().expect("create local KMS directory"); + let mut local = KmsConfig::local(key_dir.path().to_path_buf()); + local.allow_insecure_dev_defaults = true; + let manager = KmsServiceManager::new(); + manager.configure(local.clone()).await.expect("configure local KMS"); + + let encoded_key = base64::engine::general_purpose::STANDARD.encode([0x5au8; 32]); + let error = manager + .configure(KmsConfig::static_kms("static-key".to_string(), encoded_key)) + .await + .expect_err("existing local backend must be immutable"); + + assert!(error.to_string().contains("backend cannot be changed")); + let current = manager.get_config().await.expect("local config must remain"); + assert!(matches!(current.backend_config, BackendConfig::Local(_))); + } + + #[tokio::test] + async fn reconfigure_allows_safe_local_runtime_settings_only() { + use tempfile::TempDir; + + let key_dir = TempDir::new().expect("create local KMS directory"); + let mut initial = KmsConfig::local(key_dir.path().to_path_buf()); + initial.allow_insecure_dev_defaults = true; + let manager = KmsServiceManager::new(); + manager.configure(initial.clone()).await.expect("configure local KMS"); + manager.start().await.expect("start local KMS"); + + let mut updated = initial; + updated.default_key_id = Some("evaluation-key".to_string()); + updated.timeout = std::time::Duration::from_secs(45); + updated.enable_cache = false; + manager + .reconfigure(updated.clone()) + .await + .expect("update safe local settings"); + + let current = manager.get_config().await.expect("updated config"); + assert_eq!(current.default_key_id, updated.default_key_id); + assert_eq!(current.timeout, updated.timeout); + assert!(!current.enable_cache); + } } diff --git a/rustfs/src/admin/handlers/kms_dynamic.rs b/rustfs/src/admin/handlers/kms_dynamic.rs index e4753542d..e8fb0ee64 100644 --- a/rustfs/src/admin/handlers/kms_dynamic.rs +++ b/rustfs/src/admin/handlers/kms_dynamic.rs @@ -79,10 +79,40 @@ fn kms_service_control_actions() -> Vec { vec![Action::KmsAction(KmsAction::ServiceControlAction)] } -fn normalize_configure_request_auth( +fn normalize_configure_request_secrets( request: &mut ConfigureKmsRequest, existing_config: Option<&KmsConfig>, ) -> Result<(), String> { + if existing_config.is_some_and(|config| matches!(&config.backend_config, rustfs_kms::BackendConfig::Local(_))) + && !matches!(request, ConfigureKmsRequest::Local(_)) + { + return Err("Changing from the Local KMS backend is not supported".to_string()); + } + + if let ConfigureKmsRequest::Local(request) = request + && let Some(KmsConfig { + backend_config: rustfs_kms::BackendConfig::Local(existing), + allow_insecure_dev_defaults, + .. + }) = existing_config + { + if request.key_dir != existing.key_dir { + return Err("Changing the Local KMS key directory is not supported".to_string()); + } + match request.file_permissions { + Some(permissions) if Some(permissions) != existing.file_permissions => { + return Err("Changing Local KMS file permissions is not supported".to_string()); + } + None => request.file_permissions = existing.file_permissions, + Some(_) => {} + } + if request.master_key.as_deref().is_some_and(|master_key| !master_key.is_empty()) { + return Err("Changing the Local KMS master key is not supported".to_string()); + } + request.master_key.clone_from(&existing.master_key); + request.allow_insecure_dev_defaults = Some(*allow_insecure_dev_defaults); + } + let needs_existing_auth = match request { ConfigureKmsRequest::VaultKv2(req) => token_is_blank(&req.auth_method), ConfigureKmsRequest::VaultTransit(req) => token_is_blank(&req.auth_method), @@ -350,9 +380,9 @@ impl Operation for ConfigureKmsHandler { ); let service_manager = kms_service_manager_from_context(); - let existing_config = service_manager.get_redacted_config().await; + let existing_config = service_manager.get_config().await; - if let Err(e) = normalize_configure_request_auth(&mut configure_request, existing_config.as_ref()) { + if let Err(e) = normalize_configure_request_secrets(&mut configure_request, existing_config.as_ref()) { return Ok(S3Response::new((StatusCode::BAD_REQUEST, Body::from(e)))); } @@ -895,9 +925,9 @@ impl Operation for ReconfigureKmsHandler { ); let service_manager = kms_service_manager_from_context(); - let existing_config = service_manager.get_redacted_config().await; + let existing_config = service_manager.get_config().await; - if let Err(e) = normalize_configure_request_auth(&mut configure_request, existing_config.as_ref()) { + if let Err(e) = normalize_configure_request_secrets(&mut configure_request, existing_config.as_ref()) { return Ok(S3Response::new((StatusCode::BAD_REQUEST, Body::from(e)))); } @@ -986,8 +1016,12 @@ impl Operation for ReconfigureKmsHandler { #[cfg(test)] mod tests { - use super::{decode_persisted_kms_config, ensure_kms_config_persistable, kms_configure_actions, kms_service_control_actions}; + use super::{ + decode_persisted_kms_config, ensure_kms_config_persistable, kms_configure_actions, kms_service_control_actions, + normalize_configure_request_secrets, + }; use rustfs_policy::policy::action::{Action, AdminAction, KmsAction}; + use std::path::PathBuf; use tempfile::TempDir; fn assert_has_action(actions: &[Action], action: Action) { @@ -1084,4 +1118,140 @@ mod tests { assert!(ensure_kms_config_persistable(&config).is_err()); } + + #[test] + fn local_reconfigure_preserves_hidden_master_key_for_same_directory() { + let key_dir = PathBuf::from("/var/lib/rustfs/kms"); + let mut existing = rustfs_kms::KmsConfig::local(key_dir.clone()); + let rustfs_kms::BackendConfig::Local(existing_local) = &mut existing.backend_config else { + panic!("local constructor must create local backend config"); + }; + existing_local.master_key = Some("stored-master-key".to_string()); + + let mut request = rustfs_kms::ConfigureKmsRequest::Local(rustfs_kms::ConfigureLocalKmsRequest { + key_dir, + master_key: None, + file_permissions: Some(0o600), + default_key_id: Some("experience-key".to_string()), + timeout_seconds: Some(30), + retry_attempts: Some(3), + enable_cache: Some(true), + max_cached_keys: Some(1000), + cache_ttl_seconds: Some(3600), + allow_insecure_dev_defaults: Some(false), + }); + + normalize_configure_request_secrets(&mut request, Some(&existing)).expect("normalize local request"); + let rustfs_kms::ConfigureKmsRequest::Local(request) = request else { + panic!("request must remain local"); + }; + assert_eq!(request.master_key.as_deref(), Some("stored-master-key")); + } + + #[test] + fn local_reconfigure_preserves_unspecified_legacy_file_permissions() { + let key_dir = PathBuf::from("/var/lib/rustfs/kms"); + let mut existing = rustfs_kms::KmsConfig::local(key_dir.clone()); + let rustfs_kms::BackendConfig::Local(existing_local) = &mut existing.backend_config else { + panic!("local constructor must create local backend config"); + }; + existing_local.master_key = Some("stored-master-key".to_string()); + existing_local.file_permissions = None; + + let mut request = rustfs_kms::ConfigureKmsRequest::Local(rustfs_kms::ConfigureLocalKmsRequest { + key_dir, + master_key: None, + file_permissions: None, + default_key_id: Some("experience-key".to_string()), + timeout_seconds: None, + retry_attempts: None, + enable_cache: None, + max_cached_keys: None, + cache_ttl_seconds: None, + allow_insecure_dev_defaults: Some(false), + }); + + normalize_configure_request_secrets(&mut request, Some(&existing)).expect("normalize legacy local request"); + let rustfs_kms::ConfigureKmsRequest::Local(request) = request else { + panic!("request must remain local"); + }; + assert!(request.file_permissions.is_none()); + } + + #[test] + fn local_reconfigure_does_not_reuse_master_key_for_different_directory() { + let mut existing = rustfs_kms::KmsConfig::local(PathBuf::from("/var/lib/rustfs/kms")); + let rustfs_kms::BackendConfig::Local(existing_local) = &mut existing.backend_config else { + panic!("local constructor must create local backend config"); + }; + existing_local.master_key = Some("stored-master-key".to_string()); + + let mut request = rustfs_kms::ConfigureKmsRequest::Local(rustfs_kms::ConfigureLocalKmsRequest { + key_dir: PathBuf::from("/var/lib/rustfs/other-kms"), + master_key: None, + file_permissions: Some(0o600), + default_key_id: None, + timeout_seconds: None, + retry_attempts: None, + enable_cache: None, + max_cached_keys: None, + cache_ttl_seconds: None, + allow_insecure_dev_defaults: Some(false), + }); + + let error = normalize_configure_request_secrets(&mut request, Some(&existing)) + .expect_err("changing the local key directory must be rejected"); + assert_eq!(error, "Changing the Local KMS key directory is not supported"); + } + + #[test] + fn local_reconfigure_rejects_file_permission_and_master_key_changes() { + let key_dir = PathBuf::from("/var/lib/rustfs/kms"); + let mut existing = rustfs_kms::KmsConfig::local(key_dir.clone()); + let rustfs_kms::BackendConfig::Local(existing_local) = &mut existing.backend_config else { + panic!("local constructor must create local backend config"); + }; + existing_local.master_key = Some("stored-master-key".to_string()); + existing_local.file_permissions = Some(0o600); + + let request = |master_key, file_permissions| { + rustfs_kms::ConfigureKmsRequest::Local(rustfs_kms::ConfigureLocalKmsRequest { + key_dir: key_dir.clone(), + master_key, + file_permissions, + default_key_id: None, + timeout_seconds: None, + retry_attempts: None, + enable_cache: None, + max_cached_keys: None, + cache_ttl_seconds: None, + allow_insecure_dev_defaults: Some(false), + }) + }; + + let mut permissions_change = request(None, Some(0o666)); + let permissions_error = normalize_configure_request_secrets(&mut permissions_change, Some(&existing)) + .expect_err("changing file permissions must be rejected"); + assert_eq!(permissions_error, "Changing Local KMS file permissions is not supported"); + + let mut master_key_change = request(Some("replacement-master-key".to_string()), Some(0o600)); + let master_key_error = normalize_configure_request_secrets(&mut master_key_change, Some(&existing)) + .expect_err("changing the master key must be rejected"); + assert_eq!(master_key_error, "Changing the Local KMS master key is not supported"); + + let mut backend_change = rustfs_kms::ConfigureKmsRequest::Static(rustfs_kms::ConfigureStaticKmsRequest { + key_id: "static-key".to_string(), + secret_key: "not-used-by-normalization".to_string(), + default_key_id: None, + timeout_seconds: None, + retry_attempts: None, + enable_cache: None, + max_cached_keys: None, + cache_ttl_seconds: None, + allow_insecure_dev_defaults: None, + }); + let backend_error = normalize_configure_request_secrets(&mut backend_change, Some(&existing)) + .expect_err("changing from the local backend must be rejected"); + assert_eq!(backend_error, "Changing from the Local KMS backend is not supported"); + } } diff --git a/rustfs/src/storage/sse.rs b/rustfs/src/storage/sse.rs index 36aa2116b..f98b4e72e 100644 --- a/rustfs/src/storage/sse.rs +++ b/rustfs/src/storage/sse.rs @@ -4446,32 +4446,16 @@ mod tests { #[tokio::test] async fn test_kms_sse_dek_provider_uses_latest_reconfigured_service() { + use base64::Engine as _; use rustfs_kms::config::KmsConfig; - use rustfs_kms::types::{CreateKeyRequest, KeyUsage}; - use tempfile::TempDir; let _guard = lock_sse_test_state().await; let manager = rustfs_kms::init_global_kms_service_manager(); - let first_dir = TempDir::new().expect("first temp dir"); manager - .reconfigure(KmsConfig::local(first_dir.path().to_path_buf()).with_insecure_development_defaults()) + .reconfigure(KmsConfig::static_kms("first-key".to_string(), BASE64_STANDARD.encode([0x11; 32]))) .await .expect("first KMS reconfigure should succeed"); - manager - .get_encryption_service() - .await - .expect("first encryption service should exist") - .create_key(CreateKeyRequest { - key_name: Some("first-key".to_string()), - key_usage: KeyUsage::EncryptDecrypt, - description: None, - policy: None, - tags: HashMap::new(), - origin: None, - }) - .await - .expect("first key should be created"); let provider = KmsSseDekProvider::new_with_service_manager(manager.clone()) .await @@ -4482,25 +4466,10 @@ mod tests { .await .expect("provider should use the initial service"); - let second_dir = TempDir::new().expect("second temp dir"); manager - .reconfigure(KmsConfig::local(second_dir.path().to_path_buf()).with_insecure_development_defaults()) + .reconfigure(KmsConfig::static_kms("second-key".to_string(), BASE64_STANDARD.encode([0x22; 32]))) .await .expect("second KMS reconfigure should succeed"); - manager - .get_encryption_service() - .await - .expect("second encryption service should exist") - .create_key(CreateKeyRequest { - key_name: Some("second-key".to_string()), - key_usage: KeyUsage::EncryptDecrypt, - description: None, - policy: None, - tags: HashMap::new(), - origin: None, - }) - .await - .expect("second key should be created"); provider .generate_sse_dek(&context, "second-key")