From 183ad148de81b21a03060e4543ea0191423d3baf Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Wed, 13 May 2026 23:43:21 +0000 Subject: [PATCH] Fix XX rekey dual-init race: tie-break also when pending_new_session set The dual-initiation tie-breaker in the rekey arm of handle_msg3 (and the FSP analogue in handle_session_setup) only fired when rekey_in_progress was true. With Noise IK (one-message rekey on master/maint) that is sufficient: msg1 IS the rekey, so both sides being mid-handshake is the only state where the race can fire. With Noise XX (three-message rekey on next), set_pending_session runs when the initiator has processed msg2 and sent msg3, and that call clears rekey_in_progress. Both sides' set_pending_session can run before either peer's msg3 has landed. Each side then receives the peer's msg3 in the post-pending state with rekey_in_progress=false, falls into the "drop because pending_new_session is set" guard, and discards the peer's handshake. Each side commits its own initiator session at K-bit cutover. The two sessions use different Noise key material, so the link breaks asymmetrically after cutover. Unify both checks into a single tie-breaker that fires when either rekey_in_progress() OR pending_new_session().is_some() is true. The existing smaller-NodeAddr rule applies uniformly; abandon_rekey() already clears both states and returns whichever index needs freeing. Mirrored to the FSP rekey msg1 path in handle_session_setup for the same race shape. Logging: - The previously-silent drop in the rekey arm of handle_msg3 logs at info as a tie-break decision rather than as a silent drop. Pending_new_session is added as a log field on the tie-break win/lose lines so a reader can distinguish which of the two race states fired. - Cutover-complete and K-bit-flip log lines gained our_addr and their_addr fields so the two endpoints' logs of the same handshake can be correlated. - "Pending session set, awaiting K-bit cutover" lines remain at debug per the existing convention of info-for-cutover-completion- only. Verification: under the un-fixed handler, both sides log "rekey-msg3 drop: pending_new_session already set" within 242 microseconds of each other, with rekey_in_progress=false and different pending session indices. Failure pattern matches: six of twenty pairs FAILED post-rekey, all involving the single-peer node. Six consecutive post-fix CI attempts on the same test green across rekey, rekey-accept-off, and rekey-outbound-only suites. --- CHANGELOG.md | 14 ++++++++ src/node/handlers/encrypted.rs | 6 ++++ src/node/handlers/handshake.rs | 63 +++++++++++++++++++++------------- src/node/handlers/rekey.rs | 10 ++++-- src/node/handlers/session.rs | 48 +++++++++++++++++--------- 5 files changed, 98 insertions(+), 43 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a7a765d..e97c114 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -99,6 +99,20 @@ with v0.3.x peers. ## [Unreleased] +### Fixed + +- XX rekey dual-initiation race that broke six pair-directions + post-rekey when both endpoints initiated rekey simultaneously. + The `handle_msg3` tie-breaker only fired when `rekey_in_progress` + was still true, but XX's three-message handshake lets both sides + clear that flag (via `set_pending_session`) before either's msg3 + lands. The drop-on-pending-session guard then silently discarded + the peer's msg3, each side cut over to its own initiator session, + and the link broke asymmetrically. The tie-breaker now also fires + when `pending_new_session().is_some()`, applying the same + smaller-NodeAddr resolution rule. Mirrored to the FSP rekey msg1 + path for symmetry. + ## [0.3.0] - 2026-05-11 ### Added diff --git a/src/node/handlers/encrypted.rs b/src/node/handlers/encrypted.rs index 96dff61..f8de95e 100644 --- a/src/node/handlers/encrypted.rs +++ b/src/node/handlers/encrypted.rs @@ -60,8 +60,14 @@ impl Node { if k_bit_flipped { let display_name = self.peer_display_name(&node_addr); + let pending_our = peer.pending_our_index(); + let pending_their = peer.pending_their_index(); info!( peer = %display_name, + our_addr = %self.identity.node_addr(), + their_addr = %node_addr, + pending_our_index = ?pending_our, + pending_their_index = ?pending_their, "Peer K-bit flip detected, promoting new session" ); diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index a2a9652..3df8ad7 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -387,9 +387,10 @@ impl Node { debug!( peer = %display_name, + our_addr = %self.identity.node_addr(), new_our_index = %our_index, new_their_index = %header.sender_idx, - "Rekey completed (initiator), pending K-bit cutover" + "rekey-msg2 initiator: pending session set, awaiting K-bit cutover" ); } else { // msg3 send failed — abandon rekey @@ -1015,36 +1016,46 @@ impl Node { && existing_peer.is_healthy() && session_age_secs >= 30 { - // Guard: already have a pending session from a completed - // rekey (waiting for K-bit cutover). Don't overwrite. - if existing_peer.pending_new_session().is_some() { - debug!( - peer = %self.peer_display_name(&peer_node_addr), - "Rekey msg3 received but already have pending session, dropping" - ); - self.connections.remove(&link_id); - self.links.remove(&link_id); - return; - } - - // Dual-initiation detection: both sides sent msg1 - // simultaneously. Apply tie-breaker. - if existing_peer.rekey_in_progress() { + // Dual-initiation detection: both sides initiated rekey + // simultaneously. Two states can reach this point: + // - rekey_in_progress=true: both sides still mid-handshake + // - pending_new_session=Some && !rekey_in_progress: both + // sides already completed their initiator path + // (set_pending_session cleared rekey_in_progress) + // The IK fix only caught the first state; the XX three-message + // handshake widens the window so the second state is reached + // when both sides' set_pending_session runs before either's + // msg3 lands at the peer. Apply the smaller-NodeAddr + // tie-breaker uniformly in both states so both sides converge + // on a single Noise session post-cutover. + if existing_peer.rekey_in_progress() + || existing_peer.pending_new_session().is_some() + { let our_addr = self.identity.node_addr(); if our_addr < &peer_node_addr { - // We win as initiator — drop their msg3/handshake. - debug!( + // We win — keep our session, drop their msg3. + info!( peer = %self.peer_display_name(&peer_node_addr), - "Dual rekey initiation: we win (smaller addr), dropping their msg3" + our_addr = %our_addr, + their_addr = %peer_node_addr, + rekey_in_progress = existing_peer.rekey_in_progress(), + pending_new_session = existing_peer.pending_new_session().is_some(), + "rekey-msg3 tie-break: we win (smaller addr), drop their msg3" ); self.connections.remove(&link_id); self.links.remove(&link_id); return; } - // We lose — abandon our rekey, become responder below. - debug!( + // We lose — abandon our rekey/pending, fall through as responder. + // abandon_rekey clears both rekey_in_progress and any pending + // session state, returning whichever index needs freeing. + info!( peer = %self.peer_display_name(&peer_node_addr), - "Dual rekey initiation: we lose (larger addr), abandoning ours" + our_addr = %our_addr, + their_addr = %peer_node_addr, + rekey_in_progress = existing_peer.rekey_in_progress(), + pending_new_session = existing_peer.pending_new_session().is_some(), + "rekey-msg3 tie-break: we lose (larger addr), abandon ours" ); if let Some(peer) = self.peers.get_mut(&peer_node_addr) && let Some(idx) = peer.abandon_rekey() @@ -1103,8 +1114,10 @@ impl Node { debug!( peer = %self.peer_display_name(&peer_node_addr), + our_addr = %self.identity.node_addr(), new_our_index = %our_new_index, - "Rekey completed (responder), pending K-bit cutover" + new_their_index = %header.sender_idx, + "rekey-msg3 responder: pending session set, awaiting K-bit cutover" ); return; } @@ -1282,8 +1295,10 @@ impl Node { debug!( peer = %display_name, + our_addr = %self.identity.node_addr(), new_our_index = %our_index, - "Rekey msg3 completed (responder), pending K-bit cutover" + new_their_index = %header.sender_idx, + "rekey-msg3 responder (existing link): pending session set, awaiting K-bit cutover" ); } Err(e) => { diff --git a/src/node/handlers/rekey.rs b/src/node/handlers/rekey.rs index 5c9e1e1..193ea56 100644 --- a/src/node/handlers/rekey.rs +++ b/src/node/handlers/rekey.rs @@ -10,7 +10,7 @@ use crate::node::Node; use crate::node::wire::build_msg1; use crate::noise::HandshakeState; use crate::protocol::{SessionDatagram, SessionSetup}; -use tracing::{debug, trace, warn}; +use tracing::{debug, info, trace, warn}; /// Keep previous session alive for this long after cutover. const DRAIN_WINDOW_SECS: u64 = 10; @@ -95,8 +95,14 @@ impl Node { )), "peers_by_index should contain pre-registered new index after cutover" ); - debug!( + let our_index = peer.our_index(); + let their_index = peer.their_index(); + info!( peer = %self.peer_display_name(&node_addr), + our_addr = %self.identity.node_addr(), + their_addr = %node_addr, + our_index = ?our_index, + their_index = ?their_index, "Rekey cutover complete (initiator), K-bit flipped" ); } diff --git a/src/node/handlers/session.rs b/src/node/handlers/session.rs index 0206ce3..d763216 100644 --- a/src/node/handlers/session.rs +++ b/src/node/handlers/session.rs @@ -189,6 +189,8 @@ impl Node { let display_name = self.peer_display_name(src_addr); info!( peer = %display_name, + our_addr = %self.identity.node_addr(), + their_addr = %src_addr, "Peer FSP K-bit flip detected, promoting new session" ); let now_ms = Self::now_ms(); @@ -432,33 +434,41 @@ impl Node { let rekey_in_progress = existing.has_rekey_in_progress(); let has_pending = existing.pending_new_session().is_some(); - // Dual-initiation detection: both sides sent SessionSetup - // simultaneously. Apply tie-breaker — smaller NodeAddr - // wins as initiator (same as initial session setup). - if rekey_in_progress { + // Dual-initiation detection: both sides initiated rekey + // simultaneously. Two states can reach this point: + // - rekey_in_progress=true: both sides still mid-handshake + // - pending_new_session=Some && !rekey_in_progress: we've + // already completed our initiator path + // (set_pending_session cleared rekey_in_progress) and + // the peer's msg1 arrives now. Symmetric of the FMP + // msg3 case at handle_msg3. + // Apply the smaller-NodeAddr tie-breaker uniformly so + // both sides converge on a single Noise session. + if rekey_in_progress || has_pending { if self.identity.node_addr() < src_addr { - // We win as initiator — drop their msg1. - debug!( + // We win — keep our session, drop their msg1. + info!( src = %self.peer_display_name(src_addr), - "Dual FSP rekey initiation: we win (smaller addr), dropping their msg1" + our_addr = %self.identity.node_addr(), + their_addr = %src_addr, + rekey_in_progress = rekey_in_progress, + pending_new_session = has_pending, + "FSP rekey-msg1 tie-break: we win (smaller addr), drop their msg1" ); return; } - // We lose — abandon our rekey, become responder below. - debug!( + // We lose — abandon our rekey/pending, fall through as responder. + info!( src = %self.peer_display_name(src_addr), - "Dual FSP rekey initiation: we lose (larger addr), abandoning ours" + our_addr = %self.identity.node_addr(), + their_addr = %src_addr, + rekey_in_progress = rekey_in_progress, + pending_new_session = has_pending, + "FSP rekey-msg1 tie-break: we lose (larger addr), abandon ours" ); if let Some(entry) = self.sessions.get_mut(src_addr) { entry.abandon_rekey(); } - } else if has_pending { - // Guard: already have a pending session waiting for K-bit cutover - debug!( - src = %self.peer_display_name(src_addr), - "FSP rekey msg1 received but already have pending session, dropping" - ); - return; } let our_keypair = self.identity.keypair(); let mut handshake = HandshakeState::new_responder(our_keypair); @@ -666,6 +676,8 @@ impl Node { debug!( src = %self.peer_display_name(src_addr), + our_addr = %self.identity.node_addr(), + their_addr = %src_addr, "FSP rekey: completed XX as initiator, pending cutover" ); return; @@ -847,6 +859,8 @@ impl Node { debug!( src = %self.peer_display_name(src_addr), + our_addr = %self.identity.node_addr(), + their_addr = %src_addr, "FSP rekey: completed XX as responder, pending cutover" ); return;