From d1e2df18c3aaade4c5b42be397fa4bd2c26b5f75 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Mon, 22 Jun 2026 13:03:08 -0400 Subject: [PATCH 1/8] feat(desktop): store nsec private keys in OS keyring MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Desktop nsec private keys (the human identity and every managed-agent key) lived in plaintext on disk — identity.key and inline in managed-agents.json. Move them into the OS keyring (macOS Keychain, Windows Credential Manager, Linux Secret Service) behind a default-on system-keyring feature, mirroring goose's backend matrix. A new SecretStore wraps the keyring with a KeyringProbe that distinguishes reachable-but-empty (safe to migrate into) from unreachable (must not migrate). Migration imports the plaintext key, read-back-verifies it, then deletes the plaintext — and is skipped entirely when the keyring is unreachable, so a transient outage cannot resurrect a rotated key from a leftover file. Keyringless environments fall back to a 0o600 file. Agent keys become an in-memory #[serde(skip)] field hydrated in load_managed_agents and written-back in save_managed_agents, so the nine existing read sites are untouched and the creation path persists through the same chokepoint (no silent key loss on restart). The BUZZ_PRIVATE_KEY env override is left on its upstream path and never routed through SecretStore, preserving env-first precedence for agents and CI. SECURITY.md gains a keyring note and the audit-log description is corrected from HMAC to keyless SHA-256 hash chain. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- SECURITY.md | 27 +- desktop/src-tauri/Cargo.lock | 1 + desktop/src-tauri/Cargo.toml | 10 +- desktop/src-tauri/src/app_state.rs | 296 +++++++++++++++--- desktop/src-tauri/src/commands/agents.rs | 2 + desktop/src-tauri/src/lib.rs | 1 + .../src-tauri/src/managed_agents/storage.rs | 140 ++++++++- desktop/src-tauri/src/managed_agents/types.rs | 93 +++++- desktop/src-tauri/src/secret_store.rs | 169 ++++++++++ 9 files changed, 685 insertions(+), 54 deletions(-) create mode 100644 desktop/src-tauri/src/secret_store.rs diff --git a/SECURITY.md b/SECURITY.md index 77472c423a..96222e029d 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -67,9 +67,30 @@ unless the subscriber is a member. ### Append-Only Audit Log All events are written to a tamper-evident audit log (`buzz-audit`). Each -log entry is chained to the previous one via an HMAC, making retroactive -modification detectable. The audit log is designed for SOX-grade compliance -and eDiscovery. +log entry is chained to the previous one via a SHA-256 hash chain. Because the +chain is keyless, it is tamper-evident but not tamper-resistant: it detects +accidental corruption or single-row edits, but an attacker with database write +access can recompute the entire chain after editing. The audit log is designed +for SOX-grade compliance and eDiscovery. + +### Desktop Secret Storage — OS Keyring + +The Buzz desktop app stores nsec private keys in the operating system keyring +rather than in plaintext files: macOS Keychain, Windows Credential Manager, or +the Linux Secret Service (`gnome-keyring` / `kwallet` via D-Bus). This covers +both the human identity key and every managed-agent key. + +On first launch after upgrading, existing plaintext keys are migrated into the +keyring: the key is imported, read back to verify the round-trip, and only then +is the plaintext deleted. Migration runs only when the keyring is reachable — +if the backend is unavailable that session, the app keeps reading from the +plaintext file and does **not** migrate, so a transient outage cannot resurrect +a rotated key from a leftover file. + +When no keyring backend is available (headless Linux with no Secret Service, for +example), keys fall back to a `0o600` owner-only file. The `BUZZ_PRIVATE_KEY` +environment variable, when set, always takes precedence over both stores — this +is how harnessed agents and CI receive their identity. ### Input Validation diff --git a/desktop/src-tauri/Cargo.lock b/desktop/src-tauri/Cargo.lock index e3c4a1e9e2..32ce245a39 100644 --- a/desktop/src-tauri/Cargo.lock +++ b/desktop/src-tauri/Cargo.lock @@ -865,6 +865,7 @@ dependencies = [ "futures-util", "hex", "infer", + "keyring", "libc", "mesh-llm-host-runtime", "mesh-llm-sdk", diff --git a/desktop/src-tauri/Cargo.toml b/desktop/src-tauri/Cargo.toml index 7b0d422133..a500b70dfc 100644 --- a/desktop/src-tauri/Cargo.toml +++ b/desktop/src-tauri/Cargo.toml @@ -17,8 +17,11 @@ name = "buzz_lib" crate-type = ["staticlib", "cdylib", "rlib"] [features] -default = [] +default = ["system-keyring"] mesh-llm = ["dep:mesh-llm-sdk", "dep:mesh-llm-host-runtime"] +# OS keyring backing for desktop secret storage (nsec private keys). When +# disabled, secrets fall back to 0o600 files. On by default for real builds. +system-keyring = ["dep:keyring"] [build-dependencies] serde = { version = "1", features = ["derive"] } @@ -31,9 +34,14 @@ ctrlc = { version = "3", features = ["termination"] } [target.'cfg(target_os = "macos")'.dependencies] objc2-app-kit = { version = "0.3.2", default-features = false, features = ["NSHapticFeedback"] } +keyring = { version = "3.6.3", default-features = false, features = ["apple-native", "vendored"], optional = true } [target.'cfg(windows)'.dependencies] windows-sys = { version = "0.61", features = ["Win32_Storage_FileSystem", "Win32_System_JobObjects", "Win32_System_Threading", "Win32_Foundation"] } +keyring = { version = "3.6.3", default-features = false, features = ["windows-native", "vendored"], optional = true } + +[target.'cfg(target_os = "linux")'.dependencies] +keyring = { version = "3.6.3", default-features = false, features = ["sync-secret-service", "vendored"], optional = true } [dependencies] atomic-write-file = "0.3" diff --git a/desktop/src-tauri/src/app_state.rs b/desktop/src-tauri/src/app_state.rs index a20dc815e0..461e4e2984 100644 --- a/desktop/src-tauri/src/app_state.rs +++ b/desktop/src-tauri/src/app_state.rs @@ -45,30 +45,41 @@ pub struct AppState { pub mesh_coordinator: AsyncMutex>, } -pub fn build_app_state() -> AppState { - // Env var takes precedence (dev/CI). If absent, resolve_persisted_identity() - // in setup() will replace the ephemeral placeholder with a persisted key. - let (keys, source) = match std::env::var("BUZZ_PRIVATE_KEY") { +/// Parse the `BUZZ_PRIVATE_KEY` env var into identity keys. `Some` means the +/// env var was present and valid and MUST win over any persisted/keyring key +/// (the dev/CI/harness override). `None` means absent or malformed — callers +/// fall through to persisted resolution. A malformed value is logged and +/// treated as absent rather than left on an ephemeral identity. +fn identity_from_env() -> Option { + match std::env::var("BUZZ_PRIVATE_KEY") { Ok(nsec) => match Keys::parse(nsec.trim()) { - Ok(keys) => (keys, "configured"), + Ok(keys) => Some(keys), Err(error) => { eprintln!("buzz-desktop: invalid BUZZ_PRIVATE_KEY: {error}"); - (Keys::generate(), "ephemeral") + None } }, Err(std::env::VarError::NotUnicode(_)) => { eprintln!("buzz-desktop: BUZZ_PRIVATE_KEY contains invalid UTF-8"); - (Keys::generate(), "ephemeral") + None } - Err(std::env::VarError::NotPresent) => (Keys::generate(), "ephemeral"), - }; - - if source == "configured" { - eprintln!( - "buzz-desktop: configured identity pubkey {}", - keys.public_key().to_hex() - ); + Err(std::env::VarError::NotPresent) => None, } +} + +pub fn build_app_state() -> AppState { + // Env var takes precedence (dev/CI). If absent, resolve_persisted_identity() + // in setup() will replace the ephemeral placeholder with a persisted key. + let keys = match identity_from_env() { + Some(keys) => { + eprintln!( + "buzz-desktop: configured identity pubkey {}", + keys.public_key().to_hex() + ); + keys + } + None => Keys::generate(), + }; AppState { keys: Mutex::new(keys), @@ -135,10 +146,8 @@ pub fn resolve_persisted_identity(app: &AppHandle, state: &AppState) -> Result<( // Only skip file-based resolution if the env var was present AND parsed // successfully. A malformed env var should fall through to the persisted // key rather than leaving the app on an ephemeral identity. - if let Ok(nsec) = std::env::var("BUZZ_PRIVATE_KEY") { - if Keys::parse(nsec.trim()).is_ok() { - return Ok(()); - } + if identity_from_env().is_some() { + return Ok(()); } let data_dir = app @@ -146,48 +155,201 @@ pub fn resolve_persisted_identity(app: &AppHandle, state: &AppState) -> Result<( .app_data_dir() .map_err(|e| format!("app data dir: {e}"))?; std::fs::create_dir_all(&data_dir).map_err(|e| format!("create app data dir: {e}"))?; - let key_path = data_dir.join("identity.key"); - // Try to load an existing key. - if key_path.exists() { - match load_key_file(&key_path) { + let keys = load_or_create_identity(&data_dir)?; + *state.keys.lock().map_err(|e| e.to_string())? = keys; + Ok(()) +} + +/// Service name for the desktop OS keyring. Shared by the human identity key +/// and managed-agent keys (each addressed by a distinct key name within it). +pub(crate) const KEYRING_SERVICE: &str = "buzz-desktop"; + +/// Keyring key name for the human identity nsec. +const IDENTITY_KEY_NAME: &str = "identity"; + +/// Resolve the human identity key: migrate a legacy `identity.key` into the +/// keyring when safe, otherwise load from whichever backend holds it, else +/// generate-and-save. +/// +/// Migration rule (prevents stale-key resurrection): only import the plaintext +/// file when the keyring is REACHABLE-but-empty. If the keyring is UNREACHABLE +/// this boot, fall back to reading the file directly and do NOT migrate — a +/// later import from a leftover (possibly rotated) file could resurrect an old +/// key. +fn load_or_create_identity(data_dir: &std::path::Path) -> Result { + use crate::secret_store::KeyringProbe; + + let legacy_path = data_dir.join("identity.key"); + + // No keyring available in this build: the `0o600` file is the only store. + if !cfg!(feature = "system-keyring") { + return load_file_or_generate(&legacy_path, data_dir); + } + + let store = crate::secret_store::SecretStore::keyring(KEYRING_SERVICE); + + match store.probe(IDENTITY_KEY_NAME) { + KeyringProbe::Present => { + if let Some(nsec) = store.load(IDENTITY_KEY_NAME)? { + return parse_or_quarantine(&nsec, &legacy_path, data_dir); + } + // Probe said Present but load found nothing — treat as empty. + } + KeyringProbe::ReachableButEmpty => { + // One-time migration: import the legacy plaintext file, read-back + // verify, THEN delete it. + if legacy_path.exists() { + if let Some(keys) = migrate_identity_file(&store, &legacy_path)? { + return Ok(keys); + } + } + } + KeyringProbe::Unreachable => { + // Keyring down this boot — read the file directly, do NOT migrate. + return load_file_or_generate(&legacy_path, data_dir); + } + } + + generate_and_persist(&store, &legacy_path) +} + +/// Load the `0o600` identity file, quarantining corruption, else generate and +/// save a fresh key to the file. Used when no keyring is available. +fn load_file_or_generate( + legacy_path: &std::path::Path, + data_dir: &std::path::Path, +) -> Result { + if legacy_path.exists() { + match load_key_file(legacy_path) { Ok(keys) => { eprintln!( "buzz-desktop: persisted identity pubkey {}", keys.public_key().to_hex() ); - *state.keys.lock().map_err(|e| e.to_string())? = keys; - return Ok(()); + return Ok(keys); } - Err(error) => { - // Corrupted — quarantine with a timestamp so prior backups - // are never overwritten. - let ts = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .map(|d| d.as_secs()) - .unwrap_or(0); - let bad_name = format!("identity.key.bad.{ts}"); - eprintln!( - "buzz-desktop: corrupt identity.key ({error}), quarantining to {bad_name}" - ); - let bad_path = data_dir.join(bad_name); - if std::fs::rename(&key_path, &bad_path).is_err() { - let _ = std::fs::remove_file(&key_path); - } + Err(error) => quarantine_corrupt_key(legacy_path, data_dir, &error), + } + } + let keys = Keys::generate(); + save_key_file(legacy_path, &keys)?; + eprintln!( + "buzz-desktop: generated and saved identity pubkey {}", + keys.public_key().to_hex() + ); + Ok(keys) +} + +/// Import the plaintext `identity.key` into the store, verify the round-trip, +/// then delete the file. Returns `Ok(None)` if the file was corrupt (caller +/// continues to generate-and-save). +fn migrate_identity_file( + store: &crate::secret_store::SecretStore, + legacy_path: &std::path::Path, +) -> Result, String> { + let keys = match load_key_file(legacy_path) { + Ok(keys) => keys, + Err(error) => { + eprintln!("buzz-desktop: corrupt identity.key during migration ({error}), skipping"); + return Ok(None); + } + }; + let nsec = keys + .secret_key() + .to_bech32() + .map_err(|e| format!("encode nsec: {e}"))?; + + store.store(IDENTITY_KEY_NAME, &nsec)?; + // Read-back verify before deleting the plaintext file. + match store.load(IDENTITY_KEY_NAME)? { + Some(stored) if stored == nsec => { + if let Err(e) = std::fs::remove_file(legacy_path) { + eprintln!("buzz-desktop: keyring import ok but failed to delete identity.key: {e}"); + } else { + eprintln!("buzz-desktop: migrated identity key into OS keyring"); } + Ok(Some(keys)) } + _ => Err("keyring read-back verify failed for identity key".to_string()), } +} - // First run (or recovery from corruption): generate and save. +/// Generate a fresh identity, persist it through the store, return it. +fn generate_and_persist( + store: &crate::secret_store::SecretStore, + legacy_path: &std::path::Path, +) -> Result { let keys = Keys::generate(); - save_key_file(&key_path, &keys)?; - + persist_identity(store, &keys, legacy_path)?; eprintln!( "buzz-desktop: generated and saved identity pubkey {}", keys.public_key().to_hex() ); - *state.keys.lock().map_err(|e| e.to_string())? = keys; - Ok(()) + Ok(keys) +} + +/// Persist `keys` through the store, falling back to the `0o600` file when the +/// keyring write fails on an availability error. +fn persist_identity( + store: &crate::secret_store::SecretStore, + keys: &Keys, + legacy_path: &std::path::Path, +) -> Result<(), String> { + let nsec = keys + .secret_key() + .to_bech32() + .map_err(|e| format!("encode nsec: {e}"))?; + match store.store(IDENTITY_KEY_NAME, &nsec) { + Ok(()) => Ok(()), + Err(keyring_err) => { + eprintln!("buzz-desktop: keyring write failed ({keyring_err}), using file fallback"); + save_key_file(legacy_path, keys) + } + } +} + +fn parse_or_quarantine( + nsec: &str, + legacy_path: &std::path::Path, + data_dir: &std::path::Path, +) -> Result { + match Keys::parse(nsec.trim()) { + Ok(keys) => { + eprintln!( + "buzz-desktop: persisted identity pubkey {}", + keys.public_key().to_hex() + ); + Ok(keys) + } + Err(error) => { + quarantine_corrupt_key( + legacy_path, + data_dir, + &format!("parse keyring nsec: {error}"), + ); + let store = crate::secret_store::SecretStore::keyring(KEYRING_SERVICE); + generate_and_persist(&store, legacy_path) + } + } +} + +/// Quarantine a corrupt `identity.key` with a timestamp so prior backups are +/// never overwritten. +fn quarantine_corrupt_key(key_path: &std::path::Path, data_dir: &std::path::Path, error: &str) { + if !key_path.exists() { + return; + } + let ts = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .map(|d| d.as_secs()) + .unwrap_or(0); + let bad_name = format!("identity.key.bad.{ts}"); + eprintln!("buzz-desktop: corrupt identity.key ({error}), quarantining to {bad_name}"); + let bad_path = data_dir.join(bad_name); + if std::fs::rename(key_path, &bad_path).is_err() { + let _ = std::fs::remove_file(key_path); + } } fn load_key_file(path: &std::path::Path) -> Result { @@ -241,6 +403,50 @@ mod tests { assert_eq!(a.public_key().to_hex(), b.public_key().to_hex()); } + /// `BUZZ_PRIVATE_KEY` is process-global; serialize the env-mutating tests + /// so they don't race each other under the parallel test runner. + static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + + /// Run `body` with `BUZZ_PRIVATE_KEY` set to `value` (or unset when `None`), + /// restoring the prior value afterward. + fn with_env_key(value: Option<&str>, body: impl FnOnce() -> T) -> T { + let _guard = ENV_LOCK.lock().unwrap_or_else(|e| e.into_inner()); + let prior = std::env::var("BUZZ_PRIVATE_KEY").ok(); + match value { + Some(v) => std::env::set_var("BUZZ_PRIVATE_KEY", v), + None => std::env::remove_var("BUZZ_PRIVATE_KEY"), + } + let out = body(); + match prior { + Some(v) => std::env::set_var("BUZZ_PRIVATE_KEY", v), + None => std::env::remove_var("BUZZ_PRIVATE_KEY"), + } + out + } + + #[test] + fn identity_from_env_wins_when_valid() { + let configured = Keys::generate(); + let nsec = configured.secret_key().to_bech32().unwrap(); + + let resolved = + with_env_key(Some(&nsec), identity_from_env).expect("valid env key must resolve"); + + assert_key_eq(&configured, &resolved); + } + + #[test] + fn identity_from_env_none_when_absent() { + assert!(with_env_key(None, identity_from_env).is_none()); + } + + #[test] + fn identity_from_env_none_when_malformed() { + // A malformed env var falls through to persisted resolution rather than + // winning — otherwise a typo'd key would silently shadow the real one. + assert!(with_env_key(Some("not-a-valid-nsec"), identity_from_env).is_none()); + } + #[test] fn save_and_load_round_trip() { let dir = tempfile::tempdir().unwrap(); diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index 5ca1807132..c54d0b066e 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -1149,6 +1149,8 @@ pub fn delete_managed_agent( return Err(format!("agent {pubkey} not found")); } save_managed_agents(&app, &records)?; + // Remove the agent's nsec from the keyring after the record is gone. + crate::managed_agents::delete_agent_key(&pubkey); } try_regenerate_nest(&app); Ok(()) diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index 8cf55b13e3..55fa38a5a1 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -11,6 +11,7 @@ mod models; pub mod nostr_convert; mod prevent_sleep; mod relay; +mod secret_store; mod templates; mod util; diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index 2bd975ca06..6206f63f80 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -6,7 +6,25 @@ use std::{ use tauri::{AppHandle, Manager}; +use crate::app_state::KEYRING_SERVICE; use crate::managed_agents::ManagedAgentRecord; +use crate::secret_store::{KeyringProbe, SecretStore}; + +/// Keyring key name for an agent's nsec, namespaced from the human identity +/// key (`"identity"`) which shares the service. +fn agent_keyring_name(pubkey: &str) -> String { + format!("agent:{pubkey}") +} + +/// The agent secret store. `None` when the build has no keyring backend, in +/// which case agent keys stay inline in the `0o600` JSON file. +fn agent_secret_store() -> Option { + if cfg!(feature = "system-keyring") { + Some(SecretStore::keyring(KEYRING_SERVICE)) + } else { + None + } +} pub fn managed_agents_base_dir(app: &AppHandle) -> Result { let dir = app @@ -40,7 +58,41 @@ pub fn load_managed_agents(app: &AppHandle) -> Result, S let content = fs::read_to_string(&path) .map_err(|error| format!("failed to read agent store: {error}"))?; - serde_json::from_str(&content).map_err(|error| format!("failed to parse agent store: {error}")) + let mut records: Vec = serde_json::from_str(&content) + .map_err(|error| format!("failed to parse agent store: {error}"))?; + + hydrate_keys(&mut records); + Ok(records) +} + +/// Fill in each record's in-memory `private_key_nsec` from the keyring when it +/// was not serialized inline. A record loaded with a non-empty key came from +/// the JSON file-fallback (keyring was unreachable when it was written) — leave +/// it as-is. A record with an empty key has its secret in the keyring. +fn hydrate_keys(records: &mut [ManagedAgentRecord]) { + let Some(store) = agent_secret_store() else { + return; + }; + for record in records.iter_mut() { + if !record.private_key_nsec.is_empty() { + continue; + } + match store.load(&agent_keyring_name(&record.pubkey)) { + Ok(Some(nsec)) => record.private_key_nsec = nsec, + Ok(None) => { + eprintln!( + "buzz-desktop: agent {} has no key in JSON or keyring", + record.pubkey + ); + } + Err(e) => { + eprintln!( + "buzz-desktop: failed to read agent {} key from keyring: {e}", + record.pubkey + ); + } + } + } } pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> Result<(), String> { @@ -52,11 +104,95 @@ pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> R .then_with(|| left.pubkey.cmp(&right.pubkey)) }); + // Persist each key to the keyring; on success blank the inline copy so it + // is skipped from JSON (`skip_serializing_if = "String::is_empty"`). If the + // keyring is unreachable, the key stays inline and the JSON gets 0o600. + let mut any_inline_key = false; + persist_agent_keys(&mut sorted, &mut any_inline_key); + let path = managed_agents_store_path(app)?; let payload = serde_json::to_vec_pretty(&sorted) .map_err(|error| format!("failed to serialize agent store: {error}"))?; - atomic_write_json(&path, &payload) + atomic_write_json(&path, &payload)?; + + // The JSON only carries plaintext keys in the keyringless fallback; lock it + // down to owner-only in that case. + if any_inline_key { + restrict_json_permissions(&path); + } + Ok(()) +} + +/// Write each record's in-memory key to the keyring and blank the inline copy +/// on success. Sets `any_inline_key` if any key had to stay in the JSON because +/// the keyring was unreachable. Mutates `records` (a save-local clone) — the +/// caller's in-memory records keep their keys. +fn persist_agent_keys(records: &mut [ManagedAgentRecord], any_inline_key: &mut bool) { + let Some(store) = agent_secret_store() else { + // No keyring backend: keys stay inline. + *any_inline_key = records.iter().any(|r| !r.private_key_nsec.is_empty()); + return; + }; + for record in records.iter_mut() { + if record.private_key_nsec.is_empty() { + continue; + } + let name = agent_keyring_name(&record.pubkey); + match store.probe(&name) { + KeyringProbe::Unreachable => { + // Keep the key inline (file fallback); do not migrate. + *any_inline_key = true; + } + KeyringProbe::Present | KeyringProbe::ReachableButEmpty => { + match write_and_verify(&store, &name, &record.private_key_nsec) { + Ok(()) => record.private_key_nsec.clear(), + Err(e) => { + eprintln!( + "buzz-desktop: keyring write for agent {} failed ({e}), keeping inline", + record.pubkey + ); + *any_inline_key = true; + } + } + } + } + } +} + +/// Write `value` to the keyring and read it back to confirm before the caller +/// strips the inline copy. +fn write_and_verify(store: &SecretStore, name: &str, value: &str) -> Result<(), String> { + store.store(name, value)?; + match store.load(name)? { + Some(stored) if stored == value => Ok(()), + _ => Err("keyring read-back verify failed".to_string()), + } +} + +#[cfg(unix)] +fn restrict_json_permissions(path: &Path) { + use std::os::unix::fs::PermissionsExt; + let resolved = std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); + if let Err(e) = std::fs::set_permissions(&resolved, std::fs::Permissions::from_mode(0o600)) { + eprintln!( + "buzz-desktop: failed to restrict {} permissions: {e}", + resolved.display() + ); + } +} + +#[cfg(not(unix))] +fn restrict_json_permissions(_path: &Path) {} + +/// Remove an agent's key from the keyring (best-effort). Called when an agent +/// is deleted so its secret does not linger in the OS store. +pub fn delete_agent_key(pubkey: &str) { + if let Some(store) = agent_secret_store() { + if let Err(e) = store.delete(&agent_keyring_name(pubkey)) { + eprintln!("buzz-desktop: failed to delete agent {pubkey} key from keyring: {e}"); + } + } } /// Atomic, symlink-preserving JSON write. diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 09dac97d70..716c347288 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -88,9 +88,16 @@ pub struct ManagedAgentRecord { pub name: String, #[serde(default)] pub persona_id: Option, - /// `#[serde(default)]` so an old build still parses a store whose inline - /// key was stripped after a keyring build migrated it into the Keychain. - #[serde(default)] + /// nsec private key. Held in memory but persisted to the OS keyring (keyed + /// by `pubkey`) rather than serialized to `managed-agents.json`. The + /// storage layer blanks this before writing JSON once the key is safely in + /// the keyring, and re-hydrates it from the keyring on load. + /// + /// It is only serialized inline (the `0o600` JSON fallback) when the + /// keyring is unreachable — `skip_serializing_if` keeps it out of JSON in + /// the normal keyring-backed case. `default` also lets an old build parse a + /// store whose inline key was already migrated out and blanked. + #[serde(default, skip_serializing_if = "String::is_empty")] pub private_key_nsec: String, /// NIP-OA auth tag JSON. Computed at agent creation time. /// @@ -1006,4 +1013,84 @@ mod tests { assert_eq!(record.symlink_target, None); assert_eq!(record.version, None); } + + /// A record whose in-memory key was blanked (because it lives in the + /// keyring) must NOT serialize `private_key_nsec` into JSON. + #[test] + fn managed_agent_record_omits_empty_key_from_json() { + let mut record = sample_agent_record(); + record.private_key_nsec = String::new(); + + let json = serde_json::to_string(&record).expect("serialize"); + assert!( + !json.contains("private_key_nsec"), + "blanked key must be skipped from JSON, got: {json}" + ); + } + + /// A record with an inline key (the keyringless `0o600` JSON fallback) + /// serializes the key and round-trips it back. + #[test] + fn managed_agent_record_serializes_inline_key_for_fallback() { + let mut record = sample_agent_record(); + record.private_key_nsec = "nsec1fallback".to_string(); + + let json = serde_json::to_string(&record).expect("serialize"); + assert!(json.contains("nsec1fallback")); + + let back: ManagedAgentRecord = serde_json::from_str(&json).expect("deserialize"); + assert_eq!(back.private_key_nsec, "nsec1fallback"); + } + + /// A keyring-backed record on disk lacks `private_key_nsec`; it must + /// deserialize with an empty key (to be hydrated from the keyring). + #[test] + fn managed_agent_record_without_key_deserializes_empty() { + let record: ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("keyring-backed record without inline key should deserialize"); + + assert_eq!(record.private_key_nsec, ""); + } + + fn sample_agent_record() -> ManagedAgentRecord { + serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("sample record") + } } diff --git a/desktop/src-tauri/src/secret_store.rs b/desktop/src-tauri/src/secret_store.rs new file mode 100644 index 0000000000..b19be29553 --- /dev/null +++ b/desktop/src-tauri/src/secret_store.rs @@ -0,0 +1,169 @@ +//! OS keyring access for desktop nsec private keys. +//! +//! Backed by the `keyring` crate (macOS Keychain / Windows Credential Manager / +//! Linux Secret Service via D-Bus). The chosen backend is selected at compile +//! time by the per-target feature in `Cargo.toml`. The `system-keyring` +//! feature gates the whole store; when it is off, [`SecretStore`] is unusable +//! and callers fall back to their own `0o600` file storage. +//! +//! The store is deliberately NOT on any env-read path. `BUZZ_PRIVATE_KEY` +//! resolution for harnessed agents and CI is handled upstream (an env +//! short-circuit for the human key, child-process env injection for agents); +//! adding an env tier here would duplicate that precedence and create a +//! divergent-behavior trap. + +/// Result of probing the keyring before a migration: distinguishes "reachable +/// but holds no entry" (safe to migrate into) from "unreachable this boot" +/// (must NOT migrate — re-importing from a leftover plaintext file could +/// resurrect a rotated/stale key). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum KeyringProbe { + /// Keyring is reachable and an entry for the key already exists. + Present, + /// Keyring is reachable but has no entry for the key. + ReachableButEmpty, + /// Keyring backend is unavailable this boot (no Secret Service, dbus + /// failure, etc.). Migration must be skipped. + Unreachable, +} + +/// An OS keyring, addressed by service name. Each logical secret is a distinct +/// key within the service (passed to each operation as the keyring "username"). +pub struct SecretStore { + service: String, +} + +impl SecretStore { + /// Keyring-backed store under `service`. The active platform backend + /// (apple-native / windows-native / sync-secret-service) is chosen at + /// compile time. + pub fn keyring(service: impl Into) -> Self { + SecretStore { + service: service.into(), + } + } +} + +/// Whether a keyring error string indicates the backend itself is unavailable +/// (vs. a per-entry error like "not found"). Mirrors goose's discriminator +/// (`crates/goose/src/config/base.rs`): treat dbus / Secret Service / platform +/// secure-storage failures as "keyring unavailable, fall back to file". +#[cfg(feature = "system-keyring")] +fn is_keyring_availability_error(error_str: &str) -> bool { + let lower = error_str.to_lowercase(); + lower.contains("keyring") + || lower.contains("dbus") + || lower.contains("org.freedesktop.secrets") + || lower.contains("platform secure storage") + || lower.contains("no secret service") +} + +#[cfg(feature = "system-keyring")] +fn keyring_entry(service: &str, key: &str) -> Result { + keyring::Entry::new(service, key) +} + +impl SecretStore { + /// Probe whether `key` exists and whether the backend is reachable. + pub fn probe(&self, key: &str) -> KeyringProbe { + #[cfg(feature = "system-keyring")] + { + 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 + } + // A non-availability per-entry error (e.g. bad attributes) + // means the backend is reachable but the entry is unusable. + Err(_) => KeyringProbe::ReachableButEmpty, + }, + Err(e) if is_keyring_availability_error(&e.to_string()) => { + KeyringProbe::Unreachable + } + Err(_) => KeyringProbe::Unreachable, + } + } + #[cfg(not(feature = "system-keyring"))] + { + let _ = key; + KeyringProbe::Unreachable + } + } + + /// 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". + pub fn load(&self, key: &str) -> Result, String> { + #[cfg(feature = "system-keyring")] + { + let entry = + keyring_entry(&self.service, key).map_err(|e| format!("keyring entry: {e}"))?; + match entry.get_password() { + Ok(secret) => Ok(Some(secret)), + Err(keyring::Error::NoEntry) => Ok(None), + Err(e) => Err(format!("keyring get: {e}")), + } + } + #[cfg(not(feature = "system-keyring"))] + { + let _ = key; + Err("system-keyring feature disabled".to_string()) + } + } + + /// Store `value` for `key`. Reports `Err` on availability failures — callers + /// decide whether to fall back to file storage. + pub fn store(&self, key: &str, value: &str) -> Result<(), String> { + #[cfg(feature = "system-keyring")] + { + let entry = + keyring_entry(&self.service, key).map_err(|e| format!("keyring entry: {e}"))?; + entry + .set_password(value) + .map_err(|e| format!("keyring set: {e}")) + } + #[cfg(not(feature = "system-keyring"))] + { + let _ = (key, value); + Err("system-keyring feature disabled".to_string()) + } + } + + /// Delete the secret for `key`. A missing entry is not an error. + pub fn delete(&self, key: &str) -> Result<(), String> { + #[cfg(feature = "system-keyring")] + { + 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) => Err(format!("keyring delete: {e}")), + } + } + #[cfg(not(feature = "system-keyring"))] + { + let _ = key; + Err("system-keyring feature disabled".to_string()) + } + } +} + +#[cfg(all(test, feature = "system-keyring"))] +mod tests { + use super::*; + + #[test] + fn availability_error_discriminator() { + assert!(is_keyring_availability_error("dbus connection failed")); + assert!(is_keyring_availability_error( + "org.freedesktop.secrets not provided" + )); + assert!(is_keyring_availability_error("No Secret Service")); + assert!(is_keyring_availability_error( + "Platform secure storage failure" + )); + // A plain "not found" is per-entry, not an availability failure. + assert!(!is_keyring_availability_error("entry not found")); + } +} From e4dbce213f75b6340f34f1adb6f816bfd44e9795 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Mon, 22 Jun 2026 13:19:59 -0400 Subject: [PATCH 2/8] fix(desktop): purge lingering plaintext nsec in keyring failure paths Two narrow failure paths left a plaintext nsec on disk after migration, defeating the guarantee this series ships (neither lost a key): - Human identity: a migration whose remove_file failed (AV lock, read-only mount, EPERM) left identity.key on disk, and the keyring-Present boot arm never retried the delete. The Present arm now best-effort removes a leftover file once the keyring is authoritative. - Agent keys: a save during a transient keyring outage re-inlines the key into managed-agents.json, but hydrate_keys skipped non-empty records and so never re-stripped it until a later event-driven save. hydrate_keys now opportunistically re-migrates an inline key when the keyring is reachable, keeping the key in memory for readers while the next save writes clean JSON. The migrate-vs-keep decision is extracted into migrate_inline_key, the single source of truth shared by the load-time re-migrate and the save chokepoint, behind a KeyStore trait so the decision is unit-tested without the live OS keyring. The no-resurrection guard (keyring-unreachable -> keep inline, never migrate) holds on both paths. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/app_state.rs | 49 ++- .../src-tauri/src/managed_agents/storage.rs | 292 +++++++++++++++--- 2 files changed, 289 insertions(+), 52 deletions(-) diff --git a/desktop/src-tauri/src/app_state.rs b/desktop/src-tauri/src/app_state.rs index 461e4e2984..c13591b522 100644 --- a/desktop/src-tauri/src/app_state.rs +++ b/desktop/src-tauri/src/app_state.rs @@ -192,7 +192,13 @@ fn load_or_create_identity(data_dir: &std::path::Path) -> Result { match store.probe(IDENTITY_KEY_NAME) { KeyringProbe::Present => { if let Some(nsec) = store.load(IDENTITY_KEY_NAME)? { - return parse_or_quarantine(&nsec, &legacy_path, data_dir); + let keys = parse_or_quarantine(&nsec, &legacy_path, data_dir)?; + // The key is authoritative in the keyring. A leftover + // `identity.key` means a prior migration's `remove_file` failed + // (transient AV lock, read-only mount, EPERM) and never retried + // — clean it up now so plaintext does not linger on disk. + cleanup_leftover_identity_file(&legacy_path); + return Ok(keys); } // Probe said Present but load found nothing — treat as empty. } @@ -334,6 +340,19 @@ fn parse_or_quarantine( } } +/// Best-effort removal of a leftover `identity.key` once the keyring is the +/// authoritative store. Idempotent: a missing file is success. Logs but does +/// not error on failure — a delete failure must never block startup. +fn cleanup_leftover_identity_file(legacy_path: &std::path::Path) { + if !legacy_path.exists() { + return; + } + match std::fs::remove_file(legacy_path) { + Ok(()) => eprintln!("buzz-desktop: removed leftover identity.key (key is in keyring)"), + Err(e) => eprintln!("buzz-desktop: failed to remove leftover identity.key: {e}"), + } +} + /// Quarantine a corrupt `identity.key` with a timestamp so prior backups are /// never overwritten. fn quarantine_corrupt_key(key_path: &std::path::Path, data_dir: &std::path::Path, error: &str) { @@ -484,6 +503,34 @@ mod tests { assert!(load_key_file(&path).is_err()); } + #[test] + fn cleanup_removes_leftover_identity_file() { + // Item 1: a leftover identity.key (from a migration whose remove_file + // failed) is deleted once the keyring is authoritative, so plaintext + // does not linger on disk. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("identity.key"); + save_key_file(&path, &Keys::generate()).unwrap(); + assert!(path.exists()); + + cleanup_leftover_identity_file(&path); + + assert!(!path.exists()); + } + + #[test] + fn cleanup_is_noop_when_no_leftover_file() { + // Idempotent: the cleanup runs on every keyring-Present boot, so a + // missing file must be a silent success, not an error or panic. + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("identity.key"); + assert!(!path.exists()); + + cleanup_leftover_identity_file(&path); + + assert!(!path.exists()); + } + #[test] fn save_creates_file_with_valid_nsec() { let dir = tempfile::tempdir().unwrap(); diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index 6206f63f80..b513e04cb2 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -50,6 +50,72 @@ pub fn managed_agent_log_path(app: &AppHandle, pubkey: &str) -> Result KeyringProbe; + /// Write `value` and read it back to confirm before the caller strips the + /// inline copy. + fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String>; +} + +impl KeyStore for SecretStore { + fn probe(&self, name: &str) -> KeyringProbe { + SecretStore::probe(self, name) + } + 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()), + } + } +} + +/// Outcome of attempting to lift a record's inline key into the keyring. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum KeyMigration { + /// Written to the keyring and read-back verified. Safe to drop the inline + /// copy when serializing. + Persisted, + /// Could not persist (keyring unreachable, or write/verify failed). The key + /// must stay inline (0o600 file fallback); do NOT drop it. + KeptInline, +} + +/// Attempt to lift one record's inline key into the keyring with read-back +/// verify. Pure decision logic — does NOT mutate the record, so the caller +/// chooses whether to strip the inline copy based on the returned outcome. +/// +/// The single source of truth for the migrate-vs-keep decision, shared by the +/// load-time opportunistic re-migrate ([`hydrate_keys`]) and the save-time +/// chokepoint ([`persist_agent_keys`]). An empty key is [`KeyMigration::Persisted`] +/// (nothing to keep inline). +fn migrate_inline_key(store: &impl KeyStore, record: &ManagedAgentRecord) -> KeyMigration { + if record.private_key_nsec.is_empty() { + return KeyMigration::Persisted; + } + let name = agent_keyring_name(&record.pubkey); + match store.probe(&name) { + // Keyring down this boot: keep the key inline (file fallback), do NOT + // migrate — re-importing later could resurrect a rotated key. + KeyringProbe::Unreachable => KeyMigration::KeptInline, + KeyringProbe::Present | KeyringProbe::ReachableButEmpty => { + match store.write_and_verify(&name, &record.private_key_nsec) { + Ok(()) => KeyMigration::Persisted, + Err(e) => { + eprintln!( + "buzz-desktop: keyring write for agent {} failed ({e}), keeping inline", + record.pubkey + ); + KeyMigration::KeptInline + } + } + } + } +} + pub fn load_managed_agents(app: &AppHandle) -> Result, String> { let path = managed_agents_store_path(app)?; if !path.exists() { @@ -65,32 +131,44 @@ pub fn load_managed_agents(app: &AppHandle) -> Result, S Ok(records) } -/// Fill in each record's in-memory `private_key_nsec` from the keyring when it -/// was not serialized inline. A record loaded with a non-empty key came from -/// the JSON file-fallback (keyring was unreachable when it was written) — leave -/// it as-is. A record with an empty key has its secret in the keyring. +/// Fill in each record's in-memory `private_key_nsec` from the keyring, and +/// opportunistically re-migrate any key that is still inline. +/// +/// - Empty key → fetch it from the keyring (the normal keyring-backed case). +/// - Non-empty key → the JSON carried it inline because the keyring was +/// unreachable at its last save. Re-migrate it now ([`migrate_inline_key`]): +/// if the keyring is reachable this boot, write-verify-strip so the next save +/// writes clean JSON and plaintext stops lingering on disk; if still +/// unreachable, leave it inline. This makes the strip deterministic on the +/// next reachable boot rather than waiting for a non-deterministic save. fn hydrate_keys(records: &mut [ManagedAgentRecord]) { let Some(store) = agent_secret_store() else { return; }; for record in records.iter_mut() { - if !record.private_key_nsec.is_empty() { - continue; - } - match store.load(&agent_keyring_name(&record.pubkey)) { - Ok(Some(nsec)) => record.private_key_nsec = nsec, - Ok(None) => { - eprintln!( - "buzz-desktop: agent {} has no key in JSON or keyring", - record.pubkey - ); - } - Err(e) => { - eprintln!( - "buzz-desktop: failed to read agent {} key from keyring: {e}", - record.pubkey - ); + if record.private_key_nsec.is_empty() { + match store.load(&agent_keyring_name(&record.pubkey)) { + Ok(Some(nsec)) => record.private_key_nsec = nsec, + Ok(None) => { + eprintln!( + "buzz-desktop: agent {} has no key in JSON or keyring", + record.pubkey + ); + } + Err(e) => { + eprintln!( + "buzz-desktop: failed to read agent {} key from keyring: {e}", + record.pubkey + ); + } } + } else { + // Inline residue from a prior keyring-unreachable save. Lift it + // into the keyring now (side effect) but KEEP it in memory — the + // returned record must carry the key for readers. The next save + // then strips it from JSON. Outcome is intentionally ignored: + // on failure the key simply stays inline until a later boot. + let _ = migrate_inline_key(&store, record); } } } @@ -135,41 +213,17 @@ fn persist_agent_keys(records: &mut [ManagedAgentRecord], any_inline_key: &mut b return; }; for record in records.iter_mut() { - if record.private_key_nsec.is_empty() { - continue; - } - let name = agent_keyring_name(&record.pubkey); - match store.probe(&name) { - KeyringProbe::Unreachable => { - // Keep the key inline (file fallback); do not migrate. - *any_inline_key = true; - } - KeyringProbe::Present | KeyringProbe::ReachableButEmpty => { - match write_and_verify(&store, &name, &record.private_key_nsec) { - Ok(()) => record.private_key_nsec.clear(), - Err(e) => { - eprintln!( - "buzz-desktop: keyring write for agent {} failed ({e}), keeping inline", - record.pubkey - ); - *any_inline_key = true; - } - } - } + match migrate_inline_key(&store, record) { + // Verified in the keyring — drop the inline copy so it stays out of + // JSON. Safe: this is a save-local clone, callers keep their keys. + KeyMigration::Persisted => record.private_key_nsec.clear(), + // Only ever returned for a non-empty key — JSON will carry it, so + // the file must be locked down. + KeyMigration::KeptInline => *any_inline_key = true, } } } -/// Write `value` to the keyring and read it back to confirm before the caller -/// strips the inline copy. -fn write_and_verify(store: &SecretStore, name: &str, value: &str) -> Result<(), String> { - store.store(name, value)?; - match store.load(name)? { - Some(stored) if stored == value => Ok(()), - _ => Err("keyring read-back verify failed".to_string()), - } -} - #[cfg(unix)] fn restrict_json_permissions(path: &Path) { use std::os::unix::fs::PermissionsExt; @@ -349,10 +403,146 @@ pub fn meaningful_agent_error_from_log(path: &Path) -> Option { #[cfg(test)] mod tests { + use std::cell::RefCell; + use std::collections::HashMap; use std::io::Write as _; use tempfile::NamedTempFile; + use super::{ + agent_keyring_name, migrate_inline_key, KeyMigration, KeyStore, KeyringProbe, + ManagedAgentRecord, + }; + + /// In-memory [`KeyStore`] for testing the migrate decision without the OS + /// keyring. `reachable=false` simulates a backend outage; `fail_verify` + /// simulates a write whose read-back does not confirm. + struct FakeKeyStore { + reachable: bool, + fail_verify: bool, + stored: RefCell>, + } + + impl FakeKeyStore { + fn reachable() -> Self { + Self { + reachable: true, + fail_verify: false, + stored: RefCell::new(HashMap::new()), + } + } + fn unreachable() -> Self { + Self { + reachable: false, + fail_verify: false, + stored: RefCell::new(HashMap::new()), + } + } + fn verify_fails() -> Self { + Self { + reachable: true, + fail_verify: true, + stored: RefCell::new(HashMap::new()), + } + } + } + + impl KeyStore for FakeKeyStore { + fn probe(&self, _name: &str) -> KeyringProbe { + if self.reachable { + KeyringProbe::ReachableButEmpty + } else { + KeyringProbe::Unreachable + } + } + fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String> { + if self.fail_verify { + return Err("read-back verify failed".to_string()); + } + self.stored + .borrow_mut() + .insert(name.to_string(), value.to_string()); + Ok(()) + } + } + + fn record_with_key(nsec: &str) -> ManagedAgentRecord { + serde_json::from_str(&format!( + r#"{{ + "pubkey": "agent-pubkey", + "name": "test-agent", + "private_key_nsec": "{nsec}", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }}"# + )) + .expect("sample record") + } + + #[test] + fn migrate_persists_and_signals_stripping_when_keyring_reachable() { + // Item 2: an inline key (residue from a prior keyring-unreachable save) + // is written to the keyring and verified when the backend is reachable, + // so the next save can drop it from JSON. + let store = FakeKeyStore::reachable(); + let record = record_with_key("nsec1realkey"); + + let outcome = migrate_inline_key(&store, &record); + + assert_eq!(outcome, KeyMigration::Persisted); + assert_eq!( + store + .stored + .borrow() + .get(&agent_keyring_name("agent-pubkey")) + .map(String::as_str), + Some("nsec1realkey") + ); + } + + #[test] + fn migrate_keeps_inline_when_keyring_unreachable() { + // No-resurrection guard: a transient outage must NOT migrate; the key + // stays inline (file fallback) so it is not lost. + let store = FakeKeyStore::unreachable(); + let record = record_with_key("nsec1realkey"); + + let outcome = migrate_inline_key(&store, &record); + + assert_eq!(outcome, KeyMigration::KeptInline); + assert!(store.stored.borrow().is_empty()); + } + + #[test] + fn migrate_keeps_inline_when_verify_fails() { + // A write whose read-back does not confirm must keep the key inline — + // never drop plaintext on an unverified write. + let store = FakeKeyStore::verify_fails(); + let record = record_with_key("nsec1realkey"); + + assert_eq!( + migrate_inline_key(&store, &record), + KeyMigration::KeptInline + ); + } + + #[test] + fn migrate_is_noop_for_empty_key() { + // A record whose key already lives in the keyring (empty inline) has + // nothing to migrate. + let store = FakeKeyStore::reachable(); + let record = record_with_key(""); + + assert_eq!(migrate_inline_key(&store, &record), KeyMigration::Persisted); + assert!(store.stored.borrow().is_empty()); + } + fn write_log(content: &str) -> NamedTempFile { let mut file = NamedTempFile::new().expect("temp log"); file.write_all(content.as_bytes()).expect("write log"); From 819d1373e9095862c0179d3002bc4a3b37d78589 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Mon, 22 Jun 2026 13:45:03 -0400 Subject: [PATCH 3/8] refactor(desktop): move types.rs test module to sibling tests.rs The inline test module pushed types.rs to 1075 lines, over the 1000-line file-size limit enforced by check:file-sizes. Extract the #[cfg(test)] mod into a sibling types/tests.rs, matching the existing convention used by env_vars.rs, personas.rs, and runtime.rs in this crate. Production code is unchanged; all 22 type tests still execute by name. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/managed_agents/types.rs | 432 +----------------- .../src/managed_agents/types/tests.rs | 429 +++++++++++++++++ 2 files changed, 430 insertions(+), 431 deletions(-) create mode 100644 desktop/src-tauri/src/managed_agents/types/tests.rs diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 716c347288..7f595bccc0 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -663,434 +663,4 @@ pub fn validate_respond_to_allowlist(input: &[String]) -> Result, St } #[cfg(test)] -mod tests { - use super::{ManagedAgentRecord, PersonaRecord}; - use std::path::PathBuf; - - #[test] - fn persona_record_defaults_active_when_field_is_missing() { - let record: PersonaRecord = serde_json::from_str( - r#"{ - "id": "builtin:fizz", - "display_name": "Fizz", - "avatar_url": null, - "system_prompt": "Prompt", - "created_at": "2026-03-19T00:00:00Z", - "updated_at": "2026-03-19T00:00:00Z" - }"#, - ) - .expect("legacy persona payload should deserialize"); - - assert!(record.is_active); - assert!(!record.is_builtin); - assert_eq!(record.runtime, None); - assert_eq!(record.model, None); - assert!(record.name_pool.is_empty()); - } - - /// Legacy agent records (created before NIP-OA) lack the `auth_tag` field. - /// `#[serde(default)]` must ensure they deserialize with `auth_tag: None`. - #[test] - fn managed_agent_record_without_auth_tag_deserializes() { - let record: ManagedAgentRecord = serde_json::from_str( - r#"{ - "pubkey": "abcd1234", - "name": "test-agent", - "private_key_nsec": "nsec1fake", - "relay_url": "wss://localhost:3000", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": null, - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "last_started_at": null, - "last_stopped_at": null, - "last_exit_code": null, - "last_error": null - }"#, - ) - .expect("legacy agent record without auth_tag should deserialize"); - - assert_eq!(record.auth_tag, None); - assert_eq!(record.avatar_url, None); - assert_eq!(record.pubkey, "abcd1234"); - } - - /// Agent records WITH an auth_tag round-trip correctly through serde. - #[test] - fn managed_agent_record_with_auth_tag_round_trips() { - let json = r#"{ - "pubkey": "abcd1234", - "name": "test-agent", - "private_key_nsec": "nsec1fake", - "auth_tag": "[\"auth\",\"deadbeef\",\"\",\"cafebabe\"]", - "relay_url": "wss://localhost:3000", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": null, - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "last_started_at": null, - "last_stopped_at": null, - "last_exit_code": null, - "last_error": null - }"#; - - let record: ManagedAgentRecord = - serde_json::from_str(json).expect("record with auth_tag should deserialize"); - - assert_eq!( - record.auth_tag.as_deref(), - Some(r#"["auth","deadbeef","","cafebabe"]"#) - ); - - // Round-trip: serialize and deserialize again. - let serialized = serde_json::to_string(&record).expect("should serialize"); - let record2: ManagedAgentRecord = - serde_json::from_str(&serialized).expect("round-trip should deserialize"); - assert_eq!(record.auth_tag, record2.auth_tag); - } - - // ── Inbound author gate tests ──────────────────────────────────────── - - use super::{validate_respond_to_allowlist, RespondTo}; - - #[test] - fn respond_to_default_is_owner_only() { - assert_eq!(RespondTo::default(), RespondTo::OwnerOnly); - } - - #[test] - fn respond_to_serde_is_kebab_case() { - assert_eq!( - serde_json::to_string(&RespondTo::OwnerOnly).unwrap(), - "\"owner-only\"" - ); - assert_eq!( - serde_json::to_string(&RespondTo::Allowlist).unwrap(), - "\"allowlist\"" - ); - assert_eq!( - serde_json::to_string(&RespondTo::Anyone).unwrap(), - "\"anyone\"" - ); - let parsed: RespondTo = serde_json::from_str("\"owner-only\"").unwrap(); - assert_eq!(parsed, RespondTo::OwnerOnly); - let parsed: RespondTo = serde_json::from_str("\"allowlist\"").unwrap(); - assert_eq!(parsed, RespondTo::Allowlist); - let parsed: RespondTo = serde_json::from_str("\"anyone\"").unwrap(); - assert_eq!(parsed, RespondTo::Anyone); - } - - #[test] - fn respond_to_rejects_unknown_modes() { - // `nobody` is a valid harness mode but intentionally not exposed - // through the desktop request types. - assert!(serde_json::from_str::("\"nobody\"").is_err()); - assert!(serde_json::from_str::("\"OwnerOnly\"").is_err()); - } - - /// Records persisted before this feature must continue to load, - /// defaulting to OwnerOnly (the safe, matches-harness-default value). - #[test] - fn managed_agent_record_without_respond_to_fields_defaults_to_owner_only() { - let record: ManagedAgentRecord = serde_json::from_str( - r#"{ - "pubkey": "abcd1234", - "name": "legacy-agent", - "private_key_nsec": "nsec1fake", - "relay_url": "wss://localhost:3000", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": null, - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "last_started_at": null, - "last_stopped_at": null, - "last_exit_code": null, - "last_error": null - }"#, - ) - .expect("legacy record without respond_to fields should deserialize"); - assert_eq!(record.respond_to, RespondTo::OwnerOnly); - assert!(record.respond_to_allowlist.is_empty()); - } - - #[test] - fn validate_respond_to_allowlist_accepts_valid_hex_and_lowercases() { - let upper = "A".repeat(64); - let lower = "a".repeat(64); - let result = validate_respond_to_allowlist(std::slice::from_ref(&upper)).unwrap(); - assert_eq!(result, vec![lower.clone()]); - } - - #[test] - fn validate_respond_to_allowlist_dedups_preserving_order() { - let a = "a".repeat(64); - let b = "b".repeat(64); - let a_upper = "A".repeat(64); - let input = vec![a.clone(), b.clone(), a_upper]; - let result = validate_respond_to_allowlist(&input).unwrap(); - assert_eq!(result, vec![a, b]); - } - - #[test] - fn validate_respond_to_allowlist_rejects_wrong_length() { - let too_short = "a".repeat(63); - assert!(validate_respond_to_allowlist(&[too_short]).is_err()); - let too_long = "a".repeat(65); - assert!(validate_respond_to_allowlist(&[too_long]).is_err()); - } - - #[test] - fn validate_respond_to_allowlist_rejects_non_hex() { - let bad = "z".repeat(64); - assert!(validate_respond_to_allowlist(&[bad]).is_err()); - // npub-style strings should not slip through. - let npub = format!("npub1{}", "a".repeat(59)); - assert!(validate_respond_to_allowlist(&[npub]).is_err()); - } - - #[test] - fn validate_respond_to_allowlist_trims_whitespace() { - let padded = format!(" {} ", "a".repeat(64)); - let result = validate_respond_to_allowlist(&[padded]).unwrap(); - assert_eq!(result, vec!["a".repeat(64)]); - } - - #[test] - fn validate_respond_to_allowlist_accepts_empty() { - // Empty is allowed at this layer; the boundary check - // (Allowlist mode requires ≥1 entry) is the caller's job. - let result = validate_respond_to_allowlist(&[]).unwrap(); - assert!(result.is_empty()); - } - - use super::{CreateManagedAgentRequest, RelayMeshConfig}; - - /// Wire-shape test: the create request arrives from TS as camelCase - /// (`relayMesh: { modelRef }`). `rename_all = "camelCase"` on - /// `CreateManagedAgentRequest` does NOT recurse into nested structs, so - /// `RelayMeshConfig` needs its own `alias = "modelRef"`. This test pins - /// the exact JSON the frontend sends; if the alias is dropped, creating - /// a relay-mesh agent fails to deserialize at the Tauri boundary. - #[test] - fn create_request_deserializes_camel_case_relay_mesh() { - let request: CreateManagedAgentRequest = serde_json::from_str( - r#"{ - "name": "mesh-agent", - "relayMesh": { "modelRef": "Qwen3" } - }"#, - ) - .expect("camelCase relayMesh payload from TS should deserialize"); - assert_eq!( - request.relay_mesh, - Some(RelayMeshConfig { - model_ref: "Qwen3".to_string() - }) - ); - } - - /// Persisted records use snake_case; the camelCase alias must not break - /// the stored-record round trip. - #[test] - fn relay_mesh_config_round_trips_snake_case() { - let config = RelayMeshConfig { - model_ref: "Qwen3".to_string(), - }; - let json = serde_json::to_string(&config).unwrap(); - assert_eq!(json, r#"{"model_ref":"Qwen3"}"#); - let back: RelayMeshConfig = serde_json::from_str(&json).unwrap(); - assert_eq!(back, config); - } - - // ── Packs → Teams serde alias backward compatibility ──────────────── - - #[test] - fn persona_record_deserializes_old_source_pack_fields_via_alias() { - let record: PersonaRecord = serde_json::from_str( - r#"{ - "id": "persona-1", - "display_name": "Test", - "avatar_url": null, - "system_prompt": "Prompt", - "source_pack": "com.example.my-pack", - "source_pack_persona_slug": "agent-one", - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z" - }"#, - ) - .expect("old-format persona with source_pack should deserialize via alias"); - - assert_eq!(record.source_team.as_deref(), Some("com.example.my-pack")); - assert_eq!( - record.source_team_persona_slug.as_deref(), - Some("agent-one") - ); - } - - #[test] - fn persona_record_serializes_new_field_names() { - let record: PersonaRecord = serde_json::from_str( - r#"{ - "id": "persona-1", - "display_name": "Test", - "avatar_url": null, - "system_prompt": "Prompt", - "source_team": "com.example.my-team", - "source_team_persona_slug": "agent-one", - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z" - }"#, - ) - .unwrap(); - - let json = serde_json::to_string(&record).unwrap(); - assert!(json.contains("source_team")); - assert!(json.contains("source_team_persona_slug")); - assert!(!json.contains("source_pack")); - } - - #[test] - fn managed_agent_record_deserializes_old_pack_path_fields_via_alias() { - let record: ManagedAgentRecord = serde_json::from_str( - r#"{ - "pubkey": "abcd1234", - "name": "test-agent", - "private_key_nsec": "nsec1fake", - "relay_url": "wss://localhost:3000", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": null, - "persona_pack_path": "/path/to/agents/packs/my-pack", - "persona_name_in_pack": "agent-one", - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "last_started_at": null, - "last_stopped_at": null, - "last_exit_code": null, - "last_error": null - }"#, - ) - .expect("old-format agent with persona_pack_path should deserialize via alias"); - - assert_eq!( - record.persona_team_dir, - Some(PathBuf::from("/path/to/agents/packs/my-pack")) - ); - assert_eq!(record.persona_name_in_team.as_deref(), Some("agent-one")); - } - - #[test] - fn team_record_deserializes_without_new_fields() { - let record: super::TeamRecord = serde_json::from_str( - r#"{ - "id": "team-1", - "name": "My Team", - "description": null, - "persona_ids": ["p1", "p2"], - "is_builtin": false, - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z" - }"#, - ) - .expect("team record without new fields should deserialize with defaults"); - - assert_eq!(record.source_dir, None); - assert!(!record.is_symlink); - assert_eq!(record.symlink_target, None); - assert_eq!(record.version, None); - } - - /// A record whose in-memory key was blanked (because it lives in the - /// keyring) must NOT serialize `private_key_nsec` into JSON. - #[test] - fn managed_agent_record_omits_empty_key_from_json() { - let mut record = sample_agent_record(); - record.private_key_nsec = String::new(); - - let json = serde_json::to_string(&record).expect("serialize"); - assert!( - !json.contains("private_key_nsec"), - "blanked key must be skipped from JSON, got: {json}" - ); - } - - /// A record with an inline key (the keyringless `0o600` JSON fallback) - /// serializes the key and round-trips it back. - #[test] - fn managed_agent_record_serializes_inline_key_for_fallback() { - let mut record = sample_agent_record(); - record.private_key_nsec = "nsec1fallback".to_string(); - - let json = serde_json::to_string(&record).expect("serialize"); - assert!(json.contains("nsec1fallback")); - - let back: ManagedAgentRecord = serde_json::from_str(&json).expect("deserialize"); - assert_eq!(back.private_key_nsec, "nsec1fallback"); - } - - /// A keyring-backed record on disk lacks `private_key_nsec`; it must - /// deserialize with an empty key (to be hydrated from the keyring). - #[test] - fn managed_agent_record_without_key_deserializes_empty() { - let record: ManagedAgentRecord = serde_json::from_str( - r#"{ - "pubkey": "abcd1234", - "name": "test-agent", - "relay_url": "wss://localhost:3000", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": null, - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "last_started_at": null, - "last_stopped_at": null, - "last_exit_code": null, - "last_error": null - }"#, - ) - .expect("keyring-backed record without inline key should deserialize"); - - assert_eq!(record.private_key_nsec, ""); - } - - fn sample_agent_record() -> ManagedAgentRecord { - serde_json::from_str( - r#"{ - "pubkey": "abcd1234", - "name": "test-agent", - "private_key_nsec": "nsec1fake", - "relay_url": "wss://localhost:3000", - "acp_command": "buzz-acp", - "agent_command": "goose", - "agent_args": [], - "mcp_command": "", - "turn_timeout_seconds": 320, - "system_prompt": null, - "created_at": "2026-01-01T00:00:00Z", - "updated_at": "2026-01-01T00:00:00Z", - "last_started_at": null, - "last_stopped_at": null, - "last_exit_code": null, - "last_error": null - }"#, - ) - .expect("sample record") - } -} +mod tests; diff --git a/desktop/src-tauri/src/managed_agents/types/tests.rs b/desktop/src-tauri/src/managed_agents/types/tests.rs new file mode 100644 index 0000000000..b7a725762c --- /dev/null +++ b/desktop/src-tauri/src/managed_agents/types/tests.rs @@ -0,0 +1,429 @@ +use super::{ManagedAgentRecord, PersonaRecord}; +use std::path::PathBuf; + +#[test] +fn persona_record_defaults_active_when_field_is_missing() { + let record: PersonaRecord = serde_json::from_str( + r#"{ + "id": "builtin:fizz", + "display_name": "Fizz", + "avatar_url": null, + "system_prompt": "Prompt", + "created_at": "2026-03-19T00:00:00Z", + "updated_at": "2026-03-19T00:00:00Z" + }"#, + ) + .expect("legacy persona payload should deserialize"); + + assert!(record.is_active); + assert!(!record.is_builtin); + assert_eq!(record.runtime, None); + assert_eq!(record.model, None); + assert!(record.name_pool.is_empty()); +} + +/// Legacy agent records (created before NIP-OA) lack the `auth_tag` field. +/// `#[serde(default)]` must ensure they deserialize with `auth_tag: None`. +#[test] +fn managed_agent_record_without_auth_tag_deserializes() { + let record: ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("legacy agent record without auth_tag should deserialize"); + + assert_eq!(record.auth_tag, None); + assert_eq!(record.avatar_url, None); + assert_eq!(record.pubkey, "abcd1234"); +} + +/// Agent records WITH an auth_tag round-trip correctly through serde. +#[test] +fn managed_agent_record_with_auth_tag_round_trips() { + let json = r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "private_key_nsec": "nsec1fake", + "auth_tag": "[\"auth\",\"deadbeef\",\"\",\"cafebabe\"]", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#; + + let record: ManagedAgentRecord = + serde_json::from_str(json).expect("record with auth_tag should deserialize"); + + assert_eq!( + record.auth_tag.as_deref(), + Some(r#"["auth","deadbeef","","cafebabe"]"#) + ); + + // Round-trip: serialize and deserialize again. + let serialized = serde_json::to_string(&record).expect("should serialize"); + let record2: ManagedAgentRecord = + serde_json::from_str(&serialized).expect("round-trip should deserialize"); + assert_eq!(record.auth_tag, record2.auth_tag); +} + +// ── Inbound author gate tests ──────────────────────────────────────── + +use super::{validate_respond_to_allowlist, RespondTo}; + +#[test] +fn respond_to_default_is_owner_only() { + assert_eq!(RespondTo::default(), RespondTo::OwnerOnly); +} + +#[test] +fn respond_to_serde_is_kebab_case() { + assert_eq!( + serde_json::to_string(&RespondTo::OwnerOnly).unwrap(), + "\"owner-only\"" + ); + assert_eq!( + serde_json::to_string(&RespondTo::Allowlist).unwrap(), + "\"allowlist\"" + ); + assert_eq!( + serde_json::to_string(&RespondTo::Anyone).unwrap(), + "\"anyone\"" + ); + let parsed: RespondTo = serde_json::from_str("\"owner-only\"").unwrap(); + assert_eq!(parsed, RespondTo::OwnerOnly); + let parsed: RespondTo = serde_json::from_str("\"allowlist\"").unwrap(); + assert_eq!(parsed, RespondTo::Allowlist); + let parsed: RespondTo = serde_json::from_str("\"anyone\"").unwrap(); + assert_eq!(parsed, RespondTo::Anyone); +} + +#[test] +fn respond_to_rejects_unknown_modes() { + // `nobody` is a valid harness mode but intentionally not exposed + // through the desktop request types. + assert!(serde_json::from_str::("\"nobody\"").is_err()); + assert!(serde_json::from_str::("\"OwnerOnly\"").is_err()); +} + +/// Records persisted before this feature must continue to load, +/// defaulting to OwnerOnly (the safe, matches-harness-default value). +#[test] +fn managed_agent_record_without_respond_to_fields_defaults_to_owner_only() { + let record: ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "legacy-agent", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("legacy record without respond_to fields should deserialize"); + assert_eq!(record.respond_to, RespondTo::OwnerOnly); + assert!(record.respond_to_allowlist.is_empty()); +} + +#[test] +fn validate_respond_to_allowlist_accepts_valid_hex_and_lowercases() { + let upper = "A".repeat(64); + let lower = "a".repeat(64); + let result = validate_respond_to_allowlist(std::slice::from_ref(&upper)).unwrap(); + assert_eq!(result, vec![lower.clone()]); +} + +#[test] +fn validate_respond_to_allowlist_dedups_preserving_order() { + let a = "a".repeat(64); + let b = "b".repeat(64); + let a_upper = "A".repeat(64); + let input = vec![a.clone(), b.clone(), a_upper]; + let result = validate_respond_to_allowlist(&input).unwrap(); + assert_eq!(result, vec![a, b]); +} + +#[test] +fn validate_respond_to_allowlist_rejects_wrong_length() { + let too_short = "a".repeat(63); + assert!(validate_respond_to_allowlist(&[too_short]).is_err()); + let too_long = "a".repeat(65); + assert!(validate_respond_to_allowlist(&[too_long]).is_err()); +} + +#[test] +fn validate_respond_to_allowlist_rejects_non_hex() { + let bad = "z".repeat(64); + assert!(validate_respond_to_allowlist(&[bad]).is_err()); + // npub-style strings should not slip through. + let npub = format!("npub1{}", "a".repeat(59)); + assert!(validate_respond_to_allowlist(&[npub]).is_err()); +} + +#[test] +fn validate_respond_to_allowlist_trims_whitespace() { + let padded = format!(" {} ", "a".repeat(64)); + let result = validate_respond_to_allowlist(&[padded]).unwrap(); + assert_eq!(result, vec!["a".repeat(64)]); +} + +#[test] +fn validate_respond_to_allowlist_accepts_empty() { + // Empty is allowed at this layer; the boundary check + // (Allowlist mode requires ≥1 entry) is the caller's job. + let result = validate_respond_to_allowlist(&[]).unwrap(); + assert!(result.is_empty()); +} + +use super::{CreateManagedAgentRequest, RelayMeshConfig}; + +/// Wire-shape test: the create request arrives from TS as camelCase +/// (`relayMesh: { modelRef }`). `rename_all = "camelCase"` on +/// `CreateManagedAgentRequest` does NOT recurse into nested structs, so +/// `RelayMeshConfig` needs its own `alias = "modelRef"`. This test pins +/// the exact JSON the frontend sends; if the alias is dropped, creating +/// a relay-mesh agent fails to deserialize at the Tauri boundary. +#[test] +fn create_request_deserializes_camel_case_relay_mesh() { + let request: CreateManagedAgentRequest = serde_json::from_str( + r#"{ + "name": "mesh-agent", + "relayMesh": { "modelRef": "Qwen3" } + }"#, + ) + .expect("camelCase relayMesh payload from TS should deserialize"); + assert_eq!( + request.relay_mesh, + Some(RelayMeshConfig { + model_ref: "Qwen3".to_string() + }) + ); +} + +/// Persisted records use snake_case; the camelCase alias must not break +/// the stored-record round trip. +#[test] +fn relay_mesh_config_round_trips_snake_case() { + let config = RelayMeshConfig { + model_ref: "Qwen3".to_string(), + }; + let json = serde_json::to_string(&config).unwrap(); + assert_eq!(json, r#"{"model_ref":"Qwen3"}"#); + let back: RelayMeshConfig = serde_json::from_str(&json).unwrap(); + assert_eq!(back, config); +} + +// ── Packs → Teams serde alias backward compatibility ──────────────── + +#[test] +fn persona_record_deserializes_old_source_pack_fields_via_alias() { + let record: PersonaRecord = serde_json::from_str( + r#"{ + "id": "persona-1", + "display_name": "Test", + "avatar_url": null, + "system_prompt": "Prompt", + "source_pack": "com.example.my-pack", + "source_pack_persona_slug": "agent-one", + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }"#, + ) + .expect("old-format persona with source_pack should deserialize via alias"); + + assert_eq!(record.source_team.as_deref(), Some("com.example.my-pack")); + assert_eq!( + record.source_team_persona_slug.as_deref(), + Some("agent-one") + ); +} + +#[test] +fn persona_record_serializes_new_field_names() { + let record: PersonaRecord = serde_json::from_str( + r#"{ + "id": "persona-1", + "display_name": "Test", + "avatar_url": null, + "system_prompt": "Prompt", + "source_team": "com.example.my-team", + "source_team_persona_slug": "agent-one", + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }"#, + ) + .unwrap(); + + let json = serde_json::to_string(&record).unwrap(); + assert!(json.contains("source_team")); + assert!(json.contains("source_team_persona_slug")); + assert!(!json.contains("source_pack")); +} + +#[test] +fn managed_agent_record_deserializes_old_pack_path_fields_via_alias() { + let record: ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "persona_pack_path": "/path/to/agents/packs/my-pack", + "persona_name_in_pack": "agent-one", + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("old-format agent with persona_pack_path should deserialize via alias"); + + assert_eq!( + record.persona_team_dir, + Some(PathBuf::from("/path/to/agents/packs/my-pack")) + ); + assert_eq!(record.persona_name_in_team.as_deref(), Some("agent-one")); +} + +#[test] +fn team_record_deserializes_without_new_fields() { + let record: super::TeamRecord = serde_json::from_str( + r#"{ + "id": "team-1", + "name": "My Team", + "description": null, + "persona_ids": ["p1", "p2"], + "is_builtin": false, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z" + }"#, + ) + .expect("team record without new fields should deserialize with defaults"); + + assert_eq!(record.source_dir, None); + assert!(!record.is_symlink); + assert_eq!(record.symlink_target, None); + assert_eq!(record.version, None); +} + +/// A record whose in-memory key was blanked (because it lives in the +/// keyring) must NOT serialize `private_key_nsec` into JSON. +#[test] +fn managed_agent_record_omits_empty_key_from_json() { + let mut record = sample_agent_record(); + record.private_key_nsec = String::new(); + + let json = serde_json::to_string(&record).expect("serialize"); + assert!( + !json.contains("private_key_nsec"), + "blanked key must be skipped from JSON, got: {json}" + ); +} + +/// A record with an inline key (the keyringless `0o600` JSON fallback) +/// serializes the key and round-trips it back. +#[test] +fn managed_agent_record_serializes_inline_key_for_fallback() { + let mut record = sample_agent_record(); + record.private_key_nsec = "nsec1fallback".to_string(); + + let json = serde_json::to_string(&record).expect("serialize"); + assert!(json.contains("nsec1fallback")); + + let back: ManagedAgentRecord = serde_json::from_str(&json).expect("deserialize"); + assert_eq!(back.private_key_nsec, "nsec1fallback"); +} + +/// A keyring-backed record on disk lacks `private_key_nsec`; it must +/// deserialize with an empty key (to be hydrated from the keyring). +#[test] +fn managed_agent_record_without_key_deserializes_empty() { + let record: ManagedAgentRecord = serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("keyring-backed record without inline key should deserialize"); + + assert_eq!(record.private_key_nsec, ""); +} + +fn sample_agent_record() -> ManagedAgentRecord { + serde_json::from_str( + r#"{ + "pubkey": "abcd1234", + "name": "test-agent", + "private_key_nsec": "nsec1fake", + "relay_url": "wss://localhost:3000", + "acp_command": "buzz-acp", + "agent_command": "goose", + "agent_args": [], + "mcp_command": "", + "turn_timeout_seconds": 320, + "system_prompt": null, + "created_at": "2026-01-01T00:00:00Z", + "updated_at": "2026-01-01T00:00:00Z", + "last_started_at": null, + "last_stopped_at": null, + "last_exit_code": null, + "last_error": null + }"#, + ) + .expect("sample record") +} From 406312d8eee0ae9c3784b71523e6a044a6f4bea9 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Mon, 22 Jun 2026 14:46:46 -0400 Subject: [PATCH 4/8] fix(desktop): recover from a corrupt keyring nsec without rotating identity The keyring-Present-but-corrupt path quarantined identity.key (the file) and generated a fresh identity, but the corruption is in the keyring, not the file. A valid leftover identity.key (from a prior migration whose remove_file failed) would be destroyed and the user silently rotated to a new key. Now clear the corrupt keyring value, migrate a valid file if one exists, and generate fresh only as a last resort. Adds an IdentityKeyStore seam (mirroring managed_agents::storage's KeyStore) so the recovery decision is unit-tested without a live keyring. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/app_state.rs | 252 ++++++++++++++++++++++++----- 1 file changed, 212 insertions(+), 40 deletions(-) diff --git a/desktop/src-tauri/src/app_state.rs b/desktop/src-tauri/src/app_state.rs index c13591b522..411f40df44 100644 --- a/desktop/src-tauri/src/app_state.rs +++ b/desktop/src-tauri/src/app_state.rs @@ -168,6 +168,31 @@ pub(crate) const KEYRING_SERVICE: &str = "buzz-desktop"; /// Keyring key name for the human identity nsec. const IDENTITY_KEY_NAME: &str = "identity"; +/// The keyring operations the identity resolution flow needs. Abstracted so the +/// corrupt-keyring recovery decision ([`recover_from_keyring`]) can be +/// unit-tested against a fake without touching the live OS keyring. +trait IdentityKeyStore { + fn probe(&self, name: &str) -> crate::secret_store::KeyringProbe; + fn load(&self, name: &str) -> Result, String>; + fn store(&self, name: &str, value: &str) -> Result<(), String>; + fn delete(&self, name: &str) -> Result<(), String>; +} + +impl IdentityKeyStore for crate::secret_store::SecretStore { + fn probe(&self, name: &str) -> crate::secret_store::KeyringProbe { + crate::secret_store::SecretStore::probe(self, name) + } + fn load(&self, name: &str) -> Result, String> { + crate::secret_store::SecretStore::load(self, name) + } + fn store(&self, name: &str, value: &str) -> Result<(), String> { + crate::secret_store::SecretStore::store(self, name, value) + } + fn delete(&self, name: &str) -> Result<(), String> { + crate::secret_store::SecretStore::delete(self, name) + } +} + /// Resolve the human identity key: migrate a legacy `identity.key` into the /// keyring when safe, otherwise load from whichever backend holds it, else /// generate-and-save. @@ -178,8 +203,6 @@ const IDENTITY_KEY_NAME: &str = "identity"; /// later import from a leftover (possibly rotated) file could resurrect an old /// key. fn load_or_create_identity(data_dir: &std::path::Path) -> Result { - use crate::secret_store::KeyringProbe; - let legacy_path = data_dir.join("identity.key"); // No keyring available in this build: the `0o600` file is the only store. @@ -188,17 +211,44 @@ fn load_or_create_identity(data_dir: &std::path::Path) -> Result { } let store = crate::secret_store::SecretStore::keyring(KEYRING_SERVICE); + resolve_identity_with_store(&store, &legacy_path, data_dir) +} + +/// Identity resolution over an [`IdentityKeyStore`] seam. Split from +/// [`load_or_create_identity`] so the probe/recover branches are testable +/// without the live OS keyring. +fn resolve_identity_with_store( + store: &impl IdentityKeyStore, + legacy_path: &std::path::Path, + data_dir: &std::path::Path, +) -> Result { + use crate::secret_store::KeyringProbe; match store.probe(IDENTITY_KEY_NAME) { KeyringProbe::Present => { if let Some(nsec) = store.load(IDENTITY_KEY_NAME)? { - let keys = parse_or_quarantine(&nsec, &legacy_path, data_dir)?; - // The key is authoritative in the keyring. A leftover - // `identity.key` means a prior migration's `remove_file` failed - // (transient AV lock, read-only mount, EPERM) and never retried - // — clean it up now so plaintext does not linger on disk. - cleanup_leftover_identity_file(&legacy_path); - return Ok(keys); + match Keys::parse(nsec.trim()) { + Ok(keys) => { + eprintln!( + "buzz-desktop: persisted identity pubkey {}", + keys.public_key().to_hex() + ); + // The key is authoritative in the keyring. A leftover + // `identity.key` means a prior migration's `remove_file` + // failed (transient AV lock, read-only mount, EPERM) and + // never retried — clean it up now so plaintext does not + // linger on disk. + cleanup_leftover_identity_file(legacy_path); + return Ok(keys); + } + // The corruption is in the KEYRING, not the file. Clear the + // bad keyring value and recover from the file (or generate + // fresh) — do NOT quarantine a valid leftover `identity.key` + // that holds the user's only good key. + Err(error) => { + return recover_from_keyring(store, legacy_path, &error.to_string()); + } + } } // Probe said Present but load found nothing — treat as empty. } @@ -206,18 +256,39 @@ fn load_or_create_identity(data_dir: &std::path::Path) -> Result { // One-time migration: import the legacy plaintext file, read-back // verify, THEN delete it. if legacy_path.exists() { - if let Some(keys) = migrate_identity_file(&store, &legacy_path)? { + if let Some(keys) = migrate_identity_file(store, legacy_path)? { return Ok(keys); } } } KeyringProbe::Unreachable => { // Keyring down this boot — read the file directly, do NOT migrate. - return load_file_or_generate(&legacy_path, data_dir); + return load_file_or_generate(legacy_path, data_dir); } } - generate_and_persist(&store, &legacy_path) + generate_and_persist(store, legacy_path) +} + +/// Recover from a corrupt nsec in the keyring (parse failed). Clear the bad +/// keyring value, then migrate a valid leftover `identity.key` if one exists, +/// generating fresh only as a last resort. The keyring delete is best-effort: +/// a delete failure logs and continues — it must never block startup. +fn recover_from_keyring( + store: &impl IdentityKeyStore, + legacy_path: &std::path::Path, + error: &str, +) -> Result { + eprintln!("buzz-desktop: corrupt nsec in keyring ({error}), clearing and recovering from file"); + if let Err(e) = store.delete(IDENTITY_KEY_NAME) { + eprintln!("buzz-desktop: failed to clear corrupt keyring value: {e}"); + } + if legacy_path.exists() { + if let Some(keys) = migrate_identity_file(store, legacy_path)? { + return Ok(keys); + } + } + generate_and_persist(store, legacy_path) } /// Load the `0o600` identity file, quarantining corruption, else generate and @@ -251,7 +322,7 @@ fn load_file_or_generate( /// then delete the file. Returns `Ok(None)` if the file was corrupt (caller /// continues to generate-and-save). fn migrate_identity_file( - store: &crate::secret_store::SecretStore, + store: &impl IdentityKeyStore, legacy_path: &std::path::Path, ) -> Result, String> { let keys = match load_key_file(legacy_path) { @@ -283,7 +354,7 @@ fn migrate_identity_file( /// Generate a fresh identity, persist it through the store, return it. fn generate_and_persist( - store: &crate::secret_store::SecretStore, + store: &impl IdentityKeyStore, legacy_path: &std::path::Path, ) -> Result { let keys = Keys::generate(); @@ -298,7 +369,7 @@ fn generate_and_persist( /// Persist `keys` through the store, falling back to the `0o600` file when the /// keyring write fails on an availability error. fn persist_identity( - store: &crate::secret_store::SecretStore, + store: &impl IdentityKeyStore, keys: &Keys, legacy_path: &std::path::Path, ) -> Result<(), String> { @@ -315,31 +386,6 @@ fn persist_identity( } } -fn parse_or_quarantine( - nsec: &str, - legacy_path: &std::path::Path, - data_dir: &std::path::Path, -) -> Result { - match Keys::parse(nsec.trim()) { - Ok(keys) => { - eprintln!( - "buzz-desktop: persisted identity pubkey {}", - keys.public_key().to_hex() - ); - Ok(keys) - } - Err(error) => { - quarantine_corrupt_key( - legacy_path, - data_dir, - &format!("parse keyring nsec: {error}"), - ); - let store = crate::secret_store::SecretStore::keyring(KEYRING_SERVICE); - generate_and_persist(&store, legacy_path) - } - } -} - /// Best-effort removal of a leftover `identity.key` once the keyring is the /// authoritative store. Idempotent: a missing file is success. Logs but does /// not error on failure — a delete failure must never block startup. @@ -572,4 +618,130 @@ mod tests { let loaded = load_key_file(&path).unwrap(); assert_key_eq(&keys2, &loaded); } + + use std::cell::RefCell; + use std::collections::HashMap; + + use crate::secret_store::KeyringProbe; + + /// In-memory [`IdentityKeyStore`] for testing identity recovery without the + /// OS keyring. Seeded with an initial value and a probe outcome; records + /// every `delete`/`store` so tests can assert the keyring was cleared and + /// rewritten. `write_and_verify` succeeds (store then load reflects it). + struct FakeIdentityStore { + probe: KeyringProbe, + slot: RefCell>, + deleted: RefCell>, + } + + impl FakeIdentityStore { + fn present_with(value: &str) -> Self { + let mut slot = HashMap::new(); + slot.insert(IDENTITY_KEY_NAME.to_string(), value.to_string()); + Self { + probe: KeyringProbe::Present, + slot: RefCell::new(slot), + deleted: RefCell::new(Vec::new()), + } + } + } + + impl IdentityKeyStore for FakeIdentityStore { + fn probe(&self, _name: &str) -> KeyringProbe { + self.probe + } + fn load(&self, name: &str) -> Result, String> { + Ok(self.slot.borrow().get(name).cloned()) + } + fn store(&self, name: &str, value: &str) -> Result<(), String> { + self.slot + .borrow_mut() + .insert(name.to_string(), value.to_string()); + Ok(()) + } + fn delete(&self, name: &str) -> Result<(), String> { + self.deleted.borrow_mut().push(name.to_string()); + self.slot.borrow_mut().remove(name); + Ok(()) + } + } + + #[test] + fn corrupt_keyring_recovers_valid_file_without_rotating() { + // The load-bearing regression guard. When the keyring holds a corrupt + // nsec (Present) AND a valid `identity.key` is on disk (leftover from a + // failed prior migration), recovery must RECOVER THE FILE'S identity — + // not quarantine the file and rotate to a fresh key (the original + // hazard). The corrupt keyring value must be cleared and replaced by the + // file's key (migrated in). + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + let file_keys = Keys::generate(); + save_key_file(&legacy_path, &file_keys).unwrap(); + + let store = FakeIdentityStore::present_with("not-a-valid-nsec"); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + // The FILE's identity is recovered — NOT a freshly generated one. + assert_key_eq(&file_keys, &resolved); + // The corrupt keyring value was cleared. + assert_eq!(store.deleted.borrow().as_slice(), [IDENTITY_KEY_NAME]); + // The keyring now holds the file's key (migrated in, read-back verified). + let file_nsec = file_keys.secret_key().to_bech32().unwrap(); + assert_eq!( + store + .slot + .borrow() + .get(IDENTITY_KEY_NAME) + .map(String::as_str), + Some(file_nsec.as_str()) + ); + // The valid file was migrated (deleted), not quarantined to .bad.*. + assert!(!legacy_path.exists()); + assert!(std::fs::read_dir(dir.path()).unwrap().all(|e| !e + .unwrap() + .file_name() + .to_string_lossy() + .contains(".bad."))); + } + + #[test] + fn corrupt_keyring_generates_fresh_only_when_no_file() { + // With a corrupt keyring value and NO file on disk, generate-fresh is + // the correct last resort — and the corrupt keyring value is cleared + // first. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + assert!(!legacy_path.exists()); + + let store = FakeIdentityStore::present_with("not-a-valid-nsec"); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + assert_eq!(store.deleted.borrow().as_slice(), [IDENTITY_KEY_NAME]); + // A fresh, valid key was persisted to the keyring (replacing the cleared + // corrupt value). + let stored = store.slot.borrow().get(IDENTITY_KEY_NAME).cloned(); + assert_eq!( + stored.as_deref(), + Some(resolved.secret_key().to_bech32().unwrap().as_str()) + ); + } + + #[test] + fn valid_keyring_is_used_and_leftover_file_cleaned_up() { + // The happy path is unchanged: a valid keyring value is used as-is, and + // a leftover plaintext file is cleaned up (keyring is authoritative). + let keyring_keys = Keys::generate(); + let nsec = keyring_keys.secret_key().to_bech32().unwrap(); + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + save_key_file(&legacy_path, &Keys::generate()).unwrap(); + + let store = FakeIdentityStore::present_with(&nsec); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + assert_key_eq(&keyring_keys, &resolved); + assert!(store.deleted.borrow().is_empty()); + assert!(!legacy_path.exists()); + } } From e6f865be15b223f52d3058a3b6167891666f1e90 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Mon, 22 Jun 2026 16:03:20 -0400 Subject: [PATCH 5/8] fix(desktop): fail closed on keyring outage instead of rotating identity A keyring outage after migration was indistinguishable from a fresh install (both probe Unreachable with no identity file), so the human identity could be silently regenerated, and an agent whose key failed to load was spawned with an empty BUZZ_PRIVATE_KEY/NOSTR_PRIVATE_KEY. Identity: a migration-completed marker, written and fsynced before the legacy file is deleted, is the durable signal that a key lives in the keyring. Unreachable + no file + marker now fails closed rather than generating; first-ever launch (no marker) still generates to the 0o600 file. Agent keys: a keyring LOAD error is treated as an outage (key left empty), distinct from a genuinely absent entry, and an empty key now reports KeyMigration::Nothing rather than Persisted so it is never mistaken for a verified entry. The spawn path refuses to start an agent whose key is unavailable. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/src-tauri/src/app_state.rs | 154 +++++++++++++++++- .../src-tauri/src/managed_agents/runtime.rs | 5 +- .../src-tauri/src/managed_agents/storage.rs | 132 +++++++++++++-- 3 files changed, 276 insertions(+), 15 deletions(-) diff --git a/desktop/src-tauri/src/app_state.rs b/desktop/src-tauri/src/app_state.rs index 411f40df44..a9a11e3de4 100644 --- a/desktop/src-tauri/src/app_state.rs +++ b/desktop/src-tauri/src/app_state.rs @@ -168,6 +168,13 @@ pub(crate) const KEYRING_SERVICE: &str = "buzz-desktop"; /// Keyring key name for the human identity nsec. const IDENTITY_KEY_NAME: &str = "identity"; +/// Filename of the marker written once a successful keyring migration deletes +/// the legacy `identity.key`. Its presence is the only durable signal that a +/// key once lived in the keyring — used to tell a genuine first-ever launch +/// (no key anywhere, generating is correct) from a post-migration boot whose +/// keyring is merely unreachable (the key IS in the keyring, must NOT generate). +const MIGRATION_MARKER_NAME: &str = "identity.migrated"; + /// The keyring operations the identity resolution flow needs. Abstracted so the /// corrupt-keyring recovery decision ([`recover_from_keyring`]) can be /// unit-tested against a fake without touching the live OS keyring. @@ -246,7 +253,12 @@ fn resolve_identity_with_store( // fresh) — do NOT quarantine a valid leftover `identity.key` // that holds the user's only good key. Err(error) => { - return recover_from_keyring(store, legacy_path, &error.to_string()); + return recover_from_keyring( + store, + legacy_path, + data_dir, + &error.to_string(), + ); } } } @@ -256,13 +268,28 @@ fn resolve_identity_with_store( // One-time migration: import the legacy plaintext file, read-back // verify, THEN delete it. if legacy_path.exists() { - if let Some(keys) = migrate_identity_file(store, legacy_path)? { + if let Some(keys) = migrate_identity_file(store, legacy_path, data_dir)? { return Ok(keys); } } } KeyringProbe::Unreachable => { - // Keyring down this boot — read the file directly, do NOT migrate. + // Keyring down this boot. If a recoverable file is present, use it + // (and do NOT migrate — re-importing later could resurrect a + // rotated key). With NO file, the marker disambiguates two states + // that are otherwise byte-identical (Unreachable + no file): + // - marker present → the key was migrated into the keyring and the + // file deleted. The real key is unreachable, not gone. Fail + // CLOSED — generating here would silently rotate the identity. + // - no marker → genuine first-ever launch with nothing to protect. + // Generate to the `0o600` file (legitimate first-run). + if !legacy_path.exists() && migration_marker_path(data_dir).exists() { + return Err( + "identity key is in the OS keyring but the keyring is unavailable this boot; \ + retry once the keyring (Keychain / Credential Manager / Secret Service) is reachable" + .to_string(), + ); + } return load_file_or_generate(legacy_path, data_dir); } } @@ -277,6 +304,7 @@ fn resolve_identity_with_store( fn recover_from_keyring( store: &impl IdentityKeyStore, legacy_path: &std::path::Path, + data_dir: &std::path::Path, error: &str, ) -> Result { eprintln!("buzz-desktop: corrupt nsec in keyring ({error}), clearing and recovering from file"); @@ -284,7 +312,7 @@ fn recover_from_keyring( eprintln!("buzz-desktop: failed to clear corrupt keyring value: {e}"); } if legacy_path.exists() { - if let Some(keys) = migrate_identity_file(store, legacy_path)? { + if let Some(keys) = migrate_identity_file(store, legacy_path, data_dir)? { return Ok(keys); } } @@ -324,6 +352,7 @@ fn load_file_or_generate( fn migrate_identity_file( store: &impl IdentityKeyStore, legacy_path: &std::path::Path, + data_dir: &std::path::Path, ) -> Result, String> { let keys = match load_key_file(legacy_path) { Ok(keys) => keys, @@ -341,6 +370,20 @@ fn migrate_identity_file( // Read-back verify before deleting the plaintext file. match store.load(IDENTITY_KEY_NAME)? { Some(stored) if stored == nsec => { + // Crash-safe ordering: record that the key now lives in the keyring + // (marker write + fsync) BEFORE deleting the file. A crash between + // the two must never leave "file gone, no marker" — that state is + // indistinguishable from a fresh install and would silently rotate + // the identity on the next keyring-unreachable boot. If the marker + // cannot be written, keep the file so the key is never stranded. + let marker_path = migration_marker_path(data_dir); + if let Err(e) = write_migration_marker(&marker_path) { + eprintln!( + "buzz-desktop: keyring import ok but failed to write migration marker ({e}); \ + keeping identity.key so the key is not stranded" + ); + return Ok(Some(keys)); + } if let Err(e) = std::fs::remove_file(legacy_path) { eprintln!("buzz-desktop: keyring import ok but failed to delete identity.key: {e}"); } else { @@ -352,6 +395,27 @@ fn migrate_identity_file( } } +/// Path of the migration-completed marker within `data_dir`. +fn migration_marker_path(data_dir: &std::path::Path) -> std::path::PathBuf { + data_dir.join(MIGRATION_MARKER_NAME) +} + +/// Atomically write (and fsync) the migration-completed marker. The content is +/// irrelevant — only the file's durable existence is the signal — so a single +/// byte keeps it minimal. Atomicity + fsync guarantee that once this returns +/// `Ok`, the marker survives a crash, which is what makes deleting the legacy +/// file afterward safe. +fn write_migration_marker(marker_path: &std::path::Path) -> Result<(), String> { + use atomic_write_file::AtomicWriteFile; + + let mut file = AtomicWriteFile::open(marker_path) + .map_err(|e| format!("open migration marker for atomic write: {e}"))?; + file.write_all(b"1") + .map_err(|e| format!("write migration marker: {e}"))?; + file.commit() + .map_err(|e| format!("commit migration marker: {e}")) +} + /// Generate a fresh identity, persist it through the store, return it. fn generate_and_persist( store: &impl IdentityKeyStore, @@ -644,6 +708,26 @@ mod tests { deleted: RefCell::new(Vec::new()), } } + + /// Backend down this boot: probe is `Unreachable` and the slot is empty + /// (the real key, if any, is in the keyring we cannot reach). + fn unreachable() -> Self { + Self { + probe: KeyringProbe::Unreachable, + slot: RefCell::new(HashMap::new()), + deleted: RefCell::new(Vec::new()), + } + } + + /// Backend reachable with no entry — drives the one-time migration path. + /// `store`/`load` go through the slot, so a read-back verify succeeds. + fn reachable_but_empty() -> Self { + Self { + probe: KeyringProbe::ReachableButEmpty, + slot: RefCell::new(HashMap::new()), + deleted: RefCell::new(Vec::new()), + } + } } impl IdentityKeyStore for FakeIdentityStore { @@ -744,4 +828,66 @@ mod tests { assert!(store.deleted.borrow().is_empty()); assert!(!legacy_path.exists()); } + + #[test] + fn unreachable_post_migration_fails_closed_when_marker_present() { + // The silent-rotation hazard (Wes Comment 1). After a migration the + // file is gone and the marker exists; a later boot with the keyring + // unreachable must FAIL CLOSED — the real key is in the keyring, not + // gone, so generating a fresh one would silently rotate the identity. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + write_migration_marker(&migration_marker_path(dir.path())).unwrap(); + assert!(!legacy_path.exists()); + + let store = FakeIdentityStore::unreachable(); + let result = resolve_identity_with_store(&store, &legacy_path, dir.path()); + + assert!( + result.is_err(), + "must fail closed, not generate a fresh key" + ); + // No identity file was written — nothing was generated or persisted. + assert!(!legacy_path.exists()); + } + + #[test] + fn unreachable_first_run_generates_to_file_when_no_marker() { + // Genuine first-EVER launch on a machine whose keyring is down: no file, + // no marker. There is no prior identity to protect, so generating to the + // `0o600` file is correct — fail-closed here would block a legitimate + // first launch. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + assert!(!legacy_path.exists()); + assert!(!migration_marker_path(dir.path()).exists()); + + let store = FakeIdentityStore::unreachable(); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + // A fresh key was generated and persisted to the file (keyring is down). + let from_file = load_key_file(&legacy_path).unwrap(); + assert_key_eq(&resolved, &from_file); + } + + #[test] + fn migration_writes_marker_before_deleting_file() { + // Crash-safe ordering: a successful migration must leave the marker on + // disk AND remove the file. The marker existing while the file is gone + // is the durable post-migration signal the Unreachable arm relies on; + // "file gone, no marker" must never be the resting state. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + let file_keys = Keys::generate(); + save_key_file(&legacy_path, &file_keys).unwrap(); + + // ReachableButEmpty drives the one-time migration path. + let store = FakeIdentityStore::reachable_but_empty(); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + assert_key_eq(&file_keys, &resolved); + // Marker written, file deleted — the safe resting state. + assert!(migration_marker_path(dir.path()).exists()); + assert!(!legacy_path.exists()); + } } diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index 65fd220bec..9ed093237e 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -6,7 +6,7 @@ use crate::{ managed_agents::{ append_log_marker, known_acp_runtime, login_shell_path, managed_agent_log_path, missing_command_message, normalize_agent_args, open_log_file, resolve_command, - ManagedAgentProcess, ManagedAgentRecord, ManagedAgentSummary, + spawn_key_refusal, ManagedAgentProcess, ManagedAgentRecord, ManagedAgentSummary, }, util::now_iso, }; @@ -1499,6 +1499,9 @@ pub fn spawn_agent_child( record: &ManagedAgentRecord, owner_hex: Option<&str>, ) -> Result { + if let Some(error) = spawn_key_refusal(record) { + return Err(error); + } let log_path = managed_agent_log_path(app, &record.pubkey)?; append_log_marker( &log_path, diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index b513e04cb2..0ad1f881a8 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -55,6 +55,9 @@ pub fn managed_agent_log_path(app: &AppHandle, pubkey: &str) -> Result KeyringProbe; + /// Read a key. `Ok(None)` is "no such entry" (absent); `Err` is a backend + /// failure (keyring unreachable) — the caller MUST NOT collapse the two. + fn load(&self, name: &str) -> Result, String>; /// Write `value` and read it back to confirm before the caller strips the /// inline copy. fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String>; @@ -64,6 +67,9 @@ impl KeyStore for SecretStore { fn probe(&self, name: &str) -> KeyringProbe { SecretStore::probe(self, name) } + fn load(&self, name: &str) -> Result, String> { + SecretStore::load(self, name) + } fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String> { self.store(name, value)?; match self.load(name)? { @@ -82,6 +88,11 @@ enum KeyMigration { /// Could not persist (keyring unreachable, or write/verify failed). The key /// must stay inline (0o600 file fallback); do NOT drop it. KeptInline, + /// The record carried no inline key, so there was nothing to migrate. Kept + /// distinct from [`KeyMigration::Persisted`] so an empty key is never + /// mistaken for "verified present in the keyring" — an empty key after a + /// keyring outage means the secret is currently unavailable, not persisted. + Nothing, } /// Attempt to lift one record's inline key into the keyring with read-back @@ -90,11 +101,12 @@ enum KeyMigration { /// /// The single source of truth for the migrate-vs-keep decision, shared by the /// load-time opportunistic re-migrate ([`hydrate_keys`]) and the save-time -/// chokepoint ([`persist_agent_keys`]). An empty key is [`KeyMigration::Persisted`] -/// (nothing to keep inline). +/// chokepoint ([`persist_agent_keys`]). An empty key returns +/// [`KeyMigration::Nothing`] — never [`KeyMigration::Persisted`], so a record +/// left empty by a keyring outage is not mistaken for one verified present. fn migrate_inline_key(store: &impl KeyStore, record: &ManagedAgentRecord) -> KeyMigration { if record.private_key_nsec.is_empty() { - return KeyMigration::Persisted; + return KeyMigration::Nothing; } let name = agent_keyring_name(&record.pubkey); match store.probe(&name) { @@ -116,6 +128,22 @@ fn migrate_inline_key(store: &impl KeyStore, record: &ManagedAgentRecord) -> Key } } +/// Refuse to spawn an agent whose private key is unavailable. Returns +/// `Some(error)` when `private_key_nsec` is empty — after [`hydrate_keys`] an +/// empty key means a keyring outage or a genuinely absent secret, NOT a +/// deliberately keyless agent. Spawning anyway would inject an empty +/// `BUZZ_PRIVATE_KEY`/`NOSTR_PRIVATE_KEY`, launching with no identity. Callers +/// (the spawn path) must fail closed (Wes storage.rs:158). +pub(crate) fn spawn_key_refusal(record: &ManagedAgentRecord) -> Option { + record.private_key_nsec.is_empty().then(|| { + format!( + "agent {} has no private key available — the OS keyring may be unreachable. \ + Refusing to start without an identity; retry once the keyring is reachable.", + record.pubkey + ) + }) +} + pub fn load_managed_agents(app: &AppHandle) -> Result, String> { let path = managed_agents_store_path(app)?; if !path.exists() { @@ -145,6 +173,18 @@ fn hydrate_keys(records: &mut [ManagedAgentRecord]) { let Some(store) = agent_secret_store() else { return; }; + hydrate_keys_with(&store, records); +} + +/// Testable core of [`hydrate_keys`], generic over the [`KeyStore`] seam. +/// +/// A keyring LOAD error (`Err`) is an OUTAGE — distinct from `Ok(None)` +/// (genuinely absent). On an outage the key is left empty and the record is +/// surfaced as unavailable rather than silently swallowed: callers must refuse +/// to spawn an agent whose key could not be read (see the empty-key bail in +/// `spawn_agent_child`). Empty here never means "fine" — it means "no usable +/// key this boot." +fn hydrate_keys_with(store: &impl KeyStore, records: &mut [ManagedAgentRecord]) { for record in records.iter_mut() { if record.private_key_nsec.is_empty() { match store.load(&agent_keyring_name(&record.pubkey)) { @@ -155,9 +195,13 @@ fn hydrate_keys(records: &mut [ManagedAgentRecord]) { record.pubkey ); } + // Outage, NOT absence: the key may exist in the keyring but is + // unreadable this boot. Leave it empty so the spawn path + // refuses rather than launching with no identity. Err(e) => { eprintln!( - "buzz-desktop: failed to read agent {} key from keyring: {e}", + "buzz-desktop: agent {} key unavailable — keyring read failed ({e}); \ + agent will be refused until the keyring is reachable", record.pubkey ); } @@ -168,7 +212,7 @@ fn hydrate_keys(records: &mut [ManagedAgentRecord]) { // returned record must carry the key for readers. The next save // then strips it from JSON. Outcome is intentionally ignored: // on failure the key simply stays inline until a later boot. - let _ = migrate_inline_key(&store, record); + let _ = migrate_inline_key(store, record); } } } @@ -220,6 +264,9 @@ fn persist_agent_keys(records: &mut [ManagedAgentRecord], any_inline_key: &mut b // Only ever returned for a non-empty key — JSON will carry it, so // the file must be locked down. KeyMigration::KeptInline => *any_inline_key = true, + // Empty key: nothing inline to carry and nothing to clear. Do not + // treat as persisted — there is no verified keyring entry to claim. + KeyMigration::Nothing => {} } } } @@ -410,8 +457,8 @@ mod tests { use tempfile::NamedTempFile; use super::{ - agent_keyring_name, migrate_inline_key, KeyMigration, KeyStore, KeyringProbe, - ManagedAgentRecord, + agent_keyring_name, hydrate_keys_with, migrate_inline_key, KeyMigration, KeyStore, + KeyringProbe, ManagedAgentRecord, }; /// In-memory [`KeyStore`] for testing the migrate decision without the OS @@ -445,6 +492,13 @@ mod tests { stored: RefCell::new(HashMap::new()), } } + /// Seed a key as already present in the keyring. + fn with_key(self, name: &str, value: &str) -> Self { + self.stored + .borrow_mut() + .insert(name.to_string(), value.to_string()); + self + } } impl KeyStore for FakeKeyStore { @@ -455,6 +509,14 @@ mod tests { KeyringProbe::Unreachable } } + fn load(&self, name: &str) -> Result, String> { + // An unreachable backend errors on read (outage), distinct from a + // reachable backend returning `Ok(None)` for an absent entry. + if !self.reachable { + return Err("keyring backend unreachable".to_string()); + } + Ok(self.stored.borrow().get(name).cloned()) + } fn write_and_verify(&self, name: &str, value: &str) -> Result<(), String> { if self.fail_verify { return Err("read-back verify failed".to_string()); @@ -533,16 +595,66 @@ mod tests { } #[test] - fn migrate_is_noop_for_empty_key() { + fn migrate_reports_nothing_for_empty_key() { // A record whose key already lives in the keyring (empty inline) has - // nothing to migrate. + // nothing to migrate. It must NOT be reported as `Persisted` — an + // empty key after a keyring outage means the secret is unavailable, + // not verified present (Wes storage.rs:158). let store = FakeKeyStore::reachable(); let record = record_with_key(""); - assert_eq!(migrate_inline_key(&store, &record), KeyMigration::Persisted); + assert_eq!(migrate_inline_key(&store, &record), KeyMigration::Nothing); assert!(store.stored.borrow().is_empty()); } + #[test] + fn hydrate_fills_key_from_keyring_when_reachable() { + // The normal keyring-backed case: an empty inline key is filled from + // the keyring on load. + let store = + FakeKeyStore::reachable().with_key(&agent_keyring_name("agent-pubkey"), "nsec1stored"); + let mut records = vec![record_with_key("")]; + + hydrate_keys_with(&store, &mut records); + + assert_eq!(records[0].private_key_nsec, "nsec1stored"); + } + + #[test] + fn hydrate_leaves_key_empty_on_keyring_outage() { + // Outage edge (Wes storage.rs:158): when the keyring read ERRORS, the + // key must be left empty — never silently treated as resolved — so the + // spawn path refuses rather than launching the agent with no identity. + let store = FakeKeyStore::unreachable(); + let mut records = vec![record_with_key("")]; + + hydrate_keys_with(&store, &mut records); + + assert!( + records[0].private_key_nsec.is_empty(), + "an unreadable key must stay empty, not be fabricated" + ); + } + + #[test] + fn spawn_refused_when_private_key_empty() { + // The spawn path MUST refuse a record left empty by an outage/absence + // before injecting an empty BUZZ_PRIVATE_KEY / NOSTR_PRIVATE_KEY — never + // launch an agent with no identity (Wes storage.rs:158). + let record = record_with_key(""); + assert!( + super::spawn_key_refusal(&record).is_some(), + "an agent with no private key must be refused" + ); + } + + #[test] + fn spawn_allowed_when_private_key_present() { + // A record carrying a key must not be blocked by the refusal guard. + let record = record_with_key("nsec1realkey"); + assert!(super::spawn_key_refusal(&record).is_none()); + } + fn write_log(content: &str) -> NamedTempFile { let mut file = NamedTempFile::new().expect("temp log"); file.write_all(content.as_bytes()).expect("write log"); From d42248286327ef28cdb114b9deed973c48d83f2c Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Tue, 23 Jun 2026 12:20:06 -0400 Subject: [PATCH 6/8] fix(desktop): close two keyring-outage fail-closed gaps A fresh install that generated straight into a reachable keyring stored the key but never wrote the migration marker, so a later keyring- unreachable boot saw "no file, no marker" (indistinguishable from a never-launched machine) and silently rotated the identity. Write the marker after a keyring-success persist only; on the file-fallback arm the key is on disk and a marker would wrongly fail closed. Provider deploy serialized private_key_nsec unconditionally, so an empty key left by a keyring outage could deploy an agent with no identity. The local spawn path already refuses this via spawn_key_refusal; reuse the same guard at the top of build_deploy_payload so all backends fail closed. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/scripts/check-file-sizes.mjs | 5 + desktop/src-tauri/src/app_state.rs | 132 +++++++++++++++++++++-- desktop/src-tauri/src/commands/agents.rs | 18 +++- 3 files changed, 143 insertions(+), 12 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index a92231ad64..78f84e7bc2 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -70,6 +70,11 @@ const overrides = new Map([ // the Inbox nav badge — a small overage from load-bearing badge plumbing, // not generic debt growth. Approved override; still queued to split. ["src/app/AppShell.tsx", 1008], + // PersistBackend enum + marker-on-keyring-success plumbing and its three + // fail-closed regression tests (silent identity rotation on keyring outage). + // A small overage from load-bearing security plumbing on a file already at + // 893 lines, not generic debt growth. Approved override; still queued to split. + ["src-tauri/src/app_state.rs", 1012], ]); await runFileSizeCheck({ diff --git a/desktop/src-tauri/src/app_state.rs b/desktop/src-tauri/src/app_state.rs index a9a11e3de4..c63ecb2fff 100644 --- a/desktop/src-tauri/src/app_state.rs +++ b/desktop/src-tauri/src/app_state.rs @@ -294,7 +294,7 @@ fn resolve_identity_with_store( } } - generate_and_persist(store, legacy_path) + generate_and_persist(store, legacy_path, data_dir) } /// Recover from a corrupt nsec in the keyring (parse failed). Clear the bad @@ -316,7 +316,7 @@ fn recover_from_keyring( return Ok(keys); } } - generate_and_persist(store, legacy_path) + generate_and_persist(store, legacy_path, data_dir) } /// Load the `0o600` identity file, quarantining corruption, else generate and @@ -416,13 +416,38 @@ fn write_migration_marker(marker_path: &std::path::Path) -> Result<(), String> { .map_err(|e| format!("commit migration marker: {e}")) } +/// Which backend `persist_identity` wrote to. The caller writes the migration +/// marker only after a keyring success — on the file-fallback arm the key is on +/// disk and a marker would wrongly trip the next Unreachable boot into failing +/// closed. +enum PersistBackend { + Keyring, + File, +} + /// Generate a fresh identity, persist it through the store, return it. +/// +/// On a keyring-backed persist no file is written, so a later +/// keyring-Unreachable boot would see "no file, no marker" (identical to a +/// fresh install) and silently rotate the identity. Writing the marker here +/// makes that boot fail closed. If the marker write fails, fall back to the +/// `0o600` file so the key is never keyring-only-without-marker. fn generate_and_persist( store: &impl IdentityKeyStore, legacy_path: &std::path::Path, + data_dir: &std::path::Path, ) -> Result { let keys = Keys::generate(); - persist_identity(store, &keys, legacy_path)?; + if let PersistBackend::Keyring = persist_identity(store, &keys, legacy_path)? { + let marker_path = migration_marker_path(data_dir); + if let Err(e) = write_migration_marker(&marker_path) { + eprintln!( + "buzz-desktop: stored identity in keyring but failed to write migration marker \ + ({e}); saving identity.key fallback so the key is not stranded" + ); + save_key_file(legacy_path, &keys)?; + } + } eprintln!( "buzz-desktop: generated and saved identity pubkey {}", keys.public_key().to_hex() @@ -431,21 +456,23 @@ fn generate_and_persist( } /// Persist `keys` through the store, falling back to the `0o600` file when the -/// keyring write fails on an availability error. +/// keyring write fails on an availability error. Reports which backend held the +/// key so the caller can write the migration marker only on keyring success. fn persist_identity( store: &impl IdentityKeyStore, keys: &Keys, legacy_path: &std::path::Path, -) -> Result<(), String> { +) -> Result { let nsec = keys .secret_key() .to_bech32() .map_err(|e| format!("encode nsec: {e}"))?; match store.store(IDENTITY_KEY_NAME, &nsec) { - Ok(()) => Ok(()), + Ok(()) => Ok(PersistBackend::Keyring), Err(keyring_err) => { eprintln!("buzz-desktop: keyring write failed ({keyring_err}), using file fallback"); - save_key_file(legacy_path, keys) + save_key_file(legacy_path, keys)?; + Ok(PersistBackend::File) } } } @@ -696,6 +723,9 @@ mod tests { probe: KeyringProbe, slot: RefCell>, deleted: RefCell>, + /// When true, `store` returns an availability error, driving the + /// keyring-write-failure → file-fallback arm of `persist_identity`. + store_fails: bool, } impl FakeIdentityStore { @@ -706,6 +736,7 @@ mod tests { probe: KeyringProbe::Present, slot: RefCell::new(slot), deleted: RefCell::new(Vec::new()), + store_fails: false, } } @@ -716,6 +747,7 @@ mod tests { probe: KeyringProbe::Unreachable, slot: RefCell::new(HashMap::new()), deleted: RefCell::new(Vec::new()), + store_fails: false, } } @@ -726,6 +758,18 @@ mod tests { probe: KeyringProbe::ReachableButEmpty, slot: RefCell::new(HashMap::new()), deleted: RefCell::new(Vec::new()), + store_fails: false, + } + } + + /// Reachable-but-empty probe whose `store` always fails — exercises the + /// keyring-write-failure → `0o600` file-fallback arm. + fn store_failing() -> Self { + Self { + probe: KeyringProbe::ReachableButEmpty, + slot: RefCell::new(HashMap::new()), + deleted: RefCell::new(Vec::new()), + store_fails: true, } } } @@ -738,6 +782,9 @@ mod tests { Ok(self.slot.borrow().get(name).cloned()) } fn store(&self, name: &str, value: &str) -> Result<(), String> { + if self.store_fails { + return Err("simulated keyring write failure".to_string()); + } self.slot .borrow_mut() .insert(name.to_string(), value.to_string()); @@ -890,4 +937,75 @@ mod tests { assert!(migration_marker_path(dir.path()).exists()); assert!(!legacy_path.exists()); } + + #[test] + fn fresh_keyring_generate_writes_marker() { + // Fix 1 (Pinky comment 1): a fresh install generating straight into a + // reachable-but-empty keyring must write the marker. Without it, "no + // file, no marker" matches a never-launched machine, so a later + // Unreachable boot would silently rotate the key. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + assert!(!legacy_path.exists()); + + let store = FakeIdentityStore::reachable_but_empty(); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + // The key was stored in the keyring (not the file), and the marker marks it. + assert!(!legacy_path.exists()); + assert!(migration_marker_path(dir.path()).exists()); + assert_eq!( + store + .slot + .borrow() + .get(IDENTITY_KEY_NAME) + .map(String::as_str), + Some(resolved.secret_key().to_bech32().unwrap().as_str()) + ); + } + + #[test] + fn fresh_keyring_generate_then_unreachable_fails_closed() { + // The end-to-end guard for Fix 1: after a fresh keyring-created identity + // (marker written, no file), a later boot with the keyring unreachable + // must FAIL CLOSED rather than generate a new key and rotate identity. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + + // First boot: fresh generate into a reachable keyring. + let reachable = FakeIdentityStore::reachable_but_empty(); + resolve_identity_with_store(&reachable, &legacy_path, dir.path()).unwrap(); + assert!(!legacy_path.exists()); + assert!(migration_marker_path(dir.path()).exists()); + + // Second boot: keyring is down. No file + marker present → fail closed. + let unreachable = FakeIdentityStore::unreachable(); + let result = resolve_identity_with_store(&unreachable, &legacy_path, dir.path()); + + assert!( + result.is_err(), + "must fail closed, not generate a fresh key" + ); + assert!(!legacy_path.exists()); + } + + #[test] + fn fresh_generate_keyring_failure_falls_back_to_file_without_marker() { + // Fix 1 correctness on the file-fallback arm: when the keyring write + // FAILS during a fresh generate, the key must land in the `0o600` file + // and the marker must NOT be written — a marker here would wrongly trip + // the next Unreachable boot into failing closed even though the key is + // sitting in the file. + let dir = tempfile::tempdir().unwrap(); + let legacy_path = dir.path().join("identity.key"); + + let store = FakeIdentityStore::store_failing(); + let resolved = resolve_identity_with_store(&store, &legacy_path, dir.path()).unwrap(); + + // Key persisted to the file (fallback), and recoverable from it. + let from_file = load_key_file(&legacy_path).unwrap(); + assert_key_eq(&resolved, &from_file); + // No marker: the file is the authoritative store, not the keyring. + assert!(!migration_marker_path(dir.path()).exists()); + } } diff --git a/desktop/src-tauri/src/commands/agents.rs b/desktop/src-tauri/src/commands/agents.rs index c54d0b066e..14a2faafce 100644 --- a/desktop/src-tauri/src/commands/agents.rs +++ b/desktop/src-tauri/src/commands/agents.rs @@ -8,11 +8,12 @@ use crate::{ find_managed_agent_mut, invoke_provider, load_managed_agents, load_personas, managed_agent_avatar_url, managed_agent_log_path, managed_agents_base_dir, normalize_agent_args, provider_deploy, read_log_tail, resolve_provider_binary, - save_managed_agents, start_managed_agent_process, stop_managed_agent_process, - sync_managed_agent_processes, try_regenerate_nest, validate_provider_config, BackendKind, - BackendProviderInfo, CreateManagedAgentRequest, CreateManagedAgentResponse, - ManagedAgentLogResponse, ManagedAgentRecord, ManagedAgentSummary, RelayMeshConfig, - DEFAULT_ACP_COMMAND, DEFAULT_AGENT_PARALLELISM, DEFAULT_AGENT_TURN_TIMEOUT_SECONDS, + save_managed_agents, spawn_key_refusal, start_managed_agent_process, + stop_managed_agent_process, sync_managed_agent_processes, try_regenerate_nest, + validate_provider_config, BackendKind, BackendProviderInfo, CreateManagedAgentRequest, + CreateManagedAgentResponse, ManagedAgentLogResponse, ManagedAgentRecord, + ManagedAgentSummary, RelayMeshConfig, DEFAULT_ACP_COMMAND, DEFAULT_AGENT_PARALLELISM, + DEFAULT_AGENT_TURN_TIMEOUT_SECONDS, }, relay::{relay_ws_url_with_override, sync_managed_agent_profile}, util::now_iso, @@ -148,6 +149,13 @@ fn build_deploy_payload( state: &AppState, record: &ManagedAgentRecord, ) -> Result { + // Fail closed when the private key is unavailable (keyring outage leaves it + // empty after hydration). Without this, a provider deploy would serialize + // `"private_key_nsec": ""` and launch the agent with no identity — the same + // hazard the local spawn path already refuses via this guard. + if let Some(error) = spawn_key_refusal(record) { + return Err(error); + } // Merge persona env_vars + agent env_vars for provider deploy. Same // precedence as local spawn: persona first, agent overrides last. Without // this, provider-backed agents wouldn't receive credentials saved on the From 129ca837da9e059d82ae38b10c723954e936d76e Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Wed, 24 Jun 2026 15:48:56 -0400 Subject: [PATCH 7/8] fix(desktop): write managed-agents.json 0o600 before secret bytes hit disk The keyringless fallback wrote the agent store via std::fs::write + rename (process umask, typically 0644) and only chmod'd it 0o600 after the rename. A crash in that window could leave managed-agents.json with plaintext agent nsecs world-readable, contradicting the SECURITY.md owner-only fallback guarantee. The new atomic_write_json_restricted mirrors save_key_file: it sets 0o600 on the temp file before any bytes are written, then commits. The managed-agents write is now always owner-only, which retires the any_inline_key plumbing and the restrict_json_permissions chmod helper. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- .../src-tauri/src/managed_agents/storage.rs | 114 +++++++++++------- 1 file changed, 72 insertions(+), 42 deletions(-) diff --git a/desktop/src-tauri/src/managed_agents/storage.rs b/desktop/src-tauri/src/managed_agents/storage.rs index 0ad1f881a8..495c7c1dd1 100644 --- a/desktop/src-tauri/src/managed_agents/storage.rs +++ b/desktop/src-tauri/src/managed_agents/storage.rs @@ -228,64 +228,40 @@ pub fn save_managed_agents(app: &AppHandle, records: &[ManagedAgentRecord]) -> R // Persist each key to the keyring; on success blank the inline copy so it // is skipped from JSON (`skip_serializing_if = "String::is_empty"`). If the - // keyring is unreachable, the key stays inline and the JSON gets 0o600. - let mut any_inline_key = false; - persist_agent_keys(&mut sorted, &mut any_inline_key); + // keyring is unreachable, the key stays inline. + persist_agent_keys(&mut sorted); let path = managed_agents_store_path(app)?; let payload = serde_json::to_vec_pretty(&sorted) .map_err(|error| format!("failed to serialize agent store: {error}"))?; - atomic_write_json(&path, &payload)?; - - // The JSON only carries plaintext keys in the keyringless fallback; lock it - // down to owner-only in that case. - if any_inline_key { - restrict_json_permissions(&path); - } - Ok(()) + // `managed-agents.json` carries plaintext agent nsecs in the keyringless + // fallback. Write it owner-only (`0o600`) unconditionally — harmless for the + // keyring-backed case (it is the user's own agent store) and closes the + // umask window a post-write `chmod` would leave open. + atomic_write_json_restricted(&path, &payload) } /// Write each record's in-memory key to the keyring and blank the inline copy -/// on success. Sets `any_inline_key` if any key had to stay in the JSON because -/// the keyring was unreachable. Mutates `records` (a save-local clone) — the -/// caller's in-memory records keep their keys. -fn persist_agent_keys(records: &mut [ManagedAgentRecord], any_inline_key: &mut bool) { +/// on success. Keys that cannot be persisted (keyring unreachable) stay inline +/// in the JSON. Mutates `records` (a save-local clone) — the caller's in-memory +/// records keep their keys. +fn persist_agent_keys(records: &mut [ManagedAgentRecord]) { let Some(store) = agent_secret_store() else { // No keyring backend: keys stay inline. - *any_inline_key = records.iter().any(|r| !r.private_key_nsec.is_empty()); return; }; for record in records.iter_mut() { - match migrate_inline_key(&store, record) { - // Verified in the keyring — drop the inline copy so it stays out of - // JSON. Safe: this is a save-local clone, callers keep their keys. - KeyMigration::Persisted => record.private_key_nsec.clear(), - // Only ever returned for a non-empty key — JSON will carry it, so - // the file must be locked down. - KeyMigration::KeptInline => *any_inline_key = true, - // Empty key: nothing inline to carry and nothing to clear. Do not - // treat as persisted — there is no verified keyring entry to claim. - KeyMigration::Nothing => {} + // Only a verified keyring entry lets us drop the inline copy. Both + // other outcomes keep the key inline: `KeptInline` (keyring + // unreachable) so it is not lost, and `Nothing` (empty key) because + // there is no verified entry to claim. This is a save-local clone, so + // callers keep their keys regardless. + if migrate_inline_key(&store, record) == KeyMigration::Persisted { + record.private_key_nsec.clear(); } } } - -#[cfg(unix)] -fn restrict_json_permissions(path: &Path) { - use std::os::unix::fs::PermissionsExt; - let resolved = std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); - if let Err(e) = std::fs::set_permissions(&resolved, std::fs::Permissions::from_mode(0o600)) { - eprintln!( - "buzz-desktop: failed to restrict {} permissions: {e}", - resolved.display() - ); - } -} - -#[cfg(not(unix))] -fn restrict_json_permissions(_path: &Path) {} - /// Remove an agent's key from the keyring (best-effort). Called when an agent /// is deleted so its secret does not linger in the OS store. pub fn delete_agent_key(pubkey: &str) { @@ -307,6 +283,34 @@ pub(crate) fn atomic_write_json(path: &Path, payload: &[u8]) -> Result<(), Strin .map_err(|e| format!("failed to rename {}: {e}", resolved.display())) } +/// Atomic, symlink-preserving JSON write that creates the file `0o600` BEFORE +/// any bytes hit disk — closing the umask window the post-write `chmod` left +/// open. Used for `managed-agents.json`, which carries plaintext agent nsecs in +/// the keyringless fallback. Mirrors [`crate::app_state::save_key_file`]. +/// +/// Canonicalizes `path` first so the write lands at the real target, preserving +/// any symlink at `path` exactly like [`atomic_write_json`]. +pub(crate) fn atomic_write_json_restricted(path: &Path, payload: &[u8]) -> Result<(), String> { + use atomic_write_file::AtomicWriteFile; + + let resolved = std::fs::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()); + let mut file = AtomicWriteFile::open(&resolved) + .map_err(|e| format!("open {} for atomic write: {e}", resolved.display()))?; + + // Set owner-only permissions before writing the secret bytes. + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + file.set_permissions(std::fs::Permissions::from_mode(0o600)) + .map_err(|e| format!("set {} permissions: {e}", resolved.display()))?; + } + + file.write_all(payload) + .map_err(|e| format!("write {}: {e}", resolved.display()))?; + file.commit() + .map_err(|e| format!("commit {}: {e}", resolved.display())) +} + /// Maximum log file size before rotation (10 MB). const MAX_LOG_FILE_SIZE: u64 = 10 * 1024 * 1024; @@ -661,6 +665,32 @@ mod tests { file } + /// The keyringless fallback write must land `0o600` from the write itself — + /// not a post-write `chmod` — so a crash in the umask window can never leave + /// plaintext agent nsecs world-readable (Wes storage.rs:239, SECURITY.md:90). + #[cfg(unix)] + #[test] + fn restricted_write_lands_owner_only_without_post_write_chmod() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().expect("temp dir"); + let path = dir.path().join("managed-agents.json"); + + super::atomic_write_json_restricted(&path, br#"[{"private_key_nsec":"nsec1secret"}]"#) + .expect("restricted write"); + + let mode = std::fs::metadata(&path) + .expect("metadata") + .permissions() + .mode() + & 0o777; + assert_eq!(mode, 0o600, "secret-bearing write must be owner-only"); + assert_eq!( + std::fs::read_to_string(&path).expect("read back"), + r#"[{"private_key_nsec":"nsec1secret"}]"# + ); + } + #[test] fn meaningful_agent_error_from_log_promotes_wrapped_llm_auth() { let file = write_log("noise\nAgent reported error: llm auth: denied\n"); From fb539bfdfda58c8a6f8c58bdec1db7a69b9c3ca6 Mon Sep 17 00:00:00 2001 From: npub1mn7jgtj4w2pd0g0zeuhxsa6jy6p0rewxz4kujt98my82ahfmp72sxjexk7 Date: Wed, 24 Jun 2026 16:05:42 -0400 Subject: [PATCH 8/8] fix(desktop): write managed-agents.json 0o600 on the migration reconcile path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit patch_json_records rewrote the whole store via atomic_write_json (raw write+rename, umask ~0o644, no chmod), reopening the SECURITY.md:90 owner-only window on keyringless hosts whenever a launch-time field reconcile touches managed-agents.json. Route its writeback through atomic_write_json_restricted unconditionally — all targets live in the single-user agents/ dir, so 0o600 on personas/teams is harmless and avoids reintroducing the route-by-filename guard that caused the bug. Co-authored-by: Will Pfleger Signed-off-by: Will Pfleger --- desktop/scripts/check-file-sizes.mjs | 12 ++++++---- desktop/src-tauri/src/migration.rs | 7 +++++- desktop/src-tauri/src/migration_tests.rs | 30 ++++++++++++++++++++++++ 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/desktop/scripts/check-file-sizes.mjs b/desktop/scripts/check-file-sizes.mjs index 78f84e7bc2..679ce2ed51 100644 --- a/desktop/scripts/check-file-sizes.mjs +++ b/desktop/scripts/check-file-sizes.mjs @@ -41,7 +41,7 @@ const overrides = new Map([ // harness-persona-sync: persona-runtime resolution threaded into the spawn // path here. Load-bearing feature growth; queued to split in the resolver // unify refactor followup. - ["src-tauri/src/managed_agents/runtime.rs", 1966], + ["src-tauri/src/managed_agents/runtime.rs", 1969], ["src-tauri/src/managed_agents/personas.rs", 1080], ["src-tauri/src/managed_agents/persona_card.rs", 1050], // applyWorkspace reposDir parameter plus the validateReposDir binding, @@ -53,11 +53,15 @@ const overrides = new Map([ // harness-persona-sync feature growth, queued to split in the resolver-unify // refactor followup. discovery.rs is dominated by the new test module // (the effective_agent_command / divergent / create-time override matrix); - // types.rs adds the persona/instance harness fields; migration_tests.rs adds - // the harness-sync migration coverage. Load-bearing, not generic debt. + // types.rs adds the persona/instance harness fields. Load-bearing, not + // generic debt. ["src-tauri/src/managed_agents/discovery.rs", 1043], ["src-tauri/src/managed_agents/types.rs", 1010], - ["src-tauri/src/migration_tests.rs", 1033], + // migration_tests.rs carries the harness-sync migration coverage plus the + // patch_json_records owner-only writeback regression test (SECURITY.md:90 + // crash-safe 0o600 fallback). Load-bearing security + feature coverage, not + // generic debt growth. Approved override; still queued to split. + ["src-tauri/src/migration_tests.rs", 1063], ["src-tauri/src/nostr_convert.rs", 1126], ["src/shared/api/relayClientSession.ts", 1022], ["src-tauri/src/migration.rs", 1295], diff --git a/desktop/src-tauri/src/migration.rs b/desktop/src-tauri/src/migration.rs index d44245190b..1da537ea9b 100644 --- a/desktop/src-tauri/src/migration.rs +++ b/desktop/src-tauri/src/migration.rs @@ -263,6 +263,11 @@ fn copy_file_over_generated_default(src: &Path, dst: &Path) -> std::io::Result<( /// Read a JSON array of objects from `path`, apply `f` to each object, /// and write back if any mutation returned `true`. +/// +/// Writes back via [`crate::managed_agents::atomic_write_json_restricted`] +/// (owner-only `0o600`): the store files this rewrites can carry plaintext +/// agent nsecs on a keyringless host, so the write must not reopen the umask +/// window SECURITY.md:90 closes. fn patch_json_records( path: &Path, mut f: impl FnMut(&mut serde_json::Map) -> bool, @@ -285,7 +290,7 @@ fn patch_json_records( } if changed { if let Ok(bytes) = serde_json::to_vec_pretty(&records) { - if let Err(e) = crate::managed_agents::atomic_write_json(path, &bytes) { + if let Err(e) = crate::managed_agents::atomic_write_json_restricted(path, &bytes) { eprintln!("buzz-desktop: patch-json-records: {e}"); } } diff --git a/desktop/src-tauri/src/migration_tests.rs b/desktop/src-tauri/src/migration_tests.rs index f60f394fd6..2f013d77a3 100644 --- a/desktop/src-tauri/src/migration_tests.rs +++ b/desktop/src-tauri/src/migration_tests.rs @@ -654,6 +654,36 @@ fn migrate_packs_to_teams_rewrites_agents_json() { assert!(records[0].get("persona_name_in_pack").is_none()); } +/// `patch_json_records` rewrites `managed-agents.json`, which carries plaintext +/// agent nsecs on a keyringless host — the writeback must land `0o600` from the +/// write itself (no post-write `chmod`), or a launch-time reconcile reopens the +/// umask window SECURITY.md:90 closes (Thufir, migration.rs:288). +#[cfg(unix)] +#[test] +fn patch_json_records_rewrites_secret_store_owner_only() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + write_agents_json( + dir.path(), + &serde_json::json!([{ "private_key_nsec": "nsec1secret", "provider": "goose" }]), + ); + let path = dir.path().join("agents/managed-agents.json"); + + // Mutate so the write actually fires (it only writes back on `changed`). + patch_json_records(&path, |obj| { + let provider = obj.remove("provider").unwrap(); + obj.insert("runtime".to_string(), provider); + true + }); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600, "secret-bearing rewrite must be owner-only"); + let records = read_agents_json(dir.path()); + assert_eq!(records[0]["private_key_nsec"], "nsec1secret"); + assert_eq!(records[0]["runtime"], "goose"); +} + #[test] fn rename_provider_to_runtime_migrates_field() { let dir = tempfile::tempdir().unwrap();