From 159898d8f6e471dddf579fcb5ad49fc0bb24853f Mon Sep 17 00:00:00 2001 From: Duncan Date: Mon, 17 Aug 2026 18:28:40 -0400 Subject: [PATCH] fix(desktop): verify minted keys via durable OS read-back not cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit KeyStore::write_and_verify confirmed a write by calling load(), which returns the in-process cache that store() itself just advanced — proving the cache was updated, not that the OS keyring durably holds the value. mint_bound_identity relies on this before returning a commit-ready binding, so a backend that acks a write without persisting it could let Phase 4b commit an identity binding whose only secret dies with the process, violating the §2.5 invariant that no binding exists with an unverified key. Route the production confirmation through SecretStore::verify_stored_raw, which bypasses the cache and reads the OS backend directly — the same primitive the identity path already uses. The other caller (migrate_inline_key) is upgraded for free; its Ok/Err contract is unchanged, only strengthened to mean durably-verified. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src-tauri/src/managed_agents/library.rs | 22 ++++--- .../src-tauri/src/managed_agents/storage.rs | 18 ++++-- .../src/managed_agents/storage_tests.rs | 59 +++++++++++++++++++ 3 files changed, 86 insertions(+), 13 deletions(-) diff --git a/desktop/src-tauri/src/managed_agents/library.rs b/desktop/src-tauri/src/managed_agents/library.rs index a441eeeba..bcb4760a3 100644 --- a/desktop/src-tauri/src/managed_agents/library.rs +++ b/desktop/src-tauri/src/managed_agents/library.rs @@ -784,9 +784,11 @@ pub(crate) struct MintedIdentity { /// 1. generate the keypair in memory (nothing persisted anywhere); /// 2. journal its derived pubkey to `orphan_keys` and DURABLY persist the /// document via `persist` — the coordinate outlives a crash; -/// 3. write the nsec to the keyring and read it back to verify; -/// 4. build the verified binding from the READ-BACK nsec (never the in-memory -/// copy) — proof the keyring entry backs exactly this pubkey. +/// 3. write the nsec to the keyring and confirm it is DURABLY retrievable +/// (raw OS read-back via [`KeyStore::write_and_verify`], not a cache read); +/// 4. build the verified binding from the keyring's own read-back of the nsec +/// (never the in-memory copy) — proof the keyring entry backs exactly this +/// pubkey. /// /// On `Ok`, the orphan row is left IN `document`; the caller commits `binding` /// and drops the row in ONE atomic write (Phase 4b), so a crash before that @@ -815,8 +817,10 @@ pub(crate) fn mint_bound_identity( document.journal_orphan_pubkey(&agent_pubkey); persist(document)?; - // (3) keyring write + read-back verify. A crash at or after this leaves the - // durable orphan row (step 2) whose keyring entry the recovery sweep reaps. + // (3) keyring write + DURABLE read-back verify (raw OS round-trip, not a + // cache read — see KeyStore::write_and_verify). A crash at or after this + // leaves the durable orphan row (step 2) whose keyring entry the recovery + // sweep reaps. let name = agent_keyring_name(&agent_pubkey); let nsec = agent_keys .secret_key() @@ -824,9 +828,11 @@ pub(crate) fn mint_bound_identity( .map_err(|e| format!("encode minted nsec: {e}"))?; store.write_and_verify(&name, &nsec)?; - // (4) build the binding from the READ-BACK nsec — proves the keyring entry - // backs exactly this pubkey. The caller commits it and removes the orphan - // row in one atomic write (Phase 4b). + // (4) build the binding from the keyring's read-back — proves the entry + // backs exactly this pubkey. Sound because step 3 already confirmed the + // value is durably retrievable, so this load reflects the verified backend + // state; an absent value here is still fail-closed. The caller commits the + // binding and removes the orphan row in one atomic write (Phase 4b). let read_back = store.load(&name)?.ok_or_else(|| { "minted key absent from keyring immediately after verified write".to_string() })?; diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index 9ab361fb2..c02b5bb94 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -159,8 +159,13 @@ pub(crate) trait KeyStore { /// `Ok(None)` when no blob exists yet; `Err` only on backend failure. /// Callers must not call `migrate_legacy_key` — this is a read-only view. fn load_all_readonly(&self) -> Result>, String>; - /// Write `value` and read it back to confirm before the caller strips the - /// inline copy. + /// Write `value` and confirm it is durably retrievable from the OS keyring + /// before the caller strips the inline copy or commits a binding. The + /// confirmation MUST read through the backend, NOT merely the in-process + /// cache the write itself just advanced — it proves the OS keyring + /// round-trip, so a backend that acknowledges a write without durably + /// persisting the value fails here rather than reporting a false success + /// (see [`SecretStore::verify_stored_raw`]). fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String>; /// Insert all entries from `entries` in a single blob mutation. fn store_all(&self, entries: &HashMap) -> Result<(), String>; @@ -183,9 +188,12 @@ impl KeyStore for SecretStore { } fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String> { self.store(name, value)?; - match self.load(name)? { - Some(stored) if stored == value => Ok(()), - _ => Err("keyring read-back verify failed".to_string()), + // Confirm through the OS backend, bypassing the cache the `store` above + // just advanced — proving durable retrievability, not merely that the + // cache was updated (see `SecretStore::verify_stored_raw`). + match self.verify_stored_raw(name, value)? { + true => Ok(()), + false => Err("keyring read-back verify failed".to_string()), } } fn store_all(&self, entries: &HashMap) -> Result<(), String> { diff --git a/desktop/src-tauri/src/managed_agents/storage_tests.rs b/desktop/src-tauri/src/managed_agents/storage_tests.rs index 62855b24b..91f0965fc 100644 --- a/desktop/src-tauri/src/managed_agents/storage_tests.rs +++ b/desktop/src-tauri/src/managed_agents/storage_tests.rs @@ -840,3 +840,62 @@ fn install_log_filename_accepts_ordinary_runtime_ids() { ); } } + +// ── write_and_verify: durable OS read-back, not a cache read (P3A-I1) ───────── + +/// The library mint (`mint_bound_identity`) commits an identity binding only +/// after `KeyStore::write_and_verify` confirms the nsec. That confirmation must +/// prove DURABLE retrievability from the OS keyring, not merely that the write +/// advanced the in-process cache — otherwise a backend that acknowledges a write +/// without persisting it could let a binding commit against a secret that dies +/// with the process, violating §2.5 ("no state in which a binding exists but its +/// key is unverified"). +/// +/// `write_and_verify` calls `store` first, and `store` durably persists before +/// returning, so on an honest backend cache and durable always agree the instant +/// it verifies — the cache-vs-durable divergence that the defect would expose is +/// only observable against an adverse backend (covered by the live keyring +/// probe). What this test guards deterministically is the delegated primitive +/// the fix now depends on: `verify_stored_raw` reads the OS backend and ignores a +/// stale in-process cache. Reverting it to a cache read (the original `load` +/// path) flips these assertions RED. Requires a real OS keychain. +#[ignore = "requires real OS keychain (run locally)"] +#[test] +fn write_and_verify_confirms_durable_state_and_verify_raw_ignores_stale_cache() { + use crate::secret_store::SecretStore; + + let svc = "buzz-test-write-verify-durable"; + let name = agent_keyring_name("wv-agent"); + + // Clean slate, then exercise the fixed production seam positively: the write + // is confirmed durably retrievable, so it returns Ok. + let writer = SecretStore::keyring(svc); + let _ = KeyStore::delete(&writer, &name); + KeyStore::write_and_verify(&writer, &name, "nsec1durable").expect("durable write verifies"); + + // A second "process": its own cache, warmed to a value the backend no longer + // holds. `stale` stores v_old (warming its cache to v_old), then `writer` + // overwrites the durable blob with v_new — `stale`'s cache is now behind. + let stale = SecretStore::keyring(svc); + KeyStore::write_and_verify(&stale, &name, "nsec1old").expect("stale warms its cache to old"); + KeyStore::write_and_verify(&writer, &name, "nsec1new").expect("durable advances to new"); + + // A cache read from `stale` returns the stale value it last wrote… + assert_eq!( + KeyStore::load(&stale, &name).unwrap(), + Some("nsec1old".to_string()), + "stale instance's cache still holds the old value" + ); + // …but raw verification reflects DURABLE storage, bypassing that cache: + assert!( + stale.verify_stored_raw(&name, "nsec1new").unwrap(), + "verify_stored_raw must see the durable value, not the stale cache" + ); + assert!( + !stale.verify_stored_raw(&name, "nsec1old").unwrap(), + "verify_stored_raw must reject a stale-cache value absent from the backend" + ); + + // Cleanup. + let _ = KeyStore::delete(&writer, &name); +}