Skip to content

Commit d2e69f8

Browse files
committed
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 <key> <value>` 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
1 parent 457ffd4 commit d2e69f8

11 files changed

Lines changed: 252 additions & 32 deletions

File tree

CHANGELOG.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
5454

5555
### Fixed
5656

57+
- `crates/zeph-vault/src/age.rs`: `AgeVaultProvider::set_secret_mut` silently overwrote an
58+
existing secret with no confirmation, diff, or backup — the same defect class as the
59+
`ZEPH_DURABLE_KEY` incident fixed for the `zeph init` wizard in #5880/#5874, but one layer
60+
down in the vault crate itself, so every caller (CLI, wizards, OAuth credential store)
61+
inherited the same silent-overwrite risk (#5955). `set_secret_mut` now takes an explicit
62+
`overwrite: bool` and returns `AgeVaultError::AlreadyExists` when a key already exists and
63+
`overwrite` is `false`, leaving the previous value untouched. `zeph vault set <key> <value>`
64+
gained a `--force` flag: without it, attempting to overwrite an existing key fails with a
65+
clear error telling the operator to re-run with `--force`; the previous secret value is never
66+
printed, only its presence. Call sites that intentionally always overwrite (OAuth token
67+
refresh in `src/bootstrap/oauth.rs`, and `zeph init`'s durable-key wizard step, which already
68+
gates rotation behind its own explicit "rotate" confirmation phrase) pass `overwrite: true`
69+
explicitly.
5770
- `src/commands/skill.rs`/`src/commands/plugin.rs`: `zeph skill search`/`get` and
5871
`zeph plugin search`/`get` printed the correct, actionable FR-004 message
5972
(`REGISTRY_NOT_CONFIGURED_MSG`) to stdout when the registry is disabled, then

crates/zeph-vault/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ let mut vault = AgeVaultProvider::new(
3636
)?;
3737
3838
// Store a secret, then persist the re-encrypted vault to disk.
39-
vault.set_secret_mut("ZEPH_CLAUDE_API_KEY".to_owned(), "sk-ant-...".to_owned());
39+
vault.set_secret_mut("ZEPH_CLAUDE_API_KEY".to_owned(), "sk-ant-...".to_owned(), false)?;
4040
vault.save()?;
4141
4242
// Retrieve a secret synchronously via the direct getter.

crates/zeph-vault/src/age.rs

Lines changed: 74 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,10 @@ pub enum AgeVaultError {
6666
/// The key file could not be written to disk.
6767
#[error("failed to write key file: {0}")]
6868
KeyWrite(std::io::Error),
69+
/// [`AgeVaultProvider::set_secret_mut`] was called with `overwrite: false` for a key that
70+
/// already exists in the vault.
71+
#[error("secret key already exists: {0} (pass overwrite=true to replace it)")]
72+
AlreadyExists(String),
6973
}
7074

7175
// ---------------------------------------------------------------------------
@@ -249,7 +253,7 @@ impl AgeVaultProvider {
249253
/// Path::new("/etc/zeph/vault-key.txt"),
250254
/// Path::new("/etc/zeph/secrets.age"),
251255
/// )?;
252-
/// vault.set_secret_mut("MY_TOKEN".into(), "tok_abc123".into());
256+
/// vault.set_secret_mut("MY_TOKEN".into(), "tok_abc123".into(), false)?;
253257
/// vault.save()?;
254258
/// # Ok::<_, zeph_vault::AgeVaultError>(())
255259
/// ```
@@ -282,7 +286,7 @@ impl AgeVaultProvider {
282286
/// Path::new("/etc/zeph/vault-key.txt"),
283287
/// Path::new("/etc/zeph/secrets.age"),
284288
/// )?;
285-
/// vault.set_secret_mut("MY_TOKEN".into(), "tok_abc123".into());
289+
/// vault.set_secret_mut("MY_TOKEN".into(), "tok_abc123".into(), false)?;
286290
/// vault.save_async().await?;
287291
/// # Ok(())
288292
/// # }
@@ -309,8 +313,19 @@ impl AgeVaultProvider {
309313

310314
/// Insert or update a secret in the in-memory map.
311315
///
316+
/// Refuses to replace an existing key unless `overwrite` is `true`, so that callers cannot
317+
/// silently destroy a previously-stored secret by accident — see #5955 (and the sibling
318+
/// incident #5874, which hit the same gap in the `zeph init` durable-execution wizard before
319+
/// this guard existed at the vault layer). Callers that intend an unconditional update (e.g.
320+
/// OAuth token refresh) pass `overwrite: true` explicitly.
321+
///
312322
/// Call [`save`][Self::save] afterwards to persist the change to disk.
313323
///
324+
/// # Errors
325+
///
326+
/// Returns [`AgeVaultError::AlreadyExists`] if `key` is already present and `overwrite` is
327+
/// `false`. The in-memory map is left untouched in that case.
328+
///
314329
/// # Examples
315330
///
316331
/// ```no_run
@@ -321,12 +336,21 @@ impl AgeVaultProvider {
321336
/// Path::new("/etc/zeph/vault-key.txt"),
322337
/// Path::new("/etc/zeph/secrets.age"),
323338
/// )?;
324-
/// vault.set_secret_mut("API_KEY".into(), "sk-...".into());
339+
/// vault.set_secret_mut("API_KEY".into(), "sk-...".into(), false)?;
325340
/// vault.save()?;
326341
/// # Ok::<_, zeph_vault::AgeVaultError>(())
327342
/// ```
328-
pub fn set_secret_mut(&mut self, key: String, value: String) {
343+
pub fn set_secret_mut(
344+
&mut self,
345+
key: String,
346+
value: String,
347+
overwrite: bool,
348+
) -> Result<(), AgeVaultError> {
349+
if !overwrite && self.secrets.contains_key(&key) {
350+
return Err(AgeVaultError::AlreadyExists(key));
351+
}
329352
self.secrets.insert(key, Zeroizing::new(value));
353+
Ok(())
330354
}
331355

332356
/// Remove a secret from the in-memory map.
@@ -563,7 +587,9 @@ mod tests {
563587
let (key_path, vault_path) = init_temp_vault(dir.path());
564588

565589
let mut vault = AgeVaultProvider::new(&key_path, &vault_path).unwrap();
566-
vault.set_secret_mut("KEY".into(), "val".into());
590+
vault
591+
.set_secret_mut("KEY".into(), "val".into(), false)
592+
.unwrap();
567593
vault.save().unwrap();
568594

569595
let loaded = AgeVaultProvider::load(&key_path, &vault_path).unwrap();
@@ -576,7 +602,9 @@ mod tests {
576602
let (key_path, vault_path) = init_temp_vault(dir.path());
577603

578604
let mut vault = AgeVaultProvider::new(&key_path, &vault_path).unwrap();
579-
vault.set_secret_mut("KEY".into(), "val".into());
605+
vault
606+
.set_secret_mut("KEY".into(), "val".into(), false)
607+
.unwrap();
580608

581609
assert!(vault.remove_secret_mut("KEY"));
582610
assert!(!vault.remove_secret_mut("KEY"));
@@ -656,11 +684,50 @@ mod tests {
656684
let (key_path, vault_path) = init_temp_vault(dir.path());
657685

658686
let mut vault = AgeVaultProvider::new(&key_path, &vault_path).unwrap();
659-
vault.set_secret_mut("TMP_TEST".into(), "value".into());
687+
vault
688+
.set_secret_mut("TMP_TEST".into(), "value".into(), false)
689+
.unwrap();
660690
vault.save().unwrap();
661691

662692
let tmp_path = vault_path.with_added_extension("tmp");
663693
assert!(!tmp_path.exists(), ".age.tmp must not exist after save()");
664694
assert!(vault_path.exists(), "secrets.age must exist after save()");
665695
}
696+
697+
/// Regression for #5955: `set_secret_mut` must refuse to replace an existing key when
698+
/// `overwrite` is `false`, and must leave the previous value untouched.
699+
#[test]
700+
fn set_secret_mut_rejects_overwrite_when_not_requested() {
701+
let dir = tempdir().unwrap();
702+
let (key_path, vault_path) = init_temp_vault(dir.path());
703+
704+
let mut vault = AgeVaultProvider::new(&key_path, &vault_path).unwrap();
705+
vault
706+
.set_secret_mut("KEY".into(), "original".into(), false)
707+
.unwrap();
708+
709+
let result = vault.set_secret_mut("KEY".into(), "clobbered".into(), false);
710+
assert!(
711+
matches!(result, Err(AgeVaultError::AlreadyExists(ref k)) if k == "KEY"),
712+
"expected AlreadyExists(\"KEY\"), got {result:?}",
713+
);
714+
assert_eq!(vault.get("KEY"), Some("original"));
715+
}
716+
717+
/// Regression for #5955: `overwrite: true` must replace an existing value.
718+
#[test]
719+
fn set_secret_mut_replaces_when_overwrite_requested() {
720+
let dir = tempdir().unwrap();
721+
let (key_path, vault_path) = init_temp_vault(dir.path());
722+
723+
let mut vault = AgeVaultProvider::new(&key_path, &vault_path).unwrap();
724+
vault
725+
.set_secret_mut("KEY".into(), "original".into(), false)
726+
.unwrap();
727+
vault
728+
.set_secret_mut("KEY".into(), "updated".into(), true)
729+
.unwrap();
730+
731+
assert_eq!(vault.get("KEY"), Some("updated"));
732+
}
666733
}

crates/zeph-vault/src/arc.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,8 @@ mod tests {
9191
)
9292
.unwrap();
9393
for (k, v) in keys {
94-
age.set_secret_mut((*k).to_owned(), (*v).to_owned());
94+
age.set_secret_mut((*k).to_owned(), (*v).to_owned(), false)
95+
.unwrap();
9596
}
9697
// Keep tempdir alive by leaking — tests are short-lived, no I/O after this.
9798
std::mem::forget(dir);

crates/zeph-vault/src/lib.rs

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -501,8 +501,12 @@ mod age_tests {
501501
let (_dir, key_path, vault_path) = write_temp_files(&identity, &encrypted);
502502

503503
let mut vault = AgeVaultProvider::load(&key_path, &vault_path).unwrap();
504-
vault.set_secret_mut("B".to_owned(), "2".to_owned());
505-
vault.set_secret_mut("C".to_owned(), "3".to_owned());
504+
vault
505+
.set_secret_mut("B".to_owned(), "2".to_owned(), false)
506+
.unwrap();
507+
vault
508+
.set_secret_mut("C".to_owned(), "3".to_owned(), false)
509+
.unwrap();
506510

507511
let keys = vault.list_keys();
508512
assert_eq!(keys, vec!["A", "B", "C"]);
@@ -529,7 +533,9 @@ mod age_tests {
529533
let (_dir, key_path, vault_path) = write_temp_files(&identity, &encrypted);
530534

531535
let mut vault = AgeVaultProvider::load(&key_path, &vault_path).unwrap();
532-
vault.set_secret_mut("NEW_KEY".to_owned(), "new_value".to_owned());
536+
vault
537+
.set_secret_mut("NEW_KEY".to_owned(), "new_value".to_owned(), false)
538+
.unwrap();
533539
vault.save().unwrap();
534540

535541
let reloaded = AgeVaultProvider::load(&key_path, &vault_path).unwrap();
@@ -598,7 +604,9 @@ mod age_tests {
598604
let (_dir, key_path, vault_path) = write_temp_files(&identity, &encrypted);
599605

600606
let mut vault = AgeVaultProvider::load(&key_path, &vault_path).unwrap();
601-
vault.set_secret_mut("B_KEY".to_owned(), "b".to_owned());
607+
vault
608+
.set_secret_mut("B_KEY".to_owned(), "b".to_owned(), false)
609+
.unwrap();
602610
vault.save().unwrap();
603611

604612
let reloaded = AgeVaultProvider::load(&key_path, &vault_path).unwrap();
@@ -654,7 +662,9 @@ mod age_tests {
654662
let mut vault = AgeVaultProvider::load_async(&key_path, &vault_path)
655663
.await
656664
.unwrap();
657-
vault.set_secret_mut("ADDED".to_owned(), "added_val".to_owned());
665+
vault
666+
.set_secret_mut("ADDED".to_owned(), "added_val".to_owned(), false)
667+
.unwrap();
658668
vault.save_async().await.unwrap();
659669

660670
let reloaded = AgeVaultProvider::load_async(&key_path, &vault_path)

src/bootstrap/oauth.rs

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,11 @@ impl CredentialStore for VaultCredentialStore {
6969
let mut guard = vault
7070
.write()
7171
.unwrap_or_else(std::sync::PoisonError::into_inner);
72-
guard.set_secret_mut(key, json);
72+
// OAuth credential refresh is an intentional update of the store's own managed
73+
// entry, not a user-facing secret set — always overwrite.
74+
guard
75+
.set_secret_mut(key, json, true)
76+
.map_err(|e| AuthError::InternalError(format!("vault save: {e}")))?;
7377
guard
7478
.save()
7579
.map_err(|e| AuthError::InternalError(format!("vault save: {e}")))

src/cli.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1139,6 +1139,10 @@ pub(crate) enum VaultCommand {
11391139
key: String,
11401140
#[arg()]
11411141
value: String,
1142+
/// Overwrite an existing key. Without this flag, `vault set` refuses to replace
1143+
/// a key that is already present in the vault.
1144+
#[arg(long)]
1145+
force: bool,
11421146
},
11431147
/// Decrypt and print a secret value
11441148
Get {

src/commands/durable.rs

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -747,10 +747,13 @@ mod tests {
747747
&vault_root.join("secrets.age"),
748748
)
749749
.unwrap();
750-
provider.set_secret_mut(
751-
"ZEPH_DURABLE_KEY".to_owned(),
752-
zeph_core::durable::generate_durable_key_b64(),
753-
);
750+
provider
751+
.set_secret_mut(
752+
"ZEPH_DURABLE_KEY".to_owned(),
753+
zeph_core::durable::generate_durable_key_b64(),
754+
false,
755+
)
756+
.unwrap();
754757
provider.save().unwrap();
755758

756759
// Exercise the exact glue the write paths (runner.rs, scheduler_daemon.rs) call.
@@ -943,10 +946,13 @@ mod tests {
943946
&vault_root.join("secrets.age"),
944947
)
945948
.unwrap();
946-
provider.set_secret_mut(
947-
"ZEPH_DURABLE_KEY".to_owned(),
948-
zeph_core::durable::generate_durable_key_b64(),
949-
);
949+
provider
950+
.set_secret_mut(
951+
"ZEPH_DURABLE_KEY".to_owned(),
952+
zeph_core::durable::generate_durable_key_b64(),
953+
false,
954+
)
955+
.unwrap();
950956
provider.save().unwrap();
951957

952958
let result = load_write_hmac_key(&config);

0 commit comments

Comments
 (0)