From 7bb13b0afaac75c43b5272dc98e9f0ca7b7b5cf1 Mon Sep 17 00:00:00 2001 From: j-goodz <22462858+j-goodz@users.noreply.github.com> Date: Fri, 7 Aug 2026 20:27:05 -0400 Subject: [PATCH] fix(desktop): one OS credential per secret, lifting the 2560-byte agent cap Every managed-agent private key shared a single Windows Generic credential. The keyring crate validates the blob against CRED_MAX_CREDENTIAL_BLOB_SIZE (2560 bytes) before CredWriteW, so an oversized write fails atomically rather than truncating. A real store holding an identity plus 8 agents already occupies ~2380 bytes, leaving ~90 bytes of headroom against the ~139 that each further `agent:` -> `nsec...` entry needs. Agent #9 onward therefore could never reach the OS keyring at all. The failure was silent, which is what made it hard to spot: the store fell back to writing the key inline in managed-agents.json, so the app kept working and surfaced no error, while those private keys quietly stopped being protected by the OS credential store. SecretStore now writes one credential per secret, keyed by the existing per-secret name under the same service, so no single blob can reach the cap. load_all_readonly() and the sign-out wipe need to ENUMERATE secrets, which per-credential storage cannot do portably, so this adds an index holding key NAMES only. The index is chunked at 1000 chars per credential; a single index credential would have re-created the identical cliff at ~17 agents, since an `agent:` name is ~70 chars. load() addresses credentials by name and never consults the index, so a damaged index degrades enumeration but cannot lose a key. Migration runs once on first access under the existing advisory lock and is all-or-nothing: read the blob, write each secret to its own credential, read each one back from the OS and verify it, add the names to the index, and only then delete the blob. Any failure restores every credential the attempt touched to its prior value and leaves the blob byte-for-byte intact with migrated=false, so the next access retries and reads fall back to the surviving blob. The deleted lines are the now-dead blob machinery (mutate_blob, write_blob_raw, load_blob, read_blob_raw_keyring) plus probe_legacy_key_keyring and migrate_legacy_key_keyring. Those last two became actively harmful: per-key entries written by #1264 live at exactly the address the new format uses, so the legacy migrator would have written a credential and then deleted it. Tests use an in-process fake backend that rejects writes over CRED_MAX_CREDENTIAL_BLOB_SIZE exactly as the Windows backend does, so they need no real keychain. sixteen_agents_plus_identity_all_reach_the_credential_store asserts the fixture exceeds 2560 bytes when packed as one blob, so it cannot pass vacuously; the rollback tests assert that a failed write leaves no partial credentials behind and an intact blob. A public-API guard test pins every entry point callers bind to. Verified on Windows 11: a build without this change shows a single `secrets.buzz-desktop` credential, while a build with it shows one credential per agent (`agent:.buzz-desktop-dev`). Known trade-offs: on macOS the per-item "Always Allow" grant means the first load_all_readonly() after upgrade can prompt once per credential (Windows has no prompts). Once migrated, an older Buzz build will not find the secrets. Signed-off-by: j-goodz <22462858+j-goodz@users.noreply.github.com> --- desktop/src-tauri/src/secret_store.rs | 1442 +++++++++++++++++++------ 1 file changed, 1114 insertions(+), 328 deletions(-) diff --git a/desktop/src-tauri/src/secret_store.rs b/desktop/src-tauri/src/secret_store.rs index 43854761b50..b9c52d2f6ae 100644 --- a/desktop/src-tauri/src/secret_store.rs +++ b/desktop/src-tauri/src/secret_store.rs @@ -1,9 +1,20 @@ //! OS keyring access for desktop nsec private keys. //! -//! All secrets are stored as a single JSON blob under one keychain entry -//! (service = the store's service name, username = `"secrets"`). This means -//! exactly one OS prompt per process lifetime regardless of how many keys are -//! stored — the same pattern used by Goose. +//! Each secret is stored as its **own** OS credential (service = the store's +//! service name, username = the secret's key name, e.g. `"identity"` or +//! `"agent:"`). A separate chunked *index* (`"secrets-index"`, +//! `"secrets-index-1"`, …) records which key names exist so the whole set can +//! be enumerated without a platform-specific credential-enumeration API. +//! +//! Secrets used to live in a single JSON blob under one entry (username = +//! `"secrets"`). That blob is a hard cap, not a style choice: the `keyring` +//! crate rejects any write over Windows' `CRED_MAX_CREDENTIAL_BLOB_SIZE` +//! (2560 bytes) *before* calling `CredWriteW`, so once the blob held an +//! identity plus 8 agents (~2380 bytes) every subsequent agent write failed +//! atomically and fell back to inline plaintext storage. One credential per +//! secret removes the shared budget — each agent key is ~140 bytes on its own. +//! [`SecretStore::ensure_migrated`] performs the one-time, verified, all-or- +//! nothing move out of the legacy blob on first access. //! //! The chosen backend is selected at compile time by the per-target feature in //! `Cargo.toml`. On macOS the legacy `keyring` crate (SecKeychain API) is used @@ -39,10 +50,53 @@ pub enum KeyringProbe { Unreachable, } -/// Username used for the single blob keychain entry. All secrets are stored -/// as a JSON map under this name within the service. +/// Username of the **legacy** single-blob keychain entry. No longer written; +/// read once by [`SecretStore::ensure_migrated`] and deleted after every secret +/// it held has been rewritten as its own credential and read back verified. const BLOB_KEY: &str = "secrets"; +/// Username of index chunk 0. Chunk `i > 0` lives at `"secrets-index-"`. +/// +/// The index holds key *names* only — never secret values — so a corrupt or +/// partially-written index can never lose a secret: `load()` addresses each +/// credential by name and never consults the index. +#[cfg(feature = "system-keyring")] +const INDEX_KEY: &str = "secrets-index"; + +/// Character budget for one index chunk. Windows caps a credential blob at +/// `CRED_MAX_CREDENTIAL_BLOB_SIZE` = 2560 bytes and the backend measures the +/// UTF-16 encoding, so 1000 chars = 2000 bytes leaves ample headroom. Without +/// chunking the index would simply reintroduce the blob's cliff at ~17 agents +/// (an `"agent:"` name is ~70 chars). +#[cfg(feature = "system-keyring")] +const INDEX_CHUNK_CHARS: usize = 1000; + +/// Hard ceiling on index chunks, so a corrupt chunk count can never spin the +/// reader over unbounded credential lookups. 512 chunks ≈ 6500 secrets. +#[cfg(feature = "system-keyring")] +const MAX_INDEX_CHUNKS: usize = 512; + +/// Username of index chunk `i`. +#[cfg(feature = "system-keyring")] +fn index_chunk_key(i: usize) -> String { + if i == 0 { + INDEX_KEY.to_string() + } else { + format!("{INDEX_KEY}-{i}") + } +} + +/// One index chunk as stored. `chunks` is present only on chunk 0, where it +/// declares how many chunks the reader must visit. +#[cfg(feature = "system-keyring")] +#[derive(serde::Serialize, serde::Deserialize, Default)] +struct IndexChunk { + #[serde(default, skip_serializing_if = "Option::is_none")] + chunks: Option, + #[serde(default)] + names: Vec, +} + // ── Interprocess advisory lock ───────────────────────────────────────────── // // Two concurrent Buzz processes (e.g. the signed DMG build and an unsigned dev @@ -213,12 +267,23 @@ impl Drop for BlobLockGuard { // ── End interprocess advisory lock ──────────────────────────────────────── -/// An OS keyring, addressed by service name. All secrets are stored in a -/// single JSON blob entry (one OS prompt per process lifetime). +/// An OS keyring, addressed by service name. Each secret is one credential +/// under that service, keyed by the secret's name. pub struct SecretStore { service: String, - /// In-memory cache of the deserialized blob. `None` means "not yet loaded". + /// In-memory cache of key→value pairs already read from (or written to) + /// the OS this process. **Partial**: a missing entry means "not loaded", + /// never "not stored", so a cache miss always falls through to the OS. cache: Mutex>>, + /// `true` once the legacy blob has been migrated away (or was never + /// present). Left `false` on failure so the next access retries — the + /// migration is idempotent. + migrated: Mutex, + /// Test-only: route all credential I/O to an in-process fake backend that + /// enforces the same Windows blob-size cap the real backend does. + #[cfg(test)] + #[allow(dead_code)] + fake: bool, } impl SecretStore { @@ -229,6 +294,9 @@ impl SecretStore { SecretStore { service: service.into(), cache: Mutex::new(None), + migrated: Mutex::new(false), + #[cfg(test)] + fake: false, } } @@ -298,200 +366,420 @@ fn dpk_opts(service: &str, key: &str) -> PasswordOptions { } impl SecretStore { - /// Read the blob from the keychain and return the deserialized map. - /// - /// Returns `Ok(None)` when no blob entry exists yet (first launch or - /// fresh install). Returns `Err` when the backend is unavailable or the - /// stored JSON is corrupt. + // ── Credential I/O ──────────────────────────────────────────────────── + // + // Every credential read/write/delete in this module funnels through these + // three functions, so the test fake has exactly one injection point and a + // fake-backed store can never reach the real OS keychain. + + /// Read one credential under this service. `Ok(None)` = no such entry. /// - /// On success the result is stored in `self.cache` so subsequent calls - /// within the same process return immediately without a keychain round-trip. + /// Always uses the `keyring` crate — on macOS that is the legacy + /// SecKeychain API, so signed release builds and unsigned dev builds share + /// one store. DPK is used only by the `migrate_legacy_key` read paths. #[cfg(feature = "system-keyring")] - fn load_blob(&self) -> Result>, String> { + fn entry_get(&self, key: &str) -> Result, String> { + #[cfg(test)] { - let guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); - if let Some(ref map) = *guard { - return Ok(Some(map.clone())); + if self.fake { + return fake_backend::get(&self.service, key); } } + let entry = keyring_entry(&self.service, key).map_err(|e| format!("keyring entry: {e}"))?; + match entry.get_password() { + Ok(s) => Ok(Some(s)), + Err(keyring::Error::NoEntry) => Ok(None), + Err(e) if is_keyring_availability_error(&e.to_string()) => { + Err(format!("keyring unavailable: {e}")) + } + Err(e) => Err(format!("keyring read: {e}")), + } + } - let raw = self.read_blob_raw()?; - let map = match raw { - None => return Ok(None), - Some(bytes) => { - let json = String::from_utf8(bytes).map_err(|e| format!("blob utf8: {e}"))?; - serde_json::from_str::>(&json) - .map_err(|e| format!("blob json: {e}"))? + /// Write one credential under this service. + #[cfg(feature = "system-keyring")] + fn entry_set(&self, key: &str, value: &str) -> Result<(), String> { + #[cfg(test)] + { + if self.fake { + return fake_backend::set(&self.service, key, value); } - }; + } + let entry = keyring_entry(&self.service, key).map_err(|e| format!("keyring entry: {e}"))?; + entry + .set_password(value) + .map_err(|e| format!("keyring write: {e}")) + } - // Only populate the cache if it is still empty — a concurrent - // mutate_blob() may have written a newer value while we were reading. - let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); - if guard.is_none() { - *guard = Some(map.clone()); + /// Delete one credential under this service. A missing entry is not an error. + #[cfg(feature = "system-keyring")] + fn entry_delete(&self, key: &str) -> Result<(), String> { + #[cfg(test)] + { + if self.fake { + return fake_backend::delete(&self.service, key); + } + } + let entry = keyring_entry(&self.service, key).map_err(|e| format!("keyring entry: {e}"))?; + match entry.delete_credential() { + Ok(()) | Err(keyring::Error::NoEntry) => Ok(()), + Err(e) if is_keyring_availability_error(&e.to_string()) => { + Err(format!("keyring unavailable: {e}")) + } + Err(e) => Err(format!("keyring delete: {e}")), } - Ok(Some(map)) } - /// Read the raw blob bytes from the keychain. `Ok(None)` = not found. - /// - /// Always uses the legacy keyring crate on macOS so that signed and - /// unsigned (dev) builds share the same store. DPK is only used by - /// `migrate_legacy_key` to read old per-key entries written by #1264. - #[cfg(all(feature = "system-keyring", target_os = "macos"))] + /// Read the raw **legacy** blob bytes. `Ok(None)` = not found (the normal + /// state after migration). + #[cfg(feature = "system-keyring")] fn read_blob_raw(&self) -> Result>, String> { - self.read_blob_raw_keyring() + Ok(self.entry_get(BLOB_KEY)?.map(String::into_bytes)) } - #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] - fn read_blob_raw(&self) -> Result>, String> { - self.read_blob_raw_keyring() + /// Read the credential holding the secret named `key`, bypassing the cache. + #[cfg(feature = "system-keyring")] + fn read_secret_raw(&self, key: &str) -> Result, String> { + self.entry_get(key) } - /// Read blob via the legacy `keyring` crate (Windows, Linux, or macOS dev - /// builds that lack hardened-runtime entitlements). + // ── Name index ──────────────────────────────────────────────────────── + + /// Read every indexed key name plus the chunk count currently on disk. + /// + /// A missing or corrupt chunk is skipped rather than fatal: the index is + /// only an enumeration aid, and `load()` addresses credentials by name. #[cfg(feature = "system-keyring")] - fn read_blob_raw_keyring(&self) -> Result>, String> { - let entry = - keyring_entry(&self.service, BLOB_KEY).map_err(|e| format!("keyring entry: {e}"))?; - match entry.get_password() { - Ok(s) => Ok(Some(s.into_bytes())), - Err(keyring::Error::NoEntry) => Ok(None), - Err(e) if is_keyring_availability_error(&e.to_string()) => { - Err(format!("keyring unavailable: {e}")) + fn read_index_raw(&self) -> Result<(Vec, usize), String> { + let Some(head_raw) = self.entry_get(INDEX_KEY)? else { + return Ok((Vec::new(), 0)); + }; + let head: IndexChunk = + serde_json::from_str(&head_raw).map_err(|e| format!("index json: {e}"))?; + let count = head.chunks.unwrap_or(1).clamp(1, MAX_INDEX_CHUNKS); + let mut names = head.names; + for i in 1..count { + if let Some(raw) = self.entry_get(&index_chunk_key(i))? { + if let Ok(chunk) = serde_json::from_str::(&raw) { + names.extend(chunk.names); + } } - Err(e) => Err(format!("keyring read: {e}")), } + names.sort(); + names.dedup(); + Ok((names, count)) } - /// Atomically load the blob, apply `f` to a candidate map, write back if - /// changed, and only then advance the cache. + /// Replace the index with `names`, splitting it across as many chunks as + /// the per-credential size cap requires. /// - /// **Cross-process safety**: acquires an exclusive advisory file lock - /// (`flock(2)` on Unix, `LockFileEx` on Windows) before reading, mutating, - /// and writing. The lock is keyed by service name and stored in the system - /// temp directory, making it reachable from both the signed DMG build and - /// unsigned dev builds. Inside the lock a fresh `read_blob_raw()` is always - /// performed (even when the cache is warm) so a concurrent process's write - /// is never silently dropped. - /// - /// **Idempotent**: when `f` leaves the candidate equal to the freshly-read - /// map, `write_blob_raw` is skipped entirely. On macOS the legacy - /// `SecKeychain` API treats a write as a distinct ACL operation from the - /// "Always Allow"-ed read, so skipping no-op writes eliminates the keychain - /// prompt that fires when saving an agent whose model changed but whose key - /// did not. + /// Tail chunks are written before chunk 0 so the count chunk 0 declares is + /// never larger than the data actually present. `prev_chunks` is the count + /// read alongside the names, used to drop chunks a shrinking index leaves + /// behind. + #[cfg(feature = "system-keyring")] + fn write_index(&self, names: &[String], prev_chunks: usize) -> Result<(), String> { + let mut sorted: Vec = names.to_vec(); + sorted.sort(); + sorted.dedup(); + + let mut chunks: Vec> = vec![Vec::new()]; + let mut used = 0usize; + for name in sorted { + // +4 covers the two quotes, the comma and JSON escaping slack. + let cost = name.chars().count() + 4; + if used + cost > INDEX_CHUNK_CHARS + && !chunks.last().map(Vec::is_empty).unwrap_or(true) + && chunks.len() < MAX_INDEX_CHUNKS + { + chunks.push(Vec::new()); + used = 0; + } + used += cost; + if let Some(last) = chunks.last_mut() { + last.push(name); + } + } + let n_chunks = chunks.len(); + + for i in (1..n_chunks).rev() { + let body = serde_json::to_string(&IndexChunk { + chunks: None, + names: chunks[i].clone(), + }) + .map_err(|e| format!("index serialize: {e}"))?; + self.entry_set(&index_chunk_key(i), &body)?; + } + let head = serde_json::to_string(&IndexChunk { + chunks: Some(n_chunks), + names: chunks[0].clone(), + }) + .map_err(|e| format!("index serialize: {e}"))?; + self.entry_set(INDEX_KEY, &head)?; + + for i in n_chunks..prev_chunks.min(MAX_INDEX_CHUNKS) { + let _ = self.entry_delete(&index_chunk_key(i)); + } + Ok(()) + } + + /// Add `keys` to the index if any are missing. No write when all present. + #[cfg(feature = "system-keyring")] + fn index_insert(&self, keys: &[String]) -> Result<(), String> { + let (mut names, prev) = self.read_index_raw()?; + let mut changed = false; + for key in keys { + if !names.iter().any(|n| n == key) { + names.push(key.clone()); + changed = true; + } + } + if changed { + self.write_index(&names, prev)?; + } + Ok(()) + } + + /// Drop `key` from the index. No write when it was not listed. + #[cfg(feature = "system-keyring")] + fn index_remove(&self, key: &str) -> Result<(), String> { + let (mut names, prev) = self.read_index_raw()?; + let before = names.len(); + names.retain(|n| n != key); + if names.len() != before { + self.write_index(&names, prev)?; + } + Ok(()) + } + + // ── One-time blob → per-credential migration ────────────────────────── + + /// Move every secret out of the legacy single-blob entry into its own + /// credential, exactly once, without ever producing a partial state. /// - /// **Copy-on-write**: the candidate `next` is a separate allocation from - /// `current`. The cache is only replaced with `next` after `write_blob_raw` - /// succeeds. On write failure the cache is cleared to `None` so the next - /// caller re-reads from the keychain rather than building on a stale state. + /// Sequence, under the interprocess advisory lock: + /// 1. Read the blob. Absent → nothing to do, mark migrated. + /// 2. For each secret: write its own credential, then **read it back from + /// the OS and verify** the value matches. + /// 3. Add every name to the index. + /// 4. Only then delete the blob entry. /// - /// Deadlock-free: `read_blob_raw` and `write_blob_raw` do not acquire the - /// cache mutex. `load_blob` does acquire it, but `mutate_blob` does not call - /// `load_blob` — it reads from the keyring directly inside the file lock. + /// If any single write, read-back, verify or index write fails, every + /// credential this attempt touched is restored to its prior value (deleted + /// when there was none), the blob is left **intact**, and `migrated` stays + /// `false` so the next access retries. Reads prefer the per-credential + /// value and fall back to the blob, so an aborted attempt is invisible. #[cfg(feature = "system-keyring")] - fn mutate_blob(&self, f: F) -> Result<(), String> - where - F: FnOnce(&mut HashMap), - { - // Acquire the interprocess advisory lock first. All Buzz processes - // using the same service name contend on the same lockfile at - // /tmp/buzz-keychain--.lock (a deterministic per-user - // path invariant to $TMPDIR), so only one process performs a - // read-modify-write at a time. - let _lock = acquire_blob_lock(&self.service)?; + fn ensure_migrated(&self) -> Result<(), String> { + { + let guard = self.migrated.lock().unwrap_or_else(|e| e.into_inner()); + if *guard { + return Ok(()); + } + } - // Always do a fresh read from the keychain while holding the lock — - // this is the critical correction over the prior warm-cache path. A - // stale warm cache would make us build our candidate on an outdated - // baseline and drop keys written by another process. - let raw = self.read_blob_raw()?; - let current: HashMap = match raw { - None => HashMap::new(), - Some(bytes) => { - let json = String::from_utf8(bytes).map_err(|e| format!("blob utf8: {e}"))?; - serde_json::from_str::>(&json) - .map_err(|e| format!("blob json: {e}"))? + // Cross-process exclusion: another Buzz process may be migrating the + // same service right now. Note this must not be held by our caller — + // `flock(2)` on a second fd in the same process would self-deadlock. + let _lock = acquire_blob_lock(&self.service)?; + { + let guard = self.migrated.lock().unwrap_or_else(|e| e.into_inner()); + if *guard { + return Ok(()); } + } + + let Some(bytes) = self.read_blob_raw()? else { + // No legacy blob: fresh install, or another process already + // finished the migration. + *self.migrated.lock().unwrap_or_else(|e| e.into_inner()) = true; + return Ok(()); }; + let json = String::from_utf8(bytes).map_err(|e| format!("blob utf8: {e}"))?; + let map = serde_json::from_str::>(&json) + .map_err(|e| format!("blob json: {e}"))?; + + // (name, value before this attempt touched it) — the rollback journal. + let mut touched: Vec<(String, Option)> = Vec::new(); + for (key, value) in &map { + let prior = match self.read_secret_raw(key) { + Ok(p) => p, + Err(e) => { + self.rollback_migration(&touched); + return Err(format!("blob migration: read {key}: {e}")); + } + }; + if prior.as_deref() == Some(value.as_str()) { + continue; // already migrated by an earlier attempt + } + if let Err(e) = self.entry_set(key, value) { + self.rollback_migration(&touched); + return Err(format!("blob migration: write {key}: {e}")); + } + touched.push((key.clone(), prior)); + // Read back from the OS — proves the round-trip, not just that an + // in-process buffer was updated. + match self.read_secret_raw(key) { + Ok(Some(got)) if got == *value => {} + other => { + self.rollback_migration(&touched); + return Err(format!( + "blob migration: read-back verify failed for {key}: {other:?}" + )); + } + } + } - // Build the candidate state in a separate allocation so that a write - // failure below cannot leave the cache ahead of durable storage. - let mut next = current.clone(); - f(&mut next); + let names: Vec = map.keys().cloned().collect(); + if let Err(e) = self.index_insert(&names) { + self.rollback_migration(&touched); + return Err(format!("blob migration: index: {e}")); + } + + // Every secret is durable and verified in its own credential; the blob + // is now redundant. A failure here is not data loss — the blob is + // simply retried on the next process start. + if let Err(e) = self.entry_delete(BLOB_KEY) { + eprintln!("buzz-desktop: secret_store: blob migrated but not deleted: {e}"); + } - // Skip the keychain write when the candidate equals the freshly-read - // durable state — no I/O needed and no keychain ACL prompt on macOS. - if next == current { - // Update the cache to the fresh read even on no-op so subsequent - // reads in this process see any keys another process may have added. + { let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); - *guard = Some(current); - return Ok(()); + let cached = guard.get_or_insert_with(HashMap::new); + for (key, value) in map { + cached.insert(key, value); + } } + *self.migrated.lock().unwrap_or_else(|e| e.into_inner()) = true; + Ok(()) + } - // Write to keyring while still holding the file lock. - let json = serde_json::to_string(&next).map_err(|e| format!("blob serialize: {e}"))?; - match self.write_blob_raw(json.as_bytes()) { - Ok(()) => { - // Advance the cache to `next` only after the durable write succeeds. - let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); - *guard = Some(next); - Ok(()) + /// Undo the credential writes journalled in `touched`, newest first. + /// Best-effort by construction: the blob is still intact, so anything that + /// cannot be restored here is re-derived on the next migration attempt. + #[cfg(feature = "system-keyring")] + fn rollback_migration(&self, touched: &[(String, Option)]) { + for (key, prior) in touched.iter().rev() { + let _ = match prior { + Some(old) => self.entry_set(key, old), + None => self.entry_delete(key), + }; + } + } + + /// Run [`Self::ensure_migrated`] but let a failure fall through to the + /// caller's own OS access, which produces the more precise error. + #[cfg(feature = "system-keyring")] + fn try_migrate(&self) { + if let Err(e) = self.ensure_migrated() { + eprintln!("buzz-desktop: secret_store: blob migration deferred: {e}"); + } + } + + /// Write `entries` as one credential each, then record their names in the + /// index, then advance the cache. Replaces the old `mutate_blob`. + /// + /// **Cross-process safety**: acquires the same exclusive advisory lock + /// (`flock(2)` on Unix, a named kernel mutex on Windows) the blob path used, + /// keyed by service name, so a concurrent process cannot interleave an + /// index read-modify-write with ours. The index is always re-read fresh + /// inside the lock, never from cache, so another process's names are never + /// dropped. Per-secret values no longer share a record at all, so a write + /// here cannot clobber a secret it does not name. + /// + /// **Idempotent**: a credential whose stored value already equals the new + /// one is not rewritten. On macOS the legacy `SecKeychain` API treats a + /// write as a distinct ACL operation from the "Always Allow"-ed read, so + /// skipping no-op writes still avoids the prompt that would otherwise fire + /// when saving an agent whose model changed but whose key did not. + /// + /// **Cache honesty**: only keys that reached the OS are added to the cache, + /// so a failed write is never visible to a later `load()`. + /// + /// Deadlock-free: `ensure_migrated` takes the same lock, so it must run to + /// completion *before* the lock is acquired here, not inside it. + #[cfg(feature = "system-keyring")] + fn store_secrets(&self, entries: &HashMap) -> Result<(), String> { + self.ensure_migrated()?; + let _lock = acquire_blob_lock(&self.service)?; + + let mut written: Vec = Vec::new(); + let mut first_err: Option = None; + for (key, value) in entries { + match self.read_secret_raw(key) { + // Already durable with this exact value — no write, no prompt. + Ok(Some(cur)) if cur == *value => written.push(key.clone()), + Ok(_) => match self.entry_set(key, value) { + Ok(()) => written.push(key.clone()), + Err(e) => { + first_err.get_or_insert(e); + } + }, + Err(e) => { + first_err.get_or_insert(e); + } } - Err(e) => { - // On write failure, clear the cache so the next caller re-reads - // from the keychain rather than building on a stale state. - let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); - *guard = None; - Err(e) + } + + if !written.is_empty() { + if let Err(e) = self.index_insert(&written) { + first_err.get_or_insert(e); } } - } - /// Always uses the legacy keyring crate on macOS — see `read_blob_raw`. - #[cfg(all(feature = "system-keyring", target_os = "macos"))] - fn write_blob_raw(&self, bytes: &[u8]) -> Result<(), String> { - self.write_blob_raw_keyring(bytes) + { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + let map = guard.get_or_insert_with(HashMap::new); + for key in &written { + if let Some(value) = entries.get(key) { + map.insert(key.clone(), value.clone()); + } + } + } + + match first_err { + Some(e) => Err(e), + None => Ok(()), + } } - #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] - fn write_blob_raw(&self, bytes: &[u8]) -> Result<(), String> { - self.write_blob_raw_keyring(bytes) + /// Value for `key` if this process has already read or written it. Never + /// authoritative for absence — a miss must fall through to the OS. + #[cfg(feature = "system-keyring")] + fn cached(&self, key: &str) -> Option { + let guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + guard.as_ref().and_then(|m| m.get(key).cloned()) } + /// Record `key`→`value` as read from the OS. #[cfg(feature = "system-keyring")] - fn write_blob_raw_keyring(&self, bytes: &[u8]) -> Result<(), String> { - let value = std::str::from_utf8(bytes).map_err(|e| format!("blob utf8 encode: {e}"))?; - let entry = - keyring_entry(&self.service, BLOB_KEY).map_err(|e| format!("keyring entry: {e}"))?; - entry - .set_password(value) - .map_err(|e| format!("keyring write: {e}")) + fn cache_put(&self, key: &str, value: &str) { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + guard + .get_or_insert_with(HashMap::new) + .insert(key.to_string(), value.to_string()); } /// Probe whether `key` exists and whether the backend is reachable. pub fn probe(&self, key: &str) -> KeyringProbe { #[cfg(feature = "system-keyring")] { - match self.load_blob() { - Ok(Some(map)) => { - if map.contains_key(key) { - KeyringProbe::Present - } else { - // Blob exists but key absent — still check old per-key - // entries so a partial migration (e.g. identity migrated - // first) doesn't silently drop agent keys. - self.probe_legacy_key(key) - } - } - // No blob yet — check old per-key entries so callers that - // gate `load()` on `Present` still trigger migration. - Ok(None) => self.probe_legacy_key(key), + self.try_migrate(); + if self.cached(key).is_some() { + return KeyringProbe::Present; + } + match self.read_secret_raw(key) { + Ok(Some(_)) => KeyringProbe::Present, + // No credential of its own — check the shapes an unfinished + // migration can leave behind (legacy blob, then old DPK items) + // so callers that gate `load()` on `Present` still fire. + Ok(None) => match self.blob_value(key) { + Ok(Some(_)) => KeyringProbe::Present, + Ok(None) => self.probe_legacy_key(key), + Err(_) => KeyringProbe::Unreachable, // corrupt blob — fail closed + }, Err(e) if is_keyring_availability_error(&e) => KeyringProbe::Unreachable, - Err(_) => KeyringProbe::Unreachable, // corrupt blob — fail closed + Err(_) => KeyringProbe::Unreachable, } } #[cfg(not(feature = "system-keyring"))] @@ -501,14 +789,28 @@ impl SecretStore { } } - /// Check old per-key DPK/keyring entries for `key`. Used by `probe()` when - /// the blob doesn't exist yet (first launch after upgrade). + /// Value for `key` still sitting in the legacy blob, if the blob survives + /// (i.e. a migration attempt has not yet succeeded). This is what keeps an + /// aborted migration invisible to readers. + #[cfg(feature = "system-keyring")] + fn blob_value(&self, key: &str) -> Result, String> { + let Some(bytes) = self.read_blob_raw()? else { + return Ok(None); + }; + let json = String::from_utf8(bytes).map_err(|e| format!("blob utf8: {e}"))?; + let map = serde_json::from_str::>(&json) + .map_err(|e| format!("blob json: {e}"))?; + Ok(map.get(key).cloned()) + } + + /// Check old per-key DPK entries for `key`. Used by `probe()` once the + /// key's own credential and the legacy blob have both come up empty. #[cfg(all(feature = "system-keyring", target_os = "macos"))] fn probe_legacy_key(&self, key: &str) -> KeyringProbe { match generic_password(dpk_opts(&self.service, key)) { Ok(_) => KeyringProbe::Present, - Err(ref e) if is_not_found(e) => self.probe_legacy_key_keyring(key), - Err(ref e) if is_dpk_unavailable(e) => self.probe_legacy_key_keyring(key), + Err(ref e) if is_not_found(e) => KeyringProbe::ReachableButEmpty, + Err(ref e) if is_dpk_unavailable(e) => KeyringProbe::ReachableButEmpty, Err(ref e) if is_keyring_availability_error(&e.to_string()) => { KeyringProbe::Unreachable } @@ -517,56 +819,36 @@ impl SecretStore { } #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] - fn probe_legacy_key(&self, key: &str) -> KeyringProbe { - self.probe_legacy_key_keyring(key) - } - - #[cfg(feature = "system-keyring")] - fn probe_legacy_key_keyring(&self, key: &str) -> KeyringProbe { - match keyring_entry(&self.service, key) { - Ok(entry) => match entry.get_password() { - Ok(_) => KeyringProbe::Present, - Err(keyring::Error::NoEntry) => KeyringProbe::ReachableButEmpty, - Err(e) if is_keyring_availability_error(&e.to_string()) => { - KeyringProbe::Unreachable - } - Err(_) => KeyringProbe::ReachableButEmpty, - }, - Err(e) if is_keyring_availability_error(&e.to_string()) => KeyringProbe::Unreachable, - Err(_) => KeyringProbe::Unreachable, - } + fn probe_legacy_key(&self, _key: &str) -> KeyringProbe { + // No DPK off macOS: the key's own credential is the only shape, and + // `probe` already found it absent. + KeyringProbe::ReachableButEmpty } /// Load the secret for `key`. `Ok(None)` when there is no entry; `Err` only /// when the backend errored in a way that is not "missing". /// - /// On first launch after an upgrade from the per-key DPK format, the blob - /// will not exist yet. In that case the macOS path falls back to reading the - /// old per-key DPK entry for `key` specifically, writes it into a new blob, - /// and deletes the old item — a one-time migration per key. The same - /// migration fires when the blob exists but the key is absent, covering - /// partial-migration scenarios (e.g. identity migrated first, agents not yet). + /// Resolution order: in-process cache → the key's own credential → the + /// legacy blob (only reachable while a migration attempt is outstanding) → + /// the old per-key DPK item on macOS, which is migrated in place and + /// deleted. Pre-blob installs are handled for free: their per-key `keyring` + /// entries live at exactly the address this format uses. pub fn load(&self, key: &str) -> Result, String> { #[cfg(feature = "system-keyring")] { - match self.load_blob() { - Ok(Some(map)) => { - if let Some(value) = map.get(key) { - Ok(Some(value.clone())) - } else { - // Blob exists but key absent — attempt migration from old - // per-key entry. migrate_legacy_key writes the result into - // the blob if found, so subsequent loads hit the cache. - self.migrate_legacy_key(key) - } - } - Ok(None) => { - // No blob yet — attempt one-time migration from old per-key - // DPK entry (macOS) or return Ok(None) (other platforms). - self.migrate_legacy_key(key) - } - Err(e) => Err(e), + self.try_migrate(); + if let Some(value) = self.cached(key) { + return Ok(Some(value)); } + if let Some(value) = self.read_secret_raw(key)? { + self.cache_put(key, &value); + return Ok(Some(value)); + } + // A migration that aborted leaves the blob authoritative. + if let Some(value) = self.blob_value(key)? { + return Ok(Some(value)); + } + self.migrate_legacy_key(key) } #[cfg(not(feature = "system-keyring"))] { @@ -575,17 +857,68 @@ impl SecretStore { } } - /// Read the secret for `key` without any legacy-migration side effects. + /// Read every stored secret without any legacy-migration side effects. /// - /// Read the entire blob without any legacy-migration side effects. + /// Returns the full key→value map, `Ok(None)` when nothing has ever been + /// stored for this service, and `Err` only when the backend is unavailable. + /// Never calls `migrate_legacy_key`. /// - /// Returns the full key→value map when a blob exists, `Ok(None)` when no - /// blob has been written yet, and `Err` only when the backend is - /// unavailable. Never calls `migrate_legacy_key`. + /// The key set comes from the index, unioned with any names still in an + /// unmigrated blob and with `"identity"` (which predates the index and can + /// exist as a bare credential on a pre-blob install). pub fn load_all_readonly(&self) -> Result>, String> { #[cfg(feature = "system-keyring")] { - self.load_blob() + self.try_migrate(); + + let (mut names, _) = self.read_index_raw()?; + let indexed = !names.is_empty(); + + // A blob only survives here when a migration attempt aborted; its + // contents are still the authoritative copy for those keys. + let blob: HashMap = match self.read_blob_raw()? { + Some(bytes) => { + let json = String::from_utf8(bytes).map_err(|e| format!("blob utf8: {e}"))?; + serde_json::from_str(&json).map_err(|e| format!("blob json: {e}"))? + } + None => HashMap::new(), + }; + for name in blob.keys() { + if !names.iter().any(|n| n == name) { + names.push(name.clone()); + } + } + if !names.iter().any(|n| n == "identity") { + names.push("identity".to_string()); + } + + let mut map = HashMap::new(); + for name in names { + match self.read_secret_raw(&name)? { + Some(value) => { + map.insert(name, value); + } + // Indexed but no credential: an index entry outliving its + // secret is a benign inconsistency, not a read failure. + None => { + if let Some(value) = blob.get(&name) { + map.insert(name, value.clone()); + } + } + } + } + + if map.is_empty() && !indexed { + return Ok(None); // nothing has ever been stored + } + { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + let cached = guard.get_or_insert_with(HashMap::new); + for (k, v) in &map { + cached.insert(k.clone(), v.clone()); + } + } + Ok(Some(map)) } #[cfg(not(feature = "system-keyring"))] { @@ -593,19 +926,15 @@ impl SecretStore { } } - /// Insert all entries from `entries` into the blob in a single mutation. + /// Store every entry in `entries`, one credential each. /// - /// Entries that already exist in the blob are overwritten; entries not - /// present in `entries` are left unchanged. If the resulting blob is - /// identical to what is already stored, no keychain write occurs. + /// Entries that already exist are overwritten; keys not present in + /// `entries` are left untouched. A credential whose stored value already + /// equals the new one is not rewritten, so a no-op save costs no write. pub fn store_all(&self, entries: &HashMap) -> Result<(), String> { #[cfg(feature = "system-keyring")] { - self.mutate_blob(|map| { - for (k, v) in entries { - map.insert(k.clone(), v.clone()); - } - }) + self.store_secrets(entries) } #[cfg(not(feature = "system-keyring"))] { @@ -615,13 +944,18 @@ impl SecretStore { } /// On first launch after upgrading from the per-key DPK format, read the - /// old DPK entry for `key`, write it into a new blob, and delete the old - /// item. Returns `Ok(None)` when no old entry exists. + /// old DPK entry for `key`, write it into the key's own credential, and + /// delete the old item. Returns `Ok(None)` when no old entry exists. /// /// Also handles a one-time migration from the DPK blob format written by /// #1267 (before the dev/release split was fixed). Anyone who ran main /// while #1267 was present has a DPK blob instead of per-key entries; this - /// reads it, merges all keys into the legacy blob, and deletes the DPK blob. + /// reads it, lifts every key it holds into its own credential, and deletes + /// the DPK blob. + /// + /// The pre-#1264 `keyring`-crate per-key entries need no handling: they sit + /// at exactly the address the current format uses, so `load` already found + /// them before reaching here. #[cfg(all(feature = "system-keyring", target_os = "macos"))] fn migrate_legacy_key(&self, key: &str) -> Result, String> { // One-time migration: check for a DPK blob (key = BLOB_KEY = "secrets") @@ -631,12 +965,17 @@ impl SecretStore { let json = String::from_utf8(bytes).map_err(|e| format!("dpk blob utf8: {e}"))?; let dpk_map = serde_json::from_str::>(&json) .map_err(|e| format!("dpk blob json: {e}"))?; - // Merge all keys from the DPK blob into the legacy blob. - self.mutate_blob(|map| { - for (k, v) in &dpk_map { - map.entry(k.clone()).or_insert_with(|| v.clone()); + // Lift each key into its own credential, never overwriting one + // that already exists (the existing copy is the newer one). + let mut fresh = HashMap::new(); + for (k, v) in &dpk_map { + if self.read_secret_raw(k)?.is_none() { + fresh.insert(k.clone(), v.clone()); } - })?; + } + if !fresh.is_empty() { + self.store_secrets(&fresh)?; + } // Best-effort delete the DPK blob. let _ = delete_generic_password_options(dpk_opts(&self.service, BLOB_KEY)); return Ok(dpk_map.get(key).cloned()); @@ -654,44 +993,23 @@ impl SecretStore { match generic_password(dpk_opts(&self.service, key)) { Ok(bytes) => { let value = String::from_utf8(bytes).map_err(|e| format!("keyring utf8: {e}"))?; - // Write into blob (creates the blob if it doesn't exist). + // Write into the key's own credential. self.store(key, &value)?; // Best-effort cleanup of the old per-key entry. let _ = delete_generic_password_options(dpk_opts(&self.service, key)); Ok(Some(value)) } - Err(ref e) if is_not_found(e) => { - // Also check the old keyring-crate entry (pre-#1264 installs). - self.migrate_legacy_key_keyring(key) - } - Err(ref e) if is_dpk_unavailable(e) => { - // Unsigned dev build — check old keyring-crate entry only. - self.migrate_legacy_key_keyring(key) - } + Err(ref e) if is_not_found(e) => Ok(None), + Err(ref e) if is_dpk_unavailable(e) => Ok(None), Err(e) => Err(format!("keyring get: {e}")), } } #[cfg(all(feature = "system-keyring", not(target_os = "macos")))] - fn migrate_legacy_key(&self, key: &str) -> Result, String> { - // Non-macOS: no DPK, just check the old keyring-crate per-key entry. - self.migrate_legacy_key_keyring(key) - } - - /// Check the old per-key `keyring` crate entry (pre-#1264 format) and - /// migrate it into the blob if found. - #[cfg(feature = "system-keyring")] - fn migrate_legacy_key_keyring(&self, key: &str) -> Result, String> { - let entry = keyring_entry(&self.service, key).map_err(|e| format!("keyring entry: {e}"))?; - match entry.get_password() { - Ok(value) => { - self.store(key, &value)?; - let _ = entry.delete_credential(); - Ok(Some(value)) - } - Err(keyring::Error::NoEntry) => Ok(None), - Err(e) => Err(format!("keyring get: {e}")), - } + fn migrate_legacy_key(&self, _key: &str) -> Result, String> { + // No DPK off macOS, and the pre-#1264 per-key `keyring` entries share + // the current format's address — `load` has already checked there. + Ok(None) } /// Verify that `key` holds `expected` by reading directly from the OS @@ -705,17 +1023,11 @@ impl SecretStore { pub fn verify_stored_raw(&self, key: &str, expected: &str) -> Result { #[cfg(feature = "system-keyring")] { - let raw = self.read_blob_raw()?; - match raw { - None => Ok(false), - Some(bytes) => { - let json = String::from_utf8(bytes).map_err(|e| format!("blob utf8: {e}"))?; - let map = - serde_json::from_str::>(&json) - .map_err(|e| format!("blob json: {e}"))?; - Ok(map.get(key).is_some_and(|v| v == expected)) - } + if self.read_secret_raw(key)?.as_deref() == Some(expected) { + return Ok(true); } + // A key still awaiting migration is durable in the blob. + Ok(self.blob_value(key)?.as_deref() == Some(expected)) } #[cfg(not(feature = "system-keyring"))] { @@ -729,9 +1041,9 @@ impl SecretStore { pub fn store(&self, key: &str, value: &str) -> Result<(), String> { #[cfg(feature = "system-keyring")] { - self.mutate_blob(|map| { - map.insert(key.to_string(), value.to_string()); - }) + let mut one = HashMap::with_capacity(1); + one.insert(key.to_string(), value.to_string()); + self.store_secrets(&one) } #[cfg(not(feature = "system-keyring"))] { @@ -740,43 +1052,44 @@ impl SecretStore { } } - /// Delete the entire keychain blob for this service, plus all legacy per-key - /// entries that could resurrect an identity on next boot. + /// Delete every secret credential for this service, plus every legacy shape + /// that could resurrect an identity on next boot. /// /// Order of operations: - /// 1. Read the blob to collect every key name (e.g. `identity`, agent keys). + /// 1. Collect every key name: the index, plus any names left in an + /// unmigrated blob, plus `"identity"` unconditionally. /// 2. Delete legacy per-key DPK entries for every key + the DPK blob itself. - /// 3. Delete legacy per-key keyring entries for every key. - /// 4. Delete the blob entry. + /// 3. Delete each key's own credential. + /// 4. Delete the legacy blob entry and every index chunk. /// 5. Clear the in-memory cache. /// /// This is the correct wipe path for sign-out: the old `delete_all` skipped - /// step 1–3 so stale per-key entries could be re-imported on the next launch - /// via `migrate_legacy_key`. This method prevents that resurrection. + /// steps 1–3 so stale per-key entries could be re-imported on the next + /// launch via `migrate_legacy_key`. This method prevents that resurrection. pub fn delete_all_with_legacy_cleanup(&self) -> Result<(), String> { #[cfg(feature = "system-keyring")] { let _lock = acquire_blob_lock(&self.service)?; - // Step 1: read current blob keys (best-effort; no entry = empty set). - let blob_keys: Vec = match self.read_blob_raw() { - Ok(Some(bytes)) => { - let json = String::from_utf8(bytes).unwrap_or_default(); - serde_json::from_str::>(&json) - .map(|m| m.into_keys().collect()) - .unwrap_or_default() + // Step 1: every name we know about (best-effort; errors = empty set). + let (mut all_keys, index_chunks) = self.read_index_raw().unwrap_or_default(); + if let Ok(Some(bytes)) = self.read_blob_raw() { + let json = String::from_utf8(bytes).unwrap_or_default(); + if let Ok(map) = serde_json::from_str::>(&json) { + for key in map.into_keys() { + if !all_keys.contains(&key) { + all_keys.push(key); + } + } } - _ => vec![], - }; - - // Always include "identity" even if the blob is empty or absent — - // it may exist only as a legacy per-key entry. - let mut all_keys = blob_keys; + } + // Always include "identity" even when nothing is indexed — it may + // exist only as a bare credential from a pre-blob install. if !all_keys.contains(&"identity".to_string()) { all_keys.push("identity".to_string()); } - // Steps 2 & 3: delete legacy per-key entries for every key. + // Steps 2 & 3: delete the DPK entry and the credential for every key. for key in &all_keys { #[cfg(target_os = "macos")] { @@ -787,19 +1100,8 @@ impl SecretStore { Err(e) => return Err(format!("dpk per-key delete {key}: {e}")), } } - { - let entry = keyring_entry(&self.service, key) - .map_err(|e| format!("keyring entry constructor {key}: {e}"))?; - match entry.delete_credential() { - Ok(()) | Err(keyring::Error::NoEntry) => {} - Err(e) if is_keyring_availability_error(&e.to_string()) => { - return Err(format!("keyring unavailable deleting {key}: {e}")); - } - Err(e) => { - return Err(format!("keyring per-key delete {key}: {e}")); - } - } - } + self.entry_delete(key) + .map_err(|e| format!("keyring per-key delete {key}: {e}"))?; } // Step 2 (cont.): also delete the legacy DPK blob written by #1267. #[cfg(target_os = "macos")] @@ -812,22 +1114,17 @@ impl SecretStore { } } - // Step 4: delete the main blob entry. - { - let entry = keyring_entry(&self.service, BLOB_KEY) - .map_err(|e| format!("keyring entry constructor blob: {e}"))?; - match entry.delete_credential() { - Ok(()) | Err(keyring::Error::NoEntry) => {} - Err(e) if is_keyring_availability_error(&e.to_string()) => { - return Err(format!("keyring unavailable: {e}")); - } - Err(e) => { - return Err(format!("keyring blob delete: {e}")); - } - } + // Step 4: delete the legacy blob entry and every index chunk. + self.entry_delete(BLOB_KEY) + .map_err(|e| format!("keyring blob delete: {e}"))?; + for i in 0..index_chunks.clamp(1, MAX_INDEX_CHUNKS) { + self.entry_delete(&index_chunk_key(i)) + .map_err(|e| format!("keyring index delete {i}: {e}"))?; } - // Step 5: clear the in-memory cache. + // Step 5: clear the in-memory cache. The blob is gone, so there is + // nothing left to migrate either. + *self.migrated.lock().unwrap_or_else(|e| e.into_inner()) = true; let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); *guard = None; Ok(()) @@ -854,16 +1151,11 @@ impl SecretStore { Ok(Some(_)) => return false, Err(_) => return false, } - // 2. Per-key "identity" via legacy keyring must be absent. - match keyring_entry(&self.service, "identity") { - Ok(entry) => match entry.get_password() { - Err(keyring::Error::NoEntry) => {} - Ok(_) => return false, - // Any other error (availability, unknown, transient) → fail closed. - // Only explicit NoEntry is proof of absence. - Err(_) => return false, - }, - // Constructor failure → cannot verify → fail closed. + // 2. The "identity" credential itself must be absent. Only an + // explicit "no entry" is proof of absence; any error fails closed. + match self.entry_get("identity") { + Ok(None) => {} + Ok(Some(_)) => return false, Err(_) => return false, } // 3. DPK blob (macOS only). @@ -901,16 +1193,23 @@ impl SecretStore { pub fn delete(&self, key: &str) -> Result<(), String> { #[cfg(feature = "system-keyring")] { - self.mutate_blob(|map| { - map.remove(key); - })?; - // Best-effort: also delete any old per-key entry for this key to + // Migrate first: a key still sitting in the blob would otherwise + // survive the delete and be resurrected on the next load. + self.try_migrate(); + let _lock = acquire_blob_lock(&self.service)?; + + self.entry_delete(key)?; + self.index_remove(key)?; + { + let mut guard = self.cache.lock().unwrap_or_else(|e| e.into_inner()); + if let Some(map) = guard.as_mut() { + map.remove(key); + } + } + // Best-effort: also delete any old DPK entry for this key to // prevent resurrection on the next probe/load (migration path). #[cfg(target_os = "macos")] let _ = delete_generic_password_options(dpk_opts(&self.service, key)); - if let Ok(entry) = keyring_entry(&self.service, key) { - let _ = entry.delete_credential(); - } Ok(()) } #[cfg(not(feature = "system-keyring"))] @@ -921,6 +1220,131 @@ impl SecretStore { } } +/// In-process stand-in for the OS credential store, used by the unit tests so +/// they never touch (or depend on) a real keychain. +/// +/// It reproduces the constraint that motivated per-credential storage: a write +/// whose value exceeds Windows' `CRED_MAX_CREDENTIAL_BLOB_SIZE` is rejected +/// before anything is stored, exactly as `keyring`'s Windows backend rejects it +/// ahead of `CredWriteW`. Sizes are measured over the UTF-16 encoding the +/// backend actually writes. +#[cfg(all(test, feature = "system-keyring"))] +mod fake_backend { + use std::collections::{HashMap, HashSet}; + use std::sync::{Mutex, OnceLock}; + + /// `CRED_MAX_CREDENTIAL_BLOB_SIZE` from `wincred.h`. + pub const MAX_BLOB_BYTES: usize = 2560; + + type Creds = HashMap<(String, String), String>; + + fn creds() -> &'static Mutex { + static CREDS: OnceLock> = OnceLock::new(); + CREDS.get_or_init(|| Mutex::new(HashMap::new())) + } + + fn failures() -> &'static Mutex> { + static FAILURES: OnceLock>> = OnceLock::new(); + FAILURES.get_or_init(|| Mutex::new(HashSet::new())) + } + + /// UTF-16 byte length, i.e. what the Windows backend measures. + pub fn blob_bytes(value: &str) -> usize { + value.encode_utf16().count() * 2 + } + + pub fn get(service: &str, key: &str) -> Result, String> { + let guard = creds().lock().unwrap_or_else(|e| e.into_inner()); + Ok(guard.get(&(service.to_string(), key.to_string())).cloned()) + } + + pub fn set(service: &str, key: &str, value: &str) -> Result<(), String> { + let id = (service.to_string(), key.to_string()); + if failures() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .contains(&id) + { + return Err("keyring write: injected backend failure".to_string()); + } + if blob_bytes(value) > MAX_BLOB_BYTES { + return Err(format!( + "keyring write: credential blob too long ({} bytes > {MAX_BLOB_BYTES})", + blob_bytes(value) + )); + } + creds() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert(id, value.to_string()); + Ok(()) + } + + pub fn delete(service: &str, key: &str) -> Result<(), String> { + creds() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .remove(&(service.to_string(), key.to_string())); + Ok(()) + } + + // ── helpers for tests ───────────────────────────────────────────────── + + /// Write a credential directly, bypassing the failure injection. + pub fn seed(service: &str, key: &str, value: &str) { + creds() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert((service.to_string(), key.to_string()), value.to_string()); + } + + /// Read a credential directly, bypassing `SecretStore` entirely. + pub fn raw(service: &str, key: &str) -> Option { + creds() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .get(&(service.to_string(), key.to_string())) + .cloned() + } + + /// Every credential name that exists under `service`, sorted. + pub fn names_for(service: &str) -> Vec { + let guard = creds().lock().unwrap_or_else(|e| e.into_inner()); + let mut names: Vec = guard + .keys() + .filter(|(s, _)| s == service) + .map(|(_, k)| k.clone()) + .collect(); + names.sort(); + names + } + + /// Make every write to `service`/`key` fail, simulating a denied prompt or + /// a transient backend outage part-way through a migration. + pub fn fail_writes_for(service: &str, key: &str) { + failures() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .insert((service.to_string(), key.to_string())); + } + + pub fn clear_failures() { + failures().lock().unwrap_or_else(|e| e.into_inner()).clear(); + } + + /// Drop all state for `service`. + pub fn reset(service: &str) { + creds() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .retain(|(s, _), _| s != service); + failures() + .lock() + .unwrap_or_else(|e| e.into_inner()) + .retain(|(s, _)| s != service); + } +} + #[cfg(all(test, feature = "system-keyring"))] mod tests { use super::*; @@ -931,10 +1355,35 @@ mod tests { SecretStore { service: service.to_string(), cache: Mutex::new(cache), + // A pre-seeded cache stands in for "already read from the OS", + // so there is nothing left to migrate. + migrated: Mutex::new(true), + fake: true, + } + } + + /// Store backed by the in-process fake credential backend. + fn with_fake(service: &str) -> Self { + SecretStore { + service: service.to_string(), + cache: Mutex::new(None), + migrated: Mutex::new(false), + fake: true, } } } + /// Serialize `entries` the way the legacy blob stored them. + fn blob_json(entries: &HashMap) -> String { + serde_json::to_string(entries).unwrap() + } + + /// A realistically-sized agent key name/value pair (`agent:` → + /// `nsec…`), the shape that made the blob overflow. + fn agent_pair(i: usize) -> (String, String) { + (format!("agent:npub1{:0>58}", i), format!("nsec1{:0>58}", i)) + } + #[test] fn probe_returns_present_when_key_in_cache() { let mut map = HashMap::new(); @@ -1303,4 +1752,341 @@ mod tests { // Agent key should also be gone. assert_eq!(store3.load("agent:abc123").unwrap(), None); } + + // ── Blob → per-credential migration (fake backend, no OS keychain) ───── + + #[test] + fn migration_moves_every_blob_secret_into_its_own_credential() { + let svc = "buzz-test-fake-migrate-basic"; + fake_backend::reset(svc); + + let mut blob = HashMap::new(); + blob.insert("identity".to_string(), "nsec1identityvalue".to_string()); + blob.insert("agent:aaa".to_string(), "nsec1aaa".to_string()); + blob.insert("agent:bbb".to_string(), "nsec1bbb".to_string()); + fake_backend::seed(svc, BLOB_KEY, &blob_json(&blob)); + + // First access triggers the migration. + let store = SecretStore::with_fake(svc); + assert_eq!( + store.load("identity").unwrap(), + Some("nsec1identityvalue".to_string()) + ); + + for (key, value) in &blob { + assert_eq!( + fake_backend::raw(svc, key).as_deref(), + Some(value.as_str()), + "{key} must now live in its own credential" + ); + } + assert_eq!( + fake_backend::raw(svc, BLOB_KEY), + None, + "the legacy blob must be deleted once every secret is written and verified" + ); + assert!( + fake_backend::raw(svc, INDEX_KEY).is_some(), + "the index must list the migrated names" + ); + + // A cold store sees the full set, and migrating again is a no-op. + let reader = SecretStore::with_fake(svc); + let all = reader.load_all_readonly().unwrap().unwrap(); + assert_eq!(all, blob, "every secret must survive the migration"); + reader.ensure_migrated().unwrap(); + assert_eq!(reader.load("agent:bbb").unwrap(), Some("nsec1bbb".into())); + + fake_backend::reset(svc); + } + + #[test] + fn migration_rolls_back_and_leaves_blob_intact_when_one_write_fails() { + let svc = "buzz-test-fake-migrate-rollback"; + fake_backend::reset(svc); + + let mut blob = HashMap::new(); + blob.insert("identity".to_string(), "nsec1identityvalue".to_string()); + for i in 0..6 { + let (key, value) = agent_pair(i); + blob.insert(key, value); + } + let original = blob_json(&blob); + fake_backend::seed(svc, BLOB_KEY, &original); + + // One secret's write fails part-way through (denied prompt / outage). + let (doomed, _) = agent_pair(3); + fake_backend::fail_writes_for(svc, &doomed); + + let store = SecretStore::with_fake(svc); + let err = store.ensure_migrated().unwrap_err(); + assert!( + err.contains(&doomed), + "the error must name the secret that failed: {err}" + ); + + assert_eq!( + fake_backend::raw(svc, BLOB_KEY).as_deref(), + Some(original.as_str()), + "the blob must be left byte-for-byte intact so the migration can retry" + ); + assert_eq!( + fake_backend::names_for(svc), + vec![BLOB_KEY.to_string()], + "rollback must leave no partially-migrated credentials and no index" + ); + + // Reads still resolve, out of the surviving blob — the abort is invisible. + assert_eq!( + store.load("identity").unwrap(), + Some("nsec1identityvalue".to_string()) + ); + let (key5, value5) = agent_pair(5); + assert_eq!(store.load(&key5).unwrap(), Some(value5)); + assert_eq!(store.probe(&doomed), KeyringProbe::Present); + + // Once the backend recovers, the retry completes. + fake_backend::clear_failures(); + store.ensure_migrated().unwrap(); + assert_eq!(fake_backend::raw(svc, BLOB_KEY), None); + let (key3, value3) = agent_pair(3); + assert_eq!( + fake_backend::raw(svc, &key3).as_deref(), + Some(value3.as_str()) + ); + assert_eq!( + SecretStore::with_fake(svc) + .load_all_readonly() + .unwrap() + .unwrap(), + blob + ); + + fake_backend::reset(svc); + } + + #[test] + fn migration_rollback_restores_prior_credential_values() { + // The previous test aborts at whatever point `HashMap` iteration puts + // the doomed key. This one fails the *index* write, which runs only + // after every secret has been written and verified, so the rollback + // journal is guaranteed full — including one credential that already + // existed and must be put back rather than deleted. + let svc = "buzz-test-fake-migrate-rollback-restore"; + fake_backend::reset(svc); + + let mut blob = HashMap::new(); + blob.insert("identity".to_string(), "nsec1new-identity".to_string()); + for i in 0..4 { + let (key, value) = agent_pair(i); + blob.insert(key, value); + } + let original = blob_json(&blob); + fake_backend::seed(svc, BLOB_KEY, &original); + fake_backend::seed(svc, "identity", "nsec1stale-identity"); + fake_backend::fail_writes_for(svc, INDEX_KEY); + + let store = SecretStore::with_fake(svc); + let err = store.ensure_migrated().unwrap_err(); + assert!( + err.contains("index"), + "the index write must be the failure: {err}" + ); + + assert_eq!( + fake_backend::raw(svc, BLOB_KEY).as_deref(), + Some(original.as_str()), + "the blob must survive a failure at the index step too" + ); + assert_eq!( + fake_backend::raw(svc, "identity").as_deref(), + Some("nsec1stale-identity"), + "a credential that existed before the migration must be restored to \ + its prior value, not deleted" + ); + for i in 0..4 { + let (key, _) = agent_pair(i); + assert_eq!( + fake_backend::raw(svc, &key), + None, + "{key} was created by this attempt and must be rolled back" + ); + } + assert_eq!( + fake_backend::names_for(svc), + vec!["identity".to_string(), BLOB_KEY.to_string()], + "nothing else may be left behind" + ); + + // The retry then completes, and the blob's copy wins over the stale one. + fake_backend::clear_failures(); + store.ensure_migrated().unwrap(); + assert_eq!( + fake_backend::raw(svc, "identity").as_deref(), + Some("nsec1new-identity") + ); + assert_eq!(fake_backend::raw(svc, BLOB_KEY), None); + assert_eq!( + SecretStore::with_fake(svc) + .load_all_readonly() + .unwrap() + .unwrap(), + blob + ); + + fake_backend::reset(svc); + } + + #[test] + fn sixteen_agents_plus_identity_all_reach_the_credential_store() { + // The regression this whole change exists for: as one JSON blob, + // identity + 8 agents already filled ~2380 of the 2560 bytes Windows + // allows in a credential, so agent 9 onward could never be written at + // all and silently fell back to inline plaintext. One credential per + // secret removes the shared budget. + let svc = "buzz-test-fake-sixteen-agents"; + fake_backend::reset(svc); + + let store = SecretStore::with_fake(svc); + let identity = format!("nsec1{:0>58}", 999); + store.store("identity", &identity).unwrap(); + + let mut expected = HashMap::new(); + expected.insert("identity".to_string(), identity); + for i in 0..16 { + let (key, value) = agent_pair(i); + store + .store(&key, &value) + .unwrap_or_else(|e| panic!("agent {i} must reach the credential store: {e}")); + expected.insert(key, value); + } + + // Cold store: every secret round-trips out of the OS, not the cache. + let reader = SecretStore::with_fake(svc); + for (key, value) in &expected { + assert_eq!( + reader.load(key).unwrap().as_deref(), + Some(value.as_str()), + "{key} must load back" + ); + } + assert_eq!( + reader.load_all_readonly().unwrap().unwrap(), + expected, + "identity + 16 agents must all be enumerable" + ); + + // Proof the test is not vacuous: the old format put this whole set in + // one credential, and that write is still rejected by the backend. + let as_one_blob = blob_json(&expected); + assert!( + fake_backend::blob_bytes(&as_one_blob) > fake_backend::MAX_BLOB_BYTES, + "regression guard: the fixture must exceed the single-credential cap" + ); + assert!( + fake_backend::set(svc, "would-be-blob", &as_one_blob).is_err(), + "regression guard: the single-blob write must still fail — that is the bug being fixed" + ); + // The index is over one credential's worth of names too, so it chunks. + assert!( + fake_backend::raw(svc, &index_chunk_key(1)).is_some(), + "the name index must spill into a second chunk rather than overflow" + ); + // And nothing written anywhere is over the cap. + for name in fake_backend::names_for(svc) { + let value = fake_backend::raw(svc, &name).unwrap(); + assert!( + fake_backend::blob_bytes(&value) <= fake_backend::MAX_BLOB_BYTES, + "credential {name} is {} bytes, over the OS cap", + fake_backend::blob_bytes(&value) + ); + } + + // Deleting one agent leaves the other fifteen and the identity alone. + let (gone, _) = agent_pair(7); + store.delete(&gone).unwrap(); + let after = SecretStore::with_fake(svc) + .load_all_readonly() + .unwrap() + .unwrap(); + assert_eq!(after.len(), 16); + assert!(!after.contains_key(&gone)); + assert!(after.contains_key("identity")); + + fake_backend::reset(svc); + } + + #[test] + fn identity_survives_blob_migration_on_both_service_names() { + // The human identity shares the blob with the agent keys, on both the + // release and the dev service name. Neither may lose it. + for svc in ["buzz-desktop", "buzz-desktop-dev"] { + fake_backend::reset(svc); + + let expected = format!("nsec1identity-{svc}"); + let mut blob = HashMap::new(); + blob.insert("identity".to_string(), expected.clone()); + blob.insert("agent:aaa".to_string(), "nsec1aaa".to_string()); + fake_backend::seed(svc, BLOB_KEY, &blob_json(&blob)); + + let store = SecretStore::with_fake(svc); + assert_eq!( + store.load("identity").unwrap(), + Some(expected.clone()), + "{svc}: identity must survive the migration" + ); + assert_eq!(store.probe("identity"), KeyringProbe::Present, "{svc}"); + assert!( + store.verify_stored_raw("identity", &expected).unwrap(), + "{svc}: identity must verify against the OS, not the cache" + ); + assert_eq!( + fake_backend::raw(svc, "identity").as_deref(), + Some(expected.as_str()), + "{svc}: identity must have its own credential" + ); + assert_eq!(fake_backend::raw(svc, BLOB_KEY), None, "{svc}: blob gone"); + + // Sign-out still wipes it everywhere. + store.delete_all_with_legacy_cleanup().unwrap(); + assert!(store.verify_fully_wiped(), "{svc}: wipe must verify"); + assert_eq!( + fake_backend::names_for(svc), + Vec::::new(), + "{svc}: no credential may survive sign-out" + ); + + fake_backend::reset(svc); + } + } + + // The point of this guard is to spell out each signature verbatim, so + // factoring one out into a type alias would defeat it. + #[allow(clippy::type_complexity)] + #[test] + fn public_api_surface_is_unchanged() { + // Amputation guard: every public entry point callers depend on must + // keep its exact signature. `storage.rs`, `app_state.rs`, `reset.rs`, + // `identity.rs` and `pairing.rs` all bind to these. + let _: fn(String) -> SecretStore = SecretStore::keyring; + let _: fn(&'static str) -> SecretStore = SecretStore::keyring; + let _: fn(&'static str) -> &'static SecretStore = SecretStore::shared; + let _: fn(&SecretStore, &str) -> KeyringProbe = SecretStore::probe; + let _: fn(&SecretStore, &str) -> Result, String> = SecretStore::load; + let _: fn(&SecretStore) -> Result>, String> = + SecretStore::load_all_readonly; + let _: fn(&SecretStore, &str, &str) -> Result<(), String> = SecretStore::store; + let _: fn(&SecretStore, &HashMap) -> Result<(), String> = + SecretStore::store_all; + let _: fn(&SecretStore, &str, &str) -> Result = + SecretStore::verify_stored_raw; + let _: fn(&SecretStore, &str) -> Result<(), String> = SecretStore::delete; + let _: fn(&SecretStore) -> Result<(), String> = SecretStore::delete_all_with_legacy_cleanup; + let _: fn(&SecretStore) -> bool = SecretStore::verify_fully_wiped; + + // KeyringProbe's variants are matched on by callers. + let _ = KeyringProbe::Present; + let _ = KeyringProbe::ReachableButEmpty; + let _ = KeyringProbe::Unreachable; + } }