diff --git a/keep-frost-net/src/descriptor_session.rs b/keep-frost-net/src/descriptor_session.rs index ec7abd27..2655dea9 100644 --- a/keep-frost-net/src/descriptor_session.rs +++ b/keep-frost-net/src/descriptor_session.rs @@ -11,10 +11,13 @@ use sha2::{Digest, Sha256}; use crate::error::{FrostNetError, Result}; use crate::protocol::{ - KeySlot, WalletPolicy, DESCRIPTOR_SESSION_TIMEOUT_SECS, MAX_FINGERPRINT_LENGTH, MAX_XPUB_LENGTH, + KeySlot, WalletPolicy, DESCRIPTOR_ACK_PHASE_TIMEOUT_SECS, DESCRIPTOR_CONTRIBUTION_TIMEOUT_SECS, + DESCRIPTOR_FINALIZE_TIMEOUT_SECS, DESCRIPTOR_SESSION_TIMEOUT_SECS, MAX_FINGERPRINT_LENGTH, + MAX_XPUB_LENGTH, }; const MAX_SESSIONS: usize = 64; +const REAP_GRACE_SECS: u64 = 60; #[derive(Debug, Clone, PartialEq, Eq)] pub enum DescriptorSessionState { @@ -51,7 +54,12 @@ pub struct DescriptorSession { expected_acks: HashSet, state: DescriptorSessionState, created_at: Instant, + contributions_complete_at: Option, + finalized_at: Option, timeout: Duration, + contribution_timeout: Duration, + finalize_timeout: Duration, + ack_phase_timeout: Duration, } impl DescriptorSession { @@ -78,7 +86,12 @@ impl DescriptorSession { expected_acks, state: DescriptorSessionState::Proposed, created_at: Instant::now(), + contributions_complete_at: None, + finalized_at: None, timeout, + contribution_timeout: Duration::from_secs(DESCRIPTOR_CONTRIBUTION_TIMEOUT_SECS), + finalize_timeout: Duration::from_secs(DESCRIPTOR_FINALIZE_TIMEOUT_SECS), + ack_phase_timeout: Duration::from_secs(DESCRIPTOR_ACK_PHASE_TIMEOUT_SECS), } } @@ -103,7 +116,9 @@ impl DescriptorSession { } pub fn set_initiator(&mut self, initiator: PublicKey) { - self.initiator = Some(initiator); + if self.initiator.is_none() { + self.initiator = Some(initiator); + } } pub fn state(&self) -> &DescriptorSessionState { @@ -133,17 +148,23 @@ impl DescriptorSession { if xpub.len() > MAX_XPUB_LENGTH { return Err(FrostNetError::Session("xpub exceeds maximum length".into())); } - if fingerprint.len() > MAX_FINGERPRINT_LENGTH { + if fingerprint.len() != MAX_FINGERPRINT_LENGTH + || !fingerprint.chars().all(|c| c.is_ascii_hexdigit()) + { return Err(FrostNetError::Session( - "fingerprint exceeds maximum length".into(), + "fingerprint must be exactly 8 hex characters".into(), )); } let is_mainnet = self.network == "bitcoin"; - let valid_prefix = if is_mainnet { "xpub" } else { "tpub" }; - if !xpub.starts_with(valid_prefix) { + let valid_prefixes: &[&str] = if is_mainnet { + &["xpub"] + } else { + &["tpub", "Vpub", "Upub"] + }; + if !valid_prefixes.iter().any(|p| xpub.starts_with(p)) { return Err(FrostNetError::Session(format!( - "xpub must start with '{valid_prefix}' for network '{}'", - self.network + "xpub must start with one of {:?} for network '{}'", + valid_prefixes, self.network ))); } @@ -161,6 +182,10 @@ impl DescriptorSession { }, ); + if self.has_all_contributions() { + self.contributions_complete_at = Some(Instant::now()); + } + Ok(()) } @@ -195,6 +220,7 @@ impl DescriptorSession { self.descriptor = Some(descriptor); self.state = DescriptorSessionState::Finalized; + self.finalized_at = Some(Instant::now()); Ok(()) } @@ -268,7 +294,43 @@ impl DescriptorSession { } pub fn is_expired(&self) -> bool { - self.created_at.elapsed() > self.timeout + self.expired_phase().is_some() + } + + pub fn expired_phase(&self) -> Option<&'static str> { + match self.state { + DescriptorSessionState::Complete | DescriptorSessionState::Failed(_) => { + if self.created_at.elapsed() > self.timeout + Duration::from_secs(REAP_GRACE_SECS) { + return Some("reap"); + } + None + } + DescriptorSessionState::Proposed => { + if self.created_at.elapsed() > self.timeout { + return Some("session"); + } + if let Some(complete_at) = self.contributions_complete_at { + if complete_at.elapsed() > self.finalize_timeout { + return Some("finalize"); + } + } else if self.created_at.elapsed() > self.contribution_timeout { + return Some("contribution"); + } + None + } + DescriptorSessionState::Finalized => { + if self.created_at.elapsed() > self.timeout { + return Some("session"); + } + let Some(fin_at) = self.finalized_at else { + return Some("session"); + }; + if fin_at.elapsed() > self.ack_phase_timeout { + return Some("ack"); + } + None + } + } } pub fn is_participant(&self, share_index: u16) -> bool { @@ -289,7 +351,9 @@ impl DescriptorSession { } pub fn fail(&mut self, reason: String) { - self.state = DescriptorSessionState::Failed(reason); + if !self.is_complete() && !self.is_failed() { + self.state = DescriptorSessionState::Failed(reason); + } } } @@ -336,7 +400,7 @@ impl DescriptorSessionManager { self.sessions.remove(&session_id); } - self.cleanup_expired(); + let _ = self.cleanup_expired(); if self.sessions.len() >= MAX_SESSIONS { return Err(FrostNetError::Session( @@ -355,7 +419,9 @@ impl DescriptorSessionManager { ); self.sessions.insert(session_id, session); - Ok(self.sessions.get_mut(&session_id).unwrap()) + self.sessions + .get_mut(&session_id) + .ok_or_else(|| FrostNetError::Session("Failed to retrieve created session".into())) } pub fn get_session(&self, session_id: &[u8; 32]) -> Option<&DescriptorSession> { @@ -372,8 +438,17 @@ impl DescriptorSessionManager { self.sessions.remove(session_id); } - pub fn cleanup_expired(&mut self) { - self.sessions.retain(|_, session| !session.is_expired()); + pub fn cleanup_expired(&mut self) -> Vec<([u8; 32], String)> { + let mut expired = Vec::new(); + self.sessions.retain(|id, session| { + if let Some(phase) = session.expired_phase() { + expired.push((*id, format!("timeout:{phase}"))); + false + } else { + true + } + }); + expired } } @@ -835,13 +910,13 @@ mod tests { let mut session = test_session(); session - .add_contribution(1, "tpub1".into(), "aa".into()) + .add_contribution(1, "tpub1".into(), "aabb0011".into()) .unwrap(); session - .add_contribution(2, "tpub2".into(), "bb".into()) + .add_contribution(2, "tpub2".into(), "bbcc2233".into()) .unwrap(); session - .add_contribution(3, "tpub3".into(), "cc".into()) + .add_contribution(3, "tpub3".into(), "ccdd4455".into()) .unwrap(); let finalized = FinalizedDescriptor { @@ -851,7 +926,7 @@ mod tests { }; session.set_finalized(finalized).unwrap(); - let result = session.add_contribution(1, "tpub1newzzzzzzzzzzzzz".into(), "aa".into()); + let result = session.add_contribution(1, "tpub1newzzzzzzzzzzzzz".into(), "aabb0011".into()); assert!(result.is_err()); } @@ -949,7 +1024,8 @@ mod tests { .unwrap(); std::thread::sleep(Duration::from_millis(10)); - manager.cleanup_expired(); + let expired = manager.cleanup_expired(); + assert!(!expired.is_empty()); assert!(manager.get_session(&[1u8; 32]).is_none()); } @@ -1001,7 +1077,7 @@ mod tests { let session = manager.get_session_mut(&[1u8; 32]).unwrap(); session - .add_contribution(1, "tpub1".into(), "aa".into()) + .add_contribution(1, "tpub1".into(), "aabb0011".into()) .unwrap(); let session = manager.get_session(&[1u8; 32]).unwrap(); @@ -1014,6 +1090,128 @@ mod tests { assert!(manager.get_session(&[0u8; 32]).is_none()); } + #[test] + fn test_contribution_phase_timeout() { + let policy = test_policy(); + let contributors: HashSet = [1, 2, 3].into(); + let acks: HashSet = [1, 2, 3].into(); + let mut session = DescriptorSession::new( + [1u8; 32], + [2u8; 32], + policy, + "signet".into(), + contributors, + acks, + Duration::from_secs(600), + ); + session.contribution_timeout = Duration::from_millis(1); + + session + .add_contribution(1, "tpub1zzzzzzzzzzzzzzz".into(), "aabbccdd".into()) + .unwrap(); + + std::thread::sleep(Duration::from_millis(10)); + assert!(session.is_expired()); + assert_eq!(session.expired_phase(), Some("contribution")); + } + + #[test] + fn test_finalize_phase_timeout() { + let policy = test_policy(); + let contributors: HashSet = [1, 2, 3].into(); + let acks: HashSet = [1, 2, 3].into(); + let mut session = DescriptorSession::new( + [1u8; 32], + [2u8; 32], + policy, + "signet".into(), + contributors, + acks, + Duration::from_secs(600), + ); + session.finalize_timeout = Duration::from_millis(1); + + session + .add_contribution(1, "tpub1zzzzzzzzzzzzzzz".into(), "aabbccdd".into()) + .unwrap(); + session + .add_contribution(2, "tpub2zzzzzzzzzzzzzzz".into(), "11223344".into()) + .unwrap(); + session + .add_contribution(3, "tpub3zzzzzzzzzzzzzzz".into(), "55667788".into()) + .unwrap(); + assert!(session.contributions_complete_at.is_some()); + + std::thread::sleep(Duration::from_millis(10)); + assert!(session.is_expired()); + assert_eq!(session.expired_phase(), Some("finalize")); + } + + #[test] + fn test_ack_phase_timeout() { + let policy = test_policy(); + let contributors: HashSet = [1, 2, 3].into(); + let acks: HashSet = [1, 2, 3].into(); + let mut session = DescriptorSession::new( + [1u8; 32], + [2u8; 32], + policy, + "signet".into(), + contributors, + acks, + Duration::from_secs(600), + ); + session.ack_phase_timeout = Duration::from_millis(1); + + session + .add_contribution(1, "tpub1zzzzzzzzzzzzzzz".into(), "aabbccdd".into()) + .unwrap(); + session + .add_contribution(2, "tpub2zzzzzzzzzzzzzzz".into(), "11223344".into()) + .unwrap(); + session + .add_contribution(3, "tpub3zzzzzzzzzzzzzzz".into(), "55667788".into()) + .unwrap(); + + let finalized = FinalizedDescriptor { + external: "tr(frost_key)".into(), + internal: "tr(frost_key)/1".into(), + policy_hash: [0; 32], + }; + session.set_finalized(finalized).unwrap(); + assert!(session.finalized_at.is_some()); + + std::thread::sleep(Duration::from_millis(10)); + assert!(session.is_expired()); + assert_eq!(session.expired_phase(), Some("ack")); + } + + #[test] + fn test_cleanup_returns_phase_reasons() { + let mut manager = DescriptorSessionManager::with_timeout(Duration::from_secs(600)); + let policy = test_policy(); + + { + let session = manager + .create_session( + [1u8; 32], + [2u8; 32], + policy, + "signet".into(), + [1, 2, 3].into(), + [1, 2, 3].into(), + ) + .unwrap(); + session.contribution_timeout = Duration::from_millis(1); + } + + std::thread::sleep(Duration::from_millis(10)); + let expired = manager.cleanup_expired(); + assert_eq!(expired.len(), 1); + assert_eq!(expired[0].0, [1u8; 32]); + assert_eq!(expired[0].1, "timeout:contribution"); + } + #[test] fn test_duplicate_xpub_across_participants_rejected() { let mut session = test_session(); diff --git a/keep-frost-net/src/node/descriptor.rs b/keep-frost-net/src/node/descriptor.rs index 012d146e..17d6504d 100644 --- a/keep-frost-net/src/node/descriptor.rs +++ b/keep-frost-net/src/node/descriptor.rs @@ -25,10 +25,18 @@ impl KfpNode { own_xpub: &str, own_fingerprint: &str, ) -> Result<[u8; 32]> { - let created_at = chrono::Utc::now().timestamp() as u64; - let session_id = derive_descriptor_session_id(&self.group_pubkey, &policy, created_at); + if !VALID_NETWORKS.contains(&network) { + return Err(FrostNetError::Session(format!( + "Invalid network: {network}" + ))); + } let our_index = self.share.metadata.identifier; + + self.check_proposer_authorized(our_index)?; + + let created_at = chrono::Utc::now().timestamp().max(0) as u64; + let session_id = derive_descriptor_session_id(&self.group_pubkey, &policy, created_at); let expected_contributors = participant_indices(&policy); let we_are_contributor = expected_contributors.contains(&our_index); @@ -53,22 +61,23 @@ impl KfpNode { expected_acks, )?; session.set_initiator(self.keys.public_key()); - } - if we_are_contributor { - let mut sessions = self.descriptor_sessions.write(); - if let Some(session) = sessions.get_session_mut(&session_id) { - session.add_contribution( + if we_are_contributor { + if let Err(e) = session.add_contribution( our_index, own_xpub.to_string(), own_fingerprint.to_string(), - )?; + ) { + sessions.remove_session(&session_id); + return Err(e); + } } } let payload = DescriptorProposePayload::new( session_id, self.group_pubkey, + created_at, network, policy, own_xpub, @@ -140,14 +149,28 @@ impl KfpNode { )); } - { + let expected_id = derive_descriptor_session_id( + &payload.group_pubkey, + &payload.policy, + payload.created_at, + ); + if payload.session_id != expected_id { + return Err(FrostNetError::Session( + "session_id does not match derived value".into(), + )); + } + + let sender_share_index = { let peers = self.peers.read(); - if peers.get_peer_by_pubkey(&sender).is_none() { - return Err(FrostNetError::UntrustedPeer(format!( + let peer = peers.get_peer_by_pubkey(&sender).ok_or_else(|| { + FrostNetError::UntrustedPeer(format!( "Descriptor proposal from unknown peer: {sender}" - ))); - } - } + )) + })?; + peer.share_index + }; + + self.check_proposer_authorized(sender_share_index)?; info!( session_id = %hex::encode(payload.session_id), @@ -160,12 +183,7 @@ impl KfpNode { let our_index = self.share.metadata.identifier; let we_are_contributor = expected_contributors.contains(&our_index); - let initiator_share_index = { - let peers = self.peers.read(); - peers.get_peer_by_pubkey(&sender).map(|p| p.share_index) - }; - - { + let session_created = { let mut sessions = self.descriptor_sessions.write(); match sessions.create_session( payload.session_id, @@ -178,20 +196,24 @@ impl KfpNode { Ok(session) => { session.set_initiator(sender); - if let Some(idx) = initiator_share_index { - if let Err(e) = session.add_contribution( - idx, - payload.initiator_xpub.clone(), - payload.initiator_fingerprint.clone(), - ) { - debug!("Failed to store initiator contribution: {e}"); - } + if let Err(e) = session.add_contribution( + sender_share_index, + payload.initiator_xpub.clone(), + payload.initiator_fingerprint.clone(), + ) { + debug!("Failed to store initiator contribution: {e}"); } + true } Err(_) => { debug!("Descriptor session already exists, ignoring duplicate proposal"); + false } } + }; + + if !session_created { + return Ok(()); } let _ = self.event_tx.send(KfpNodeEvent::DescriptorProposed { @@ -436,23 +458,26 @@ impl KfpNode { )); } - if payload.external_descriptor.len() > MAX_DESCRIPTOR_LENGTH { - return Err(FrostNetError::Session( - "External descriptor exceeds maximum length".into(), - )); - } - if payload.internal_descriptor.len() > MAX_DESCRIPTOR_LENGTH { - return Err(FrostNetError::Session( - "Internal descriptor exceeds maximum length".into(), - )); + for (name, desc) in [ + ("External", &payload.external_descriptor), + ("Internal", &payload.internal_descriptor), + ] { + if desc.len() > MAX_DESCRIPTOR_LENGTH { + return Err(FrostNetError::Session(format!( + "{name} descriptor exceeds maximum length" + ))); + } } - { + let sender_share_index = { let peers = self.peers.read(); - if peers.get_peer_by_pubkey(&sender).is_none() { - return Err(FrostNetError::UntrustedPeer(sender.to_string())); - } - } + let peer = peers + .get_peer_by_pubkey(&sender) + .ok_or_else(|| FrostNetError::UntrustedPeer(sender.to_string()))?; + peer.share_index + }; + + self.check_proposer_authorized(sender_share_index)?; let our_index = self.share.metadata.identifier; @@ -464,29 +489,26 @@ impl KfpNode { let network = session.network().to_string(); - match session.initiator() { - Some(initiator) if *initiator != sender => { - return Err(FrostNetError::Session( - "DescriptorFinalize sender is not the session initiator".into(), - )); - } - None => { - let _ = self.event_tx.send(KfpNodeEvent::DescriptorFailed { - session_id: payload.session_id, - error: "Missing initiator".into(), - }); - return Err(FrostNetError::Session( - "DescriptorFinalize missing session initiator".into(), - )); - } - Some(_) => {} + let initiator = session.initiator().ok_or_else(|| { + let _ = self.event_tx.send(KfpNodeEvent::DescriptorFailed { + session_id: payload.session_id, + error: "Missing initiator".into(), + }); + FrostNetError::Session("DescriptorFinalize missing session initiator".into()) + })?; + if *initiator != sender { + return Err(FrostNetError::Session( + "DescriptorFinalize sender is not the session initiator".into(), + )); } let own_xpub_tampered = match session.contributions().get(&our_index) { - Some(our_stored) => payload.contributions.get(&our_index).is_none_or(|fwd| { - fwd.account_xpub != our_stored.account_xpub - || fwd.fingerprint != our_stored.fingerprint - }), + Some(our_stored) => { + let forwarded = payload.contributions.get(&our_index); + !matches!(forwarded, Some(fwd) + if fwd.account_xpub == our_stored.account_xpub + && fwd.fingerprint == our_stored.fingerprint) + } None => session.is_participant(our_index), }; @@ -497,19 +519,16 @@ impl KfpNode { let result = if own_xpub_tampered { Err("Own xpub contribution was tampered with in finalize".into()) + } else if payload.policy_hash != derive_policy_hash(session.policy()) { + Err("Policy hash does not match proposal".into()) } else { - let expected_hash = derive_policy_hash(session.policy()); - if payload.policy_hash != expected_hash { - Err("Policy hash does not match proposal".into()) - } else { - reconstruct_descriptor( - session.group_pubkey(), - session.policy(), - &payload.contributions, - session.network(), - ) - .map_err(|e| format!("Descriptor reconstruction failed: {e}")) - } + reconstruct_descriptor( + session.group_pubkey(), + session.policy(), + &payload.contributions, + session.network(), + ) + .map_err(|e| format!("Descriptor reconstruction failed: {e}")) }; (result, network, our_xpub) }; @@ -559,20 +578,26 @@ impl KfpNode { hasher.finalize().into() }; - let key_proof_psbt_bytes = { - let our_xpub = our_xpub.ok_or_else(|| { - FrostNetError::Session("Missing own xpub contribution for key proof".into()) - })?; - - let net = crate::descriptor_session::parse_network(&session_network)?; - let signing_share_bytes = self.signing_share_bytes()?; - - let mut proof_psbt = - keep_bitcoin::build_key_proof_psbt(&payload.session_id, our_index, &our_xpub, net) - .map_err(|e| FrostNetError::Crypto(format!("key proof build: {e}")))?; - - keep_bitcoin::sign_key_proof(&mut proof_psbt, &signing_share_bytes, net) - .map_err(|e| FrostNetError::Crypto(format!("key proof sign: {e}")))? + let key_proof_psbt_bytes = match self.build_key_proof( + &payload.session_id, + our_index, + our_xpub.as_deref(), + &session_network, + ) { + Ok(bytes) => bytes, + Err(e) => { + let reason = format!("Key proof failed: {e}"); + self.descriptor_sessions + .write() + .remove_session(&payload.session_id); + self.send_descriptor_nack(payload.session_id, &sender, &reason) + .await; + let _ = self.event_tx.send(KfpNodeEvent::DescriptorFailed { + session_id: payload.session_id, + error: reason.clone(), + }); + return Err(FrostNetError::Session(reason)); + } }; let ack = DescriptorAckPayload::new( @@ -606,6 +631,20 @@ impl KfpNode { .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 { + return Err(FrostNetError::Session( + "Session not found for finalize".into(), + )); + }; + session.set_finalized(FinalizedDescriptor { + external: payload.external_descriptor.clone(), + internal: payload.internal_descriptor.clone(), + policy_hash: payload.policy_hash, + })?; + } + let _ = self.event_tx.send(KfpNodeEvent::DescriptorComplete { session_id: payload.session_id, external_descriptor: payload.external_descriptor, @@ -699,7 +738,7 @@ impl KfpNode { ))); } - if session.has_nacked(share_index) { + if session.is_complete() || session.has_nacked(share_index) { return Ok(()); } @@ -841,9 +880,16 @@ impl KfpNode { } let digest: [u8; 32] = hasher.finalize().into(); let dedup_key = (payload.share_index, payload.created_at, digest); - if !self.seen_xpub_announces.write().insert(dedup_key) { + let mut seen = self.seen_xpub_announces.write(); + if seen.contains(&dedup_key) { return Ok(()); } + if seen.len() >= 10_000 { + if let Some(&oldest) = seen.iter().min_by_key(|(_, ts, _)| *ts) { + seen.remove(&oldest); + } + } + seen.insert(dedup_key); } { @@ -868,6 +914,34 @@ impl KfpNode { Ok(()) } + fn check_proposer_authorized(&self, share_index: u16) -> Result<()> { + let proposers = self.descriptor_proposers.read(); + if !proposers.is_empty() && !proposers.contains(&share_index) { + return Err(FrostNetError::Session(format!( + "Share {share_index} is not authorized to propose descriptors" + ))); + } + Ok(()) + } + + fn build_key_proof( + &self, + session_id: &[u8; 32], + share_index: u16, + xpub: Option<&str>, + network_str: &str, + ) -> Result> { + let xpub = xpub.ok_or_else(|| { + FrostNetError::Session("Missing own xpub contribution for key proof".into()) + })?; + let net = crate::descriptor_session::parse_network(network_str)?; + let signing_share_bytes = self.signing_share_bytes()?; + let mut proof_psbt = keep_bitcoin::build_key_proof_psbt(session_id, share_index, xpub, net) + .map_err(|e| FrostNetError::Crypto(format!("key proof build: {e}")))?; + keep_bitcoin::sign_key_proof(&mut proof_psbt, &signing_share_bytes, net) + .map_err(|e| FrostNetError::Crypto(format!("key proof sign: {e}"))) + } + fn signing_share_bytes(&self) -> Result> { let key_package = self .share diff --git a/keep-frost-net/src/node/mod.rs b/keep-frost-net/src/node/mod.rs index 935bc3f0..27bd87a9 100644 --- a/keep-frost-net/src/node/mod.rs +++ b/keep-frost-net/src/node/mod.rs @@ -284,6 +284,7 @@ pub struct KfpNode { pub(crate) audit_log: Arc, expected_pcrs: Option, pub(crate) seen_xpub_announces: RwLock>, + pub(crate) descriptor_proposers: RwLock>, } impl KfpNode { @@ -398,6 +399,7 @@ impl KfpNode { audit_log, expected_pcrs: None, seen_xpub_announces: RwLock::new(HashSet::new()), + descriptor_proposers: RwLock::new(HashSet::new()), }) } @@ -516,6 +518,14 @@ impl KfpNode { .map(|xpubs| xpubs.to_vec()) } + pub fn set_descriptor_proposers(&self, indices: HashSet) { + *self.descriptor_proposers.write() = indices; + } + + pub fn descriptor_proposers(&self) -> HashSet { + self.descriptor_proposers.read().clone() + } + pub fn set_hooks(&self, hooks: Arc) { *self.hooks.write() = hooks; } @@ -719,7 +729,13 @@ impl KfpNode { _ = cleanup_interval.tick() => { self.sessions.write().cleanup_expired(); self.ecdh_sessions.write().cleanup_expired(); - self.descriptor_sessions.write().cleanup_expired(); + let expired = self.descriptor_sessions.write().cleanup_expired(); + for (session_id, reason) in expired { + let _ = self.event_tx.send(KfpNodeEvent::DescriptorFailed { + session_id, + error: reason, + }); + } { let now = chrono::Utc::now().timestamp().max(0) as u64; let window = self.replay_window_secs + MAX_FUTURE_SKEW_SECS; diff --git a/keep-frost-net/src/protocol.rs b/keep-frost-net/src/protocol.rs index cc883e20..65ed40ec 100644 --- a/keep-frost-net/src/protocol.rs +++ b/keep-frost-net/src/protocol.rs @@ -26,6 +26,9 @@ pub const MAX_XPUB_LENGTH: usize = 256; pub const MAX_FINGERPRINT_LENGTH: usize = 8; pub const DESCRIPTOR_SESSION_TIMEOUT_SECS: u64 = 600; pub const DESCRIPTOR_ACK_TIMEOUT_SECS: u64 = 60; +pub const DESCRIPTOR_CONTRIBUTION_TIMEOUT_SECS: u64 = 300; +pub const DESCRIPTOR_FINALIZE_TIMEOUT_SECS: u64 = 120; +pub const DESCRIPTOR_ACK_PHASE_TIMEOUT_SECS: u64 = 120; pub const MAX_DESCRIPTOR_LENGTH: usize = 4096; pub const MAX_NACK_REASON_LENGTH: usize = 1024; pub const MAX_RECOVERY_XPUBS: usize = 20; @@ -37,12 +40,22 @@ pub const VALID_NETWORKS: &[&str] = &["bitcoin", "testnet", "signet", "regtest"] pub(crate) const MAX_FUTURE_SKEW_SECS: u64 = 30; fn within_replay_window(created_at: u64, window_secs: u64) -> bool { - let now = chrono::Utc::now().timestamp() as u64; + let now = chrono::Utc::now().timestamp().max(0) as u64; let min_valid = now.saturating_sub(window_secs); let max_valid = now.saturating_add(MAX_FUTURE_SKEW_SECS); created_at >= min_valid && created_at <= max_valid } +fn is_valid_xpub(xpub: &str) -> bool { + xpub.len() >= MIN_XPUB_LENGTH + && xpub.len() <= MAX_XPUB_LENGTH + && VALID_XPUB_PREFIXES.iter().any(|pfx| xpub.starts_with(pfx)) +} + +fn is_valid_fingerprint(fp: &str) -> bool { + fp.len() == MAX_FINGERPRINT_LENGTH && fp.chars().all(|c| c.is_ascii_hexdigit()) +} + #[derive(Serialize, Deserialize, Debug)] #[serde(tag = "type", rename_all = "snake_case")] pub enum KfpMessage { @@ -262,17 +275,11 @@ impl KfpMessage { if !VALID_NETWORKS.contains(&p.network.as_str()) { return Err("Invalid network value"); } - if p.initiator_xpub.is_empty() { - return Err("Initiator xpub cannot be empty"); - } - if p.initiator_xpub.len() > MAX_XPUB_LENGTH { - return Err("Initiator xpub exceeds maximum length"); - } - if p.initiator_fingerprint.is_empty() { - return Err("Initiator fingerprint cannot be empty"); + if !is_valid_xpub(&p.initiator_xpub) { + return Err("Invalid initiator xpub"); } - if p.initiator_fingerprint.len() > MAX_FINGERPRINT_LENGTH { - return Err("Initiator fingerprint exceeds maximum length"); + if !is_valid_fingerprint(&p.initiator_fingerprint) { + return Err("Initiator fingerprint must be exactly 8 hex characters"); } if p.policy.recovery_tiers.is_empty() { return Err("Policy must have at least one recovery tier"); @@ -294,12 +301,21 @@ impl KfpMessage { return Err("Tier threshold exceeds number of key slots"); } for slot in &tier.key_slots { - if let KeySlot::External { xpub, fingerprint } = slot { - if xpub.len() > MAX_XPUB_LENGTH { - return Err("External xpub exceeds maximum length"); + match slot { + KeySlot::Participant { share_index } => { + if *share_index == 0 { + return Err("Participant share_index must be non-zero"); + } } - if fingerprint.len() > MAX_FINGERPRINT_LENGTH { - return Err("External fingerprint exceeds maximum length"); + KeySlot::External { xpub, fingerprint } => { + if !is_valid_xpub(xpub) { + return Err("Invalid external xpub"); + } + if !is_valid_fingerprint(fingerprint) { + return Err( + "External fingerprint must be exactly 8 hex characters", + ); + } } } } @@ -309,17 +325,11 @@ impl KfpMessage { if p.share_index == 0 { return Err("share_index must be non-zero"); } - if p.account_xpub.len() < MIN_XPUB_LENGTH { - return Err("Account xpub too short"); + if !is_valid_xpub(&p.account_xpub) { + return Err("Invalid account xpub"); } - if p.account_xpub.len() > MAX_XPUB_LENGTH { - return Err("Account xpub exceeds maximum length"); - } - if p.fingerprint.is_empty() { - return Err("Fingerprint cannot be empty"); - } - if p.fingerprint.len() > MAX_FINGERPRINT_LENGTH { - return Err("Fingerprint exceeds maximum length"); + if !is_valid_fingerprint(&p.fingerprint) { + return Err("Fingerprint must be exactly 8 hex characters"); } } KfpMessage::DescriptorFinalize(p) => { @@ -338,23 +348,18 @@ impl KfpMessage { if p.contributions.len() > MAX_PARTICIPANTS { return Err("Too many contributions in finalize payload"); } - for &idx in p.contributions.keys() { + for (&idx, contrib) in &p.contributions { if idx == 0 { return Err("Invalid contribution index"); } if idx as usize > MAX_PARTICIPANTS { return Err("Contribution index exceeds maximum"); } - } - for contrib in p.contributions.values() { - if contrib.account_xpub.len() < MIN_XPUB_LENGTH { - return Err("Contribution xpub too short"); + if !is_valid_xpub(&contrib.account_xpub) { + return Err("Invalid contribution xpub"); } - if contrib.account_xpub.len() > MAX_XPUB_LENGTH { - return Err("Contribution xpub exceeds maximum length"); - } - if contrib.fingerprint.len() > MAX_FINGERPRINT_LENGTH { - return Err("Contribution fingerprint exceeds maximum length"); + if !is_valid_fingerprint(&contrib.fingerprint) { + return Err("Contribution fingerprint must be exactly 8 hex characters"); } } } @@ -963,6 +968,7 @@ impl DescriptorProposePayload { pub fn new( session_id: [u8; 32], group_pubkey: [u8; 32], + created_at: u64, network: &str, policy: WalletPolicy, initiator_xpub: &str, @@ -971,7 +977,7 @@ impl DescriptorProposePayload { Self { session_id, group_pubkey, - created_at: chrono::Utc::now().timestamp() as u64, + created_at, network: network.to_string(), policy, initiator_xpub: initiator_xpub.to_string(),