From d95fc708e0369ac8b665e749b2eafe20f6b8cb37 Mon Sep 17 00:00:00 2001 From: Johnathan Corgan Date: Sat, 22 Aug 2026 20:24:16 +0100 Subject: [PATCH] Cap how long a superseded FSP key epoch can be retained The drain deadline for the `previous` slot slides forward on every inbound frame that authenticates against it. That is deliberate: it stops the old epoch being erased out from under a peer that lost msg3 and is still sealing in it. But the only party that can push the deadline out is the authenticated peer holding that key, so a peer that keeps using the old epoch keeps the retired key resident indefinitely. Add an absolute ceiling measured from the cutover, so the sliding grace delays erasure by a bounded amount rather than preventing it. The ceiling has to clear the worst-case legitimate recovery of a peer that lost msg3, which is the msg3 resend ladder plus the responder's handshake timeout plus the rekey dampening window, about 90 seconds at stock settings. It defaults to 120 seconds and is raised to the budget the configured handshake timers actually imply, so tightening a timer cannot push the ceiling under the recovery it has to leave room for. This does not close the related gap where an FSP rekey we initiate and the peer never answers is never abandoned, which leaves the session's current epoch pinned and not rotating. The cap erases the old epoch on schedule regardless, which is a strict improvement, but a session read afterwards can show no drain alongside a stale current key for that reason rather than because of this change. --- CHANGELOG.md | 20 +++++++ src/node/handlers/mod.rs | 2 +- src/node/handlers/rekey.rs | 45 +++++++++++++- src/node/session.rs | 117 +++++++++++++++++++++++++++++++++++-- 4 files changed, 176 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb1d360a..f09f5eb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -591,6 +591,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 handshake window and below `link_dead_timeout_secs`, and nothing changes on the wire. +- Retention of a superseded FSP key epoch is now capped at an absolute + ceiling measured from the cutover, defaulting to 120 seconds against the + 10-second drain window. The drain deadline slides forward on every inbound + frame that authenticates against the `previous` slot, which is what keeps a + peer that lost msg3 from having the old epoch erased out from under it, but + it also meant the authenticated peer holding that key could keep the retired + key resident for as long as it kept sealing frames in the old epoch. The + sliding grace is unchanged; it now delays erasure by a bounded amount rather + than preventing it. The ceiling is set to clear the worst-case legitimate + recovery of a peer that lost msg3 (the msg3 resend ladder, then + `handshake_timeout_secs` before the responder abandons, then the rekey + dampening window before it may re-initiate, about 90 seconds at stock + settings), and it is raised automatically if the configured handshake timers + imply a longer budget, so shortening a timer cannot push the ceiling under + the recovery it has to leave room for. Nothing changes on the wire; each side + runs its own drain. A peer that has still not recovered when the ceiling + fires is left with undecryptable frames until its own rekey retry + re-converges the epochs, since nothing tears an established session down on + repeated decrypt failure. + - A session setup message naming an already-established peer no longer replaces that peer's session. The handler did this whenever `node.rekey.enabled` was false: it ran a fresh responder handshake and overwrote the entry, discarding diff --git a/src/node/handlers/mod.rs b/src/node/handlers/mod.rs index 5270cad3..d5feb846 100644 --- a/src/node/handlers/mod.rs +++ b/src/node/handlers/mod.rs @@ -8,7 +8,7 @@ mod encrypted; mod forwarding; pub(in crate::node) mod handshake; mod mmp; -mod rekey; +pub(in crate::node) mod rekey; mod rx_loop; pub(in crate::node) mod session; mod timeout; diff --git a/src/node/handlers/rekey.rs b/src/node/handlers/rekey.rs index bac07b8a..4c957e1c 100644 --- a/src/node/handlers/rekey.rs +++ b/src/node/handlers/rekey.rs @@ -19,6 +19,48 @@ const DRAIN_WINDOW_SECS: u64 = 10; /// a peer's rekey msg1. const REKEY_DAMPENING_SECS: u64 = 30; +/// Floor on the absolute ceiling for `previous`-slot retention after a +/// cutover, in seconds. +/// +/// The drain deadline is peer-progress-aware: it slides forward on every +/// inbound frame that authenticates against the old epoch, so a peer that +/// keeps sealing in that epoch holds the retired key for as long as it +/// likes. This bounds that. It has to stay longer than the worst-case +/// recovery of a legitimate peer that lost msg3, which at stock defaults +/// is the msg3 resend ladder (about 31 s) plus `handshake_timeout_secs` +/// (30 s) before the responder abandons plus `REKEY_DAMPENING_SECS` +/// (30 s) before it may re-initiate, so about 90 s. 120 s clears that +/// with margin and still bounds retention to roughly one +/// `node.rekey.after_secs` period. `drain_max_retention_ms` takes the +/// larger of this floor and the budget the running configuration +/// actually implies, so a shortened handshake timer cannot push the +/// ceiling under the recovery it has to clear. +/// +/// Lowering it below that budget cuts off legitimate slow peers: their +/// frames go silently undecryptable until their own rekey retry +/// re-converges the epochs, because nothing tears an established session +/// down on repeated decrypt failure. Raising it lengthens the window in +/// which a retired key stays resident. +const DRAIN_MAX_RETENTION_SECS: u64 = DRAIN_WINDOW_SECS * 12; + +/// Effective ceiling on total `previous`-slot retention, in milliseconds. +/// +/// The larger of `DRAIN_MAX_RETENTION_SECS` and the msg3 recovery budget +/// the configured handshake timers imply, so the ceiling always clears +/// the recovery it is supposed to leave room for. +pub(in crate::node) fn drain_max_retention_ms(rate_limit: &crate::config::RateLimitConfig) -> u64 { + let mut ladder_ms: u64 = 0; + let mut interval = rate_limit.handshake_resend_interval_ms as f64; + for _ in 0..rate_limit.handshake_max_resends { + ladder_ms = ladder_ms.saturating_add(interval as u64); + interval *= rate_limit.handshake_resend_backoff; + } + let recovery_budget_ms = ladder_ms + .saturating_add(rate_limit.handshake_timeout_secs.saturating_mul(1000)) + .saturating_add(REKEY_DAMPENING_SECS * 1000); + (DRAIN_MAX_RETENTION_SECS * 1000).max(recovery_budget_ms) +} + /// Liveness bound on how long the FSP rekey initiator holds the /// `current` + `pending` state before cutting over to the new epoch. /// @@ -443,6 +485,7 @@ impl Node { let rekey_after_messages = self.config().node.rekey.after_messages; let now_ms = Self::now_ms(); let drain_ms = DRAIN_WINDOW_SECS * 1000; + let drain_max_ms = drain_max_retention_ms(&self.config().node.rate_limit); let dampening_ms = REKEY_DAMPENING_SECS * 1000; let mut sessions_to_cutover: Vec = Vec::new(); @@ -492,7 +535,7 @@ impl Node { } // 2. Drain window expiry - if entry.is_draining() && entry.drain_expired(now_ms, drain_ms) { + if entry.is_draining() && entry.drain_expired(now_ms, drain_ms, drain_max_ms) { sessions_to_drain.push(*node_addr); } diff --git a/src/node/session.rs b/src/node/session.rs index 4af0d463..4c036e59 100644 --- a/src/node/session.rs +++ b/src/node/session.rs @@ -726,10 +726,24 @@ impl SessionEntry { /// permanent silent decrypt failure. A peer that never catches up /// is instead handled by the FSP session liveness path (fresh /// handshake / teardown of a genuinely dead link). - pub(crate) fn drain_expired(&self, now_ms: u64, drain_ms: u64) -> bool { + /// + /// `max_drain_ms` is an absolute ceiling measured from the cutover + /// alone, so the sliding deadline delays erasure by a bounded amount + /// rather than preventing it: the only party that can push the + /// deadline out is the authenticated peer holding the old key, and + /// without a ceiling it holds that key for as long as it keeps using + /// it. What the ceiling costs is that a peer which has still not + /// recovered by then is cut off deliberately, and its frames are + /// undecryptable until its own rekey retry re-converges the epochs. + /// It must therefore stay above the worst-case legitimate recovery; + /// see `DRAIN_MAX_RETENTION_SECS`. + pub(crate) fn drain_expired(&self, now_ms: u64, drain_ms: u64, max_drain_ms: u64) -> bool { if self.drain_started_ms == 0 { return false; } + if now_ms.saturating_sub(self.drain_started_ms) >= max_drain_ms { + return true; + } let deadline_anchor = self.drain_started_ms.max(self.previous_last_used_ms); now_ms.saturating_sub(deadline_anchor) >= drain_ms } @@ -1226,6 +1240,9 @@ mod overlapping_epoch_tests { #[test] fn drain_expiry_is_peer_progress_aware() { const DRAIN_MS: u64 = 10_000; + // Well clear of the shipped ceiling, so this case still exercises + // the sliding deadline and nothing else. + const MAX_MS: u64 = 120_000; let cutover_ms = 1_000; // Build the post-cutover state via the production cutover path: @@ -1254,7 +1271,7 @@ mod overlapping_epoch_tests { // Even though `now - drain_started_ms` exceeds DRAIN_MS, the // window is NOT expired: the peer just used `previous`. assert!( - !entry.drain_expired(t, DRAIN_MS), + !entry.drain_expired(t, DRAIN_MS, MAX_MS), "previous slot must not be retired while peer keeps using it (t={t})" ); assert!( @@ -1267,11 +1284,11 @@ mod overlapping_epoch_tests { // last `previous`-slot use was at t=25_000; the window now // elapses DRAIN_MS after that, NOT DRAIN_MS after the cutover. assert!( - !entry.drain_expired(34_999, DRAIN_MS), + !entry.drain_expired(34_999, DRAIN_MS, MAX_MS), "window must not expire before DRAIN_MS past the last previous use" ); assert!( - entry.drain_expired(35_000, DRAIN_MS), + entry.drain_expired(35_000, DRAIN_MS, MAX_MS), "window must expire DRAIN_MS after the last previous-slot decrypt" ); @@ -1289,6 +1306,7 @@ mod overlapping_epoch_tests { #[test] fn drain_expiry_unaffected_when_peer_off_old_epoch() { const DRAIN_MS: u64 = 10_000; + const MAX_MS: u64 = 120_000; let cutover_ms = 1_000; let (_old_send, old_recv) = xk_pair(1, 2); @@ -1300,12 +1318,99 @@ mod overlapping_epoch_tests { // No old-epoch frames ever arrive: `previous_last_used_ms` stays // 0, the deadline anchor is the cutover time. assert!( - !entry.drain_expired(cutover_ms + DRAIN_MS - 1, DRAIN_MS), + !entry.drain_expired(cutover_ms + DRAIN_MS - 1, DRAIN_MS, MAX_MS), "window must not expire early" ); assert!( - entry.drain_expired(cutover_ms + DRAIN_MS, DRAIN_MS), + entry.drain_expired(cutover_ms + DRAIN_MS, DRAIN_MS, MAX_MS), "window must expire on the plain wall-clock timer when peer is off the old epoch" ); } + // 12. A peer that keeps exercising the old epoch delays erasure by a + // bounded amount rather than preventing it. The refreshes must + // continue past the ceiling: a case that stops refreshing at the + // boundary passes without the ceiling and proves nothing. + #[test] + fn drain_retention_is_capped_against_a_peer_pinning_the_old_epoch() { + const DRAIN_MS: u64 = 10_000; + const MAX_MS: u64 = 120_000; + + let (_old_send, old_recv) = xk_pair(1, 2); + let (_new_send, new_recv) = xk_pair(3, 4); + let mut entry = entry_with_current(old_recv); + entry.set_pending_session(new_recv); + assert!(entry.cutover_to_new_session(1)); + + // One old-epoch frame every half window, which is what a peer + // pinning the drain deadline actually does. + let mut t = 1u64; + while t < MAX_MS { + entry.refresh_previous_use(t); + assert!( + !entry.drain_expired(t, DRAIN_MS, MAX_MS), + "ceiling fired before the peer's grace ran out (t={t})" + ); + t += DRAIN_MS / 2; + } + + // Still refreshing, so the sliding deadline is nowhere near due. + entry.refresh_previous_use(MAX_MS + 1); + assert!( + entry.drain_expired(MAX_MS + 1, DRAIN_MS, MAX_MS), + "a peer pinning the old epoch retained the retired key past the ceiling" + ); + } + + // 13. The ceiling must not shorten the grace the sliding deadline + // exists to give a peer that lost msg3 and is still catching up. + #[test] + fn drain_retention_cap_does_not_shorten_the_ordinary_grace() { + const DRAIN_MS: u64 = 10_000; + const MAX_MS: u64 = 120_000; + const CUTOVER_MS: u64 = 1_000; + + let (_old_send, old_recv) = xk_pair(1, 2); + let (_new_send, new_recv) = xk_pair(3, 4); + let mut entry = entry_with_current(old_recv); + entry.set_pending_session(new_recv); + assert!(entry.cutover_to_new_session(CUTOVER_MS)); + assert!(entry.is_draining()); + + // A peer that lost msg3 and is still catching up sends one + // old-epoch frame part-way through the window. + let last_use = CUTOVER_MS + DRAIN_MS / 2; + entry.refresh_previous_use(last_use); + assert!(!entry.drain_expired(CUTOVER_MS + DRAIN_MS, DRAIN_MS, MAX_MS)); + assert!(!entry.drain_expired(last_use + DRAIN_MS - 1, DRAIN_MS, MAX_MS)); + assert!(entry.drain_expired(last_use + DRAIN_MS, DRAIN_MS, MAX_MS)); + } + + // 14. The shipped ceiling has to clear the worst-case legitimate + // recovery of a peer that lost msg3: the msg3 resend ladder, the + // responder's handshake timeout, and the rekey dampening window + // before it may re-initiate. A later tightening of the ceiling + // reds this rather than silently amputating that recovery. + #[test] + fn drain_retention_cap_clears_the_msg3_recovery_budget() { + const DRAIN_MS: u64 = 10_000; + // 31 s resend ladder + 30 s handshake timeout + 30 s dampening. + const RECOVERY_MS: u64 = 91_000; + const CUTOVER_MS: u64 = 1_000; + let max_ms = crate::node::handlers::rekey::drain_max_retention_ms( + &crate::config::RateLimitConfig::default(), + ); + + let (_old_send, old_recv) = xk_pair(1, 2); + let (_new_send, new_recv) = xk_pair(3, 4); + let mut entry = entry_with_current(old_recv); + entry.set_pending_session(new_recv); + assert!(entry.cutover_to_new_session(CUTOVER_MS)); + assert!(entry.is_draining()); + + entry.refresh_previous_use(CUTOVER_MS + RECOVERY_MS); + assert!( + !entry.drain_expired(CUTOVER_MS + RECOVERY_MS, DRAIN_MS, max_ms), + "ceiling fires inside the msg3 recovery budget" + ); + } }