From 03eb10b07f5f968c531151ae667dfe218050493d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=AE=89=E6=AD=A3=E8=B6=85?= Date: Tue, 9 Jun 2026 22:08:58 +0800 Subject: [PATCH] feat: add KMS config redaction safeguards (#3303) --- Cargo.lock | 1 + crates/kms/Cargo.toml | 1 + crates/kms/src/api_types.rs | 120 ++++++++++++++++- crates/kms/src/config.rs | 167 +++++++++++++++++++++++- docs/architecture/migration-progress.md | 75 +++++++---- 5 files changed, 329 insertions(+), 35 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4eaba5c1d..294519549 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9564,6 +9564,7 @@ dependencies = [ "moka", "rand 0.10.1", "reqwest", + "rustfs-security-governance", "rustfs-utils", "serde", "serde_json", diff --git a/crates/kms/Cargo.toml b/crates/kms/Cargo.toml index b786ff6b0..e1f41e731 100644 --- a/crates/kms/Cargo.toml +++ b/crates/kms/Cargo.toml @@ -57,6 +57,7 @@ moka = { workspace = true, features = ["future"] } md5 = { workspace = true } arc-swap = { workspace = true } rustfs-utils = { workspace = true } +rustfs-security-governance = { workspace = true } # HTTP client for Vault reqwest = { workspace = true } diff --git a/crates/kms/src/api_types.rs b/crates/kms/src/api_types.rs index c8957ea72..d0f26d7d8 100644 --- a/crates/kms/src/api_types.rs +++ b/crates/kms/src/api_types.rs @@ -16,16 +16,18 @@ use crate::config::{ BackendConfig, CacheConfig, KmsBackend, KmsConfig, LocalConfig, TlsConfig, VaultAuthMethod, VaultConfig, VaultTransitConfig, + redacted_secret_option, }; use crate::service_manager::KmsServiceStatus; use crate::types::{KeyMetadata, KeyUsage}; use serde::{Deserialize, Serialize}; use std::collections::HashMap; +use std::fmt; use std::path::PathBuf; use std::time::Duration; /// Request to configure KMS with Local backend -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Clone, Serialize, Deserialize)] pub struct ConfigureLocalKmsRequest { /// Directory to store key files pub key_dir: PathBuf, @@ -47,6 +49,23 @@ pub struct ConfigureLocalKmsRequest { pub cache_ttl_seconds: Option, } +impl fmt::Debug for ConfigureLocalKmsRequest { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let master_key = redacted_secret_option(self.master_key.as_deref()); + f.debug_struct("ConfigureLocalKmsRequest") + .field("key_dir", &self.key_dir) + .field("master_key", &master_key) + .field("file_permissions", &self.file_permissions) + .field("default_key_id", &self.default_key_id) + .field("timeout_seconds", &self.timeout_seconds) + .field("retry_attempts", &self.retry_attempts) + .field("enable_cache", &self.enable_cache) + .field("max_cached_keys", &self.max_cached_keys) + .field("cache_ttl_seconds", &self.cache_ttl_seconds) + .finish() + } +} + /// Request to configure KMS with Vault KV v2 + Transit backend #[derive(Debug, Clone, Serialize, Deserialize)] pub struct ConfigureVaultKmsRequest { @@ -428,6 +447,7 @@ impl ConfigureKmsRequest { #[cfg(test)] mod tests { use super::*; + use crate::config::REDACTED_SECRET; #[test] fn test_deserialize_vault_kv2_configure_request_accepts_type_aliases() { @@ -536,6 +556,104 @@ mod tests { other => panic!("expected vault-transit summary, got {other:?}"), } } + + #[test] + fn test_configure_request_debug_redacts_kms_secret_fields() { + let local = ConfigureKmsRequest::Local(ConfigureLocalKmsRequest { + key_dir: PathBuf::from("/tmp/kms"), + master_key: Some("local-configure-master-secret".to_string()), + file_permissions: Some(0o600), + default_key_id: Some("default-key".to_string()), + timeout_seconds: Some(30), + retry_attempts: Some(3), + enable_cache: Some(true), + max_cached_keys: Some(16), + cache_ttl_seconds: Some(60), + }); + let vault = ConfigureKmsRequest::VaultTransit(ConfigureVaultTransitKmsRequest { + address: "https://vault.example.com:8200".to_string(), + auth_method: VaultAuthMethod::Token { + token: "configure-vault-token-secret".to_string(), + }, + namespace: None, + mount_path: Some("transit".to_string()), + skip_tls_verify: Some(false), + default_key_id: None, + timeout_seconds: None, + retry_attempts: None, + enable_cache: None, + max_cached_keys: None, + cache_ttl_seconds: None, + }); + let approle = ConfigureKmsRequest::VaultKv2(ConfigureVaultKmsRequest { + address: "https://vault.example.com:8200".to_string(), + auth_method: VaultAuthMethod::AppRole { + role_id: "configure-role-id".to_string(), + secret_id: "configure-approle-secret-id".to_string(), + }, + namespace: None, + mount_path: Some("transit".to_string()), + kv_mount: Some("secret".to_string()), + key_path_prefix: Some("rustfs/kms/keys".to_string()), + skip_tls_verify: Some(false), + default_key_id: None, + timeout_seconds: None, + retry_attempts: None, + enable_cache: None, + max_cached_keys: None, + cache_ttl_seconds: None, + }); + + let rendered = format!("{local:?}\n{vault:?}\n{approle:?}"); + + assert!(!rendered.contains("local-configure-master-secret")); + assert!(!rendered.contains("configure-vault-token-secret")); + assert!(!rendered.contains("configure-approle-secret-id")); + assert!(rendered.contains("configure-role-id")); + assert!(rendered.contains(REDACTED_SECRET)); + } + + #[test] + fn test_kms_status_response_omits_secret_values_from_json_and_debug() { + let configs = [ + KmsConfig { + backend: KmsBackend::Local, + backend_config: BackendConfig::Local(LocalConfig { + key_dir: PathBuf::from("/tmp/kms"), + master_key: Some("local-summary-master-secret".to_string()), + file_permissions: Some(0o600), + }), + ..Default::default() + }, + KmsConfig::vault( + url::Url::parse("https://vault.example.com:8200").expect("vault URL"), + "summary-vault-token-secret".to_string(), + ), + KmsConfig::vault_approle( + url::Url::parse("https://vault.example.com:8200").expect("vault URL"), + "summary-role-id".to_string(), + "summary-approle-secret-id".to_string(), + ), + ]; + + for config in configs { + let summary = KmsConfigSummary::from(&config); + let response = KmsStatusResponse { + status: KmsServiceStatus::Configured, + backend_type: Some(config.backend.clone()), + healthy: None, + config_summary: Some(summary), + }; + let json = serde_json::to_string(&response).expect("kms status response should serialize"); + let debug = format!("{response:?}"); + let rendered = format!("{json}\n{debug}"); + + assert!(!rendered.contains("local-summary-master-secret")); + assert!(!rendered.contains("summary-vault-token-secret")); + assert!(!rendered.contains("summary-approle-secret-id")); + assert!(rendered.contains("has_master_key") || rendered.contains("has_stored_credentials")); + } + } } // ======================================== diff --git a/crates/kms/src/config.rs b/crates/kms/src/config.rs index e8721212c..194887ff5 100644 --- a/crates/kms/src/config.rs +++ b/crates/kms/src/config.rs @@ -15,12 +15,61 @@ //! KMS configuration management use crate::error::{KmsError, Result}; +use rustfs_security_governance::{RedactionLevel, RedactionRule}; use rustfs_utils::{get_env_bool, get_env_opt_str, get_env_str}; use serde::{Deserialize, Serialize}; +use std::fmt; use std::path::PathBuf; use std::time::Duration; use url::Url; +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"), + RedactionRule::new("kms.vault.approle.secret_id", RedactionLevel::Secret, "vault approle secret"), + RedactionRule::new("kms.vault_transit.token", RedactionLevel::Secret, "vault transit authentication token"), + RedactionRule::new( + "kms.vault_transit.approle.secret_id", + RedactionLevel::Secret, + "vault transit approle secret", + ), + RedactionRule::new( + "kms.configure.local.master_key", + RedactionLevel::Secret, + "admin configure request local master key", + ), + RedactionRule::new( + "kms.configure.vault.token", + RedactionLevel::Secret, + "admin configure request vault authentication token", + ), + RedactionRule::new( + "kms.configure.vault.approle.secret_id", + RedactionLevel::Secret, + "admin configure request vault approle secret", + ), + RedactionRule::new( + "kms.configure.vault_transit.token", + RedactionLevel::Secret, + "admin configure request vault transit authentication token", + ), + RedactionRule::new( + "kms.configure.vault_transit.approle.secret_id", + RedactionLevel::Secret, + "admin configure request vault transit approle secret", + ), +]; + +pub(crate) const REDACTED_SECRET: &str = "***redacted***"; + +pub(crate) fn redacted_secret(value: &str) -> &'static str { + if value.is_empty() { "" } else { REDACTED_SECRET } +} + +pub(crate) fn redacted_secret_option(value: Option<&str>) -> Option<&'static str> { + value.map(redacted_secret) +} + /// KMS backend types #[derive(Debug, Default, Clone, Serialize, Deserialize, PartialEq, Eq)] pub enum KmsBackend { @@ -69,7 +118,7 @@ impl Default for KmsConfig { } /// Backend-specific configuration -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Clone, Serialize, Deserialize)] pub enum BackendConfig { /// Local backend configuration Local(LocalConfig), @@ -86,8 +135,18 @@ impl Default for BackendConfig { } } +impl fmt::Debug for BackendConfig { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Local(config) => f.debug_tuple("Local").field(config).finish(), + Self::VaultKv2(config) => f.debug_tuple("VaultKv2").field(config).finish(), + Self::VaultTransit(config) => f.debug_tuple("VaultTransit").field(config).finish(), + } + } +} + /// Local KMS backend configuration -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Clone, Serialize, Deserialize)] pub struct LocalConfig { /// Directory to store key files pub key_dir: PathBuf, @@ -97,6 +156,17 @@ pub struct LocalConfig { pub file_permissions: Option, } +impl fmt::Debug for LocalConfig { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + let master_key = redacted_secret_option(self.master_key.as_deref()); + f.debug_struct("LocalConfig") + .field("key_dir", &self.key_dir) + .field("master_key", &master_key) + .field("file_permissions", &self.file_permissions) + .finish() + } +} + impl Default for LocalConfig { fn default() -> Self { Self { @@ -108,7 +178,7 @@ impl Default for LocalConfig { } /// Vault KV v2 + Transit backend configuration (metadata in KV, key wrapping via Transit) -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Clone, Serialize, Deserialize)] pub struct VaultConfig { /// Vault server URL pub address: String, @@ -126,6 +196,20 @@ pub struct VaultConfig { pub tls: Option, } +impl fmt::Debug for VaultConfig { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("VaultConfig") + .field("address", &self.address) + .field("auth_method", &self.auth_method) + .field("namespace", &self.namespace) + .field("mount_path", &self.mount_path) + .field("kv_mount", &self.kv_mount) + .field("key_path_prefix", &self.key_path_prefix) + .field("tls", &self.tls) + .finish() + } +} + impl Default for VaultConfig { fn default() -> Self { Self { @@ -143,7 +227,7 @@ impl Default for VaultConfig { } /// Vault Transit backend configuration -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Clone, Serialize, Deserialize)] pub struct VaultTransitConfig { /// Vault server URL pub address: String, @@ -157,6 +241,18 @@ pub struct VaultTransitConfig { pub tls: Option, } +impl fmt::Debug for VaultTransitConfig { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("VaultTransitConfig") + .field("address", &self.address) + .field("auth_method", &self.auth_method) + .field("namespace", &self.namespace) + .field("mount_path", &self.mount_path) + .field("tls", &self.tls) + .finish() + } +} + impl Default for VaultTransitConfig { fn default() -> Self { Self { @@ -172,7 +268,7 @@ impl Default for VaultTransitConfig { } /// Vault authentication methods -#[derive(Debug, Clone, Serialize, Deserialize)] +#[derive(Clone, Serialize, Deserialize)] pub enum VaultAuthMethod { /// Token authentication Token { token: String }, @@ -180,6 +276,19 @@ pub enum VaultAuthMethod { AppRole { role_id: String, secret_id: String }, } +impl fmt::Debug for VaultAuthMethod { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Token { token } => f.debug_struct("Token").field("token", &redacted_secret(token)).finish(), + Self::AppRole { role_id, secret_id } => f + .debug_struct("AppRole") + .field("role_id", role_id) + .field("secret_id", &redacted_secret(secret_id)) + .finish(), + } + } +} + /// TLS configuration for Vault #[derive(Debug, Clone, Serialize, Deserialize)] pub struct TlsConfig { @@ -460,6 +569,7 @@ impl KmsConfig { #[cfg(test)] mod tests { use super::*; + use rustfs_security_governance::validate_redaction_rules; use temp_env::with_vars; use tempfile::TempDir; @@ -551,6 +661,53 @@ mod tests { assert_eq!(serialized, "\"VaultTransit\""); } + #[test] + fn test_kms_redaction_rules_are_valid() { + assert!(validate_redaction_rules(KMS_CONFIG_REDACTION_RULES).is_ok()); + } + + #[test] + fn test_kms_config_debug_redacts_secret_fields() { + let local = KmsConfig { + backend: KmsBackend::Local, + backend_config: BackendConfig::Local(LocalConfig { + key_dir: PathBuf::from("/tmp/kms"), + master_key: Some("local-master-secret".to_string()), + file_permissions: Some(0o600), + }), + ..Default::default() + }; + let vault = KmsConfig::vault( + Url::parse("https://vault.example.com:8200").expect("vault URL"), + "vault-token-secret".to_string(), + ); + let approle = KmsConfig::vault_approle( + Url::parse("https://vault.example.com:8200").expect("vault URL"), + "role-id-visible".to_string(), + "approle-secret-id".to_string(), + ); + + let rendered = format!("{local:?}\n{vault:?}\n{approle:?}"); + + assert!(!rendered.contains("local-master-secret")); + assert!(!rendered.contains("vault-token-secret")); + assert!(!rendered.contains("approle-secret-id")); + assert!(rendered.contains("role-id-visible")); + assert!(rendered.contains(REDACTED_SECRET)); + } + + #[test] + fn test_kms_config_serialization_preserves_secret_fields_for_persistence() { + let config = KmsConfig::vault( + Url::parse("https://vault.example.com:8200").expect("vault URL"), + "persisted-token-secret".to_string(), + ); + + let serialized = serde_json::to_string(&config).expect("kms config should serialize for persistence"); + + assert!(serialized.contains("persisted-token-secret")); + } + #[test] fn test_config_validation() { let mut config = KmsConfig::default(); diff --git a/docs/architecture/migration-progress.md b/docs/architecture/migration-progress.md index 8d8e81422..239df75cc 100644 --- a/docs/architecture/migration-progress.md +++ b/docs/architecture/migration-progress.md @@ -5,16 +5,20 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block ## Current Context - Issue: [`rustfs/backlog#660`](https://github.com/rustfs/backlog/issues/660) -- Branch: `overtrue/arch-kms-handler-actions` -- Baseline: `upstream/main` at `4fec606dc4f92b19e085f1609a188e82a72720ff` +- Branch: `overtrue/arch-kms-redaction` +- Baseline: `upstream/main` at `0cdcd1eb7bfd5fc229eb45f851c624084b072365` - PR type for this branch: `security-change` -- Runtime behavior changes: high-risk KMS admin handlers now authorize through - dedicated `kms:*` actions instead of broad `ServerInfoAdminAction`. -- Rust code changes: migrate KMS handler action lists and route policy inventory - to dedicated KMS actions, while keeping temporary legacy create/status admin - action compatibility with `RUSTFS_COMPAT_TODO(S-012)`. +- Runtime behavior changes: KMS secret-bearing `Debug` output and admin status + summary views no longer expose local master keys, Vault tokens, or AppRole + secret IDs. KMS backend behavior, authorization, production defaults, and + config persistence are unchanged. +- Rust code changes: add KMS redaction rules, safe `Debug` implementations for + secret-bearing KMS config and configure request types, and focused tests that + prove secrets are absent from debug/admin views while serde persistence keeps + the original values. - CI/script changes: none -- Docs changes: record S-012 action migration status and temporary compatibility cleanup. +- Docs changes: record S-013 redaction status and the no-behavior-drift + migration boundary. ## Phase 0 Tasks @@ -108,50 +112,63 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block [`compat-cleanup-register.md`](compat-cleanup-register.md). - Verification: focused handler and route policy tests, migration rules, formatting, and `make pre-commit`. +- [x] `S-013` Apply KMS redaction. + - Acceptance: KMS Debug output and admin status response summaries contain no + Vault token, AppRole secret ID, or local master key values. + - Must preserve: internal KMS config values remain available to runtime code + and persisted config serialization still writes the original secret values. + - Verification: focused KMS redaction/status tests, full KMS tests, migration + guards, Rust quality scan, clippy, and `make pre-commit` passed. ## Next PRs -1. `contract`: add initial policy inventory tables for redaction, serde, or - supply-chain governance only after the contract shape remains stable. -2. `security-change`: apply KMS response/config redaction after action - migration settles. +1. `security-change`: inventory KMS development defaults before any production + default hardening. +2. `security-change`: apply IAM and plugin redaction in a separate S-014 PR. ## Pre-Push Review Log | Expert | Status | Notes | |---|---|---| -| Quality/architecture | pass | Single `security-change` PR; KMS auth action lists are local helper functions, names match `KmsAction`, and no storage/startup/global-state logic is touched. | -| Migration preservation | pass | Legacy create/status admin actions are retained only behind `RUSTFS_COMPAT_TODO(S-012)` and registered for cleanup; broad `ServerInfoAdminAction` is intentionally not retained for high-risk KMS operations. | -| Testing/verification | pass | Focused handler/route-policy tests cover dedicated actions and broad-action negative cases; migration rules, formatting, full `make pre-commit`, nextest, and doctests pass. | +| Quality/architecture | pass | Single `security-change` PR; redaction rules use the security-governance crate, custom `Debug` stays local to secret-bearing KMS types, and no startup/storage/global-state path is touched. | +| Migration preservation | pass | Runtime secret access and persisted config serialization are explicitly preserved by tests; no temporary compatibility path is introduced. | +| Testing/verification | pass | Focused redaction/status tests, full KMS tests, admin KMS handler tests, governance tests, clippy, migration guards, Rust quality scan, nextest, doctests, and `make pre-commit` passed. | ## Verification Notes Passed: -- Baseline `cargo test -p rustfs admin::handlers::kms --no-fail-fast` -- Baseline `cargo test -p rustfs admin::route_policy --no-fail-fast` -- `cargo fmt --all --check` +- `cargo test -p rustfs-kms redaction -- --nocapture` +- `cargo test -p rustfs-kms status_response -- --nocapture` +- `cargo test -p rustfs-kms --no-fail-fast` - `cargo test -p rustfs admin::handlers::kms --no-fail-fast` -- `cargo test -p rustfs admin::route_policy --no-fail-fast` +- `cargo test -p rustfs-security-governance --no-fail-fast` +- `cargo clippy -p rustfs-kms --all-targets --all-features -- -D warnings` +- Rust code quality scan on changed KMS source files +- `cargo fmt --all --check` +- `./scripts/check_layer_dependencies.sh` - `./scripts/check_architecture_migration_rules.sh` +- `./scripts/check_metrics_migration_refs.sh` - `git diff --check` - `make pre-commit` Notes: -- This branch changes only KMS admin authorization action selection and route - policy inventory. It does not change KMS runtime defaults, redaction, startup - order, global state, storage paths, or crate boundaries. -- `make pre-commit` passed all checks, including 5682 nextest tests and - workspace doctests. +- This branch changes only KMS redaction for debug/admin view surfaces. It does + not change KMS authorization, production defaults, startup order, global + state, storage paths, route registration, or crate boundaries. +- Config serialization still preserves secret values for persisted cluster + config; this is tested explicitly to avoid runtime data loss. +- `make pre-commit` passed all checks, including 5691 nextest tests, 111 + skipped tests, and workspace doctests. ## Handoff Notes -- Keep this S-012 branch as a focused `security-change` PR. Do not change KMS - defaults, redaction, admin route registration shape, Config moves, Storage API - moves, Runtime moves, or ECStore moves. +- Keep this S-013 branch as a focused `security-change` PR. Do not change KMS + defaults, admin authorization, admin route registration shape, Config moves, + Storage API moves, Runtime moves, or ECStore moves. - `rustfs` may depend on `rustfs-security-governance` for contract metadata; the security-governance crate must stay independent from implementation crates and runtime state. - Do not add temporary compatibility code without a matching `RUSTFS_COMPAT_TODO()` marker and cleanup-register entry. -- The next KMS security PR should handle redaction or production default - hardening separately; do not bundle those with this action migration. +- KMS production default hardening remains a separate task group; do not bundle + it with this redaction PR.