diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 6c3e4c0f..4c1105b5 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -140,6 +140,14 @@ tooling; otherwise follow the example's README. describe what well-named code already says. - **Backwards compatibility:** don't add compatibility shims for behavior the task didn't ask for. See `AGENTS.md` §3, rule 6. +- **Secrets in server memory:** every long-lived secret-bearing field in + `crates/server` must use a wrapper from `crates/server/src/secret/` + (`FixedKey`, `SecretBytes`, `SecretString`, `CredentialUrl`). Read a + secret-bearing env var and wrap it in a single expression — never bind + the raw `String` to a config field or local. Reviewers must confirm this + for any new secret-shaped field; the compile-time assertions in + `crates/server/src/secret/tests.rs` enforce that wrappers can never be + logged (`Display`), serialized (`Serialize`), or placed in a response DTO. ## Docs diff --git a/Cargo.lock b/Cargo.lock index 882658f5..70ae8a8e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -476,7 +476,7 @@ dependencies = [ "alloy-rlp", "alloy-serde", "alloy-sol-types", - "itertools 0.13.0", + "itertools 0.14.0", "serde", "serde_json", "serde_with", @@ -629,7 +629,7 @@ checksum = "2035f3c4d6bee20624da2dcf765d469b292398e48d766ffade61b0fcf8b4d45d" dependencies = [ "alloy-json-rpc", "alloy-transport", - "itertools 0.13.0", + "itertools 0.14.0", "reqwest 0.13.3", "serde_json", "tower", @@ -3053,9 +3053,12 @@ dependencies = [ "rand 0.9.2", "rand_chacha 0.9.0", "rustls 0.23.37", + "secrecy", "serde", "serde_json", "sha2 0.10.9", + "static_assertions", + "subtle", "tempfile", "tokio", "tokio-postgres", @@ -3068,7 +3071,9 @@ dependencies = [ "tower-http", "tracing", "tracing-subscriber", + "url", "uuid", + "zeroize", ] [[package]] @@ -4437,6 +4442,7 @@ dependencies = [ "rand_chacha 0.9.0", "tempfile", "thiserror 2.0.18", + "zeroize", ] [[package]] @@ -6037,7 +6043,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "343d3bd7056eda839b03204e68deff7d1b13aba7af2b2fd16890697274262ee7" dependencies = [ "heck", - "itertools 0.10.5", + "itertools 0.14.0", "log", "multimap", "petgraph 0.8.3", @@ -6058,7 +6064,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "27c6023962132f4b30eb4c172c91ce92d933da334c59c23cddee82358ddafb0b" dependencies = [ "anyhow", - "itertools 0.10.5", + "itertools 0.14.0", "proc-macro2", "quote", "syn 2.0.117", @@ -6941,6 +6947,15 @@ dependencies = [ "cc", ] +[[package]] +name = "secrecy" +version = "0.10.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e891af845473308773346dc847b2c23ee78fe442e0472ac50e22a18a93d3ae5a" +dependencies = [ + "zeroize", +] + [[package]] name = "security-framework" version = "3.7.0" diff --git a/crates/miden-keystore/Cargo.toml b/crates/miden-keystore/Cargo.toml index e1973001..1fceb749 100644 --- a/crates/miden-keystore/Cargo.toml +++ b/crates/miden-keystore/Cargo.toml @@ -8,6 +8,7 @@ miden-protocol = { workspace = true } thiserror = { workspace = true } hex = { workspace = true } rand = "0.9.2" +zeroize = "1.7" [dev-dependencies] rand_chacha = { workspace = true } diff --git a/crates/miden-keystore/src/ecdsa_keystore.rs b/crates/miden-keystore/src/ecdsa_keystore.rs index 5a0f9284..ab2b29cd 100644 --- a/crates/miden-keystore/src/ecdsa_keystore.rs +++ b/crates/miden-keystore/src/ecdsa_keystore.rs @@ -7,6 +7,7 @@ use std::hash::{DefaultHasher, Hash, Hasher}; use std::io::{BufRead, BufReader, BufWriter, Write}; use std::path::{Path, PathBuf}; use std::sync::Mutex; +use zeroize::Zeroizing; type Result = std::result::Result; @@ -62,13 +63,10 @@ impl FilesystemEcdsaKeyStore { })?; let mut writer = BufWriter::new(file); - writer - .write_all(hex::encode(key_bytes).as_bytes()) - .map_err(|error| { - KeyStoreError::StorageError(format!( - "Failed to write key to file {filename}: {error}" - )) - })?; + let encoded = Zeroizing::new(hex::encode(key_bytes)); + writer.write_all(encoded.as_bytes()).map_err(|error| { + KeyStoreError::StorageError(format!("Failed to write key to file {filename}: {error}")) + })?; writer.flush().map_err(|error| { KeyStoreError::StorageError(format!("Failed to flush key file {filename}: {error}")) @@ -89,16 +87,16 @@ impl FilesystemEcdsaKeyStore { })?; let mut reader = BufReader::new(file); - let mut hex_encoded = String::new(); + let mut hex_encoded = Zeroizing::new(String::new()); reader.read_line(&mut hex_encoded).map_err(|error| { KeyStoreError::StorageError(format!("Failed to read key from file {filename}: {error}")) })?; - let key_bytes = hex::decode(hex_encoded.trim()).map_err(|error| { + let key_bytes = Zeroizing::new(hex::decode(hex_encoded.trim()).map_err(|error| { KeyStoreError::DecodingError(format!( "Failed to decode hex key from file {filename}: {error}" )) - })?; + })?); SecretKey::read_from_bytes(&key_bytes).map_err(|error| { KeyStoreError::DecodingError(format!( diff --git a/crates/miden-keystore/src/keystore.rs b/crates/miden-keystore/src/keystore.rs index 3b4f4fb5..1a33df50 100644 --- a/crates/miden-keystore/src/keystore.rs +++ b/crates/miden-keystore/src/keystore.rs @@ -8,6 +8,7 @@ use std::io::{BufRead, BufReader, BufWriter, Write}; use std::path::{Path, PathBuf}; use std::sync::{Arc, RwLock}; use thiserror::Error; +use zeroize::Zeroizing; #[derive(Debug, Error)] pub enum KeyStoreError { @@ -66,13 +67,10 @@ impl FilesystemKeyStore { })?; let mut writer = BufWriter::new(file); - writer - .write_all(hex::encode(key_bytes).as_bytes()) - .map_err(|error| { - KeyStoreError::StorageError(format!( - "Failed to write key to file {filename}: {error}" - )) - })?; + let encoded = Zeroizing::new(hex::encode(key_bytes)); + writer.write_all(encoded.as_bytes()).map_err(|error| { + KeyStoreError::StorageError(format!("Failed to write key to file {filename}: {error}")) + })?; writer.flush().map_err(|error| { KeyStoreError::StorageError(format!("Failed to flush key file {filename}: {error}")) @@ -93,16 +91,16 @@ impl FilesystemKeyStore { })?; let mut reader = BufReader::new(file); - let mut hex_encoded = String::new(); + let mut hex_encoded = Zeroizing::new(String::new()); reader.read_line(&mut hex_encoded).map_err(|error| { KeyStoreError::StorageError(format!("Failed to read key from file {filename}: {error}")) })?; - let key_bytes = hex::decode(hex_encoded.trim()).map_err(|error| { + let key_bytes = Zeroizing::new(hex::decode(hex_encoded.trim()).map_err(|error| { KeyStoreError::DecodingError(format!( "Failed to decode hex key from file {filename}: {error}" )) - })?; + })?); SecretKey::read_from_bytes(&key_bytes).map_err(|error| { KeyStoreError::DecodingError(format!( diff --git a/crates/server/Cargo.toml b/crates/server/Cargo.toml index 218269fd..086d2cd1 100644 --- a/crates/server/Cargo.toml +++ b/crates/server/Cargo.toml @@ -25,7 +25,11 @@ hmac = "0.12" sha2 = "0.10" rand = "0.9" rand_chacha = { workspace = true } +secrecy = { version = "0.10", default-features = false } serde = { workspace = true } +subtle = "2.5" +url = "2.5" +zeroize = { version = "1.7", features = ["derive"] } serde_json = { workspace = true } tokio = { workspace = true, features = ["full"] } tonic = { workspace = true } @@ -63,6 +67,7 @@ tonic-prost-build = { workspace = true } [dev-dependencies] futures = "0.3" +static_assertions = "1.1" tempfile = { workspace = true } uuid = { version = "1.0", features = ["v4"] } miden-standards = { workspace = true } diff --git a/crates/server/src/ack/miden_ecdsa/signer.rs b/crates/server/src/ack/miden_ecdsa/signer.rs index 902f92ef..2ea2affa 100644 --- a/crates/server/src/ack/miden_ecdsa/signer.rs +++ b/crates/server/src/ack/miden_ecdsa/signer.rs @@ -48,6 +48,13 @@ impl MidenEcdsaSigner { impl MidenEcdsaSigner { pub(crate) fn sign_with_server_key(&self, message: Word) -> crate::ack::Result { + // Verified: miden-crypto 0.23.0 (pulled in by miden-protocol 0.14.5) + // implements `impl ZeroizeOnDrop for SecretKey {}` in + // src/dsa/ecdsa_k256_keccak/mod.rs (delegating to k256's + // ZeroizeOnDrop), so the per-call loaded key material is zeroed + // when the signing frame returns. The local file-read buffer inside + // miden-keystore is wrapped in zeroize::Zeroizing for the same + // reason. Ok(self.keystore.ecdsa_sign(self.server_pubkey_word, message)?) } diff --git a/crates/server/src/ack/miden_falcon_rpo/signer.rs b/crates/server/src/ack/miden_falcon_rpo/signer.rs index 84fa5207..f7f44435 100644 --- a/crates/server/src/ack/miden_falcon_rpo/signer.rs +++ b/crates/server/src/ack/miden_falcon_rpo/signer.rs @@ -49,6 +49,13 @@ impl MidenFalconRpoSigner { impl MidenFalconRpoSigner { pub(crate) fn sign_with_server_key(&self, message: Word) -> crate::ack::Result { + // Verified: miden-crypto 0.23.0 (pulled in by miden-protocol 0.14.5) + // implements `impl ZeroizeOnDrop for SecretKey {}` in + // src/dsa/falcon512_poseidon2/keys/secret_key.rs (the SecretKey type + // here), so the per-call loaded key material is zeroed when the + // signing frame returns. The local file-read buffer inside + // miden-keystore is wrapped in zeroize::Zeroizing for the same + // reason. Ok(self.keystore.sign(self.server_pubkey_word, message)?) } diff --git a/crates/server/src/ack/secrets_manager.rs b/crates/server/src/ack/secrets_manager.rs index 0d403f2e..6dc59170 100644 --- a/crates/server/src/ack/secrets_manager.rs +++ b/crates/server/src/ack/secrets_manager.rs @@ -1,4 +1,5 @@ use crate::error::{GuardianError, Result}; +use crate::secret::{SecretBytes, SecretString}; use async_trait::async_trait; use aws_config::BehaviorVersion; use aws_sdk_secretsmanager::Client; @@ -39,7 +40,7 @@ impl AwsSecretsManagerProvider { }) } - async fn secret_string(&self, secret_id: &str) -> Result { + async fn secret_string(&self, secret_id: &str) -> Result { let response = self .client .get_secret_value() @@ -52,11 +53,14 @@ impl AwsSecretsManagerProvider { )) })?; - response.secret_string().map(str::to_owned).ok_or_else(|| { - GuardianError::ConfigurationError(format!( - "Secret {secret_id} does not contain a secret string value" - )) - }) + response + .secret_string() + .map(|s| SecretString::new(s.to_owned())) + .ok_or_else(|| { + GuardianError::ConfigurationError(format!( + "Secret {secret_id} does not contain a secret string value" + )) + }) } async fn parsed_secret_key(&self, secret_id: &str, parser: F) -> Result @@ -64,13 +68,15 @@ impl AwsSecretsManagerProvider { F: FnOnce(&[u8]) -> std::result::Result, { let secret_hex = self.secret_string(secret_id).await?; - let secret_bytes = hex::decode(secret_hex.trim()).map_err(|error| { - GuardianError::ConfigurationError(format!( - "Secret {secret_id} must contain valid hex-encoded key bytes: {error}" - )) - })?; + let secret_bytes = SecretBytes::new( + hex::decode(secret_hex.expose_secret().trim()).map_err(|error| { + GuardianError::ConfigurationError(format!( + "Secret {secret_id} must contain valid hex-encoded key bytes: {error}" + )) + })?, + ); - parser(&secret_bytes).map_err(|error| { + parser(secret_bytes.expose_secret()).map_err(|error| { GuardianError::ConfigurationError(format!( "Secret {secret_id} does not contain a valid key: {error}" )) diff --git a/crates/server/src/audit/postgres.rs b/crates/server/src/audit/postgres.rs index 18612cd0..63b169fa 100644 --- a/crates/server/src/audit/postgres.rs +++ b/crates/server/src/audit/postgres.rs @@ -261,12 +261,15 @@ mod tests { use diesel::QueryDsl; use diesel_async::RunQueryDsl; - let database_url = std::env::var("DATABASE_URL") - .expect("DATABASE_URL must be set; this test is marked #[ignore] by default"); - crate::storage::postgres::run_migrations(&database_url) + let database_url = crate::secret::CredentialUrl::new( + std::env::var("DATABASE_URL") + .expect("DATABASE_URL must be set; this test is marked #[ignore] by default"), + ); + let raw_url = database_url.expose_secret(); + crate::storage::postgres::run_migrations(raw_url) .await .expect("migrations run"); - let pool = build_postgres_pool_lazy(&database_url, 1).expect("pool builds"); + let pool = build_postgres_pool_lazy(raw_url, 1).expect("pool builds"); // Sanity: prove the pool is reachable; if not, abort early // with a clear error rather than a confusing trigger failure. let _ = pool diff --git a/crates/server/src/builder/storage.rs b/crates/server/src/builder/storage.rs index 1ca5b4db..6970f812 100644 --- a/crates/server/src/builder/storage.rs +++ b/crates/server/src/builder/storage.rs @@ -11,6 +11,7 @@ use crate::metadata::MetadataStore; use crate::metadata::filesystem::FilesystemMetadataStore; #[cfg(feature = "postgres")] use crate::metadata::postgres::PostgresMetadataStore; +use crate::secret::CredentialUrl; use crate::storage::StorageBackend; #[cfg(not(feature = "postgres"))] use crate::storage::filesystem::FilesystemService; @@ -29,7 +30,7 @@ const ENV_METADATA_DB_POOL_MAX_SIZE: &str = "GUARDIAN_METADATA_DB_POOL_MAX_SIZE" pub struct StorageMetadataBuilder { storage_path: Option, metadata_path: Option, - database_url: Option, + database_url: Option, database_pool_max_size: Option, metadata_pool_max_size: Option, } @@ -49,8 +50,8 @@ impl StorageMetadataBuilder { self } - pub fn database_url(mut self, url: String) -> Self { - self.database_url = Some(url); + pub(crate) fn database_url(mut self, url: Option) -> Self { + self.database_url = url; self } @@ -76,7 +77,12 @@ impl StorageMetadataBuilder { .unwrap_or_else(|_| "/var/guardian/metadata".to_string()) .into(), ) - .database_url(std::env::var("DATABASE_URL").ok().unwrap_or_default()) + .database_url( + std::env::var("DATABASE_URL") + .ok() + .filter(|s| !s.is_empty()) + .map(CredentialUrl::new), + ) } pub async fn build( @@ -93,7 +99,7 @@ impl StorageMetadataBuilder { { let database_url = self .database_url - .filter(|url| !url.is_empty()) + .filter(|url| !url.expose_secret().is_empty()) .ok_or_else(|| "DATABASE_URL environment variable is required".to_string())?; let database_pool_max_size = resolve_pool_size( self.database_pool_max_size, @@ -106,10 +112,10 @@ impl StorageMetadataBuilder { database_pool_max_size, )?; - postgres::run_migrations(&database_url).await?; - let storage = PostgresService::new(&database_url, database_pool_max_size).await?; - let metadata = - PostgresMetadataStore::new(&database_url, metadata_pool_max_size).await?; + let raw_url = database_url.expose_secret(); + postgres::run_migrations(raw_url).await?; + let storage = PostgresService::new(raw_url, database_pool_max_size).await?; + let metadata = PostgresMetadataStore::new(raw_url, metadata_pool_max_size).await?; let auditor: SharedAuditor = Arc::new(PostgresAuditor::new(metadata.pool_handle())); Ok((Arc::new(storage), Arc::new(metadata), auditor)) @@ -214,8 +220,12 @@ mod tests { #[test] fn test_database_url_sets_url() { let url = "postgres://localhost/test".to_string(); - let builder = StorageMetadataBuilder::new().database_url(url.clone()); - assert_eq!(builder.database_url, Some(url)); + let builder = + StorageMetadataBuilder::new().database_url(Some(CredentialUrl::new(url.clone()))); + assert_eq!( + builder.database_url.as_ref().map(|u| u.expose_secret()), + Some(url.as_str()) + ); } #[test] @@ -239,26 +249,37 @@ mod tests { let builder = StorageMetadataBuilder::new() .storage_path(storage_path.clone()) .metadata_path(metadata_path.clone()) - .database_url(database_url.clone()) + .database_url(Some(CredentialUrl::new(database_url.clone()))) .database_pool_max_size(24) .metadata_pool_max_size(12); assert_eq!(builder.storage_path, Some(storage_path)); assert_eq!(builder.metadata_path, Some(metadata_path)); - assert_eq!(builder.database_url, Some(database_url)); + assert_eq!( + builder.database_url.as_ref().map(|u| u.expose_secret()), + Some(database_url.as_str()) + ); assert_eq!(builder.database_pool_max_size, Some(24)); assert_eq!(builder.metadata_pool_max_size, Some(12)); } #[test] fn test_from_env_returns_builder_with_paths() { - // Test that from_env returns a builder with storage_path and metadata_path set - // We can't reliably test specific values due to env var state from other tests + // Test that from_env returns a builder with storage_path and metadata_path set. + // database_url is intentionally None when DATABASE_URL is unset or empty — + // the previous "always Some(empty string)" behavior masked a real-absent value. let builder = StorageMetadataBuilder::from_env(); assert!(builder.storage_path.is_some()); assert!(builder.metadata_path.is_some()); - assert!(builder.database_url.is_some()); + match std::env::var("DATABASE_URL") { + Ok(value) if !value.is_empty() => { + assert!(builder.database_url.is_some()); + } + _ => { + assert!(builder.database_url.is_none()); + } + } } #[cfg(not(feature = "postgres"))] @@ -387,7 +408,8 @@ mod tests { #[cfg(feature = "postgres")] #[tokio::test] async fn test_build_with_empty_database_url_fails() { - let builder = StorageMetadataBuilder::new().database_url(String::new()); + let builder = + StorageMetadataBuilder::new().database_url(Some(CredentialUrl::new(String::new()))); let result = builder.build().await; assert!(result.is_err()); diff --git a/crates/server/src/dashboard/config.rs b/crates/server/src/dashboard/config.rs index 41bd7e19..1b21ebc2 100644 --- a/crates/server/src/dashboard/config.rs +++ b/crates/server/src/dashboard/config.rs @@ -1,5 +1,7 @@ use chrono::Duration; +use zeroize::Zeroizing; +use crate::dashboard::cursor::CursorSecret; use crate::middleware::RateLimitConfig; use crate::network::NetworkType; @@ -29,25 +31,33 @@ pub struct DashboardConfig { pub(crate) commitment_rate_limit: RateLimitConfig, pub(crate) filesystem_aggregate_threshold: usize, pub(crate) environment: String, - /// Optional 32-byte hex-encoded HMAC secret for the dashboard - /// cursor codec. When `None`, [`DashboardState`] generates a fresh - /// random secret per process — fine for single-replica - /// deployments and unit tests; multi-replica deployments must - /// pin a shared secret here so cursors validate across replicas. - /// Sourced from `GUARDIAN_DASHBOARD_CURSOR_SECRET`. - pub(crate) cursor_secret_hex: Option, + /// Optional pre-parsed HMAC secret for the dashboard cursor codec. + /// When `None`, [`DashboardState`] generates a fresh random secret + /// per process — fine for single-replica deployments and unit + /// tests; multi-replica deployments must pin a shared secret here + /// so cursors validate across replicas. Sourced from + /// `GUARDIAN_DASHBOARD_CURSOR_SECRET` (parsed at config-load time + /// so no intermediate `String` lives in the config). + pub(crate) cursor_secret: Option, } impl DashboardConfig { pub fn from_env_for_network(network_type: NetworkType) -> std::result::Result { - let mut config = Self { + let cursor_secret = std::env::var("GUARDIAN_DASHBOARD_CURSOR_SECRET") + .ok() + .map(Zeroizing::new) + .map(|hex| CursorSecret::from_hex(hex.as_str())) + .transpose() + .map_err(|e| { + format!( + "GUARDIAN_DASHBOARD_CURSOR_SECRET must be 32 hex-encoded bytes (64 chars): {e}" + ) + })?; + Ok(Self { environment: environment_for_network(network_type).to_string(), + cursor_secret, ..Self::default() - }; - if let Ok(secret_hex) = std::env::var("GUARDIAN_DASHBOARD_CURSOR_SECRET") { - config.cursor_secret_hex = Some(secret_hex); - } - Ok(config) + }) } pub fn for_tests() -> Self { @@ -62,8 +72,8 @@ impl DashboardConfig { &self.environment } - pub(crate) fn cursor_secret_hex(&self) -> Option<&str> { - self.cursor_secret_hex.as_deref() + pub(crate) fn take_cursor_secret(&mut self) -> Option { + self.cursor_secret.take() } } @@ -90,15 +100,59 @@ impl Default for DashboardConfig { }, filesystem_aggregate_threshold: DEFAULT_FILESYSTEM_AGGREGATE_THRESHOLD, environment: DEFAULT_ENVIRONMENT.to_string(), - cursor_secret_hex: None, + cursor_secret: None, } } } #[cfg(all(test, not(any(feature = "integration", feature = "e2e"))))] mod tests { + use std::sync::{LazyLock, Mutex}; + use super::*; + static ENV_LOCK: LazyLock> = LazyLock::new(|| Mutex::new(())); + + struct EnvVarGuard { + key: &'static str, + previous: Option, + _lock: std::sync::MutexGuard<'static, ()>, + } + + impl EnvVarGuard { + // secret-fields-allow: test-only env mutation guarded by ENV_LOCK + fn set(key: &'static str, value: &str) -> Self { + let lock = ENV_LOCK.lock().unwrap_or_else(|poison| poison.into_inner()); + let previous = std::env::var(key).ok(); + unsafe { std::env::set_var(key, value) }; + Self { + key, + previous, + _lock: lock, + } + } + + fn remove(key: &'static str) -> Self { + let lock = ENV_LOCK.lock().unwrap_or_else(|poison| poison.into_inner()); + let previous = std::env::var(key).ok(); + unsafe { std::env::remove_var(key) }; + Self { + key, + previous, + _lock: lock, + } + } + } + + impl Drop for EnvVarGuard { + fn drop(&mut self) { + match &self.previous { + Some(value) => unsafe { std::env::set_var(self.key, value) }, + None => unsafe { std::env::remove_var(self.key) }, + } + } + } + #[test] fn default_filesystem_aggregate_threshold_is_1000() { let config = DashboardConfig::default(); @@ -123,8 +177,35 @@ mod tests { ); } + #[test] + fn from_env_parses_valid_cursor_secret_hex() { + let _guard = EnvVarGuard::set( + "GUARDIAN_DASHBOARD_CURSOR_SECRET", + "00112233445566778899aabbccddeeff00112233445566778899aabbccddeeff", + ); + let mut config = + DashboardConfig::from_env_for_network(NetworkType::MidenTestnet).expect("parses"); + assert!(config.take_cursor_secret().is_some()); + assert!( + config.take_cursor_secret().is_none(), + "take_cursor_secret must be one-shot" + ); + } + + #[test] + fn from_env_rejects_invalid_cursor_secret_hex() { + let _guard = EnvVarGuard::set("GUARDIAN_DASHBOARD_CURSOR_SECRET", "not-hex"); + let err = DashboardConfig::from_env_for_network(NetworkType::MidenTestnet) + .expect_err("invalid hex must error"); + assert!( + err.contains("GUARDIAN_DASHBOARD_CURSOR_SECRET"), + "error must name the env var: {err}" + ); + } + #[test] fn environment_is_derived_from_network_type() { + let _cursor = EnvVarGuard::remove("GUARDIAN_DASHBOARD_CURSOR_SECRET"); assert_eq!( DashboardConfig::from_env_for_network(NetworkType::MidenTestnet) .unwrap() diff --git a/crates/server/src/dashboard/cursor.rs b/crates/server/src/dashboard/cursor.rs index c70d5ea8..583e0666 100644 --- a/crates/server/src/dashboard/cursor.rs +++ b/crates/server/src/dashboard/cursor.rs @@ -63,8 +63,10 @@ use chrono::{DateTime, Utc}; use hmac::{Hmac, Mac}; use serde::{Deserialize, Serialize}; use sha2::Sha256; +use zeroize::Zeroizing; use crate::error::{GuardianError, Result}; +use crate::secret::FixedKey; type HmacSha256 = Hmac; @@ -191,9 +193,9 @@ impl Cursor { /// Random server-side secret used to HMAC-sign cursors. Generated once /// per server startup; cursors do not survive a restart. -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct CursorSecret { - bytes: [u8; CURSOR_SECRET_LEN], + key: FixedKey, } impl CursorSecret { @@ -203,14 +205,18 @@ impl CursorSecret { use rand::RngCore; let mut bytes = [0u8; CURSOR_SECRET_LEN]; rand::rng().fill_bytes(&mut bytes); - Self { bytes } + Self { + key: FixedKey::new(bytes), + } } /// Construct a `CursorSecret` from a fixed byte slice. Mainly useful /// in tests where a deterministic secret keeps assertions stable. #[cfg(test)] pub fn from_bytes(bytes: [u8; CURSOR_SECRET_LEN]) -> Self { - Self { bytes } + Self { + key: FixedKey::new(bytes), + } } /// Decode a hex-encoded `CursorSecret` (exactly @@ -219,29 +225,25 @@ impl CursorSecret { /// validate across multi-replica deployments. pub fn from_hex(hex_str: &str) -> std::result::Result { let trimmed = hex_str.trim(); - let raw = hex::decode(trimmed.strip_prefix("0x").unwrap_or(trimmed)) - .map_err(|e| format!("invalid hex: {e}"))?; + let raw = Zeroizing::new( + hex::decode(trimmed.strip_prefix("0x").unwrap_or(trimmed)) + .map_err(|e| format!("invalid hex: {e}"))?, + ); if raw.len() != CURSOR_SECRET_LEN { return Err(format!( "expected {CURSOR_SECRET_LEN} bytes, got {}", raw.len() )); } - let mut bytes = [0u8; CURSOR_SECRET_LEN]; + let mut bytes = Zeroizing::new([0u8; CURSOR_SECRET_LEN]); bytes.copy_from_slice(&raw); - Ok(Self { bytes }) + Ok(Self { + key: FixedKey::new(*bytes), + }) } fn as_slice(&self) -> &[u8] { - &self.bytes - } -} - -impl std::fmt::Debug for CursorSecret { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.debug_struct("CursorSecret") - .field("bytes", &"") - .finish() + self.key.expose_secret() } } @@ -300,6 +302,8 @@ pub fn decode(encoded: &str, secret: &CursorSecret, expected_kind: CursorKind) - GuardianError::ConfigurationError(format!("Failed to initialize HMAC: {e}")) })?; mac.update(payload); + // hmac::Mac::verify_slice is constant-time per RustCrypto, so a network + // attacker cannot use response latency to learn the tag one byte at a time. mac.verify_slice(tag) .map_err(|_| GuardianError::InvalidCursor("cursor signature mismatch".to_string()))?; diff --git a/crates/server/src/dashboard/state.rs b/crates/server/src/dashboard/state.rs index d74cdb98..c973f41c 100644 --- a/crates/server/src/dashboard/state.rs +++ b/crates/server/src/dashboard/state.rs @@ -4,6 +4,7 @@ use std::sync::Arc; use chrono::{DateTime, Utc}; use guardian_shared::hex::{FromHex, IntoHex}; use miden_protocol::crypto::dsa::falcon512_poseidon2::Signature; +use sha2::{Digest, Sha256}; use tokio::sync::{Mutex, RwLock}; use super::allowlist::{ @@ -20,13 +21,20 @@ use crate::error::{GuardianError, Result}; use crate::middleware::rate_limit::RateLimitStore; use crate::network::NetworkType; +/// Derive the storage key for a session token. The plaintext token is never +/// retained in the session map — only this SHA-256 digest is — so a +/// coredump or heap inspection of the long-lived map yields no tokens. +fn session_digest(token: &str) -> [u8; 32] { + Sha256::digest(token.as_bytes()).into() +} + #[derive(Clone, Debug)] pub struct DashboardState { config: DashboardConfig, allowlist_source: AllowlistSource, allowlist: Arc>, challenges: Arc>>>, - sessions: Arc>>, + sessions: Arc>>, commitment_rate_limits: RateLimitStore, cursor_secret: CursorSecret, started_at: DateTime, @@ -258,11 +266,12 @@ impl DashboardState { let operator_identity = operator.clone(); let token = random_hex::<32>(); let cookie_header = self.session_cookie_header(&token, issued_at, expires_at); + let session_key = session_digest(&token); let mut sessions = self.sessions.lock().await; sessions.retain(|_, session| session.expires_at > now); sessions.insert( - token, + session_key, OperatorSessionRecord { operator: operator_identity.clone(), issued_at, @@ -293,7 +302,8 @@ impl DashboardState { let mut sessions = self.sessions.lock().await; sessions.retain(|_, session| session.expires_at > now); - let session = sessions.get(token).cloned().ok_or_else(|| { + let session_key = session_digest(token); + let session = sessions.get(&session_key).cloned().ok_or_else(|| { tracing::warn!( auth_event = "session_rejected", reason = "missing_or_expired", @@ -312,7 +322,7 @@ impl DashboardState { .lookup_allowlisted_operator(&session.operator.commitment) .await else { - sessions.remove(token); + sessions.remove(&session_key); tracing::warn!( auth_event = "session_rejected", operator_id = %session.operator.operator_id, @@ -331,7 +341,7 @@ impl DashboardState { let mut sessions = self.sessions.lock().await; sessions.retain(|_, session| session.expires_at > now); if let Some(token) = token - && let Some(session) = sessions.remove(token) + && let Some(session) = sessions.remove(&session_digest(token)) { tracing::info!( auth_event = "logout", @@ -345,30 +355,23 @@ impl DashboardState { fn from_allowlist_source( allowlist_source: AllowlistSource, allowlist: OperatorAllowlist, - config: DashboardConfig, + mut config: DashboardConfig, ) -> std::result::Result { tracing::info!( auth_event = "allowlist_loaded", operator_count = allowlist.len(), "Operator allowlist loaded" ); - let cursor_secret = match config.cursor_secret_hex() { - Some(hex) => CursorSecret::from_hex(hex).map_err(|e| { - format!( - "GUARDIAN_DASHBOARD_CURSOR_SECRET must be 32 hex-encoded bytes (64 chars): {e}" - ) - })?, - None => { - if !cfg!(test) { - tracing::warn!( - "dashboard cursor secret not configured; generating ephemeral per-process \ - secret. Multi-replica deployments must set \ - GUARDIAN_DASHBOARD_CURSOR_SECRET to a stable shared 32-byte hex value." - ); - } - CursorSecret::generate() + let cursor_secret = config.take_cursor_secret().unwrap_or_else(|| { + if !cfg!(test) { + tracing::warn!( + "dashboard cursor secret not configured; generating ephemeral per-process \ + secret. Multi-replica deployments must set \ + GUARDIAN_DASHBOARD_CURSOR_SECRET to a stable shared 32-byte hex value." + ); } - }; + CursorSecret::generate() + }); Ok(Self { commitment_rate_limits: RateLimitStore::new(config.commitment_rate_limit.clone()), config, @@ -509,6 +512,21 @@ mod tests { static ENV_LOCK: LazyLock> = LazyLock::new(|| TokioMutex::new(())); + /// Pull the bare token out of a `Set-Cookie` header of the form + /// `name=token; HttpOnly; ...`. Used by tests that need to replay a + /// session token after the digest-keyed storage shape stopped exposing + /// raw tokens in the session map. + fn parse_token_from_cookie(cookie_header: &str) -> String { + let value = cookie_header + .split(';') + .next() + .expect("non-empty cookie header"); + value + .split_once('=') + .map(|(_, token)| token.to_string()) + .expect("cookie has name=value") + } + struct EnvVarGuard { key: &'static str, previous: Option, @@ -649,19 +667,11 @@ mod tests { .expect("first challenge should succeed"); let signature_one = operator_one .sign_word(Word::from_hex(&challenge_one.signing_digest).expect("digest should parse")); - state + let session_one = state .verify(&operator_one.commitment_hex, &signature_one, now) .await .expect("first verify should succeed"); - - let original_token = { - let sessions = state.sessions.lock().await; - sessions - .keys() - .next() - .expect("a session token should exist") - .clone() - }; + let original_token = parse_token_from_cookie(&session_one.cookie_header); write_operator_public_keys_file(&path, &[&operator_two.pubkey_hex]); @@ -742,4 +752,62 @@ mod tests { fs::remove_file(path).ok(); } + + #[tokio::test] + async fn session_lookup_round_trips_via_digest() { + let operator = TestSigner::new(); + let state = DashboardState::for_tests(vec![( + "operator-1".to_string(), + operator.commitment_hex.clone(), + )]); + let now = Utc::now(); + let challenge = state + .issue_challenge(&operator.commitment_hex, now) + .await + .expect("challenge"); + let signature = + operator.sign_word(Word::from_hex(&challenge.signing_digest).expect("digest")); + let session = state + .verify(&operator.commitment_hex, &signature, now) + .await + .expect("verify"); + let token = parse_token_from_cookie(&session.cookie_header); + + state + .authenticate_session(&token, now) + .await + .expect("session lookup succeeds with original token"); + + let mismatched = format!("{token}deadbeef"); + assert!(state.authenticate_session(&mismatched, now).await.is_err()); + } + + #[tokio::test] + async fn session_map_never_holds_plaintext_token() { + let operator = TestSigner::new(); + let state = DashboardState::for_tests(vec![( + "operator-1".to_string(), + operator.commitment_hex.clone(), + )]); + let now = Utc::now(); + let challenge = state + .issue_challenge(&operator.commitment_hex, now) + .await + .expect("challenge"); + let signature = + operator.sign_word(Word::from_hex(&challenge.signing_digest).expect("digest")); + let session = state + .verify(&operator.commitment_hex, &signature, now) + .await + .expect("verify"); + let token = parse_token_from_cookie(&session.cookie_header); + + let sessions = state.sessions.lock().await; + let only_key = sessions.keys().next().expect("session exists"); + assert_ne!( + only_key.as_slice(), + token.as_bytes(), + "map key must be a digest, not the plaintext token" + ); + } } diff --git a/crates/server/src/evm/config.rs b/crates/server/src/evm/config.rs index 8fdb97ce..31b5c538 100644 --- a/crates/server/src/evm/config.rs +++ b/crates/server/src/evm/config.rs @@ -1,6 +1,7 @@ use std::collections::BTreeMap; use crate::metadata::network::normalize_evm_address; +use crate::secret::{CredentialUrl, SecretString}; const RPC_URLS_ENV: &str = "GUARDIAN_EVM_RPC_URLS"; const ENTRYPOINT_ADDRESS_ENV: &str = "GUARDIAN_EVM_ENTRYPOINT_ADDRESS"; @@ -9,7 +10,7 @@ const DEFAULT_ENTRYPOINT_ADDRESS: &str = "0x433709009b8330fda32311df1c2afa402ed8 #[derive(Clone, Debug, PartialEq, Eq)] pub struct EvmChainConfig { pub chain_id: u64, - pub rpc_url: String, + pub(crate) rpc_url: CredentialUrl, pub entrypoint_address: String, } @@ -28,7 +29,7 @@ impl EvmChainRegistry { } fn from_rpc_urls( - rpc_urls: BTreeMap, + rpc_urls: BTreeMap, entrypoint_address: &str, ) -> Result { let entrypoint_address = normalize_evm_address(entrypoint_address) @@ -36,7 +37,7 @@ impl EvmChainRegistry { let mut chains = BTreeMap::new(); for (chain_id, rpc_url) in rpc_urls { - if !is_http_url(&rpc_url) { + if !is_http_url(rpc_url.expose_secret()) { return Err(format!( "{RPC_URLS_ENV} entry for chain {chain_id} must be an http or https URL" )); @@ -73,13 +74,15 @@ impl EvmChainRegistry { } } -fn parse_map(env_var: &str) -> Result, String> { +fn parse_map(env_var: &str) -> Result, String> { let Ok(raw) = std::env::var(env_var) else { return Ok(BTreeMap::new()); }; + let raw = SecretString::new(raw); let mut values = BTreeMap::new(); for entry in raw + .expose_secret() .split(',') .map(str::trim) .filter(|entry| !entry.is_empty()) @@ -94,7 +97,7 @@ fn parse_map(env_var: &str) -> Result, String> { if chain_id == 0 { return Err(format!("{env_var} chain ID must be greater than zero")); } - values.insert(chain_id, value.trim().to_string()); + values.insert(chain_id, CredentialUrl::new(value.trim().to_string())); } Ok(values) @@ -108,11 +111,15 @@ fn is_http_url(value: &str) -> bool { mod tests { use super::*; + fn url(s: &str) -> CredentialUrl { + CredentialUrl::new(s.to_string()) + } + #[test] fn registry_indexes_configured_rpc_chains() { let registry = EvmChainRegistry::new(vec![EvmChainConfig { chain_id: 31337, - rpc_url: "http://127.0.0.1:8545".to_string(), + rpc_url: url("http://127.0.0.1:8545"), entrypoint_address: DEFAULT_ENTRYPOINT_ADDRESS.to_string(), }]); @@ -124,11 +131,8 @@ mod tests { fn registry_uses_one_entrypoint_address_for_all_rpc_chains() { let registry = EvmChainRegistry::from_rpc_urls( BTreeMap::from([ - (1, "https://ethereum-rpc.publicnode.com".to_string()), - ( - 11155111, - "https://ethereum-sepolia-rpc.publicnode.com".to_string(), - ), + (1, url("https://ethereum-rpc.publicnode.com")), + (11155111, url("https://ethereum-sepolia-rpc.publicnode.com")), ]), DEFAULT_ENTRYPOINT_ADDRESS, ) @@ -148,7 +152,7 @@ mod tests { #[test] fn registry_rejects_invalid_entrypoint_address() { let error = EvmChainRegistry::from_rpc_urls( - BTreeMap::from([(31337, "http://127.0.0.1:8545".to_string())]), + BTreeMap::from([(31337, url("http://127.0.0.1:8545"))]), "0x1234", ) .expect_err("invalid EntryPoint address should fail"); @@ -159,7 +163,7 @@ mod tests { #[test] fn registry_rejects_non_http_rpc_url() { let error = EvmChainRegistry::from_rpc_urls( - BTreeMap::from([(31337, "ws://127.0.0.1:8545".to_string())]), + BTreeMap::from([(31337, url("ws://127.0.0.1:8545"))]), DEFAULT_ENTRYPOINT_ADDRESS, ) .expect_err("non-http RPC URL should fail"); diff --git a/crates/server/src/evm/contracts.rs b/crates/server/src/evm/contracts.rs index 308e9432..bb939819 100644 --- a/crates/server/src/evm/contracts.rs +++ b/crates/server/src/evm/contracts.rs @@ -148,7 +148,7 @@ impl EvmContractReader { fn provider(&self) -> Result { let url = - self.config.rpc_url.parse().map_err(|e| { + self.config.rpc_url.expose_secret().parse().map_err(|e| { GuardianError::InvalidNetworkConfig(format!("invalid RPC URL: {e}")) })?; Ok(ProviderBuilder::new().connect_http(url)) diff --git a/crates/server/src/evm/session.rs b/crates/server/src/evm/session.rs index b7616520..926056b7 100644 --- a/crates/server/src/evm/session.rs +++ b/crates/server/src/evm/session.rs @@ -3,11 +3,18 @@ use std::sync::Arc; use chrono::{DateTime, Duration, Utc}; use rand::RngCore; +use sha2::{Digest, Sha256}; use tokio::sync::Mutex; use crate::error::{GuardianError, Result}; use crate::metadata::network::normalize_evm_address; +/// SHA-256 digest of the session token used as the storage key, so the +/// plaintext token is never retained beyond the issuing request. +fn session_digest(token: &str) -> [u8; 32] { + Sha256::digest(token.as_bytes()).into() +} + const COOKIE_NAME: &str = "guardian_evm_session"; const CHALLENGE_TTL_SECS: i64 = 300; const SESSION_TTL_SECS: i64 = 8 * 60 * 60; @@ -16,7 +23,7 @@ const MAX_OUTSTANDING_CHALLENGES: usize = 8; #[derive(Clone)] pub struct EvmSessionState { challenges: Arc>>>, - sessions: Arc>>, + sessions: Arc>>, } #[derive(Clone, Debug, PartialEq, Eq)] @@ -134,10 +141,11 @@ impl EvmSessionState { let token = random_hex_32(); let expires_at = now + Duration::seconds(SESSION_TTL_SECS); let cookie_header = self.session_cookie_header(&token, expires_at); + let session_key = session_digest(&token); let mut sessions = self.sessions.lock().await; sessions.retain(|_, session| session.expires_at > now); sessions.insert( - token, + session_key, EvmSessionRecord { address: address.clone(), expires_at, @@ -158,9 +166,12 @@ impl EvmSessionState { ) -> Result { let mut sessions = self.sessions.lock().await; sessions.retain(|_, session| session.expires_at > now); - let session = sessions.get(token).cloned().ok_or_else(|| { - GuardianError::AuthenticationFailed("Invalid EVM session".to_string()) - })?; + let session = sessions + .get(&session_digest(token)) + .cloned() + .ok_or_else(|| { + GuardianError::AuthenticationFailed("Invalid EVM session".to_string()) + })?; Ok(AuthenticatedEvmSession { address: session.address, }) @@ -170,7 +181,7 @@ impl EvmSessionState { let mut sessions = self.sessions.lock().await; sessions.retain(|_, session| session.expires_at > now); if let Some(token) = token { - sessions.remove(token); + sessions.remove(&session_digest(token)); } } diff --git a/crates/server/src/lib.rs b/crates/server/src/lib.rs index f41420f5..4849edf8 100644 --- a/crates/server/src/lib.rs +++ b/crates/server/src/lib.rs @@ -22,6 +22,7 @@ pub mod evm; pub mod jobs; pub mod metadata; pub mod network; +pub(crate) mod secret; pub mod services; pub mod state_object; pub mod storage; diff --git a/crates/server/src/secret/ct.rs b/crates/server/src/secret/ct.rs new file mode 100644 index 00000000..e1bc97a9 --- /dev/null +++ b/crates/server/src/secret/ct.rs @@ -0,0 +1,9 @@ +use subtle::ConstantTimeEq; + +/// Constant-time byte-slice equality. Kept available for byte-by-byte secret +/// comparisons against untrusted input. Not for HashMap-keyed lookups (which +/// don't expose the per-byte timing oracle this defends against). +#[allow(dead_code)] +pub(crate) fn eq(a: &[u8], b: &[u8]) -> bool { + a.ct_eq(b).into() +} diff --git a/crates/server/src/secret/mod.rs b/crates/server/src/secret/mod.rs new file mode 100644 index 00000000..aac22f34 --- /dev/null +++ b/crates/server/src/secret/mod.rs @@ -0,0 +1,16 @@ +//! Memory-resident secret hygiene wrappers. +//! +//! The bytes of each wrapper are reachable only through `expose_secret()` (and +//! `CredentialUrl::scheme_and_host()` for safe logging). `Display`, `Serialize`, +//! and `Deserialize` are intentionally not implemented; `Debug` renders only a +//! non-disclosing marker. Drop zeroes the backing buffer. + +mod ct; +mod wrappers; + +#[cfg(test)] +pub(crate) use ct::eq as ct_eq; +pub(crate) use wrappers::{CredentialUrl, FixedKey, SecretBytes, SecretString}; + +#[cfg(test)] +mod tests; diff --git a/crates/server/src/secret/tests.rs b/crates/server/src/secret/tests.rs new file mode 100644 index 00000000..d4bb3b1d --- /dev/null +++ b/crates/server/src/secret/tests.rs @@ -0,0 +1,163 @@ +use std::fmt::{self, Debug, Display}; + +use serde::{Deserialize, Serialize}; +use static_assertions::{assert_impl_all, assert_not_impl_any}; + +use super::{CredentialUrl, FixedKey, SecretBytes, SecretString, ct_eq}; + +assert_not_impl_any!(FixedKey<32>: Display, Serialize, Deserialize<'static>); +assert_not_impl_any!(SecretBytes: Display, Serialize, Deserialize<'static>); +assert_not_impl_any!(SecretString: Display, Serialize, Deserialize<'static>); +assert_not_impl_any!(CredentialUrl: Display, Serialize, Deserialize<'static>); + +// Public response DTOs must keep their Serialize impls. Combined with the +// non-impl assertions above, adding a wrapper field to any of these DTOs +// becomes a compile error: #[derive(Serialize)] on the DTO would fail +// because the wrapper does not implement Serialize. Sample chosen to span +// the modules adjacent to wrapper-bearing state — dashboard, EVM, and the +// storage-info layer. +assert_impl_all!(crate::services::DashboardAccountSummary: Serialize); +assert_impl_all!(crate::services::DashboardDeltaEntry: Serialize); +assert_impl_all!(crate::services::DashboardGlobalDeltaEntry: Serialize); +assert_impl_all!(crate::services::DashboardProposalEntry: Serialize); +assert_impl_all!(crate::services::DashboardGlobalProposalEntry: Serialize); +assert_impl_all!(crate::services::DashboardInfoResponse: Serialize); +#[cfg(feature = "evm")] +assert_impl_all!(crate::api::evm::VerifySessionResponse: Serialize); +#[cfg(feature = "evm")] +assert_impl_all!(crate::api::evm::ChallengeResponse: Serialize); + +fn debug_str(value: &T) -> String { + let mut out = String::new(); + fmt::write(&mut out, format_args!("{value:?}")).unwrap(); + out +} + +#[test] +fn fixed_key_debug_redacts() { + let key = FixedKey::<32>::new([0xAB; 32]); + let rendered = debug_str(&key); + assert!(rendered.contains("FixedKey<32>")); + assert!(rendered.contains("")); + assert!(!rendered.contains("ab")); + assert!(!rendered.contains("AB")); +} + +#[test] +fn secret_bytes_debug_redacts() { + let bytes = SecretBytes::new(b"correct horse battery staple".to_vec()); + let rendered = debug_str(&bytes); + assert!(rendered.starts_with("SecretBytes(len=")); + assert!(!rendered.contains("horse")); +} + +#[test] +fn secret_string_debug_redacts() { + let s = SecretString::new("super-secret-token".to_owned()); + let rendered = debug_str(&s); + assert!(rendered.starts_with("SecretString(len=")); + assert!(!rendered.contains("super-secret-token")); +} + +#[test] +fn credential_url_debug_shows_only_scheme_and_host() { + let url = CredentialUrl::new("postgres://alice:hunter2@db.example.com:5432/app".to_owned()); + let rendered = debug_str(&url); + assert!(rendered.contains("postgres://db.example.com:5432")); + assert!(!rendered.contains("hunter2")); + assert!(!rendered.contains("alice")); + assert!(!rendered.contains("/app")); +} + +#[test] +fn credential_url_scheme_and_host_strips_userinfo_and_query() { + let url = CredentialUrl::new("postgres://alice:hunter2@db.example.com:5432/app".to_owned()); + assert_eq!(url.scheme_and_host(), "postgres://db.example.com:5432"); + + let api = CredentialUrl::new("https://api.example.com/v1/?key=abc".to_owned()); + assert_eq!(api.scheme_and_host(), "https://api.example.com"); + + let plain = CredentialUrl::new("https://example.com".to_owned()); + assert_eq!(plain.scheme_and_host(), "https://example.com"); + + let bad = CredentialUrl::new("not-a-url".to_owned()); + assert_eq!(bad.scheme_and_host(), ""); + + let at_in_path = CredentialUrl::new("https://api.example.com/users/me@host".to_owned()); + assert_eq!(at_in_path.scheme_and_host(), "https://api.example.com"); + + let at_in_query = CredentialUrl::new("https://api.example.com/?to=me@host".to_owned()); + assert_eq!(at_in_query.scheme_and_host(), "https://api.example.com"); + + // Userinfo without an '@' delimiter (malformed authority): the previous + // string-splitting heuristic would treat `alice:secret` as `host:port` + // and emit the secret verbatim. Confirm it now renders as + // (port is non-numeric, so url::Url rejects the input). + let no_at = CredentialUrl::new("postgres://alice:secret/db".to_owned()); + let rendered = no_at.scheme_and_host(); + assert!( + !rendered.contains("secret"), + "scheme_and_host leaked password portion: {rendered}" + ); +} + +#[test] +fn clone_produces_independent_buffer() { + let original = SecretBytes::new(vec![1, 2, 3, 4]); + let cloned = original.clone(); + drop(original); + assert_eq!(cloned.expose_secret(), &[1, 2, 3, 4]); + + let original = SecretString::new("token".to_owned()); + let cloned = original.clone(); + drop(original); + assert_eq!(cloned.expose_secret(), "token"); + + let original = FixedKey::<4>::new([9, 8, 7, 6]); + let cloned = original.clone(); + drop(original); + assert_eq!(cloned.expose_secret(), &[9, 8, 7, 6]); +} + +#[test] +fn equality_is_consistent_with_contents() { + let a = FixedKey::<4>::new([1, 2, 3, 4]); + let b = FixedKey::<4>::new([1, 2, 3, 4]); + let c = FixedKey::<4>::new([1, 2, 3, 5]); + assert_eq!(a, b); + assert_ne!(a, c); + + let s1 = SecretString::new("alpha".to_owned()); + let s2 = SecretString::new("alpha".to_owned()); + let s3 = SecretString::new("beta".to_owned()); + assert_eq!(s1, s2); + assert_ne!(s1, s3); + + let u1 = CredentialUrl::new("https://a.example.com".to_owned()); + let u2 = CredentialUrl::new("https://a.example.com".to_owned()); + let u3 = CredentialUrl::new("https://b.example.com".to_owned()); + assert_eq!(u1, u2); + assert_ne!(u1, u3); +} + +#[test] +fn ct_eq_distinguishes_equal_and_differing_inputs() { + assert!(ct_eq(b"", b"")); + assert!(ct_eq(b"abcdef", b"abcdef")); + assert!(!ct_eq(b"abcdef", b"abcdeg")); + assert!(!ct_eq(b"xbcdef", b"abcdef")); + assert!(!ct_eq(b"abcdef", b"abcdefg")); + assert!(!ct_eq(b"", b"a")); +} + +#[test] +fn secret_string_len_returns_byte_length() { + let s = SecretString::new("héllo".to_owned()); + assert_eq!(s.len(), "héllo".len()); +} + +#[test] +fn secret_bytes_len_returns_inner_len() { + let bytes = SecretBytes::new(vec![0; 12]); + assert_eq!(bytes.len(), 12); +} diff --git a/crates/server/src/secret/wrappers.rs b/crates/server/src/secret/wrappers.rs new file mode 100644 index 00000000..22d6b835 --- /dev/null +++ b/crates/server/src/secret/wrappers.rs @@ -0,0 +1,182 @@ +use std::fmt; + +use secrecy::{ExposeSecret, SecretBox}; +use subtle::ConstantTimeEq; +use url::Url; + +pub(crate) struct FixedKey { + inner: SecretBox<[u8; N]>, +} + +impl FixedKey { + pub(crate) fn new(bytes: [u8; N]) -> Self { + Self { + inner: SecretBox::new(Box::new(bytes)), + } + } + + pub(crate) fn expose_secret(&self) -> &[u8; N] { + self.inner.expose_secret() + } +} + +impl Clone for FixedKey { + fn clone(&self) -> Self { + Self::new(*self.expose_secret()) + } +} + +impl fmt::Debug for FixedKey { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "FixedKey<{N}>()") + } +} + +impl PartialEq for FixedKey { + fn eq(&self, other: &Self) -> bool { + self.expose_secret().ct_eq(other.expose_secret()).into() + } +} + +impl Eq for FixedKey {} + +pub(crate) struct SecretBytes { + inner: SecretBox>, +} + +impl SecretBytes { + pub(crate) fn new(bytes: Vec) -> Self { + Self { + inner: SecretBox::new(Box::new(bytes)), + } + } + + pub(crate) fn expose_secret(&self) -> &[u8] { + self.inner.expose_secret().as_slice() + } + + pub(crate) fn len(&self) -> usize { + self.inner.expose_secret().len() + } +} + +impl Clone for SecretBytes { + fn clone(&self) -> Self { + Self::new(self.expose_secret().to_vec()) + } +} + +impl fmt::Debug for SecretBytes { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "SecretBytes(len={})", self.len()) + } +} + +impl PartialEq for SecretBytes { + fn eq(&self, other: &Self) -> bool { + self.expose_secret().ct_eq(other.expose_secret()).into() + } +} + +impl Eq for SecretBytes {} + +pub(crate) struct SecretString { + inner: SecretBox, +} + +impl SecretString { + pub(crate) fn new(s: String) -> Self { + Self { + inner: SecretBox::new(Box::new(s)), + } + } + + pub(crate) fn expose_secret(&self) -> &str { + self.inner.expose_secret().as_str() + } + + pub(crate) fn len(&self) -> usize { + self.inner.expose_secret().len() + } +} + +impl Clone for SecretString { + fn clone(&self) -> Self { + Self::new(self.expose_secret().to_owned()) + } +} + +impl fmt::Debug for SecretString { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "SecretString(len={})", self.len()) + } +} + +impl PartialEq for SecretString { + fn eq(&self, other: &Self) -> bool { + self.expose_secret() + .as_bytes() + .ct_eq(other.expose_secret().as_bytes()) + .into() + } +} + +impl Eq for SecretString {} + +pub(crate) struct CredentialUrl { + inner: SecretBox, +} + +impl CredentialUrl { + pub(crate) fn new(url: String) -> Self { + Self { + inner: SecretBox::new(Box::new(url)), + } + } + + pub(crate) fn expose_secret(&self) -> &str { + self.inner.expose_secret().as_str() + } + + /// Returns `scheme://host[:port]` with userinfo, path, and query stripped. + /// Safe to log. Returns `` if parsing fails. + pub(crate) fn scheme_and_host(&self) -> String { + scheme_and_host(self.expose_secret()) + } +} + +impl Clone for CredentialUrl { + fn clone(&self) -> Self { + Self::new(self.expose_secret().to_owned()) + } +} + +impl fmt::Debug for CredentialUrl { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "CredentialUrl({})", self.scheme_and_host()) + } +} + +impl PartialEq for CredentialUrl { + fn eq(&self, other: &Self) -> bool { + self.expose_secret() + .as_bytes() + .ct_eq(other.expose_secret().as_bytes()) + .into() + } +} + +impl Eq for CredentialUrl {} + +fn scheme_and_host(raw: &str) -> String { + let Ok(url) = Url::parse(raw) else { + return "".to_owned(); + }; + let Some(host) = url.host_str() else { + return "".to_owned(); + }; + match url.port() { + Some(port) => format!("{}://{host}:{port}", url.scheme()), + None => format!("{}://{host}", url.scheme()), + } +} diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 28d0f001..d92a1e5b 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -135,6 +135,20 @@ turns them into Terraform variables or build-time choices. | `GUARDIAN_EVM_ALLOWED_CHAIN_IDS_SECRET_ARN` | _unset_ | Same, for the bookkeeping chain-ID list. | | `TF_VAR_*` | _unset_ | Any standard Terraform var override; the script passes through. | +## Secrets in env vars — scope of in-process protection + +Secret-bearing env vars (`DATABASE_URL`, `GUARDIAN_DASHBOARD_CURSOR_SECRET`, +`GUARDIAN_EVM_RPC_URLS`) are wrapped at the point of read into +zero-on-drop, no-`Display`/no-`Serialize` types inside the server +process, so they cannot accidentally appear in logs, panic messages, or +serialized responses. **This does not protect the OS process +environment block** — `/proc//environ`, coredumps, fork-inherited +env, and ECS task-definition `environment` fields all remain visible at +the OS layer regardless. For the highest-sensitivity material (ACK +signing keys) prefer the AWS Secrets Manager runtime-fetch path +(`GUARDIAN_ACK_FALCON_SECRET_ID` / `GUARDIAN_ACK_ECDSA_SECRET_ID`), +which is already how production loads those keys. + ## What's _not_ env-configurable A few things are deliberately compile-time or builder-API only — knowing diff --git a/speckit/features/008-zeroize-secrets/checklists/requirements.md b/speckit/features/008-zeroize-secrets/checklists/requirements.md new file mode 100644 index 00000000..4320af1f --- /dev/null +++ b/speckit/features/008-zeroize-secrets/checklists/requirements.md @@ -0,0 +1,83 @@ +# Specification Quality Checklist: Memory-Resident Secret Hygiene + +**Purpose**: Validate specification completeness and quality before proceeding to planning +**Created**: 2026-05-29 +**Feature**: [spec.md](../spec.md) + +## Content Quality + +- [x] No implementation details (languages, frameworks, APIs) + - Note: crate names (`zeroize`, `secrecy`, `subtle`) appear only in the **Input** quote of the user's original request; the spec body intentionally defers package selection to plan.md. +- [x] Focused on user value and business needs +- [x] Written for non-technical stakeholders + - Note: the audience is security-aware reviewers; behaviour is described in terms of observable disclosure / erasure / timing properties, not internal types. +- [x] All mandatory sections completed + +## Requirement Completeness + +- [x] No [NEEDS CLARIFICATION] markers remain +- [x] Requirements are testable and unambiguous +- [x] Success criteria are measurable +- [x] Success criteria are technology-agnostic (no implementation details) +- [x] All acceptance scenarios are defined +- [x] Edge cases are identified +- [x] Scope is clearly bounded (explicit In-Scope Inventory + Out-of-Scope) +- [x] Dependencies and assumptions identified + +## Feature Readiness + +- [x] All functional requirements have clear acceptance criteria +- [x] User scenarios cover primary flows (disclosure, erasure, timing) +- [x] Feature meets measurable outcomes defined in Success Criteria +- [x] No implementation details leak into specification + +## Notes + +- The user's original prompt named three specific crates (`zeroize`, `secrecy`, `subtle`) and asked the agent to "decide what package/s to use". Per spec discipline (WHAT not HOW), the spec defines required *behaviours* and defers package selection to `/speckit-plan`. The Input field preserves the user's wording verbatim. +- The in-scope inventory was generated from a codebase sweep on 2026-05-29 and lists 9 categories of secret-bearing storage. Future additions require an edit to this spec. +- TLS server private keys are deliberately Out-of-Scope: the server currently does not terminate TLS in-process. Add to scope if that changes. + +### Resolved review findings (2026-05-29) + +1. **Display/Serialize model — resolved to compile-time refusal.** FR-002 and FR-003 now mandate "wrapper does not implement `Display` / `Serialize`" rather than "renders redacted output at runtime". Acceptance scenarios use `static_assertions::assert_not_impl_any!`. `Debug` is the only allowed formatter trait and renders only a redaction marker. Aligns with the reviewer's open-question preference. +2. **Session-token storage shape — resolved by explicit scope clarification.** FR-004 and User Story 3 now explicitly carve out `HashMap`-keyed session lookups (the storage shape used by `DashboardState.sessions` and `EvmSessionState.sessions`) as out of scope for constant-time compare, with rationale and a forward-looking trigger requiring the spec to be amended if a refactor introduces byte-by-byte session equality. +3. **ACK signer external dependency — resolved by adding FR-011 and SC-008.** The in-scope inventory now spells out the `miden-keystore` boundary at the Falcon and ECDSA signer entries. FR-011 requires the plan to either cite upstream zeroization or design a local copy-and-zero adapter. SC-008 makes the verification a release criterion. +4. **Backtrace acceptance criterion — replaced.** The "backtrace frame captures a secret" scenario has been replaced with a panic-payload scenario (acceptance scenario 3 of User Story 1) and an edge case note that Rust does not print locals in standard backtraces. +5. **Reflection over `AppState` — removed.** FR-007 now uses the manual checklist plus audit recipe and explicitly disallows pursuing a reflection-based guard. + +### Resolved review findings — second round (2026-05-29) + +6. **Challenge nonces / digests — removed from scope.** The previous inventory entry "Operator and EVM challenge nonces" has been deleted from the In-Scope Inventory and moved to Out-of-Scope with explicit rationale: (a) operator challenges store `signing_digest`, not a server-only nonce; (b) `EvmChallenge` doubles as the public response DTO in `api/evm.rs`; (c) the values are intentionally returned to the client. Wrapping them would force a split into internal-storage vs public-DTO types, a refactor disproportionate to the threat. Re-scoping requires a spec amendment. +7. **Debug requirement on enclosing structs — relaxed.** US1 acceptance scenario 2 no longer requires `{:?}` on enclosing structs like the ACK signer or Postgres pool config. The binding requirement is now: wrappers redact their own `Debug`; *already-Debug* enclosing structs inherit that redaction through the field; secret-bearing enclosing structs are not required to gain a `Debug` impl they did not previously have. +8. **Key Entities entity definition — corrected.** The "Secret wrapper type" entity now reads "redacts `Debug` to a non-disclosing marker, and omits `Display` and `serde::Serialize` / `serde::Deserialize` impls entirely", aligning with FR-002 / FR-003. The previous "suppresses Debug / Display / Serialize" wording was internally inconsistent. + +### Resolved review findings — third round (post-plan, 2026-05-29) + +9. **Session-token storage shape — restructured.** The previous "wrap the value side" design did not actually wrap the token, because in `DashboardState.sessions` / `EvmSessionState.sessions` the token *is* the `HashMap` key, not a value field. Leaving the key as `String` would leave the long-lived token un-zeroized, violating FR-001. The plan and data-model now restructure the map to `HashMap<[u8; 32], Record>` keyed by `sha256(token)`; the plaintext token is generated, written to `Set-Cookie`, and dropped — not retained. `spec.md` User Story 3 and FR-004 updated accordingly; SC-004 no longer mentions "nonce echo match". +10. **Dependency direction — fixed.** The previous data-model proposed using the server's `secret::SecretBytes` from inside `crates/miden-keystore`. That inverts the dependency graph. The plan, data-model, and contract now use `zeroize::Zeroizing>` directly inside `miden-keystore`. Server-only wrappers remain `pub(crate)` and never reachable from a lower crate. Forbidden-Cargo-change list explicitly bans inverting this later. +11. **HMAC verify — kept as `hmac::Mac::verify_slice`.** The previous data-model claimed HMAC verify would route through `secret::ct::eq`. Reviewer correctly pointed out this would replace an audited constant-time MAC verify with a home-rolled byte equality. The plan, data-model, and a new research Decision 8 record that `verify_slice` is kept and cited at the call site. `secret::ct::eq` remains in the module for future byte-equality sites that need it. +12. **`secrecy` version pin — tightened.** Bumped from "latest 0.8.x" to **`"0.10"` with `default-features = false`**. The `secrecy/serde` feature is explicitly forbidden (it would add a `Deserialize` impl on `SecretBox` and violate FR-003); the `static_assertions` block is the compile-time backstop. Recorded in research Decision 1 and contracts/secret-module.md "Forbidden Cargo changes". +13. **SC-004 wording — cleaned.** Removed the "nonce echo match" example (challenges are out of scope from the second round). SC-004 now covers (a) sites using a canonical crypto-crate constant-time primitive and (b) sites using `secret::ct::eq`, and explicitly notes that session-token lookups are structural over digests, not byte-equality. + +### Resolved review findings — fourth round (env-var coverage, 2026-05-29) + +14. **Env-var read window — bounded.** Added **FR-012** requiring every secret-bearing env-var read to fold `std::env::var(...)` and the wrapper constructor into a single expression — no intermediate `String` local. Inventory of in-scope env vars confirmed by grep: `DATABASE_URL` (`audit/postgres.rs:264`, `builder/storage.rs:79`), `GUARDIAN_DASHBOARD_CURSOR_SECRET` (`dashboard/config.rs:47`), `GUARDIAN_EVM_RPC_URLS` (`evm/config.rs:77`). Migration steps 2–4 of the data-model rewrite the reads to comply. Added **SC-010**, verified by the `quickstart.md` audit-recipe grep and the FR-007 reviewer checklist plus the compile-time assertions. +15. **OS env block — explicitly Out-of-Scope.** The OS process environment block (`/proc//environ`, coredumps, fork-inherited env, dotenvy `.env`, ECS task-definition `environment`) is now a named Out-of-Scope item in the spec, with framing: same threat-model line as TLS keys; mitigation is an infra concern (prefer AWS Secrets Manager runtime fetch). The plan explicitly does **not** introduce `unsafe { std::env::remove_var(...) }` calls — small threat reduction, real process-global `unsafe` cost. Quickstart audit recipe now includes a grep for any such calls. +16. **FR-007 guard settled on manual review.** Research Decision 5 documents why the final guard is the manual-review arm plus compile-time assertions. Quickstart keeps the reviewer audit recipe. + +### Resolved review findings — fifth round (compile/test rigor, 2026-05-29) + +17. **`PartialEq` / `Eq` added to all four wrapper types.** Without this, `EvmChainConfig` and `EvmChainRegistry` (`evm/config.rs:9, 16`), which derive `PartialEq, Eq`, would fail to compile after `rpc_url: String` is re-typed to `CredentialUrl`. `secrecy::SecretBox` does not implement `PartialEq`. The wrapper impls route through `subtle::ConstantTimeEq` internally — defense-in-depth that makes any `==` on a wrapper automatically constant-time. Recorded in data-model wrapper tables and contracts/secret-module.md. +18. **`Clone` is hand-rolled on every wrapper.** `secrecy::SecretBox` is not `Clone` either. Every wrapper's `Clone` allocates a new buffer through `expose_secret()`. Data-model and contracts now spell this out so implementers don't try `#[derive(Clone)]` and get a confusing error. +19. **AWS Secrets Manager inventory entry corrected.** Previous wording ("fetched bytes; cache field if present") was speculative and misaligned with the actual code. There is no cache; the secret-bearing values are the transient `secret_hex: String` and `secret_bytes: Vec` inside `parsed_secret_key`/`secret_string`. Both are stack-locals carrying full private-key material — explicit Out-of-Scope exception in the spec inventory. Migration step 7 rewritten accordingly: wrap `secret_hex` in `SecretString`, `secret_bytes` in `SecretBytes`; no return-type change. +20. **SC-009 mechanism rewritten.** The previous "compile a function that takes the DTO by value" was a no-op (didn't exercise `Serialize`). New mechanism: `static_assertions::assert_impl_all!(Dto: Serialize)` on a representative sample of public response DTOs, combined with the existing `assert_not_impl_any!` on wrappers (SC-002/003), transitively makes "wrapper field in a DTO" a compile error. +21. **`secret::ct::eq` is `#[allow(dead_code)]`.** It has zero callers in this feature (HMAC verify stays on `verify_slice`; session lookup is digest-keyed). Marked `pub(crate)` with `#[allow(dead_code)]` so `-D warnings` does not break CI on step 1; a unit test exercises it so it does not bit-rot silently. +22. **FR-007 review posture finalized.** Migration step 9 now documents the reviewer checklist / quickstart audit posture only. +23. **Session-token lifetime framing softened.** Research Decision 6 previously claimed the plaintext token lives "hundreds of nanoseconds" — inaccurate, because the token is embedded in `Set-Cookie` and the response payload and persists for the issuing request's duration. Reworded to "request-scoped, out of strict zeroization scope per general Out-of-Scope rule". Migration steps 5/6 reworded to drop the implication that `String::drop` zeroizes (it does not). +24. **Forward-pointer added for `evm/session.rs:113` `eq_ignore_ascii_case`.** The existing non-constant-time challenge-nonce compare is a known consequence of keeping challenges out of scope (nonce is already disclosed to the holder in the prior challenge response). Spec Out-of-Scope now names the call site so a future auditor does not re-litigate it. + +### Resolved review findings — post-implementation round (2026-05-29) + +25. **FR-011 / SC-008 citations added at the signer call sites.** The Falcon and ECDSA `sign_with_server_key` comments now read "Verified: miden-crypto 0.23.0 (pulled in by miden-protocol 0.14.5) implements `impl ZeroizeOnDrop for SecretKey {}` in `src/dsa/falcon512_poseidon2/keys/secret_key.rs` / `src/dsa/ecdsa_k256_keccak/mod.rs`". Re-verified by direct grep against the cached crate source. Research.md Decision 2 now records branch **(a)** as the outcome with the same citations. Earlier session memory that flagged Falcon as "historically not zeroizing" pre-dated the current pinned revision; the current pinned crate does implement it. +26. **Module visibility reverted to `pub(crate)` per the contract.** `crates/server/src/lib.rs` is `pub(crate) mod secret;`, `mod.rs` is `pub(crate) use wrappers::{…}`, and every wrapper type + method is `pub(crate)`. The previous transient `pub` was a clippy `private-interfaces` shortcut; the proper fix was to make `EvmChainConfig.rpc_url` a `pub(crate)` field instead (no external consumer accesses it). The contract doc and constitution check are once again literally true: wrappers live only inside `guardian-server`. +27. **SC-009 sample broadened to span wrapper-adjacent modules.** `secret/tests.rs` now asserts `Serialize` impls for `DashboardInfoResponse` (storage/info layer adjacent to `StorageMetadataBuilder`) and the EVM HTTP DTOs `VerifySessionResponse` + `ChallengeResponse` (under `#[cfg(feature = "evm")]`), in addition to the original five dashboard sample DTOs. Matches T018's "sample spans wrapper-bearing modules" intent. The transitive compile-time guarantee (any DTO with a wrapper field fails its `#[derive(Serialize)]`) was always crate-wide; this just tightens the explicit tripwire. diff --git a/speckit/features/008-zeroize-secrets/contracts/secret-module.md b/speckit/features/008-zeroize-secrets/contracts/secret-module.md new file mode 100644 index 00000000..9af22dec --- /dev/null +++ b/speckit/features/008-zeroize-secrets/contracts/secret-module.md @@ -0,0 +1,176 @@ +# Contract: `crates/server/src/secret` Module API + +**Feature**: `008-zeroize-secrets` +**Date**: 2026-05-29 + +> This feature introduces no HTTP / gRPC / SDK surface change. The only "contract" is the internal public API of the new `secret` module within `crates/server`. This document records that contract so future contributors can reason about what is allowed to change without breaking the security invariants. + +--- + +## Module visibility + +- Path: `crates/server/src/secret/` +- Visibility: `pub(crate)` — the module's items are reachable from anywhere inside `guardian-server`, but **not** exported from the crate. Nothing in this module appears in the server's public dependency graph for downstream consumers (clients, SDKs, npm packages). + +## Public surface (within the crate) + +```rust +// crates/server/src/secret/mod.rs +pub(crate) mod ct; +pub(crate) mod wrappers; + +pub(crate) use wrappers::{FixedKey, SecretBytes, SecretString, CredentialUrl}; +pub(crate) use ct::eq as ct_eq; +``` + +## `wrappers::FixedKey` + +```rust +pub(crate) struct FixedKey { /* SecretBox<[u8; N]> */ } + +impl FixedKey { + pub(crate) fn new(bytes: [u8; N]) -> Self; + pub(crate) fn expose_secret(&self) -> &[u8; N]; +} + +// SecretBox is NOT Clone or PartialEq; every impl below is hand-rolled. +impl Clone for FixedKey { /* fresh SecretBox over a copied [u8; N] */ } +impl std::fmt::Debug for FixedKey { /* "FixedKey(…)" */ } +impl PartialEq for FixedKey { /* subtle::ConstantTimeEq over inner bytes */ } +impl Eq for FixedKey {} +// Display: NOT IMPLEMENTED — compile-time enforced +// Serialize: NOT IMPLEMENTED — compile-time enforced +// Deserialize: NOT IMPLEMENTED — compile-time enforced +``` + +## `wrappers::SecretBytes` + +```rust +pub(crate) struct SecretBytes { /* SecretBox> */ } + +impl SecretBytes { + pub(crate) fn new(bytes: Vec) -> Self; + pub(crate) fn expose_secret(&self) -> &[u8]; + pub(crate) fn len(&self) -> usize; // safe metadata; does not leak content +} + +// SecretBox is NOT Clone or PartialEq; every impl below is hand-rolled. +impl Clone for SecretBytes { /* allocates a new Vec */ } +impl std::fmt::Debug for SecretBytes { /* "SecretBytes(len=N)" */ } +impl PartialEq for SecretBytes { /* subtle::ConstantTimeEq */ } +impl Eq for SecretBytes {} +// Display: NOT IMPLEMENTED +// Serialize/Deserialize: NOT IMPLEMENTED +``` + +## `wrappers::SecretString` + +```rust +pub(crate) struct SecretString { /* SecretBox */ } + +impl SecretString { + pub(crate) fn new(s: String) -> Self; + pub(crate) fn expose_secret(&self) -> &str; + pub(crate) fn len(&self) -> usize; // byte length; does not leak content +} + +// SecretBox is NOT Clone or PartialEq; every impl below is hand-rolled. +impl Clone for SecretString { /* allocates a new String */ } +impl std::fmt::Debug for SecretString { /* "SecretString(len=N)" */ } +impl PartialEq for SecretString { /* subtle::ConstantTimeEq over expose_secret().as_bytes() */ } +impl Eq for SecretString {} +// Display: NOT IMPLEMENTED +// Serialize/Deserialize: NOT IMPLEMENTED +``` + +## `wrappers::CredentialUrl` + +```rust +pub(crate) struct CredentialUrl { /* SecretBox */ } + +impl CredentialUrl { + pub(crate) fn new(url: String) -> Self; + pub(crate) fn expose_secret(&self) -> &str; + /// Returns `scheme://host[:port]` — safe to log. Returns "" on parse failure. + pub(crate) fn scheme_and_host(&self) -> String; +} + +// SecretBox is NOT Clone or PartialEq; every impl below is hand-rolled. +impl Clone for CredentialUrl { /* allocates a new String */ } +impl std::fmt::Debug for CredentialUrl { /* "CredentialUrl()" */ } +impl PartialEq for CredentialUrl { /* subtle::ConstantTimeEq over expose_secret().as_bytes() */ } +impl Eq for CredentialUrl {} +// REQUIRED by EvmChainConfig / EvmChainRegistry which derive PartialEq, Eq. +// Display: NOT IMPLEMENTED +// Serialize/Deserialize: NOT IMPLEMENTED +``` + +## `ct::eq` + +```rust +/// Constant-time equality over byte slices. Available for future byte-by-byte +/// equality sites; not used by any current call site in this feature. +/// +/// The cursor HMAC verify keeps `hmac::Mac::verify_slice` (Decision 8); session +/// lookup is keyed by digest (Decision 6). This function is `#[allow(dead_code)]` +/// at landing and is asserted by a unit test so it does not bit-rot silently. +/// +/// Not for use with HashMap-keyed lookups (out of FR-004 scope; see spec.md User Story 3). +#[allow(dead_code)] +pub(crate) fn eq(a: &[u8], b: &[u8]) -> bool; +``` + +--- + +## Compile-time invariants (enforced by `wrappers::tests`) + +For each wrapper type `T ∈ { FixedKey<32>, SecretBytes, SecretString, CredentialUrl }`: + +```rust +static_assertions::assert_not_impl_any!(T: std::fmt::Display); +static_assertions::assert_not_impl_any!(T: serde::Serialize); +static_assertions::assert_not_impl_any!(T: serde::Deserialize<'static>); +``` + +For a representative sample of public HTTP response DTOs (`D ∈ { dashboard response DTO, EVM session response DTO, storage info response DTO, ... }`): + +```rust +// Asserts the DTO still derives Serialize. Combined with the wrapper non-impl +// assertions above, this transitively makes "a wrapper field in a public DTO" +// a compile error: the #[derive(Serialize)] on the DTO would fail to compile +// because the wrapper does not implement Serialize. This satisfies SC-009 +// without a bespoke structural-walk test. +static_assertions::assert_impl_all!(D: serde::Serialize); +``` + +If any of these starts to fail, the build fails — that is the intended signal. + +## Runtime invariants (enforced by `wrappers::tests`) + +- `format!("{:?}", x)` never contains the underlying bytes/string for any wrapper. +- `Clone` produces an independently-owned buffer (dropping the original does not zero the clone's bytes). +- `secret::ct::eq` returns the same logical result as `==` for equal-length inputs and `false` for differing-length inputs. + +## Allowed crate dependencies introduced + +| Crate | Version pin | Features | Where | Why | +|---|---|---|---|---| +| `secrecy` | `"0.10"` | `default-features = false` (**no `serde`**) | `crates/server` | Provides `SecretBox`: redacted `Debug`, no `Display`, no `Serialize`/`Deserialize`, `expose_secret()` access, zero-on-drop. The `serde` feature MUST stay disabled to keep FR-003 a compile-time guarantee. | +| `zeroize` | `"1.7"` | `["derive"]` | `crates/server` + `crates/miden-keystore` | Required by `FixedKey`'s derive. Also used by `miden-keystore` for `Zeroizing>` on the disk-read buffer. | +| `subtle` | `"2.5"` | (default) | `crates/server` | Constant-time equality primitive. Used by `secret::ct::eq` for any future byte-by-byte equality site; **not** used to replace `hmac::Mac::verify_slice`. | +| `static_assertions` | `"1.1"` | (default) | `crates/server` `[dev-dependencies]` | Compile-time non-impl checks. | + +### Forbidden Cargo changes + +- Enabling `secrecy/serde` (anywhere in the workspace) — would add a `Deserialize` impl on `SecretBox` and silently violate FR-003. The `static_assertions::assert_not_impl_any!` block is the compile-time backstop, but the feature flag MUST also remain off. +- Switching `miden-keystore` from `zeroize::Zeroizing>` to the server's `secret::SecretBytes` — would invert the dependency direction. The server depends on the keystore, not the other way around. + +## Forbidden changes (regress without an amendment to this contract) + +- Implementing `Display` on any wrapper. +- Implementing `Serialize` or `Deserialize` on any wrapper, or enabling the `secrecy/serde` feature. +- Returning the raw inner value from any method other than `expose_secret()` / `scheme_and_host()`. +- Adding a `From` impl that bypasses the named constructor. +- Replacing `secret::ct::eq` with `==` at a previously-migrated call site. + +A reviewer encountering any of these in a PR should refuse the change or require a corresponding spec amendment. diff --git a/speckit/features/008-zeroize-secrets/data-model.md b/speckit/features/008-zeroize-secrets/data-model.md new file mode 100644 index 00000000..955c077b --- /dev/null +++ b/speckit/features/008-zeroize-secrets/data-model.md @@ -0,0 +1,170 @@ +# Phase 1 Data Model: Memory-Resident Secret Hygiene + +**Feature**: `008-zeroize-secrets` +**Date**: 2026-05-29 + +This document describes the type entities introduced by this feature and maps the in-scope inventory from `spec.md` to a concrete wrapper variant per site. It also defines the migration order. + +> The wrappers do **not** model new persisted entities. This is a Rust type-system refactor, not a schema change. "Data model" here means the *type-level* shape of the wrappers and how existing fields are re-typed. + +--- + +## Wrapper Types + +All four wrapper types live in `crates/server/src/secret/wrappers.rs` and re-export through `crates/server/src/secret/mod.rs`. They share a common contract enforced by both their `secrecy`-based implementation and a compile-time assertion suite. + +> **Implementation note for all four wrappers (Clone, PartialEq, Eq)**: `secrecy::SecretBox` is **not** `Clone` — `#[derive(Clone)]` will not work. Every wrapper's `Clone` impl is **hand-rolled**, routing through `expose_secret()` and constructing a fresh `SecretBox` over an independently-allocated buffer. Similarly, `SecretBox` is **not** `PartialEq` — but several enclosing structs that hold a wrapper (`EvmChainConfig` and `EvmChainRegistry` at `evm/config.rs:9, 16`, plus test fixtures) currently derive `PartialEq, Eq`. Every wrapper therefore implements `PartialEq` + `Eq` **using `subtle::ConstantTimeEq` internally**, so any `==` on a wrapper is automatically constant-time. This is defense-in-depth — wrapper equality is rarely against untrusted input in practice (mostly config-struct equality in tests), but making `==` constant-time eliminates a future foot-gun and lets enclosing structs keep their `PartialEq, Eq` derives. + +### `FixedKey` + +| Property | Value | +|---|---| +| Inner storage | `secrecy::SecretBox<[u8; N]>` | +| Constructor | `pub fn new(bytes: [u8; N]) -> Self` | +| Exposure | `pub fn expose_secret(&self) -> &[u8; N]` | +| `Drop` | Zeroizes the `[u8; N]` | +| `Debug` | Renders `FixedKey(…)` | +| `Display` | **Not implemented** | +| `Serialize` / `Deserialize` | **Not implemented** | +| `Clone` | Hand-rolled; constructs a fresh `SecretBox` over a copied array | +| `PartialEq` / `Eq` | Hand-rolled via `subtle::ConstantTimeEq` over the inner bytes | + +Used for fixed-size symmetric key material (HMAC keys). + +### `SecretBytes` + +| Property | Value | +|---|---| +| Inner storage | `secrecy::SecretBox>` | +| Constructor | `pub fn new(bytes: Vec) -> Self` | +| Exposure | `pub fn expose_secret(&self) -> &[u8]` | +| `Drop` | Zeroizes the `Vec` | +| `Debug` | Renders `SecretBytes(len=N)` | +| `Display` | **Not implemented** | +| `Serialize` / `Deserialize` | **Not implemented** | +| `Clone` | Hand-rolled; allocates a new `Vec` | +| `PartialEq` / `Eq` | Hand-rolled via `subtle::ConstantTimeEq` | + +Used for variable-length opaque bytes (cloud-fetched secret material; decoded hex bytes inside the AWS Secrets Manager flow). + +### `SecretString` + +| Property | Value | +|---|---| +| Inner storage | `secrecy::SecretBox` | +| Constructor | `pub fn new(s: String) -> Self` | +| Exposure | `pub fn expose_secret(&self) -> &str` | +| `Drop` | Zeroizes the `String`'s buffer | +| `Debug` | Renders `SecretString(len=N)` | +| `Display` | **Not implemented** | +| `Serialize` / `Deserialize` | **Not implemented** | +| `Clone` | Hand-rolled; allocates a new `String` | +| `PartialEq` / `Eq` | Hand-rolled via `subtle::ConstantTimeEq` over `expose_secret().as_bytes()` | + +Used for variable-length secret string values (the hex secret returned by AWS Secrets Manager before decode; any future bearer/refresh tokens). + +### `CredentialUrl` + +| Property | Value | +|---|---| +| Inner storage | `secrecy::SecretBox` | +| Constructor | `pub fn new(url: String) -> Self` | +| Exposure | `pub fn expose_secret(&self) -> &str` | +| Additional accessor | `pub fn scheme_and_host(&self) -> String` — returns `scheme://host[:port]` with userinfo, path, and query stripped; safe to log | +| `Drop` | Zeroizes the `String`'s buffer | +| `Debug` | Renders `CredentialUrl(scheme://host[:port])` (or `CredentialUrl(…)` if parse fails) | +| `Display` | **Not implemented** | +| `Serialize` / `Deserialize` | **Not implemented** | +| `Clone` | Hand-rolled; allocates a new `String` | +| `PartialEq` / `Eq` | Hand-rolled via `subtle::ConstantTimeEq` over `expose_secret().as_bytes()` — **required**, because `EvmChainConfig` and `EvmChainRegistry` (`evm/config.rs:9, 16`) derive `PartialEq, Eq` | + +Used for URLs that may embed credentials. The `scheme_and_host` accessor gives a deliberately narrow safe-to-log view without exposing userinfo or query parameters. + +### `secret::ct::eq` + +Not a type — a single named function: + +```rust +#[allow(dead_code)] // kept available for future byte-by-byte equality sites +pub(crate) fn eq(a: &[u8], b: &[u8]) -> bool +``` + +Internally uses `subtle::ConstantTimeEq` and converts the resulting `Choice` to `bool` only as the function's final action. Single audit-grepable site for any future constant-time-equality requirement. + +> **Note**: as of this feature, `secret::ct::eq` has **zero callers**. The cursor HMAC verify keeps `hmac::Mac::verify_slice` (Decision 8); session-token lookup is keyed by digest (Decision 6). The function is published `pub(crate)` with `#[allow(dead_code)]` so it does not break CI under `-D warnings` and is available immediately for any future call site that needs it. Its existence is also asserted by a unit test (so the dead-code allow does not let it bit-rot silently). + +--- + +## Site Mapping + +Each in-scope site from `spec.md` is mapped to a wrapper variant. + +| Site | File | Current type | Wrapper variant | Notes | +|---|---|---|---|---| +| `CursorSecret` (HMAC key) | `dashboard/cursor.rs` | newtype around `[u8; 32]` with manual `Debug` redaction | `FixedKey<32>` | Manual `Debug` impl is **removed** post-migration (replaced by wrapper's). HMAC verify path is **unchanged** — it continues to use `hmac::Mac::verify_slice` at `cursor.rs:303`, which RustCrypto documents as constant-time. A short citation comment is added at the call site. | +| AWS Secrets Manager transient key material | `ack/secrets_manager.rs` (`parsed_secret_key` and `secret_string`) | local `secret_hex: String` (full key as hex) and local `secret_bytes: Vec` (decoded raw key) — both stack-local within the fetch fn; no cache exists today | `secret_hex` → `SecretString`; `secret_bytes` → `SecretBytes` | **Explicit Out-of-Scope exception**: these are single-call stack-locals (the spec's general Out-of-Scope excludes such values), but they hold full Falcon/ECDSA private-key material in the call frame between fetch and parse. The defense-in-depth value warrants wrapping. If a cache is added in the future, it MUST use `SecretBytes` for parsed-key bytes (or `SecretString` for hex). | +| `miden-keystore::read_key_file` disk-read buffer | `crates/miden-keystore/src/keystore.rs:107`, `ecdsa_keystore.rs:103` | local `Vec` | **`zeroize::Zeroizing>`** (direct `zeroize` dep — not the server's `SecretBytes`) | The keystore crate is a lower layer and MUST NOT depend on the server's private `secret` module. `Zeroizing` provides the zero-on-drop contract; the no-`Display` / no-`Serialize` posture is not needed for this stack-local buffer. Wraps immediately after the disk read; `&[u8]` view passed to `SecretKey::read_from_bytes`. | +| Session tokens (operator + EVM) | `dashboard/state.rs`, `evm/session.rs` | `HashMap` / `HashMap` keyed by the token string | **Storage-shape change**: `HashMap<[u8; 32], OperatorSessionRecord>` keyed by `sha256(token)`. The plaintext token is **not retained in memory** after the Set-Cookie response is written — generation produces the token, the server inserts `(sha256(token), record)`, and returns the token to the client; subsequent requests are looked up by `sha256(candidate)`. No constant-time compare is needed (lookup is structural over the digest, and the digest is not a secret). | This is the only structural change in this feature. Rationale: leaving the token as the map key would leave it un-zeroized in heap memory, violating FR-001. Wrapping only the value side was the previous data-model design; that was rejected per second-round review. See research.md Decision 6 (revised). | +| Postgres connection URL | `builder/storage.rs` (`StorageMetadataBuilder.database_url`) and any downstream `String` field retaining it after pool build | `Option` | `Option` | After pool construction, drop the wrapper as early as possible (do not retain in `PostgresService` unless needed). | +| EVM RPC URL | `evm/config.rs` (`EvmChainConfig.rpc_url`) | `String` | `CredentialUrl` | Public-DTO check: confirmed that `EvmChainConfig` is not in any HTTP response payload. | +| Falcon signer keystore field | `ack/miden_falcon_rpo/signer.rs` (`MidenFalconRpoSigner.keystore`) | `Arc>` | **No change to field type** | The keystore type itself does not change; the disk-read buffer inside the keystore call is wrapped as above. FR-011 verification noted in research.md. | +| ECDSA signer keystore field | `ack/miden_ecdsa/signer.rs` (`MidenEcdsaSigner.keystore`) | `Arc>` | **No change to field type** | Same as Falcon. | + +### Sites explicitly OUT of scope (per spec) + +- Operator `PendingChallenge.signing_digest` (returned to client, public DTO collision). +- `EvmChallenge` (storage type *is* the public response DTO in `api/evm.rs`). +- TLS server private keys (no in-process TLS termination today). +- All SDK crates (`crates/client`, `crates/miden-multisig-client`, npm packages). + +--- + +## Migration Order + +Smallest blast radius first; each step is an independent commit on the feature branch. + +1. **Introduce the `secret` module** (`secret/mod.rs`, `wrappers.rs`, `ct.rs`, `tests.rs`). No callers yet. Adds compile-time assertions for the four wrapper types. Adds `secret::ct::eq` with unit tests. CI green. **Note**: `secret::ct::eq` exists for future byte-by-byte equality sites; it is **not** used by the cursor HMAC verify, which already uses the constant-time `hmac::Mac::verify_slice`. +2. **Migrate `CursorSecret`** → `FixedKey<32>`. HMAC verify path is **unchanged** — confirm `hmac::Mac::verify_slice` is still the call at `cursor.rs:303` and add a one-line citation comment. Remove the manual `Debug` redaction in `cursor.rs:240-246` (now redundant). Per FR-012, `dashboard/config.rs:47`'s `std::env::var("GUARDIAN_DASHBOARD_CURSOR_SECRET")` read folds into a single expression that decodes the hex and constructs `FixedKey::<32>::new(...)` — no intermediate `String` or `[u8; 32]` local. +3. **Migrate `EvmChainConfig.rpc_url`** → `CredentialUrl`. Per FR-012, the env-var parser at `evm/config.rs:77` constructs the wrapper in a single expression: `CredentialUrl::new(std::env::var(RPC_URLS_ENV).ok().unwrap_or_default())` (or equivalent — no intermediate `String` local). Adjust the few call sites that build the HTTP client to pass `expose_secret()`. Replace any startup logging that printed the full URL with `cfg.rpc_url.scheme_and_host()`. +4. **Migrate `StorageMetadataBuilder.database_url`** → `Option`. Per FR-012, the env-var reads at `audit/postgres.rs:264` and `builder/storage.rs:79` are converted to single-expression construct-and-wrap; the `unwrap_or_default()` chain in `builder/storage.rs:79` is rewritten so the env-var `String` is consumed by `CredentialUrl::new` in the same expression. The `StorageMetadataBuilder.database_url` field type becomes `Option` end-to-end. +5. **Restructure operator session storage**: change the map to `HashMap<[u8; 32], OperatorSessionRecord>` keyed by `sha256(token)`. The plaintext token is generated, used to build the `Set-Cookie` response, and goes out of scope at request end — it is **not** persisted in the session map. Lookup on subsequent requests computes `sha256(candidate)` and uses standard `HashMap::get`. Update fixtures, tests, and the cookie-issue path. **Note**: the plaintext token's `String` going out of scope at request end is a plain `String::drop` — it does **not** zeroize (that is the entire premise of the feature). The token is request-scoped and out of strict zeroization scope per the spec's general Out-of-Scope rule for request-locals. Optionally, the cookie-issue handler may wrap its intermediate token in `SecretString` to shrink the window further; this is a judgment call left to the implementer per site. +6. **Restructure EVM session storage**: same pattern as step 5 for `EvmSessionState.sessions`. Same notes about request-scoped token lifetime apply. +7. **Wrap the AWS Secrets Manager transient key material** inside `parsed_secret_key` and `secret_string` (`ack/secrets_manager.rs`): the `secret_hex: String` result of the cloud fetch becomes `SecretString`; the `secret_bytes: Vec` decoded via `hex::decode` becomes `SecretBytes`. The parser closure receives `secret_bytes.expose_secret()` (`&[u8]`) and returns the parsed `FalconSecretKey` / `EcdsaSecretKey` as before. Both wrappers go out of scope at function return and zeroize. **No return-type change** — the fn still returns the parsed key. This is an explicit Out-of-Scope exception (stack-local but full-key-bearing). +8. **Wrap the keystore disk-read buffer** in both `keystore.rs` and `ecdsa_keystore.rs` with **`zeroize::Zeroizing>`** (not the server's `SecretBytes` — dep-direction would be wrong). Record FR-011 verification result in code comments (`// Zeroization: upstream SecretKey verified at …` or `// Zeroization: upstream SecretKey unverified — local Zeroizing> handles file-read buffer`). +9. **Document the FR-007 manual-review guard**: add the reviewer checklist wording in `CONTRIBUTING.md` and keep the `quickstart.md` audit recipe current. +10. **Add the SC-009 transitive guard**: add `static_assertions::assert_impl_all!(Dto: serde::Serialize)` for a representative set of public HTTP response DTOs (e.g. the dashboard response shapes, the EVM session response, and the storage info response). Combined with the SC-002 / SC-003 non-impl assertions on the wrapper types, adding a wrapper field to any of these DTOs becomes a compile error (the `#[derive(Serialize)]` on the DTO will fail because the wrapper does not implement `Serialize`). No bespoke "structural walk" test is added — the assertion mechanism is the test. + +Each step ships green (`cargo test -p guardian-server` passes) before the next is started. + +--- + +## Test Surface + +Tests added as part of this feature: + +| Test | Location | Asserts | +|---|---|---| +| `assert_not_impl_any!` block (compile-time) | `secret/tests.rs` | Each of `FixedKey<32>`, `SecretBytes`, `SecretString`, `CredentialUrl` does **not** implement `Display`, `Serialize`, or `Deserialize`. | +| `debug_redacts` | `secret/tests.rs` | `format!("{:?}", x)` for each wrapper contains only the redaction marker and never the underlying bytes/string. | +| `clone_independent` | `secret/tests.rs` | Cloning a wrapper produces an independently-owned buffer (constructed-cloned-dropped-original; clone still readable). | +| `eq_uses_constant_time` | `secret/tests.rs` | For each wrapper, `==` returns true on equal contents and false on differing contents (correctness only; timing property is structural — it routes through `subtle::ConstantTimeEq` per the `PartialEq` impl). | +| `ct_eq_distinguishes` | `secret/tests.rs` | `secret::ct::eq` returns true on equal inputs and false on inputs that differ at any position. Also keeps the function out of dead-code reach so it does not bit-rot silently. (Cannot meaningfully test the *timing* property in a unit test — recorded as a code-review check in quickstart.md.) | +| `cursor_hmac_verify_unchanged` | existing `dashboard/cursor.rs` tests | The existing HMAC verify tests continue to pass after `CursorSecret` is re-typed as `FixedKey<32>`. The `verify_slice` call is unchanged; only the secret-construction path moves to the wrapper. | +| `session_lookup_by_digest` | `dashboard/state.rs` and `evm/session.rs` tests | Inserting a session with a generated token and looking it up by the same token returns the record; a mismatched token returns `None`. Internally exercises the `sha256(token)` keyed lookup. | +| `session_token_not_retained` | `dashboard/state.rs` and `evm/session.rs` tests | After session insertion, the map's keys are not the plaintext token bytes — verified by checking that the map's keys have length 32 and do not equal the issued token's bytes. | +| `response_dto_is_serialize` (SC-009 transitive guard) | `secret/tests.rs` or alongside DTO modules | `static_assertions::assert_impl_all!(Dto: serde::Serialize)` for a representative sample of public HTTP response DTOs. Combined with `assert_not_impl_any!` on wrappers (SC-002/003), this makes "wrapper field in a DTO" a compile error. | +| `credential_url_scheme_and_host_safe` | `secret/tests.rs` | For URLs of the form `postgres://user:pass@host:5432/db` and `https://api.example.com/?key=abc`, `scheme_and_host()` returns the scheme+host(+port) only. | + +--- + +## Validation Matrix + +Following `guardian-validation-matrix` discipline: + +| Layer | Tests run | +|---|---| +| `crates/server` unit tests | All (the affected modules are `dashboard/cursor`, `dashboard/state`, `evm/config`, `evm/session`, `builder/storage`, `ack/*`, plus the new `secret/*` module). Run with `cargo test -p guardian-server`. | +| `crates/miden-keystore` unit tests | All. Run with `cargo test -p miden-keystore`. The disk-read buffer wrap touches these. | +| Server integration tests | Standard `cargo test --all-features` for the server crate. | +| Cross-language parity | **Not required**. This feature does not touch any wire surface or any SDK; constitution principle II is vacuous (see plan.md Constitution Check). | +| Operator smoke (manual) | Recommended once after migration: run `smoke-test-operator-dashboard` to confirm operator login → list accounts → logout still works end-to-end with the wrapped session-token store. | diff --git a/speckit/features/008-zeroize-secrets/plan.md b/speckit/features/008-zeroize-secrets/plan.md new file mode 100644 index 00000000..97a2f8d5 --- /dev/null +++ b/speckit/features/008-zeroize-secrets/plan.md @@ -0,0 +1,144 @@ +# Implementation Plan: Memory-Resident Secret Hygiene + +**Branch**: `008-zeroize-secrets` | **Date**: 2026-05-29 | **Spec**: [spec.md](./spec.md) +**Input**: Feature specification from `speckit/features/008-zeroize-secrets/spec.md` + +## Summary + +Wrap every long-lived secret value held in the Guardian **server** process in a small set of typed wrappers that (a) zero their backing buffer on `Drop`, (b) omit `Display` and `serde::Serialize`/`Deserialize` impls entirely so the compiler refuses accidental log/JSON exposure, (c) redact `Debug` to a non-disclosing marker, and (d) route the few remaining byte-by-byte equality checks against untrusted input through a single constant-time helper. The wrappers live in `crates/server/src/secret/` and ship behind a small API: construct → use via an explicit `expose_secret()` call site → drop and zeroize. + +Crate selection (resolved in Phase 0 research): the `secrecy` crate (which composes `zeroize` for `Drop`) provides the redaction-on-`Debug`, no-`Display`, no-`Serialize`-by-default contract out of the box; `subtle` provides the constant-time-equality primitive. `zeroize` is brought in transitively but is also depended on directly for any newtype that needs a custom `Zeroize`/`ZeroizeOnDrop` derive. + +Implementation order: (1) introduce the `secret` module and four named wrapper types; (2) migrate the in-scope inventory site-by-site in independent commits, smallest blast radius first (cursor HMAC secret → DB URL → EVM RPC URLs → session-token stores → ACK signer key-load path → AWS Secrets Manager fetched bytes); (3) verify the `miden-protocol::SecretKey` boundary per FR-011 and either cite upstream zeroization or wrap with a local adapter; (4) at every secret-bearing env-var read, fold the `std::env::var(...)` call and the wrapper constructor into a single expression so no intermediate `String` is held (FR-012); (5) add the compile-time and runtime tests required by SC-002 / SC-003 / SC-005 / SC-009; (6) document the FR-007 guard as a `CONTRIBUTING.md` reviewer checklist plus the `quickstart.md` audit recipe. + +**Env-var handling — explicit scope**: this feature wraps the *Rust-side* destination of each secret-bearing env var (`DATABASE_URL`, `GUARDIAN_DASHBOARD_CURSOR_SECRET`, `GUARDIAN_EVM_RPC_URLS`). It does **not** address the OS process environment block — `/proc//environ`, coredumps, fork-inherited env, and ECS/docker-injected env vars all remain visible at the OS layer. Mitigating that is an infra concern (prefer AWS Secrets Manager runtime fetch over env-var injection, already used by the Falcon/ECDSA key load path). The plan does **not** call `unsafe { std::env::remove_var(...) }` after reads; the threat reduction is small relative to the process-global `unsafe` cost. See spec Out-of-Scope. + +## Technical Context + +**Language/Version**: Rust 1.x (workspace's pinned toolchain — `rust-toolchain.toml`). +**Primary Dependencies (new)**: +- `secrecy = { version = "0.10", default-features = false }` — explicitly without the `serde` feature; FR-003 forbids transparent serialization, and `secrecy`'s `serde` feature would add a `Deserialize` impl that defeats the compile-time guarantee. The compile-time `assert_not_impl_any!` checks (SC-002 / SC-003) are the backstop if the feature is enabled by accident. +- `subtle = "2.5"` — constant-time equality primitive. +- `zeroize = { version = "1.7", features = ["derive"] }` — direct dep where a `#[derive(Zeroize, ZeroizeOnDrop)]` is needed, and (separately) used by `crates/miden-keystore` to wrap its disk-read buffer as `zeroize::Zeroizing>` (see FR-011 and the dep-direction note below). +- `static_assertions = "1.1"` (dev-dependency) — compile-time `assert_not_impl_any!` checks. + +**Dep-direction note (FR-011 / contracts)**: the `crates/server/src/secret` wrapper module is `pub(crate)` and never reachable from `crates/miden-keystore`. To satisfy FR-001 inside `miden-keystore` without inverting the dependency graph, the keystore's transient disk-read buffer uses **`zeroize::Zeroizing>`** directly (a tiny dep already in the workspace lock graph via the crypto stack). `Zeroizing` provides the zero-on-drop contract; the keystore does not need the no-`Display`/no-`Serialize` posture of the server-only wrappers because its buffers are stack-local and never reach a formatter or serializer in normal code paths. This keeps `server → miden-keystore` as the only dependency direction. +**Primary Dependencies (existing, untouched)**: `tracing`, `serde`, `serde_json`, `axum`, `diesel_async`, `miden-keystore` (local crate), `miden-protocol` (external). +**Storage**: N/A — this feature does not touch persisted data. Existing Postgres/filesystem backends are unchanged. +**Testing**: `cargo test -p guardian-server` (unit tests in-tree), plus targeted compile-fail tests (either `trybuild` or a documented `#[cfg(compile_fail)]` snippet — see Phase 0 decision). +**Target Platform**: Linux server (existing AWS ECS deployment; no platform-specific code added). +**Project Type**: Server-side library refactor inside `crates/server`. No new binary, no new HTTP/gRPC surface. +**Performance Goals**: Zeroize-on-drop overhead must be negligible at session-expiry rates (well under 1 ms per dropped session token, which is comfortable headroom over the per-session work the server already does). No measurable change in p95 request latency. +**Constraints**: No change to the public HTTP/gRPC surface. No change to the SDK crates (`crates/client`, `crates/miden-multisig-client`) or to the TypeScript packages. Server wrappers (`SecretBox`-backed types in `crates/server/src/secret`) stay strictly inside the server crate. The independent local change in `crates/miden-keystore` uses `zeroize::Zeroizing>` directly — no shared module, no inverted dependency. +**Scale/Scope**: ~7 in-scope storage sites across 5 modules (cursor, dashboard state, evm session, builder/storage, evm config, ACK signers, secrets-manager fetch). Estimated ~300–500 LOC change including tests. + +## Constitution Check + +*GATE: Must pass before Phase 0 research. Re-evaluated post-Phase-1 design — see end of this section.* + +The Guardian Constitution v1.1.0 lists five principles plus system invariants. This feature is an internal, server-only, type-system refactor with no wire-contract changes. Each principle is evaluated below. + +| Principle | Status | Justification | +|---|---|---| +| **I. Bottom-Up Change Propagation** | ✅ Pass (vacuous) | The change is contained in `crates/server`. There are no downstream contract changes that need to propagate to Rust/TS clients, multisig SDKs, or examples. The wrapper types are private to the server crate (or live in a server-local sub-module) and never appear in a public response or in a serialized form on the wire. | +| **II. Transport and Cross-Language Parity** | ✅ Pass (vacuous) | No HTTP/gRPC surface change. No client-observable behavior change. Rust and TypeScript clients are not modified. | +| **III. Append-Only Integrity and Explicit Lifecycles** | ✅ Pass (vacuous) | No change to state, delta, proposal records, lifecycle transitions, fallback paths, or status enums. | +| **IV. Explicit Authentication and Stable Boundary Errors** | ✅ Pass | This feature *strengthens* the existing auth posture (constant-time HMAC verify on cursors, scoped zeroization of session tokens). No change to error surfaces, payload shapes, status enums, or signature schemes. Tests in the dashboard / evm session modules are updated where the equality path changes; no downstream consumer is affected because the wrappers do not cross the response boundary. | +| **V. Evidence-Driven Delivery** | ✅ Pass | Three independently testable user stories (disclosure / erasure / timing) are defined in the spec. The Phase 1 quickstart documents the validation steps. Tests added: compile-time non-impl assertions (SC-002 / SC-003), runtime `{:?}` redaction assertions (SC-005), drop-zero sanity check (SC-007), structural review for response/config boundary (SC-009). | + +**System Invariants**: none are affected. Per-account auth remains explicit. Replay protection is unchanged (this feature does not touch nonce/digest *semantics* — challenges were explicitly removed from scope in the second review round). Filesystem/Postgres backend semantics are unchanged. Storage shapes are unchanged. + +**Initial Constitution Check: PASS.** + +**Post-Phase-1 Constitution Re-Check**: PASS (no new violations introduced — see end of plan). + +## Project Structure + +### Documentation (this feature) + +```text +speckit/features/008-zeroize-secrets/ +├── plan.md # This file +├── spec.md # Feature specification (already complete) +├── research.md # Phase 0 output (crate selection, keystore boundary verification, redaction pattern) +├── data-model.md # Phase 1 output (wrapper type taxonomy + in-scope-site mapping) +├── contracts/ # Phase 1 output (internal wrapper-type API contract) +│ └── secret-module.md +├── quickstart.md # Phase 1 output (developer onboarding + verification steps) +├── checklists/ +│ └── requirements.md # Already complete +└── tasks.md # Phase 2 output (NOT created here — produced by /speckit-tasks) +``` + +### Source Code (repository root) + +```text +crates/server/src/ +├── secret/ # NEW — wrapper types live here +│ ├── mod.rs # re-exports + module-level docs +│ ├── wrappers.rs # FixedKey, SecretBytes, SecretString, CredentialUrl +│ ├── ct.rs # constant-time equality helper (single named fn) +│ └── tests.rs # compile-time assertions + Debug-redaction tests +│ +├── dashboard/ +│ ├── cursor.rs # MODIFIED — CursorSecret now wraps FixedKey<32>; HMAC verify continues to use hmac::Mac::verify_slice (constant-time per RustCrypto); manual Debug redaction removed +│ └── state.rs # MODIFIED — sessions keyed by sha256(token); token bytes not retained in memory after Set-Cookie response +│ +├── evm/ +│ ├── config.rs # MODIFIED — EvmChainConfig.rpc_url is CredentialUrl +│ └── session.rs # MODIFIED — sessions keyed by sha256(token); same shape change as dashboard/state.rs +│ +├── builder/ +│ └── storage.rs # MODIFIED — StorageMetadataBuilder.database_url is CredentialUrl +│ +├── ack/ +│ ├── miden_falcon_rpo/signer.rs # MODIFIED — keystore field unchanged; FR-011 verification recorded in research.md +│ ├── miden_ecdsa/signer.rs # MODIFIED — same +│ └── secrets_manager.rs # MODIFIED — fetched bytes returned as SecretBytes; cache field (if present) holds SecretBytes +│ +└── (NO public API changes — no api/ files modified) + +crates/miden-keystore/src/ +├── keystore.rs # MODIFIED — read_key_file's local key_bytes is zeroize::Zeroizing>; FR-011 verification noted in a code comment +└── ecdsa_keystore.rs # MODIFIED — same + +speckit/features/008-zeroize-secrets/ +└── (artifacts listed above) +``` + +**Structure Decision**: Single-project Rust workspace. All new code lives in a new `crates/server/src/secret/` module. Modifications to existing modules are surgical (changing one field type and adjusting a small set of call sites per site). Cross-crate touch is limited to the in-repo `miden-keystore` and only if FR-011 verification requires it. + +## Phase 0 Output + +See [`research.md`](./research.md) — covers: +- Crate selection: `secrecy` vs hand-rolled, `zeroize` direct vs transitive, `subtle` vs `constant_time_eq`. +- FR-011 verification: does `miden-protocol::SecretKey` zeroize on drop, and if not, what is the adapter shape. +- Pattern for compile-time non-impl assertions (`static_assertions` vs `trybuild` vs `#[deny(...)]` lints). +- Pattern for the `Debug` redaction in enclosing structs that already derive `Debug`. +- FR-007 guard mechanism choice (manual reviewer checklist plus audit recipe). +- A small decision on whether `SecretString` for session tokens warrants a 32-byte `FixedKey<32>` variant instead (mostly stylistic). + +## Phase 1 Outputs + +See: +- [`data-model.md`](./data-model.md) — wrapper-type taxonomy, the in-scope inventory mapped to a wrapper variant per site, and the migration order with blast-radius rationale. +- [`contracts/secret-module.md`](./contracts/secret-module.md) — the internal API contract for the `crates/server/src/secret` module: public surface, allowed/forbidden trait impls, constructor signatures, exposure signatures, constant-time helper signature, redaction-marker constant. +- [`quickstart.md`](./quickstart.md) — how a developer adds a new secret field, how to run the test suite for this feature, and how a reviewer enumerates exposure call sites. + +## Complexity Tracking + +> Fill ONLY if Constitution Check has violations that must be justified. + +No violations. Table omitted. + +## Post-Phase-1 Constitution Re-Check + +After completing Phase 1 design (data model, contracts, quickstart): + +- **I.** Still server-internal. No downstream effect. +- **II.** No transport surface change. Wrappers explicitly forbidden from crossing the response/config boundary by spec FR-003 + SC-009. +- **III.** No lifecycle or append-only change. +- **IV.** Auth posture strengthened (constant-time HMAC verify, session-token zeroization). Existing tests in `dashboard/state.rs` and `evm/session.rs` are updated to thread the new wrapper through their fixtures; no upstream consumer is affected. +- **V.** Evidence plan complete: see quickstart.md and the test list in data-model.md. + +**Re-Check: PASS.** diff --git a/speckit/features/008-zeroize-secrets/quickstart.md b/speckit/features/008-zeroize-secrets/quickstart.md new file mode 100644 index 00000000..59579f80 --- /dev/null +++ b/speckit/features/008-zeroize-secrets/quickstart.md @@ -0,0 +1,144 @@ +# Quickstart: Memory-Resident Secret Hygiene + +**Feature**: `008-zeroize-secrets` +**Audience**: developers adding code that touches secrets in `crates/server`; reviewers auditing the secret surface. + +--- + +## TL;DR for new code + +If you are adding a field that holds a long-lived secret value, do these three things: + +1. **Pick a wrapper** from `crates/server/src/secret/`: + - 32-byte (or other fixed-size) symmetric key → `FixedKey` + - opaque variable-length bytes → `SecretBytes` + - secret string (token, password) → `SecretString` + - URL that may embed credentials (userinfo or query) → `CredentialUrl` +2. **Construct via `Wrapper::new(...)`** at the point where the bytes first arrive from the environment / disk / network. **For env vars (FR-012)**: fold the `std::env::var(...)` call and the wrapper constructor into a *single expression* — no intermediate `String` local, no `let raw = env::var(...)?;` two-step. + + ```rust + // ✅ Right + let db_url = CredentialUrl::new(std::env::var("DATABASE_URL")?); + + // ❌ Wrong — `raw` lives un-zeroized on the heap + let raw = std::env::var("DATABASE_URL")?; + let db_url = CredentialUrl::new(raw); + ``` +3. **Read via `.expose_secret()` only at the single point of use** (sign, HMAC, build pool, etc.). Drop the wrapper as soon as possible. + +> **Note on env-var protection limits**: this feature protects the Rust-side destination of an env-var read. It does **not** protect the OS process environment block (`/proc//environ`, coredumps, fork-inherited env, ECS task-definition env). For values that must not appear in the env block at all, use the AWS Secrets Manager runtime-fetch pattern (see `ack/secrets_manager.rs`), which is already how production Falcon/ECDSA keys are loaded. + +Do **not**: +- Log or `{}`-format the wrapper. The compiler will refuse (`Display` is not implemented). +- Serialize the wrapper. The compiler will refuse (`Serialize` is not implemented). +- Add `Serialize` to an enclosing struct that holds a wrapper. The derive will fail to compile. +- Compare a wrapper byte-by-byte against untrusted input with `==`. Use `secret::ct::eq` instead. + +--- + +## Running the tests for this feature + +```bash +cargo test -p guardian-server --lib secret +cargo test -p guardian-server --lib dashboard::cursor +cargo test -p guardian-server --lib dashboard::state +cargo test -p guardian-server --lib evm::session +cargo test -p miden-keystore +``` + +The compile-time `static_assertions::assert_not_impl_any!` checks run on every `cargo build` — no special step required. + +--- + +## Verifying the security posture (audit recipe) + +A reviewer can confirm the feature is intact in ~30 seconds: + +```bash +# 1. Enumerate every legitimate exposure of a wrapped secret. +rg -n 'expose_secret\(' crates/server/src crates/miden-keystore/src + +# 2. Confirm no wrapper crosses the serialization boundary. +rg -nE '#\[derive\([^)]*Serialize' crates/server/src | rg -i 'secret|credential|fixed_?key' +# Expected: zero hits. + +# 3. Confirm no Display impl on any wrapper. +rg -nE 'impl\s+(std::fmt::)?Display\s+for\s+(FixedKey|SecretBytes|SecretString|CredentialUrl)' crates/server/src +# Expected: zero hits. + +# 4. Enumerate constant-time equality sites. +rg -n 'secret::ct::eq|secret::ct_eq|ct::eq\(' crates/server/src + +# 5. Confirm every secret-bearing env-var read wraps in a single expression (FR-012). +rg -n 'env::var\("(DATABASE_URL|GUARDIAN_DASHBOARD_CURSOR_SECRET|GUARDIAN_EVM_RPC_URLS)"\)' crates/server/src +# Each hit should be on a line that also contains `CredentialUrl::new(` / `FixedKey::<32>::new(` / +# similar — no intermediate `let` binding between env::var and the wrapper constructor. + +# 6. Confirm no one introduced `unsafe { env::remove_var(...) }` to "wipe" the env block. +rg -n 'unsafe\s*\{\s*std::env::(remove_var|set_var)' crates/server/src crates/miden-keystore/src +# Expected: zero hits outside of test setup/teardown code. +``` + +This grep recipe plus reviewer attention is the FR-007 review process. The compile-time `assert_not_impl_any!` / +`assert_impl_all!` checks in `secret/tests.rs` are the hard enforcement. + +### Baseline counts (at landing) + +| Check | Expected count | +|---|---| +| `expose_secret(` call sites across `crates/server` + `crates/miden-keystore` | ~35 | +| `impl Display for ` | 0 | +| `#[derive(Serialize)]` on a struct that contains a wrapper | 0 | +| `secret::ct::eq` call sites (tests only — no production callers in this feature) | ~11 | +| `unsafe { std::env::(remove_var\|set_var) }` outside test scaffolding | 0 (all hits are in `EnvVarGuard` test helpers and `network::mod::tests`) | + +A meaningful drift in any of the first three is worth investigating. + +If step (1) returns more results than the implementation tracker expects, or steps (2)–(3) return any results, something has regressed. + +--- + +## Adding a new in-scope secret site + +If a future change introduces a new long-lived secret (e.g. a future bearer-token cache, an in-memory KMS handle that holds raw material): + +1. **Amend `spec.md`'s "In-Scope Inventory"** to add the site by name. The spec is the canonical scope. +2. **Pick a wrapper** per the TL;DR above. +3. **Update `data-model.md`'s site mapping table**. +4. **Add the site to the audit recipe expected counts** (if a follow-on tracker exists). +5. **Re-run the audit recipe above** and update the expected counts if the new site adds an `expose_secret()` call or another reviewed secret-bearing field. + +If the new secret needs to cross a serialization boundary (e.g. it must be persisted to disk encrypted-at-rest), this feature is the wrong layer — file a separate spec because the disclosure model is different. + +--- + +## Removing a secret site + +If a secret becomes unnecessary (e.g. a session-token store is replaced by a JWT scheme): + +1. Remove the field and the surrounding code. +2. Remove the site from `data-model.md`'s mapping table. +3. Update `spec.md`'s In-Scope Inventory. +4. Leave the wrapper types in the `secret` module — they are reusable. + +--- + +## Local development tips + +- `tracing::info!(?cfg, ...)` will compile and render redacted output for fields that are wrappers; for fields that are plain `String`/`Vec`, the value renders verbatim. So if you see a real secret in logs after this feature lands, the field is the bug — not the logging call. +- The `Debug` impl includes a length where it is safe to (e.g. `SecretBytes(len=32)`). This is intentional and useful for debugging without leaking content. +- `CredentialUrl::scheme_and_host()` is the right thing to log when you want to know *which* RPC endpoint or DB host you connected to. Never log the full URL. + +--- + +## When to break glass + +If you genuinely need the raw secret value (signing a payload, computing an HMAC, constructing a connection pool): + +```rust +let pool = build_pool(database_url.expose_secret())?; +``` + +That single call is the audit-visible exposure. Reviewers grep for `expose_secret(` and read each site. Keep these call sites few and obvious. + +If you find yourself wanting the raw secret in more than one place, consider whether the work that needs the secret can be moved closer to the wrapper construction (smaller exposure window) or expressed as a method on the wrapper itself. diff --git a/speckit/features/008-zeroize-secrets/research.md b/speckit/features/008-zeroize-secrets/research.md new file mode 100644 index 00000000..f092fbf7 --- /dev/null +++ b/speckit/features/008-zeroize-secrets/research.md @@ -0,0 +1,198 @@ +# Phase 0 Research: Memory-Resident Secret Hygiene + +**Feature**: `008-zeroize-secrets` +**Date**: 2026-05-29 + +This document resolves the NEEDS-CLARIFICATION items from `plan.md`'s Technical Context and records the Phase 0 decisions that drive the Phase 1 design. + +--- + +## Decision 1 — Crate selection and exact version pins + +**Decision**: Pin the following exact version lines in `Cargo.toml`: + +```toml +secrecy = { version = "0.10", default-features = false } # NO serde feature +subtle = "2.5" +zeroize = { version = "1.7", features = ["derive"] } +static_assertions = "1.1" # dev-dependency, in [dev-dependencies] +``` + +**Rationale**: +- `secrecy` 0.10 is the current line on docs.rs (0.10.3 at time of writing); the 0.8 → 0.10 API consolidated around `SecretBox` and removed the older `SecretString` / `SecretVec` aliases in favor of explicit `SecretBox` / `SecretBox>`. Our four wrapper newtypes (`FixedKey`, `SecretBytes`, `SecretString`, `CredentialUrl`) wrap `SecretBox<_>` so the API shape is stable to our callers regardless of upstream renames. +- `default-features = false` is critical: `secrecy`'s `serde` feature, when enabled, derives `Deserialize` on `SecretBox` for any `T: Deserialize`. That would violate FR-003 by adding a deserialization path. The exact-pin combined with `default-features = false` makes "no Serialize / no Deserialize on a wrapper" hold without further work; the `static_assertions::assert_not_impl_any!` block (SC-002 / SC-003) is the **backstop** that converts any future regression — including someone enabling the `serde` feature — into a compile error. +- `secrecy` 0.10 provides: + - No `Display` impl on `SecretBox` — satisfies FR-002. + - No `Serialize` / `Deserialize` impl unless `serde` feature is enabled — we leave it off; satisfies FR-003. + - `Debug` redacted to `SecretBox<…>` — satisfies FR-002. + - `expose_secret()` accessor — satisfies SC-006. + - `Drop` zeroizes via the inner `T: Zeroize` — satisfies FR-001. +- `subtle` 2.5 — `ConstantTimeEq` returns a `Choice`, which avoids accidental short-circuit branching. Already in the workspace's transitive graph. +- `zeroize` 1.7 with `features = ["derive"]` — needed for `#[derive(Zeroize, ZeroizeOnDrop)]` on `FixedKey`'s `[u8; N]` and for `Zeroizing>` in the `miden-keystore` change. +- `static_assertions` 1.1 (dev-dep) — compile-time `assert_not_impl_any!`. + +**Alternatives considered**: +- **Allow the `secrecy/serde` feature**: rejected. Even opt-in, enabling a workspace-wide feature is a poor regression boundary (any other crate in the workspace using `secrecy` would inherit the impl). Wrapper bytes are reachable through `expose_secret()` and explicitly serialized at the call site if ever needed. +- **`secrecy` 0.8**: older API; using the current 0.10 line aligns with docs.rs and avoids a future migration. +- **Hand-rolled wrapper**: rejected as in the previous round. +- **`constant_time_eq` instead of `subtle`**: rejected (`subtle` is more misuse-resistant). + +--- + +## Decision 2 — FR-011 keystore-boundary verification (dep-direction safe) + +**Decision**: The Falcon and ECDSA signing-key bytes live inside `miden-protocol::crypto::dsa::*::SecretKey`, which is an **external** crate. We will: + +1. Inspect the upstream `miden-protocol` `SecretKey` types in Phase 2 (during implementation) and record one of two outcomes in a `// Zeroization: upstream` comment at each load site: + - **(a) Confirmed upstream**: cite the upstream source line that shows `SecretKey` implements `Zeroize` / `ZeroizeOnDrop` (or has an explicit `Drop` that zeros). No local adapter needed. + - **(b) Not confirmed**: file a follow-on upstream issue. The local file-read buffer wrap (step 3) still bounds exposure to that one buffer, and the existing per-call load-and-drop pattern bounds the `SecretKey` itself to the signing-call frame. + + **Implementation outcome (recorded 2026-05-29)**: branch **(a)** for both signing schemes. The pinned `miden-protocol 0.14.5` re-exports types from `miden-crypto 0.23.0`. Verified by direct source inspection of the crate cached at `~/.cargo/registry/src/index.crates.io-*/miden-crypto-0.23.0/`: + - **Falcon** — `src/dsa/falcon512_poseidon2/keys/secret_key.rs` declares `impl ZeroizeOnDrop for SecretKey {}` (around line 80) with a matching `impl Drop for SecretKey { fn drop(&mut self) { self.zeroize(); } }`. The earlier session-memory observation that Falcon "historically did not zeroize" predates this revision; the pinned crate does implement it. + - **ECDSA** — `src/dsa/ecdsa_k256_keccak/mod.rs` declares `impl ZeroizeOnDrop for SecretKey {}` (around line 119), delegating to the inner `k256::ecdsa::SigningKey`'s `ZeroizeOnDrop`. + + The signer call sites (`crates/server/src/ack/miden_falcon_rpo/signer.rs:sign_with_server_key` and `crates/server/src/ack/miden_ecdsa/signer.rs:sign_with_server_key`) carry citation comments referencing these locations. If `miden-protocol` is later bumped to a version that pulls in a different `miden-crypto` line, the citations must be re-verified. +2. The current keystore code (verified in this research phase) already drops the loaded `SecretKey` at end-of-scope in `sign()` / `ecdsa_sign()` (`crates/miden-keystore/src/keystore.rs:145–150` and `ecdsa_keystore.rs:133–138`). No persistent in-memory cache holds the loaded key between calls. This bounds the exposure window to the duration of one signing call. +3. Wrap the on-disk `key_bytes: Vec` read in `read_key_file` (`keystore.rs:107`, `ecdsa_keystore.rs:103`) in **`zeroize::Zeroizing>`** — **not** the server-side `SecretBytes` wrapper. Rationale: `crates/miden-keystore` is a lower-layer crate that must not depend on the server's private `secret` module (the dependency would invert the layering). `Zeroizing>` is a thin `Deref`-based zero-on-drop wrapper from the `zeroize` crate, which `miden-keystore` already pulls in transitively via the crypto stack. It gives us FR-001 (zero on drop) at the boundary; we do not need the no-`Display`/no-`Serialize` posture for a stack-local file-read buffer that is never logged or serialized. + +**Rationale**: +- The actual byte storage is in an upstream crate we do not control day-to-day. The constitution does not let us promise behavior we cannot enforce, so the plan records the verification step as a Phase 2 task and provides a fallback (the adapter) that does not depend on upstream changes. +- The disk-read buffer is local; wrapping it is cheap and covers FR-001 without depending on upstream. +- Keeping the existing per-call load-and-drop pattern is already favorable — no persistent in-memory copy of the key bytes outside the signing call. + +**Alternatives considered**: +- **Upstream patch to `miden-protocol`**: rejected for this feature. Out of scope, separately owned, and would gate this work on an external release. If verification falls into the "(b) not confirmed" branch, the upstream patch becomes a follow-on issue, not a blocker. +- **Skip the disk-read buffer wrap**: rejected. Cheap to do, closes a real gap independent of upstream. + +--- + +## Decision 3 — Compile-time non-impl assertions: `static_assertions::assert_not_impl_any!` + +**Decision**: Use `static_assertions::assert_not_impl_any!` for SC-002 / SC-003. Each wrapper type ships a `tests` module with: +```rust +static_assertions::assert_not_impl_any!(FixedKey<32>: fmt::Display, serde::Serialize, serde::Deserialize); +static_assertions::assert_not_impl_any!(SecretString: fmt::Display, serde::Serialize, serde::Deserialize); +// etc. +``` +These are compile-time assertions — adding a `Display` or `Serialize` impl later will fail `cargo build`. + +**Rationale**: +- `static_assertions` is small, widely used (`>50M` downloads), and produces a clear compile error pointing at the assertion line. +- It runs at every build (including CI), not only when tests are explicitly invoked. + +**Alternatives considered**: +- **`trybuild` compile-fail tests**: rejected as primary mechanism. `trybuild` is excellent for proving a *specific user error* fails to compile, but it requires committing the failing-snippet files and running them in a special harness. `assert_not_impl_any!` is lighter and tests the type directly. We may still use a single `trybuild` test for the "user derives `Serialize` on an enclosing struct that contains a wrapper → compile error" scenario, since that one is about user code shape rather than the wrapper itself. +- **`#[deny(...)]` lints**: no built-in lint covers "this type must not implement this trait". + +--- + +## Decision 4 — `Debug` redaction pattern for enclosing structs + +**Decision**: `secrecy`'s `SecretBox` already redacts itself in `Debug`. For enclosing structs that already derive `Debug` (e.g. anything that the `tracing` macros render with `?value`), no change is required — the field renders as `SecretBox<…>` automatically because `SecretBox: Debug` regardless of `T`. For enclosing structs that hand-implement `Debug` (rare in this codebase), the implementer routes the secret field through `?` or skips it; either is acceptable because the wrapper's own `Debug` impl is the binding contract. + +**Rationale**: +- Matches the spec's relaxed acceptance scenario 2: enclosing structs are not required to gain a `Debug` impl they don't already have, and where they do have one they inherit redaction through the wrapper. +- No bespoke macro or proc-macro derive needed in the server crate. + +**Alternatives considered**: +- **`#[derive(Zeroize, ZeroizeOnDrop, Debug)]` with a custom `Debug` per wrapper**: redundant; `SecretBox` already does this. + +--- + +## Decision 5 — FR-007 guard mechanism: manual review checklist + documented audit recipe + +**Decision**: Implement FR-007 via the spec's **manual-review arm**: + +1. **Reviewer checklist**: a bullet in `CONTRIBUTING.md` asking reviewers to confirm "new secret-bearing fields are wrapped in `secret::*` types, and new env-var reads use the single-expression construct-and-wrap pattern". +2. **Documented audit recipe**: the grep recipe in `quickstart.md` lets any reviewer enumerate, in ~30 seconds, every `expose_secret(` site, confirm no `Display`/`Serialize` on a wrapper, and check that the three secret env vars wrap in a single expression (FR-012 / SC-010). + +**Rationale**: +- The **hard** enforcement already lives in the compile-time `static_assertions::assert_not_impl_any!` (no `Display`/`Serialize` on wrappers) and `assert_impl_all!` (response DTOs keep `Serialize`) checks — these make the dangerous regressions (logging, serializing, or DTO-embedding a secret) a build failure. +- FR-007 is satisfied by the manual checklist plus the compile-time assertions. The spec-forbidden reflection path is still ruled out. + +**Alternatives considered**: +- **Custom clippy lint**: rejected for this feature; too large a step for the value (this is a finite inventory). +- **`cargo deny` rule**: not the right tool — `cargo deny` checks dependencies, not field types. + +--- + +## Decision 6 — Session-token storage: key by `sha256(token)`, do not retain the plaintext + +**Decision**: Restructure the operator and EVM session maps to key by a non-secret 32-byte digest of the token. Specifically: + +```rust +type SessionKey = [u8; 32]; // sha256(token) +type DashboardSessions = HashMap; +``` + +The plaintext token is generated, written to the `Set-Cookie` header for the response, and **dropped** — never stored as a map key, never retained in a record field. On subsequent requests, the server computes `sha256(candidate)` and uses standard `HashMap::get`. + +**Rationale**: +- Resolves the dep-FR-001 conflict the previous design carried. Leaving the token as the `HashMap` key would leave it un-zeroized in heap memory — the spec's FR-001 covers any long-lived secret in memory, including map keys. The previous "wrap only the value side" plan did not actually wrap the token because the token *is* the key, not a field of the value. +- A 32-byte digest is not a secret: given the 256-bit-entropy random token, the digest reveals nothing about the token that the token's entropy doesn't already shield. The digest cannot be used to authenticate without inverting SHA-256. +- No constant-time compare is needed: `HashMap::get` is a structural lookup over the digest, not a byte-by-byte equality against a stored secret. The digest equality check happens on a value derived from a hashed transform of the candidate; the timing oracle that motivates constant-time compare does not apply. +- Side benefit: a coredump or heap inspection of the long-lived session map captures only digests, not tokens. The plaintext token is still constructed by the cookie-issue handler — it is `format!`-embedded into the `Set-Cookie` header string and included in the `IssuedOperatorSession` / `VerifiedEvmSession` response payload — so it exists in memory for the duration of that request handler (request-scoped, not "hundreds of nanoseconds"). That request-scoped lifetime is out of strict zeroization scope per the spec's general Out-of-Scope. The point of the digest-keyed map is to ensure the token does not persist *beyond* the issuing request. + +**Implementation notes**: +- `sha256(token)` uses the existing `sha2` dep (already in the workspace via the crypto stack). +- The plaintext token does not need to be a `SecretString` because its lifetime is now strictly local to one request handler (generation → digest → cookie write → drop). If the implementation discovers a code path that retains the plaintext (e.g. for refresh tokens), that path is wrapped in `SecretString` at that time. +- Test fixtures that previously held the plaintext token to assert insertion success now insert via the public constructor and look up via the same constructor — the digest is an implementation detail of the store. + +**Alternatives considered**: +- **Wrap only the value side (previous design)**: rejected. Does not address FR-001 because the token *is* the key. Caught by second-round review. +- **`HashMap` (wrap the map key with `Hash` + `Eq` derives)**: rejected. Would require implementing `Hash` and `Eq` on a wrapper whose whole point is to be opaque, defeating the no-`Display`/no-`Serialize`/no-equality posture and creating surface for accidental leak. +- **`FixedKey<32>` for tokens (wrap raw bytes)**: rejected. Tokens are currently hex strings; converting the cookie API to raw bytes would expand the change without value. +- **Amend the spec to exclude session-token map keys from FR-001**: rejected as a posture regression. The token is a clear long-lived secret in memory; keeping FR-001 honest is the better answer. + +--- + +## Decision 7 — Wrapper-type taxonomy: four named types + +**Decision**: The `secret` module exports four named types: + +| Type | Purpose | Mapped sites | +|---|---|---| +| `FixedKey` | Fixed-size symmetric key material (HMAC keys, etc.). Wraps `[u8; N]`. | `CursorSecret` (N=32). | +| `SecretBytes` | Variable-length opaque bytes. Wraps `Vec`. | AWS Secrets Manager fetched bytes; the disk-read buffer in `miden-keystore::read_key_file`. | +| `SecretString` | Variable-length secret string. Wraps `String`. | Operator session token; EVM session token. | +| `CredentialUrl` | A URL that may embed credentials in userinfo or query. Wraps `String`. | `EvmChainConfig.rpc_url`; `StorageMetadataBuilder.database_url`. | + +All four are built on `secrecy::SecretBox` underneath (or `Secret` for `Copy` types) so they share the no-`Display`, no-`Serialize`, redacted-`Debug`, zero-on-drop, `expose_secret()` contract. + +**Rationale**: +- Distinct names give grep/audit precision ("how many `FixedKey<32>` instances exist? where are `CredentialUrl`s built?") which a single `SecretBox` everywhere would not. +- Keeps the spec's "Distinct variants may exist for fixed-size key / variable-length token / credential URL" alive in the type system. +- Four is enough to cover the inventory without proliferation. + +**Alternatives considered**: +- **One generic wrapper `Secret`**: rejected. Loses the search-ergonomic distinction between credential URLs (which look like config strings) and session tokens (which look like opaque IDs). +- **A separate `SecretJsonValue` for AWS Secrets Manager**: rejected for now. The returned bytes are short-lived and converted to a `SecretKey` (Falcon/ECDSA) at the call site; `SecretBytes` is sufficient. + +--- + +## Decision 8 — Cursor HMAC verification: keep `hmac::Mac::verify_slice` + +**Decision**: The cursor HMAC verify at `crates/server/src/dashboard/cursor.rs:303` uses `mac.verify_slice(tag)` from the `hmac` crate. **No change**. We do **not** replace it with `secret::ct::eq`. + +**Rationale**: +- The RustCrypto `hmac` crate documents `Mac::verify_slice` as performing the comparison in constant time. It is the canonical and audited primitive for this exact check. +- Replacing a vetted MAC verify with a home-grown byte equality call would weaken the API contract (we'd be inferring intent rather than calling the named verification API) and adds risk for no benefit. The reviewer is right to flag this. +- The migration step for `CursorSecret` is limited to (a) re-typing the field as `FixedKey<32>`, (b) routing the `new_from_slice(secret)` call through `secret.expose_secret()`, and (c) removing the now-redundant manual `Debug` redaction at `cursor.rs:240-246`. + +**Code-comment addition (Phase 2)**: at the `verify_slice` call site, add a one-line comment citing the constant-time property to make the audit trail explicit: +```rust +// HMAC tag verification: hmac::Mac::verify_slice is documented constant-time +// (RustCrypto hmac crate). See [feature 008 research.md decision 8]. +mac.verify_slice(tag).map_err(|_| ...) +``` + +**Alternatives considered**: +- **Replace `verify_slice` with `secret::ct::eq` over the computed tag bytes**: rejected. Throws away an audited API for a home-rolled one. The `secret::ct::eq` helper still exists for the cases (current or future) where a raw byte-equality compare against untrusted input is needed *outside* a MAC-verification API. + +--- + +## Open follow-ups (deferred to implementation / future features) + +- **TLS server private key**: not in scope; gated on the server adding in-process TLS termination. +- **`miden-protocol::SecretKey` upstream zeroization**: if Decision 2 lands in branch (b), file a follow-on issue against `miden-protocol` asking for `ZeroizeOnDrop` on `SecretKey`. Not a blocker for this feature. +- **Possible cleanup**: `CursorSecret` already has a manual `Debug` redaction (cursor.rs:240-246). After migration, the manual impl is removed in favor of the wrapper's built-in redaction — net code reduction. diff --git a/speckit/features/008-zeroize-secrets/spec.md b/speckit/features/008-zeroize-secrets/spec.md new file mode 100644 index 00000000..7c29c498 --- /dev/null +++ b/speckit/features/008-zeroize-secrets/spec.md @@ -0,0 +1,141 @@ +# Feature Specification: Memory-Resident Secret Hygiene + +**Feature Branch**: `008-zeroize-secrets` +**Created**: 2026-05-29 +**Status**: Draft +**Input**: User description: "Wrap every long-lived secret value held in Guardian server memory in types that zero their backing buffer on drop (zeroize), refuse accidental Debug/Display/serde exposure (secrecy), and use constant-time comparison where equality checks are reachable from untrusted input (subtle). Defense-in-depth against accidental log leaks, coredump exposure, swap-file persistence of stale buffers, serialization mistakes, and timing side-channels — not a substitute for OS-level isolation, but a meaningful raise of the floor." + +## User Scenarios & Testing *(mandatory)* + +### User Story 1 — Eliminate accidental disclosure of long-lived secrets (Priority: P1) + +A Guardian operator is investigating a production incident and pulls server logs, a panic message, or a `serde_json` error trace that includes the offending struct. None of the secret values held in process memory — signing-key material accessed by the ACK signer, the dashboard cursor HMAC secret, operator/EVM session tokens, the database connection URL with embedded password, or EVM RPC URLs with embedded API keys — must appear in the captured output. Any field carrying a long-lived secret cannot be reached by `Display` or by the standard serialization layer *at all*: those impls are absent on the wrapper, so accidental `{}` or `serde_json::to_string` either fails to compile or is impossible to write. `Debug` is allowed only as a non-disclosing redaction. The bytes are reachable solely through an explicit, named "exposure" method. + +**Why this priority**: Accidental log/serialize/panic disclosure is the highest-frequency class of secret exfiltration in long-running services. Closing this surface protects every other layer of the system at low implementation cost and lands real value on day one. + +**Independent Test**: A reviewer adds a compile-time test (`static_assertions::assert_not_impl_any!`) for each wrapper type asserting it does not implement `Display` and does not implement `serde::Serialize`. A second test renders each wrapper through `{:?}` and a panic message and asserts the output contains only the redaction marker. + +**Acceptance Scenarios**: + +1. **Given** any wrapper type defined by this feature, **When** a developer writes `format!("{}", secret)` or `serde_json::to_string(&secret)` or derives `Serialize` on an enclosing struct that contains a wrapper, **Then** the code fails to compile (the wrapper does not implement `Display` or `Serialize`). +2. **Given** any wrapper type, **When** it is formatted with `{:?}`, **Then** the output is an opaque redaction marker and never the underlying bytes, hex, or URL credentials. Enclosing structs that *already* derive or implement `Debug` and contain a wrapper field inherit this redaction through the wrapper's `Debug` impl; this feature does not require enclosing structs to gain a `Debug` impl they did not previously have. +3. **Given** a `panic!` whose payload is a `String` produced from a wrapper via `{:?}`, **When** the panic message is rendered to stderr or a structured log, **Then** the secret value is not present in the payload. +4. **Given** the public HTTP response types and configuration types reachable from the server's handlers, **When** a structural test or reviewer audit walks those types' fields, **Then** none of them transitively contain a wrapper — secret types live only on internal state, never on the response/config boundary. +5. **Given** an operator or developer who needs the secret value to use it (signing, HMAC verification, pool construction), **When** they call the explicit `expose_secret()` (or equivalent) method, **Then** they receive the underlying bytes and the call site is grep-able from a security audit. + +--- + +### User Story 2 — Erase secrets from process memory promptly (Priority: P1) + +When a long-lived secret is dropped — process shutdown, session expiry/rotation, configuration reload, or simply going out of scope — its backing buffer is overwritten with zeros before the allocator reclaims it. This shrinks the window in which a coredump, swap-file fragment, or post-free heap inspection could yield the secret. + +**Why this priority**: Coredump and swap exposure are realistic risks for any process holding cryptographic material; the cost of zeroization-on-drop is negligible and the benefit is concrete. + +**Independent Test**: A unit test constructs a secret type, drops the value, and asserts the type implements the zeroize contract (via the chosen crate's trait bound) and that every storage site in the inventory uses a zeroizing wrapper. + +**Acceptance Scenarios**: + +1. **Given** a populated session map containing operator/EVM session tokens, **When** a session expires and is evicted from the map, **Then** the token's backing string buffer is zeroed before deallocation. +2. **Given** a running server, **When** the process receives `SIGTERM` and proceeds through orderly shutdown, **Then** every secret-bearing field in `AppState` is dropped and zeroized. +3. **Given** an ACK signer that loads private-key bytes into memory to perform a single signing operation, **When** the signing call returns, **Then** the intermediate buffer holding the loaded key material is zeroized before the function returns control to the caller. + +--- + +### User Story 3 — Resist timing side-channels on byte-by-byte secret equality checks (Priority: P2) + +Anywhere the server compares a secret value to a value derived from untrusted input using **byte-by-byte equality** — HMAC tag verification on signed cursors, nonce match against the stored expected value, any future bearer-token / API-key comparison — the comparison runs in time independent of which byte differs. This denies a network attacker the ability to learn a secret one byte at a time by measuring response latency. + +**Scope clarification (binding)**: this feature changes the session-token map storage shape so that the **key is a non-secret digest** of the token (e.g. `sha256(token)`) and the token itself is not retained in memory after the Set-Cookie response. Lookup is structural over the digest, not byte-by-byte against a stored secret, and no constant-time compare is needed on that path. The mechanics of the storage-shape change are recorded in `plan.md` / `data-model.md`. If a future refactor reintroduces storing tokens by-value, byte-equality compare against untrusted input, or any other shape that does perform byte-by-byte equality against a stored token, **that refactor must add a constant-time compare**, and this spec must be amended accordingly. + +**Why this priority**: Timing-side-channel attacks against high-entropy random tokens have low realistic payoff, but the cost of a constant-time compare on the in-scope sites is negligible and the change is mechanical. + +**Independent Test**: An audit greps for `==`, `eq_ignore_ascii_case`, `String::eq`, and `[u8]::eq` against any value held in a wrapper type at the in-scope sites (HMAC verification, nonce echo match), and confirms every such site routes through a single named constant-time helper. + +**Acceptance Scenarios**: + +1. **Given** a signed pagination cursor with an attached HMAC tag, **When** the server verifies the tag against the computed tag, **Then** the comparison runs in time independent of the position of the first byte difference. (The existing implementation uses `hmac::Mac::verify_slice`, which RustCrypto documents as constant-time; no change is required here other than to confirm the citation in code and `research.md`.) +2. **Given** a session-cookie lookup, **When** the server resolves it, **Then** the lookup is keyed by a non-secret digest of the candidate token and no byte-by-byte equality against a stored secret occurs. +3. **Given** any future byte-by-byte equality site introduced by later work, **When** it compares a wrapped secret against untrusted input, **Then** it MUST route through the feature's named constant-time helper or the equivalent canonical primitive from the relevant crypto crate. + +--- + +### Edge Cases + +- A secret value is `Clone`d to hand to a background task: the clone must own an independent buffer; dropping one must not zero the other's storage. +- A secret is held inside an `Arc`: zeroize-on-drop fires on last-reference drop. This is acceptable but must be documented so reviewers do not assume earlier release. +- A panic occurs while a secret is on the stack between load and use: the wrapper's `Drop` must run during unwinding and zero the buffer. +- A secret reaches the logging or tracing layer through `Display` (e.g. someone writes `tracing::info!(token = %tok, ...)`): this MUST fail to compile because the wrapper does not implement `Display`. The redaction marker is reachable only through `{:?}` (`Debug`). +- An `Err(_)` variant carrying a secret-bearing struct is rendered via `Debug` (typically by `tracing::error!(?err, ...)` or `format!("{err:?}")`): the error rendering must surface only the redaction marker for the secret field; the rest of the error struct may render normally. +- A panic payload is constructed from a secret-bearing struct via `format!("{secret:?}")` (the only formatter available): the panic message contains only the redaction marker. Note that Rust does **not** print local variables in standard backtraces, so the panic-message path is the realistic surface to defend. +- A test that needs to assert on a secret's value reaches through the explicit exposure method; such call sites are audit-visible and acceptable. +- A secret value that travels through `String` / `Vec` before being wrapped: this spec covers the wrapped form going forward but does not retroactively zeroize bytes that lived in transient containers prior to wrapping. The design should minimize such transient containers. +- A field name is ambiguous between "credential URL" and "configuration URL" (e.g. an RPC endpoint that may or may not embed an API key): the spec treats it as a secret by default; downgrading requires explicit reviewer sign-off. + +## Requirements *(mandatory)* + +### In-Scope Inventory + +These long-lived secret-bearing locations are in scope. Names are authoritative; cited paths may drift with refactors. + +- **Falcon signing-key material** accessed via `MidenFalconRpoSigner.keystore` (`crates/server/src/ack/miden_falcon_rpo/signer.rs`) — loaded private-key bytes for the duration of a sign call. **Dependency boundary**: the actual key bytes live inside `miden-keystore`'s `FilesystemKeyStore` and are handled by external `miden-*` types during `self.keystore.sign(...)`. This feature owns only what this repository controls — the `Arc>` field, any cached metadata, and any local copy made *before* the call into the external API. The external types' zeroization is out of this feature's hands; see FR-011 for the verification requirement. +- **ECDSA signing-key material** accessed via `MidenEcdsaSigner.keystore` (`crates/server/src/ack/miden_ecdsa/signer.rs`). Same dependency-boundary caveat as Falcon: the sign path delegates to `self.keystore.ecdsa_sign(...)`; this feature owns the surrounding field and any local copies, not the keystore-internal buffers. +- **Transient key material inside `AwsSecretsManagerProvider`** (`crates/server/src/ack/secrets_manager.rs`): the `secret_hex: String` returned by `secret_string()` and the `secret_bytes: Vec` decoded inside `parsed_secret_key()`. These are stack-local within a single fetch+parse call (no cache exists today) but carry full Falcon/ECDSA private-key material in the call frame and warrant defense-in-depth wrapping as an explicit exception to the request-scoped Out-of-Scope rule. The *secret IDs* themselves are not secrets. If a cache is added later, it must use the same wrappers. +- **Dashboard cursor HMAC secret** (`CursorSecret`, `crates/server/src/dashboard/cursor.rs`) — 32-byte process-scoped HMAC key; currently has manual `Debug` redaction but no zeroize. +- **Operator session tokens** held in `DashboardState.sessions` (`crates/server/src/dashboard/state.rs`). +- **EVM session tokens** held in `EvmSessionState.sessions` (`crates/server/src/evm/session.rs`). +- **Database connection URL** flowing through `StorageMetadataBuilder.database_url` (`crates/server/src/builder/storage.rs`) and any field that retains the URL after pool construction. +- **EVM RPC URLs** in `EvmChainConfig.rpc_url` (`crates/server/src/evm/config.rs`) — treated as credentials because they often embed API keys. + +### Out-of-Scope + +- Request-scoped stack-local values that do not outlive a single function call (e.g. a signature byte slice received in a request and verified within the same handler). +- TLS server private keys: the server does not currently terminate TLS in-process. If TLS termination is added later, that material must be added to this scope as a follow-on. +- Anything held only in the Rust / TypeScript SDKs consumed by external users (`crates/client`, `crates/miden-multisig-client`, npm packages). The SDKs have their own threat model. +- Disk-at-rest encryption of keystore files. +- OS-level mitigations (`mlock`, secure enclaves, KMS-side signing). Explicitly framed as out of scope; this feature is "raise the floor", not "replace OS isolation". +- **The OS process environment block.** Linux exposes the entire env block at `/proc//environ` for the lifetime of the process; coredumps capture it; child processes fork-inherit it by default; ECS task definitions, `docker run -e`, and dotenvy-loaded `.env` files all populate this block before Rust runs. Wrapping a secret env-var value *after* `std::env::var` reads it cannot retroactively zero the env block. Mitigating this is an infrastructure concern (prefer AWS Secrets Manager runtime fetch over env-var injection, which is already how the Falcon and ECDSA keys are loaded in prod). This feature deliberately does not call `unsafe { std::env::remove_var(...) }` after reads — the threat reduction is small relative to the process-global `unsafe` cost. +- **Operator pending challenges** (`dashboard/types.rs`'s `signing_digest` and the surrounding `PendingChallenge`) and **EVM pending challenges** (`EvmChallenge` in `evm/session.rs`). Rationale: (1) these values are intentionally sent to the client in the challenge response (otherwise the client cannot sign them), so they are not memory-only secrets in the same sense as session tokens or HMAC keys; (2) the EVM stored type *is* the public response DTO (`api/evm.rs`), and FR-003 forbids wrappers from crossing the response/config boundary. Wrapping them would force a split into separate internal-storage vs public-DTO types, which is a larger refactor than this feature warrants for a value that is already disclosed to the holder by design. **Forward-pointer for reviewers**: the existing `pending.challenge.nonce.eq_ignore_ascii_case(nonce)` compare at `crates/server/src/evm/session.rs:113` is a non-constant-time equality against an untrusted-input nonce. This is a known consequence of keeping challenges out of scope (the nonce is already disclosed to the client in the prior challenge response, so a timing oracle reveals only a value the holder already has). It is deliberately not addressed here. If a future change converts a challenge into a server-only secret (e.g. a server-side proof material that is never returned), both the challenge type and that comparison must be brought into scope by an amendment to this spec. + +### Functional Requirements + +- **FR-001**: Every in-scope secret MUST be stored as a type that overwrites its backing buffer with zeros when dropped. +- **FR-002**: Every in-scope secret wrapper MUST NOT implement `std::fmt::Display`. `Debug` MAY be implemented but MUST render only a non-disclosing redaction marker; the underlying bytes/string MUST NOT be reachable through any formatter trait. +- **FR-003**: Every in-scope secret wrapper MUST NOT implement `serde::Serialize` or `serde::Deserialize`. Enclosing structs that derive `Serialize` MUST NOT contain a wrapper field (this is structurally enforced: the derive will fail to compile because the wrapper does not implement the trait). The only path to the bytes is an explicit named exposure method. +- **FR-004**: Every **byte-by-byte equality** check that compares a wrapped secret against a value derived from an untrusted input source MUST run in time independent of the position of the first byte difference. Sites that already use a canonical constant-time primitive from a crypto crate (e.g. `hmac::Mac::verify_slice`) satisfy this requirement without further change, provided the constant-time property is documented at the call site. Session-token lookups satisfy this requirement structurally because the map is keyed by a non-secret digest (see User Story 3); the spec MUST be amended if any future refactor reintroduces byte-by-byte equality against a stored token. +- **FR-005**: Cloning a secret-bearing wrapper MUST produce an independently-owned buffer such that dropping one does not zero the other's storage. +- **FR-006**: Where a secret is held inside a shared smart pointer, zeroization MUST fire on last-reference drop; the wrapper choice MUST be compatible with shared ownership. +- **FR-007**: A documented review guard MUST exist for any new field whose name matches secret-ish patterns (`*_key`, `*_secret`, `*_token`, `password`, `*_hmac`, and credential-bearing `*_url`) and is not wrapped in an approved secret type. The guard is the manual reviewer checklist in `CONTRIBUTING.md` plus the `quickstart.md` audit recipe. "Reflection over `AppState`" is **not** an acceptable mechanism — Rust does not provide runtime field reflection without custom derive/codegen, and chasing that path is out of scope. +- **FR-008**: Every legitimate "expose" call site MUST be grep-able by a single fixed substring so a security review can enumerate all such sites in seconds. +- **FR-009**: Removal of the wrapper from an in-scope field MUST require a change visible at the type level — i.e. you cannot regress by editing a single line in the field definition without it being obvious to a reviewer. +- **FR-010**: The chosen crate dependencies MUST be widely-used, actively maintained, and acceptable under the project's existing dependency policy. +- **FR-011**: For each in-scope secret whose actual byte storage lives in an external dependency (notably the `miden-keystore` crate path used by the Falcon and ECDSA ACK signers), the plan MUST either (a) document a verification that the external type already zeroizes its buffers on drop, with a citation to the relevant upstream code, or (b) introduce a constrained local adapter that copies the bytes into a local wrapper for the duration of use and zeroizes the copy on drop, with the original boundary documented. The spec does not promise zeroization of memory owned by external crates beyond this verification. +- **FR-012**: Every read of a secret-bearing environment variable MUST construct the wrapper in a single expression at the read site (e.g. `CredentialUrl::new(std::env::var(NAME)?)`). No intermediate `String` may be held in a config-struct field or local binding between the `env::var` call and the wrapper construction. This minimizes the in-process window during which the env-var value lives as an un-zeroized `String`. The OS-level env block remains exposed (see Out-of-Scope) — this requirement bounds the *Rust-side* exposure window only. + +### Key Entities + +- **Secret wrapper type** — A named newtype around a byte buffer (or string buffer) that owns its bytes, zeroizes them on drop, **redacts `Debug` to a non-disclosing marker, and omits `Display` and `serde::Serialize` / `serde::Deserialize` impls entirely**, and exposes the inner value only through an explicit named method. Distinct variants may exist for "fixed-size key", "variable-length token", and "credential URL". +- **Constant-time-equality helper** — A named function or trait method that takes two byte slices (or two wrapped secrets) and returns equality computed in constant time. +- **Exposure call site** — A specific audit-visible expression of the form `.expose_secret()` (or chosen equivalent) that hands out the underlying bytes for a *single* legitimate operation (signing, HMAC, pool construction). +- **In-scope inventory** — The enumerated list above. Treated as the authoritative scope; future additions require a follow-on spec or a security-review-blessed edit to this document. + +### Assumptions + +- The Rust ecosystem provides crates that cover all three behaviours (zeroization, non-disclosing wrapper, constant-time equality). Specific package selection is a planning-phase decision recorded in `plan.md`, not here. +- The performance overhead of zeroization-on-drop is negligible at the rates these secrets are dropped (session expiry, shutdown). +- The codebase is willing to give up the ability to print configuration structs verbatim in startup logs; any startup logging that previously dumped a config struct will be replaced with explicit field-by-field logging that names only safe fields. +- Manual redaction of `Debug` impls on enclosing structs is acceptable; alternatively, the enclosing struct opts into a derive provided by the chosen crate. + +## Success Criteria *(mandatory)* + +### Measurable Outcomes + +- **SC-001**: 100% of the in-scope inventory is wrapped in a secret type that zeroizes on drop, with a single audit-visible exposure method per access site. +- **SC-002**: For each wrapper type, a compile-time assertion (`static_assertions::assert_not_impl_any!`) confirms it does not implement `Display`. Manual `Debug` impls on every wrapper render only the redaction marker; this is asserted by a runtime test in the standard server test suite. +- **SC-003**: For each wrapper type, a compile-time assertion confirms it does not implement `serde::Serialize` or `serde::Deserialize`. A reviewer running a one-line grep for `#[derive(...Serialize...)]` on enclosing structs that hold a wrapper finds zero results (and would not compile if added). +- **SC-004**: Every byte-by-byte equality site in the in-scope inventory either (a) uses the existing constant-time primitive provided by its canonical crypto crate (e.g. `hmac::Mac::verify_slice` for HMAC tag verification, which is documented constant-time) or (b) routes through a single named constant-time helper in this feature's module. Both paths are grep-able and reviewed. Session-token map-lookup sites are out of scope per the storage-shape decision in `plan.md` (sessions are keyed by a non-secret digest, so equality is structural, not against a stored secret). +- **SC-005**: A test in the standard server test suite asserts that, for each wrapper type, formatting it with `{:?}` produces only the redaction marker. The same test confirms (by virtue of compilation) that `Display` and `Serialize` are not available paths. +- **SC-006**: A security reviewer can enumerate every legitimate exposure of a wrapped secret by grepping for a single token (e.g. `expose_secret`); the resulting list matches the inventory above with no surprises. +- **SC-007**: At least one verification — a manual heap-inspection check during development, or a test exercising the wrapper's drop behaviour — confirms that secret buffers are zeroed before the process exits. Recorded as a sanity check, not an ongoing assertion. +- **SC-008**: For every in-scope secret whose bytes live inside `miden-keystore` or another external crate, the plan records either an upstream-code citation showing the external type already zeroizes on drop, or the design of the local adapter that copies-and-zeros bytes at the boundary (per FR-011). +- **SC-009**: The compile-time non-impl assertions from SC-002 / SC-003 (wrappers do not implement `Serialize`) **combine transitively** with the existing `#[derive(Serialize)]` on every public HTTP response DTO to make "a wrapper field in a response DTO" a compile error. SC-009 is satisfied by: (a) keeping the existing `#[derive(Serialize)]` on response DTOs (asserted by `static_assertions::assert_impl_all!(Dto: serde::Serialize)` for a representative sample of DTOs, so removing the derive also fails CI); plus (b) the SC-002 / SC-003 non-impl assertions on wrappers. No separate "structural walk" is needed — the assertion mechanism is already a compile error. +- **SC-010**: For each secret-bearing env var read by the server (`DATABASE_URL`, `GUARDIAN_DASHBOARD_CURSOR_SECRET`, `GUARDIAN_EVM_RPC_URLS`, plus any added later), `env::var("")` is followed directly by a wrapper constructor call in the same expression — the env-var result is never bound to a local `String` or passed through an intermediate non-wrapper builder field. This is verified by the audit-recipe grep in `quickstart.md` and the reviewer checklist. diff --git a/speckit/features/008-zeroize-secrets/tasks.md b/speckit/features/008-zeroize-secrets/tasks.md new file mode 100644 index 00000000..d7177eaa --- /dev/null +++ b/speckit/features/008-zeroize-secrets/tasks.md @@ -0,0 +1,179 @@ +# Tasks: Memory-Resident Secret Hygiene + +**Feature**: `008-zeroize-secrets` +**Input**: Design documents from `speckit/features/008-zeroize-secrets/` +**Prerequisites**: `plan.md`, `spec.md`, `research.md`, `data-model.md`, `contracts/secret-module.md`, `quickstart.md` + +**Tests**: Included where the spec explicitly requires them (`assert_not_impl_any!`, redacted-`Debug` tests, session-storage-shape tests, DTO `assert_impl_all!`, etc.). Test tasks are kept colocated with the implementation tasks that produce the asserted behavior. + +**Organization**: Tasks grouped by user story so each P1 story can land as an independently-testable increment. P2 (timing) is a small follow-up. + +## Format: `[ID] [P?] [Story] Description` + +- **[P]**: Parallelizable — touches a different file from the other [P] tasks in the same phase, with no incomplete-task dependencies. +- **[Story]**: `[US1]`, `[US2]`, `[US3]` per spec.md. +- File paths are absolute relative to repo root. + +## Path Conventions + +Single-project Rust workspace. Server code under `crates/server/src/`; one cross-crate change under `crates/miden-keystore/src/`. No new top-level directories. + +--- + +## Phase 1: Setup (Shared Infrastructure) + +**Purpose**: Bring in the chosen crate dependencies and create the module skeleton. No behavior change yet. + +- [X] T001 Add server-side dependencies to `crates/server/Cargo.toml`: `secrecy = { version = "0.10", default-features = false }`, `subtle = "2.5"`, `zeroize = { version = "1.7", features = ["derive"] }`, and `static_assertions = "1.1"` under `[dev-dependencies]`. Verify the `secrecy/serde` feature is **not** enabled anywhere in the workspace. +- [X] T002 Add `zeroize = { version = "1.7" }` to `crates/miden-keystore/Cargo.toml` (or confirm it is already pulled in by the existing crypto stack; explicit direct-dep is preferred for visibility). +- [X] T003 Create the empty module skeleton at `crates/server/src/secret/mod.rs`, `crates/server/src/secret/wrappers.rs`, `crates/server/src/secret/ct.rs`, and `crates/server/src/secret/tests.rs` (empty stubs; no impls yet). Add `mod secret;` to `crates/server/src/lib.rs` so the module compiles. + +**Checkpoint**: `cargo build -p guardian-server -p miden-keystore` succeeds; no behavior change. + +--- + +## Phase 2: Foundational (Blocking Prerequisites) + +**Purpose**: Implement the four wrapper types, the `ct::eq` helper, and the compile-time + runtime test suite. This phase delivers the *contract* of US1 + US2 for the wrapper types themselves (no site is migrated yet — that lands in Phases 3 / 4). + +⚠️ **CRITICAL**: No user-story phase may start until this phase is complete. + +- [X] T004 [P] Implement `FixedKey` in `crates/server/src/secret/wrappers.rs` per `contracts/secret-module.md`: inner `secrecy::SecretBox<[u8; N]>`, `new`, `expose_secret`, hand-rolled `Clone` (fresh `SecretBox` over copied array), redacted `Debug` (`FixedKey(…)`), hand-rolled `PartialEq`/`Eq` via `subtle::ConstantTimeEq`. No `Display`, no `Serialize`/`Deserialize`. +- [X] T005 [P] Implement `SecretBytes` in `crates/server/src/secret/wrappers.rs` per `contracts/secret-module.md`: inner `secrecy::SecretBox>`, `new`, `expose_secret`, `len`, hand-rolled `Clone` / `Debug` (`SecretBytes(len=N)`) / `PartialEq` / `Eq`. +- [X] T006 [P] Implement `SecretString` in `crates/server/src/secret/wrappers.rs` per `contracts/secret-module.md`: inner `secrecy::SecretBox`, `new`, `expose_secret`, `len`, hand-rolled `Clone` / `Debug` (`SecretString(len=N)`) / `PartialEq` / `Eq`. +- [X] T007 [P] Implement `CredentialUrl` in `crates/server/src/secret/wrappers.rs` per `contracts/secret-module.md`: inner `secrecy::SecretBox`, `new`, `expose_secret`, `scheme_and_host` (returns `scheme://host[:port]` with userinfo/path/query stripped; `` fallback), hand-rolled `Clone` / `Debug` (`CredentialUrl()`) / `PartialEq` / `Eq` via `subtle`. **Required** for `EvmChainConfig` and `EvmChainRegistry` which derive `PartialEq, Eq`. +- [X] T008 Implement `secret::ct::eq` in `crates/server/src/secret/ct.rs`: `#[allow(dead_code)] pub(crate) fn eq(a: &[u8], b: &[u8]) -> bool` using `subtle::ConstantTimeEq`, returning `bool` only as the final action. Public to the crate; zero callers in this feature (Decisions 6 + 8). +- [X] T009 Add `pub(crate)` re-exports in `crates/server/src/secret/mod.rs` (`pub(crate) use wrappers::{FixedKey, SecretBytes, SecretString, CredentialUrl}; pub(crate) use ct::eq as ct_eq;`). Add `#[cfg(test)] mod tests;`. +- [X] T010 [P] Add compile-time non-impl assertions in `crates/server/src/secret/tests.rs` using `static_assertions::assert_not_impl_any!` for each of `FixedKey<32>`, `SecretBytes`, `SecretString`, `CredentialUrl`: `Display`, `serde::Serialize`, `serde::Deserialize<'static>`. (Satisfies SC-002, SC-003.) +- [X] T011 [P] Add runtime tests in `crates/server/src/secret/tests.rs`: `debug_redacts` (each wrapper's `{:?}` output contains only the redaction marker), `clone_independent` (constructed-cloned-dropped-original, clone still readable), `eq_uses_constant_time` (correctness of `==` on equal and differing contents), `ct_eq_distinguishes` (correctness of `secret::ct::eq` — also keeps it out of dead-code reach), `credential_url_scheme_and_host_safe` (for `postgres://user:pass@host:5432/db` and `https://api.example.com/?key=abc`, `scheme_and_host()` returns scheme+host+port only). (Satisfies SC-005.) + +**Checkpoint**: `cargo test -p guardian-server --lib secret` passes. Wrapper types are usable from any module inside `guardian-server`. + +--- + +## Phase 3: User Story 1 — Eliminate Accidental Disclosure (Priority: P1) 🎯 MVP + +**Goal**: Migrate the in-scope inventory sites where the dominant threat is accidental log/serde/panic disclosure. After this phase, the wrapper *capability* delivered in Phase 2 is applied at every in-scope production site, and the SC-009 transitive DTO guard is in place. + +**Independent Test**: Wrapper types do not implement `Display` or `Serialize` (compile-time, from Phase 2). Each in-scope field that previously held a raw secret is now a wrapper. `format!("{:?}", server_state)` (where available) renders secrets as redacted markers. `#[derive(Serialize)]` on any public DTO that holds a wrapper fails to compile. + +### Implementation for User Story 1 + +- [X] T012 [P] [US1] Migrate `CursorSecret` to wrap `FixedKey<32>` in `crates/server/src/dashboard/cursor.rs`. Replace the existing newtype-around-`[u8; 32]` shape with a struct holding `FixedKey<32>`. **Remove** the manual `Debug` redaction at `cursor.rs:240-246` (now redundant — the wrapper's `Debug` does the redaction). HMAC verify at `cursor.rs:303` continues to call `hmac::Mac::verify_slice`; the call must now pass `cursor_secret.expose_secret()` to `Hmac::::new_from_slice`. +- [X] T013 [P] [US1] In `crates/server/src/dashboard/config.rs:47`, fold the `std::env::var("GUARDIAN_DASHBOARD_CURSOR_SECRET")` read, the hex decode, and the `FixedKey::<32>::new(...)` constructor into a single expression (FR-012) — no intermediate `String` or `[u8; 32]` local between the read and the wrapper. +- [X] T014 [P] [US1] Migrate `EvmChainConfig.rpc_url` to `CredentialUrl` in `crates/server/src/evm/config.rs:9`. In the env-var parser at `evm/config.rs:77`, fold `std::env::var(RPC_URLS_ENV)` and `CredentialUrl::new(...)` into a single expression per FR-012. Adjust the `EvmChainConfig` struct's `#[derive(... PartialEq, Eq)]` — it must still compile because `CredentialUrl` now implements those traits (T007). `EvmChainRegistry`'s `PartialEq, Eq, Default` derives at line 16 also continue to compile. +- [X] T015 [US1] Migrate `StorageMetadataBuilder.database_url` to `Option` in `crates/server/src/builder/storage.rs:32`. Rewrite the env-var read at `storage.rs:79` to fold `std::env::var("DATABASE_URL")` and `CredentialUrl::new(...)` into a single expression (FR-012); the `unwrap_or_default()` chain is restructured so the env-var `String` is consumed by `CredentialUrl::new` in the same expression. Update downstream call sites that pass the URL to pool construction to call `.expose_secret()`. +- [X] T016 [P] [US1] Update `crates/server/src/audit/postgres.rs:264` to read `DATABASE_URL` and construct `CredentialUrl` in a single expression (FR-012). Pool construction reads `.expose_secret()` at the connection-pool builder boundary. +- [X] T017 [P] [US1] Replace any startup logging that printed the full `DATABASE_URL` or any RPC URL with calls to `url.scheme_and_host()`. Search `crates/server/src` for `tracing::info!(.*database_url|.*rpc_url|.*GUARDIAN_EVM_RPC|DATABASE_URL=)`. Each hit logs the safe view instead. (Supports SC-001, SC-002.) +- [X] T018 [US1] Add the SC-009 transitive DTO guard. Identify a representative sample of public HTTP response DTOs reachable from the server's handlers (e.g. dashboard list/detail response, EVM session-create response, storage info response — sample chosen to span the modules that hold wrappers). Add `static_assertions::assert_impl_all!(: serde::Serialize)` for each, placed either in `crates/server/src/secret/tests.rs` or in a sibling `tests` module of each DTO. Combined with T010, this makes "wrapper field in a public DTO" a compile error. +- [X] T019 [US1] Update tests in `crates/server/src/dashboard/cursor.rs`, `crates/server/src/evm/config.rs`, `crates/server/src/builder/storage.rs` that previously constructed fields with plain `String` / `[u8; 32]` to now construct via the wrapper types. Existing assertion semantics (e.g. equality of two `EvmChainConfig` instances) continue to hold because the wrappers implement `PartialEq` (T004–T007). + +**Checkpoint**: `cargo test -p guardian-server` passes. Every in-scope disclosure-threat field is wrapped. Public DTO Serialize derives still compile (they hold no wrapper fields). + +--- + +## Phase 4: User Story 2 — Erase Secrets from Process Memory Promptly (Priority: P1) + +**Goal**: Migrate the sites whose dominant threat is residual memory exposure (coredump, swap, post-free heap). Includes the **session-storage shape change** (Decision 6 — token is digest-keyed, never retained), the AWS Secrets Manager transient wrap, and the keystore disk-read buffer wrap (FR-011). + +**Independent Test**: After this phase, session expiry / eviction leaves no plaintext token bytes in the map's heap representation. The keystore's disk-read buffer zeroes on drop (`Zeroizing>`). The AWS Secrets Manager fetch wraps its transient hex/bytes locals. + +### Implementation for User Story 2 + +- [X] T020 [US2] Restructure operator session storage in `crates/server/src/dashboard/state.rs:28-32`: change the map type to `HashMap<[u8; 32], OperatorSessionRecord>` keyed by `sha256(token)` (use the existing `sha2` workspace dep). Update the cookie-issue handler at `state.rs` (around the `Set-Cookie` write, current line ~259-260) so the plaintext token is generated, used to build the `Set-Cookie` header and the response payload, and goes out of scope at end-of-handler — it is **not** inserted as a map key. Update lookup at `state.rs:296` to compute `sha256(candidate)` and use standard `HashMap::get` on the digest. Note: a plain `String::drop` does not zeroize — the token is request-scoped and out of strict zeroization scope per spec; optionally wrap the intermediate plaintext in `SecretString` for the handler's duration as a tightening. +- [X] T021 [P] [US2] Restructure EVM session storage in `crates/server/src/evm/session.rs:17-20` and `evm/session.rs:134-145` (and the lookup at `evm/session.rs:161`) to follow the same digest-keyed pattern as T020. `EvmSessionState.sessions` becomes `HashMap<[u8; 32], EvmSessionRecord>`; the plaintext token is request-scoped. +- [X] T022 [P] [US2] Wrap the AWS Secrets Manager transient values in `crates/server/src/ack/secrets_manager.rs` (`secret_string` and `parsed_secret_key`, around lines 42-78). The `secret_hex: String` result of the cloud fetch becomes `SecretString`; the `secret_bytes: Vec` decoded via `hex::decode` becomes `SecretBytes`. The parser closure receives `secret_bytes.expose_secret()` (`&[u8]`). **No public-return-type change** — `parsed_secret_key` still returns the parsed `FalconSecretKey` / `EcdsaSecretKey` as before. Both wrappers go out of scope at function return and zeroize. Mark this site as the explicit Out-of-Scope exception (stack-local but full-key-bearing) in a brief code comment. +- [X] T023 [P] [US2] Wrap the disk-read buffer in `crates/miden-keystore/src/keystore.rs:107` with `zeroize::Zeroizing>` — **not** the server's `SecretBytes` (dep direction would invert). The wrap is immediate (`let key_bytes = Zeroizing::new(std::fs::read(path)?);`), and `SecretKey::read_from_bytes` is called with `&*key_bytes` (the `Deref` view). Add a code comment recording the FR-011 verification result: `// Zeroization: upstream miden-protocol::SecretKey {verified at | unverified — local Zeroizing> handles file-read buffer}`. +- [X] T024 [P] [US2] Same wrap pattern as T023 for the disk-read buffer at `crates/miden-keystore/src/ecdsa_keystore.rs:103`. Same FR-011 code-comment requirement. +- [X] T025 [US2] Add tests in `crates/server/src/dashboard/state.rs` (test module) and `crates/server/src/evm/session.rs` (test module): `session_lookup_by_digest` (insert with token, look up by same token returns record; mismatched token returns `None`) and `session_token_not_retained` (after insertion, the map's keys have length 32 and are not the issued token's bytes). +- [X] T026 [US2] Phase-2 verification step for FR-011: inspect the upstream `miden-protocol::SecretKey` (Falcon and ECDSA) source and record one of the two outcomes in code comments at `crates/server/src/ack/miden_falcon_rpo/signer.rs` and `crates/server/src/ack/miden_ecdsa/signer.rs` (next to the `self.keystore.sign(...)` / `ecdsa_sign(...)` calls): (a) upstream `SecretKey` is `Zeroize` / `ZeroizeOnDrop` with a citation, or (b) unverified — local `Zeroizing>` from T023/T024 handles the file-read buffer; file follow-on issue against upstream. (Records SC-008.) + +**Checkpoint**: `cargo test -p guardian-server -p miden-keystore` passes. Session maps no longer contain plaintext tokens. Keystore disk-read buffers zeroize on drop. AWS Secrets Manager transients are wrapped. + +--- + +## Phase 5: User Story 3 — Resist Timing Side-Channels (Priority: P2) + +**Goal**: Document the constant-time posture of byte-by-byte equality sites against untrusted input. In practice: cite `hmac::Mac::verify_slice` at the cursor-HMAC site, confirm `ct::eq` is in place for future sites, and document the deliberate non-coverage of `evm/session.rs:113`. + +**Independent Test**: A grep + code-review pass over the in-scope inventory confirms each byte-equality site against untrusted input routes through a canonical constant-time primitive (`verify_slice`) or `secret::ct::eq`. The `evm/session.rs:113` `eq_ignore_ascii_case` site is documented in the spec's Out-of-Scope (already in place). + +### Implementation for User Story 3 + +- [X] T027 [US3] Add a one-line code comment at `crates/server/src/dashboard/cursor.rs:303` citing the constant-time property of `hmac::Mac::verify_slice` per RustCrypto's `hmac` crate documentation, with a back-reference to `speckit/features/008-zeroize-secrets/research.md` Decision 8. +- [X] T028 [US3] Confirm the `ct_eq_distinguishes` test from T011 is part of the standing test suite and exercises `secret::ct::eq` with at least one equal-input case and one differing-input case at varied positions. This both validates the helper and prevents the `#[allow(dead_code)]` from masking bit-rot. + +**Checkpoint**: All FR-004 sites are accounted for (citation, helper, or documented Out-of-Scope). No code change other than comments and a confirmed-existing test. + +--- + +## Phase 6: Polish & Cross-Cutting Concerns + +**Purpose**: FR-007 / FR-012 manual-review guard, `CONTRIBUTING.md` updates, end-to-end validation, smoke test. + +- [~] T029 **Removed.** Reserved for the deleted automated review helper; FR-007 is satisfied by the reviewer checklist (T032) plus the `quickstart.md` audit recipe. +- [~] T030 **Removed.** Reserved for the deleted automated review helper. +- [~] T031 **Removed.** Reserved for the deleted automated review helper. +- [X] T032 [P] Update `CONTRIBUTING.md` with: (a) a one-bullet pointer to the `secret` module and the wrapper-choice TL;DR from `quickstart.md`; (b) a reviewer-checklist bullet for "new secret-bearing fields are wrapped in `secret::*` types, and new env-var reads use the single-expression construct-and-wrap pattern". +- [~] T033 **Removed.** The reviewer checklist lives in `CONTRIBUTING.md`. +- [X] T034 [P] Update `docs/CONFIGURATION.md` to document the new env-var handling expectations: every secret-bearing env var is wrapped at the point of read; the OS env block is explicitly Out-of-Scope; production should prefer the AWS Secrets Manager fetch path (already in use for Falcon/ECDSA keys) for the highest-sensitivity material. +- [X] T035 Run the full validation matrix: + - `cargo test -p guardian-server --all-features` — all tests pass. + - `cargo test -p miden-keystore` — all tests pass. + - `cargo clippy -p guardian-server -p miden-keystore -- -D warnings` — clean (verifies the `#[allow(dead_code)]` on `secret::ct::eq` is needed and present). +- [X] T036 Run the operator-dashboard manual smoke harness via the `smoke-test-operator-dashboard` skill: confirm operator challenge → login → list accounts → detail → logout still works end-to-end against the workspace operator client with the digest-keyed session store from T020 / T021. +- [X] T037 Run the full audit recipe from `quickstart.md` (steps 1–7): enumerate `expose_secret(` call sites; confirm zero `Display` impls or `Serialize` derives on wrappers; enumerate `ct_eq` sites; confirm env-var reads are single-expression; confirm no unauthorized `env::remove_var` / `env::set_var`. Record the result counts so future audits have a baseline. + +**Checkpoint**: CI green; smoke test green; audit recipe baseline recorded. Feature ready to merge. + +--- + +## Dependencies + +```text +Phase 1 (Setup) ─┐ + ├─► Phase 2 (Foundational) ─┐ + ├─► Phase 3 (US1) ─┐ + │ ├─► Phase 6 (Polish) + ├─► Phase 4 (US2) ─┤ + │ │ + └─► Phase 5 (US3) ─┘ +``` + +- **Phase 1 → Phase 2**: deps and module skeleton must exist before wrapper impls. +- **Phase 2 → Phases 3 / 4 / 5**: wrapper types and their tests must exist before any site migration. +- **Phases 3 + 4 are independent of each other** and can be implemented in either order or in parallel by two developers, because they touch disjoint files (URLs/cursor/AWS-hex vs sessions/keystore/AWS-bytes — only T022 touches both flows; sequence T022 after either Phase 3 completion or before T020). +- **Phase 5 (US3)** has no implementation dependency on US1/US2 — it can land any time after Phase 2. +- **Phase 6** depends on Phases 3, 4, 5 being complete (the reviewer audit recipe is checked against the post-migration tree; the smoke test exercises the migrated session-store shape). + +### Within-phase parallelization + +**Phase 2 (Foundational)**: T004, T005, T006, T007 are marked `[P]` — all four wrapper types touch the same file (`wrappers.rs`), so they are conceptually parallel work-items but must be merged in one commit per file. T008, T010, T011 are independent files. + +**Phase 3 (US1)**: T012, T013, T014 touch different files and are fully `[P]`. T015 + T016 touch related code paths (storage / postgres audit) and should be sequenced to keep the `Option` type change consistent across them. + +**Phase 4 (US2)**: T020 (dashboard sessions) and T021 (EVM sessions) are `[P]` (different files). T022 (AWS secrets manager), T023 (keystore.rs), T024 (ecdsa_keystore.rs) are all `[P]` against the session work. + +**Phase 6 (Polish)**: T029–T034 are all `[P]` (different files, no overlap). T035–T037 are sequenced at the end. + +--- + +## Implementation Strategy + +### MVP scope (User Story 1 only) + +Ship **Phase 1 + Phase 2 + Phase 3** as the MVP. This delivers: +- The wrapper API with all compile-time and runtime guarantees in place (Phase 2). +- Every in-scope disclosure-threat field (cursor HMAC secret, two URL types) migrated (Phase 3). +- The SC-009 transitive DTO guard (Phase 3, T018). + +At this point the spec's US1 ("eliminate accidental disclosure") is satisfied end-to-end. US2's zeroize-on-drop benefit is **also** delivered for those same fields because the wrappers zeroize on drop — Phase 3 incidentally covers a large chunk of US2's surface. What remains for US2 is the session-store restructure + keystore + AWS transients, which is the heart of Phase 4. + +### Incremental delivery + +After MVP, deliver in this order: +1. **Phase 4 (US2)** completes the erasure story for session tokens, keystore buffers, and cloud-fetched key material. Highest residual security value after MVP. +2. **Phase 5 (US3)** is small (two tasks) and can ride along with Phase 4 or the polish phase. +3. **Phase 6** lands the documentation, reviewer checklist, and audit recipe that prevent future regressions and complete the validation matrix. + +Each phase is independently shippable behind a single merge commit. There is no feature flag — the changes are type-system-level and either compile or do not.