mirror of
https://github.com/jmcorgan/fips.git
synced 2026-08-12 09:33:23 +00:00
Fix FSP rekey cutover race and MMP metric discontinuity
The FSP XK rekey handshake has a race condition where the initiator can cut over (K-bit flip) and send data encrypted with the new session before msg3 reaches the responder. The responder has no pending session yet, so K-bit detection fails and packets are dropped. Defer FSP initiator cutover by 2 seconds after handshake completion (FSP_CUTOVER_DELAY_MS) to give msg3 time to traverse the mesh. FMP (IK, 2 messages) is unaffected since the responder completes during msg1 processing. Also fix MMP metric corruption after rekey cutover: the new session starts with counter 0 but MMP state carries highest_counter and GapTracker.expected_next from the old session, producing false reorder counts, jitter spikes, and invalid OWD trends. Add reset_for_rekey() methods that clear counter-dependent state while preserving RTT estimates. Additional fixes: - Remove stale peers_by_index entry on abandon_rekey error path - Replace redundant peers_by_index inserts with debug assertions verifying the pre-registration invariant - Tighten rekey integration test to zero tolerance (was 4 failures)
This commit is contained in:
@@ -192,6 +192,11 @@ impl OwdTrendDetector {
|
||||
}
|
||||
}
|
||||
|
||||
/// Clear all samples, keeping the same capacity.
|
||||
pub fn clear(&mut self) {
|
||||
self.samples.clear();
|
||||
}
|
||||
|
||||
/// Add an OWD sample.
|
||||
///
|
||||
/// `seq` is a monotonic sequence number (e.g., truncated frame counter).
|
||||
|
||||
@@ -48,6 +48,22 @@ pub struct MmpMetrics {
|
||||
}
|
||||
|
||||
impl MmpMetrics {
|
||||
/// Reset state derived from ReceiverReport counters for rekey cutover.
|
||||
///
|
||||
/// The new session starts with counter 0, so the prev_rr deltas must
|
||||
/// be reset to avoid computing bogus loss/goodput from the counter
|
||||
/// discontinuity. RTT (SRTT) is preserved since it remains valid.
|
||||
pub fn reset_for_rekey(&mut self) {
|
||||
self.prev_rr_cum_packets = 0;
|
||||
self.prev_rr_cum_bytes = 0;
|
||||
self.prev_rr_highest_counter = 0;
|
||||
self.prev_rr_ecn_ce = 0;
|
||||
self.prev_rr_reorder = 0;
|
||||
self.prev_rr_time = None;
|
||||
self.delivery_ratio_forward = 1.0;
|
||||
// Keep srtt, etx, trends, goodput_bps — they'll refresh from data
|
||||
}
|
||||
|
||||
pub fn new() -> Self {
|
||||
Self {
|
||||
srtt: SrttEstimator::new(),
|
||||
|
||||
@@ -197,6 +197,12 @@ impl MmpPeerState {
|
||||
}
|
||||
}
|
||||
|
||||
/// Reset counter-dependent state for rekey cutover.
|
||||
pub fn reset_for_rekey(&mut self) {
|
||||
self.receiver.reset_for_rekey();
|
||||
self.metrics.reset_for_rekey();
|
||||
}
|
||||
|
||||
/// Current operating mode.
|
||||
pub fn mode(&self) -> MmpMode {
|
||||
self.mode
|
||||
@@ -256,6 +262,12 @@ impl MmpSessionState {
|
||||
}
|
||||
}
|
||||
|
||||
/// Reset counter-dependent state for rekey cutover.
|
||||
pub fn reset_for_rekey(&mut self) {
|
||||
self.receiver.reset_for_rekey();
|
||||
self.metrics.reset_for_rekey();
|
||||
}
|
||||
|
||||
/// Current operating mode.
|
||||
pub fn mode(&self) -> MmpMode {
|
||||
self.mode
|
||||
|
||||
@@ -198,6 +198,28 @@ impl ReceiverState {
|
||||
}
|
||||
}
|
||||
|
||||
/// Reset counter-dependent state for rekey cutover.
|
||||
///
|
||||
/// After cutover, the new session starts with counter 0 and reset
|
||||
/// timestamps. Without resetting, the old `highest_counter` and
|
||||
/// `GapTracker.expected_next` cause false reorder/loss detection.
|
||||
pub fn reset_for_rekey(&mut self) {
|
||||
self.highest_counter = 0;
|
||||
self.cumulative_reorder_count = 0;
|
||||
self.gap_tracker = GapTracker::new();
|
||||
self.interval_packets_recv = 0;
|
||||
self.interval_bytes_recv = 0;
|
||||
self.jitter = JitterEstimator::new();
|
||||
self.owd_trend.clear();
|
||||
self.owd_seq = 0;
|
||||
self.last_sender_timestamp = 0;
|
||||
self.last_recv_time = None;
|
||||
self.ecn_ce_count = 0;
|
||||
self.interval_has_data = false;
|
||||
// Keep cumulative_packets_recv, cumulative_bytes_recv (lifetime stats)
|
||||
// Keep last_report_time, report_interval (report scheduling)
|
||||
}
|
||||
|
||||
/// Record a received frame from this peer.
|
||||
///
|
||||
/// Called on the RX path after AEAD decryption, before message dispatch.
|
||||
|
||||
@@ -64,13 +64,16 @@ impl Node {
|
||||
);
|
||||
|
||||
let peer = self.peers.get_mut(&node_addr).unwrap();
|
||||
if let Some(_old_our_index) = peer.handle_peer_kbit_flip()
|
||||
&& let (Some(transport_id), Some(new_our_index)) =
|
||||
(peer.transport_id(), peer.our_index())
|
||||
{
|
||||
self.peers_by_index.insert(
|
||||
(transport_id, new_our_index.as_u32()),
|
||||
node_addr,
|
||||
if let Some(_old_our_index) = peer.handle_peer_kbit_flip() {
|
||||
// New index was pre-registered in peers_by_index during
|
||||
// msg1 handling (handshake.rs). Verify, don't duplicate.
|
||||
debug_assert!(
|
||||
peer.transport_id().is_some()
|
||||
&& peer.our_index().is_some()
|
||||
&& self.peers_by_index.contains_key(
|
||||
&(peer.transport_id().unwrap(), peer.our_index().unwrap().as_u32())
|
||||
),
|
||||
"peers_by_index should contain pre-registered new index after K-bit flip"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -537,6 +537,9 @@ impl Node {
|
||||
"Rekey msg2 processing failed"
|
||||
);
|
||||
if let Some(idx) = peer.abandon_rekey() {
|
||||
if let Some(tid) = peer.transport_id() {
|
||||
self.peers_by_index.remove(&(tid, idx.as_u32()));
|
||||
}
|
||||
let _ = self.index_allocator.free(idx);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -19,6 +19,10 @@ const DRAIN_WINDOW_SECS: u64 = 10;
|
||||
/// a peer's rekey msg1.
|
||||
const REKEY_DAMPENING_SECS: u64 = 30;
|
||||
|
||||
/// Delay FSP initiator cutover after handshake completion to allow
|
||||
/// XK msg3 to reach the responder before K-bit-flipped data arrives.
|
||||
const FSP_CUTOVER_DELAY_MS: u64 = 2000;
|
||||
|
||||
impl Node {
|
||||
/// Periodic rekey check. Called from the tick loop.
|
||||
///
|
||||
@@ -79,14 +83,16 @@ impl Node {
|
||||
if let Some(peer) = self.peers.get_mut(&node_addr)
|
||||
&& let Some(_old_our_index) = peer.cutover_to_new_session()
|
||||
{
|
||||
if let (Some(transport_id), Some(new_our_index)) =
|
||||
(peer.transport_id(), peer.our_index())
|
||||
{
|
||||
self.peers_by_index.insert(
|
||||
(transport_id, new_our_index.as_u32()),
|
||||
node_addr,
|
||||
);
|
||||
}
|
||||
// New index was pre-registered in peers_by_index during
|
||||
// msg2 handling (handshake.rs). Verify, don't duplicate.
|
||||
debug_assert!(
|
||||
peer.transport_id().is_some()
|
||||
&& peer.our_index().is_some()
|
||||
&& self.peers_by_index.contains_key(
|
||||
&(peer.transport_id().unwrap(), peer.our_index().unwrap().as_u32())
|
||||
),
|
||||
"peers_by_index should contain pre-registered new index after cutover"
|
||||
);
|
||||
info!(
|
||||
peer = %self.peer_display_name(&node_addr),
|
||||
"Rekey cutover complete (initiator), K-bit flipped"
|
||||
@@ -281,10 +287,14 @@ impl Node {
|
||||
continue;
|
||||
}
|
||||
|
||||
// 1. Initiator-side cutover: completed rekey, pending session ready
|
||||
// 1. Initiator-side cutover: completed rekey, pending session ready.
|
||||
// Defer cutover until msg3 has had time to reach the responder.
|
||||
// Without this delay, K-bit-flipped data can arrive before
|
||||
// msg3, causing decryption failures on the responder.
|
||||
if entry.pending_new_session().is_some()
|
||||
&& !entry.has_rekey_in_progress()
|
||||
&& entry.is_rekey_initiator()
|
||||
&& now_ms.saturating_sub(entry.rekey_completed_ms()) >= FSP_CUTOVER_DELAY_MS
|
||||
{
|
||||
sessions_to_cutover.push(*node_addr);
|
||||
continue;
|
||||
|
||||
@@ -595,6 +595,7 @@ impl Node {
|
||||
};
|
||||
|
||||
entry.set_pending_session(session);
|
||||
entry.set_rekey_completed_ms(Self::now_ms());
|
||||
self.sessions.insert(*src_addr, entry);
|
||||
|
||||
debug!(
|
||||
|
||||
@@ -107,6 +107,9 @@ pub(crate) struct SessionEntry {
|
||||
rekey_initiator: bool,
|
||||
/// Dampening: last time peer sent us a rekey msg1 (Unix ms).
|
||||
last_peer_rekey_ms: u64,
|
||||
/// When the FSP rekey handshake completed (initiator sent msg3, Unix ms).
|
||||
/// Used to defer cutover until msg3 has time to reach the responder.
|
||||
rekey_completed_ms: u64,
|
||||
}
|
||||
|
||||
impl SessionEntry {
|
||||
@@ -142,6 +145,7 @@ impl SessionEntry {
|
||||
pending_new_session: None,
|
||||
rekey_initiator: false,
|
||||
last_peer_rekey_ms: 0,
|
||||
rekey_completed_ms: 0,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -369,6 +373,16 @@ impl SessionEntry {
|
||||
}
|
||||
}
|
||||
|
||||
/// When the FSP rekey handshake completed (initiator sent msg3).
|
||||
pub(crate) fn rekey_completed_ms(&self) -> u64 {
|
||||
self.rekey_completed_ms
|
||||
}
|
||||
|
||||
/// Record when the FSP rekey handshake completed (initiator side).
|
||||
pub(crate) fn set_rekey_completed_ms(&mut self, ms: u64) {
|
||||
self.rekey_completed_ms = ms;
|
||||
}
|
||||
|
||||
/// Store a completed rekey session.
|
||||
pub(crate) fn set_pending_session(&mut self, session: NoiseSession) {
|
||||
self.pending_new_session = Some(session);
|
||||
@@ -408,6 +422,12 @@ impl SessionEntry {
|
||||
self.session_start_ms = now_ms;
|
||||
self.rekey_state = None;
|
||||
self.rekey_initiator = false;
|
||||
self.rekey_completed_ms = 0;
|
||||
|
||||
// Reset MMP counters to avoid metric discontinuity
|
||||
if let Some(mmp) = &mut self.mmp {
|
||||
mmp.reset_for_rekey();
|
||||
}
|
||||
true
|
||||
}
|
||||
|
||||
@@ -430,6 +450,11 @@ impl SessionEntry {
|
||||
self.session_start_ms = now_ms;
|
||||
self.rekey_state = None;
|
||||
self.rekey_initiator = false;
|
||||
|
||||
// Reset MMP counters to avoid metric discontinuity
|
||||
if let Some(mmp) = &mut self.mmp {
|
||||
mmp.reset_for_rekey();
|
||||
}
|
||||
true
|
||||
}
|
||||
|
||||
|
||||
@@ -880,6 +880,11 @@ impl ActivePeer {
|
||||
self.rekey_in_progress = false;
|
||||
self.reset_replay_suppressed();
|
||||
|
||||
// Reset MMP counters to avoid metric discontinuity
|
||||
if let Some(mmp) = &mut self.mmp {
|
||||
mmp.reset_for_rekey();
|
||||
}
|
||||
|
||||
self.previous_our_index
|
||||
}
|
||||
|
||||
@@ -909,6 +914,11 @@ impl ActivePeer {
|
||||
self.rekey_in_progress = false;
|
||||
self.reset_replay_suppressed();
|
||||
|
||||
// Reset MMP counters to avoid metric discontinuity
|
||||
if let Some(mmp) = &mut self.mmp {
|
||||
mmp.reset_for_rekey();
|
||||
}
|
||||
|
||||
self.previous_our_index
|
||||
}
|
||||
|
||||
|
||||
@@ -63,9 +63,6 @@ FIRST_REKEY_WAIT=40 # > REKEY_AFTER_SECS, allow margin
|
||||
REKEY_SETTLE=5 # settle time after rekey for cutover to complete
|
||||
SECOND_REKEY_WAIT=40 # wait for second cycle
|
||||
|
||||
# Phase 3 allows transient failures during cutover; Phase 5 must be clean
|
||||
MAX_PHASE3_FAILURES=4 # allow up to 4 pair failures during first rekey
|
||||
|
||||
TIMEOUT=5
|
||||
PASSED=0
|
||||
FAILED=0
|
||||
@@ -199,20 +196,11 @@ assert_min_count "Rekey cutover complete (initiator), K-bit flipped" 1 "FMP reke
|
||||
phase_result "FMP rekey events"
|
||||
echo ""
|
||||
|
||||
# Verify connectivity after first rekey (allow transient cutover failures)
|
||||
# Verify connectivity after first rekey (strict — no failures allowed)
|
||||
echo "Phase 3: Post-rekey connectivity (settling ${REKEY_SETTLE}s)"
|
||||
sleep "$REKEY_SETTLE"
|
||||
ping_all
|
||||
if [ "$FAILED" -le "$MAX_PHASE3_FAILURES" ]; then
|
||||
if [ "$FAILED" -gt 0 ]; then
|
||||
echo " (transient failures within threshold: $FAILED <= $MAX_PHASE3_FAILURES)"
|
||||
fi
|
||||
TOTAL_PASSED=$((TOTAL_PASSED + PASSED))
|
||||
# Don't count transient failures toward total
|
||||
echo " ✓ Post-first-rekey (all 20 pairs): $PASSED passed, $FAILED transient"
|
||||
else
|
||||
phase_result "Post-first-rekey (all 20 pairs)"
|
||||
fi
|
||||
phase_result "Post-first-rekey (all 20 pairs)"
|
||||
echo ""
|
||||
|
||||
# ── Phase 4: Wait for second rekey cycle ──────────────────────────────
|
||||
@@ -247,6 +235,8 @@ assert_zero_count "MMP link teardown" "Spurious link teardowns"
|
||||
assert_zero_count "Excessive decrypt failures" \
|
||||
"Excessive decrypt failure removals"
|
||||
assert_zero_count "Rekey msg2 processing failed" "Rekey msg2 failures"
|
||||
assert_zero_count "Session AEAD decryption failed" \
|
||||
"FSP decryption failures during rekey"
|
||||
|
||||
phase_result "Log analysis"
|
||||
echo ""
|
||||
|
||||
Reference in New Issue
Block a user