mirror of
https://github.com/jmcorgan/fips.git
synced 2026-08-10 00:26:59 +00:00
Fix rekey dual-initiation, parent selection, peer reconnect, and control socket
Rekey dual-initiation race (FMP + FSP): When both sides' rekey timers fire simultaneously on high-latency links (Tor ~700ms RTT), both msg1s cross in flight before dampening can suppress the second initiation. Each node acts as both initiator and responder, with set_pending_session() from the responder path destroying the initiator's in-progress state. Each side ends up with a pending_new_session from a different Noise handshake, causing AEAD verification failure after cutover and link death. Fix: deterministic tie-breaker in both FMP msg1 handler and FSP SessionSetup handler (smaller NodeAddr wins as initiator). Also guard against pending session overwrite from retransmitted msg1s. Parent selection SRTT gate: Peers without MMP RTT data are excluded from parent candidacy via has_srtt() filter, preventing freshly reconnected peers from appearing artificially attractive. First-RTT triggers immediate parent re-eval. The gate in evaluate_parent() skips unmeasured candidates when any peer has cost data, but falls back to default cost 1.0 during cold start when no peer has MMP data yet. Auto-reconnect on all peer removal paths: The excessive-decryption-failure and peer-restart epoch-mismatch removal paths were missing schedule_reconnect() calls, causing auto-connect peers to be permanently abandoned after those events. Control socket accessibility: Socket and parent directory chowned to root:fips with mode 0770/0750 so group members can use fipsctl/fipstop without root. Log level changes: Default RUST_LOG changed to debug in systemd service files. "Unknown session index" log reduced from debug to trace.
This commit is contained in:
@@ -27,7 +27,7 @@ impl Node {
|
||||
}
|
||||
0x02 => {
|
||||
// ReceiverReport
|
||||
self.handle_receiver_report(from, payload);
|
||||
self.handle_receiver_report(from, payload).await;
|
||||
}
|
||||
0x10 => {
|
||||
// TreeAnnounce
|
||||
|
||||
@@ -5,7 +5,7 @@ use crate::node::Node;
|
||||
use crate::node::wire::{EncryptedHeader, strip_inner_header, FLAG_CE, FLAG_KEY_EPOCH, FLAG_SP};
|
||||
use crate::transport::ReceivedPacket;
|
||||
use std::time::Instant;
|
||||
use tracing::{debug, info, warn};
|
||||
use tracing::{debug, info, trace, warn};
|
||||
|
||||
/// Force-remove a peer after this many consecutive decryption failures.
|
||||
const DECRYPT_FAILURE_THRESHOLD: u32 = 20;
|
||||
@@ -32,7 +32,7 @@ impl Node {
|
||||
let node_addr = match self.peers_by_index.get(&key) {
|
||||
Some(id) => *id,
|
||||
None => {
|
||||
debug!(
|
||||
trace!(
|
||||
receiver_idx = %header.receiver_idx,
|
||||
transport_id = %packet.transport_id,
|
||||
"Unknown session index, dropping"
|
||||
@@ -221,7 +221,13 @@ impl Node {
|
||||
consecutive_failures = count,
|
||||
"Excessive decryption failures, removing peer"
|
||||
);
|
||||
let addr = *node_addr;
|
||||
self.remove_active_peer(node_addr);
|
||||
let now_ms = std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
.map(|d| d.as_millis() as u64)
|
||||
.unwrap_or(0);
|
||||
self.schedule_reconnect(addr, now_ms);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -176,6 +176,11 @@ impl Node {
|
||||
"Peer restart detected (epoch mismatch), removing stale session"
|
||||
);
|
||||
self.remove_active_peer(&peer_node_addr);
|
||||
let now_ms = std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
.map(|d| d.as_millis() as u64)
|
||||
.unwrap_or(0);
|
||||
self.schedule_reconnect(peer_node_addr, now_ms);
|
||||
// Fall through to process as new connection
|
||||
}
|
||||
_ => {
|
||||
@@ -196,6 +201,55 @@ 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 it
|
||||
// with a new handshake — drop this msg1.
|
||||
if existing_peer.pending_new_session().is_some() {
|
||||
debug!(
|
||||
peer = %self.peer_display_name(&peer_node_addr),
|
||||
"Rekey msg1 received but already have pending session, dropping"
|
||||
);
|
||||
self.connections.remove(&link_id);
|
||||
self.links.remove(&link_id);
|
||||
self.msg1_rate_limiter.complete_handshake();
|
||||
return;
|
||||
}
|
||||
|
||||
// Dual-initiation detection: both sides sent msg1
|
||||
// simultaneously. Apply tie-breaker — smaller NodeAddr
|
||||
// wins as initiator (same as cross-connection resolution).
|
||||
if existing_peer.rekey_in_progress() {
|
||||
let our_addr = self.identity.node_addr();
|
||||
if our_addr < &peer_node_addr {
|
||||
// We win as initiator — drop their msg1.
|
||||
// Our msg2 will arrive at peer, who completes
|
||||
// as our responder.
|
||||
debug!(
|
||||
peer = %self.peer_display_name(&peer_node_addr),
|
||||
"Dual rekey initiation: we win (smaller addr), dropping their msg1"
|
||||
);
|
||||
self.connections.remove(&link_id);
|
||||
self.links.remove(&link_id);
|
||||
self.msg1_rate_limiter.complete_handshake();
|
||||
return;
|
||||
}
|
||||
// We lose — abandon our rekey, become responder below.
|
||||
debug!(
|
||||
peer = %self.peer_display_name(&peer_node_addr),
|
||||
"Dual rekey initiation: we lose (larger addr), abandoning ours"
|
||||
);
|
||||
if let Some(peer) = self.peers.get_mut(&peer_node_addr)
|
||||
&& let Some(idx) = peer.abandon_rekey()
|
||||
{
|
||||
if let Some(tid) = peer.transport_id() {
|
||||
self.peers_by_index.remove(&(tid, idx.as_u32()));
|
||||
self.pending_outbound.remove(&(tid, idx.as_u32()));
|
||||
}
|
||||
let _ = self.index_allocator.free(idx);
|
||||
}
|
||||
// Fall through to respond as responder
|
||||
}
|
||||
|
||||
// Rekey: process as responder, store new session as pending
|
||||
let noise_session = conn.take_session();
|
||||
let our_new_index = match self.index_allocator.allocate() {
|
||||
|
||||
@@ -72,7 +72,7 @@ impl Node {
|
||||
///
|
||||
/// The peer is telling us about what they received from us. We feed
|
||||
/// this to our metrics to compute RTT, loss rate, and trend indicators.
|
||||
pub(in crate::node) fn handle_receiver_report(&mut self, from: &NodeAddr, payload: &[u8]) {
|
||||
pub(in crate::node) async fn handle_receiver_report(&mut self, from: &NodeAddr, payload: &[u8]) {
|
||||
let rr = match ReceiverReport::decode(payload) {
|
||||
Ok(rr) => rr,
|
||||
Err(e) => {
|
||||
@@ -101,7 +101,7 @@ impl Node {
|
||||
// Process the report: computes RTT from timestamp echo, updates
|
||||
// loss rate, goodput rate, jitter trend, and ETX.
|
||||
let now = Instant::now();
|
||||
mmp.metrics.process_receiver_report(&rr, our_timestamp_ms, now);
|
||||
let first_rtt = mmp.metrics.process_receiver_report(&rr, our_timestamp_ms, now);
|
||||
|
||||
// Feed SRTT back to sender/receiver report interval tuning
|
||||
if let Some(srtt_ms) = mmp.metrics.srtt_ms() {
|
||||
@@ -126,6 +126,47 @@ impl Node {
|
||||
etx = format_args!("{:.2}", mmp.metrics.etx),
|
||||
"Processed ReceiverReport"
|
||||
);
|
||||
|
||||
// First RTT sample — peer is now eligible for parent selection.
|
||||
// Trigger re-evaluation so the node doesn't wait for the next
|
||||
// periodic tick or TreeAnnounce.
|
||||
if first_rtt {
|
||||
let peer_costs: std::collections::HashMap<crate::NodeAddr, f64> = self.peers.iter()
|
||||
.filter(|(_, p)| p.has_srtt())
|
||||
.map(|(a, p)| (*a, p.link_cost()))
|
||||
.collect();
|
||||
if let Some(new_parent) = self.tree_state.evaluate_parent(&peer_costs) {
|
||||
let new_seq = self.tree_state.my_declaration().sequence() + 1;
|
||||
let timestamp = std::time::SystemTime::now()
|
||||
.duration_since(std::time::UNIX_EPOCH)
|
||||
.map(|d| d.as_secs())
|
||||
.unwrap_or(0);
|
||||
let flap_dampened = self.tree_state.set_parent(new_parent, new_seq, timestamp);
|
||||
if let Err(e) = self.tree_state.sign_declaration(&self.identity) {
|
||||
warn!(error = %e, "Failed to sign declaration after first-RTT parent eval");
|
||||
return;
|
||||
}
|
||||
self.tree_state.recompute_coords();
|
||||
self.coord_cache.clear();
|
||||
self.stats_mut().tree.parent_switched += 1;
|
||||
self.stats_mut().tree.parent_switches += 1;
|
||||
info!(
|
||||
new_parent = %self.peer_display_name(&new_parent),
|
||||
new_seq = new_seq,
|
||||
new_root = %self.tree_state.root(),
|
||||
depth = self.tree_state.my_coords().depth(),
|
||||
trigger = "first-rtt",
|
||||
"Parent switched after first RTT measurement"
|
||||
);
|
||||
if flap_dampened {
|
||||
self.stats_mut().tree.flap_dampened += 1;
|
||||
warn!("Flap dampening engaged: excessive parent switches detected");
|
||||
}
|
||||
self.send_tree_announce_to_all().await;
|
||||
let all_peers: Vec<crate::NodeAddr> = self.peers.keys().copied().collect();
|
||||
self.bloom_state.mark_all_updates_needed(all_peers);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// Check all peers for pending MMP reports and send them.
|
||||
|
||||
@@ -410,7 +410,37 @@ impl Node {
|
||||
} else if existing.is_established() {
|
||||
// Rekey: if rekey enabled, treat as rekey for key rotation.
|
||||
// The existing established session remains active for traffic.
|
||||
if self.config.node.rekey.enabled && !existing.has_rekey_in_progress() {
|
||||
if self.config.node.rekey.enabled {
|
||||
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 {
|
||||
if self.identity.node_addr() < src_addr {
|
||||
// We win as initiator — drop their msg1.
|
||||
debug!(
|
||||
src = %self.peer_display_name(src_addr),
|
||||
"Dual FSP rekey initiation: we win (smaller addr), dropping their msg1"
|
||||
);
|
||||
return;
|
||||
}
|
||||
// We lose — abandon our rekey, become responder below.
|
||||
debug!(
|
||||
src = %self.peer_display_name(src_addr),
|
||||
"Dual FSP rekey initiation: we lose (larger addr), abandoning ours"
|
||||
);
|
||||
let entry = self.sessions.get_mut(src_addr).unwrap();
|
||||
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_xk_responder(our_keypair);
|
||||
handshake.set_local_epoch(self.startup_epoch);
|
||||
|
||||
+7
-1
@@ -211,8 +211,11 @@ impl Node {
|
||||
self.bloom_state.mark_update_needed(*from);
|
||||
}
|
||||
|
||||
// Re-evaluate parent selection with current link costs
|
||||
// Re-evaluate parent selection with current link costs.
|
||||
// Exclude peers without MMP RTT data — they are not yet eligible
|
||||
// as parent candidates (prevents oscillation from optimistic defaults).
|
||||
let peer_costs: HashMap<NodeAddr, f64> = self.peers.iter()
|
||||
.filter(|(_, peer)| peer.has_srtt())
|
||||
.map(|(addr, peer)| (*addr, peer.link_cost()))
|
||||
.collect();
|
||||
if let Some(new_parent) = self.tree_state.evaluate_parent(&peer_costs) {
|
||||
@@ -263,6 +266,7 @@ impl Node {
|
||||
"Parent ancestry contains us — loop detected, dropping parent"
|
||||
);
|
||||
let peer_costs: HashMap<NodeAddr, f64> = self.peers.iter()
|
||||
.filter(|(_, peer)| peer.has_srtt())
|
||||
.map(|(addr, peer)| (*addr, peer.link_cost()))
|
||||
.collect();
|
||||
if self.tree_state.handle_parent_lost(&peer_costs) {
|
||||
@@ -354,6 +358,7 @@ impl Node {
|
||||
self.last_parent_reeval = Some(now);
|
||||
|
||||
let peer_costs: HashMap<NodeAddr, f64> = self.peers.iter()
|
||||
.filter(|(_, peer)| peer.has_srtt())
|
||||
.map(|(addr, peer)| (*addr, peer.link_cost()))
|
||||
.collect();
|
||||
|
||||
@@ -410,6 +415,7 @@ impl Node {
|
||||
if was_parent {
|
||||
self.stats_mut().tree.parent_losses += 1;
|
||||
let peer_costs: HashMap<NodeAddr, f64> = self.peers.iter()
|
||||
.filter(|(_, peer)| peer.has_srtt())
|
||||
.map(|(addr, peer)| (*addr, peer.link_cost()))
|
||||
.collect();
|
||||
let changed = self.tree_state.handle_parent_lost(&peer_costs);
|
||||
|
||||
Reference in New Issue
Block a user