mirror of
https://github.com/jmcorgan/fips.git
synced 2026-08-09 00:04:54 +00:00
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.
This commit is contained in:
Johnathan Corgan
@@ -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
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
|
||||
|
||||
@@ -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) => {
|
||||
|
||||
@@ -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"
|
||||
);
|
||||
}
|
||||
|
||||
@@ -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;
|
||||
|
||||
Reference in New Issue
Block a user