Swap on a dial that crosses a rekey, and cover the marker's two ends

The rekey marker's cross-connection arm declined to swap while a rekey of
ours was in flight, reasoning that the swap rewrites the session and both
indices while touching no rekey state, so it would orphan that rekey. The
reasoning was one-sided and the arm was wrong: this is half of a two-sided
tie-break whose other half reads only whether a peer exists and the address
ordering, and cannot see our rekey. Declining here while the peer's outbound
half swaps anyway leaves the two ends on four distinct indices with neither
holding a pending session, dead both directions until the link-dead timer.
Measured against the pre-marker tree, which converges: it holds the crossing
dial as pending and adopts it at cutover.

The orphaned-rekey hazard is real but belongs to the executor. The swap now
abandons the rekey it displaces and frees that index first, exactly as the
rekey-responder path already does when it loses the dual-rekey tie-break, so
the classifier can take the arm unconditionally.

Two further failure paths of the marker are tightened. A malformed marker
tore the handshake down without freeing the index msg1 allocated, and that
arm sits ahead of the ACL gate, so any peer able to complete a msg3 could
grow the allocator set one entry at a time. A rekey with no session index to
declare silently omitted the marker, which puts a real rekey back on the
cross-connection path; it is unreachable today and is now asserted rather
than skipped.

On the test side, the marker's wire contract gains coverage: the accessor
round-trips, an unknown TLV field leaves it alone in both directions, and a
well-formed marker of the wrong length is an error rather than an absence,
which is the case the container codec cannot see and the whole reason the
accessor is fallible.

Two node-level tests now assert both ends rather than one. A rekey fired by
message count on a zero-age session is the mechanism behind the datagram
drops, and nothing exercised it: it fails on the pre-marker tree and passes
here. The mutual-dial test gains an index-pair assertion, since every
per-end predicate it checked holds when the ends have diverged.

One existing test had stopped reaching its own arm. Driving a bare second
handshake into a rekey-disabled peer now lands on the cross-connection arm,
which that flag does not gate, and every assertion held there too; it is
retargeted onto a declared rekey, which is the real route to the duplicate
arm, and asserts the session is left alone so the two arms are told apart.

Documentation that described the removed session-age floor is rewritten
around the declaration.
This commit is contained in:
Johnathan Corgan
2026-07-24 21:48:49 +00:00
parent 911a000d85
commit aa204cf729
9 changed files with 447 additions and 68 deletions
+18
View File
@@ -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);
+8
View File
@@ -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;
+5 -3
View File
@@ -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;
+247 -14
View File
@@ -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
+30 -10
View File
@@ -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()?;
+43 -24
View File
@@ -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*
+24 -14
View File
@@ -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:?}"),
}
}
}
+2 -2
View File
@@ -71,8 +71,8 @@ pub(super) fn resend_snapshot(link: LinkId, resend_count: u32, msg1: Vec<u8>) ->
}
/// 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]`.
+70 -1
View File
@@ -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);