From 5b4de12c5f8f7dd984ef9b346f508088ff8b9f67 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Mon, 13 Jul 2026 01:22:12 +0200 Subject: [PATCH] fix(vault): require --force to overwrite an existing secret AgeVaultProvider::set_secret_mut now takes an explicit overwrite flag and returns AgeVaultError::AlreadyExists when a key already exists and overwrite is not requested, instead of silently replacing it. The CLI `zeph vault set ` command gained a --force flag to opt in; without it, an existing key is left untouched and the error names the key without ever printing its value. The guard lives in the shared vault crate rather than a single call site, so every current and future caller (CLI, OAuth token refresh, durable-key wizard) must state its overwrite intent explicitly. This closes the same defect class as the ZEPH_DURABLE_KEY incident (#5874), one layer down from that fix's wizard-specific gate. Closes #5955 --- CHANGELOG.md | 13 ++++++ crates/zeph-vault/README.md | 2 +- crates/zeph-vault/src/age.rs | 81 +++++++++++++++++++++++++++++++--- crates/zeph-vault/src/arc.rs | 3 +- crates/zeph-vault/src/lib.rs | 20 ++++++--- src/bootstrap/oauth.rs | 6 ++- src/cli.rs | 4 ++ src/commands/durable.rs | 22 ++++++---- src/commands/vault.rs | 84 ++++++++++++++++++++++++++++++++++-- src/init/durable.rs | 34 ++++++++++++--- src/tests.rs | 15 ++++++- 11 files changed, 252 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7571cb5b3..663482978 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -80,6 +80,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Fixed +- `crates/zeph-vault/src/age.rs`: `AgeVaultProvider::set_secret_mut` silently overwrote an + existing secret with no confirmation, diff, or backup — the same defect class as the + `ZEPH_DURABLE_KEY` incident fixed for the `zeph init` wizard in #5880/#5874, but one layer + down in the vault crate itself, so every caller (CLI, wizards, OAuth credential store) + inherited the same silent-overwrite risk (#5955). `set_secret_mut` now takes an explicit + `overwrite: bool` and returns `AgeVaultError::AlreadyExists` when a key already exists and + `overwrite` is `false`, leaving the previous value untouched. `zeph vault set ` + gained a `--force` flag: without it, attempting to overwrite an existing key fails with a + clear error telling the operator to re-run with `--force`; the previous secret value is never + printed, only its presence. Call sites that intentionally always overwrite (OAuth token + refresh in `src/bootstrap/oauth.rs`, and `zeph init`'s durable-key wizard step, which already + gates rotation behind its own explicit "rotate" confirmation phrase) pass `overwrite: true` + explicitly. - `src/commands/skill.rs`/`src/commands/plugin.rs`: `zeph skill search`/`get` and `zeph plugin search`/`get` printed the correct, actionable FR-004 message (`REGISTRY_NOT_CONFIGURED_MSG`) to stdout when the registry is disabled, then diff --git a/crates/zeph-vault/README.md b/crates/zeph-vault/README.md index 2bb9a78af..1b5f4e71f 100644 --- a/crates/zeph-vault/README.md +++ b/crates/zeph-vault/README.md @@ -36,7 +36,7 @@ let mut vault = AgeVaultProvider::new( )?; // Store a secret, then persist the re-encrypted vault to disk. -vault.set_secret_mut("ZEPH_CLAUDE_API_KEY".to_owned(), "sk-ant-...".to_owned()); +vault.set_secret_mut("ZEPH_CLAUDE_API_KEY".to_owned(), "sk-ant-...".to_owned(), false)?; vault.save()?; // Retrieve a secret synchronously via the direct getter. diff --git a/crates/zeph-vault/src/age.rs b/crates/zeph-vault/src/age.rs index 520c2a17c..b2ebe5b65 100644 --- a/crates/zeph-vault/src/age.rs +++ b/crates/zeph-vault/src/age.rs @@ -66,6 +66,10 @@ pub enum AgeVaultError { /// The key file could not be written to disk. #[error("failed to write key file: {0}")] KeyWrite(std::io::Error), + /// [`AgeVaultProvider::set_secret_mut`] was called with `overwrite: false` for a key that + /// already exists in the vault. + #[error("secret key already exists: {0} (pass overwrite=true to replace it)")] + AlreadyExists(String), } // --------------------------------------------------------------------------- @@ -249,7 +253,7 @@ impl AgeVaultProvider { /// Path::new("/etc/zeph/vault-key.txt"), /// Path::new("/etc/zeph/secrets.age"), /// )?; - /// vault.set_secret_mut("MY_TOKEN".into(), "tok_abc123".into()); + /// vault.set_secret_mut("MY_TOKEN".into(), "tok_abc123".into(), false)?; /// vault.save()?; /// # Ok::<_, zeph_vault::AgeVaultError>(()) /// ``` @@ -282,7 +286,7 @@ impl AgeVaultProvider { /// Path::new("/etc/zeph/vault-key.txt"), /// Path::new("/etc/zeph/secrets.age"), /// )?; - /// vault.set_secret_mut("MY_TOKEN".into(), "tok_abc123".into()); + /// vault.set_secret_mut("MY_TOKEN".into(), "tok_abc123".into(), false)?; /// vault.save_async().await?; /// # Ok(()) /// # } @@ -309,8 +313,19 @@ impl AgeVaultProvider { /// Insert or update a secret in the in-memory map. /// + /// Refuses to replace an existing key unless `overwrite` is `true`, so that callers cannot + /// silently destroy a previously-stored secret by accident — see #5955 (and the sibling + /// incident #5874, which hit the same gap in the `zeph init` durable-execution wizard before + /// this guard existed at the vault layer). Callers that intend an unconditional update (e.g. + /// OAuth token refresh) pass `overwrite: true` explicitly. + /// /// Call [`save`][Self::save] afterwards to persist the change to disk. /// + /// # Errors + /// + /// Returns [`AgeVaultError::AlreadyExists`] if `key` is already present and `overwrite` is + /// `false`. The in-memory map is left untouched in that case. + /// /// # Examples /// /// ```no_run @@ -321,12 +336,21 @@ impl AgeVaultProvider { /// Path::new("/etc/zeph/vault-key.txt"), /// Path::new("/etc/zeph/secrets.age"), /// )?; - /// vault.set_secret_mut("API_KEY".into(), "sk-...".into()); + /// vault.set_secret_mut("API_KEY".into(), "sk-...".into(), false)?; /// vault.save()?; /// # Ok::<_, zeph_vault::AgeVaultError>(()) /// ``` - pub fn set_secret_mut(&mut self, key: String, value: String) { + pub fn set_secret_mut( + &mut self, + key: String, + value: String, + overwrite: bool, + ) -> Result<(), AgeVaultError> { + if !overwrite && self.secrets.contains_key(&key) { + return Err(AgeVaultError::AlreadyExists(key)); + } self.secrets.insert(key, Zeroizing::new(value)); + Ok(()) } /// Remove a secret from the in-memory map. @@ -563,7 +587,9 @@ mod tests { let (key_path, vault_path) = init_temp_vault(dir.path()); let mut vault = AgeVaultProvider::new(&key_path, &vault_path).unwrap(); - vault.set_secret_mut("KEY".into(), "val".into()); + vault + .set_secret_mut("KEY".into(), "val".into(), false) + .unwrap(); vault.save().unwrap(); let loaded = AgeVaultProvider::load(&key_path, &vault_path).unwrap(); @@ -576,7 +602,9 @@ mod tests { let (key_path, vault_path) = init_temp_vault(dir.path()); let mut vault = AgeVaultProvider::new(&key_path, &vault_path).unwrap(); - vault.set_secret_mut("KEY".into(), "val".into()); + vault + .set_secret_mut("KEY".into(), "val".into(), false) + .unwrap(); assert!(vault.remove_secret_mut("KEY")); assert!(!vault.remove_secret_mut("KEY")); @@ -656,11 +684,50 @@ mod tests { let (key_path, vault_path) = init_temp_vault(dir.path()); let mut vault = AgeVaultProvider::new(&key_path, &vault_path).unwrap(); - vault.set_secret_mut("TMP_TEST".into(), "value".into()); + vault + .set_secret_mut("TMP_TEST".into(), "value".into(), false) + .unwrap(); vault.save().unwrap(); let tmp_path = vault_path.with_added_extension("tmp"); assert!(!tmp_path.exists(), ".age.tmp must not exist after save()"); assert!(vault_path.exists(), "secrets.age must exist after save()"); } + + /// Regression for #5955: `set_secret_mut` must refuse to replace an existing key when + /// `overwrite` is `false`, and must leave the previous value untouched. + #[test] + fn set_secret_mut_rejects_overwrite_when_not_requested() { + let dir = tempdir().unwrap(); + let (key_path, vault_path) = init_temp_vault(dir.path()); + + let mut vault = AgeVaultProvider::new(&key_path, &vault_path).unwrap(); + vault + .set_secret_mut("KEY".into(), "original".into(), false) + .unwrap(); + + let result = vault.set_secret_mut("KEY".into(), "clobbered".into(), false); + assert!( + matches!(result, Err(AgeVaultError::AlreadyExists(ref k)) if k == "KEY"), + "expected AlreadyExists(\"KEY\"), got {result:?}", + ); + assert_eq!(vault.get("KEY"), Some("original")); + } + + /// Regression for #5955: `overwrite: true` must replace an existing value. + #[test] + fn set_secret_mut_replaces_when_overwrite_requested() { + let dir = tempdir().unwrap(); + let (key_path, vault_path) = init_temp_vault(dir.path()); + + let mut vault = AgeVaultProvider::new(&key_path, &vault_path).unwrap(); + vault + .set_secret_mut("KEY".into(), "original".into(), false) + .unwrap(); + vault + .set_secret_mut("KEY".into(), "updated".into(), true) + .unwrap(); + + assert_eq!(vault.get("KEY"), Some("updated")); + } } diff --git a/crates/zeph-vault/src/arc.rs b/crates/zeph-vault/src/arc.rs index 7124f7793..63e985077 100644 --- a/crates/zeph-vault/src/arc.rs +++ b/crates/zeph-vault/src/arc.rs @@ -91,7 +91,8 @@ mod tests { ) .unwrap(); for (k, v) in keys { - age.set_secret_mut((*k).to_owned(), (*v).to_owned()); + age.set_secret_mut((*k).to_owned(), (*v).to_owned(), false) + .unwrap(); } // Keep tempdir alive by leaking — tests are short-lived, no I/O after this. std::mem::forget(dir); diff --git a/crates/zeph-vault/src/lib.rs b/crates/zeph-vault/src/lib.rs index c38c46d21..a68d80a7b 100644 --- a/crates/zeph-vault/src/lib.rs +++ b/crates/zeph-vault/src/lib.rs @@ -501,8 +501,12 @@ mod age_tests { let (_dir, key_path, vault_path) = write_temp_files(&identity, &encrypted); let mut vault = AgeVaultProvider::load(&key_path, &vault_path).unwrap(); - vault.set_secret_mut("B".to_owned(), "2".to_owned()); - vault.set_secret_mut("C".to_owned(), "3".to_owned()); + vault + .set_secret_mut("B".to_owned(), "2".to_owned(), false) + .unwrap(); + vault + .set_secret_mut("C".to_owned(), "3".to_owned(), false) + .unwrap(); let keys = vault.list_keys(); assert_eq!(keys, vec!["A", "B", "C"]); @@ -529,7 +533,9 @@ mod age_tests { let (_dir, key_path, vault_path) = write_temp_files(&identity, &encrypted); let mut vault = AgeVaultProvider::load(&key_path, &vault_path).unwrap(); - vault.set_secret_mut("NEW_KEY".to_owned(), "new_value".to_owned()); + vault + .set_secret_mut("NEW_KEY".to_owned(), "new_value".to_owned(), false) + .unwrap(); vault.save().unwrap(); let reloaded = AgeVaultProvider::load(&key_path, &vault_path).unwrap(); @@ -598,7 +604,9 @@ mod age_tests { let (_dir, key_path, vault_path) = write_temp_files(&identity, &encrypted); let mut vault = AgeVaultProvider::load(&key_path, &vault_path).unwrap(); - vault.set_secret_mut("B_KEY".to_owned(), "b".to_owned()); + vault + .set_secret_mut("B_KEY".to_owned(), "b".to_owned(), false) + .unwrap(); vault.save().unwrap(); let reloaded = AgeVaultProvider::load(&key_path, &vault_path).unwrap(); @@ -654,7 +662,9 @@ mod age_tests { let mut vault = AgeVaultProvider::load_async(&key_path, &vault_path) .await .unwrap(); - vault.set_secret_mut("ADDED".to_owned(), "added_val".to_owned()); + vault + .set_secret_mut("ADDED".to_owned(), "added_val".to_owned(), false) + .unwrap(); vault.save_async().await.unwrap(); let reloaded = AgeVaultProvider::load_async(&key_path, &vault_path) diff --git a/src/bootstrap/oauth.rs b/src/bootstrap/oauth.rs index 8ce27e51b..1783608d5 100644 --- a/src/bootstrap/oauth.rs +++ b/src/bootstrap/oauth.rs @@ -69,7 +69,11 @@ impl CredentialStore for VaultCredentialStore { let mut guard = vault .write() .unwrap_or_else(std::sync::PoisonError::into_inner); - guard.set_secret_mut(key, json); + // OAuth credential refresh is an intentional update of the store's own managed + // entry, not a user-facing secret set — always overwrite. + guard + .set_secret_mut(key, json, true) + .map_err(|e| AuthError::InternalError(format!("vault save: {e}")))?; guard .save() .map_err(|e| AuthError::InternalError(format!("vault save: {e}"))) diff --git a/src/cli.rs b/src/cli.rs index dc37e1c58..3a8e9e5f8 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -1139,6 +1139,10 @@ pub(crate) enum VaultCommand { key: String, #[arg()] value: String, + /// Overwrite an existing key. Without this flag, `vault set` refuses to replace + /// a key that is already present in the vault. + #[arg(long)] + force: bool, }, /// Decrypt and print a secret value Get { diff --git a/src/commands/durable.rs b/src/commands/durable.rs index 559298772..df5ae9cce 100644 --- a/src/commands/durable.rs +++ b/src/commands/durable.rs @@ -747,10 +747,13 @@ mod tests { &vault_root.join("secrets.age"), ) .unwrap(); - provider.set_secret_mut( - "ZEPH_DURABLE_KEY".to_owned(), - zeph_core::durable::generate_durable_key_b64(), - ); + provider + .set_secret_mut( + "ZEPH_DURABLE_KEY".to_owned(), + zeph_core::durable::generate_durable_key_b64(), + false, + ) + .unwrap(); provider.save().unwrap(); // Exercise the exact glue the write paths (runner.rs, scheduler_daemon.rs) call. @@ -943,10 +946,13 @@ mod tests { &vault_root.join("secrets.age"), ) .unwrap(); - provider.set_secret_mut( - "ZEPH_DURABLE_KEY".to_owned(), - zeph_core::durable::generate_durable_key_b64(), - ); + provider + .set_secret_mut( + "ZEPH_DURABLE_KEY".to_owned(), + zeph_core::durable::generate_durable_key_b64(), + false, + ) + .unwrap(); provider.save().unwrap(); let result = load_write_hmac_key(&config); diff --git a/src/commands/vault.rs b/src/commands/vault.rs index 24a0f3b3c..a2d8c6b8c 100644 --- a/src/commands/vault.rs +++ b/src/commands/vault.rs @@ -3,7 +3,7 @@ use std::path::PathBuf; -use zeph_core::vault::AgeVaultProvider; +use zeph_core::vault::{AgeVaultError, AgeVaultProvider}; use crate::cli::VaultCommand; @@ -25,10 +25,18 @@ pub(crate) fn handle_vault_command( AgeVaultProvider::init_vault(&dir) .map_err(|e| anyhow::anyhow!("vault init failed: {e}"))?; } - VaultCommand::Set { key, value } => { + VaultCommand::Set { key, value, force } => { let mut provider = AgeVaultProvider::load(&key_path_owned, &vault_path_owned) .map_err(|e| anyhow::anyhow!("failed to load vault: {e}"))?; - provider.set_secret_mut(key, value); + provider + .set_secret_mut(key.clone(), value, force) + .map_err(|e| match e { + AgeVaultError::AlreadyExists(_) => anyhow::anyhow!( + "key '{key}' already exists in the vault. Re-run with --force to \ + overwrite it." + ), + other => anyhow::anyhow!("failed to set secret: {other}"), + })?; provider .save() .map_err(|e| anyhow::anyhow!("failed to save vault: {e}"))?; @@ -128,6 +136,7 @@ mod tests { VaultCommand::Set { key: "FOO".into(), value: "bar".into(), + force: false, }, Some(&key_path), Some(&vault_path), @@ -170,6 +179,75 @@ mod tests { assert!(err.to_string().contains("key not found")); } + // R-05: Set refuses to silently overwrite an existing key (#5955) + #[test] + fn handle_vault_command_set_existing_key_without_force_errors() { + let dir = tempfile::tempdir().unwrap(); + let key_path = dir.path().join("vault-key.txt"); + let vault_path = dir.path().join("secrets.age"); + zeph_core::vault::AgeVaultProvider::init_vault(dir.path()).unwrap(); + + handle_vault_command( + VaultCommand::Set { + key: "FOO".into(), + value: "original".into(), + force: false, + }, + Some(&key_path), + Some(&vault_path), + ) + .unwrap(); + + let err = handle_vault_command( + VaultCommand::Set { + key: "FOO".into(), + value: "clobbered".into(), + force: false, + }, + Some(&key_path), + Some(&vault_path), + ) + .unwrap_err(); + assert!(err.to_string().contains("--force")); + + // The original value must survive the rejected overwrite attempt. + let provider = zeph_core::vault::AgeVaultProvider::load(&key_path, &vault_path).unwrap(); + assert_eq!(provider.get("FOO"), Some("original")); + } + + #[test] + fn handle_vault_command_set_existing_key_with_force_overwrites() { + let dir = tempfile::tempdir().unwrap(); + let key_path = dir.path().join("vault-key.txt"); + let vault_path = dir.path().join("secrets.age"); + zeph_core::vault::AgeVaultProvider::init_vault(dir.path()).unwrap(); + + handle_vault_command( + VaultCommand::Set { + key: "FOO".into(), + value: "original".into(), + force: false, + }, + Some(&key_path), + Some(&vault_path), + ) + .unwrap(); + + handle_vault_command( + VaultCommand::Set { + key: "FOO".into(), + value: "updated".into(), + force: true, + }, + Some(&key_path), + Some(&vault_path), + ) + .unwrap(); + + let provider = zeph_core::vault::AgeVaultProvider::load(&key_path, &vault_path).unwrap(); + assert_eq!(provider.get("FOO"), Some("updated")); + } + #[test] fn handle_vault_command_rm_missing_key_errors() { let dir = tempfile::tempdir().unwrap(); diff --git a/src/init/durable.rs b/src/init/durable.rs index e7a23b4a8..41bc47fd4 100644 --- a/src/init/durable.rs +++ b/src/init/durable.rs @@ -170,7 +170,11 @@ pub(super) fn store_durable_key(state: &WizardState) -> anyhow::Result<()> { } let mut provider = zeph_core::vault::AgeVaultProvider::load(&key_path, &vault_path) .map_err(|e| anyhow::anyhow!("failed to load age vault: {e}"))?; - provider.set_secret_mut("ZEPH_DURABLE_KEY".to_owned(), key.to_owned()); + // Reaching this point already implies the caller's own rotation gate approved an + // overwrite (no pre-existing key, or the user typed the "rotate" confirmation above). + provider + .set_secret_mut("ZEPH_DURABLE_KEY".to_owned(), key.to_owned(), true) + .map_err(|e| anyhow::anyhow!("failed to set ZEPH_DURABLE_KEY: {e}"))?; provider .save() .map_err(|e| anyhow::anyhow!("failed to save age vault: {e}"))?; @@ -249,7 +253,13 @@ mod tests { let vault_path = vault_root.join("secrets.age"); let mut provider = zeph_core::vault::AgeVaultProvider::load(&key_path, &vault_path).expect("load vault"); - provider.set_secret_mut("ZEPH_DURABLE_KEY".to_owned(), "existing-key".to_owned()); + provider + .set_secret_mut( + "ZEPH_DURABLE_KEY".to_owned(), + "existing-key".to_owned(), + false, + ) + .expect("set secret"); provider.save().expect("save vault"); let state = durable_state(None); @@ -273,7 +283,13 @@ mod tests { let vault_path = vault_root.join("secrets.age"); let mut provider = zeph_core::vault::AgeVaultProvider::load(&key_path, &vault_path).expect("load vault"); - provider.set_secret_mut("ZEPH_DURABLE_KEY".to_owned(), "existing-key".to_owned()); + provider + .set_secret_mut( + "ZEPH_DURABLE_KEY".to_owned(), + "existing-key".to_owned(), + false, + ) + .expect("set secret"); provider.save().expect("save vault"); let new_key = zeph_core::durable::generate_durable_key_b64(); @@ -355,7 +371,13 @@ mod tests { let vault_path = dir.path().join("secrets.age"); let mut provider = zeph_core::vault::AgeVaultProvider::load(&key_path, &vault_path).expect("load vault"); - provider.set_secret_mut("ZEPH_DURABLE_KEY".to_owned(), "existing-key".to_owned()); + provider + .set_secret_mut( + "ZEPH_DURABLE_KEY".to_owned(), + "existing-key".to_owned(), + false, + ) + .expect("set secret"); provider.save().expect("save vault"); assert!(vault_has_durable_key(dir.path()).expect("should not error")); @@ -369,7 +391,9 @@ mod tests { let vault_path = dir.path().join("secrets.age"); let mut provider = zeph_core::vault::AgeVaultProvider::load(&key_path, &vault_path).expect("load vault"); - provider.set_secret_mut("ZEPH_OTHER_KEY".to_owned(), "value".to_owned()); + provider + .set_secret_mut("ZEPH_OTHER_KEY".to_owned(), "value".to_owned(), false) + .expect("set secret"); provider.save().expect("save vault"); assert!(!vault_has_durable_key(dir.path()).expect("should not error")); diff --git a/src/tests.rs b/src/tests.rs index 5816e776e..3ea27880c 100644 --- a/src/tests.rs +++ b/src/tests.rs @@ -641,15 +641,28 @@ fn cli_parse_vault_set() { let cli = Cli::try_parse_from(["zeph", "vault", "set", "MY_KEY", "MY_VAL"]).unwrap(); match cli.command { Some(Command::Vault { - command: VaultCommand::Set { key, value }, + command: VaultCommand::Set { key, value, force }, }) => { assert_eq!(key, "MY_KEY"); assert_eq!(value, "MY_VAL"); + assert!(!force, "force must default to false"); } _ => panic!("expected VaultCommand::Set"), } } +// Regression for #5955: `--force` must parse and set `force = true`. +#[test] +fn cli_parse_vault_set_force() { + let cli = Cli::try_parse_from(["zeph", "vault", "set", "MY_KEY", "MY_VAL", "--force"]).unwrap(); + match cli.command { + Some(Command::Vault { + command: VaultCommand::Set { force, .. }, + }) => assert!(force, "--force must set force = true"), + _ => panic!("expected VaultCommand::Set"), + } +} + #[test] fn cli_parse_vault_get() { let cli = Cli::try_parse_from(["zeph", "vault", "get", "MY_KEY"]).unwrap();