security(kms): require explicit dev defaults opt-in (#3369)

* security(kms): require explicit dev defaults opt-in

* test(kms): satisfy clippy dev defaults checks
This commit is contained in:
安正超
2026-06-12 07:05:20 +08:00
committed by GitHub
parent a85cc0354c
commit c3055f9335
18 changed files with 446 additions and 85 deletions
+61
View File
@@ -47,6 +47,8 @@ pub struct ConfigureLocalKmsRequest {
pub max_cached_keys: Option<usize>,
/// Cache TTL in seconds
pub cache_ttl_seconds: Option<u64>,
/// Allow development-only insecure defaults
pub allow_insecure_dev_defaults: Option<bool>,
}
impl fmt::Debug for ConfigureLocalKmsRequest {
@@ -62,6 +64,7 @@ impl fmt::Debug for ConfigureLocalKmsRequest {
.field("enable_cache", &self.enable_cache)
.field("max_cached_keys", &self.max_cached_keys)
.field("cache_ttl_seconds", &self.cache_ttl_seconds)
.field("allow_insecure_dev_defaults", &self.allow_insecure_dev_defaults)
.finish()
}
}
@@ -95,6 +98,8 @@ pub struct ConfigureVaultKmsRequest {
pub max_cached_keys: Option<usize>,
/// Cache TTL in seconds
pub cache_ttl_seconds: Option<u64>,
/// Allow development-only insecure defaults
pub allow_insecure_dev_defaults: Option<bool>,
}
/// Request to configure KMS with Vault Transit backend
@@ -122,6 +127,8 @@ pub struct ConfigureVaultTransitKmsRequest {
pub max_cached_keys: Option<usize>,
/// Cache TTL in seconds
pub cache_ttl_seconds: Option<u64>,
/// Allow development-only insecure defaults
pub allow_insecure_dev_defaults: Option<bool>,
}
/// Generic KMS configuration request
@@ -351,6 +358,7 @@ impl ConfigureLocalKmsRequest {
master_key: self.master_key.clone(),
file_permissions: self.file_permissions,
}),
allow_insecure_dev_defaults: self.allow_insecure_dev_defaults.unwrap_or(false),
timeout: Duration::from_secs(self.timeout_seconds.unwrap_or(30)),
retry_attempts: self.retry_attempts.unwrap_or(3),
enable_cache: self.enable_cache.unwrap_or(true),
@@ -387,6 +395,7 @@ impl ConfigureVaultKmsRequest {
None
},
})),
allow_insecure_dev_defaults: self.allow_insecure_dev_defaults.unwrap_or(false),
timeout: Duration::from_secs(self.timeout_seconds.unwrap_or(30)),
retry_attempts: self.retry_attempts.unwrap_or(3),
enable_cache: self.enable_cache.unwrap_or(true),
@@ -421,6 +430,7 @@ impl ConfigureVaultTransitKmsRequest {
None
},
})),
allow_insecure_dev_defaults: self.allow_insecure_dev_defaults.unwrap_or(false),
timeout: Duration::from_secs(self.timeout_seconds.unwrap_or(30)),
retry_attempts: self.retry_attempts.unwrap_or(3),
enable_cache: self.enable_cache.unwrap_or(true),
@@ -509,6 +519,53 @@ mod tests {
assert_eq!(config.backend, KmsBackend::Local);
}
#[test]
fn test_configure_request_development_defaults_require_opt_in() {
let local_raw = serde_json::json!({
"backend_type": "local",
"key_dir": "/tmp/kms-key-dir"
});
let request: ConfigureKmsRequest = serde_json::from_value(local_raw).expect("local request should deserialize");
let config = request.to_kms_config();
assert!(config.validate().is_err());
let local_opt_in_raw = serde_json::json!({
"backend_type": "local",
"key_dir": "/tmp/kms-key-dir",
"allow_insecure_dev_defaults": true
});
let request: ConfigureKmsRequest = serde_json::from_value(local_opt_in_raw).expect("local request should deserialize");
assert!(request.to_kms_config().validate().is_ok());
let vault_raw = serde_json::json!({
"backend_type": "vault",
"address": "http://127.0.0.1:8200",
"auth_method": {
"Token": {
"token": "dev-token"
}
},
"skip_tls_verify": true
});
let request: ConfigureKmsRequest = serde_json::from_value(vault_raw).expect("vault request should deserialize");
let config = request.to_kms_config();
assert!(config.validate().is_err());
let vault_opt_in_raw = serde_json::json!({
"backend_type": "vault",
"address": "http://127.0.0.1:8200",
"auth_method": {
"Token": {
"token": "dev-token"
}
},
"skip_tls_verify": true,
"allow_insecure_dev_defaults": true
});
let request: ConfigureKmsRequest = serde_json::from_value(vault_opt_in_raw).expect("vault request should deserialize");
assert!(request.to_kms_config().validate().is_ok());
}
#[test]
fn test_vault_transit_summary_reports_backend_details() {
let config = KmsConfig {
@@ -523,6 +580,7 @@ mod tests {
mount_path: "transit".to_string(),
tls: None,
})),
allow_insecure_dev_defaults: true,
timeout: Duration::from_secs(30),
retry_attempts: 3,
enable_cache: true,
@@ -569,6 +627,7 @@ mod tests {
enable_cache: Some(true),
max_cached_keys: Some(16),
cache_ttl_seconds: Some(60),
allow_insecure_dev_defaults: None,
});
let vault = ConfigureKmsRequest::VaultTransit(ConfigureVaultTransitKmsRequest {
address: "https://vault.example.com:8200".to_string(),
@@ -584,6 +643,7 @@ mod tests {
enable_cache: None,
max_cached_keys: None,
cache_ttl_seconds: None,
allow_insecure_dev_defaults: None,
});
let approle = ConfigureKmsRequest::VaultKv2(ConfigureVaultKmsRequest {
address: "https://vault.example.com:8200".to_string(),
@@ -602,6 +662,7 @@ mod tests {
enable_cache: None,
max_cached_keys: None,
cache_ttl_seconds: None,
allow_insecure_dev_defaults: None,
});
let rendered = format!("{local:?}\n{vault:?}\n{approle:?}");
+2
View File
@@ -564,6 +564,8 @@ pub struct LocalKmsBackend {
impl LocalKmsBackend {
/// Create a new LocalKmsBackend
pub async fn new(config: KmsConfig) -> Result<Self> {
config.validate()?;
let local_config = match &config.backend_config {
crate::config::BackendConfig::Local(local_config) => local_config.clone(),
crate::config::BackendConfig::VaultKv2(_) | crate::config::BackendConfig::VaultTransit(_) => {
+2
View File
@@ -595,6 +595,8 @@ pub struct VaultKmsBackend {
impl VaultKmsBackend {
/// Create a new VaultKmsBackend
pub async fn new(config: KmsConfig) -> Result<Self> {
config.validate()?;
let vault_config = match &config.backend_config {
crate::config::BackendConfig::VaultKv2(vault_config) => (**vault_config).clone(),
crate::config::BackendConfig::Local(_) | crate::config::BackendConfig::VaultTransit(_) => {
+2
View File
@@ -483,6 +483,8 @@ pub struct VaultTransitKmsBackend {
impl VaultTransitKmsBackend {
pub async fn new(config: KmsConfig) -> Result<Self> {
config.validate()?;
let vault_config = match &config.backend_config {
crate::config::BackendConfig::VaultTransit(vault_config) => (**vault_config).clone(),
crate::config::BackendConfig::VaultKv2(vault_config) => VaultTransitConfig {
+199 -6
View File
@@ -19,10 +19,13 @@ 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::path::{Path, PathBuf};
use std::time::Duration;
use url::Url;
pub const ENV_KMS_ALLOW_INSECURE_DEV_DEFAULTS: &str = "RUSTFS_KMS_ALLOW_INSECURE_DEV_DEFAULTS";
pub const ENV_KMS_VAULT_SKIP_TLS_VERIFY: &str = "RUSTFS_KMS_VAULT_SKIP_TLS_VERIFY";
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"),
@@ -93,6 +96,9 @@ pub struct KmsConfig {
pub default_key_id: Option<String>,
/// Backend-specific configuration
pub backend_config: BackendConfig,
/// Allow development-only insecure defaults such as plaintext local keys or HTTP Vault.
#[serde(default)]
pub allow_insecure_dev_defaults: bool,
/// Operation timeout
pub timeout: Duration,
/// Number of retry attempts
@@ -109,6 +115,7 @@ impl Default for KmsConfig {
backend: KmsBackend::default(),
default_key_id: None,
backend_config: BackendConfig::default(),
allow_insecure_dev_defaults: false,
timeout: Duration::from_secs(30),
retry_attempts: 3,
enable_cache: true,
@@ -405,6 +412,12 @@ impl KmsConfig {
self
}
/// Explicitly allow development-only KMS defaults.
pub fn with_insecure_development_defaults(mut self) -> Self {
self.allow_insecure_dev_defaults = true;
self
}
/// Set operation timeout
pub fn with_timeout(mut self, timeout: Duration) -> Self {
self.timeout = timeout;
@@ -435,12 +448,30 @@ impl KmsConfig {
if !config.key_dir.is_absolute() {
return Err(KmsError::configuration_error("Local key directory must be an absolute path"));
}
if !self.allow_insecure_dev_defaults {
if config.master_key.as_deref().is_none_or(str::is_empty) {
return Err(development_default_error(
"Local KMS requires a master key outside explicit development mode",
));
}
if is_under_temp_dir(&config.key_dir) {
return Err(development_default_error(
"Local KMS key directory must not be under the process temp directory outside explicit development mode",
));
}
}
}
BackendConfig::VaultKv2(config) => {
if !config.address.starts_with("http://") && !config.address.starts_with("https://") {
return Err(KmsError::configuration_error("Vault KV2 address must use http or https scheme"));
}
if !self.allow_insecure_dev_defaults {
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"));
}
@@ -461,6 +492,15 @@ impl KmsConfig {
return Err(KmsError::configuration_error("Vault Transit address must use http or https scheme"));
}
if !self.allow_insecure_dev_defaults {
validate_vault_development_defaults(
"Vault Transit",
&config.address,
&config.auth_method,
config.tls.as_ref(),
)?;
}
if config.mount_path.is_empty() {
return Err(KmsError::configuration_error("Vault Transit mount path cannot be empty"));
}
@@ -520,6 +560,8 @@ impl KmsConfig {
// Enable cache
config.enable_cache = get_env_bool("RUSTFS_KMS_ENABLE_CACHE", config.enable_cache);
config.allow_insecure_dev_defaults =
get_env_bool(ENV_KMS_ALLOW_INSECURE_DEV_DEFAULTS, config.allow_insecure_dev_defaults);
// Backend-specific configuration
match config.backend {
@@ -536,6 +578,7 @@ impl KmsConfig {
KmsBackend::VaultKv2 => {
let address = get_env_str("RUSTFS_KMS_VAULT_ADDRESS", "http://localhost:8200");
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);
config.backend_config = BackendConfig::VaultKv2(Box::new(VaultConfig {
address,
@@ -544,19 +587,20 @@ impl KmsConfig {
mount_path: get_env_str("RUSTFS_KMS_VAULT_MOUNT_PATH", "transit"),
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: None,
tls: vault_tls_config(skip_tls_verify),
}));
}
KmsBackend::VaultTransit => {
let address = get_env_str("RUSTFS_KMS_VAULT_ADDRESS", "http://localhost:8200");
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);
config.backend_config = BackendConfig::VaultTransit(Box::new(VaultTransitConfig {
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"),
tls: None,
tls: vault_tls_config(skip_tls_verify),
}));
}
}
@@ -566,6 +610,50 @@ impl KmsConfig {
}
}
fn vault_tls_config(skip_tls_verify: bool) -> Option<TlsConfig> {
skip_tls_verify.then_some(TlsConfig {
ca_cert_path: None,
client_cert_path: None,
client_key_path: None,
skip_verify: true,
})
}
fn development_default_error(reason: &str) -> KmsError {
KmsError::configuration_error(format!("{reason}; set {ENV_KMS_ALLOW_INSECURE_DEV_DEFAULTS}=true only for development"))
}
fn is_under_temp_dir(path: &Path) -> bool {
path.starts_with(std::env::temp_dir())
}
fn validate_vault_development_defaults(
backend_name: &str,
address: &str,
auth_method: &VaultAuthMethod,
tls: Option<&TlsConfig>,
) -> Result<()> {
if address.starts_with("http://") {
return Err(development_default_error(&format!(
"{backend_name} requires HTTPS outside explicit development mode"
)));
}
if matches!(auth_method, VaultAuthMethod::Token { token } if token == "dev-token") {
return Err(development_default_error(&format!(
"{backend_name} default dev-token is not allowed outside explicit development mode"
)));
}
if tls.is_some_and(|tls| tls.skip_verify) {
return Err(development_default_error(&format!(
"{backend_name} skip TLS verification is not allowed outside explicit development mode"
)));
}
Ok(())
}
#[cfg(test)]
mod tests {
use super::*;
@@ -577,13 +665,14 @@ mod tests {
fn test_default_config() {
let config = KmsConfig::default();
assert_eq!(config.backend, KmsBackend::Local);
assert!(config.validate().is_ok());
assert!(config.validate().is_err());
assert!(config.with_insecure_development_defaults().validate().is_ok());
}
#[test]
fn test_local_config() {
let temp_dir = TempDir::new().expect("Failed to create temp dir");
let config = KmsConfig::local(temp_dir.path().to_path_buf());
let config = KmsConfig::local(temp_dir.path().to_path_buf()).with_insecure_development_defaults();
assert_eq!(config.backend, KmsBackend::Local);
assert!(config.validate().is_ok());
@@ -592,6 +681,27 @@ mod tests {
assert_eq!(local_config.key_dir, temp_dir.path());
}
#[test]
fn test_local_development_defaults_require_opt_in() {
let temp_dir = TempDir::new().expect("Failed to create temp dir");
let config = KmsConfig::local(temp_dir.path().to_path_buf());
assert!(config.validate().is_err());
assert!(config.with_insecure_development_defaults().validate().is_ok());
let production_config = KmsConfig {
backend: KmsBackend::Local,
backend_config: BackendConfig::Local(LocalConfig {
key_dir: PathBuf::from("/var/lib/rustfs/kms"),
master_key: Some("production-master-key".to_string()),
file_permissions: Some(0o600),
}),
..Default::default()
};
assert!(production_config.validate().is_ok());
}
#[test]
fn test_vault_config() {
let address = Url::parse("https://vault.example.com:8200").expect("Valid URL");
@@ -617,6 +727,42 @@ mod tests {
assert_eq!(vault_config.mount_path, "transit");
}
#[test]
fn test_vault_development_defaults_require_opt_in() {
let http_address = Url::parse("http://127.0.0.1:8200").expect("Valid URL");
let https_address = Url::parse("https://vault.example.com:8200").expect("Valid URL");
let http_config = KmsConfig::vault(http_address, "vault-token".to_string());
assert!(http_config.validate().is_err());
assert!(http_config.with_insecure_development_defaults().validate().is_ok());
let dev_token_config = KmsConfig::vault(https_address.clone(), "dev-token".to_string());
assert!(dev_token_config.validate().is_err());
assert!(dev_token_config.with_insecure_development_defaults().validate().is_ok());
let skip_tls_config = KmsConfig {
backend: KmsBackend::VaultTransit,
backend_config: BackendConfig::VaultTransit(Box::new(VaultTransitConfig {
address: https_address.to_string(),
auth_method: VaultAuthMethod::Token {
token: "vault-token".to_string(),
},
namespace: None,
mount_path: "transit".to_string(),
tls: Some(TlsConfig {
ca_cert_path: None,
client_cert_path: None,
client_key_path: None,
skip_verify: true,
}),
})),
..Default::default()
};
assert!(skip_tls_config.validate().is_err());
assert!(skip_tls_config.with_insecure_development_defaults().validate().is_ok());
}
#[test]
fn test_vault_kv2_backend_serialization_uses_pascal_case() {
let serialized = serde_json::to_string(&KmsBackend::VaultKv2).expect("backend should serialize");
@@ -710,7 +856,10 @@ mod tests {
#[test]
fn test_config_validation() {
let mut config = KmsConfig::default();
let mut config = KmsConfig {
allow_insecure_dev_defaults: true,
..Default::default()
};
// Valid config
assert!(config.validate().is_ok());
@@ -760,6 +909,50 @@ mod tests {
);
}
#[test]
fn test_from_env_requires_vault_development_opt_in() {
with_vars(
vec![
("RUSTFS_KMS_BACKEND", Some("vault")),
("RUSTFS_KMS_VAULT_ADDRESS", Some("http://127.0.0.1:8200")),
("RUSTFS_KMS_VAULT_TOKEN", Some("dev-token")),
],
|| {
let error = KmsConfig::from_env().expect_err("vault dev defaults should fail closed");
assert!(error.to_string().contains(ENV_KMS_ALLOW_INSECURE_DEV_DEFAULTS));
},
);
with_vars(
vec![
("RUSTFS_KMS_BACKEND", Some("vault")),
("RUSTFS_KMS_VAULT_ADDRESS", Some("http://127.0.0.1:8200")),
("RUSTFS_KMS_VAULT_TOKEN", Some("dev-token")),
(ENV_KMS_ALLOW_INSECURE_DEV_DEFAULTS, Some("true")),
],
|| {
let config = KmsConfig::from_env().expect("explicit development opt-in should allow vault dev defaults");
assert!(config.allow_insecure_dev_defaults);
},
);
}
#[test]
fn test_from_env_rejects_vault_skip_tls_verify_without_opt_in() {
with_vars(
vec![
("RUSTFS_KMS_BACKEND", Some("vault-transit")),
("RUSTFS_KMS_VAULT_ADDRESS", Some("https://vault.example.com")),
("RUSTFS_KMS_VAULT_TOKEN", Some("vault-token")),
(ENV_KMS_VAULT_SKIP_TLS_VERIFY, Some("true")),
],
|| {
let error = KmsConfig::from_env().expect_err("skip TLS verify should fail closed");
assert!(error.to_string().contains(ENV_KMS_ALLOW_INSECURE_DEV_DEFAULTS));
},
);
}
#[test]
fn test_from_env_reads_vault_transit_settings() {
with_vars(
+6 -6
View File
@@ -53,7 +53,7 @@
//! let service_manager = init_global_kms_service_manager();
//!
//! // Configure with local backend
//! let config = KmsConfig::local(PathBuf::from("./kms_keys"));
//! let config = KmsConfig::local(PathBuf::from("./kms_keys")).with_insecure_development_defaults();
//! service_manager.configure(config).await?;
//!
//! // Start the KMS service
@@ -135,7 +135,7 @@ mod tests {
// Test configuration and start
let temp_dir = TempDir::new().expect("Failed to create temp dir");
let config = KmsConfig::local(temp_dir.path().to_path_buf());
let config = KmsConfig::local(temp_dir.path().to_path_buf()).with_insecure_development_defaults();
manager.configure(config).await.expect("Configuration should succeed");
manager.start().await.expect("Start should succeed");
@@ -160,7 +160,7 @@ mod tests {
// Start first service
let temp_dir1 = TempDir::new().expect("Failed to create temp dir");
let config1 = KmsConfig::local(temp_dir1.path().to_path_buf());
let config1 = KmsConfig::local(temp_dir1.path().to_path_buf()).with_insecure_development_defaults();
manager
.configure(config1.clone())
.await
@@ -176,7 +176,7 @@ mod tests {
// 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());
let config2 = KmsConfig::local(temp_dir2.path().to_path_buf()).with_insecure_development_defaults();
manager.reconfigure(config2).await.expect("Reconfiguration should succeed");
// Verify version 2
@@ -205,7 +205,7 @@ mod tests {
let base_path = temp_dir.path().to_path_buf();
// Initial configuration
let config1 = KmsConfig::local(base_path.clone());
let config1 = KmsConfig::local(base_path.clone()).with_insecure_development_defaults();
manager.configure(config1).await.expect("Configuration should succeed");
manager.start().await.expect("Start should succeed");
@@ -215,7 +215,7 @@ mod tests {
let manager_clone = manager.clone();
let path = base_path.clone();
let handle = tokio::spawn(async move {
let config = KmsConfig::local(path);
let config = KmsConfig::local(path).with_insecure_development_defaults();
manager_clone.reconfigure(config).await
});
handles.push(handle);
+2 -2
View File
@@ -166,7 +166,7 @@ mod tests {
#[tokio::test]
async fn test_manager_operations() {
let temp_dir = tempdir().expect("Failed to create temp dir");
let config = KmsConfig::local(temp_dir.path().to_path_buf());
let config = KmsConfig::local(temp_dir.path().to_path_buf()).with_insecure_development_defaults();
let backend = Arc::new(LocalKmsBackend::new(config.clone()).await.expect("Failed to create backend"));
let manager = KmsManager::new(backend, config);
@@ -216,7 +216,7 @@ mod tests {
#[tokio::test]
async fn generate_data_key_does_not_reuse_context_bound_ciphertext() {
let temp_dir = tempdir().expect("Failed to create temp dir");
let config = KmsConfig::local(temp_dir.path().to_path_buf());
let config = KmsConfig::local(temp_dir.path().to_path_buf()).with_insecure_development_defaults();
let backend = Arc::new(LocalKmsBackend::new(config.clone()).await.expect("Failed to create backend"));
let manager = KmsManager::new(backend, config);
+3 -1
View File
@@ -738,7 +738,9 @@ mod tests {
async fn create_test_service() -> (ObjectEncryptionService, TempDir) {
let temp_dir = TempDir::new().expect("Failed to create temp dir");
let config = KmsConfig::local(temp_dir.path().to_path_buf()).with_default_key("test-key".to_string());
let config = KmsConfig::local(temp_dir.path().to_path_buf())
.with_insecure_development_defaults()
.with_default_key("test-key".to_string());
let backend = Arc::new(
crate::backends::local::LocalKmsBackend::new(config.clone())
.await
+24
View File
@@ -92,6 +92,8 @@ impl KmsServiceManager {
/// Configure KMS with new configuration
pub async fn configure(&self, new_config: KmsConfig) -> Result<()> {
new_config.validate()?;
// Update configuration
{
let mut config = self.config.write().await;
@@ -200,6 +202,7 @@ impl KmsServiceManager {
let _guard = self.lifecycle_mutex.lock().await;
info!("Reconfiguring KMS service (zero-downtime)");
new_config.validate()?;
// Configure with new config
{
@@ -307,6 +310,8 @@ impl KmsServiceManager {
///
/// This creates a new backend, manager, and service, and assigns it a new version number.
async fn create_service_version(&self, config: &KmsConfig) -> Result<ServiceVersion> {
config.validate()?;
// Increment version counter
let version = self.version_counter.fetch_add(1, Ordering::Relaxed) + 1;
@@ -371,3 +376,22 @@ pub async fn get_global_encryption_service() -> Option<Arc<ObjectEncryptionServi
let manager = get_global_kms_service_manager().unwrap_or_else(init_global_kms_service_manager);
manager.get_encryption_service().await
}
#[cfg(test)]
mod tests {
use super::*;
#[tokio::test]
async fn configure_rejects_insecure_development_defaults_before_state_update() {
let manager = KmsServiceManager::new();
let error = manager
.configure(KmsConfig::default())
.await
.expect_err("unsafe local defaults should fail validation");
assert!(error.to_string().contains(crate::config::ENV_KMS_ALLOW_INSECURE_DEV_DEFAULTS));
assert_eq!(manager.get_status().await, KmsServiceStatus::NotConfigured);
assert!(manager.get_config().await.is_none());
}
}
@@ -46,32 +46,56 @@ current KMS development defaults before any production default hardening.
- Existing validation does not fail closed for HTTP Vault addresses, dev-token,
missing local master key, temp key dirs, or explicit `skip_tls_verify = true`.
## Hardening Follow-Ups
## Hardening Behavior
`KMSD-002` should make Local KMS unsafe defaults explicit development opt-ins or
`KMSD-002` makes Local KMS unsafe defaults explicit development opt-ins or
production failures:
- no local master key;
- local key directory under the process temp directory;
- local env defaults that cannot pass validation without an absolute path.
- no local master key fails validation unless
`allow_insecure_dev_defaults = true`;
- local key directories under the process temp directory fail validation unless
`allow_insecure_dev_defaults = true`;
- `RUSTFS_KMS_LOCAL_MASTER_KEY` is the production-safe local CLI/env path for
encrypted local key files;
- `RUSTFS_KMS_ALLOW_INSECURE_DEV_DEFAULTS=true` is the development-only escape
hatch for local plaintext or temp-dir setups.
`KMSD-003` should make Vault unsafe defaults explicit development opt-ins or
`KMSD-003` makes Vault unsafe defaults explicit development opt-ins or
production failures:
- HTTP Vault addresses;
- default dev-token credentials;
- explicit `skip_tls_verify = true`.
- HTTP Vault addresses fail validation unless explicit development opt-in is set;
- default `dev-token` credentials fail validation unless explicit development
opt-in is set;
- explicit `skip_tls_verify = true` fails validation unless explicit development
opt-in is set;
- `RUSTFS_KMS_ALLOW_INSECURE_DEV_DEFAULTS=true` applies to `KmsConfig::from_env`;
- admin configure requests can set `allow_insecure_dev_defaults = true` for the
same development-only behavior.
Both follow-ups must preserve existing development workflows through documented
compatibility behavior or explicit development mode. They must not modify KMS
runtime logic only to satisfy tests.
These checks run in `KmsConfig::validate()` so CLI startup, persisted dynamic
configuration, service-manager start/reconfigure, and direct backend
construction use the same fail-closed behavior.
## Test Expectations For Follow-Ups
## Compatibility Notes
- Add focused negative tests before changing production default behavior.
- Keep persisted config serialization compatibility tests separate from runtime
fail-closed tests.
- Cover both env-loaded configuration and admin configure request conversion.
- Prove development opt-in paths remain explicit and searchable.
- Do not alter KMS key operation behavior, authorization actions, cache behavior,
or storage hot paths while hardening defaults.
- Production Local KMS deployments should configure an absolute key directory
outside the process temp directory and set `RUSTFS_KMS_LOCAL_MASTER_KEY`.
- Local development setups that intentionally store plaintext key files or use
temp directories must set `RUSTFS_KMS_ALLOW_INSECURE_DEV_DEFAULTS=true` or the
admin request field `allow_insecure_dev_defaults = true`.
- Production Vault deployments should use HTTPS, non-default credentials, and
TLS verification.
- Local Vault development setups that intentionally use HTTP, `dev-token`, or
skip TLS verification must set the same explicit development opt-in.
- Legacy persisted KMS config JSON remains deserializable; old unsafe persisted
values default to production mode and fail validation until secured or
explicitly marked development-only.
## Test Coverage
- Local production fail-closed and development opt-in validation.
- Vault HTTP, default token, and skip-TLS fail-closed validation plus explicit
development opt-in.
- `KmsConfig::from_env()` development default rejection and opt-in behavior.
- Admin configure request conversion to the same validation behavior.
- KMS service manager rejects unsafe configs before moving to `Configured`.
+62 -44
View File
@@ -5,17 +5,18 @@ 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-storage-api-namespace-lock-cleanup`
- Baseline: `origin/main` at `7146c893cbbb84a0d5332a7a8a79b70d57191bcc`
- PR type for this branch: `api-extraction`
- Runtime behavior changes: none intended.
- Rust code changes: migrate remaining namespace-lock consumers to
`NamespaceLocking`, implement namespace locking directly on ECStore storage
types, and remove the temporary namespace-lock compatibility method from the
full storage trait.
- Branch: `overtrue/arch-kms-dev-defaults`
- Baseline: `origin/main` at `a85cc0354c02fc55e2dd8eb64cfc6155c37921c7`
- PR type for this branch: `security-change`
- Runtime behavior changes: KMS development-only defaults now fail closed unless
`RUSTFS_KMS_ALLOW_INSECURE_DEV_DEFAULTS=true` or an admin configure request
sets `allow_insecure_dev_defaults=true`.
- Rust code changes: harden Local KMS missing-master-key/temp-dir defaults,
Vault HTTP/default-token/skip-TLS defaults, KMS service-manager validation,
admin configure request conversion, and server CLI KMS configuration.
- CI/script changes: none.
- Docs changes: remove the API-012 cleanup-register entry and record the
completed namespace-lock compatibility cleanup in progress notes.
- Docs changes: record KMS compatibility notes and mark `KMSD-002` through
`KMSD-005` complete.
## Phase 0 Tasks
@@ -193,6 +194,27 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block
authorization, startup order, storage path, or crate boundary changes.
- Verification: docs diff review, migration guards, metrics reference guard,
and `git diff --check`.
- [x] `KMSD-002` Make Local KMS unsafe defaults explicit dev opt-in.
- Acceptance: Local KMS now rejects missing master keys and process-temp key
directories unless `allow_insecure_dev_defaults` is explicitly set.
- Compatibility: server CLI/config now accepts `RUSTFS_KMS_LOCAL_MASTER_KEY`
for production local encryption and
`RUSTFS_KMS_ALLOW_INSECURE_DEV_DEFAULTS=true` for development-only local
setups.
- [x] `KMSD-003` Make Vault unsafe defaults explicit dev opt-in.
- Acceptance: Vault KV2 and Vault Transit now reject HTTP addresses,
`dev-token`, and `skip_tls_verify` unless explicit development opt-in is set.
- Compatibility: the KMS env loader and admin configure requests support the
same explicit development opt-in.
- [x] `KMSD-004` Add production KMS default tests.
- Acceptance: focused tests cover Local and Vault production rejection plus
explicit development opt-in paths across config, env loading, admin request
conversion, and service-manager validation.
- [x] `KMSD-005` Write KMS compatibility notes.
- Acceptance:
[`kms-development-defaults-inventory.md`](kms-development-defaults-inventory.md)
now records the production-safe alternatives and explicit development opt-in
behavior for deployments that relied on old defaults.
## Phase 2 Storage API Tasks
@@ -362,57 +384,53 @@ Status values: `[ ]` not started, `[~]` in progress, `[x]` complete, `[!]` block
## Next PRs
1. `security-change`: make Local KMS unsafe defaults explicit development
opt-ins or production failures in KMSD-002.
2. `security-change`: make Vault unsafe defaults explicit development opt-ins
or production failures in KMSD-003.
1. `security-change`: apply IAM and plugin secret redaction in `S-014`.
2. `security-change`: classify and add strict serde ingress tests in `S-015`.
## Pre-Push Review Log
| Expert | Status | Notes |
|---|---|---|
| Quality/architecture | pass | Confirmed the diff removes the temporary namespace-lock method from the full `StorageAPI` trait while keeping `NamespaceLocking` as the narrow operation-group contract. |
| Migration preservation | pass | Confirmed ECStore, Sets, SetDisks, scanner leader locking, self-copy locking, replication resync, rebalance metadata, and config test storage retain their existing lock behavior and only narrow trait dependencies. |
| Testing/verification | pass | Confirmed focused compile/tests, migration guards, dependency guard, old-path scan, and added-line Rust risk scan cover this cleanup; full pre-commit is skipped under the current larger-granularity instruction. |
| Quality/architecture | pass | Confirmed KMS unsafe-development defaults are enforced in `KmsConfig::validate()` and reused by env loading, admin configure conversion, service-manager lifecycle, and direct backend construction. |
| Migration preservation | pass | Confirmed KMS runtime key operations, cache behavior, redaction behavior, storage SSE call sites, and admin route contracts keep their existing shapes except for the explicit development opt-in field. |
| Testing/verification | pass | Confirmed KMS crate tests, rustfs config/SSE focused tests, rustfs lib/test compile, migration guards, format check, and diff check cover this larger security-change slice; full pre-commit is skipped under the current larger-granularity instruction. |
## Verification Notes
Passed on `7146c893cbbb84a0d5332a7a8a79b70d57191bcc`:
- `cargo check -p rustfs-ecstore -p rustfs-scanner -p rustfs --all-targets`.
- `cargo test -p rustfs-ecstore --test storage_api_compat_test`; 2 passed.
- `cargo test -p rustfs-ecstore --lib new_ns_lock`; 2 passed.
- `cargo test -p rustfs-scanner --tests --no-run`.
- `cargo test -p rustfs --lib --no-run`.
Passed on `a85cc0354c02fc55e2dd8eb64cfc6155c37921c7`:
- `cargo check -p rustfs --lib --tests`.
- `cargo check -p rustfs-kms --tests`.
- `cargo test -p rustfs-kms --no-fail-fast`; 57 passed, 1 ignored, doc-test
passed.
- `cargo clippy -p rustfs-kms --all-targets -- -D warnings`.
- `cargo test -p rustfs --lib config::config_test --no-fail-fast`; 20 passed.
- `cargo test -p rustfs --lib storage::sse::tests::test_kms --no-fail-fast`;
1 passed.
- `cargo test -p rustfs --lib -- --list | rg "sse.*kms|kms.*sse|config::config_test::tests::test_config_new_defaults"`.
- `cargo fmt --all --check`.
- `./scripts/check_architecture_migration_rules.sh`.
- `./scripts/check_layer_dependencies.sh`.
- `./scripts/check_metrics_migration_refs.sh`.
- `git diff --check`.
- API-012 old-path and marker scan found no stale API-012 compatibility marker
or old full-storage-trait namespace-lock method references in `crates`,
`rustfs/src`, or architecture docs.
- Added-line Rust risk scan found no new production `unwrap`/`expect`, lossy
numeric casts, stringly public errors, boxed dynamic errors, stdout/stderr
printing, or relaxed atomic ordering.
Notes:
- Full pre-commit may be skipped if focused tests, compile checks, and guards
pass, per the current instruction to increase PR granularity.
- This slice removes only the old namespace-lock method from the full storage
trait. ECStore retains bucket, object, listing, multipart, heal, replication,
rebalance, scanner, config persistence, and namespace-lock implementation
behavior.
- Consumers that need namespace locks should depend on `NamespaceLocking`
instead of the full storage trait.
- `cargo test -p rustfs --lib storage::sse::tests::test_sse_kms --no-fail-fast`
and `cargo test -p rustfs --lib test_sse_kms_roundtrip --no-fail-fast`
matched 0 tests because the `rio-v2` roundtrip test is feature-gated out of
the default `rustfs --lib` test binary.
- This slice includes a minimal compile unblock after PR #3365: table catalog's
ECStore object backend now declares `NamespaceLocking` where it calls
`new_ns_lock`; the lock path itself is unchanged.
- This slice does not change KMS authorization actions, key operation behavior,
storage encryption metadata formats, or cache semantics.
## Handoff Notes
- Keep this API-012 cleanup slice as an `api-extraction` PR that only removes
the temporary namespace-lock method from the full storage trait, migrates
namespace-lock consumers to `NamespaceLocking`, and deletes the
cleanup-register entry.
- Do not move storage traits, bucket/object/list/multipart/heal operation
logic, lock implementation code, storage runtime wiring, route behavior, or
storage persistence logic in this PR.
- Do not add temporary compatibility code unless a matching task marker and
cleanup-register entry are added.
- Keep this KMS slice as a `security-change` PR covering KMSD-002 through
KMSD-005 plus the minimal table catalog compile unblock from PR #3365.
- Do not change KMS key operation behavior, storage SSE metadata formats, IAM
policy actions, admin route wiring, or cache semantics in this PR.
- If more time is available before the next slice, start `S-014` with IAM/plugin
secret redaction; otherwise start `S-015` strict serde ingress tests.
+10
View File
@@ -254,6 +254,10 @@ pub struct ServerOpts {
#[arg(long, env = "RUSTFS_KMS_KEY_DIR")]
pub kms_key_dir: Option<String>,
/// Master key for local KMS key-file encryption
#[arg(long, env = "RUSTFS_KMS_LOCAL_MASTER_KEY")]
pub kms_local_master_key: Option<String>,
/// Vault address for vault backend
#[arg(long, env = "RUSTFS_KMS_VAULT_ADDRESS")]
pub kms_vault_address: Option<String>,
@@ -270,6 +274,10 @@ pub struct ServerOpts {
#[arg(long, env = "RUSTFS_KMS_DEFAULT_KEY_ID")]
pub kms_default_key_id: Option<String>,
/// Allow development-only insecure KMS defaults
#[arg(long, default_value_t = false, env = "RUSTFS_KMS_ALLOW_INSECURE_DEV_DEFAULTS")]
pub kms_allow_insecure_dev_defaults: bool,
/// Disable adaptive buffer sizing with workload profiles
/// Set this flag to use legacy fixed-size buffer behavior from PR #869
#[arg(long, default_value_t = false, env = "RUSTFS_BUFFER_PROFILE_DISABLE")]
@@ -316,10 +324,12 @@ pub fn default_server_opts() -> ServerOpts {
kms_enable: false,
kms_backend: "local".to_string(),
kms_key_dir: None,
kms_local_master_key: None,
kms_vault_address: None,
kms_vault_token: None,
kms_vault_mount_path: None,
kms_default_key_id: None,
kms_allow_insecure_dev_defaults: false,
buffer_profile_disable: false,
buffer_profile: "GeneralPurpose".to_string(),
}
+14
View File
@@ -125,6 +125,9 @@ pub struct Config {
/// KMS key directory for local backend
pub kms_key_dir: Option<String>,
/// Master key for local KMS key-file encryption
pub kms_local_master_key: Option<String>,
/// Vault address for vault backend
pub kms_vault_address: Option<String>,
@@ -137,6 +140,9 @@ pub struct Config {
/// Default KMS key ID for encryption
pub kms_default_key_id: Option<String>,
/// Allow development-only insecure KMS defaults
pub kms_allow_insecure_dev_defaults: bool,
/// Disable adaptive buffer sizing with workload profiles
pub buffer_profile_disable: bool,
@@ -165,10 +171,12 @@ impl Config {
kms_enable: false,
kms_backend: "local".to_string(),
kms_key_dir: None,
kms_local_master_key: None,
kms_vault_address: None,
kms_vault_token: None,
kms_vault_mount_path: None,
kms_default_key_id: None,
kms_allow_insecure_dev_defaults: false,
buffer_profile_disable: false,
buffer_profile: "GeneralPurpose".to_string(),
}
@@ -197,10 +205,12 @@ impl Config {
kms_enable,
kms_backend,
kms_key_dir,
kms_local_master_key,
kms_vault_address,
kms_vault_token,
kms_vault_mount_path,
kms_default_key_id,
kms_allow_insecure_dev_defaults,
buffer_profile_disable,
buffer_profile,
} = opt;
@@ -238,10 +248,12 @@ impl Config {
kms_enable,
kms_backend,
kms_key_dir,
kms_local_master_key,
kms_vault_address,
kms_vault_token,
kms_vault_mount_path,
kms_default_key_id,
kms_allow_insecure_dev_defaults,
buffer_profile_disable,
buffer_profile,
})
@@ -279,10 +291,12 @@ impl std::fmt::Debug for Config {
.field("kms_enable", &self.kms_enable)
.field("kms_backend", &self.kms_backend)
.field("kms_key_dir", &self.kms_key_dir)
.field("kms_local_master_key", &Masked(self.kms_local_master_key.as_deref()))
.field("kms_vault_address", &self.kms_vault_address)
.field("kms_vault_token", &Masked(self.kms_vault_token.as_deref()))
.field("kms_vault_mount_path", &self.kms_vault_mount_path)
.field("kms_default_key_id", &self.kms_default_key_id)
.field("kms_allow_insecure_dev_defaults", &self.kms_allow_insecure_dev_defaults)
.field("buffer_profile_disable", &self.buffer_profile_disable)
.field("buffer_profile", &self.buffer_profile)
.finish()
+2
View File
@@ -133,10 +133,12 @@ mod tests {
assert!(!config.kms_enable);
assert_eq!(config.kms_backend, "local");
assert_eq!(config.kms_key_dir, None);
assert_eq!(config.kms_local_master_key, None);
assert_eq!(config.kms_vault_address, None);
assert_eq!(config.kms_vault_token, None);
assert_eq!(config.kms_vault_mount_path, None);
assert_eq!(config.kms_default_key_id, None);
assert!(!config.kms_allow_insecure_dev_defaults);
assert!(!config.buffer_profile_disable);
assert_eq!(config.buffer_profile, "GeneralPurpose");
}
+4
View File
@@ -44,10 +44,12 @@ pub struct Opt {
pub kms_enable: bool,
pub kms_backend: String,
pub kms_key_dir: Option<String>,
pub kms_local_master_key: Option<String>,
pub kms_vault_address: Option<String>,
pub kms_vault_token: Option<String>,
pub kms_vault_mount_path: Option<String>,
pub kms_default_key_id: Option<String>,
pub kms_allow_insecure_dev_defaults: bool,
pub buffer_profile_disable: bool,
pub buffer_profile: String,
}
@@ -72,10 +74,12 @@ impl Opt {
kms_enable: o.kms_enable,
kms_backend: o.kms_backend,
kms_key_dir: o.kms_key_dir,
kms_local_master_key: o.kms_local_master_key,
kms_vault_address: o.kms_vault_address,
kms_vault_token: o.kms_vault_token,
kms_vault_mount_path: o.kms_vault_mount_path,
kms_default_key_id: o.kms_default_key_id,
kms_allow_insecure_dev_defaults: o.kms_allow_insecure_dev_defaults,
buffer_profile_disable: o.buffer_profile_disable,
buffer_profile: o.buffer_profile,
}
+4 -1
View File
@@ -174,9 +174,10 @@ fn build_local_kms_config(cfg: &config::Config) -> std::io::Result<rustfs_kms::c
backend: rustfs_kms::config::KmsBackend::Local,
backend_config: rustfs_kms::config::BackendConfig::Local(rustfs_kms::config::LocalConfig {
key_dir: std::path::PathBuf::from(key_dir),
master_key: None,
master_key: cfg.kms_local_master_key.clone(),
file_permissions: Some(0o600),
}),
allow_insecure_dev_defaults: cfg.kms_allow_insecure_dev_defaults,
default_key_id: cfg.kms_default_key_id.clone(),
timeout: std::time::Duration::from_secs(30),
retry_attempts: 3,
@@ -209,6 +210,7 @@ fn build_vault_kms_config(cfg: &config::Config) -> std::io::Result<rustfs_kms::c
key_path_prefix: "rustfs/kms/keys".to_string(),
tls: None,
})),
allow_insecure_dev_defaults: cfg.kms_allow_insecure_dev_defaults,
default_key_id: cfg.kms_default_key_id.clone(),
timeout: std::time::Duration::from_secs(30),
retry_attempts: 3,
@@ -239,6 +241,7 @@ fn build_vault_transit_kms_config(cfg: &config::Config) -> std::io::Result<rustf
mount_path: cfg.kms_vault_mount_path.clone().unwrap_or_else(|| "transit".to_string()),
tls: None,
})),
allow_insecure_dev_defaults: cfg.kms_allow_insecure_dev_defaults,
default_key_id: cfg.kms_default_key_id.clone(),
timeout: std::time::Duration::from_secs(30),
retry_attempts: 3,
+3 -3
View File
@@ -2814,7 +2814,7 @@ mod tests {
let manager = rustfs_kms::init_global_kms_service_manager();
let temp_dir = TempDir::new().expect("temp dir");
manager
.reconfigure(KmsConfig::local(temp_dir.path().to_path_buf()))
.reconfigure(KmsConfig::local(temp_dir.path().to_path_buf()).with_insecure_development_defaults())
.await
.expect("kms reconfigure should succeed");
manager
@@ -3429,7 +3429,7 @@ mod tests {
let first_dir = TempDir::new().expect("first temp dir");
manager
.reconfigure(KmsConfig::local(first_dir.path().to_path_buf()))
.reconfigure(KmsConfig::local(first_dir.path().to_path_buf()).with_insecure_development_defaults())
.await
.expect("first KMS reconfigure should succeed");
manager
@@ -3456,7 +3456,7 @@ mod tests {
let second_dir = TempDir::new().expect("second temp dir");
manager
.reconfigure(KmsConfig::local(second_dir.path().to_path_buf()))
.reconfigure(KmsConfig::local(second_dir.path().to_path_buf()).with_insecure_development_defaults())
.await
.expect("second KMS reconfigure should succeed");
manager
+2 -2
View File
@@ -38,7 +38,7 @@ use rustfs_ecstore::disk::RUSTFS_META_BUCKET;
use rustfs_ecstore::error::StorageError;
use rustfs_ecstore::{
set_disk::get_lock_acquire_timeout,
store_api::{HTTPPreconditions, ObjectOptions, PutObjReader, StorageAPI},
store_api::{HTTPPreconditions, NamespaceLocking, ObjectOptions, PutObjReader, StorageAPI},
};
use serde::{Deserialize, Serialize, de::DeserializeOwned};
use time::{Duration, OffsetDateTime};
@@ -1459,7 +1459,7 @@ pub(crate) type EcStoreTableCatalogStore<S> = ObjectTableCatalogStore<EcStoreTab
#[async_trait::async_trait]
impl<S> TableCatalogObjectBackend for EcStoreTableCatalogObjectBackend<S>
where
S: StorageAPI,
S: StorageAPI + NamespaceLocking,
{
async fn read_object(&self, bucket: &str, object: &str) -> TableCatalogStoreResult<Option<TableCatalogObject>> {
self.read_object_with_options(bucket, object, ObjectOptions::default()).await