From d4f2efa2ad12c259131292c758ffed03f7995709 Mon Sep 17 00:00:00 2001 From: Zhengchao An Date: Thu, 30 Jul 2026 22:56:43 +0800 Subject: [PATCH] fix(kms): report accurate Vault KV2 security contract and disable unsafe rotation (#5474) --- crates/kms/src/api_types.rs | 61 +++++++++++- crates/kms/src/backends/vault.rs | 98 +++++++++++++------- crates/kms/src/config.rs | 117 ++++++++++++++++++++++-- crates/kms/src/lib.rs | 2 +- docs/operations/kms-backend-security.md | 78 ++++++++++++++++ rustfs/src/config/cli.rs | 2 +- 6 files changed, 308 insertions(+), 50 deletions(-) create mode 100644 docs/operations/kms-backend-security.md diff --git a/crates/kms/src/api_types.rs b/crates/kms/src/api_types.rs index 3af643c48..221529f5c 100644 --- a/crates/kms/src/api_types.rs +++ b/crates/kms/src/api_types.rs @@ -71,7 +71,10 @@ impl fmt::Debug for ConfigureLocalKmsRequest { } } -/// Request to configure KMS with Vault KV v2 + Transit backend +/// Request to configure KMS with the Vault KV v2 storage backend. +/// +/// This backend stores master key material directly in KV v2; confidentiality relies on +/// Vault ACLs and KV v2 at-rest encryption, with no Transit wrapping involved. #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct ConfigureVaultKmsRequest { @@ -82,7 +85,8 @@ pub struct ConfigureVaultKmsRequest { pub auth_method: VaultAuthMethod, /// Vault namespace (Vault Enterprise, optional) pub namespace: Option, - /// Transit engine mount path + /// Deprecated: legacy Transit engine mount path. Still accepted so older clients keep + /// working, but the Vault KV2 backend never uses it. pub mount_path: Option, /// KV engine mount path for storing keys pub kv_mount: Option, @@ -192,7 +196,7 @@ pub enum ConfigureKmsRequest { /// Configure with Local backend #[serde(alias = "local", alias = "Local")] Local(ConfigureLocalKmsRequest), - /// Configure with Vault KV v2 + Transit backend + /// Configure with the Vault KV v2 storage backend #[serde( rename = "VaultKV2", alias = "Vault", @@ -333,7 +337,7 @@ pub enum BackendSummary { /// File permissions (octal) file_permissions: Option, }, - /// Vault KV v2 + Transit backend summary + /// Vault KV v2 storage backend summary #[serde(alias = "vault")] VaultKv2 { /// Vault server address @@ -344,7 +348,8 @@ pub enum BackendSummary { has_stored_credentials: bool, /// Namespace (if configured) namespace: Option, - /// Transit engine mount path + /// Deprecated: legacy Transit mount path. Unused by the backend; kept only so the + /// serialized response shape stays stable for existing consumers. mount_path: String, /// KV engine mount path kv_mount: String, @@ -621,6 +626,52 @@ mod tests { } } + #[test] + fn test_deserialize_vault_kv2_configure_request_mount_path_optional_but_accepted() { + // deny_unknown_fields regression guard: mount_path is deprecated but must remain + // accepted so older clients that still send it do not get a 400. + let with_mount_path = serde_json::json!({ + "backend_type": "VaultKV2", + "address": "http://127.0.0.1:8200", + "auth_method": { "Token": { "token": "dev-root-token" } }, + "mount_path": "transit" + }); + let request: ConfigureKmsRequest = + serde_json::from_value(with_mount_path).expect("request with deprecated mount_path should deserialize"); + let config = request.to_kms_config(); + assert_eq!(config.vault_config().expect("vault-kv2 config").mount_path, "transit"); + + let without_mount_path = serde_json::json!({ + "backend_type": "VaultKV2", + "address": "http://127.0.0.1:8200", + "auth_method": { "Token": { "token": "dev-root-token" } } + }); + let request: ConfigureKmsRequest = + serde_json::from_value(without_mount_path).expect("request without mount_path should deserialize"); + let config = request.to_kms_config(); + assert_eq!(config.vault_config().expect("vault-kv2 config").mount_path, "transit"); + } + + #[test] + fn test_vault_kv2_status_summary_does_not_mention_transit() { + let config = KmsConfig::vault( + url::Url::parse("https://vault.example.com:8200").expect("vault URL"), + "summary-token".to_string(), + ); + let response = KmsStatusResponse { + status: KmsServiceStatus::Running, + backend_type: Some(config.backend.clone()), + healthy: Some(true), + config_summary: Some(KmsConfigSummary::from(&config)), + }; + + let json = serde_json::to_string(&response).expect("kms status response should serialize"); + assert!( + !json.contains("Transit"), + "vault-kv2 status output must not describe the backend as Transit: {json}" + ); + } + #[test] fn test_deserialize_vault_transit_configure_request() { let cases = ["VaultTransit", "vault-transit", "vault_transit"]; diff --git a/crates/kms/src/backends/vault.rs b/crates/kms/src/backends/vault.rs index bc89a54cb..1a72bff12 100644 --- a/crates/kms/src/backends/vault.rs +++ b/crates/kms/src/backends/vault.rs @@ -150,17 +150,18 @@ impl VaultKmsClient { format!("{}/{}", self.key_path_prefix, key_id) } - /// Encrypt key material using Vault's transit engine + /// Encode key material for KV2 storage. + /// + /// This is plain Base64 encoding, not encryption: the KV2 backend stores master key + /// material as-is and relies on Vault ACLs plus KV2 at-rest encryption for + /// confidentiality. Any identity with KV read access to the key path can recover the + /// plaintext master key. async fn encrypt_key_material(&self, key_material: &[u8]) -> Result { - // For simplicity, we'll base64 encode the key material - // In a production setup, you would use Vault's transit engine for additional encryption Ok(general_purpose::STANDARD.encode(key_material)) } - /// Decrypt key material + /// Decode key material from KV2 storage (plain Base64, see `encrypt_key_material`). async fn decrypt_key_material(&self, encrypted_material: &str) -> Result> { - // For simplicity, we'll base64 decode the key material - // In a production setup, you would use Vault's transit engine for decryption general_purpose::STANDARD .decode(encrypted_material) .map_err(|e| KmsError::cryptographic_error("decrypt", e.to_string())) @@ -529,33 +530,13 @@ impl KmsClient for VaultKmsClient { Ok(()) } - async fn rotate_key(&self, key_id: &str, _context: Option<&OperationContext>) -> Result { - debug!("Rotating key: {}", key_id); - - let mut key_data = self.get_key_data(key_id).await?; - key_data.version += 1; - - // Generate new key material - let key_material = generate_key_material(&key_data.algorithm)?; - key_data.encrypted_key_material = self.encrypt_key_material(&key_material).await?; - - self.store_key_data(key_id, &key_data).await?; - - let master_key = MasterKeyInfo { - key_id: key_id.to_string(), - version: key_data.version, - algorithm: key_data.algorithm, - usage: key_data.usage, - status: key_data.status, - description: None, // Rotate preserves existing description (would need key lookup) - metadata: key_data.metadata, - created_at: key_data.created_at, - rotated_at: Some(Zoned::now()), - created_by: None, - }; - - debug!(key_id, "Vault KMS key rotated"); - Ok(master_key) + async fn rotate_key(&self, _key_id: &str, _context: Option<&OperationContext>) -> Result { + // Rotation previously overwrote the stored master key with fresh material, which + // permanently orphaned every DEK wrapped by the prior version. Reject before any + // storage access so existing material can never be touched. + Err(KmsError::invalid_operation( + "Vault KV2 rotation is unavailable until versioned key material retention lands", + )) } async fn health_check(&self) -> Result<()> { @@ -585,6 +566,9 @@ impl KmsClient for VaultKmsClient { BackendInfo::new("vault-kv2".to_string(), "0.1.0".to_string(), self.config.address.clone(), true) .with_metadata("kv_mount".to_string(), self.kv_mount.clone()) .with_metadata("key_prefix".to_string(), self.key_path_prefix.clone()) + // Master key material is protected only by Vault ACLs and KV2 at-rest + // encryption; there is no additional cryptographic wrapping. + .with_metadata("at_rest_protection".to_string(), "vault-kv2-acl".to_string()) } } @@ -950,6 +934,54 @@ mod tests { } } + #[tokio::test] + async fn test_vault_kv2_rotate_key_rejected_without_touching_storage() { + // No Vault instance needed: rotation must be rejected before any storage access, + // so the call cannot read or overwrite key material. + let client = VaultKmsClient::new(integration_vault_config()).await.expect("client"); + + let err = client + .rotate_key("any-key", None) + .await + .expect_err("Vault KV2 rotation must be rejected"); + assert!(matches!(err, KmsError::InvalidOperation { .. }), "expected InvalidOperation, got {err:?}"); + assert!(err.to_string().contains("rotation is unavailable")); + } + + #[tokio::test] + async fn test_vault_kv2_backend_info_reports_at_rest_protection() { + let client = VaultKmsClient::new(integration_vault_config()).await.expect("client"); + + let info = client.backend_info(); + assert_eq!(info.backend_type, "vault-kv2"); + assert_eq!(info.metadata.get("at_rest_protection").map(String::as_str), Some("vault-kv2-acl")); + // The KV2 backend must not present itself as Transit-backed. + assert!(!format!("{info:?}").contains("Transit")); + } + + #[tokio::test] + #[ignore] // Requires a running Vault instance (dev mode) + async fn test_vault_kv2_rotate_rejected_and_material_untouched() { + let client = VaultKmsClient::new(integration_vault_config()).await.expect("client"); + + let key_id = format!("rotate-{}", uuid::Uuid::new_v4()); + client.create_key(&key_id, "AES_256", None).await.expect("create"); + let before = client.get_key_data(&key_id).await.expect("read"); + + let err = client + .rotate_key(&key_id, None) + .await + .expect_err("Vault KV2 rotation must be rejected"); + assert!(matches!(err, KmsError::InvalidOperation { .. })); + + let after = client.get_key_data(&key_id).await.expect("reread"); + assert_eq!( + after.encrypted_key_material, before.encrypted_key_material, + "rejected rotation must leave stored key material untouched" + ); + assert_eq!(after.version, before.version, "rejected rotation must not bump the key version"); + } + #[tokio::test] #[ignore] // Requires a running Vault instance (dev mode) async fn test_corrupted_key_material_does_not_regenerate() { diff --git a/crates/kms/src/config.rs b/crates/kms/src/config.rs index 04aab91d6..2e2f5e80e 100644 --- a/crates/kms/src/config.rs +++ b/crates/kms/src/config.rs @@ -49,6 +49,10 @@ fn default_vault_transit_metadata_key_prefix() -> String { DEFAULT_VAULT_TRANSIT_METADATA_KEY_PREFIX.to_string() } +fn default_vault_kv2_mount_path() -> String { + "transit".to_string() +} + pub const KMS_CONFIG_REDACTION_RULES: &[RedactionRule] = &[ RedactionRule::new("kms.local.master_key", RedactionLevel::Secret, "local backend key encryption material"), RedactionRule::new("kms.vault.token", RedactionLevel::Secret, "vault authentication token"), @@ -100,7 +104,9 @@ pub(crate) fn redacted_secret_option(value: Option<&str>) -> Option<&'static str /// KMS backend types #[derive(Debug, Default, Clone, Serialize, Deserialize, PartialEq, Eq)] pub enum KmsBackend { - /// Vault KV v2 + Transit backend (key metadata in KV, wrapping via Transit) + /// Vault KV v2 storage backend: master key material is stored directly in KV v2. + /// Confidentiality relies on Vault ACLs, KV v2 at-rest encryption, and TLS; the + /// backend performs no Transit wrapping of key material. #[serde(rename = "VaultKV2", alias = "Vault")] VaultKv2, /// Vault Transit backend using Vault as the cryptographic source of truth @@ -162,7 +168,7 @@ impl Default for KmsConfig { pub enum BackendConfig { /// Local backend configuration Local(LocalConfig), - /// Vault KV v2 + Transit backend configuration + /// Vault KV v2 storage backend configuration #[serde(rename = "VaultKV2", alias = "Vault")] VaultKv2(Box), /// Vault Transit backend configuration @@ -270,7 +276,11 @@ impl StaticConfig { } } -/// Vault KV v2 + Transit backend configuration (metadata in KV, key wrapping via Transit) +/// Vault KV v2 backend configuration. +/// +/// Key material and metadata are stored directly in KV v2; any identity with KV read +/// access to the key path can recover plaintext master key material. Use the Vault +/// Transit backend when cryptographic isolation of key material is required. #[derive(Clone, Serialize, Deserialize)] pub struct VaultConfig { /// Vault server URL @@ -279,7 +289,10 @@ pub struct VaultConfig { pub auth_method: VaultAuthMethod, /// Vault namespace (Vault Enterprise) pub namespace: Option, - /// Transit engine mount path + /// Deprecated: legacy Transit engine mount path. The Vault KV2 backend never calls + /// the Transit engine, so this value is unused; the field is retained (and + /// defaulted) only so previously persisted configurations keep deserializing. + #[serde(default = "default_vault_kv2_mount_path")] pub mount_path: String, /// KV engine mount path for storing keys pub kv_mount: String, @@ -439,7 +452,12 @@ impl KmsConfig { } } - /// Create a new KMS configuration for Vault backend with token authentication (recommended for production) + /// Create a new KMS configuration for the Vault KV v2 backend with token authentication. + /// + /// Master key material is stored directly in Vault KV v2: confidentiality relies on + /// Vault ACLs, KV v2 at-rest encryption, and TLS. KV read access to the key path is + /// equivalent to holding the plaintext master keys. Use [`KmsConfig::vault_transit`] + /// when key material must never be readable through Vault storage APIs. pub fn vault(address: Url, token: String) -> Self { Self { backend: KmsBackend::VaultKv2, @@ -452,7 +470,10 @@ impl KmsConfig { } } - /// Create a new KMS configuration for Vault backend with AppRole authentication (recommended for production) + /// Create a new KMS configuration for the Vault KV v2 backend with AppRole authentication. + /// + /// Shares the security boundary described on [`KmsConfig::vault`]: key material lives + /// in KV v2 and is protected only by Vault ACLs and KV v2 at-rest encryption. pub fn vault_approle(address: Url, role_id: String, secret_id: String) -> Self { Self { backend: KmsBackend::VaultKv2, @@ -620,9 +641,8 @@ impl KmsConfig { validate_vault_development_defaults("Vault KV2", &config.address, &config.auth_method, config.tls.as_ref())?; } - if config.mount_path.is_empty() { - return Err(KmsError::configuration_error("Vault KV2 mount path cannot be empty")); - } + // `mount_path` is deprecated and unused by this backend, so an empty value + // is deliberately not an error. // Validate TLS configuration if using HTTPS if config.address.starts_with("https://") @@ -747,11 +767,21 @@ impl KmsConfig { let token = get_env_str("RUSTFS_KMS_VAULT_TOKEN", "dev-token"); let skip_tls_verify = get_env_bool(ENV_KMS_VAULT_SKIP_TLS_VERIFY, false); + let mount_path = match get_env_opt_str("RUSTFS_KMS_VAULT_MOUNT_PATH") { + Some(path) => { + tracing::warn!( + "RUSTFS_KMS_VAULT_MOUNT_PATH is deprecated for the Vault KV2 backend: it never calls the Transit engine and the value is stored but unused" + ); + path + } + None => default_vault_kv2_mount_path(), + }; + config.backend_config = BackendConfig::VaultKv2(Box::new(VaultConfig { address, auth_method: VaultAuthMethod::Token { token }, namespace: get_env_opt_str("RUSTFS_KMS_VAULT_NAMESPACE"), - mount_path: get_env_str("RUSTFS_KMS_VAULT_MOUNT_PATH", "transit"), + mount_path, kv_mount: get_env_str("RUSTFS_KMS_VAULT_KV_MOUNT", "secret"), key_path_prefix: get_env_str("RUSTFS_KMS_VAULT_KEY_PREFIX", "rustfs/kms/keys"), tls: vault_tls_config(skip_tls_verify), @@ -1044,6 +1074,73 @@ mod tests { assert!(config.vault_config().is_some()); } + #[test] + fn test_persisted_vault_kv2_config_without_mount_path_deserializes() { + // Configurations persisted after mount_path was deprecated may omit the field; + // it must default instead of failing deserialization. + let raw = r#"{ + "backend": "VaultKV2", + "backend_config": { + "VaultKV2": { + "address": "http://127.0.0.1:8200", + "auth_method": { "Token": { "token": "t" } }, + "namespace": null, + "kv_mount": "secret", + "key_path_prefix": "rustfs/kms/keys", + "tls": null + } + }, + "default_key_id": null, + "timeout": {"secs": 30, "nanos": 0}, + "retry_attempts": 3, + "enable_cache": true, + "cache_config": { + "max_keys": 1000, + "ttl": {"secs": 3600, "nanos": 0}, + "enable_metrics": true + } + }"#; + let config: KmsConfig = serde_json::from_str(raw).expect("persisted kms config without mount_path"); + assert_eq!(config.backend, KmsBackend::VaultKv2); + let vault = config.vault_config().expect("vault-kv2 config"); + assert_eq!(vault.mount_path, "transit"); + } + + #[test] + fn test_vault_kv2_empty_mount_path_passes_validation() { + let address = Url::parse("https://vault.example.com:8200").expect("Valid URL"); + let mut config = KmsConfig::vault(address, "test-token".to_string()); + if let BackendConfig::VaultKv2(vault) = &mut config.backend_config { + vault.mount_path = String::new(); + } + assert!(config.validate().is_ok(), "deprecated mount_path must not be required"); + } + + #[test] + fn test_vault_kv2_sources_do_not_claim_transit_wrapping() { + let sources = [ + ("config.rs", include_str!("config.rs")), + ("api_types.rs", include_str!("api_types.rs")), + ("backends/vault.rs", include_str!("backends/vault.rs")), + ("lib.rs", include_str!("lib.rs")), + ]; + // Assemble the needles at runtime so this guard does not match its own source. + let needles = [ + format!("wrapping via {}", "Transit"), + format!("KV v2 + {}", "Transit"), + format!("KV2+{}", "Transit"), + format!("you would use Vault's {} engine", "transit"), + ]; + for (name, source) in sources { + for needle in &needles { + assert!( + !source.contains(needle.as_str()), + "{name} still describes the Vault KV2 backend with `{needle}`" + ); + } + } + } + #[test] fn test_legacy_persisted_vault_transit_config_uses_metadata_defaults() { let raw = r#"{ diff --git a/crates/kms/src/lib.rs b/crates/kms/src/lib.rs index d15718ef9..3867ef7b0 100644 --- a/crates/kms/src/lib.rs +++ b/crates/kms/src/lib.rs @@ -20,7 +20,7 @@ //! //! ## Features //! -//! - **Multiple Backends**: Local file storage, Vault KV2+Transit, and Vault Transit (optional) +//! - **Multiple Backends**: Local file storage, Vault KV2 (plain KV storage), and Vault Transit (optional) //! - **Object Encryption**: Transparent S3-compatible object encryption //! - **Streaming Encryption**: Memory-efficient encryption for large files //! - **Key Management**: Full lifecycle management of encryption keys diff --git a/docs/operations/kms-backend-security.md b/docs/operations/kms-backend-security.md new file mode 100644 index 000000000..31c0d2e67 --- /dev/null +++ b/docs/operations/kms-backend-security.md @@ -0,0 +1,78 @@ +# KMS backend security properties + +RustFS ships several KMS backends. They differ not only in deployment effort +but in **where master key material lives and who can read it**. Pick a backend +based on the confidentiality boundary you need, not on the name alone. + +## Backend comparison + +| Backend | Config tag | Master key material location | At-rest protection of key material | Rotation | Intended use | +| --- | --- | --- | --- | --- | --- | +| Local | `Local` | Files under `key_dir`, encrypted with the configured local master key | Local master key (AES-GCM) + file permissions | Rejected (no versioned retention yet) | Development; single-node setups that accept host-level trust | +| Static | `Static` | Provided out-of-band via environment/file; never persisted by RustFS | Operator-managed secret distribution | Rejected (read-only backend) | Simple deployments with an external secret manager | +| Vault KV2 | `VaultKV2` (legacy alias `Vault`) | Stored **directly** in Vault KV v2 (Base64-encoded plaintext) | Vault ACLs + KV v2 at-rest encryption + TLS only | Rejected (no versioned retention yet) | Deployments that accept Vault KV ACLs as the sole confidentiality boundary | +| Vault Transit | `VaultTransit` | Key-encryption keys never leave Vault; only Transit ciphertext is visible outside | Vault Transit engine (cryptographic isolation) | Via Vault Transit key versioning | Deployments that need key material to be unreadable through storage APIs | + +## Vault KV2: what the backend does and does not do + +The Vault KV2 backend uses Vault purely as a **secure storage** service: + +- Master key material is generated by RustFS and written to KV v2 as a + Base64-encoded value (`encrypted_key_material` is an encoding, not a + ciphertext). +- The backend never calls the Vault Transit engine. The `mount_path` + configuration field and the `RUSTFS_KMS_VAULT_MOUNT_PATH` environment + variable are deprecated leftovers: they are accepted for compatibility and + ignored. +- Data-encryption keys (DEKs) handed to the object-encryption path are still + wrapped with AES-256-GCM under the master key; the statement above concerns + the master key's storage in Vault, not the DEK envelope. +- The backend reports this boundary in its `backend_info` metadata as + `at_rest_protection: vault-kv2-acl`. +- Key rotation is rejected (`InvalidOperation`) until versioned key material + retention lands; rotating by overwriting the stored key would orphan every + DEK wrapped by the previous version. + +> **Warning: KV read access is equivalent to holding the master keys.** +> Any Vault identity (token, AppRole, or policy) that can `read` the RustFS +> key path in KV v2 can recover the plaintext master key material and decrypt +> every object protected by those keys. Treat KV read grants on that path with +> the same care as handing out the keys themselves. If this is not acceptable, +> use the Vault Transit backend instead. + +## Minimal Vault policy for the KV2 backend + +Scope the RustFS token/AppRole to exactly the KV v2 mount and key prefix it is +configured with (defaults shown: mount `secret`, prefix `rustfs/kms/keys`), +and grant no other identity read access to that subtree: + +```hcl +# RustFS KMS (Vault KV2 backend) — key storage only, no Transit access needed. +path "secret/data/rustfs/kms/keys/*" { + capabilities = ["create", "read", "update"] +} + +path "secret/metadata/rustfs/kms/keys/*" { + capabilities = ["list", "read", "delete"] +} +``` + +Notes: + +- `delete` on the metadata path is required for permanent key deletion + (`force_immediate`); drop it if you never hard-delete keys. +- Do not attach `sudo`, wildcard mounts, or Transit paths to this policy; the + KV2 backend does not use them. +- Auditing KV reads on the key prefix is strongly recommended: every read + event is a potential master-key disclosure. + +## Choosing between Vault KV2 and Vault Transit + +Use **Vault Transit** (`VaultTransit`) when key material must be +cryptographically isolated from anyone holding storage-level read access: +Transit keeps key-encryption keys inside Vault and only ever returns +ciphertext, and supports server-side key versioning/rotation. + +Use **Vault KV2** only when you accept that the Vault ACL on the key path *is* +the confidentiality boundary and you want the operational simplicity of a +single KV mount. diff --git a/rustfs/src/config/cli.rs b/rustfs/src/config/cli.rs index 35328037e..30332d128 100644 --- a/rustfs/src/config/cli.rs +++ b/rustfs/src/config/cli.rs @@ -314,7 +314,7 @@ pub struct ServerOpts { #[arg(long, default_value_t = false, env = "RUSTFS_KMS_ENABLE")] pub kms_enable: bool, - /// KMS backend type: local, vault or vault-kv2 (Vault KV2+Transit), vault-transit + /// KMS backend type: local, vault or vault-kv2 (plain Vault KV v2 storage), vault-transit #[arg(long, default_value_t = rustfs_config::DEFAULT_KMS_BACKEND.to_string(), env = "RUSTFS_KMS_BACKEND")] pub kms_backend: String,