Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <key> <value>`
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
Expand Down
2 changes: 1 addition & 1 deletion crates/zeph-vault/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
81 changes: 74 additions & 7 deletions crates/zeph-vault/src/age.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
}

// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -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>(())
/// ```
Expand Down Expand Up @@ -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(())
/// # }
Expand All @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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();
Expand All @@ -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"));
Expand Down Expand Up @@ -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"));
}
}
3 changes: 2 additions & 1 deletion crates/zeph-vault/src/arc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
20 changes: 15 additions & 5 deletions crates/zeph-vault/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"]);
Expand All @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 5 additions & 1 deletion src/bootstrap/oauth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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}")))
Expand Down
4 changes: 4 additions & 0 deletions src/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
22 changes: 14 additions & 8 deletions src/commands/durable.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down
Loading
Loading