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); +}