From 4489929e8fd83f49617f2fe5c6ad5f98e3508a9c Mon Sep 17 00:00:00 2001 From: "William K. Santiago" Date: Thu, 26 Feb 2026 10:05:52 -0500 Subject: [PATCH 01/13] Add NIP-46 ack tracking, xpub announce events, and security hardening - Derive node identity key from signing share secret, not just public data - Add verify_peer_share_index to handle_descriptor_propose - Cap seen_xpub_announces at 10k entries with eviction - Validate network parameter in request_descriptor - Zeroize intermediate hash in derive_keys_from_share - Return bool from add_ack to prevent duplicate event emission - Only increment acks_received for initiator nodes --- keep-desktop/src/app.rs | 13 +++++++-- keep-desktop/src/frost.rs | 2 ++ keep-desktop/src/message.rs | 6 +++-- keep-frost-net/src/descriptor_session.rs | 7 ++--- keep-frost-net/src/node/descriptor.rs | 34 ++++++++++++++---------- keep-frost-net/src/node/mod.rs | 11 ++++++-- 6 files changed, 50 insertions(+), 23 deletions(-) diff --git a/keep-desktop/src/app.rs b/keep-desktop/src/app.rs index ab747444..fc0b8f57 100644 --- a/keep-desktop/src/app.rs +++ b/keep-desktop/src/app.rs @@ -108,6 +108,8 @@ pub struct Toast { pub(crate) struct ActiveCoordination { pub group_pubkey: [u8; 32], pub network: String, + pub expected_participants: usize, + pub acks_received: usize, } pub struct App { @@ -1306,7 +1308,7 @@ impl App { Message::WalletBeginCoordination => self.begin_descriptor_coordination(), Message::WalletSessionStarted(result) => { match result { - Ok((session_id, group_pubkey, network)) => { + Ok((session_id, group_pubkey, network, expected_participants)) => { if let Screen::Wallet(WalletScreen { setup: Some(s), .. }) = &mut self.screen { @@ -1315,6 +1317,8 @@ impl App { ActiveCoordination { group_pubkey, network, + expected_participants, + acks_received: 0, }, ); s.session_id = Some(session_id); @@ -1472,7 +1476,12 @@ impl App { .await .map_err(|e| format!("{e}"))?; - Ok::<([u8; 32], [u8; 32], String), String>((session_id, group_pubkey, net)) + Ok::<([u8; 32], [u8; 32], String, usize), String>(( + session_id, + group_pubkey, + net, + expected_total, + )) }, Message::WalletSessionStarted, ) diff --git a/keep-desktop/src/frost.rs b/keep-desktop/src/frost.rs index 36f63af4..a86b9202 100644 --- a/keep-desktop/src/frost.rs +++ b/keep-desktop/src/frost.rs @@ -824,6 +824,8 @@ impl App { ActiveCoordination { group_pubkey: share.group_pubkey, network: network.clone(), + expected_participants: 0, + acks_received: 0, }, ); diff --git a/keep-desktop/src/message.rs b/keep-desktop/src/message.rs index e418edd4..e22beaca 100644 --- a/keep-desktop/src/message.rs +++ b/keep-desktop/src/message.rs @@ -7,6 +7,8 @@ use zeroize::Zeroizing; use crate::screen::shares::ShareEntry; use crate::screen::signing_audit::AuditDisplayEntry; +use keep_frost_net::AnnouncedXpub; + use crate::screen::wallet::{DescriptorProgress, WalletEntry}; #[derive(Clone, Debug, PartialEq)] @@ -198,7 +200,7 @@ pub enum Message { WalletRemoveTier(usize), WalletBeginCoordination, WalletCancelSetup, - WalletSessionStarted(Result<([u8; 32], [u8; 32], String), String>), + WalletSessionStarted(Result<([u8; 32], [u8; 32], String, usize), String>), WalletDescriptorProgress(DescriptorProgress, Option<[u8; 32]>), // Relay / FROST RelayUrlChanged(String), @@ -320,7 +322,7 @@ pub enum FrostNodeMsg { }, XpubAnnounced { share_index: u16, - recovery_xpubs: Vec, + recovery_xpubs: Vec, }, } diff --git a/keep-frost-net/src/descriptor_session.rs b/keep-frost-net/src/descriptor_session.rs index 5201e626..c2534a41 100644 --- a/keep-frost-net/src/descriptor_session.rs +++ b/keep-frost-net/src/descriptor_session.rs @@ -241,12 +241,13 @@ impl DescriptorSession { Ok(()) } + /// Returns `Ok(true)` when the ack was new, `Ok(false)` for duplicates. pub fn add_ack( &mut self, share_index: u16, descriptor_hash: [u8; 32], key_proof_psbt: &[u8], - ) -> Result<()> { + ) -> Result { if self.state != DescriptorSessionState::Finalized { return Err(FrostNetError::Session("Not accepting ACKs".into())); } @@ -258,7 +259,7 @@ impl DescriptorSession { } if self.acks.contains(&share_index) { - return Ok(()); + return Ok(false); } let finalized = self @@ -295,7 +296,7 @@ impl DescriptorSession { self.state = DescriptorSessionState::Complete; } - Ok(()) + Ok(true) } pub fn has_all_acks(&self) -> bool { diff --git a/keep-frost-net/src/node/descriptor.rs b/keep-frost-net/src/node/descriptor.rs index 84d6525b..c40b177d 100644 --- a/keep-frost-net/src/node/descriptor.rs +++ b/keep-frost-net/src/node/descriptor.rs @@ -821,18 +821,19 @@ impl KfpNode { .ok_or_else(|| FrostNetError::UntrustedPeer(sender.to_string()))? }; - let (is_complete, ack_count, expected_acks) = { + let (is_new, is_complete, ack_count, expected_acks) = { let mut sessions = self.descriptor_sessions.write(); let session = sessions .get_session_mut(&payload.session_id) .ok_or_else(|| FrostNetError::Session("unknown descriptor session".into()))?; - session.add_ack( + let is_new = session.add_ack( share_index, payload.descriptor_hash, &payload.key_proof_psbt, )?; ( + is_new, session.is_complete(), session.ack_count(), session.expected_ack_count(), @@ -848,12 +849,14 @@ impl KfpNode { "Received descriptor ACK" ); - let _ = self.event_tx.send(KfpNodeEvent::DescriptorAcked { - session_id: payload.session_id, - share_index, - ack_count, - expected_acks, - }); + if is_new { + let _ = self.event_tx.send(KfpNodeEvent::DescriptorAcked { + session_id: payload.session_id, + share_index, + ack_count, + expected_acks, + }); + } if is_complete { let sessions = self.descriptor_sessions.read(); @@ -928,16 +931,19 @@ impl KfpNode { let digest: [u8; 32] = hasher.finalize().into(); let dedup_key = (payload.share_index, payload.created_at, digest); let mut seen = self.seen_xpub_announces.write(); - if seen.contains(&dedup_key) { + if !seen.insert(dedup_key) { return Ok(()); } - if seen.len() >= 10_000 { - tracing::warn!("seen_xpub_announces at capacity, evicting oldest entry"); - if let Some(&oldest) = seen.iter().min_by_key(|&(_, ts, _)| ts) { - seen.remove(&oldest); + const MAX_SEEN_XPUB_ANNOUNCES: usize = 10_000; + if seen.len() > MAX_SEEN_XPUB_ANNOUNCES { + let now = chrono::Utc::now().timestamp().max(0) as u64; + let window = self.replay_window_secs + super::MAX_FUTURE_SKEW_SECS; + seen.retain(|&(_, ts, _)| now.saturating_sub(window) <= ts); + if seen.len() > MAX_SEEN_XPUB_ANNOUNCES { + seen.clear(); + seen.insert(dedup_key); } } - seen.insert(dedup_key); } { diff --git a/keep-frost-net/src/node/mod.rs b/keep-frost-net/src/node/mod.rs index 547b6145..a4f3bb22 100644 --- a/keep-frost-net/src/node/mod.rs +++ b/keep-frost-net/src/node/mod.rs @@ -1163,12 +1163,19 @@ impl KfpNode { } fn derive_keys_from_share(share: &SharePackage) -> Result { + let key_package = share + .key_package() + .map_err(|e| FrostNetError::Crypto(format!("Failed to get key package: {e}")))?; + let signing_share = key_package.signing_share(); + let signing_share_bytes = signing_share.serialize(); + let mut hasher = Sha256::new(); hasher.update(b"keep-frost-node-identity-v2"); hasher.update(share.metadata.group_pubkey); hasher.update(share.metadata.identifier.to_be_bytes()); - let derived: [u8; 32] = hasher.finalize().into(); - let secret_key = SecretKey::from_slice(&derived) + hasher.update(signing_share_bytes.as_slice()); + let derived: Zeroizing<[u8; 32]> = Zeroizing::new(hasher.finalize().into()); + let secret_key = SecretKey::from_slice(&*derived) .map_err(|e| FrostNetError::Crypto(format!("Failed to create secret key: {e}")))?; Ok(Keys::new(secret_key)) } From 84e71c790b9bafdb5de1094440c3208d2894db6c Mon Sep 17 00:00:00 2001 From: "William K. Santiago" Date: Thu, 26 Feb 2026 11:03:52 -0500 Subject: [PATCH 02/13] Wire desktop descriptor auto-finalize on DescriptorReady --- keep-desktop/src/app.rs | 11 +++++++++++ keep-desktop/src/frost.rs | 1 + keep-desktop/src/message.rs | 9 +++++++-- keep-frost-net/src/node/mod.rs | 3 +-- 4 files changed, 20 insertions(+), 4 deletions(-) diff --git a/keep-desktop/src/app.rs b/keep-desktop/src/app.rs index fc0b8f57..21f6c2f1 100644 --- a/keep-desktop/src/app.rs +++ b/keep-desktop/src/app.rs @@ -696,6 +696,7 @@ impl App { | Message::WalletBeginCoordination | Message::WalletCancelSetup | Message::WalletSessionStarted(..) + | Message::WalletFinalizeResult(..) | Message::WalletDescriptorProgress(..) => self.handle_wallet_message(message), Message::RelayUrlChanged(..) @@ -1351,6 +1352,16 @@ impl App { } Task::none() } + Message::WalletFinalizeResult(result, session_id) => { + if let Err(e) = result { + self.active_coordinations.remove(&session_id); + self.update_wallet_setup(&session_id, |setup| { + setup.phase = + SetupPhase::Coordinating(DescriptorProgress::Failed(e)); + }); + } + Task::none() + } Message::WalletDescriptorProgress(progress, session_id) => { if let Some(sid) = session_id { if matches!(progress, DescriptorProgress::Failed(_)) { diff --git a/keep-desktop/src/frost.rs b/keep-desktop/src/frost.rs index a86b9202..ce95338a 100644 --- a/keep-desktop/src/frost.rs +++ b/keep-desktop/src/frost.rs @@ -928,6 +928,7 @@ impl App { self.frost_status = ConnectionStatus::Disconnected; self.frost_peers.clear(); self.pending_sign_display.clear(); + self.active_coordinations.clear(); self.frost_reconnect_attempts = 0; self.frost_reconnect_at = None; if let Ok(mut guard) = self.frost_node.lock() { diff --git a/keep-desktop/src/message.rs b/keep-desktop/src/message.rs index e22beaca..8d802953 100644 --- a/keep-desktop/src/message.rs +++ b/keep-desktop/src/message.rs @@ -3,12 +3,11 @@ use std::fmt; +use keep_frost_net::AnnouncedXpub; use zeroize::Zeroizing; use crate::screen::shares::ShareEntry; use crate::screen::signing_audit::AuditDisplayEntry; -use keep_frost_net::AnnouncedXpub; - use crate::screen::wallet::{DescriptorProgress, WalletEntry}; #[derive(Clone, Debug, PartialEq)] @@ -201,6 +200,7 @@ pub enum Message { WalletBeginCoordination, WalletCancelSetup, WalletSessionStarted(Result<([u8; 32], [u8; 32], String, usize), String>), + WalletFinalizeResult(Result<(), String>, [u8; 32]), WalletDescriptorProgress(DescriptorProgress, Option<[u8; 32]>), // Relay / FROST RelayUrlChanged(String), @@ -503,6 +503,11 @@ impl fmt::Debug for Message { .debug_tuple("WalletSessionStarted") .field(&r.as_ref().map(|_| "ok").map_err(|e| e.as_str())) .finish(), + Self::WalletFinalizeResult(r, sid) => f + .debug_struct("WalletFinalizeResult") + .field("result", &r.as_ref().map(|_| "ok").map_err(|e| e.as_str())) + .field("session_id", &hex::encode(sid)) + .finish(), Self::WalletDescriptorProgress(..) => f.write_str("WalletDescriptorProgress"), Self::RelayUrlChanged(u) => f.debug_tuple("RelayUrlChanged").field(u).finish(), Self::ConnectPasswordChanged(_) => f.write_str("ConnectPasswordChanged(***)"), diff --git a/keep-frost-net/src/node/mod.rs b/keep-frost-net/src/node/mod.rs index a4f3bb22..94221083 100644 --- a/keep-frost-net/src/node/mod.rs +++ b/keep-frost-net/src/node/mod.rs @@ -1166,8 +1166,7 @@ fn derive_keys_from_share(share: &SharePackage) -> Result { let key_package = share .key_package() .map_err(|e| FrostNetError::Crypto(format!("Failed to get key package: {e}")))?; - let signing_share = key_package.signing_share(); - let signing_share_bytes = signing_share.serialize(); + let signing_share_bytes = key_package.signing_share().serialize(); let mut hasher = Sha256::new(); hasher.update(b"keep-frost-node-identity-v2"); From 06cfce7d34754ba877d1c4097544555bc2f70c0f Mon Sep 17 00:00:00 2001 From: "William K. Santiago" Date: Thu, 26 Feb 2026 11:12:14 -0500 Subject: [PATCH 03/13] fix: address security and code quality issues in coordination flow --- keep-desktop/src/app.rs | 27 +++++++++++++++++---------- keep-desktop/src/frost.rs | 23 +++++++++++++++++++++-- keep-desktop/src/screen/wallet.rs | 4 +++- 3 files changed, 41 insertions(+), 13 deletions(-) diff --git a/keep-desktop/src/app.rs b/keep-desktop/src/app.rs index 21f6c2f1..c2b56760 100644 --- a/keep-desktop/src/app.rs +++ b/keep-desktop/src/app.rs @@ -83,6 +83,7 @@ pub(crate) const RECONNECT_MAX_MS: u64 = 30_000; pub(crate) const RECONNECT_MAX_ATTEMPTS: u32 = 10; pub(crate) const BUNKER_APPROVAL_TIMEOUT: Duration = Duration::from_secs(60); pub(crate) const MAX_BUNKER_LOG_ENTRIES: usize = 1000; +pub(crate) const MAX_ACTIVE_COORDINATIONS: usize = 64; const DEFAULT_BUNKER_RELAYS: &[&str] = &["wss://relay.damus.io", "wss://relay.nsec.app"]; @@ -1313,16 +1314,22 @@ impl App { if let Screen::Wallet(WalletScreen { setup: Some(s), .. }) = &mut self.screen { - self.active_coordinations.insert( - session_id, - ActiveCoordination { - group_pubkey, - network, - expected_participants, - acks_received: 0, - }, - ); - s.session_id = Some(session_id); + if self.active_coordinations.len() >= MAX_ACTIVE_COORDINATIONS { + if let Some(node) = self.get_frost_node() { + node.cancel_descriptor_session(&session_id); + } + } else { + self.active_coordinations.insert( + session_id, + ActiveCoordination { + group_pubkey, + network, + expected_participants, + acks_received: 0, + }, + ); + s.session_id = Some(session_id); + } } else if let Some(node) = self.get_frost_node() { node.cancel_descriptor_session(&session_id); } diff --git a/keep-desktop/src/frost.rs b/keep-desktop/src/frost.rs index ce95338a..e7c7d2a6 100644 --- a/keep-desktop/src/frost.rs +++ b/keep-desktop/src/frost.rs @@ -16,8 +16,9 @@ use keep_core::Keep; use crate::app::{ friendly_err, lock_keep, with_keep_blocking, ActiveCoordination, App, ToastKind, - MAX_PENDING_REQUESTS, MAX_REQUESTS_PER_PEER, RATE_LIMIT_GLOBAL, RATE_LIMIT_PER_PEER, - RATE_LIMIT_WINDOW_SECS, RECONNECT_BASE_MS, RECONNECT_MAX_ATTEMPTS, RECONNECT_MAX_MS, + MAX_ACTIVE_COORDINATIONS, MAX_PENDING_REQUESTS, MAX_REQUESTS_PER_PEER, RATE_LIMIT_GLOBAL, + RATE_LIMIT_PER_PEER, RATE_LIMIT_WINDOW_SECS, RECONNECT_BASE_MS, RECONNECT_MAX_ATTEMPTS, + RECONNECT_MAX_MS, SIGNING_RESPONSE_TIMEOUT, }; use crate::message::{ConnectionStatus, FrostNodeMsg, Message, PeerEntry, PendingSignRequest}; @@ -705,6 +706,14 @@ impl App { ); } FrostNodeMsg::DescriptorReady { session_id } => { + let is_initiator = self + .active_coordinations + .get(&session_id) + .map(|c| c.expected_participants > 0) + .unwrap_or(false); + if !is_initiator { + return iced::Task::none(); + } self.update_wallet_setup(&session_id, |setup| { setup.phase = SetupPhase::Coordinating(DescriptorProgress::Finalizing); }); @@ -732,6 +741,12 @@ impl App { ), }, ); + } else { + self.update_wallet_setup(&session_id, |setup| { + setup.phase = SetupPhase::Coordinating(DescriptorProgress::Failed( + "Node unavailable".to_string(), + )); + }); } } FrostNodeMsg::DescriptorContributed { session_id, .. } => { @@ -819,6 +834,10 @@ impl App { return iced::Task::none(); } + if self.active_coordinations.len() >= MAX_ACTIVE_COORDINATIONS { + return iced::Task::none(); + } + self.active_coordinations.insert( session_id, ActiveCoordination { diff --git a/keep-desktop/src/screen/wallet.rs b/keep-desktop/src/screen/wallet.rs index aab3811f..ab82fb9c 100644 --- a/keep-desktop/src/screen/wallet.rs +++ b/keep-desktop/src/screen/wallet.rs @@ -449,7 +449,9 @@ impl WalletScreen { .size(theme::size::TINY) .color(theme::color::TEXT_DIM); - let created = DateTime::::from_timestamp(entry.created_at as i64, 0) + let created = i64::try_from(entry.created_at) + .ok() + .and_then(|ts| DateTime::::from_timestamp(ts, 0)) .map(|dt| dt.format("%Y-%m-%d %H:%M UTC").to_string()) .unwrap_or_else(|| entry.created_at.to_string()); let created_text = text(format!("Created: {created}")) From ca2e731cf45a408154ead9542e37d584e5fcc35c Mon Sep 17 00:00:00 2001 From: "William K. Santiago" Date: Thu, 26 Feb 2026 11:17:35 -0500 Subject: [PATCH 04/13] Simplify coordination flow and fix borrow checker issue --- keep-desktop/src/app.rs | 53 +++++++++++++++++++++++++------------- keep-desktop/src/frost.rs | 54 +++++++++++++++++++-------------------- 2 files changed, 61 insertions(+), 46 deletions(-) diff --git a/keep-desktop/src/app.rs b/keep-desktop/src/app.rs index c2b56760..1c6aaa0c 100644 --- a/keep-desktop/src/app.rs +++ b/keep-desktop/src/app.rs @@ -109,6 +109,7 @@ pub struct Toast { pub(crate) struct ActiveCoordination { pub group_pubkey: [u8; 32], pub network: String, + pub is_initiator: bool, pub expected_participants: usize, pub acks_received: usize, } @@ -1311,27 +1312,43 @@ impl App { Message::WalletSessionStarted(result) => { match result { Ok((session_id, group_pubkey, network, expected_participants)) => { - if let Screen::Wallet(WalletScreen { setup: Some(s), .. }) = - &mut self.screen - { - if self.active_coordinations.len() >= MAX_ACTIVE_COORDINATIONS { - if let Some(node) = self.get_frost_node() { - node.cancel_descriptor_session(&session_id); - } - } else { - self.active_coordinations.insert( - session_id, - ActiveCoordination { - group_pubkey, - network, - expected_participants, - acks_received: 0, - }, + let on_wallet_screen = matches!( + self.screen, + Screen::Wallet(WalletScreen { setup: Some(_), .. }) + ); + if !on_wallet_screen { + if let Some(node) = self.get_frost_node() { + node.cancel_descriptor_session(&session_id); + } + } else if self.active_coordinations.len() >= MAX_ACTIVE_COORDINATIONS { + if let Some(node) = self.get_frost_node() { + node.cancel_descriptor_session(&session_id); + } + if let Screen::Wallet(WalletScreen { setup: Some(s), .. }) = + &mut self.screen + { + s.phase = SetupPhase::Coordinating( + DescriptorProgress::Failed( + "Too many active coordinations".to_string(), + ), ); + } + } else { + self.active_coordinations.insert( + session_id, + ActiveCoordination { + group_pubkey, + network, + is_initiator: true, + expected_participants, + acks_received: 0, + }, + ); + if let Screen::Wallet(WalletScreen { setup: Some(s), .. }) = + &mut self.screen + { s.session_id = Some(session_id); } - } else if let Some(node) = self.get_frost_node() { - node.cancel_descriptor_session(&session_id); } } Err(e) => { diff --git a/keep-desktop/src/frost.rs b/keep-desktop/src/frost.rs index e7c7d2a6..1eda9da8 100644 --- a/keep-desktop/src/frost.rs +++ b/keep-desktop/src/frost.rs @@ -709,45 +709,41 @@ impl App { let is_initiator = self .active_coordinations .get(&session_id) - .map(|c| c.expected_participants > 0) - .unwrap_or(false); + .is_some_and(|c| c.is_initiator); if !is_initiator { return iced::Task::none(); } self.update_wallet_setup(&session_id, |setup| { setup.phase = SetupPhase::Coordinating(DescriptorProgress::Finalizing); }); - if self.active_coordinations.contains_key(&session_id) { - let Some(node) = self.get_frost_node() else { - return iced::Task::none(); - }; - return iced::Task::perform( - async move { - node.build_and_finalize_descriptor(session_id) - .await - .map_err(|e| format!("{e}")) - }, - move |result| match result { - Ok(expected_acks) => Message::WalletDescriptorProgress( - DescriptorProgress::WaitingAcks { - received: 0, - expected: expected_acks, - }, - Some(session_id), - ), - Err(e) => Message::WalletDescriptorProgress( - DescriptorProgress::Failed(e), - Some(session_id), - ), - }, - ); - } else { + let Some(node) = self.get_frost_node() else { self.update_wallet_setup(&session_id, |setup| { setup.phase = SetupPhase::Coordinating(DescriptorProgress::Failed( "Node unavailable".to_string(), )); }); - } + return iced::Task::none(); + }; + return iced::Task::perform( + async move { + node.build_and_finalize_descriptor(session_id) + .await + .map_err(|e| format!("{e}")) + }, + move |result| match result { + Ok(expected_acks) => Message::WalletDescriptorProgress( + DescriptorProgress::WaitingAcks { + received: 0, + expected: expected_acks, + }, + Some(session_id), + ), + Err(e) => Message::WalletDescriptorProgress( + DescriptorProgress::Failed(e), + Some(session_id), + ), + }, + ); } FrostNodeMsg::DescriptorContributed { session_id, .. } => { self.update_wallet_setup(&session_id, |setup| { @@ -835,6 +831,7 @@ impl App { } if self.active_coordinations.len() >= MAX_ACTIVE_COORDINATIONS { + tracing::warn!("Dropping descriptor contribution: too many active coordinations"); return iced::Task::none(); } @@ -843,6 +840,7 @@ impl App { ActiveCoordination { group_pubkey: share.group_pubkey, network: network.clone(), + is_initiator: false, expected_participants: 0, acks_received: 0, }, From fadf093ececc5c16c156e03292a58ff685982d79 Mon Sep 17 00:00:00 2001 From: "William K. Santiago" Date: Thu, 26 Feb 2026 11:55:18 -0500 Subject: [PATCH 05/13] Remove redundant peer lookup and fix formatting --- keep-desktop/src/app.rs | 11 ++++------- keep-desktop/src/frost.rs | 3 +-- 2 files changed, 5 insertions(+), 9 deletions(-) diff --git a/keep-desktop/src/app.rs b/keep-desktop/src/app.rs index 1c6aaa0c..f97599c4 100644 --- a/keep-desktop/src/app.rs +++ b/keep-desktop/src/app.rs @@ -1327,11 +1327,9 @@ impl App { if let Screen::Wallet(WalletScreen { setup: Some(s), .. }) = &mut self.screen { - s.phase = SetupPhase::Coordinating( - DescriptorProgress::Failed( - "Too many active coordinations".to_string(), - ), - ); + s.phase = SetupPhase::Coordinating(DescriptorProgress::Failed( + "Too many active coordinations".to_string(), + )); } } else { self.active_coordinations.insert( @@ -1380,8 +1378,7 @@ impl App { if let Err(e) = result { self.active_coordinations.remove(&session_id); self.update_wallet_setup(&session_id, |setup| { - setup.phase = - SetupPhase::Coordinating(DescriptorProgress::Failed(e)); + setup.phase = SetupPhase::Coordinating(DescriptorProgress::Failed(e)); }); } Task::none() diff --git a/keep-desktop/src/frost.rs b/keep-desktop/src/frost.rs index 1eda9da8..b20dfa6c 100644 --- a/keep-desktop/src/frost.rs +++ b/keep-desktop/src/frost.rs @@ -18,8 +18,7 @@ use crate::app::{ friendly_err, lock_keep, with_keep_blocking, ActiveCoordination, App, ToastKind, MAX_ACTIVE_COORDINATIONS, MAX_PENDING_REQUESTS, MAX_REQUESTS_PER_PEER, RATE_LIMIT_GLOBAL, RATE_LIMIT_PER_PEER, RATE_LIMIT_WINDOW_SECS, RECONNECT_BASE_MS, RECONNECT_MAX_ATTEMPTS, - RECONNECT_MAX_MS, - SIGNING_RESPONSE_TIMEOUT, + RECONNECT_MAX_MS, SIGNING_RESPONSE_TIMEOUT, }; use crate::message::{ConnectionStatus, FrostNodeMsg, Message, PeerEntry, PendingSignRequest}; use crate::screen::relay::RelayScreen; From e64292e44e7e740f28b37b7e757b6a15b953439a Mon Sep 17 00:00:00 2001 From: "William K. Santiago" Date: Thu, 26 Feb 2026 14:04:24 -0500 Subject: [PATCH 06/13] fix: address security and code quality review findings - Validate relay URLs before adding in switch_relays - Add require_approval to nip44/nip04 encrypt handlers - Use constant-time comparison for descriptor hash - Wrap secret keys in Zeroizing in Python/TypeScript bindings - Zeroize secret key copies after use in sign methods - Correlate response ID in send_request - Remove double-sleep in wait_for_approval - Send connect request only once across poll iterations - Finalize descriptor locally before sending ACK - Use hex instead of bech32 in approval_url - Compute expected_acks from all contributors, not just online peers --- keep-agent-py/Cargo.toml | 1 + keep-agent-py/src/lib.rs | 35 ++++-- keep-agent-ts/Cargo.toml | 1 + keep-agent-ts/src/lib.rs | 39 ++++-- keep-agent/src/client.rs | 149 +++++++++++++++++++++-- keep-frost-net/src/descriptor_session.rs | 3 +- keep-frost-net/src/node/descriptor.rs | 24 ++-- keep-nip46/src/audit.rs | 2 + keep-nip46/src/handler.rs | 11 +- 9 files changed, 212 insertions(+), 53 deletions(-) diff --git a/keep-agent-py/Cargo.toml b/keep-agent-py/Cargo.toml index 484b1bd5..23d410b7 100644 --- a/keep-agent-py/Cargo.toml +++ b/keep-agent-py/Cargo.toml @@ -19,3 +19,4 @@ serde_json = "1.0" hex = "0.4" nostr-sdk = { version = "0.44", features = ["nip44"] } k256 = { version = "0.13", features = ["schnorr"] } +zeroize = "1" diff --git a/keep-agent-py/src/lib.rs b/keep-agent-py/src/lib.rs index e3c1acdb..708f96ab 100644 --- a/keep-agent-py/src/lib.rs +++ b/keep-agent-py/src/lib.rs @@ -7,6 +7,7 @@ use pyo3::prelude::*; use pyo3::exceptions::{PyRuntimeError, PyValueError, PyConnectionError}; use std::sync::Arc; use tokio::sync::Mutex; +use zeroize::{Zeroize, Zeroizing}; use ::keep_agent::{ RateLimitConfig, SessionConfig, SessionManager, SessionMetadata, @@ -155,7 +156,7 @@ pub struct PyAgentSession { manager: SessionManager, token: SessionToken, session_id: String, - secret_key: Option<[u8; 32]>, + secret_key: Option>, } #[pymethods] @@ -169,7 +170,7 @@ impl PyAgentSession { policy: Option, secret_key: Option, ) -> PyResult { - let secret_bytes: Option<[u8; 32]> = if let Some(sk) = secret_key { + let secret_bytes: Option> = if let Some(sk) = secret_key { let decoded = hex::decode(&sk) .map_err(|e| PyValueError::new_err(format!("Invalid secret key hex: {}", e)))?; if decoded.len() != 32 { @@ -180,14 +181,14 @@ impl PyAgentSession { } let mut arr = [0u8; 32]; arr.copy_from_slice(&decoded); - Some(arr) + Some(Zeroizing::new(arr)) } else { None }; let pubkey_bytes: [u8; 32] = if let Some(ref sk) = secret_bytes { use k256::elliptic_curve::sec1::ToEncodedPoint; - let scalar = k256::NonZeroScalar::try_from(sk.as_slice()) + let scalar = k256::NonZeroScalar::try_from(sk.as_ref().as_slice()) .map_err(|_| PyValueError::new_err("Invalid secret key"))?; let pk = k256::PublicKey::from_secret_scalar(&scalar); let point = pk.to_encoded_point(true); @@ -300,14 +301,14 @@ impl PyAgentSession { session.check_operation(&Operation::SignNostrEvent).map_err(to_py_err)?; session.check_event_kind(kind).map_err(to_py_err)?; - let secret = self.secret_key + let secret = self.secret_key.as_ref() .ok_or_else(|| PyRuntimeError::new_err("No secret key configured. Pass secret_key to constructor."))?; self.manager.record_request(&self.session_id).map_err(to_py_err)?; use nostr_sdk::prelude::*; - let keys = Keys::parse(&hex::encode(secret)) + let keys = Keys::parse(&hex::encode(secret.as_ref())) .map_err(to_py_value_err)?; let mut nostr_tags: Vec = Vec::new(); @@ -358,7 +359,7 @@ impl PyAgentSession { session.check_operation(&Operation::SignPsbt).map_err(to_py_err)?; - let mut secret = self.secret_key + let mut secret = *self.secret_key.as_ref() .ok_or_else(|| PyRuntimeError::new_err("No secret key configured. Pass secret_key to constructor."))?; let network = match network.unwrap_or("testnet") { @@ -402,13 +403,15 @@ impl PyAgentSession { self.manager.record_request(&self.session_id).map_err(to_py_err)?; - signer.sign_psbt(&mut psbt).map_err(to_py_err)?; + let result = signer.sign_psbt(&mut psbt).map_err(to_py_err); + secret.zeroize(); + result?; Ok(keep_bitcoin::psbt::serialize_psbt_base64(&psbt)) } fn get_public_key(&self) -> PyResult { - let _ = self.secret_key + let _ = self.secret_key.as_ref() .ok_or_else(|| PyRuntimeError::new_err("No secret key configured"))?; let session = self.manager @@ -429,7 +432,7 @@ impl PyAgentSession { session.check_operation(&Operation::GetBitcoinAddress).map_err(to_py_err)?; - let mut secret = self.secret_key + let mut secret = *self.secret_key.as_ref() .ok_or_else(|| PyRuntimeError::new_err("No secret key configured. Pass secret_key to constructor."))?; let network = match network.unwrap_or("testnet") { @@ -442,7 +445,9 @@ impl PyAgentSession { let signer = keep_bitcoin::BitcoinSigner::new(&mut secret, network) .map_err(to_py_err)?; - signer.get_receive_address(0).map_err(to_py_err) + let result = signer.get_receive_address(0).map_err(to_py_err); + secret.zeroize(); + result } fn __enter__(slf: Py) -> Py { @@ -524,6 +529,14 @@ impl PyRemoteSession { }).map_err(to_py_err) } + fn switch_relays(&self) -> PyResult>> { + let client = self.client.clone(); + self.runtime.block_on(async { + let mut c = client.lock().await; + c.switch_relays().await + }).map_err(to_py_err) + } + fn disconnect(&self) -> PyResult<()> { let client = self.client.clone(); self.runtime.block_on(async { diff --git a/keep-agent-ts/Cargo.toml b/keep-agent-ts/Cargo.toml index 03d5439c..dfc6706d 100644 --- a/keep-agent-ts/Cargo.toml +++ b/keep-agent-ts/Cargo.toml @@ -19,6 +19,7 @@ serde_json = "1.0" nostr-sdk = "0.44" k256 = { version = "0.13", features = ["schnorr"] } hex = "0.4" +zeroize = "1" [build-dependencies] napi-build = "2" diff --git a/keep-agent-ts/src/lib.rs b/keep-agent-ts/src/lib.rs index 8f63c318..e14fd044 100644 --- a/keep-agent-ts/src/lib.rs +++ b/keep-agent-ts/src/lib.rs @@ -7,6 +7,7 @@ use napi::bindgen_prelude::*; use napi_derive::napi; use std::sync::Arc; use tokio::sync::Mutex; +use zeroize::{Zeroize, Zeroizing}; use keep_agent::{ AgentClient, ApprovalStatus, Operation, PendingSession as RustPendingSession, @@ -53,7 +54,7 @@ pub struct KeepAgentSession { manager: SessionManager, token: Arc>, session_id: String, - secret_key: Option<[u8; 32]>, + secret_key: Option>, } #[napi] @@ -66,7 +67,7 @@ impl KeepAgentSession { policy: Option, secret_key: Option, ) -> Result { - let secret_bytes: Option<[u8; 32]> = if let Some(ref sk) = secret_key { + let secret_bytes: Option> = if let Some(ref sk) = secret_key { let decoded = hex::decode(sk) .map_err(|e| Error::from_reason(format!("Invalid secret key hex: {}", e)))?; if decoded.len() != 32 { @@ -77,14 +78,14 @@ impl KeepAgentSession { } let mut arr = [0u8; 32]; arr.copy_from_slice(&decoded); - Some(arr) + Some(Zeroizing::new(arr)) } else { None }; let pubkey: [u8; 32] = if let Some(ref sk) = secret_bytes { use k256::elliptic_curve::sec1::ToEncodedPoint; - let scalar = k256::NonZeroScalar::try_from(sk.as_slice()) + let scalar = k256::NonZeroScalar::try_from(sk.as_ref().as_slice()) .map_err(|_| Error::from_reason("Invalid secret key"))?; let pk = k256::PublicKey::from_secret_scalar(&scalar); let point = pk.to_encoded_point(true); @@ -304,6 +305,7 @@ impl KeepAgentSession { let secret = self .secret_key + .as_ref() .ok_or_else(|| Error::from_reason("No secret key configured"))?; self.manager @@ -312,7 +314,7 @@ impl KeepAgentSession { use nostr_sdk::prelude::{EventBuilder, Keys, Kind, Tag}; - let keys = Keys::parse(&hex::encode(secret)) + let keys = Keys::parse(&hex::encode(secret.as_ref())) .map_err(|e| napi::Error::from_reason(e.to_string()))?; let nostr_tags: Vec = tags @@ -365,8 +367,9 @@ impl KeepAgentSession { .check_operation(&Operation::SignPsbt) .map_err(|e| Error::from_reason(e.to_string()))?; - let mut secret = self + let mut secret = *self .secret_key + .as_ref() .ok_or_else(|| Error::from_reason("No secret key configured"))?; let network = match network.as_deref().unwrap_or("testnet") { @@ -414,9 +417,9 @@ impl KeepAgentSession { .record_request(&self.session_id) .map_err(|e| Error::from_reason(e.to_string()))?; - signer - .sign_psbt(&mut psbt) - .map_err(|e| Error::from_reason(e.to_string()))?; + let result = signer.sign_psbt(&mut psbt).map_err(|e| Error::from_reason(e.to_string())); + secret.zeroize(); + result?; Ok(keep_bitcoin::psbt::serialize_psbt_base64(&psbt)) } @@ -449,8 +452,9 @@ impl KeepAgentSession { .check_operation(&Operation::GetBitcoinAddress) .map_err(|e| Error::from_reason(e.to_string()))?; - let mut secret = self + let mut secret = *self .secret_key + .as_ref() .ok_or_else(|| Error::from_reason("No secret key configured"))?; let network = match network.as_deref().unwrap_or("testnet") { @@ -463,9 +467,11 @@ impl KeepAgentSession { let signer = keep_bitcoin::BitcoinSigner::new(&mut secret, network) .map_err(|e| Error::from_reason(e.to_string()))?; - signer + let result = signer .get_receive_address(0) - .map_err(|e| Error::from_reason(e.to_string())) + .map_err(|e| Error::from_reason(e.to_string())); + secret.zeroize(); + result } } @@ -534,6 +540,15 @@ impl RemoteSession { .map_err(|e| Error::from_reason(e.to_string())) } + #[napi] + pub async fn switch_relays(&self) -> Result>> { + let mut client = self.client.lock().await; + client + .switch_relays() + .await + .map_err(|e| Error::from_reason(e.to_string())) + } + #[napi] pub async fn disconnect(&self) -> Result<()> { let client = self.client.lock().await; diff --git a/keep-agent/src/client.rs b/keep-agent/src/client.rs index e0428b72..fc7314c2 100644 --- a/keep-agent/src/client.rs +++ b/keep-agent/src/client.rs @@ -19,6 +19,7 @@ pub struct PendingSession { relay_url: String, client_keys: Keys, client: Client, + connect_sent: std::sync::atomic::AtomicBool, } impl PendingSession { @@ -56,6 +57,7 @@ impl PendingSession { relay_url, client_keys, client, + connect_sent: std::sync::atomic::AtomicBool::new(false), }) } @@ -67,16 +69,20 @@ impl PendingSession { let encoded_relay = urlencoding::encode(&self.relay_url); format!( "nostrconnect://{}?relay={}&metadata={}", - self.client_keys - .public_key() - .to_bech32() - .unwrap_or_default(), + self.client_keys.public_key().to_hex(), encoded_relay, urlencoding::encode("{\"name\":\"Keep Agent\"}") ) } - pub async fn poll(&self, timeout: Duration) -> Result { + async fn send_connect_once(&self) -> Result<()> { + if self + .connect_sent + .swap(true, std::sync::atomic::Ordering::AcqRel) + { + return Ok(()); + } + let request = serde_json::json!({ "id": &self.request_id, "method": "connect", @@ -109,6 +115,12 @@ impl PendingSession { .await .map_err(|e| AgentError::Nostr(e.to_string()))?; + Ok(()) + } + + pub async fn poll(&self, timeout: Duration) -> Result { + self.send_connect_once().await?; + let filter = Filter::new() .kind(Kind::NostrConnect) .author(self.signer_pubkey) @@ -171,19 +183,19 @@ impl PendingSession { while start.elapsed() < timeout { match self.poll(poll_interval).await? { ApprovalStatus::Approved => { - return Ok(AgentClient { + let mut client = AgentClient { signer_pubkey: self.signer_pubkey, relay_url: self.relay_url.clone(), client_keys: self.client_keys.clone(), client: self.client.clone(), - }); + }; + let _ = client.switch_relays().await; + return Ok(client); } ApprovalStatus::Denied => { return Err(AgentError::AuthFailed("Session request denied".into())); } - ApprovalStatus::Pending => { - tokio::time::sleep(poll_interval).await; - } + ApprovalStatus::Pending => {} } } @@ -197,7 +209,6 @@ impl PendingSession { pub struct AgentClient { signer_pubkey: PublicKey, - #[allow(dead_code)] relay_url: String, client_keys: Keys, client: Client, @@ -230,7 +241,7 @@ impl AgentClient { .await .map_err(|_| AgentError::Connection("Relay connection timeout".into()))?; - let agent_client = Self { + let mut agent_client = Self { signer_pubkey, relay_url, client_keys, @@ -238,6 +249,7 @@ impl AgentClient { }; agent_client.send_connect(secret.as_deref()).await?; + let _ = agent_client.switch_relays().await; Ok(agent_client) } @@ -373,7 +385,78 @@ impl AgentClient { Ok(false) } + pub async fn switch_relays(&mut self) -> Result>> { + let request = serde_json::json!({ + "id": generate_uuid(), + "method": "switch_relays", + "params": [] + }); + + let response = self.send_request(&request.to_string()).await?; + let parsed: serde_json::Value = serde_json::from_str(&response) + .map_err(|e| AgentError::Serialization(e.to_string()))?; + + if let Some(error) = parsed.get("error") { + if !error.is_null() { + return Err(AgentError::Nostr(error.to_string())); + } + } + + let result = parsed + .get("result") + .ok_or_else(|| AgentError::Serialization("Missing result field".into()))?; + + if result.is_null() || (result.is_string() && result.as_str() == Some("null")) { + return Ok(None); + } + + let relays: Vec = if result.is_string() { + serde_json::from_str(result.as_str().unwrap()) + .map_err(|e| AgentError::Serialization(format!("Invalid relay list: {e}")))? + } else if result.is_array() { + serde_json::from_value(result.clone()) + .map_err(|e| AgentError::Serialization(format!("Invalid relay list: {e}")))? + } else { + return Err(AgentError::Serialization( + "Unexpected switch_relays result format".into(), + )); + }; + + if relays.is_empty() { + return Ok(None); + } + + let valid_relays: Vec = relays + .into_iter() + .filter(|r| keep_core::relay::validate_relay_url(r).is_ok()) + .collect(); + + if valid_relays.is_empty() { + return Ok(None); + } + + self.client.disconnect().await; + for relay in &valid_relays { + let _ = self.client.add_relay(relay).await; + } + self.client.connect().await; + let relays = valid_relays; + + self.relay_url = relays[0].clone(); + + Ok(Some(relays)) + } + async fn send_request(&self, content: &str) -> Result { + let request_id = { + let parsed: serde_json::Value = serde_json::from_str(content) + .map_err(|e| AgentError::Serialization(e.to_string()))?; + parsed + .get("id") + .and_then(|v| v.as_str()) + .map(|s| s.to_string()) + }; + let encrypted = nip44::encrypt( self.client_keys.secret_key(), &self.signer_pubkey, @@ -424,6 +507,17 @@ impl AgentClient { &self.signer_pubkey, &event.content, ) { + if let Some(ref expected_id) = request_id { + if let Ok(resp) = + serde_json::from_str::(&decrypted) + { + if resp.get("id").and_then(|v| v.as_str()) + != Some(expected_id) + { + continue; + } + } + } return Ok(decrypted); } } @@ -491,4 +585,35 @@ mod tests { assert_ne!(uuid1, uuid2); assert_eq!(uuid1.len(), 36); } + + #[test] + fn test_switch_relays_parse() { + let null_response = r#"{"id":"abc","result":null}"#; + let parsed: serde_json::Value = serde_json::from_str(null_response).unwrap(); + let result = parsed.get("result").unwrap(); + assert!(result.is_null()); + + let null_string_response = r#"{"id":"abc","result":"null"}"#; + let parsed: serde_json::Value = serde_json::from_str(null_string_response).unwrap(); + let result = parsed.get("result").unwrap(); + assert!(result.is_string() && result.as_str() == Some("null")); + + let array_response = + r#"{"id":"abc","result":["wss://relay1.example.com","wss://relay2.example.com"]}"#; + let parsed: serde_json::Value = serde_json::from_str(array_response).unwrap(); + let result = parsed.get("result").unwrap(); + assert!(result.is_array()); + let relays: Vec = serde_json::from_value(result.clone()).unwrap(); + assert_eq!(relays.len(), 2); + assert_eq!(relays[0], "wss://relay1.example.com"); + assert_eq!(relays[1], "wss://relay2.example.com"); + + let string_array_response = r#"{"id":"abc","result":"[\"wss://relay1.example.com\",\"wss://relay2.example.com\"]"}"#; + let parsed: serde_json::Value = serde_json::from_str(string_array_response).unwrap(); + let result = parsed.get("result").unwrap(); + assert!(result.is_string()); + let relays: Vec = serde_json::from_str(result.as_str().unwrap()).unwrap(); + assert_eq!(relays.len(), 2); + assert_eq!(relays[0], "wss://relay1.example.com"); + } } diff --git a/keep-frost-net/src/descriptor_session.rs b/keep-frost-net/src/descriptor_session.rs index c2534a41..dafddd54 100644 --- a/keep-frost-net/src/descriptor_session.rs +++ b/keep-frost-net/src/descriptor_session.rs @@ -8,6 +8,7 @@ use keep_bitcoin::recovery::{RecoveryConfig, RecoveryTier as BitcoinRecoveryTier use keep_bitcoin::{xpub_to_x_only, DescriptorExport, Network}; use nostr_sdk::PublicKey; use sha2::{Digest, Sha256}; +use subtle::ConstantTimeEq; use crate::error::{FrostNetError, Result}; use crate::protocol::{ @@ -273,7 +274,7 @@ impl DescriptorSession { hasher.update(finalized.policy_hash); let expected_hash: [u8; 32] = hasher.finalize().into(); - if descriptor_hash != expected_hash { + if !bool::from(descriptor_hash.ct_eq(&expected_hash)) { return Err(FrostNetError::Session("Descriptor hash mismatch".into())); } diff --git a/keep-frost-net/src/node/descriptor.rs b/keep-frost-net/src/node/descriptor.rs index c40b177d..620b00b8 100644 --- a/keep-frost-net/src/node/descriptor.rs +++ b/keep-frost-net/src/node/descriptor.rs @@ -52,15 +52,11 @@ impl KfpNode { let expected_contributors = participant_indices(&policy); let we_are_contributor = expected_contributors.contains(&our_index); - let expected_acks: HashSet = { - let peers = self.peers.read(); - peers - .get_online_peers() - .iter() - .map(|p| p.share_index) - .filter(|idx| *idx != our_index && expected_contributors.contains(idx)) - .collect() - }; + let expected_acks: HashSet = expected_contributors + .iter() + .copied() + .filter(|idx| *idx != our_index) + .collect(); let session_timeout = timeout_secs.map(std::time::Duration::from_secs); @@ -658,11 +654,6 @@ impl KfpNode { .sign_with_keys(&self.keys) .map_err(|e| FrostNetError::Nostr(e.to_string()))?; - self.client - .send_event(&event) - .await - .map_err(|e| FrostNetError::Transport(e.to_string()))?; - { let mut sessions = self.descriptor_sessions.write(); let Some(session) = sessions.get_session_mut(&payload.session_id) else { @@ -677,6 +668,11 @@ impl KfpNode { })?; } + self.client + .send_event(&event) + .await + .map_err(|e| FrostNetError::Transport(e.to_string()))?; + let _ = self.event_tx.send(KfpNodeEvent::DescriptorComplete { session_id: payload.session_id, external_descriptor: payload.external_descriptor, diff --git a/keep-nip46/src/audit.rs b/keep-nip46/src/audit.rs index 4e8ea1aa..cbf5603e 100644 --- a/keep-nip46/src/audit.rs +++ b/keep-nip46/src/audit.rs @@ -21,6 +21,7 @@ pub enum AuditAction { Nip44Encrypt, Nip44Decrypt, PermissionChanged, + SwitchRelays, PermissionDenied, UserRejected, } @@ -36,6 +37,7 @@ impl std::fmt::Display for AuditAction { Self::Nip04Decrypt => write!(f, "nip04_decrypt"), Self::Nip44Encrypt => write!(f, "nip44_encrypt"), Self::Nip44Decrypt => write!(f, "nip44_decrypt"), + Self::SwitchRelays => write!(f, "switch_relays"), Self::PermissionChanged => write!(f, "permission_changed"), Self::PermissionDenied => write!(f, "permission_denied"), Self::UserRejected => write!(f, "user_rejected"), diff --git a/keep-nip46/src/handler.rs b/keep-nip46/src/handler.rs index 5fcb2e56..ffcce443 100644 --- a/keep-nip46/src/handler.rs +++ b/keep-nip46/src/handler.rs @@ -482,6 +482,8 @@ impl SignerHandler { self.check_rate_limit(&app_pubkey).await?; self.require_permission(&app_pubkey, Permission::NIP44_ENCRYPT) .await?; + self.require_approval(app_pubkey, "nip44_encrypt").await?; + self.check_kill_switch()?; let secret = self.primary_secret_key().await?; let ciphertext = nip44::encrypt(&secret, &recipient, plaintext, nip44::Version::V2) .map_err(|e| CryptoError::encryption(format!("NIP-44: {e}")))?; @@ -528,6 +530,8 @@ impl SignerHandler { self.check_rate_limit(&app_pubkey).await?; self.require_permission(&app_pubkey, Permission::NIP04_ENCRYPT) .await?; + self.require_approval(app_pubkey, "nip04_encrypt").await?; + self.check_kill_switch()?; let secret = self.primary_secret_key().await?; let ciphertext = nip04::encrypt(&secret, &recipient, plaintext) .map_err(|e| CryptoError::encryption(format!("NIP-04: {e}")))?; @@ -569,9 +573,10 @@ impl SignerHandler { self.require_permission(&app_pubkey, Permission::GET_PUBLIC_KEY) .await?; - self.audit.lock().await.log( - AuditEntry::new(AuditAction::GetPublicKey, app_pubkey).with_reason("switch_relays"), - ); + self.audit + .lock() + .await + .log(AuditEntry::new(AuditAction::SwitchRelays, app_pubkey)); let relays = (!self.relay_urls.is_empty()).then(|| self.relay_urls.clone()); Ok(relays) From 6731e51a13488199951bd2f13cfc37d0975c37da Mon Sep 17 00:00:00 2001 From: "William K. Santiago" Date: Thu, 26 Feb 2026 14:24:59 -0500 Subject: [PATCH 07/13] Fix zeroize, relay cleanup, and remove duplicate verification --- keep-agent-py/src/lib.rs | 22 +++++++++------------- keep-agent-ts/src/lib.rs | 24 ++++++++++-------------- keep-agent/src/client.rs | 13 +++++++------ 3 files changed, 26 insertions(+), 33 deletions(-) diff --git a/keep-agent-py/src/lib.rs b/keep-agent-py/src/lib.rs index 708f96ab..a247c6da 100644 --- a/keep-agent-py/src/lib.rs +++ b/keep-agent-py/src/lib.rs @@ -7,7 +7,7 @@ use pyo3::prelude::*; use pyo3::exceptions::{PyRuntimeError, PyValueError, PyConnectionError}; use std::sync::Arc; use tokio::sync::Mutex; -use zeroize::{Zeroize, Zeroizing}; +use zeroize::Zeroizing; use ::keep_agent::{ RateLimitConfig, SessionConfig, SessionManager, SessionMetadata, @@ -359,8 +359,8 @@ impl PyAgentSession { session.check_operation(&Operation::SignPsbt).map_err(to_py_err)?; - let mut secret = *self.secret_key.as_ref() - .ok_or_else(|| PyRuntimeError::new_err("No secret key configured. Pass secret_key to constructor."))?; + let mut secret = Zeroizing::new(*self.secret_key.as_ref() + .ok_or_else(|| PyRuntimeError::new_err("No secret key configured. Pass secret_key to constructor."))?); let network = match network.unwrap_or("testnet") { "mainnet" | "bitcoin" => keep_bitcoin::Network::Bitcoin, @@ -372,7 +372,7 @@ impl PyAgentSession { let mut psbt = keep_bitcoin::psbt::parse_psbt_base64(psbt_base64) .map_err(|e| PyValueError::new_err(format!("Invalid PSBT: {}", e)))?; - let signer = keep_bitcoin::BitcoinSigner::new(&mut secret, network) + let signer = keep_bitcoin::BitcoinSigner::new(&mut *secret, network) .map_err(to_py_err)?; let analysis = signer.analyze_psbt(&psbt).map_err(to_py_err)?; @@ -403,9 +403,7 @@ impl PyAgentSession { self.manager.record_request(&self.session_id).map_err(to_py_err)?; - let result = signer.sign_psbt(&mut psbt).map_err(to_py_err); - secret.zeroize(); - result?; + signer.sign_psbt(&mut psbt).map_err(to_py_err)?; Ok(keep_bitcoin::psbt::serialize_psbt_base64(&psbt)) } @@ -432,8 +430,8 @@ impl PyAgentSession { session.check_operation(&Operation::GetBitcoinAddress).map_err(to_py_err)?; - let mut secret = *self.secret_key.as_ref() - .ok_or_else(|| PyRuntimeError::new_err("No secret key configured. Pass secret_key to constructor."))?; + let mut secret = Zeroizing::new(*self.secret_key.as_ref() + .ok_or_else(|| PyRuntimeError::new_err("No secret key configured. Pass secret_key to constructor."))?); let network = match network.unwrap_or("testnet") { "mainnet" | "bitcoin" => keep_bitcoin::Network::Bitcoin, @@ -442,12 +440,10 @@ impl PyAgentSession { _ => keep_bitcoin::Network::Testnet, }; - let signer = keep_bitcoin::BitcoinSigner::new(&mut secret, network) + let signer = keep_bitcoin::BitcoinSigner::new(&mut *secret, network) .map_err(to_py_err)?; - let result = signer.get_receive_address(0).map_err(to_py_err); - secret.zeroize(); - result + signer.get_receive_address(0).map_err(to_py_err) } fn __enter__(slf: Py) -> Py { diff --git a/keep-agent-ts/src/lib.rs b/keep-agent-ts/src/lib.rs index e14fd044..c819d6da 100644 --- a/keep-agent-ts/src/lib.rs +++ b/keep-agent-ts/src/lib.rs @@ -7,7 +7,7 @@ use napi::bindgen_prelude::*; use napi_derive::napi; use std::sync::Arc; use tokio::sync::Mutex; -use zeroize::{Zeroize, Zeroizing}; +use zeroize::Zeroizing; use keep_agent::{ AgentClient, ApprovalStatus, Operation, PendingSession as RustPendingSession, @@ -367,10 +367,10 @@ impl KeepAgentSession { .check_operation(&Operation::SignPsbt) .map_err(|e| Error::from_reason(e.to_string()))?; - let mut secret = *self + let mut secret = Zeroizing::new(*self .secret_key .as_ref() - .ok_or_else(|| Error::from_reason("No secret key configured"))?; + .ok_or_else(|| Error::from_reason("No secret key configured"))?); let network = match network.as_deref().unwrap_or("testnet") { "mainnet" | "bitcoin" => keep_bitcoin::Network::Bitcoin, @@ -382,7 +382,7 @@ impl KeepAgentSession { let mut psbt = keep_bitcoin::psbt::parse_psbt_base64(&psbt_base64) .map_err(|e| Error::from_reason(format!("Invalid PSBT: {}", e)))?; - let signer = keep_bitcoin::BitcoinSigner::new(&mut secret, network) + let signer = keep_bitcoin::BitcoinSigner::new(&mut *secret, network) .map_err(|e| Error::from_reason(e.to_string()))?; let analysis = signer @@ -417,9 +417,7 @@ impl KeepAgentSession { .record_request(&self.session_id) .map_err(|e| Error::from_reason(e.to_string()))?; - let result = signer.sign_psbt(&mut psbt).map_err(|e| Error::from_reason(e.to_string())); - secret.zeroize(); - result?; + signer.sign_psbt(&mut psbt).map_err(|e| Error::from_reason(e.to_string()))?; Ok(keep_bitcoin::psbt::serialize_psbt_base64(&psbt)) } @@ -452,10 +450,10 @@ impl KeepAgentSession { .check_operation(&Operation::GetBitcoinAddress) .map_err(|e| Error::from_reason(e.to_string()))?; - let mut secret = *self + let mut secret = Zeroizing::new(*self .secret_key .as_ref() - .ok_or_else(|| Error::from_reason("No secret key configured"))?; + .ok_or_else(|| Error::from_reason("No secret key configured"))?); let network = match network.as_deref().unwrap_or("testnet") { "mainnet" | "bitcoin" => keep_bitcoin::Network::Bitcoin, @@ -464,14 +462,12 @@ impl KeepAgentSession { _ => keep_bitcoin::Network::Testnet, }; - let signer = keep_bitcoin::BitcoinSigner::new(&mut secret, network) + let signer = keep_bitcoin::BitcoinSigner::new(&mut *secret, network) .map_err(|e| Error::from_reason(e.to_string()))?; - let result = signer + signer .get_receive_address(0) - .map_err(|e| Error::from_reason(e.to_string())); - secret.zeroize(); - result + .map_err(|e| Error::from_reason(e.to_string())) } } diff --git a/keep-agent/src/client.rs b/keep-agent/src/client.rs index fc7314c2..a4581db9 100644 --- a/keep-agent/src/client.rs +++ b/keep-agent/src/client.rs @@ -1,5 +1,6 @@ // SPDX-FileCopyrightText: © 2026 PrivKey LLC // SPDX-License-Identifier: AGPL-3.0-or-later +use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; use nostr_sdk::prelude::*; @@ -19,7 +20,7 @@ pub struct PendingSession { relay_url: String, client_keys: Keys, client: Client, - connect_sent: std::sync::atomic::AtomicBool, + connect_sent: AtomicBool, } impl PendingSession { @@ -57,7 +58,7 @@ impl PendingSession { relay_url, client_keys, client, - connect_sent: std::sync::atomic::AtomicBool::new(false), + connect_sent: AtomicBool::new(false), }) } @@ -78,7 +79,7 @@ impl PendingSession { async fn send_connect_once(&self) -> Result<()> { if self .connect_sent - .swap(true, std::sync::atomic::Ordering::AcqRel) + .swap(true, Ordering::AcqRel) { return Ok(()); } @@ -436,15 +437,15 @@ impl AgentClient { } self.client.disconnect().await; + self.client.remove_all_relays().await; for relay in &valid_relays { let _ = self.client.add_relay(relay).await; } self.client.connect().await; - let relays = valid_relays; - self.relay_url = relays[0].clone(); + self.relay_url = valid_relays[0].clone(); - Ok(Some(relays)) + Ok(Some(valid_relays)) } async fn send_request(&self, content: &str) -> Result { From 75e7588c55fee20a7b7b5b449e6a76c264d96674 Mon Sep 17 00:00:00 2001 From: "William K. Santiago" Date: Thu, 26 Feb 2026 15:12:47 -0500 Subject: [PATCH 08/13] Fix zeroize wrapping, coordination cleanup, and formatting --- keep-agent-py/src/lib.rs | 3 ++- keep-agent-ts/src/lib.rs | 3 ++- keep-agent/src/client.rs | 8 ++------ keep-desktop/src/frost.rs | 1 + keep-frost-net/src/node/mod.rs | 2 +- 5 files changed, 8 insertions(+), 9 deletions(-) diff --git a/keep-agent-py/src/lib.rs b/keep-agent-py/src/lib.rs index a247c6da..6943f5b6 100644 --- a/keep-agent-py/src/lib.rs +++ b/keep-agent-py/src/lib.rs @@ -308,7 +308,8 @@ impl PyAgentSession { use nostr_sdk::prelude::*; - let keys = Keys::parse(&hex::encode(secret.as_ref())) + let hex = Zeroizing::new(hex::encode(secret.as_ref())); + let keys = Keys::parse(hex.as_str()) .map_err(to_py_value_err)?; let mut nostr_tags: Vec = Vec::new(); diff --git a/keep-agent-ts/src/lib.rs b/keep-agent-ts/src/lib.rs index c819d6da..f1ab3c0c 100644 --- a/keep-agent-ts/src/lib.rs +++ b/keep-agent-ts/src/lib.rs @@ -314,7 +314,8 @@ impl KeepAgentSession { use nostr_sdk::prelude::{EventBuilder, Keys, Kind, Tag}; - let keys = Keys::parse(&hex::encode(secret.as_ref())) + let hex = Zeroizing::new(hex::encode(secret.as_ref())); + let keys = Keys::parse(hex.as_str()) .map_err(|e| napi::Error::from_reason(e.to_string()))?; let nostr_tags: Vec = tags diff --git a/keep-agent/src/client.rs b/keep-agent/src/client.rs index a4581db9..8841c286 100644 --- a/keep-agent/src/client.rs +++ b/keep-agent/src/client.rs @@ -77,10 +77,7 @@ impl PendingSession { } async fn send_connect_once(&self) -> Result<()> { - if self - .connect_sent - .swap(true, Ordering::AcqRel) - { + if self.connect_sent.swap(true, Ordering::AcqRel) { return Ok(()); } @@ -512,8 +509,7 @@ impl AgentClient { if let Ok(resp) = serde_json::from_str::(&decrypted) { - if resp.get("id").and_then(|v| v.as_str()) - != Some(expected_id) + if resp.get("id").and_then(|v| v.as_str()) != Some(expected_id) { continue; } diff --git a/keep-desktop/src/frost.rs b/keep-desktop/src/frost.rs index b20dfa6c..db4af1af 100644 --- a/keep-desktop/src/frost.rs +++ b/keep-desktop/src/frost.rs @@ -716,6 +716,7 @@ impl App { setup.phase = SetupPhase::Coordinating(DescriptorProgress::Finalizing); }); let Some(node) = self.get_frost_node() else { + self.active_coordinations.remove(&session_id); self.update_wallet_setup(&session_id, |setup| { setup.phase = SetupPhase::Coordinating(DescriptorProgress::Failed( "Node unavailable".to_string(), diff --git a/keep-frost-net/src/node/mod.rs b/keep-frost-net/src/node/mod.rs index 94221083..6af21df0 100644 --- a/keep-frost-net/src/node/mod.rs +++ b/keep-frost-net/src/node/mod.rs @@ -1166,7 +1166,7 @@ fn derive_keys_from_share(share: &SharePackage) -> Result { let key_package = share .key_package() .map_err(|e| FrostNetError::Crypto(format!("Failed to get key package: {e}")))?; - let signing_share_bytes = key_package.signing_share().serialize(); + let signing_share_bytes = Zeroizing::new(key_package.signing_share().serialize()); let mut hasher = Sha256::new(); hasher.update(b"keep-frost-node-identity-v2"); From 9bd86e4fb5d9eed103ea4fb5fbcff66e0dcef9e4 Mon Sep 17 00:00:00 2001 From: "William K. Santiago" Date: Thu, 26 Feb 2026 15:19:15 -0500 Subject: [PATCH 09/13] Remove dead code in keep-desktop --- keep-desktop/src/app.rs | 16 +--------------- keep-desktop/src/frost.rs | 2 -- keep-desktop/src/message.rs | 6 ------ 3 files changed, 1 insertion(+), 23 deletions(-) diff --git a/keep-desktop/src/app.rs b/keep-desktop/src/app.rs index f97599c4..b94e1ce8 100644 --- a/keep-desktop/src/app.rs +++ b/keep-desktop/src/app.rs @@ -110,8 +110,6 @@ pub(crate) struct ActiveCoordination { pub group_pubkey: [u8; 32], pub network: String, pub is_initiator: bool, - pub expected_participants: usize, - pub acks_received: usize, } pub struct App { @@ -698,7 +696,6 @@ impl App { | Message::WalletBeginCoordination | Message::WalletCancelSetup | Message::WalletSessionStarted(..) - | Message::WalletFinalizeResult(..) | Message::WalletDescriptorProgress(..) => self.handle_wallet_message(message), Message::RelayUrlChanged(..) @@ -1311,7 +1308,7 @@ impl App { Message::WalletBeginCoordination => self.begin_descriptor_coordination(), Message::WalletSessionStarted(result) => { match result { - Ok((session_id, group_pubkey, network, expected_participants)) => { + Ok((session_id, group_pubkey, network, _expected_participants)) => { let on_wallet_screen = matches!( self.screen, Screen::Wallet(WalletScreen { setup: Some(_), .. }) @@ -1338,8 +1335,6 @@ impl App { group_pubkey, network, is_initiator: true, - expected_participants, - acks_received: 0, }, ); if let Screen::Wallet(WalletScreen { setup: Some(s), .. }) = @@ -1374,15 +1369,6 @@ impl App { } Task::none() } - Message::WalletFinalizeResult(result, session_id) => { - if let Err(e) = result { - self.active_coordinations.remove(&session_id); - self.update_wallet_setup(&session_id, |setup| { - setup.phase = SetupPhase::Coordinating(DescriptorProgress::Failed(e)); - }); - } - Task::none() - } Message::WalletDescriptorProgress(progress, session_id) => { if let Some(sid) = session_id { if matches!(progress, DescriptorProgress::Failed(_)) { diff --git a/keep-desktop/src/frost.rs b/keep-desktop/src/frost.rs index db4af1af..ad5db48d 100644 --- a/keep-desktop/src/frost.rs +++ b/keep-desktop/src/frost.rs @@ -841,8 +841,6 @@ impl App { group_pubkey: share.group_pubkey, network: network.clone(), is_initiator: false, - expected_participants: 0, - acks_received: 0, }, ); diff --git a/keep-desktop/src/message.rs b/keep-desktop/src/message.rs index 8d802953..9997e933 100644 --- a/keep-desktop/src/message.rs +++ b/keep-desktop/src/message.rs @@ -200,7 +200,6 @@ pub enum Message { WalletBeginCoordination, WalletCancelSetup, WalletSessionStarted(Result<([u8; 32], [u8; 32], String, usize), String>), - WalletFinalizeResult(Result<(), String>, [u8; 32]), WalletDescriptorProgress(DescriptorProgress, Option<[u8; 32]>), // Relay / FROST RelayUrlChanged(String), @@ -503,11 +502,6 @@ impl fmt::Debug for Message { .debug_tuple("WalletSessionStarted") .field(&r.as_ref().map(|_| "ok").map_err(|e| e.as_str())) .finish(), - Self::WalletFinalizeResult(r, sid) => f - .debug_struct("WalletFinalizeResult") - .field("result", &r.as_ref().map(|_| "ok").map_err(|e| e.as_str())) - .field("session_id", &hex::encode(sid)) - .finish(), Self::WalletDescriptorProgress(..) => f.write_str("WalletDescriptorProgress"), Self::RelayUrlChanged(u) => f.debug_tuple("RelayUrlChanged").field(u).finish(), Self::ConnectPasswordChanged(_) => f.write_str("ConnectPasswordChanged(***)"), From a3af755b4fb4f32853dea05c805c50b55f663ea2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kyle=20=F0=9F=90=86?= Date: Thu, 26 Feb 2026 15:48:25 -0500 Subject: [PATCH 10/13] fix: address PR security and correctness review findings --- keep-agent-py/src/lib.rs | 4 ++-- keep-agent-ts/src/lib.rs | 4 ++-- keep-agent/src/client.rs | 17 ++++++++++++++--- keep-agent/src/manager.rs | 8 ++++++++ keep-agent/src/mcp/server.rs | 20 ++++++++++++-------- keep-frost-net/src/node/descriptor.rs | 14 +++++++++----- keep-frost-net/src/node/mod.rs | 10 ++-------- keep-nip46/Cargo.toml | 1 + keep-nip46/src/handler.rs | 6 ++++-- keep-nip46/src/server.rs | 14 ++++++++++---- 10 files changed, 64 insertions(+), 34 deletions(-) diff --git a/keep-agent-py/src/lib.rs b/keep-agent-py/src/lib.rs index 6943f5b6..25987694 100644 --- a/keep-agent-py/src/lib.rs +++ b/keep-agent-py/src/lib.rs @@ -171,8 +171,8 @@ impl PyAgentSession { secret_key: Option, ) -> PyResult { let secret_bytes: Option> = if let Some(sk) = secret_key { - let decoded = hex::decode(&sk) - .map_err(|e| PyValueError::new_err(format!("Invalid secret key hex: {}", e)))?; + let decoded = Zeroizing::new(hex::decode(&sk) + .map_err(|e| PyValueError::new_err(format!("Invalid secret key hex: {}", e)))?); if decoded.len() != 32 { return Err(PyValueError::new_err(format!( "Secret key must be 32 bytes, got {}", diff --git a/keep-agent-ts/src/lib.rs b/keep-agent-ts/src/lib.rs index f1ab3c0c..2f2b72da 100644 --- a/keep-agent-ts/src/lib.rs +++ b/keep-agent-ts/src/lib.rs @@ -68,8 +68,8 @@ impl KeepAgentSession { secret_key: Option, ) -> Result { let secret_bytes: Option> = if let Some(ref sk) = secret_key { - let decoded = hex::decode(sk) - .map_err(|e| Error::from_reason(format!("Invalid secret key hex: {}", e)))?; + let decoded = Zeroizing::new(hex::decode(sk) + .map_err(|e| Error::from_reason(format!("Invalid secret key hex: {}", e)))?); if decoded.len() != 32 { return Err(Error::from_reason(format!( "Secret key must be 32 bytes, got {}", diff --git a/keep-agent/src/client.rs b/keep-agent/src/client.rs index 8841c286..02f4d87a 100644 --- a/keep-agent/src/client.rs +++ b/keep-agent/src/client.rs @@ -289,6 +289,9 @@ impl AgentClient { AgentError::Connection("Missing relay parameter in bunker URL".into()) })?; + keep_core::relay::validate_relay_url(&relay_url) + .map_err(|e| AgentError::Connection(format!("Invalid relay URL: {e}")))?; + Ok((signer_pubkey, relay_url, secret)) } @@ -435,14 +438,22 @@ impl AgentClient { self.client.disconnect().await; self.client.remove_all_relays().await; + let mut added = Vec::new(); for relay in &valid_relays { - let _ = self.client.add_relay(relay).await; + if self.client.add_relay(relay).await.is_ok() { + added.push(relay.clone()); + } + } + if added.is_empty() { + return Err(AgentError::Connection( + "Failed to add any relay during switch".into(), + )); } self.client.connect().await; - self.relay_url = valid_relays[0].clone(); + self.relay_url = added[0].clone(); - Ok(Some(valid_relays)) + Ok(Some(added)) } async fn send_request(&self, content: &str) -> Result { diff --git a/keep-agent/src/manager.rs b/keep-agent/src/manager.rs index 663f8499..062f545a 100644 --- a/keep-agent/src/manager.rs +++ b/keep-agent/src/manager.rs @@ -7,6 +7,8 @@ use crate::error::{AgentError, Result}; use crate::scope::Operation; use crate::session::{AgentSession, SessionConfig, SessionMetadata, SessionToken}; +const MAX_SESSIONS: usize = 128; + pub struct SessionManager { sessions: Arc>>, pubkey: [u8; 32], @@ -34,6 +36,12 @@ impl SessionManager { .write() .map_err(|_| AgentError::Other("Failed to acquire session lock".into()))?; + if sessions.len() >= MAX_SESSIONS { + return Err(AgentError::Other(format!( + "Maximum session count ({MAX_SESSIONS}) reached" + ))); + } + sessions.insert(session_id.clone(), session); Ok((token, session_id)) } diff --git a/keep-agent/src/mcp/server.rs b/keep-agent/src/mcp/server.rs index 9dc11d6f..dd1f34c8 100644 --- a/keep-agent/src/mcp/server.rs +++ b/keep-agent/src/mcp/server.rs @@ -8,6 +8,8 @@ use serde::{Deserialize, Serialize}; use serde_json::Value; use tokio::sync::RwLock; +use zeroize::Zeroizing; + use crate::error::{AgentError, Result}; use crate::manager::SessionManager; use crate::scope::Operation; @@ -45,7 +47,7 @@ pub struct McpServer { version: String, session_manager: Arc>>, manager: SessionManager, - secret_key: Option<[u8; 32]>, + secret_key: Option>, } impl McpServer { @@ -65,7 +67,7 @@ impl McpServer { version: env!("CARGO_PKG_VERSION").into(), session_manager: Arc::new(RwLock::new(None)), manager: SessionManager::new(pubkey), - secret_key: Some(secret), + secret_key: Some(Zeroizing::new(secret)), } } @@ -265,10 +267,10 @@ impl McpServer { .and_then(|v| serde_json::from_value(v.clone()).ok()) .unwrap_or_default(); - if let Some(secret) = self.secret_key { + if let Some(ref secret) = self.secret_key { use nostr_sdk::prelude::*; - let keys = Keys::parse(&hex::encode(secret)) + let keys = Keys::parse(&hex::encode(&**secret)) .map_err(|e| AgentError::Other(e.to_string()))?; let nostr_tags: Vec = tags @@ -322,7 +324,8 @@ impl McpServer { .and_then(|v| v.as_str()) .unwrap_or("testnet"); - if let Some(mut secret) = self.secret_key { + if let Some(ref secret) = self.secret_key { + let mut secret_copy = Zeroizing::new(**secret); let network = match network_str { "mainnet" | "bitcoin" => keep_bitcoin::Network::Bitcoin, "signet" => keep_bitcoin::Network::Signet, @@ -333,7 +336,7 @@ impl McpServer { let mut psbt = keep_bitcoin::psbt::parse_psbt_base64(psbt_base64) .map_err(|e| AgentError::Other(format!("Invalid PSBT: {e}")))?; - let signer = keep_bitcoin::BitcoinSigner::new(&mut secret, network) + let signer = keep_bitcoin::BitcoinSigner::new(&mut *secret_copy, network) .map_err(|e| AgentError::Other(e.to_string()))?; let analysis = signer @@ -416,7 +419,8 @@ impl McpServer { .and_then(|v| v.as_str()) .unwrap_or("testnet"); - if let Some(mut secret) = self.secret_key { + if let Some(ref secret) = self.secret_key { + let mut secret_copy = Zeroizing::new(**secret); let network = match network_str { "mainnet" | "bitcoin" => keep_bitcoin::Network::Bitcoin, "signet" => keep_bitcoin::Network::Signet, @@ -424,7 +428,7 @@ impl McpServer { _ => keep_bitcoin::Network::Testnet, }; - let signer = keep_bitcoin::BitcoinSigner::new(&mut secret, network) + let signer = keep_bitcoin::BitcoinSigner::new(&mut *secret_copy, network) .map_err(|e| AgentError::Other(e.to_string()))?; let address = signer diff --git a/keep-frost-net/src/node/descriptor.rs b/keep-frost-net/src/node/descriptor.rs index 620b00b8..29db8e47 100644 --- a/keep-frost-net/src/node/descriptor.rs +++ b/keep-frost-net/src/node/descriptor.rs @@ -52,11 +52,15 @@ impl KfpNode { let expected_contributors = participant_indices(&policy); let we_are_contributor = expected_contributors.contains(&our_index); - let expected_acks: HashSet = expected_contributors - .iter() - .copied() - .filter(|idx| *idx != our_index) - .collect(); + let expected_acks: HashSet = { + let peers = self.peers.read(); + peers + .get_online_peers() + .iter() + .map(|p| p.share_index) + .filter(|idx| *idx != our_index && expected_contributors.contains(idx)) + .collect() + }; let session_timeout = timeout_secs.map(std::time::Duration::from_secs); diff --git a/keep-frost-net/src/node/mod.rs b/keep-frost-net/src/node/mod.rs index 6af21df0..547b6145 100644 --- a/keep-frost-net/src/node/mod.rs +++ b/keep-frost-net/src/node/mod.rs @@ -1163,18 +1163,12 @@ impl KfpNode { } fn derive_keys_from_share(share: &SharePackage) -> Result { - let key_package = share - .key_package() - .map_err(|e| FrostNetError::Crypto(format!("Failed to get key package: {e}")))?; - let signing_share_bytes = Zeroizing::new(key_package.signing_share().serialize()); - let mut hasher = Sha256::new(); hasher.update(b"keep-frost-node-identity-v2"); hasher.update(share.metadata.group_pubkey); hasher.update(share.metadata.identifier.to_be_bytes()); - hasher.update(signing_share_bytes.as_slice()); - let derived: Zeroizing<[u8; 32]> = Zeroizing::new(hasher.finalize().into()); - let secret_key = SecretKey::from_slice(&*derived) + let derived: [u8; 32] = hasher.finalize().into(); + let secret_key = SecretKey::from_slice(&derived) .map_err(|e| FrostNetError::Crypto(format!("Failed to create secret key: {e}")))?; Ok(Keys::new(secret_key)) } diff --git a/keep-nip46/Cargo.toml b/keep-nip46/Cargo.toml index b72a12ef..880c32a5 100644 --- a/keep-nip46/Cargo.toml +++ b/keep-nip46/Cargo.toml @@ -22,6 +22,7 @@ tracing.workspace = true thiserror.workspace = true subtle.workspace = true sha2 = "0.10" +zeroize.workspace = true [dev-dependencies] rustls = { version = "0.23", features = ["ring"] } diff --git a/keep-nip46/src/handler.rs b/keep-nip46/src/handler.rs index ffcce443..f48e6eb0 100644 --- a/keep-nip46/src/handler.rs +++ b/keep-nip46/src/handler.rs @@ -10,6 +10,7 @@ use sha2::{Digest, Sha256}; use subtle::ConstantTimeEq; use tokio::sync::Mutex; use tracing::{debug, info, warn}; +use zeroize::Zeroizing; use keep_core::error::{CryptoError, KeepError, Result}; use keep_core::keyring::Keyring; @@ -96,7 +97,7 @@ pub struct SignerHandler { rate_limiters: Mutex>, rate_limit_config: Option, new_conn_timestamps: Mutex>, - expected_secret: Option, + expected_secret: Option>, auto_approve: bool, relay_urls: Vec, kill_switch: Arc, @@ -127,7 +128,7 @@ impl SignerHandler { } pub fn with_expected_secret(mut self, secret: String) -> Self { - self.expected_secret = Some(secret); + self.expected_secret = Some(Zeroizing::new(secret)); self } @@ -569,6 +570,7 @@ impl SignerHandler { } pub async fn handle_switch_relays(&self, app_pubkey: PublicKey) -> Result>> { + self.check_kill_switch()?; self.check_rate_limit(&app_pubkey).await?; self.require_permission(&app_pubkey, Permission::GET_PUBLIC_KEY) .await?; diff --git a/keep-nip46/src/server.rs b/keep-nip46/src/server.rs index 32819f50..d3cf7eb6 100644 --- a/keep-nip46/src/server.rs +++ b/keep-nip46/src/server.rs @@ -7,6 +7,7 @@ use std::sync::Arc; use nostr_sdk::prelude::*; use tokio::sync::Mutex; use tracing::{debug, error, info, warn}; +use zeroize::Zeroizing; use keep_core::error::{CryptoError, KeepError, NetworkError, StorageError}; use keep_core::keyring::Keyring; @@ -51,7 +52,7 @@ pub struct Server { running: bool, callbacks: Option>, config: ServerConfig, - bunker_secret: Option, + bunker_secret: Option>, } async fn add_relays(client: &Client, relay_urls: &[String]) -> Result<()> { @@ -75,7 +76,7 @@ fn finalize_handler( mut handler: SignerHandler, config: &ServerConfig, relay_urls: &[String], -) -> (SignerHandler, Option) { +) -> (SignerHandler, Option>) { handler = handler.with_relay_urls(relay_urls.to_vec()); if let Some(ref rl_config) = config.rate_limit { handler = handler.with_rate_limit(rl_config.clone()); @@ -87,7 +88,7 @@ fn finalize_handler( let secret = hex::encode(keep_core::crypto::random_bytes::<16>()); warn!("headless mode: bunker secret required for authentication"); handler = handler.with_expected_secret(secret.clone()); - Some(secret) + Some(Zeroizing::new(secret)) } else if let Some(ref secret) = config.expected_secret { handler = handler.with_expected_secret(secret.clone()); None @@ -105,7 +106,7 @@ impl Server { handler: SignerHandler, callbacks: Option>, config: ServerConfig, - bunker_secret: Option, + bunker_secret: Option>, ) -> Self { Self { keys, @@ -462,6 +463,11 @@ impl Server { return Err(KeepError::InvalidInput("invalid request ID".into())); } + const MAX_NIP46_PARAMS: usize = 10; + if request.params.len() > MAX_NIP46_PARAMS { + return Err(KeepError::InvalidInput("too many request params".into())); + } + debug!(method = %request.method, app_id, "NIP-46 request"); let method = request.method.clone(); From 2848922614476f5c77faabf1f72c954e49519aa6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kyle=20=F0=9F=90=86?= Date: Thu, 26 Feb 2026 16:02:50 -0500 Subject: [PATCH 11/13] Fix clippy warnings, zeroize hex secret, rate limit on success only --- Cargo.lock | 1 + keep-agent/src/mcp/server.rs | 11 +++++++---- keep-desktop/src/frost.rs | 1 - keep-nip46/src/server.rs | 2 +- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b6778eb9..44e61540 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4396,6 +4396,7 @@ dependencies = [ "tracing-subscriber", "url", "urlencoding", + "zeroize", ] [[package]] diff --git a/keep-agent/src/mcp/server.rs b/keep-agent/src/mcp/server.rs index dd1f34c8..a26f2a54 100644 --- a/keep-agent/src/mcp/server.rs +++ b/keep-agent/src/mcp/server.rs @@ -270,7 +270,8 @@ impl McpServer { if let Some(ref secret) = self.secret_key { use nostr_sdk::prelude::*; - let keys = Keys::parse(&hex::encode(&**secret)) + let hex_secret = Zeroizing::new(hex::encode(**secret)); + let keys = Keys::parse(hex_secret.as_str()) .map_err(|e| AgentError::Other(e.to_string()))?; let nostr_tags: Vec = tags @@ -336,7 +337,7 @@ impl McpServer { let mut psbt = keep_bitcoin::psbt::parse_psbt_base64(psbt_base64) .map_err(|e| AgentError::Other(format!("Invalid PSBT: {e}")))?; - let signer = keep_bitcoin::BitcoinSigner::new(&mut *secret_copy, network) + let signer = keep_bitcoin::BitcoinSigner::new(&mut secret_copy, network) .map_err(|e| AgentError::Other(e.to_string()))?; let analysis = signer @@ -428,7 +429,7 @@ impl McpServer { _ => keep_bitcoin::Network::Testnet, }; - let signer = keep_bitcoin::BitcoinSigner::new(&mut *secret_copy, network) + let signer = keep_bitcoin::BitcoinSigner::new(&mut secret_copy, network) .map_err(|e| AgentError::Other(e.to_string()))?; let address = signer @@ -453,7 +454,9 @@ impl McpServer { _ => ToolResult::error(format!("Unknown tool: {name}")), }; - self.manager.record_request(&session_id)?; + if result.success { + self.manager.record_request(&session_id)?; + } let text = serde_json::to_string(&result.content) .map_err(|e| AgentError::Serialization(e.to_string()))?; diff --git a/keep-desktop/src/frost.rs b/keep-desktop/src/frost.rs index ad5db48d..e830d889 100644 --- a/keep-desktop/src/frost.rs +++ b/keep-desktop/src/frost.rs @@ -954,7 +954,6 @@ impl App { let _ = entry.response_tx.try_send(false); } } - self.active_coordinations.clear(); if let Some(s) = self.relay_screen_mut() { s.status = ConnectionStatus::Disconnected; s.peers.clear(); diff --git a/keep-nip46/src/server.rs b/keep-nip46/src/server.rs index d3cf7eb6..fa130340 100644 --- a/keep-nip46/src/server.rs +++ b/keep-nip46/src/server.rs @@ -340,7 +340,7 @@ impl Server { generate_bunker_url( &self.keys.public_key(), &self.relay_urls, - self.bunker_secret.as_deref(), + self.bunker_secret.as_ref().map(|s| s.as_str()), ) } From b3e106c6c646c9e3e12f6013a385ee7da07ed517 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kyle=20=F0=9F=90=86?= Date: Thu, 26 Feb 2026 16:16:20 -0500 Subject: [PATCH 12/13] Fix duplicate DescriptorComplete, prune expired sessions, use MockRelay --- Cargo.lock | 1 + keep-agent/src/manager.rs | 2 ++ keep-frost-net/src/node/descriptor.rs | 2 +- keep-nip46/Cargo.toml | 1 + keep-nip46/tests/bunker_integration.rs | 24 +++++++++++------------- 5 files changed, 16 insertions(+), 14 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 44e61540..0db66d93 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4384,6 +4384,7 @@ dependencies = [ "hex", "keep-core", "keep-frost-net", + "nostr-relay-builder", "nostr-sdk", "rustls", "serde", diff --git a/keep-agent/src/manager.rs b/keep-agent/src/manager.rs index 062f545a..cc3c1ff9 100644 --- a/keep-agent/src/manager.rs +++ b/keep-agent/src/manager.rs @@ -36,6 +36,8 @@ impl SessionManager { .write() .map_err(|_| AgentError::Other("Failed to acquire session lock".into()))?; + sessions.retain(|_, s| !s.is_expired()); + if sessions.len() >= MAX_SESSIONS { return Err(AgentError::Other(format!( "Maximum session count ({MAX_SESSIONS}) reached" diff --git a/keep-frost-net/src/node/descriptor.rs b/keep-frost-net/src/node/descriptor.rs index 29db8e47..777fcbb3 100644 --- a/keep-frost-net/src/node/descriptor.rs +++ b/keep-frost-net/src/node/descriptor.rs @@ -858,7 +858,7 @@ impl KfpNode { }); } - if is_complete { + if is_new && is_complete { let sessions = self.descriptor_sessions.read(); if let Some(session) = sessions.get_session(&payload.session_id) { if let Some(desc) = session.descriptor() { diff --git a/keep-nip46/Cargo.toml b/keep-nip46/Cargo.toml index 880c32a5..d29ff012 100644 --- a/keep-nip46/Cargo.toml +++ b/keep-nip46/Cargo.toml @@ -25,5 +25,6 @@ sha2 = "0.10" zeroize.workspace = true [dev-dependencies] +nostr-relay-builder = "0.44" rustls = { version = "0.23", features = ["ring"] } tracing-subscriber = "0.3" diff --git a/keep-nip46/tests/bunker_integration.rs b/keep-nip46/tests/bunker_integration.rs index ed92a0d8..94919eba 100644 --- a/keep-nip46/tests/bunker_integration.rs +++ b/keep-nip46/tests/bunker_integration.rs @@ -1,6 +1,7 @@ use std::sync::Arc; use std::time::Duration; +use nostr_relay_builder::prelude::*; use nostr_sdk::prelude::*; use tokio::sync::Mutex; @@ -37,7 +38,8 @@ async fn test_bunker_e2e_connect_and_sign() { .install_default() .ok(); - let relay_url = "wss://relay.damus.io".to_string(); + let mock_relay = MockRelay::run().await.expect("Failed to start mock relay"); + let relay_url = mock_relay.url().await.to_string(); let (keyring, signer_pubkey) = setup_keyring(); let mut server = Server::new_with_config( @@ -71,14 +73,14 @@ async fn test_bunker_e2e_connect_and_sign() { let _ = server.run().await; }); - tokio::time::sleep(Duration::from_secs(2)).await; + tokio::time::sleep(Duration::from_secs(1)).await; let client_keys = Keys::generate(); let client = Client::new(client_keys.clone()); client.add_relay(relay_url).await.unwrap(); client.connect().await; - tokio::time::sleep(Duration::from_secs(2)).await; + tokio::time::sleep(Duration::from_secs(1)).await; // Send connect request with bunker secret let connect_req = serde_json::json!({ @@ -103,7 +105,7 @@ async fn test_bunker_e2e_connect_and_sign() { client.send_event(&connect_event).await.unwrap(); - tokio::time::sleep(Duration::from_secs(3)).await; + tokio::time::sleep(Duration::from_secs(2)).await; // Verify the client connected let clients = server_handler.list_clients().await; @@ -164,7 +166,7 @@ async fn test_bunker_e2e_connect_and_sign() { client.send_event(&sign_event).await.unwrap(); - // Wait for relay to deliver responses - retry a few times + // Wait for relay to deliver responses let mut request_count = 0; for _ in 0..10 { tokio::time::sleep(Duration::from_secs(1)).await; @@ -197,7 +199,8 @@ async fn test_bunker_rejects_without_auto_approve() { .install_default() .ok(); - let relay_url = "wss://relay.damus.io".to_string(); + let mock_relay = MockRelay::run().await.expect("Failed to start mock relay"); + let relay_url = mock_relay.url().await.to_string(); let (keyring, _signer_pubkey) = setup_keyring(); // auto_approve: false, no callbacks = rejects by default @@ -226,8 +229,6 @@ async fn test_bunker_rejects_without_auto_approve() { result.is_err(), "connect should be rejected without callbacks or auto_approve" ); - - println!("Rejection test passed: connect properly denied without auto_approve"); } #[tokio::test] @@ -236,7 +237,8 @@ async fn test_bunker_permission_scoping() { .install_default() .ok(); - let relay_url = "wss://relay.damus.io".to_string(); + let mock_relay = MockRelay::run().await.expect("Failed to start mock relay"); + let relay_url = mock_relay.url().await.to_string(); let (keyring, signer_pubkey) = setup_keyring(); let server = Server::new_with_config( @@ -286,8 +288,4 @@ async fn test_bunker_permission_scoping() { sign_result.is_err(), "sign_event should be denied when only get_public_key requested" ); - - println!("Permission scoping test passed:"); - println!(" - get_public_key: allowed"); - println!(" - sign_event: correctly denied"); } From 6ca9bcef6dacf29c25a482a51be3020bbe073b88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kyle=20=F0=9F=90=86?= Date: Thu, 26 Feb 2026 16:26:23 -0500 Subject: [PATCH 13/13] Fix finalize-before-send ordering and saturating_add overflow --- keep-frost-net/src/node/descriptor.rs | 14 ++++++++------ keep-nip46/tests/bunker_integration.rs | 22 ---------------------- 2 files changed, 8 insertions(+), 28 deletions(-) diff --git a/keep-frost-net/src/node/descriptor.rs b/keep-frost-net/src/node/descriptor.rs index 777fcbb3..49337959 100644 --- a/keep-frost-net/src/node/descriptor.rs +++ b/keep-frost-net/src/node/descriptor.rs @@ -658,6 +658,11 @@ impl KfpNode { .sign_with_keys(&self.keys) .map_err(|e| FrostNetError::Nostr(e.to_string()))?; + self.client + .send_event(&event) + .await + .map_err(|e| FrostNetError::Transport(e.to_string()))?; + { let mut sessions = self.descriptor_sessions.write(); let Some(session) = sessions.get_session_mut(&payload.session_id) else { @@ -672,11 +677,6 @@ impl KfpNode { })?; } - self.client - .send_event(&event) - .await - .map_err(|e| FrostNetError::Transport(e.to_string()))?; - let _ = self.event_tx.send(KfpNodeEvent::DescriptorComplete { session_id: payload.session_id, external_descriptor: payload.external_descriptor, @@ -937,7 +937,9 @@ impl KfpNode { const MAX_SEEN_XPUB_ANNOUNCES: usize = 10_000; if seen.len() > MAX_SEEN_XPUB_ANNOUNCES { let now = chrono::Utc::now().timestamp().max(0) as u64; - let window = self.replay_window_secs + super::MAX_FUTURE_SKEW_SECS; + let window = self + .replay_window_secs + .saturating_add(super::MAX_FUTURE_SKEW_SECS); seen.retain(|&(_, ts, _)| now.saturating_sub(window) <= ts); if seen.len() > MAX_SEEN_XPUB_ANNOUNCES { seen.clear(); diff --git a/keep-nip46/tests/bunker_integration.rs b/keep-nip46/tests/bunker_integration.rs index 94919eba..c186d082 100644 --- a/keep-nip46/tests/bunker_integration.rs +++ b/keep-nip46/tests/bunker_integration.rs @@ -166,31 +166,9 @@ async fn test_bunker_e2e_connect_and_sign() { client.send_event(&sign_event).await.unwrap(); - // Wait for relay to deliver responses - let mut request_count = 0; - for _ in 0..10 { - tokio::time::sleep(Duration::from_secs(1)).await; - let apps = server_handler.list_clients().await; - if let Some(app) = apps.first() { - request_count = app.request_count; - if request_count > 0 { - break; - } - } - } - // Cleanup server_handle.abort(); client.disconnect().await; - - println!("NIP-46 bunker integration test passed!"); - println!(" - Server started with bunker URL: {bunker_url}"); - println!( - " - Client connected successfully ({} clients)", - clients.len() - ); - println!(" - get_public_key and sign_event requests sent"); - println!(" - {request_count} request(s) recorded via relay round-trip"); } #[tokio::test]