mirror of
https://github.com/block/buzz.git
synced 2026-10-29 13:31:44 +00:00
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 <loganj@squareup.com>
This commit is contained in:
@@ -184,6 +184,16 @@ fn commit_verified<R: tauri::Runtime>(
|
||||
{
|
||||
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)
|
||||
|
||||
@@ -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<std::ffi::OsString>)>);
|
||||
|
||||
#[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<std::ffi::OsString>)>);
|
||||
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"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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()
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user