From a3a0745d0c03bbb2f2b6db0f6fdcfe95867f3a30 Mon Sep 17 00:00:00 2001 From: Logan Johnson Date: Tue, 8 Sep 2026 18:00:08 -0400 Subject: [PATCH] fix(desktop): verify saved owner link before agent duplicate or repair Add existing agent admitted an existing record on pubkey/persona/community and then reported a healthy duplicate or repaired a missing key without ever checking the saved NIP-OA attestation. A legacy record with a missing, foreign, or invalid saved owner link could pass Add while launch preparation later refuses it as unowned. Under the existing commit locks, commit_verified now validates the saved record's attestation for the exact current owner and agent via the same runtime_configurations::verify_owner predicate launch preparation applies, before both the healthy-duplicate return and the key-repair write. Absent, foreign, and invalid links are refused with a redacted error and zero writes; no silent re-attestation or owner migration is introduced. Regression coverage at the real commit seam crosses healthy and missing saved keys with absent, foreign, and structurally-valid-but-unverifiable attestations, asserting refusal preserves raw store bytes and key state; positive valid-duplicate and explicit-repair controls are retained. Signed-off-by: Logan Johnson --- .../src-tauri/src/commands/existing_agent.rs | 10 ++ .../src/commands/existing_agent/tests.rs | 163 +++++++++++++++--- .../managed_agents/runtime_configurations.rs | 7 +- desktop/src-tauri/src/managed_agents/types.rs | 8 +- desktop/src/features/agents/AGENTS.md | 4 + 5 files changed, 168 insertions(+), 24 deletions(-) diff --git a/desktop/src-tauri/src/commands/existing_agent.rs b/desktop/src-tauri/src/commands/existing_agent.rs index 8d79b7c139..5f0f705520 100644 --- a/desktop/src-tauri/src/commands/existing_agent.rs +++ b/desktop/src-tauri/src/commands/existing_agent.rs @@ -184,6 +184,16 @@ fn commit_verified( { return Err("Existing agent profile or community does not match".into()); } + // The saved owner link must be usable by the exact current owner + // BEFORE any duplicate or repair effect. Launch preparation enforces + // this same verifier, so an absent/foreign/invalid attestation is + // refused here instead of reporting a healthy duplicate or a repaired + // key for a record that later refuses to run as unowned. No write, no + // silent re-attestation or owner migration. + agents::runtime_configurations::verify_owner(saved, &input.owner).map_err(|_| { + "Saved agent ownership is missing, invalid, or belongs to a different identity" + .to_string() + })?; if nostr::SecretKey::parse(&saved.private_key_nsec) .ok() .is_some_and(|key| Keys::new(key).public_key().to_hex() == input.pubkey) diff --git a/desktop/src-tauri/src/commands/existing_agent/tests.rs b/desktop/src-tauri/src/commands/existing_agent/tests.rs index 8ab016cb5e..efe97bd4be 100644 --- a/desktop/src-tauri/src/commands/existing_agent/tests.rs +++ b/desktop/src-tauri/src/commands/existing_agent/tests.rs @@ -87,6 +87,49 @@ fn conflicting_oa_and_duplicate_coordinates_are_refused() { assert!(verified_record(&input, &profiles, &[policy]).is_err()); } +/// Compute a NIP-OA attestation exactly as the production commit does. +#[cfg(all(unix, not(feature = "system-keyring")))] +fn owner_tag(owner: &Keys, agent_pubkey: &str) -> String { + buzz_sdk_pkg::nip_oa::compute_auth_tag( + owner, + &nostr::PublicKey::from_hex(agent_pubkey).unwrap(), + "", + ) + .unwrap() +} + +// Never run filesystem/credential fixtures against the real system keyring. +#[cfg(all(unix, not(feature = "system-keyring")))] +struct TempHomeEnv(Vec<(&'static str, Option)>); + +#[cfg(all(unix, not(feature = "system-keyring")))] +impl Drop for TempHomeEnv { + fn drop(&mut self) { + for (name, value) in &self.0 { + match value { + Some(v) => std::env::set_var(name, v), + None => std::env::remove_var(name), + } + } + } +} + +/// Point HOME/XDG_DATA_HOME at `temp` and restore them on drop. Pair with +/// `lock_path_mutex` so parallel env-mutating tests stay exclusive. +#[cfg(all(unix, not(feature = "system-keyring")))] +fn isolate_agent_home(temp: &std::path::Path) -> TempHomeEnv { + TempHomeEnv( + ["HOME", "XDG_DATA_HOME"] + .into_iter() + .map(|name| { + let old = std::env::var_os(name); + std::env::set_var(name, temp); + (name, old) + }) + .collect(), + ) +} + // Never run filesystem/credential fixtures against the real system keyring. #[cfg(all(unix, not(feature = "system-keyring")))] #[test] @@ -94,31 +137,19 @@ fn explicit_file_commit_repair_and_ordinary_save_f6() { use agents::storage; let _guard = agents::lock_path_mutex(); let temp = tempfile::tempdir().unwrap(); - struct Env(Vec<(&'static str, Option)>); - impl Drop for Env { - fn drop(&mut self) { - for (name, value) in &self.0 { - match value { - Some(v) => std::env::set_var(name, v), - None => std::env::remove_var(name), - } - } - } - } - let _env = Env(["HOME", "XDG_DATA_HOME"] - .into_iter() - .map(|name| { - let old = std::env::var_os(name); - std::env::set_var(name, temp.path()); - (name, old) - }) - .collect()); + let _env = isolate_agent_home(temp.path()); let app = tauri::test::mock_builder() .build(tauri::test::mock_context(tauri::test::noop_assets())) .unwrap(); let (input, profiles, policies) = fixture(); - let record = verified_record(&input, &profiles, &policies).unwrap(); + let mut record = verified_record(&input, &profiles, &policies).unwrap(); assert!(storage::save_managed_agents(app.handle(), std::slice::from_ref(&record)).is_err()); + // Seed the store exactly as the production commit leaves it: record, + // credential, and the current owner's attestation in one restricted write. + record.auth_tag = Some(owner_tag( + &Keys::parse(&"01".repeat(32)).unwrap(), + &input.pubkey, + )); storage::import_existing_agent_key(app.handle(), record.clone()).unwrap(); let path = agents::managed_agents_store_path(app.handle()).unwrap(); let state = crate::app_state::build_app_state(); @@ -181,3 +212,95 @@ fn explicit_file_commit_repair_and_ordinary_save_f6() { 0o600 ); } + +// The saved owner link gates BOTH commit outcomes: a legacy record whose saved +// attestation is absent, foreign, or invalid must be refused with zero writes, +// whatever the key state — never a healthy duplicate or repaired key for a +// record that later refuses to run (runtime_configurations::verify_owner). +#[cfg(all(unix, not(feature = "system-keyring")))] +#[test] +fn saved_owner_link_gates_duplicate_and_repair() { + use agents::storage; + let _guard = agents::lock_path_mutex(); + let temp = tempfile::tempdir().unwrap(); + let _env = isolate_agent_home(temp.path()); + let app = tauri::test::mock_builder() + .build(tauri::test::mock_context(tauri::test::noop_assets())) + .unwrap(); + let (input, profiles, policies) = fixture(); + let record = verified_record(&input, &profiles, &policies).unwrap(); + let state = crate::app_state::build_app_state(); + *state.keys.lock().unwrap() = Keys::parse(&"01".repeat(32)).unwrap(); + *state.relay_url_override.lock().unwrap() = Some(input.community.clone()); + let mut persona = agents::load_personas(app.handle()).unwrap().remove(0); + persona.id = "existing-persona".into(); + persona.is_active = true; + agents::save_personas(app.handle(), &[persona]).unwrap(); + // Production creation commit: record + credential + owner tag in one write. + commit_verified(app.handle(), &state, &input, record.clone()).unwrap(); + let path = agents::managed_agents_store_path(app.handle()).unwrap(); + let created = std::fs::read(&path).unwrap(); + // Positive controls first: this exact fixture must admit both outcomes the + // guard protects, so every refusal below varies only the saved owner link. + commit_verified(app.handle(), &state, &input, record.clone()).unwrap(); + assert_eq!(std::fs::read(&path).unwrap(), created); // Healthy duplicate. + let mut raw = storage::load_agent_store(app.handle()).unwrap(); + raw.iter_mut() + .find(|r| r.pubkey == input.pubkey) + .unwrap() + .private_key_nsec + .clear(); + std::fs::write(&path, serde_json::to_vec(&raw).unwrap()).unwrap(); + commit_verified(app.handle(), &state, &input, record.clone()).unwrap(); // Key repair. + assert_eq!( + storage::load_agent_store(app.handle()) + .unwrap() + .into_iter() + .find(|r| r.pubkey == input.pubkey) + .unwrap() + .private_key_nsec, + input.private_key + ); + let foreign = owner_tag(&Keys::parse(&"03".repeat(32)).unwrap(), &input.pubkey); + // Structurally valid, current owner embedded, but signed over a different + // agent's preimage — only signature verification rejects this one. + let invalid = owner_tag( + &Keys::parse(&"01".repeat(32)).unwrap(), + &Keys::parse(&"04".repeat(32)).unwrap().public_key().to_hex(), + ); + for (name, tag, healthy_key) in [ + ("absent attestation, healthy key", None, true), + ("absent attestation, missing key", None, false), + ("foreign attestation, healthy key", Some(&foreign), true), + ("foreign attestation, missing key", Some(&foreign), false), + ("invalid attestation, healthy key", Some(&invalid), true), + ("invalid attestation, missing key", Some(&invalid), false), + ] { + let mut raw = storage::load_agent_store(app.handle()).unwrap(); + let saved = raw.iter_mut().find(|r| r.pubkey == input.pubkey).unwrap(); + saved.auth_tag = tag.cloned(); + saved.private_key_nsec = if healthy_key { + input.private_key.clone() + } else { + String::new() + }; + std::fs::write(&path, serde_json::to_vec(&raw).unwrap()).unwrap(); + let before = std::fs::read(&path).unwrap(); + let error = commit_verified(app.handle(), &state, &input, record.clone()).expect_err(name); + assert_eq!( + error, "Saved agent ownership is missing, invalid, or belongs to a different identity", + "{name}" + ); + assert_eq!(std::fs::read(&path).unwrap(), before, "{name}: no writes"); + let persisted = storage::load_agent_store(app.handle()) + .unwrap() + .into_iter() + .find(|r| r.pubkey == input.pubkey) + .unwrap(); + assert_eq!( + persisted.private_key_nsec.is_empty(), + !healthy_key, + "{name}: key state unchanged" + ); + } +} diff --git a/desktop/src-tauri/src/managed_agents/runtime_configurations.rs b/desktop/src-tauri/src/managed_agents/runtime_configurations.rs index 2d505e752c..252c729684 100644 --- a/desktop/src-tauri/src/managed_agents/runtime_configurations.rs +++ b/desktop/src-tauri/src/managed_agents/runtime_configurations.rs @@ -398,7 +398,12 @@ pub(crate) fn preflight( Ok(()) } -fn verify_owner(record: &ManagedAgentRecord, owner: &str) -> Result<(), String> { +/// Ownership authority shared by launch preparation and explicit +/// existing-agent admission: a local record must carry a saved NIP-OA +/// attestation that verifies for the exact agent pubkey and resolves to +/// `owner`. One predicate fences both, so admission can never report success +/// for a record that later refuses to run as unowned. +pub(crate) fn verify_owner(record: &ManagedAgentRecord, owner: &str) -> Result<(), String> { let verified = record.auth_tag.as_deref().and_then(|tag| { let agent = nostr::PublicKey::from_hex(&record.pubkey).ok()?; buzz_sdk_pkg::nip_oa::verify_auth_tag(tag, &agent).ok() diff --git a/desktop/src-tauri/src/managed_agents/types.rs b/desktop/src-tauri/src/managed_agents/types.rs index 2e5a016cbf..c3ee813128 100644 --- a/desktop/src-tauri/src/managed_agents/types.rs +++ b/desktop/src-tauri/src/managed_agents/types.rs @@ -245,9 +245,11 @@ pub struct ManagedAgentRecord { /// 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 + /// It is serialized inline (the `0o600` JSON fallback) when the keyring is + /// unreachable, and deliberately by the explicit existing-agent import, + /// which provisions the user-supplied key in the restricted local file in + /// one atomic write — `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, diff --git a/desktop/src/features/agents/AGENTS.md b/desktop/src/features/agents/AGENTS.md index a8003961df..6c851e5cc0 100644 --- a/desktop/src/features/agents/AGENTS.md +++ b/desktop/src/features/agents/AGENTS.md @@ -491,6 +491,10 @@ independently owner-signed kind:30177 persona link in the current community. The linked definition must already exist; it is never cloned. Workspace apply, identity mutation and agent store locks fence the commit. Healthy duplicates do not write; explicit repair changes only the credential, not runtime/config state. +A saved record's ownership attestation must verify for the exact current owner +and agent before either outcome — the same verifier launch preparation applies +— so a missing, foreign, or invalid saved owner link is refused without writes +instead of reporting success for a record that later refuses to run. This operation deliberately stores the user-supplied key in the restricted local agent file in one atomic write with the record, avoiding a keyring-first partial commit. The dialog discloses that storage choice. Ordinary save/Move/restore and