diff --git a/src/node/dataplane/peer_actions.rs b/src/node/dataplane/peer_actions.rs index 05d87bd..979f9c9 100644 --- a/src/node/dataplane/peer_actions.rs +++ b/src/node/dataplane/peer_actions.rs @@ -611,6 +611,24 @@ impl Node { return; } }; + // A rekey of ours in flight is now stale: the session it + // was negotiated against is about to be replaced, and + // `replace_session` rewrites the session and both indices + // while touching no rekey state. Abandoning here is what + // lets the classifier take this arm unconditionally — + // declining the swap instead would desynchronize us from + // the peer's outbound half, which cannot see our rekey. + // Same clearing the rekey-responder arm does on its + // `abandon_first` path. + if let Some(peer_ref) = self.peers.get_mut(&peer) + && let Some(idx) = peer_ref.abandon_rekey() + { + if let Some(tid) = peer_ref.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); + } if let Some(peer_ref) = self.peers.get_mut(&peer) { let old_our_index = peer_ref.replace_session(inbound_session, our_index, their_index); diff --git a/src/node/handlers/handshake.rs b/src/node/handlers/handshake.rs index f12f4e0..4239d13 100644 --- a/src/node/handlers/handshake.rs +++ b/src/node/handlers/handshake.rs @@ -1300,8 +1300,16 @@ impl Node { Ok(claim) => claim, Err(e) => { warn!(link_id = %link_id, error = %e, "Malformed rekey marker in msg3"); + // The msg1-allocated index rides the machine, so capture + // it before disposal or it is orphaned. This arm is + // reachable before the ACL gate, so leaving it to leak + // would let any peer that can complete a msg3 grow the + // allocator set one entry per malformed marker. self.remove_link(&link_id); self.remove_peer_machine(link_id); + if let Some(idx) = our_index { + let _ = self.index_allocator.free(idx); + } self.stats_mut() .record_reject(RejectReason::Handshake(HandshakeReject::BadState)); return; diff --git a/src/node/mod.rs b/src/node/mod.rs index 04339a4..0f43293 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -44,9 +44,11 @@ use self::reloadable::Reloadable; /// caught by the initial-handshake cross-connection tie-breaker in /// `handle_msg3` and, on the "our outbound wins" side, the peer's rekey /// session was discarded with no `pending` slot — yet the peer cut over to it -/// regardless, starving the discarding node until the 30s dead-timer. Fixed by -/// bounding that cross-connection branch below the rekey age floor so a -/// rekey-aged msg3 falls through to the rekey-responder path. See CHANGELOG. +/// regardless, starving the discarding node until the 30s dead-timer. First +/// bounded by an age floor on that cross-connection branch; the floor is gone +/// now, and a rekey msg3 reaches the rekey-responder path because its sender +/// declares it there (`TLV_REKEY_OF`), which no amount of jitter on the +/// session-age clock can perturb. See CHANGELOG. pub(crate) const REKEY_JITTER_SECS: i64 = 15; use crate::cache::CoordCache; use crate::node::session::SessionEntry; diff --git a/src/node/tests/handshake.rs b/src/node/tests/handshake.rs index 306b336..0bdfe0d 100644 --- a/src/node/tests/handshake.rs +++ b/src/node/tests/handshake.rs @@ -707,6 +707,32 @@ async fn test_cross_connection_both_initiate() { assert!(peer_b_on_a.can_send(), "Peer B on A should be sendable"); assert!(peer_a_on_b.can_send(), "Peer A on B should be sendable"); + // The property the tie-break exists to produce: both ends kept the SAME + // session, not merely a session each. The index pair is what makes that + // checkable — A sends to B on the index B receives on, and vice versa. + // + // Every per-end predicate above holds when the two ends resolve onto + // different sessions, because each end does have a healthy session; the + // link then decrypts one direction and silently drops the other. So the + // pairing has to be asserted across the two nodes or it is not asserted at + // all. Kept as a standing guard on the tie-break rather than as a + // reproduction of any particular defect: it passes on the pre-marker tree + // as well, where this ordering was already resolved correctly. + assert_eq!( + peer_b_on_a.their_index(), + peer_a_on_b.our_index(), + "A sends to B on an index B does not receive on: the ends diverged" + ); + assert_eq!( + peer_a_on_b.their_index(), + peer_b_on_a.our_index(), + "B sends to A on an index A does not receive on: the ends diverged" + ); + assert!( + peer_b_on_a.our_index().is_some() && peer_b_on_a.their_index().is_some(), + "both indices must be set, or the equality above passes on None == None" + ); + // Clean up transports for (_, t) in node_a.transports.iter_mut() { t.stop().await.ok(); @@ -1693,15 +1719,22 @@ async fn test_msg3_dual_rekey_won_frees_index() { #[tokio::test] async fn test_msg3_resend_msg2_frees_index() { - // Rekey disabled so an aged-session inbound msg3 classifies as a duplicate - // handshake (ResendMsg2), not a rekey; the tiny interval keeps the - // cross-connection age bound (the rekey floor) at its 5s minimum so the - // aged session skips the cross-connection arm too. + // A declared rekey into a responder that has rekey DISABLED. The marker + // matches, so this is unambiguously a rekey and not a crossing dial, and the + // responder's `rekey_enabled` gate sends it to the duplicate arm. + // + // A bare second handshake will not reach this arm: it declares no rekey, and + // an undeclared msg3 on a different link is a cross-connection, which + // `rekey_enabled` does not gate. That is what this test used to do, and every + // assertion below holds on the cross-connection arm too, so it passed while + // exercising something else entirely. + let mut init_config = Config::new(); + init_config.node.rekey.enabled = true; + init_config.node.rekey.after_secs = 1; let mut config = Config::new(); config.node.rekey.enabled = false; - config.node.rekey.after_secs = 1; - let mut initiator = make_hs_node(Config::new()).await; + let mut initiator = make_hs_node(init_config).await; let mut responder = make_hs_node(config).await; // First handshake establishes the active peer. @@ -1711,23 +1744,28 @@ async fn test_msg3_resend_msg2_frees_index() { let baseline = responder.node.index_allocator.count(); assert_eq!(baseline, 1, "responder holds exactly the peer's index"); - // Age the session past the cross-connection bound so the duplicate inbound - // msg3 resolves to ResendMsg2 rather than CrossConnect. let peer_addr = *PeerIdentity::from_pubkey_full(initiator.node.identity().pubkey_full()).node_addr(); - responder + let responder_addr = + *PeerIdentity::from_pubkey_full(responder.node.identity().pubkey_full()).node_addr(); + // Age the initiator's session past the whole jitter band so its own rekey + // trigger fires and the msg3 that arrives carries a matching marker. + initiator .node - .get_peer_mut(&peer_addr) + .get_peer_mut(&responder_addr) .unwrap() .test_backdate_session_established(std::time::Duration::from_secs(120)); - // Second (duplicate) handshake: fresh index allocated at msg1, then freed - // on the ResendMsg2 arm. - let msg3b = drive_to_msg3(&mut initiator, &mut responder, 2000).await; + let before = responder.node.get_peer(&peer_addr).unwrap(); + let session_before = (before.our_index(), before.their_index()); + + // The rekey's msg1 allocates a fresh index; its msg3 then frees it on the + // duplicate arm, because this responder does not do rekeys. + let msg3b = drive_rekey_to_msg3(&mut initiator, &mut responder).await; assert_eq!( responder.node.index_allocator.count(), baseline + 1, - "second msg1 allocated a fresh index" + "the rekey msg1 allocated a fresh index" ); responder.node.handle_msg3(msg3b).await; @@ -1737,6 +1775,13 @@ async fn test_msg3_resend_msg2_frees_index() { "ResendMsg2 must free the msg1-allocated index" ); assert_eq!(responder.node.peer_count(), 1, "active peer untouched"); + let after = responder.node.get_peer(&peer_addr).unwrap(); + assert_eq!( + (after.our_index(), after.their_index()), + session_before, + "the duplicate arm must leave the session alone; changed indices mean \ + the cross-connection arm ran instead" + ); // The duplicate leg's msg1-born machine goes with the leg; only the // established peer's machine remains. let peer_link = responder.node.get_peer(&peer_addr).unwrap().link_id(); @@ -1932,6 +1977,194 @@ async fn test_msg3_rekey_respond_disposes_leg_machine() { stop_hs(&mut responder).await; } +/// A rekey triggered by message count on a *young* session is still a rekey, +/// and both ends must come out of it holding the same session. +/// +/// This is the defect the marker exists to remove. The rekey trigger is +/// `elapsed >= after_secs || counter >= after_messages`, so the count disjunct +/// fires on a session of any age — while the discriminator it replaced asked +/// only how old the session was, and read anything young as a fresh +/// cross-connection. The two ends then resolved onto different sessions, and a +/// link whose ends disagree decrypts one direction and drops the other. +/// +/// Driven entirely off the count disjunct: `after_secs` is set far out of reach, +/// so nothing here depends on the rekey jitter draw. +#[tokio::test] +async fn test_count_triggered_rekey_on_young_session_converges() { + let make_config = || { + let mut c = Config::new(); + c.node.rekey.enabled = true; + // Unreachable by age, so only the message counter can fire the trigger. + c.node.rekey.after_secs = 3600; + c.node.rekey.after_messages = 4; + c + }; + + let mut initiator = make_hs_node(make_config()).await; + let mut responder = make_hs_node(make_config()).await; + + let msg3 = drive_to_msg3(&mut initiator, &mut responder, 1000).await; + responder.node.handle_msg3(msg3).await; + + let initiator_addr = + *PeerIdentity::from_pubkey_full(initiator.node.identity().pubkey_full()).node_addr(); + let responder_addr = + *PeerIdentity::from_pubkey_full(responder.node.identity().pubkey_full()).node_addr(); + + // Carry the session past the message threshold, leaving its age at zero. + // Encrypting on the session is what the trigger actually counts, so this + // moves the real counter rather than a stand-in for it. + for _ in 0..5 { + initiator + .node + .get_peer_mut(&responder_addr) + .unwrap() + .noise_session_mut() + .unwrap() + .encrypt(b"traffic") + .expect("encrypt under the established session"); + } + + let rekey_msg3 = drive_rekey_to_msg3(&mut initiator, &mut responder).await; + responder.node.handle_msg3(rekey_msg3).await; + + assert!( + responder + .node + .get_peer(&initiator_addr) + .unwrap() + .pending_new_session() + .is_some(), + "the responder read a real rekey as something else: no pending session" + ); + + // Both ends cut over to the session they just negotiated. + initiator.node.check_rekey().await; + responder.node.check_rekey().await; + + let on_initiator = initiator.node.get_peer(&responder_addr).unwrap(); + let on_responder = responder.node.get_peer(&initiator_addr).unwrap(); + assert_eq!( + on_initiator.their_index(), + on_responder.our_index(), + "after the rekey the ends hold different sessions: traffic drops one way" + ); + assert_eq!( + on_responder.their_index(), + on_initiator.our_index(), + "after the rekey the ends hold different sessions: traffic drops one way" + ); + assert!( + on_initiator.our_index().is_some() && on_initiator.their_index().is_some(), + "both indices must be set, or the equality above passes on None == None" + ); + + stop_hs(&mut initiator).await; + stop_hs(&mut responder).await; +} + +/// A fresh dial crossing our own in-flight rekey leaves BOTH ends on the same +/// session, and leaks nothing on the way. +/// +/// The two events cross: we send a rekey msg1 and, before it completes, the peer +/// dials us anew on a different link. Its msg3 declares no rekey, correctly, so +/// it is a genuine cross-connection and resolves by the address tie-break. +/// +/// The earlier version of this test asserted only the responder's state and so +/// passed on an outcome where the two ends had diverged onto four distinct +/// indices — the initiator's half of the tie-break cannot see our rekey and +/// swaps regardless, so a responder that declines desynchronizes the pair. Any +/// assertion here has to span both nodes; a one-sided check is the failure mode +/// this whole change exists to remove. +#[tokio::test] +async fn test_fresh_dial_crossing_our_rekey_converges() { + let make_config = || { + let mut c = Config::new(); + c.node.rekey.enabled = true; + c.node.rekey.after_secs = 1; + c + }; + + let mut initiator = make_hs_node(make_config()).await; + // Pin the responder as the LARGER node addr: that is the ordering in which + // its inbound wins the tie-break and it performs the swap, which is the side + // that has to clean up the rekey it is abandoning. With a smaller responder + // it keeps its outbound and the interesting path is never taken. + let mut responder = loop { + let cand = make_hs_node(make_config()).await; + if cand.node.node_addr() > initiator.node.node_addr() { + break cand; + } + }; + + let msg3 = drive_to_msg3(&mut initiator, &mut responder, 1000).await; + responder.node.handle_msg3(msg3).await; + + let peer_addr = + *PeerIdentity::from_pubkey_full(initiator.node.identity().pubkey_full()).node_addr(); + let responder_addr = + *PeerIdentity::from_pubkey_full(responder.node.identity().pubkey_full()).node_addr(); + + // Start the responder's OWN rekey and leave it in flight. Driven from the + // real trigger rather than poking the flag, so the peer carries whatever + // state a live rekey actually leaves behind. Backdate well past the jitter + // band ([-15, +15] around `after_secs`) or firing depends on the draw. + responder + .node + .get_peer_mut(&peer_addr) + .unwrap() + .test_backdate_session_established(std::time::Duration::from_secs(120)); + responder.node.check_rekey().await; + + assert!( + responder + .node + .get_peer(&peer_addr) + .unwrap() + .rekey_in_progress(), + "the responder's own rekey must be in flight, or this tests nothing" + ); + + // The peer now dials fresh on a NEW link, unaware of our rekey. A bare + // handshake declares no rekey, because it is not one. + let fresh_msg3 = drive_to_msg3(&mut initiator, &mut responder, 5000).await; + responder.node.handle_msg3(fresh_msg3).await; + + let on_initiator = initiator.node.get_peer(&responder_addr).unwrap(); + let on_responder = responder.node.get_peer(&peer_addr).unwrap(); + assert_eq!( + on_initiator.their_index(), + on_responder.our_index(), + "ends diverged: the initiator sends on an index the responder does not receive on" + ); + assert_eq!( + on_responder.their_index(), + on_initiator.our_index(), + "ends diverged: the responder sends on an index the initiator does not receive on" + ); + assert!( + on_responder.our_index().is_some() && on_responder.their_index().is_some(), + "both indices must be set, or the equalities above pass on None == None" + ); + + // The rekey the swap displaced is gone rather than left dangling at a + // session nobody holds, and its index went back to the allocator with it. + assert!( + !on_responder.rekey_in_progress(), + "the displaced rekey must be abandoned by the swap, not left in flight" + ); + assert!( + on_responder.pending_new_session().is_none(), + "the displaced rekey must leave no pending session behind" + ); + assert!(on_responder.has_session()); + responder.node.debug_assert_peer_maps_coherent(); + initiator.node.debug_assert_peer_maps_coherent(); + + stop_hs(&mut initiator).await; + stop_hs(&mut responder).await; +} + // =========================================================================== // Anonymous-discovery outbound lifecycle: the leg's persistent machine is // born identity-less at leg birth inside `start_handshake`, learns its diff --git a/src/peer/active.rs b/src/peer/active.rs index 7371b99..7f53d16 100644 --- a/src/peer/active.rs +++ b/src/peer/active.rs @@ -1024,10 +1024,14 @@ impl ActivePeer { /// Test-only seam: backdate the session-established instant so a test can /// make `session_established_at().elapsed()` read as `age`-old (needed to - /// drive the inbound establish paths that gate on session age past the - /// rekey age floor). This only shifts the private timestamp field; it - /// changes no decision logic, no threshold, and is compiled out of release - /// builds. + /// fire the age half of the rekey trigger without waiting for it). This only + /// shifts the private timestamp field; it changes no decision logic, no + /// threshold, and is compiled out of release builds. + /// + /// Backdate past the whole jitter band, not the nominal `after_secs`: the + /// effective trigger is `after_secs + jitter` with jitter drawn from + /// `[-REKEY_JITTER_SECS, +REKEY_JITTER_SECS]`, so a smaller margin makes the + /// test depend on the draw. #[cfg(test)] pub(crate) fn test_backdate_session_established(&mut self, age: std::time::Duration) { self.session_established_at = self @@ -1326,13 +1330,29 @@ impl ActivePeer { // link — and every attempt to infer that from local state has a failure // band. The pre-rekey session is still installed at this point; the // pending one is not set until the caller drives it. + // + // An absent `their_index` would silently omit the marker and put this + // rekey back on the cross-connection path — the defect verbatim. It is + // unreachable today (the rekey cadence filters on `has_session`, and + // every production peer is built by `with_session`, which sets the + // index), so it is asserted rather than handled: a silent `if let` is + // the wrong shape for a value whose absence reintroduces the bug. let mut msg3 = hs.write_message_3()?; - if let Some(their_index) = self.their_index() { - let marker = NegotiationPayload::fmp(1, 1, our_profile) - .with_rekey_of(their_index) - .encode(); - let encrypted = hs.encrypt_payload(&marker)?; - msg3.extend_from_slice(&encrypted); + match self.their_index() { + Some(their_index) => { + let marker = NegotiationPayload::fmp(1, 1, our_profile) + .with_rekey_of(their_index) + .encode(); + let encrypted = hs.encrypt_payload(&marker)?; + msg3.extend_from_slice(&encrypted); + } + None => { + debug_assert!(false, "rekeying peer has no their_index to declare"); + tracing::warn!( + "Rekey msg3 built with no session index to declare; \ + the peer will read it as a fresh dial" + ); + } } let session = hs.into_session()?; diff --git a/src/proto/fmp/core.rs b/src/proto/fmp/core.rs index 031799c..8ec9319 100644 --- a/src/proto/fmp/core.rs +++ b/src/proto/fmp/core.rs @@ -221,10 +221,10 @@ pub(crate) struct WireOutcome { /// reads about the peer whose `msg3` just completed, taken by the shell so the /// core decides without touching live `Node` state or reading a clock. /// -/// Every clock read (`existing_session_age_secs`) and every config-derived -/// threshold (`rekey_age_floor_secs`) is resolved shell-side into a plain -/// `u64`, the same monotonic-ages asymmetry the rekey snapshot uses, so the -/// timing stays behavior-identical under a clock step. +/// The decision reads no clock at all. It formerly carried a session age and a +/// config-derived age floor, both resolved shell-side; the sender's declaration +/// in [`rekey_claim`](EstablishSnapshot::rekey_claim) replaced them, so there is +/// no timing left for a clock step to perturb. pub(crate) struct EstablishSnapshot { /// The peer identity is already an active peer in the registry. `false` here /// is the net-new path (or the post-restart re-promote) — a plain promote. @@ -437,17 +437,19 @@ pub(crate) enum InboundDecision { /// same promote sequence as [`Promote`](InboundDecision::Promote). `peer` is /// the teardown / reconnect target. RestartThenPromote { peer: NodeAddr }, - /// Same-epoch cross-connection resolved inline on `msg3`: a still-fresh - /// session (age `< rekey_age_floor_secs`) received a concurrent `msg3` on a - /// different link. `our_inbound_wins` (the larger-NodeAddr side) selects + /// Same-epoch cross-connection resolved inline on `msg3`: a healthy session + /// with no rekey of ours in flight received a concurrent `msg3` on a + /// different link that declares no rekey — so it is a fresh dial crossing + /// ours. `our_inbound_wins` (the larger-NodeAddr side) selects /// swap-to-inbound vs keep-outbound; the shell frees the loser index and /// tears down the temporary link either way. `peer` is the tie-break target. CrossConnect { peer: NodeAddr, our_inbound_wins: bool, }, - /// Same-epoch aged rekey `msg3` on a healthy session: respond as the rekey - /// responder. The shell extracts the fresh Noise session from the live + /// Same-epoch `msg3` on a healthy session, declared by its sender to replace + /// that very session: respond as the rekey responder. The shell extracts the + /// fresh Noise session from the live /// connection, allocates a new index, and stores it as the peer's pending /// (post-rekey) session awaiting K-bit cutover. `abandon_first` is set only /// on the dual-initiation *loser* path (larger NodeAddr), where we first @@ -703,21 +705,32 @@ impl Fmp { /// consumes nothing. The returned [`InboundDecision`] tells the shell which /// effect sequence to drive. /// - /// Mirrors the pre-refactor inline `handle_msg3` classification order - /// exactly: + /// Classification order. What separates a rekey from a fresh dial is the + /// sender's own declaration ([`RekeyClaim`]), never session age: a + /// message-count-triggered rekey fires on a session of any age, so an + /// age-based split misread real rekeys as cross-connections and left the two + /// ends of a link on different sessions. /// /// 1. No existing peer → net-new [`Promote`](InboundDecision::Promote). /// 2. Existing peer, different epoch → [`RestartThenPromote`]. - /// 3. Same epoch, different link, session younger than the rekey floor → - /// inline [`CrossConnect`] (the XX widening: IK resolves this on `msg2`). + /// 3. Same epoch, marker naming a session we do not hold + /// ([`Mismatch`](RekeyClaim::Mismatch)) → [`ResendMsg2`], not a reject: + /// the sender has already committed to its pending session and the reject + /// path sends nothing back. + /// 4. Same epoch, no marker, different link, healthy session → inline + /// [`CrossConnect`] (the XX widening: IK resolves this on `msg2`). /// `our_inbound_wins` is the larger-NodeAddr side, matching - /// `cross_connection_winner(our, peer, /*outbound=*/ false)`. - /// 4. Same epoch, rekey enabled + healthy session + age at/above the floor → + /// `cross_connection_winner(our, peer, /*outbound=*/ false)`. Taken + /// regardless of any rekey of ours in flight — the peer's outbound half of + /// this tie-break cannot see that state, so declining here would diverge + /// the pair; the executor abandons the displaced rekey instead. + /// 5. Same epoch, marker naming the session we hold + /// ([`Matches`](RekeyClaim::Matches)), rekey enabled, healthy session → /// [`RekeyRespond`]. The widened dual-init tie-break fires when the peer /// is in *either* the `rekey_in_progress` or the `pending_new_session` /// state: the smaller NodeAddr wins ([`Reject`] the peer's `msg3`), the /// larger loses (`abandon_first`, then respond). - /// 5. Otherwise same epoch → duplicate [`ResendMsg2`]. + /// 6. Otherwise same epoch → duplicate [`ResendMsg2`]. /// /// [`CrossConnect`]: InboundDecision::CrossConnect /// [`RestartThenPromote`]: InboundDecision::RestartThenPromote @@ -766,18 +779,24 @@ impl Fmp { // The sender declares no rekey, so a msg3 on a different link is // a fresh dial crossing ours. // - // A rekey of ours in flight suppresses the swap. `replace_session` - // rewrites the session and both indices and touches nothing else, - // so swapping while a pending rekey exists leaves that rekey and - // its K-bit state pointing at a session the counterpart no longer - // holds. The age floor excluded this case incidentally; the marker - // does not, so it is stated. + // A rekey of ours in flight does NOT suppress the swap, and must + // not: this is one half of a two-sided tie-break whose other half + // is `establish_outbound`, which sees only `has_existing_peer` and + // the address ordering. Declining here while the peer's outbound + // half swaps anyway leaves the two ends on different sessions with + // neither holding the other's — measured, and strictly worse than + // the behaviour this replaced. + // + // The hazard that suppression was reaching for is real but belongs + // to the executor: `replace_session` rewrites the session and both + // indices and touches nothing else, orphaning an in-flight rekey. + // The `SwapToInboundSession` arm therefore abandons that rekey as + // part of the swap, the same way the rekey-responder arm does when + // it loses the dual-rekey tie-break. if snap.rekey_claim == RekeyClaim::None && snap.different_link && snap.has_session && snap.is_healthy - && !snap.rekey_in_progress - && !snap.pending_new_session { // `cross_connection_winner(our, peer, this_is_outbound=false)`: // the smaller node prefers its outbound, so our *inbound* diff --git a/src/proto/fmp/tests/core.rs b/src/proto/fmp/tests/core.rs index fa2b8ea..dc8ccf4 100644 --- a/src/proto/fmp/tests/core.rs +++ b/src/proto/fmp/tests/core.rs @@ -545,29 +545,39 @@ fn establish_mismatched_marker_resends_rather_than_rejecting() { )); } -/// A fresh dial arriving while our own rekey is in flight must not swap. The -/// swap rewrites the session and both indices and touches nothing else, so it -/// would leave the pending rekey and its K-bit state pointing at a session the -/// counterpart no longer holds. The age floor excluded this case incidentally; -/// the declaration does not, so the guard is explicit. +/// A fresh dial crossing our own in-flight rekey still resolves by the +/// cross-connection tie-break, unchanged by the rekey. +/// +/// This decision is one half of a two-sided tie-break: the peer's outbound half +/// (`establish_outbound`) reads only whether it has an existing peer and the +/// address ordering, and cannot see our rekey. Declining the swap here on +/// account of our own state therefore desynchronizes the pair — the peer swaps, +/// we do not, and neither end holds the other's session. Measured on both trees: +/// declining leaves all four indices distinct with no pending session anywhere. +/// +/// The orphaned-rekey hazard is real and is handled where the swap happens, by +/// abandoning the rekey as part of it, rather than by refusing to swap. #[test] -fn establish_undeclared_during_our_rekey_does_not_swap() { +fn establish_undeclared_during_our_rekey_still_cross_connects() { let fmp = Fmp::new(); let wire = wire_outcome(0x02, SAME_EPOCH); - for (in_progress, pending) in [(true, false), (false, true)] { - let mut snap = establish_snapshot(0x09); + for (in_progress, pending) in [(true, false), (false, true), (false, false)] { + let mut snap = establish_snapshot(0x09); // our 0x09 > peer 0x02 snap.different_link = true; snap.rekey_claim = RekeyClaim::None; snap.rekey_in_progress = in_progress; snap.pending_new_session = pending; - assert!( - matches!( - fmp.establish_inbound(&snap, &wire), - InboundDecision::ResendMsg2 { .. } + match fmp.establish_inbound(&snap, &wire) { + InboundDecision::CrossConnect { + our_inbound_wins, .. + } => assert!( + our_inbound_wins, + "the larger node's inbound wins regardless of our rekey state \ + (in_progress={in_progress} pending={pending})" ), - "in_progress={in_progress} pending={pending}" - ); + other => panic!("in_progress={in_progress} pending={pending}: got {other:?}"), + } } } diff --git a/src/proto/fmp/tests/util.rs b/src/proto/fmp/tests/util.rs index 89fb6f3..ea6254f 100644 --- a/src/proto/fmp/tests/util.rs +++ b/src/proto/fmp/tests/util.rs @@ -71,8 +71,8 @@ pub(super) fn resend_snapshot(link: LinkId, resend_count: u32, msg1: Vec) -> } /// Build an `EstablishSnapshot` describing an existing, healthy, same-epoch peer -/// with a rekey-enabled config and a rekey age floor of 100s, owned by node -/// `our_byte`. The default is a quiescent session on the same link with no +/// with a rekey-enabled config, owned by node `our_byte`. +/// The default is a quiescent session on the same link with no /// in-flight rekey / pending and no rekey declared by the sender. Tests override /// only the fields their branch exercises. `existing_peer_epoch` defaults to /// `[0x01; 8]`. diff --git a/src/proto/fmp/tests/wire.rs b/src/proto/fmp/tests/wire.rs index 5e31b46..f8e8fd9 100644 --- a/src/proto/fmp/tests/wire.rs +++ b/src/proto/fmp/tests/wire.rs @@ -1,10 +1,11 @@ //! Tests for the FMP wire codec: XX handshake framing, orderly disconnect, //! and the negotiation payload. -use super::super::wire::NEGOTIATION_HEADER_SIZE; +use super::super::wire::{NEGOTIATION_HEADER_SIZE, TLV_REKEY_OF}; use crate::proto::fmp::{ Disconnect, DisconnectReason, HandshakeMessageType, NegotiationPayload, NodeProfile, }; +use crate::utils::index::SessionIndex; // ===== HandshakeMessageType Tests ===== @@ -208,6 +209,74 @@ fn test_truncated_tlv() { assert!(NegotiationPayload::decode(&partial).is_err()); } +// ===== Rekey marker Tests ===== + +#[test] +fn test_rekey_marker_roundtrip() { + let index = SessionIndex::new(0xDEADBEEF); + let payload = NegotiationPayload::fmp(1, 1, NodeProfile::Full).with_rekey_of(index); + + let decoded = NegotiationPayload::decode(&payload.encode()).unwrap(); + assert_eq!(decoded.rekey_of().unwrap(), Some(index)); +} + +#[test] +fn test_rekey_marker_absent_is_none() { + let payload = NegotiationPayload::fmp(1, 1, NodeProfile::Full); + let decoded = NegotiationPayload::decode(&payload.encode()).unwrap(); + assert_eq!(decoded.rekey_of().unwrap(), None); +} + +/// The forward-compatibility property the marker's rolling upgrade rests on: a +/// peer that adds TLV fields we do not know must not disturb our reading of the +/// one we do. Both directions matter — an unknown field alone still reads as +/// "no rekey declared", and an unknown field alongside the marker still yields +/// the marker. +#[test] +fn test_rekey_marker_unaffected_by_unknown_tlv() { + let unknown_only = NegotiationPayload::fmp(1, 1, NodeProfile::Full) + .with_tlv(9999, vec![0xFF, 0xFE, 0xFD]) + .encode(); + let decoded = NegotiationPayload::decode(&unknown_only).unwrap(); + assert_eq!(decoded.rekey_of().unwrap(), None); + + let index = SessionIndex::new(7); + let with_both = NegotiationPayload::fmp(1, 1, NodeProfile::Full) + .with_tlv(9999, vec![0xFF, 0xFE, 0xFD]) + .with_rekey_of(index) + .with_tlv(4242, vec![0x01]) + .encode(); + let decoded = NegotiationPayload::decode(&with_both).unwrap(); + assert_eq!(decoded.rekey_of().unwrap(), Some(index)); +} + +/// A marker whose value is the wrong length is an error, never an absence. +/// +/// This is the case the container codec cannot catch and the accessor exists +/// for: the TLV is well formed — correct field number, length matching its own +/// value — so `decode` accepts it, and only the accessor can tell that four +/// bytes were expected. Reading it as "no rekey declared" would put a real rekey +/// back on the cross-connection path, which is the defect the marker removes. +#[test] +fn test_rekey_marker_wrong_length_is_error() { + for value in [ + vec![], + vec![0x01, 0x02, 0x03], + vec![0x01, 0x02, 0x03, 0x04, 0x05], + ] { + let len = value.len(); + let encoded = NegotiationPayload::fmp(1, 1, NodeProfile::Full) + .with_tlv(TLV_REKEY_OF, value) + .encode(); + let decoded = NegotiationPayload::decode(&encoded) + .unwrap_or_else(|e| panic!("{len}-byte marker should decode as a TLV: {e}")); + assert!( + decoded.rekey_of().is_err(), + "{len}-byte marker must be an error, not an absence" + ); + } +} + #[test] fn test_node_profile_try_from() { assert_eq!(NodeProfile::try_from(0).unwrap(), NodeProfile::Full);