From 737d77f3744e4d6cf68d8f843f724e1c55fc1c3d Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 8 Aug 2026 00:14:20 +1000 Subject: [PATCH 1/6] =?UTF-8?q?feat(encryption):=20keyring=20rotation=20su?= =?UTF-8?q?rface=20=E2=80=94=20previous=5Fmaster=5Fkeys=20+=20sequential?= =?UTF-8?q?=20decrypt=20(LAB-686)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rotating the master key previously invalidated every encrypted entry: the SDK had no rotation surface at all. Implements the stage-2 rs child of the LAB-516 key-rotation train, per protocol/decisions/key-rotation.md and spec/encryption.md 'Key Rotation (Keyring)'. - config: .previous_master_keys(...) builder (hex validation identical to .master_key()) + CACHEKIT_PREVIOUS_MASTER_KEYS env (comma-separated hex). Cap of 3 rejected never truncated; current-key self-collision rejected at build/load (forward-only rotation, detectable subset). - encryption: EncryptionLayer holds cachekit_core::Keyring (the shared stage-1 helper, LAB-683 / core 0.5.0 — no keyring logic re-implemented here). Decrypt attempts keys sequentially, current first, identical AAD per attempt; rs entries carry no per-entry key identity, so the sequential branch is the spec-assigned one (no fingerprint selection). Writes always encrypt under the current key. - client: encryption_from_bytes_with_previous(...) + from_env wiring. - core pin bumped 0.4 -> 0.5 (Keyring ships in 0.5.0). - docs: builder doc-test, README key-rotation section + env table row. Config-level cap constant mirrors core's (feature-gated) constant; drift-guard test asserts equality. --- Cargo.lock | 4 +- README.md | 18 +++ crates/cachekit/Cargo.toml | 2 +- crates/cachekit/src/client.rs | 41 +++++- crates/cachekit/src/config.rs | 150 +++++++++++++++++++--- crates/cachekit/src/encryption.rs | 126 +++++++++++++++++- crates/cachekit/tests/config_tests.rs | 149 +++++++++++++++++++++ crates/cachekit/tests/encryption_tests.rs | 76 +++++++++++ 8 files changed, 540 insertions(+), 26 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index becdf0f..48b324a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -187,9 +187,9 @@ dependencies = [ [[package]] name = "cachekit-core" -version = "0.4.0" +version = "0.5.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6aba1513135a7b92a124ad6983f7e80e5f5c78c4f9c74384079efa3fbf491eab" +checksum = "12089baacc5ff661a62d2071588c895973bc48e42ed359178afaee22decb5559" dependencies = [ "aes", "aes-gcm", diff --git a/README.md b/README.md index d4fed37..d37ce18 100644 --- a/README.md +++ b/README.md @@ -166,6 +166,23 @@ Cross-SDK compatible — ciphertext produced by the Python SDK decrypts with the +### Key Rotation + +Rotate the master key without invalidating existing entries: promote the new key to current and keep the old one as a decrypt-only previous key during a grace window (max 3, per the [protocol keyring spec](https://github.com/cachekit-io/protocol/blob/main/spec/encryption.md)). Writes always use the current key; reads attempt the current key first, then each previous key in order. Old entries age out via TTL or re-encrypt on the next write — no bulk re-encryption. + +```rust +// Env: CACHEKIT_MASTER_KEY= CACHEKIT_PREVIOUS_MASTER_KEYS= +let cache = CacheKit::from_env()?.build()?; + +// Or explicitly on the client builder: +let cache = CacheKit::builder() + .backend(backend) + .encryption_from_bytes_with_previous(&k2_bytes, &[&k1_bytes], "tenant")? + .build()?; +``` + +Rotation is forward-only: a retired key is never re-promoted (re-promoting would resume a used AES-GCM nonce budget), and a config listing the current key among the previous keys is rejected at load. + --- ## Cross-SDK Interop Mode @@ -433,6 +450,7 @@ Requires a tokio runtime for backoff timers (the `redis` and `cachekitio` backen | `CACHEKIT_API_KEY` | ✅ | API key for cachekit.io | | `CACHEKIT_API_URL` | ❌ | Override API endpoint (default: `https://api.cachekit.io`) | | `CACHEKIT_MASTER_KEY` | ❌ | Hex-encoded master key (min 32 bytes) for encryption | +| `CACHEKIT_PREVIOUS_MASTER_KEYS` | ❌ | Comma-separated hex-encoded decrypt-only previous master keys for key rotation (max 3) | | `CACHEKIT_DEFAULT_TTL` | ❌ | Default TTL in seconds (min 1, default: 300) | > [!CAUTION] diff --git a/crates/cachekit/Cargo.toml b/crates/cachekit/Cargo.toml index e5c7b4a..709574c 100644 --- a/crates/cachekit/Cargo.toml +++ b/crates/cachekit/Cargo.toml @@ -45,7 +45,7 @@ reliability = ["tokio/time"] unsync = [] [dependencies] -cachekit-core = { version = "0.4", features = ["messagepack"] } +cachekit-core = { version = "0.5", features = ["messagepack"] } serde = { version = "1", features = ["derive"] } rmp-serde = "1" thiserror = "2.0" diff --git a/crates/cachekit/src/client.rs b/crates/cachekit/src/client.rs index a016d90..6155c6f 100644 --- a/crates/cachekit/src/client.rs +++ b/crates/cachekit/src/client.rs @@ -229,7 +229,13 @@ impl CacheKit { #[cfg(feature = "encryption")] if let Some(ref master_key) = config.master_key { let namespace = config.namespace.as_deref().unwrap_or("default"); - builder = builder.encryption_from_bytes(master_key, namespace)?; + let previous: Vec<&[u8]> = config + .previous_master_keys + .iter() + .map(|key| key.as_slice()) + .collect(); + builder = + builder.encryption_from_bytes_with_previous(master_key, &previous, namespace)?; } Ok(builder) @@ -1004,6 +1010,29 @@ impl CacheKitBuilder { Ok(self) } + /// Configure encryption with decrypt-only previous master keys for + /// key rotation. + /// + /// Writes encrypt under `master_key`; reads attempt it first, then each + /// key in `previous_keys` sequentially (attempt order = slice order). + /// At most 3 previous keys; supplying more is a config error, never + /// truncated. See [`crate::encryption::EncryptionLayer::with_previous_keys`]. + #[cfg(feature = "encryption")] + pub fn encryption_from_bytes_with_previous( + mut self, + master_key: &[u8], + previous_keys: &[&[u8]], + tenant_id: &str, + ) -> Result { + let layer = crate::encryption::EncryptionLayer::with_previous_keys( + master_key, + previous_keys, + tenant_id, + )?; + self.encryption = Some(SharedEncryption::new(layer)); + Ok(self) + } + /// Configure encryption from a hex-encoded master key string. /// /// Convenience wrapper that hex-decodes then delegates to @@ -1025,6 +1054,16 @@ impl CacheKitBuilder { Ok(self) } + #[cfg(not(feature = "encryption"))] + pub fn encryption_from_bytes_with_previous( + self, + _master_key: &[u8], + _previous_keys: &[&[u8]], + _tenant_id: &str, + ) -> Result { + Ok(self) + } + #[cfg(not(feature = "encryption"))] pub fn encryption(self, _hex_key: &str, _tenant_id: &str) -> Result { Ok(self) diff --git a/crates/cachekit/src/config.rs b/crates/cachekit/src/config.rs index b4794d2..fdae66c 100644 --- a/crates/cachekit/src/config.rs +++ b/crates/cachekit/src/config.rs @@ -4,6 +4,14 @@ use zeroize::Zeroizing; use crate::error::CachekitError; +/// Maximum number of decrypt-only previous master keys. +/// +/// Mirrors `cachekit_core::MAX_DECRYPT_ONLY_KEYS` (spec/encryption.md → "Key +/// Rotation (Keyring)"), which is feature-gated behind `encryption` and so +/// cannot be referenced here unconditionally. A drift-guard test in +/// `config_tests.rs` asserts the two stay equal. +pub const MAX_PREVIOUS_MASTER_KEYS: usize = 3; + // ── CachekitConfig ──────────────────────────────────────────────────────────── /// Runtime configuration for a [`crate::client::CacheKit`] instance. @@ -14,6 +22,11 @@ pub struct CachekitConfig { pub api_url: String, /// Master key used for zero-knowledge encryption (AES-256-GCM). pub master_key: Option>>, + /// Decrypt-only previous master keys retained during a rotation grace + /// window, in attempt order. Writes always use `master_key`; reads + /// attempt it first, then these, sequentially. At most + /// [`MAX_PREVIOUS_MASTER_KEYS`] entries. + pub previous_master_keys: Vec>>, /// Default TTL for cache entries when none is specified at call site. pub default_ttl: Duration, /// Optional namespace prefix applied to all cache keys. @@ -41,6 +54,10 @@ impl std::fmt::Debug for CachekitConfig { .field("api_key", &api_key_repr) .field("api_url", &self.api_url) .field("master_key", &master_key_repr) + .field( + "previous_master_keys", + &format_args!("[REDACTED; {}]", self.previous_master_keys.len()), + ) .field("default_ttl", &self.default_ttl) .field("namespace", &self.namespace) .field("l1_capacity", &self.l1_capacity) @@ -55,6 +72,7 @@ impl Default for CachekitConfig { api_key: None, api_url: "https://api.cachekit.io".to_owned(), master_key: None, + previous_master_keys: Vec::new(), default_ttl: Duration::from_secs(300), namespace: None, l1_capacity: 1000, @@ -71,6 +89,7 @@ impl CachekitConfig { /// | `CACHEKIT_API_KEY` | API key for cachekit.io | /// | `CACHEKIT_API_URL` | Override API base URL (must be HTTPS) | /// | `CACHEKIT_MASTER_KEY` | Hex-encoded master key (min 32 bytes) | + /// | `CACHEKIT_PREVIOUS_MASTER_KEYS` | Comma-separated hex-encoded decrypt-only previous master keys (max 3) | /// | `CACHEKIT_DEFAULT_TTL` | Default TTL in seconds (min 1) | pub fn from_env() -> Result { let mut config = Self::default(); @@ -88,19 +107,32 @@ impl CachekitConfig { // Master key — hex-decode and validate length >= 32 bytes if let Ok(val) = std::env::var("CACHEKIT_MASTER_KEY") { - let bytes = hex::decode(&val).map_err(|e| { - CachekitError::Config(format!("CACHEKIT_MASTER_KEY is not valid hex: {e}")) - })?; - if bytes.len() < 32 { - return Err(CachekitError::Config(format!( - "CACHEKIT_MASTER_KEY must be at least 32 bytes ({} hex chars); got {} bytes", - 64, - bytes.len() - ))); - } + let bytes = decode_master_key_hex(&val, "CACHEKIT_MASTER_KEY")?; config.master_key = Some(Zeroizing::new(bytes)); } + // Previous master keys — comma-separated hex, decrypt-only, max 3. + if let Ok(val) = std::env::var("CACHEKIT_PREVIOUS_MASTER_KEYS") { + let mut previous = Vec::new(); + for entry in val.split(',') { + let entry = entry.trim(); + if entry.is_empty() { + return Err(CachekitError::Config( + "CACHEKIT_PREVIOUS_MASTER_KEYS contains an empty entry".to_owned(), + )); + } + previous.push(Zeroizing::new(decode_master_key_hex( + entry, + "CACHEKIT_PREVIOUS_MASTER_KEYS entry", + )?)); + } + validate_previous_master_keys( + config.master_key.as_deref().map(Vec::as_slice), + &previous, + )?; + config.previous_master_keys = previous; + } + // Default TTL — minimum 1 second if let Ok(val) = std::env::var("CACHEKIT_DEFAULT_TTL") { let secs: u64 = val.parse().map_err(|e| { @@ -151,18 +183,55 @@ impl CachekitConfigBuilder { /// Set the master key from a hex string. Must decode to at least 32 bytes. pub fn master_key(mut self, hex_key: &str) -> Result { - let bytes = hex::decode(hex_key) - .map_err(|e| CachekitError::Config(format!("master_key is not valid hex: {e}")))?; - if bytes.len() < 32 { - return Err(CachekitError::Config(format!( - "master_key must be at least 32 bytes; got {}", - bytes.len() - ))); - } + let bytes = decode_master_key_hex(hex_key, "master_key")?; + validate_previous_master_keys(Some(bytes.as_slice()), &self.inner.previous_master_keys)?; self.inner.master_key = Some(Zeroizing::new(bytes)); Ok(self) } + /// Set decrypt-only previous master keys from hex strings, in attempt + /// order. Retained during a key-rotation grace window: reads attempt the + /// current master key first, then each of these sequentially. + /// + /// Validation is identical to [`Self::master_key`] per entry (valid hex, + /// at least 32 bytes). At most [`MAX_PREVIOUS_MASTER_KEYS`] entries — + /// more is a [`CachekitError::Config`], never truncated. The current + /// master key must not reappear here (forward-only rotation: a retired + /// key is never re-promoted). + /// + /// # Examples + /// + /// ``` + /// use cachekit::config::CachekitConfigBuilder; + /// + /// // k2 is current after rotation; k1 stays readable during the grace window. + /// let k1 = "11".repeat(32); + /// let k2 = "22".repeat(32); + /// + /// let config = CachekitConfigBuilder::new() + /// .master_key(&k2)? + /// .previous_master_keys(&[k1.as_str()])? + /// .build(); + /// + /// assert_eq!(config.previous_master_keys.len(), 1); + /// # Ok::<(), cachekit::CachekitError>(()) + /// ``` + pub fn previous_master_keys(mut self, hex_keys: &[&str]) -> Result { + let mut previous = Vec::with_capacity(hex_keys.len()); + for hex_key in hex_keys { + previous.push(Zeroizing::new(decode_master_key_hex( + hex_key, + "previous_master_keys entry", + )?)); + } + validate_previous_master_keys( + self.inner.master_key.as_deref().map(Vec::as_slice), + &previous, + )?; + self.inner.previous_master_keys = previous; + Ok(self) + } + /// Set the default TTL. Must be at least 1 second. pub fn default_ttl(mut self, ttl: Duration) -> Result { if ttl < Duration::from_secs(1) { @@ -194,6 +263,51 @@ impl CachekitConfigBuilder { // ── Helpers ─────────────────────────────────────────────────────────────────── +/// Hex-decode a master key and require at least 32 bytes. Shared by the +/// current-key and previous-key paths so validation cannot drift. +fn decode_master_key_hex(hex_key: &str, what: &str) -> Result, CachekitError> { + let bytes = hex::decode(hex_key) + .map_err(|e| CachekitError::Config(format!("{what} is not valid hex: {e}")))?; + if bytes.len() < 32 { + return Err(CachekitError::Config(format!( + "{what} must be at least 32 bytes (64 hex chars); got {} bytes", + bytes.len() + ))); + } + Ok(bytes) +} + +/// Enforce the keyring config invariants: at most [`MAX_PREVIOUS_MASTER_KEYS`] +/// previous keys (rejected, never truncated), and the current master key must +/// not also appear in the previous list (the detectable subset of the +/// forward-only rotation rule — re-promoting a retired key would resume a +/// used, unknowable AES-GCM nonce budget). +/// +/// Fail-fast mirror of the checks `cachekit_core::Keyring::new` repeats at +/// client build time; plain equality is fine — both operands are +/// operator-supplied configuration, not secrets under timing attack. +fn validate_previous_master_keys( + master_key: Option<&[u8]>, + previous: &[Zeroizing>], +) -> Result<(), CachekitError> { + if previous.len() > MAX_PREVIOUS_MASTER_KEYS { + return Err(CachekitError::Config(format!( + "previous_master_keys accepts at most {MAX_PREVIOUS_MASTER_KEYS} entries; got {}", + previous.len() + ))); + } + if let Some(master) = master_key { + if previous.iter().any(|key| key.as_slice() == master) { + return Err(CachekitError::Config( + "the current master key must not appear in previous_master_keys \ + (rotation is forward-only; retired keys are never re-promoted)" + .to_owned(), + )); + } + } + Ok(()) +} + fn validate_https(url: &str) -> Result<(), CachekitError> { if !url.starts_with("https://") { return Err(CachekitError::Config(format!( diff --git a/crates/cachekit/src/encryption.rs b/crates/cachekit/src/encryption.rs index f898048..458253e 100644 --- a/crates/cachekit/src/encryption.rs +++ b/crates/cachekit/src/encryption.rs @@ -15,7 +15,7 @@ use zeroize::Zeroizing; -use cachekit_core::ZeroKnowledgeEncryptor; +use cachekit_core::{Keyring, ZeroKnowledgeEncryptor}; use crate::error::CachekitError; @@ -24,20 +24,33 @@ const AAD_VERSION: u8 = 0x03; /// Zero-knowledge encryption layer with per-tenant key derivation. /// -/// Holds a derived encryption key (zeroized on drop) and the +/// Holds a derived encryption key (zeroized on drop), a [`Keyring`] of master +/// keys for multi-key decrypt during rotation, and the /// `ZeroKnowledgeEncryptor` from cachekit-core for AES-256-GCM operations. /// /// L1 stores **ciphertext**, not plaintext — the zero-knowledge property /// is preserved across all cache layers. +/// +/// # Key rotation +/// +/// Writes always encrypt under the current master key. Reads decrypt via the +/// keyring: sequential attempts, current key first, then each decrypt-only +/// previous key in order, rebuilding the identical AAD per attempt (rs +/// entries carry no per-entry key identity — sequential attempts are the +/// spec-assigned branch, `protocol/spec/encryption.md` → "Key Rotation +/// (Keyring)"). pub struct EncryptionLayer { encryptor: ZeroKnowledgeEncryptor, derived_key: Zeroizing<[u8; 32]>, + keyring: Keyring, tenant_id: String, } impl EncryptionLayer { /// Create a new encryption layer with HKDF-derived tenant keys. /// + /// Equivalent to [`Self::with_previous_keys`] with no previous keys. + /// /// # Arguments /// * `master_key_bytes` — Raw master key (minimum 32 bytes for AES-256) /// * `tenant_id` — Tenant identifier for cryptographic isolation @@ -47,6 +60,36 @@ impl EncryptionLayer { /// - HKDF derivation failure /// - Encryptor initialization failure pub fn new(master_key_bytes: &[u8], tenant_id: &str) -> Result { + Self::with_previous_keys(master_key_bytes, &[], tenant_id) + } + + /// Create an encryption layer with decrypt-only previous master keys. + /// + /// `previous_keys` are retained during a key-rotation grace window, in + /// attempt order. Writes always use `master_key_bytes`; reads attempt it + /// first, then each previous key sequentially. + /// + /// # Errors + /// - Any key too short (< 32 bytes) + /// - More than 3 previous keys ([`CachekitError::Config`] — rejected, + /// never truncated) + /// - The current master key also present in `previous_keys` + /// ([`CachekitError::Config`] — rotation is forward-only) + /// - HKDF derivation failure + /// - Encryptor initialization failure + pub fn with_previous_keys( + master_key_bytes: &[u8], + previous_keys: &[&[u8]], + tenant_id: &str, + ) -> Result { + for (i, key) in previous_keys.iter().enumerate() { + if key.len() < 32 { + return Err(CachekitError::Config(format!( + "previous master key {i} must be at least 32 bytes; got {}", + key.len() + ))); + } + } if master_key_bytes.len() < 32 { return Err(CachekitError::Encryption(format!( "master key must be at least 32 bytes; got {}", @@ -74,9 +117,17 @@ impl EncryptionLayer { let encryptor = ZeroKnowledgeEncryptor::new() .map_err(|e| CachekitError::Encryption(format!("encryptor init failed: {e}")))?; + // The shared multi-key decrypt helper (cachekit-core, LAB-683) owns + // the keyring invariants: cap of 3, forward-only self-collision, + // sequential current-first attempts. Its construction failures are + // configuration errors, not crypto failures. + let keyring = Keyring::new(master_key_bytes, previous_keys) + .map_err(|e| CachekitError::Config(format!("keyring: {e}")))?; + Ok(Self { encryptor, derived_key: Zeroizing::new(tenant_keys.encryption_key), + keyring, tenant_id: tenant_id.to_owned(), }) } @@ -96,11 +147,16 @@ impl EncryptionLayer { /// /// Returns the original plaintext. Fails if the cache key does not match /// the one used during encryption (ciphertext substitution protection). + /// + /// Decryption goes through the keyring: the current master key is + /// attempted first, then each decrypt-only previous key in order, with + /// the identical AAD per attempt. Entries written before a key rotation + /// stay readable as long as their key remains in the previous list. pub fn decrypt(&self, ciphertext: &[u8], cache_key: &str) -> Result, CachekitError> { // compressed=false is normative, not a stub — see build_aad's invariant note. let aad = self.build_aad(cache_key, false); - self.encryptor - .decrypt_aes_gcm(ciphertext, &*self.derived_key, &aad) + self.keyring + .decrypt(&self.encryptor, ciphertext, &self.tenant_id, &aad) .map_err(|e| CachekitError::Encryption(format!("decrypt failed: {e}"))) } @@ -299,6 +355,68 @@ mod tests { assert!(aad_false.ends_with(b"False")); } + // ── Keyring rotation ───────────────────────────────────────────────────── + + const K1: &[u8] = &[0x11; 32]; + const K2: &[u8] = &[0x22; 32]; + + #[test] + fn previous_key_decrypts_after_rotation() { + // Written under k1 before the rotation... + let old_layer = EncryptionLayer::new(K1, TEST_TENANT).unwrap(); + let ciphertext = old_layer.encrypt(b"pre-rotation value", "user:1").unwrap(); + + // ...still readable with current=k2, previous=[k1] (identical AAD)... + let rotated = EncryptionLayer::with_previous_keys(K2, &[K1], TEST_TENANT).unwrap(); + let plaintext = rotated.decrypt(&ciphertext, "user:1").unwrap(); + assert_eq!(plaintext, b"pre-rotation value"); + + // ...and unreadable after a hard cut-over (previous=[]). + let cut_over = EncryptionLayer::with_previous_keys(K2, &[], TEST_TENANT).unwrap(); + assert!(cut_over.decrypt(&ciphertext, "user:1").is_err()); + } + + #[test] + fn rotated_layer_still_writes_under_current_key() { + let rotated = EncryptionLayer::with_previous_keys(K2, &[K1], TEST_TENANT).unwrap(); + let ciphertext = rotated.encrypt(b"fresh write", "user:2").unwrap(); + + // A current-key-only layer reads it: writes never use previous keys. + let current_only = EncryptionLayer::new(K2, TEST_TENANT).unwrap(); + assert_eq!( + current_only.decrypt(&ciphertext, "user:2").unwrap(), + b"fresh write" + ); + } + + #[test] + fn more_than_three_previous_keys_is_config_error() { + let keys: Vec<[u8; 32]> = (1..=4).map(|i| [i; 32]).collect(); + let refs: Vec<&[u8]> = keys.iter().map(|k| k.as_slice()).collect(); + + let result = EncryptionLayer::with_previous_keys(K2, &refs, TEST_TENANT); + assert!( + matches!(result, Err(CachekitError::Config(_))), + "cap of 3 must reject, never truncate" + ); + } + + #[test] + fn current_key_in_previous_list_is_config_error() { + let result = EncryptionLayer::with_previous_keys(K2, &[K1, K2], TEST_TENANT); + assert!( + matches!(result, Err(CachekitError::Config(_))), + "forward-only: current key must not be decrypt-only" + ); + } + + #[test] + fn short_previous_key_is_config_error() { + let short = [0x01u8; 16]; // core would accept 16; the rs SDK contract is 32 + let result = EncryptionLayer::with_previous_keys(K2, &[&short], TEST_TENANT); + assert!(matches!(result, Err(CachekitError::Config(_)))); + } + #[test] fn debug_redacts_key() { let layer = EncryptionLayer::new(TEST_MASTER_KEY, TEST_TENANT).unwrap(); diff --git a/crates/cachekit/tests/config_tests.rs b/crates/cachekit/tests/config_tests.rs index fef83d0..d937e6b 100644 --- a/crates/cachekit/tests/config_tests.rs +++ b/crates/cachekit/tests/config_tests.rs @@ -174,3 +174,152 @@ fn config_builder_accepts_valid_master_key() { assert!(config.master_key.is_some()); assert_eq!(config.master_key.as_ref().unwrap().len(), 32); } + +// ── Previous master keys (key rotation) ─────────────────────────────────────── + +fn hexkey(byte: u8) -> String { + hex::encode([byte; 32]) +} + +fn assert_config_err(result: Result, what: &str) { + match result { + Err(cachekit::CachekitError::Config(_)) => {} + other => panic!( + "{what}: expected CachekitError::Config, got {:?}", + other.map(|_| "Ok(builder)") + ), + } +} + +#[test] +fn config_builder_accepts_previous_master_keys() { + let config = CachekitConfigBuilder::new() + .master_key(&hexkey(0x22)) + .expect("valid master key") + .previous_master_keys(&[hexkey(0x11).as_str(), hexkey(0x33).as_str()]) + .expect("valid previous keys") + .build(); + + assert_eq!(config.previous_master_keys.len(), 2); + // Attempt order preserved: slice order. + assert_eq!(config.previous_master_keys[0].as_slice(), &[0x11u8; 32]); + assert_eq!(config.previous_master_keys[1].as_slice(), &[0x33u8; 32]); +} + +#[test] +fn config_builder_rejects_more_than_three_previous_keys() { + let keys: Vec = (1..=4).map(hexkey).collect(); + let refs: Vec<&str> = keys.iter().map(String::as_str).collect(); + + let result = CachekitConfigBuilder::new() + .master_key(&hexkey(0x22)) + .expect("valid master key") + .previous_master_keys(&refs); + assert_config_err(result, "cap of 3 must reject, never truncate"); +} + +#[test] +fn config_builder_rejects_invalid_hex_previous_key() { + let result = CachekitConfigBuilder::new().previous_master_keys(&["not-hex"]); + assert_config_err(result, "invalid hex previous key"); +} + +#[test] +fn config_builder_rejects_short_previous_key() { + let short_hex = "aa".repeat(31); + let result = CachekitConfigBuilder::new().previous_master_keys(&[short_hex.as_str()]); + assert_config_err(result, "short previous key"); +} + +#[test] +fn config_builder_rejects_master_key_in_previous_list() { + let current = hexkey(0x22); + + // previous set after master_key + let result = CachekitConfigBuilder::new() + .master_key(¤t) + .expect("valid master key") + .previous_master_keys(&[current.as_str()]); + assert_config_err(result, "self-collision (previous after master)"); + + // master_key set after previous — same invariant, other call order + let result = CachekitConfigBuilder::new() + .previous_master_keys(&[current.as_str()]) + .expect("valid previous keys") + .master_key(¤t); + assert_config_err(result, "self-collision (master after previous)"); +} + +#[test] +#[serial] +fn config_from_env_reads_previous_master_keys() { + std::env::set_var("CACHEKIT_MASTER_KEY", hexkey(0x22)); + std::env::set_var( + "CACHEKIT_PREVIOUS_MASTER_KEYS", + format!("{}, {}", hexkey(0x11), hexkey(0x33)), // whitespace around commas tolerated + ); + let config = CachekitConfig::from_env(); + std::env::remove_var("CACHEKIT_MASTER_KEY"); + std::env::remove_var("CACHEKIT_PREVIOUS_MASTER_KEYS"); + + let config = config.expect("from_env failed"); + assert_eq!(config.previous_master_keys.len(), 2); + assert_eq!(config.previous_master_keys[0].as_slice(), &[0x11u8; 32]); + assert_eq!(config.previous_master_keys[1].as_slice(), &[0x33u8; 32]); +} + +#[test] +#[serial] +fn config_from_env_rejects_more_than_three_previous_keys() { + let val: Vec = (1..=4).map(hexkey).collect(); + std::env::set_var("CACHEKIT_PREVIOUS_MASTER_KEYS", val.join(",")); + let result = CachekitConfig::from_env(); + std::env::remove_var("CACHEKIT_PREVIOUS_MASTER_KEYS"); + + assert!( + matches!(result, Err(cachekit::CachekitError::Config(_))), + "cap of 3 must reject at load, never truncate" + ); +} + +#[test] +#[serial] +fn config_from_env_rejects_master_key_in_previous_list() { + std::env::set_var("CACHEKIT_MASTER_KEY", hexkey(0x22)); + std::env::set_var( + "CACHEKIT_PREVIOUS_MASTER_KEYS", + format!("{},{}", hexkey(0x11), hexkey(0x22)), + ); + let result = CachekitConfig::from_env(); + std::env::remove_var("CACHEKIT_MASTER_KEY"); + std::env::remove_var("CACHEKIT_PREVIOUS_MASTER_KEYS"); + + assert!( + matches!(result, Err(cachekit::CachekitError::Config(_))), + "current key in previous list must fail at load" + ); +} + +#[test] +#[serial] +fn config_from_env_rejects_empty_previous_key_entry() { + std::env::set_var( + "CACHEKIT_PREVIOUS_MASTER_KEYS", + format!("{},", hexkey(0x11)), // trailing comma → empty entry + ); + let result = CachekitConfig::from_env(); + std::env::remove_var("CACHEKIT_PREVIOUS_MASTER_KEYS"); + + assert!(result.is_err(), "empty entry must be rejected, not skipped"); +} + +/// Drift guard: the config-level cap (compiled without the `encryption` +/// feature) must equal the core keyring's cap. +#[test] +#[cfg(feature = "encryption")] +fn previous_key_cap_matches_core_keyring_cap() { + assert_eq!( + cachekit::config::MAX_PREVIOUS_MASTER_KEYS, + cachekit_core::MAX_DECRYPT_ONLY_KEYS + ); +} diff --git a/crates/cachekit/tests/encryption_tests.rs b/crates/cachekit/tests/encryption_tests.rs index cd64a7a..491cc3f 100644 --- a/crates/cachekit/tests/encryption_tests.rs +++ b/crates/cachekit/tests/encryption_tests.rs @@ -341,3 +341,79 @@ async fn secure_set_rejects_payload_whose_ciphertext_exceeds_limit() { let got: Option = secure.get("small:key").await.expect("small get"); assert_eq!(got.as_deref(), Some("ok")); } + +// ── Key rotation (keyring) ──────────────────────────────────────────────────── + +/// End-to-end rotation round-trip (LAB-686 acceptance): +/// value written under k1 → k2 promoted with k1 decrypt-only → read succeeds +/// without re-encryption → k1 dropped → read fails as an error. +#[tokio::test] +async fn rotation_round_trip_without_reencryption() { + const K1: &[u8] = &[0x11; 32]; + const K2: &[u8] = &[0x22; 32]; + + let (backend, store) = common::MockBackend::new_with_handle(); + + // Phase 1: pre-rotation client writes under k1. + let writer = CacheKit::builder() + .backend(backend.clone()) + .default_ttl(Duration::from_secs(60)) + .no_l1() + .encryption_from_bytes(K1, "test-tenant") + .expect("encryption setup") + .build() + .expect("client builds"); + let secret = Secret { + api_key: "sk-live-rotate-me".to_owned(), // pragma: allowlist secret + user_id: 7, + }; + writer + .secure() + .expect("secure()") + .set("secret:7", &secret) + .await + .expect("secure set under k1"); + + let ciphertext_before = store.store.lock().await.get("secret:7").cloned().unwrap(); + + // Phase 2: k2 promoted to current, k1 retained decrypt-only. + let rotated = CacheKit::builder() + .backend(backend.clone()) + .default_ttl(Duration::from_secs(60)) + .no_l1() + .encryption_from_bytes_with_previous(K2, &[K1], "test-tenant") + .expect("keyring setup") + .build() + .expect("client builds"); + let read_back: Secret = rotated + .secure() + .expect("secure()") + .get("secret:7") + .await + .expect("secure get after rotation") + .expect("value should exist"); + assert_eq!(read_back, secret); + + // The read must not have re-encrypted: stored bytes are untouched. + let ciphertext_after = store.store.lock().await.get("secret:7").cloned().unwrap(); + assert_eq!( + ciphertext_before, ciphertext_after, + "read-through rotation must not rewrite the entry" + ); + + // Phase 3: k1 dropped — hard cut-over, the k1-era entry is unreadable. + let cut_over = CacheKit::builder() + .backend(backend) + .default_ttl(Duration::from_secs(60)) + .no_l1() + .encryption_from_bytes_with_previous(K2, &[], "test-tenant") + .expect("keyring setup") + .build() + .expect("client builds"); + let result: Result, _> = + cut_over.secure().expect("secure()").get("secret:7").await; + assert!( + matches!(result, Err(CachekitError::Encryption(_))), + "dropped-key read must surface as an encryption error, got {result:?}" + ); +} From 97537b162a076b980664f39c1f9ef22143929f43 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 8 Aug 2026 00:27:27 +1000 Subject: [PATCH 2/6] fix(encryption): keep config-class errors out of the decrypt-failure class (LAB-686 panel) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expert-panel findings on PR #63: - decrypt: EncryptionError::KeyDerivation / KeyringIndexOutOfRange now map to CachekitError::Config instead of folding into ::Encryption — per the LAB-683 decision, a config bug must never masquerade as a decrypt failure that fail-open callers read as a miss. - with_previous_keys: master-key length and tenant_id checks now return CachekitError::Config, matching the previous-key and Keyring checks in the same function (was: Encryption for master/tenant, Config for previous — inconsistent within one constructor). - from_env: CACHEKIT_PREVIOUS_MASTER_KEYS without CACHEKIT_MASTER_KEY is now a load-time Config error instead of silently never wiring the previous keys (the botched-rotation-deploy case). - intent_tests: short-master-key assertion updated to the Config class (the test's intent — validation before network I/O — unchanged). --- crates/cachekit/src/config.rs | 9 +++++++++ crates/cachekit/src/encryption.rs | 20 ++++++++++++++++---- crates/cachekit/tests/config_tests.rs | 14 ++++++++++++++ crates/cachekit/tests/intent_tests.rs | 6 +++--- 4 files changed, 42 insertions(+), 7 deletions(-) diff --git a/crates/cachekit/src/config.rs b/crates/cachekit/src/config.rs index fdae66c..3be5243 100644 --- a/crates/cachekit/src/config.rs +++ b/crates/cachekit/src/config.rs @@ -126,6 +126,15 @@ impl CachekitConfig { "CACHEKIT_PREVIOUS_MASTER_KEYS entry", )?)); } + // Previous keys without a current key is a broken rotation + // deploy: nothing would ever consume them, and the operator + // would only find out at the first secure() call. Fail at load. + if config.master_key.is_none() { + return Err(CachekitError::Config( + "CACHEKIT_PREVIOUS_MASTER_KEYS requires CACHEKIT_MASTER_KEY to be set" + .to_owned(), + )); + } validate_previous_master_keys( config.master_key.as_deref().map(Vec::as_slice), &previous, diff --git a/crates/cachekit/src/encryption.rs b/crates/cachekit/src/encryption.rs index 458253e..f4afe33 100644 --- a/crates/cachekit/src/encryption.rs +++ b/crates/cachekit/src/encryption.rs @@ -90,19 +90,22 @@ impl EncryptionLayer { ))); } } + // Key-length and tenant checks are configuration errors, kept in the + // same class as the previous-key and Keyring construction checks + // below (LAB-683: config errors must not fold into crypto failures). if master_key_bytes.len() < 32 { - return Err(CachekitError::Encryption(format!( + return Err(CachekitError::Config(format!( "master key must be at least 32 bytes; got {}", master_key_bytes.len() ))); } if tenant_id.is_empty() { - return Err(CachekitError::Encryption( + return Err(CachekitError::Config( "tenant_id must not be empty".to_owned(), )); } if tenant_id.len() > 255 { - return Err(CachekitError::Encryption(format!( + return Err(CachekitError::Config(format!( "tenant_id must be at most 255 bytes; got {}", tenant_id.len() ))); @@ -157,7 +160,16 @@ impl EncryptionLayer { let aad = self.build_aad(cache_key, false); self.keyring .decrypt(&self.encryptor, ciphertext, &self.tenant_id, &aad) - .map_err(|e| CachekitError::Encryption(format!("decrypt failed: {e}"))) + .map_err(|e| match e { + // Config-class errors stay config-class (LAB-683 decision): + // a derivation failure or caller bug must never masquerade as + // a decrypt failure that fail-open callers read as a miss. + cachekit_core::EncryptionError::KeyDerivation(_) + | cachekit_core::EncryptionError::KeyringIndexOutOfRange { .. } => { + CachekitError::Config(format!("keyring decrypt misconfiguration: {e}")) + } + _ => CachekitError::Encryption(format!("decrypt failed: {e}")), + }) } /// Return the tenant ID used for key derivation. diff --git a/crates/cachekit/tests/config_tests.rs b/crates/cachekit/tests/config_tests.rs index d937e6b..eeb2160 100644 --- a/crates/cachekit/tests/config_tests.rs +++ b/crates/cachekit/tests/config_tests.rs @@ -323,3 +323,17 @@ fn previous_key_cap_matches_core_keyring_cap() { cachekit_core::MAX_DECRYPT_ONLY_KEYS ); } + +#[test] +#[serial] +fn config_from_env_rejects_previous_keys_without_master_key() { + std::env::remove_var("CACHEKIT_MASTER_KEY"); + std::env::set_var("CACHEKIT_PREVIOUS_MASTER_KEYS", hexkey(0x11)); + let result = CachekitConfig::from_env(); + std::env::remove_var("CACHEKIT_PREVIOUS_MASTER_KEYS"); + + assert!( + matches!(result, Err(cachekit::CachekitError::Config(_))), + "previous keys without a current master key must fail at load, not be silently dropped" + ); +} diff --git a/crates/cachekit/tests/intent_tests.rs b/crates/cachekit/tests/intent_tests.rs index 4eec20d..94668ae 100644 --- a/crates/cachekit/tests/intent_tests.rs +++ b/crates/cachekit/tests/intent_tests.rs @@ -82,11 +82,11 @@ mod encrypted_intent { #[tokio::test] async fn rejects_short_master_key_before_connecting() { // The URL points at an unreachable Redis on purpose: key validation - // must fire first, so we get the deterministic Encryption error — - // never a Backend (connection) error. + // must fire first, so we get the deterministic Config error (a short + // key is a configuration mistake) — never a Backend (connection) error. let result = CacheKit::encrypted("redis://127.0.0.1:1", b"too_short").await; assert!( - matches!(result, Err(CachekitError::Encryption(_))), + matches!(result, Err(CachekitError::Config(_))), "short master key must be rejected before any Redis I/O" ); } From 7efc8e68fa8b7af960a6e7e40b600bcae74cbe4f Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 8 Aug 2026 01:17:56 +1000 Subject: [PATCH 3/6] =?UTF-8?q?fix(config):=20review=20round=20=E2=80=94?= =?UTF-8?q?=20zeroized=20key=20decode,=20blank=20env=20retirement,=20bound?= =?UTF-8?q?ary=20tests=20(LAB-686)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit + Kody round on #63: - decode_master_key_hex returns Zeroizing> so decoded key material is wiped on every path, including early-drop when a later validation step fails (CodeRabbit, security) - A wholly blank CACHEKIT_PREVIOUS_MASTER_KEYS is treated as unset: blanking a variable is how shell profiles / Compose / k8s manifests retire it after a completed rotation; a blank entry inside a non-blank list is still rejected (CodeRabbit, correctness) - Boundary-success tests for the cap: exactly three previous keys build (layer + config builder) — a >= regression would have passed the rejecting-side suite (CodeRabbit) - EnvGuard RAII helper in config_tests: env tests restore pre-test variable values on drop, including on assertion failure (CodeRabbit) - Doc fixes: encryption_from_bytes said 'at least 16 bytes' while the code rejects <32; with_previous doc states the 32-byte minimum; EncryptionLayer rotation doc line-wrap; drift-guard comment now says the cap const is declared outside the encryption feature gate --- README.md | 2 +- crates/cachekit/src/client.rs | 4 +- crates/cachekit/src/config.rs | 75 +++++++------ crates/cachekit/src/encryption.rs | 23 +++- crates/cachekit/tests/config_tests.rs | 154 ++++++++++++++++++++------ 5 files changed, 185 insertions(+), 73 deletions(-) diff --git a/README.md b/README.md index d37ce18..f2720cb 100644 --- a/README.md +++ b/README.md @@ -450,7 +450,7 @@ Requires a tokio runtime for backoff timers (the `redis` and `cachekitio` backen | `CACHEKIT_API_KEY` | ✅ | API key for cachekit.io | | `CACHEKIT_API_URL` | ❌ | Override API endpoint (default: `https://api.cachekit.io`) | | `CACHEKIT_MASTER_KEY` | ❌ | Hex-encoded master key (min 32 bytes) for encryption | -| `CACHEKIT_PREVIOUS_MASTER_KEYS` | ❌ | Comma-separated hex-encoded decrypt-only previous master keys for key rotation (max 3) | +| `CACHEKIT_PREVIOUS_MASTER_KEYS` | ❌ | Comma-separated hex-encoded decrypt-only previous master keys for key rotation (max 3; a blank value is treated as unset) | | `CACHEKIT_DEFAULT_TTL` | ❌ | Default TTL in seconds (min 1, default: 300) | > [!CAUTION] diff --git a/crates/cachekit/src/client.rs b/crates/cachekit/src/client.rs index 6155c6f..9014511 100644 --- a/crates/cachekit/src/client.rs +++ b/crates/cachekit/src/client.rs @@ -997,7 +997,7 @@ impl CacheKitBuilder { /// Configure encryption from raw master key bytes and tenant ID. /// - /// The master key must be at least 16 bytes (32 recommended). + /// The master key must be at least 32 bytes. /// Keys are derived per-tenant via HKDF-SHA256. #[cfg(feature = "encryption")] pub fn encryption_from_bytes( @@ -1017,6 +1017,8 @@ impl CacheKitBuilder { /// key in `previous_keys` sequentially (attempt order = slice order). /// At most 3 previous keys; supplying more is a config error, never /// truncated. See [`crate::encryption::EncryptionLayer::with_previous_keys`]. + /// + /// Every key, current and previous, must be at least 32 bytes. #[cfg(feature = "encryption")] pub fn encryption_from_bytes_with_previous( mut self, diff --git a/crates/cachekit/src/config.rs b/crates/cachekit/src/config.rs index 3be5243..9bfe1a2 100644 --- a/crates/cachekit/src/config.rs +++ b/crates/cachekit/src/config.rs @@ -89,7 +89,7 @@ impl CachekitConfig { /// | `CACHEKIT_API_KEY` | API key for cachekit.io | /// | `CACHEKIT_API_URL` | Override API base URL (must be HTTPS) | /// | `CACHEKIT_MASTER_KEY` | Hex-encoded master key (min 32 bytes) | - /// | `CACHEKIT_PREVIOUS_MASTER_KEYS` | Comma-separated hex-encoded decrypt-only previous master keys (max 3) | + /// | `CACHEKIT_PREVIOUS_MASTER_KEYS` | Comma-separated hex-encoded decrypt-only previous master keys (max 3; blank value = unset) | /// | `CACHEKIT_DEFAULT_TTL` | Default TTL in seconds (min 1) | pub fn from_env() -> Result { let mut config = Self::default(); @@ -107,39 +107,44 @@ impl CachekitConfig { // Master key — hex-decode and validate length >= 32 bytes if let Ok(val) = std::env::var("CACHEKIT_MASTER_KEY") { - let bytes = decode_master_key_hex(&val, "CACHEKIT_MASTER_KEY")?; - config.master_key = Some(Zeroizing::new(bytes)); + config.master_key = Some(decode_master_key_hex(&val, "CACHEKIT_MASTER_KEY")?); } // Previous master keys — comma-separated hex, decrypt-only, max 3. + // A wholly blank value retires the variable (the common way to disable + // it in shell profiles, Compose files, and k8s manifests) and is + // treated as unset; a blank entry inside a non-blank list is still an + // operator mistake. if let Ok(val) = std::env::var("CACHEKIT_PREVIOUS_MASTER_KEYS") { - let mut previous = Vec::new(); - for entry in val.split(',') { - let entry = entry.trim(); - if entry.is_empty() { + if !val.trim().is_empty() { + let mut previous = Vec::new(); + for entry in val.split(',') { + let entry = entry.trim(); + if entry.is_empty() { + return Err(CachekitError::Config( + "CACHEKIT_PREVIOUS_MASTER_KEYS contains an empty entry".to_owned(), + )); + } + previous.push(decode_master_key_hex( + entry, + "CACHEKIT_PREVIOUS_MASTER_KEYS entry", + )?); + } + // Previous keys without a current key is a broken rotation + // deploy: nothing would ever consume them, and the operator + // would only find out at the first secure() call. Fail at load. + if config.master_key.is_none() { return Err(CachekitError::Config( - "CACHEKIT_PREVIOUS_MASTER_KEYS contains an empty entry".to_owned(), + "CACHEKIT_PREVIOUS_MASTER_KEYS requires CACHEKIT_MASTER_KEY to be set" + .to_owned(), )); } - previous.push(Zeroizing::new(decode_master_key_hex( - entry, - "CACHEKIT_PREVIOUS_MASTER_KEYS entry", - )?)); + validate_previous_master_keys( + config.master_key.as_deref().map(Vec::as_slice), + &previous, + )?; + config.previous_master_keys = previous; } - // Previous keys without a current key is a broken rotation - // deploy: nothing would ever consume them, and the operator - // would only find out at the first secure() call. Fail at load. - if config.master_key.is_none() { - return Err(CachekitError::Config( - "CACHEKIT_PREVIOUS_MASTER_KEYS requires CACHEKIT_MASTER_KEY to be set" - .to_owned(), - )); - } - validate_previous_master_keys( - config.master_key.as_deref().map(Vec::as_slice), - &previous, - )?; - config.previous_master_keys = previous; } // Default TTL — minimum 1 second @@ -194,7 +199,7 @@ impl CachekitConfigBuilder { pub fn master_key(mut self, hex_key: &str) -> Result { let bytes = decode_master_key_hex(hex_key, "master_key")?; validate_previous_master_keys(Some(bytes.as_slice()), &self.inner.previous_master_keys)?; - self.inner.master_key = Some(Zeroizing::new(bytes)); + self.inner.master_key = Some(bytes); Ok(self) } @@ -228,10 +233,10 @@ impl CachekitConfigBuilder { pub fn previous_master_keys(mut self, hex_keys: &[&str]) -> Result { let mut previous = Vec::with_capacity(hex_keys.len()); for hex_key in hex_keys { - previous.push(Zeroizing::new(decode_master_key_hex( + previous.push(decode_master_key_hex( hex_key, "previous_master_keys entry", - )?)); + )?); } validate_previous_master_keys( self.inner.master_key.as_deref().map(Vec::as_slice), @@ -274,9 +279,15 @@ impl CachekitConfigBuilder { /// Hex-decode a master key and require at least 32 bytes. Shared by the /// current-key and previous-key paths so validation cannot drift. -fn decode_master_key_hex(hex_key: &str, what: &str) -> Result, CachekitError> { - let bytes = hex::decode(hex_key) - .map_err(|e| CachekitError::Config(format!("{what} is not valid hex: {e}")))?; +/// +/// Returns `Zeroizing` so the decoded key material is wiped on drop for its +/// whole lifetime — including the early-drop paths where a caller's later +/// validation step fails. +fn decode_master_key_hex(hex_key: &str, what: &str) -> Result>, CachekitError> { + let bytes = Zeroizing::new( + hex::decode(hex_key) + .map_err(|e| CachekitError::Config(format!("{what} is not valid hex: {e}")))?, + ); if bytes.len() < 32 { return Err(CachekitError::Config(format!( "{what} must be at least 32 bytes (64 hex chars); got {} bytes", diff --git a/crates/cachekit/src/encryption.rs b/crates/cachekit/src/encryption.rs index f4afe33..fcee1c6 100644 --- a/crates/cachekit/src/encryption.rs +++ b/crates/cachekit/src/encryption.rs @@ -35,10 +35,10 @@ const AAD_VERSION: u8 = 0x03; /// /// Writes always encrypt under the current master key. Reads decrypt via the /// keyring: sequential attempts, current key first, then each decrypt-only -/// previous key in order, rebuilding the identical AAD per attempt (rs -/// entries carry no per-entry key identity — sequential attempts are the -/// spec-assigned branch, `protocol/spec/encryption.md` → "Key Rotation -/// (Keyring)"). +/// previous key in order, rebuilding the identical AAD per attempt +/// (cachekit-rs entries carry no per-entry key identity — sequential +/// attempts are the spec-assigned branch, `protocol/spec/encryption.md` → +/// "Key Rotation (Keyring)"). pub struct EncryptionLayer { encryptor: ZeroKnowledgeEncryptor, derived_key: Zeroizing<[u8; 32]>, @@ -401,6 +401,21 @@ mod tests { ); } + #[test] + fn exactly_three_previous_keys_is_accepted() { + // Boundary success: a `>=` cap check instead of `>` would reject a + // legitimate three-key rotation window and still pass every + // rejecting-side test. + let keys: Vec<[u8; 32]> = (1..=3).map(|i| [i; 32]).collect(); + let refs: Vec<&[u8]> = keys.iter().map(|k| k.as_slice()).collect(); + + let layer = EncryptionLayer::with_previous_keys(K2, &refs, TEST_TENANT); + assert!( + layer.is_ok(), + "cap is 3: exactly three previous keys must build" + ); + } + #[test] fn more_than_three_previous_keys_is_config_error() { let keys: Vec<[u8; 32]> = (1..=4).map(|i| [i; 32]).collect(); diff --git a/crates/cachekit/tests/config_tests.rs b/crates/cachekit/tests/config_tests.rs index eeb2160..8530999 100644 --- a/crates/cachekit/tests/config_tests.rs +++ b/crates/cachekit/tests/config_tests.rs @@ -191,6 +191,42 @@ fn assert_config_err(result: Result)>, +} + +impl EnvGuard { + /// Apply `(name, value)` pairs: `Some` sets the variable, `None` removes + /// it. The prior value of every named variable is restored on drop. + fn set(vars: &[(&'static str, Option<&str>)]) -> Self { + let saved = vars + .iter() + .map(|(name, _)| (*name, std::env::var(name).ok())) + .collect(); + for (name, value) in vars { + match value { + Some(v) => std::env::set_var(name, v), + None => std::env::remove_var(name), + } + } + Self { saved } + } +} + +impl Drop for EnvGuard { + fn drop(&mut self) { + for (name, value) in &self.saved { + match value { + Some(v) => std::env::set_var(name, v), + None => std::env::remove_var(name), + } + } + } +} + #[test] fn config_builder_accepts_previous_master_keys() { let config = CachekitConfigBuilder::new() @@ -206,6 +242,23 @@ fn config_builder_accepts_previous_master_keys() { assert_eq!(config.previous_master_keys[1].as_slice(), &[0x33u8; 32]); } +#[test] +fn config_builder_accepts_exactly_three_previous_keys() { + // Boundary success for the cap: a `>=` check instead of `>` would block a + // legitimate three-key rotation window and still pass the rejecting test. + let keys: Vec = (1..=3).map(hexkey).collect(); + let refs: Vec<&str> = keys.iter().map(String::as_str).collect(); + + let config = CachekitConfigBuilder::new() + .master_key(&hexkey(0x22)) + .expect("valid master key") + .previous_master_keys(&refs) + .expect("exactly three previous keys must be accepted") + .build(); + + assert_eq!(config.previous_master_keys.len(), 3); +} + #[test] fn config_builder_rejects_more_than_three_previous_keys() { let keys: Vec = (1..=4).map(hexkey).collect(); @@ -253,16 +306,16 @@ fn config_builder_rejects_master_key_in_previous_list() { #[test] #[serial] fn config_from_env_reads_previous_master_keys() { - std::env::set_var("CACHEKIT_MASTER_KEY", hexkey(0x22)); - std::env::set_var( - "CACHEKIT_PREVIOUS_MASTER_KEYS", - format!("{}, {}", hexkey(0x11), hexkey(0x33)), // whitespace around commas tolerated - ); - let config = CachekitConfig::from_env(); - std::env::remove_var("CACHEKIT_MASTER_KEY"); - std::env::remove_var("CACHEKIT_PREVIOUS_MASTER_KEYS"); + let _env = EnvGuard::set(&[ + ("CACHEKIT_MASTER_KEY", Some(&hexkey(0x22))), + ( + "CACHEKIT_PREVIOUS_MASTER_KEYS", + // whitespace around commas tolerated + Some(&format!("{}, {}", hexkey(0x11), hexkey(0x33))), + ), + ]); - let config = config.expect("from_env failed"); + let config = CachekitConfig::from_env().expect("from_env failed"); assert_eq!(config.previous_master_keys.len(), 2); assert_eq!(config.previous_master_keys[0].as_slice(), &[0x11u8; 32]); assert_eq!(config.previous_master_keys[1].as_slice(), &[0x33u8; 32]); @@ -272,12 +325,16 @@ fn config_from_env_reads_previous_master_keys() { #[serial] fn config_from_env_rejects_more_than_three_previous_keys() { let val: Vec = (1..=4).map(hexkey).collect(); - std::env::set_var("CACHEKIT_PREVIOUS_MASTER_KEYS", val.join(",")); - let result = CachekitConfig::from_env(); - std::env::remove_var("CACHEKIT_PREVIOUS_MASTER_KEYS"); + let _env = EnvGuard::set(&[ + ("CACHEKIT_MASTER_KEY", Some(&hexkey(0x99))), + ("CACHEKIT_PREVIOUS_MASTER_KEYS", Some(&val.join(","))), + ]); assert!( - matches!(result, Err(cachekit::CachekitError::Config(_))), + matches!( + CachekitConfig::from_env(), + Err(cachekit::CachekitError::Config(_)) + ), "cap of 3 must reject at load, never truncate" ); } @@ -285,17 +342,19 @@ fn config_from_env_rejects_more_than_three_previous_keys() { #[test] #[serial] fn config_from_env_rejects_master_key_in_previous_list() { - std::env::set_var("CACHEKIT_MASTER_KEY", hexkey(0x22)); - std::env::set_var( - "CACHEKIT_PREVIOUS_MASTER_KEYS", - format!("{},{}", hexkey(0x11), hexkey(0x22)), - ); - let result = CachekitConfig::from_env(); - std::env::remove_var("CACHEKIT_MASTER_KEY"); - std::env::remove_var("CACHEKIT_PREVIOUS_MASTER_KEYS"); + let _env = EnvGuard::set(&[ + ("CACHEKIT_MASTER_KEY", Some(&hexkey(0x22))), + ( + "CACHEKIT_PREVIOUS_MASTER_KEYS", + Some(&format!("{},{}", hexkey(0x11), hexkey(0x22))), + ), + ]); assert!( - matches!(result, Err(cachekit::CachekitError::Config(_))), + matches!( + CachekitConfig::from_env(), + Err(cachekit::CachekitError::Config(_)) + ), "current key in previous list must fail at load" ); } @@ -303,18 +362,40 @@ fn config_from_env_rejects_master_key_in_previous_list() { #[test] #[serial] fn config_from_env_rejects_empty_previous_key_entry() { - std::env::set_var( - "CACHEKIT_PREVIOUS_MASTER_KEYS", - format!("{},", hexkey(0x11)), // trailing comma → empty entry + let _env = EnvGuard::set(&[ + ("CACHEKIT_MASTER_KEY", Some(&hexkey(0x22))), + ( + "CACHEKIT_PREVIOUS_MASTER_KEYS", + // trailing comma → empty entry + Some(&format!("{},", hexkey(0x11))), + ), + ]); + + assert!( + CachekitConfig::from_env().is_err(), + "empty entry must be rejected, not skipped" ); - let result = CachekitConfig::from_env(); - std::env::remove_var("CACHEKIT_PREVIOUS_MASTER_KEYS"); +} - assert!(result.is_err(), "empty entry must be rejected, not skipped"); +#[test] +#[serial] +fn config_from_env_tolerates_blank_previous_master_keys() { + // Blanking the variable is how shell profiles, Compose files, and k8s + // manifests retire it after a completed rotation — that must be a clean + // cut-over, not a start-up failure. + let _env = EnvGuard::set(&[ + ("CACHEKIT_MASTER_KEY", None), + ("CACHEKIT_PREVIOUS_MASTER_KEYS", Some(" ")), + ]); + + let config = CachekitConfig::from_env().expect("blank value must be treated as unset"); + assert!(config.previous_master_keys.is_empty()); } -/// Drift guard: the config-level cap (compiled without the `encryption` -/// feature) must equal the core keyring's cap. +/// Drift guard: the config-level cap (`MAX_PREVIOUS_MASTER_KEYS` is declared +/// outside the `encryption` feature gate, so `config.rs` cannot reference the +/// core const directly) must equal the core keyring's cap. CI's main test job +/// enables `encryption`, so this guard runs there. #[test] #[cfg(feature = "encryption")] fn previous_key_cap_matches_core_keyring_cap() { @@ -327,13 +408,16 @@ fn previous_key_cap_matches_core_keyring_cap() { #[test] #[serial] fn config_from_env_rejects_previous_keys_without_master_key() { - std::env::remove_var("CACHEKIT_MASTER_KEY"); - std::env::set_var("CACHEKIT_PREVIOUS_MASTER_KEYS", hexkey(0x11)); - let result = CachekitConfig::from_env(); - std::env::remove_var("CACHEKIT_PREVIOUS_MASTER_KEYS"); + let _env = EnvGuard::set(&[ + ("CACHEKIT_MASTER_KEY", None), + ("CACHEKIT_PREVIOUS_MASTER_KEYS", Some(&hexkey(0x11))), + ]); assert!( - matches!(result, Err(cachekit::CachekitError::Config(_))), + matches!( + CachekitConfig::from_env(), + Err(cachekit::CachekitError::Config(_)) + ), "previous keys without a current master key must fail at load, not be silently dropped" ); } From 7e2a98f96fce9c3fe972055f43b38f627ed32f08 Mon Sep 17 00:00:00 2001 From: Ray Walker Date: Sat, 8 Aug 2026 01:25:16 +1000 Subject: [PATCH 4/6] =?UTF-8?q?test(config):=20panel=20round=20=E2=80=94?= =?UTF-8?q?=20finish=20EnvGuard=20conversion,=20isolate=20defaults=20test?= =?UTF-8?q?=20(LAB-686)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Expert-panel findings on the review-round commit: - Convert the nine pre-existing env tests to EnvGuard: the old set/call/remove pattern skips cleanup when the call panics, poisoning every later #[serial] test — the exact class EnvGuard was added to kill - config_from_env_defaults now clears CACHEKIT_PREVIOUS_MASTER_KEYS too; a shell-exported value would fail it with an unrelated requires-master-key error - Comment why blank CACHEKIT_MASTER_KEY stays strict while blank previous-keys is tolerated: blank-as-unset on the master key would silently turn encryption off - Drop a comment duplicated from the encryption.rs twin test --- crates/cachekit/src/config.rs | 5 ++- crates/cachekit/tests/config_tests.rs | 62 +++++++++++++-------------- 2 files changed, 35 insertions(+), 32 deletions(-) diff --git a/crates/cachekit/src/config.rs b/crates/cachekit/src/config.rs index 9bfe1a2..65572f4 100644 --- a/crates/cachekit/src/config.rs +++ b/crates/cachekit/src/config.rs @@ -105,7 +105,10 @@ impl CachekitConfig { config.api_url = val; } - // Master key — hex-decode and validate length >= 32 bytes + // Master key — hex-decode and validate length >= 32 bytes. + // Deliberately NO blank-value tolerance here (unlike the previous-keys + // var below): a blank CACHEKIT_MASTER_KEY treated as unset would + // silently turn encryption off. if let Ok(val) = std::env::var("CACHEKIT_MASTER_KEY") { config.master_key = Some(decode_master_key_hex(&val, "CACHEKIT_MASTER_KEY")?); } diff --git a/crates/cachekit/tests/config_tests.rs b/crates/cachekit/tests/config_tests.rs index 8530999..4e7a9c8 100644 --- a/crates/cachekit/tests/config_tests.rs +++ b/crates/cachekit/tests/config_tests.rs @@ -7,11 +7,14 @@ use std::time::Duration; #[test] #[serial] fn config_from_env_defaults() { - // Clear relevant env vars so we get defaults. - std::env::remove_var("CACHEKIT_API_KEY"); - std::env::remove_var("CACHEKIT_API_URL"); - std::env::remove_var("CACHEKIT_MASTER_KEY"); - std::env::remove_var("CACHEKIT_DEFAULT_TTL"); + // Clear every from_env-read variable so we get defaults. + let _env = EnvGuard::set(&[ + ("CACHEKIT_API_KEY", None), + ("CACHEKIT_API_URL", None), + ("CACHEKIT_MASTER_KEY", None), + ("CACHEKIT_PREVIOUS_MASTER_KEYS", None), + ("CACHEKIT_DEFAULT_TTL", None), + ]); let config = CachekitConfig::from_env().expect("from_env failed with no env vars"); @@ -28,9 +31,8 @@ fn config_from_env_defaults() { #[test] #[serial] fn config_from_env_reads_api_key() { - std::env::set_var("CACHEKIT_API_KEY", "test-key-123"); + let _env = EnvGuard::set(&[("CACHEKIT_API_KEY", Some("test-key-123"))]); let config = CachekitConfig::from_env().expect("from_env failed"); - std::env::remove_var("CACHEKIT_API_KEY"); // Use .as_ref().map(|k| k.as_str()) NOT .as_deref() assert_eq!( @@ -42,19 +44,19 @@ fn config_from_env_reads_api_key() { #[test] #[serial] fn config_from_env_rejects_http_url() { - std::env::set_var("CACHEKIT_API_URL", "http://insecure.example.com"); - let result = CachekitConfig::from_env(); - std::env::remove_var("CACHEKIT_API_URL"); + let _env = EnvGuard::set(&[("CACHEKIT_API_URL", Some("http://insecure.example.com"))]); - assert!(result.is_err(), "expected error for non-HTTPS api_url"); + assert!( + CachekitConfig::from_env().is_err(), + "expected error for non-HTTPS api_url" + ); } #[test] #[serial] fn config_from_env_accepts_https_url() { - std::env::set_var("CACHEKIT_API_URL", "https://custom.cachekit.io"); + let _env = EnvGuard::set(&[("CACHEKIT_API_URL", Some("https://custom.cachekit.io"))]); let config = CachekitConfig::from_env().expect("from_env failed"); - std::env::remove_var("CACHEKIT_API_URL"); assert_eq!(config.api_url, "https://custom.cachekit.io"); } @@ -63,20 +65,20 @@ fn config_from_env_accepts_https_url() { #[serial] fn config_from_env_rejects_short_master_key() { // 31 bytes = 62 hex chars — too short - std::env::set_var("CACHEKIT_MASTER_KEY", "aa".repeat(31)); - let result = CachekitConfig::from_env(); - std::env::remove_var("CACHEKIT_MASTER_KEY"); + let _env = EnvGuard::set(&[("CACHEKIT_MASTER_KEY", Some(&"aa".repeat(31)))]); - assert!(result.is_err(), "expected error for short master key"); + assert!( + CachekitConfig::from_env().is_err(), + "expected error for short master key" + ); } #[test] #[serial] fn config_from_env_accepts_32_byte_master_key() { // 32 bytes = 64 hex chars — minimum valid - std::env::set_var("CACHEKIT_MASTER_KEY", "ab".repeat(32)); + let _env = EnvGuard::set(&[("CACHEKIT_MASTER_KEY", Some(&"ab".repeat(32)))]); let config = CachekitConfig::from_env().expect("from_env failed"); - std::env::remove_var("CACHEKIT_MASTER_KEY"); assert!(config.master_key.is_some()); assert_eq!(config.master_key.as_ref().unwrap().len(), 32); @@ -85,19 +87,19 @@ fn config_from_env_accepts_32_byte_master_key() { #[test] #[serial] fn config_from_env_rejects_ttl_zero() { - std::env::set_var("CACHEKIT_DEFAULT_TTL", "0"); - let result = CachekitConfig::from_env(); - std::env::remove_var("CACHEKIT_DEFAULT_TTL"); + let _env = EnvGuard::set(&[("CACHEKIT_DEFAULT_TTL", Some("0"))]); - assert!(result.is_err(), "expected error for TTL=0"); + assert!( + CachekitConfig::from_env().is_err(), + "expected error for TTL=0" + ); } #[test] #[serial] fn config_from_env_accepts_ttl_one() { - std::env::set_var("CACHEKIT_DEFAULT_TTL", "1"); + let _env = EnvGuard::set(&[("CACHEKIT_DEFAULT_TTL", Some("1"))]); let config = CachekitConfig::from_env().expect("from_env failed"); - std::env::remove_var("CACHEKIT_DEFAULT_TTL"); assert_eq!(config.default_ttl, Duration::from_secs(1)); } @@ -107,11 +109,11 @@ fn config_from_env_accepts_ttl_one() { #[test] #[serial] fn config_debug_redacts_secrets() { - std::env::set_var("CACHEKIT_API_KEY", "super-secret-key"); - std::env::set_var("CACHEKIT_MASTER_KEY", "ab".repeat(32)); + let _env = EnvGuard::set(&[ + ("CACHEKIT_API_KEY", Some("super-secret-key")), + ("CACHEKIT_MASTER_KEY", Some(&"ab".repeat(32))), + ]); let config = CachekitConfig::from_env().expect("from_env failed"); - std::env::remove_var("CACHEKIT_API_KEY"); - std::env::remove_var("CACHEKIT_MASTER_KEY"); let debug_str = format!("{config:?}"); assert!( @@ -244,8 +246,6 @@ fn config_builder_accepts_previous_master_keys() { #[test] fn config_builder_accepts_exactly_three_previous_keys() { - // Boundary success for the cap: a `>=` check instead of `>` would block a - // legitimate three-key rotation window and still pass the rejecting test. let keys: Vec = (1..=3).map(hexkey).collect(); let refs: Vec<&str> = keys.iter().map(String::as_str).collect(); From 7a7d3f0d7e2f2a3cca083c6af5dcb67638775d74 Mon Sep 17 00:00:00 2001 From: Mark S Date: Sat, 8 Aug 2026 02:08:22 +1000 Subject: [PATCH 5/6] fix(config): zeroise hex secret copies from env and EnvGuard (LAB-686) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit std::env::var hands back an owned String of the hex-encoded key material for CACHEKIT_MASTER_KEY and CACHEKIT_PREVIOUS_MASTER_KEYS. The decoded bytes were already Zeroizing, but the hex source copy was dropped without being wiped — the same secret in a different encoding, equally recoverable from freed heap. EnvGuard in the config tests had the same hole: it saves each variable's pre-test shell value, which for those two vars is real operator key material. --- crates/cachekit/src/config.rs | 7 +++++++ crates/cachekit/tests/config_tests.rs | 10 +++++++--- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/crates/cachekit/src/config.rs b/crates/cachekit/src/config.rs index 65572f4..60aaf21 100644 --- a/crates/cachekit/src/config.rs +++ b/crates/cachekit/src/config.rs @@ -110,6 +110,10 @@ impl CachekitConfig { // var below): a blank CACHEKIT_MASTER_KEY treated as unset would // silently turn encryption off. if let Ok(val) = std::env::var("CACHEKIT_MASTER_KEY") { + // `env::var` hands back an owned copy of the hex secret. Wrap it so + // that copy is wiped on drop too — the decoded bytes below are + // already `Zeroizing`, but the hex form is the same key material. + let val = Zeroizing::new(val); config.master_key = Some(decode_master_key_hex(&val, "CACHEKIT_MASTER_KEY")?); } @@ -119,6 +123,9 @@ impl CachekitConfig { // treated as unset; a blank entry inside a non-blank list is still an // operator mistake. if let Ok(val) = std::env::var("CACHEKIT_PREVIOUS_MASTER_KEYS") { + // Same reasoning as CACHEKIT_MASTER_KEY above: wipe the owned copy + // of the hex list on drop. + let val = Zeroizing::new(val); if !val.trim().is_empty() { let mut previous = Vec::new(); for entry in val.split(',') { diff --git a/crates/cachekit/tests/config_tests.rs b/crates/cachekit/tests/config_tests.rs index 4e7a9c8..4496d53 100644 --- a/crates/cachekit/tests/config_tests.rs +++ b/crates/cachekit/tests/config_tests.rs @@ -1,6 +1,7 @@ use cachekit::config::{CachekitConfig, CachekitConfigBuilder}; use serial_test::serial; use std::time::Duration; +use zeroize::Zeroizing; // ── from_env defaults ──────────────────────────────────────────────────────── @@ -197,7 +198,10 @@ fn assert_config_err(result: Result)>, + /// `Zeroizing` because the saved set includes `CACHEKIT_MASTER_KEY` and + /// `CACHEKIT_PREVIOUS_MASTER_KEYS` — a pre-test shell value is real key + /// material, so the copy this guard holds is wiped on drop. + saved: Vec<(&'static str, Option>)>, } impl EnvGuard { @@ -206,7 +210,7 @@ impl EnvGuard { fn set(vars: &[(&'static str, Option<&str>)]) -> Self { let saved = vars .iter() - .map(|(name, _)| (*name, std::env::var(name).ok())) + .map(|(name, _)| (*name, std::env::var(name).ok().map(Zeroizing::new))) .collect(); for (name, value) in vars { match value { @@ -222,7 +226,7 @@ impl Drop for EnvGuard { fn drop(&mut self) { for (name, value) in &self.saved { match value { - Some(v) => std::env::set_var(name, v), + Some(v) => std::env::set_var(name, v.as_str()), None => std::env::remove_var(name), } } From 79bee95a7f12e5f8c56836508dce2435acf31bc0 Mon Sep 17 00:00:00 2001 From: Mark S Date: Sat, 8 Aug 2026 02:46:29 +1000 Subject: [PATCH 6/6] docs(config): scope the zeroise claim to the heap copy (LAB-686) Panel note: the comment implied wrapping the env string closes the exposure. It does not -- the process environ block holds the identical hex for the process lifetime and is not wiped. Name that, so the next reader does not over-trust it. --- crates/cachekit/src/config.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/cachekit/src/config.rs b/crates/cachekit/src/config.rs index 60aaf21..caf6236 100644 --- a/crates/cachekit/src/config.rs +++ b/crates/cachekit/src/config.rs @@ -113,6 +113,10 @@ impl CachekitConfig { // `env::var` hands back an owned copy of the hex secret. Wrap it so // that copy is wiped on drop too — the decoded bytes below are // already `Zeroizing`, but the hex form is the same key material. + // Defence in depth over the heap copy only: the process `environ` + // block still holds the identical hex for the process lifetime and + // is not wiped here, so this narrows post-lifetime recovery (core + // dumps, swap, heap reuse), it does not eliminate the exposure. let val = Zeroizing::new(val); config.master_key = Some(decode_master_key_hex(&val, "CACHEKIT_MASTER_KEY")?); }