laantungir/fips
1
0
Fork 0
mirror of https://github.com/jmcorgan/fips.git synced 2026-08-10 16:43:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

Meter established-link msg1 separately from stranger admission

Browse Source 
The msg1 rate limiter ran before the established-peer carve-out, so a
rekey or restart msg1 arriving on an existing link was refused on
exactly the same terms as a stranger's first packet and the carve-out
below it never applied to the traffic it was written for. On a node
with many peers this refuses a large share of ordinary maintenance
traffic: a field node at roughly 245 peers refused 8753 msg1 in 25
minutes, and 159 of the 201 distinct sources were peers it already
held sessions with.

Classify the source before metering it, and give established-link msg1
its own token bucket instead of a bypass. A bypass was rejected
deliberately: the limiter is global precisely because UDP sources are
spoofable, and an established-peer exemption is by construction keyed
on source address, so metering the exempted class keeps that property
where bypassing discards it.

The bucket is derived from settings the operator already sets, burst
from max_peers and rate from max_peers, the rekey interval and the
resend budget, so raising the peer limit sizes it automatically rather
than leaving a constant nobody revisits. Both parameters can be set
explicitly; an explicit zero burst or non-positive rate is rejected at
config validation, because it would refuse every rekey msg1 from an
established peer rather than disabling the limit.

Split out the established-link test as its own predicate so the
rate-limit classifier and the accept_connections gate cannot drift,
and convert the limiter's pending slot to a guard released on drop.
The slot was previously acquired in one place and released explicitly
at eighteen exit paths; once some paths stop acquiring one, any path
that still released one would have freed a slot belonging to a
different in-flight handshake, lifting effective concurrency above the
configured maximum with no counter moving and no log firing.

The "Msg1 rate limited" line now reports which limb refused, the
pending count or the token bucket, which it did not distinguish
before.

Classification costs an O(peers) scan on every inbound msg1 including
refused ones, where the previous order refused at O(1). The scan is
only needed because addr_to_link is keyed on the unresolved dial
address; correcting that keying reduces this to a single O(1) lookup.
Recorded at the predicate.
This commit is contained in:
Johnathan Corgan
2026-07-29 17:10:29 +00:00
parent 13a98ae702
commit f5be8ec07d
9 changed files with 958 additions and 107 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/node/handlers/handshake.rs
+71 -37
View File
@@ -2,6 +2,7 @@
use crate::PeerIdentity;
use crate::node::acl::PeerAclContext;
use crate::node::rate_limit::Msg1Class;
use crate::node::reject::{HandshakeReject, RejectReason};
use crate::node::wire::{Msg1Header, Msg2Header, build_msg2};
use crate::node::{Node, NodeError};
@@ -11,13 +12,14 @@ use std::time::Duration;
use tracing::{debug, info, warn};
impl Node {
/// Returns true if an inbound msg1 should be admitted past the
/// `accept_connections` gate.
/// Returns true if an inbound msg1's source matches an established
/// link, i.e. it is rekey/restart maintenance traffic rather than a
/// stranger's fresh handshake.
///
/// Rekey/restart msg1 from an established peer is always admitted (the
/// gate is meant to filter fresh handshakes from strangers, not
/// maintenance traffic on established sessions). Two predicates cover
/// "established peer at this transport+addr":
/// This is deliberately separate from the `accept_connections` gate:
/// it is the only half of `should_admit_msg1` that is a safe basis
/// for exempting traffic from stranger-class treatment. Two
/// predicates cover "established peer at this transport+addr":
///
/// 1. `addr_to_link` has an entry for `(transport_id, remote_addr)`.
/// This is the fast path and matches when the peer registered with
@@ -35,9 +37,14 @@ impl Node {
/// with `udp.accept_connections: false` or `udp.outbound_only: true`
/// (the production trigger for the 2026-04-30 bug).
///
/// Otherwise the transport's `accept_connections` config decides;
/// absence of a registered transport admits (no gate to apply).
pub(in crate::node) fn should_admit_msg1(
/// Cost: predicate 1 is O(1), predicate 2 is O(peers). Because
/// `handle_msg1` classifies before rate limiting, predicate 2 runs on
/// every inbound msg1 including those about to be refused, so a msg1
/// flood costs O(peers) per dropped packet rather than O(1). Predicate 2
/// exists only because `addr_to_link` is keyed on the *unresolved* dial
/// address; if that keying is corrected, this becomes a single O(1)
/// lookup and the flood cost returns to O(1).
pub(in crate::node) fn is_established_link_msg1(
&self,
transport_id: crate::transport::TransportId,
remote_addr: &crate::transport::TransportAddr,
@@ -53,6 +60,26 @@ impl Node {
}) {
return true;
}
false
}
/// Returns true if an inbound msg1 should be admitted past the
/// `accept_connections` gate.
///
/// Rekey/restart msg1 from an established peer is always admitted (the
/// gate is meant to filter fresh handshakes from strangers, not
/// maintenance traffic on established sessions).
///
/// Otherwise the transport's `accept_connections` config decides;
/// absence of a registered transport admits (no gate to apply).
pub(in crate::node) fn should_admit_msg1(
&self,
transport_id: crate::transport::TransportId,
remote_addr: &crate::transport::TransportAddr,
) -> bool {
if self.is_established_link_msg1(transport_id, remote_addr) {
return true;
}
self.transports
.get(&transport_id)
.is_none_or(|t| t.accept_connections())
@@ -62,23 +89,47 @@ impl Node {
///
/// This creates a new inbound connection. Rate limiting is applied
/// before any expensive crypto operations.
///
/// Classifying the source costs no crypto (two map/scan lookups), so it
/// happens first and selects which bucket the msg1 draws on: rekey and
/// restart traffic from an established link stops competing with
/// stranger admission, while still being metered.
pub(in crate::node) async fn handle_msg1(&mut self, packet: ReceivedPacket) {
// === RATE LIMITING (before any processing) ===
if !self.msg1_rate_limiter.start_handshake() {
debug!(
transport_id = %packet.transport_id,
remote_addr = %packet.remote_addr,
"Msg1 rate limited"
);
return;
}
// === CLASSIFY, THEN RATE LIMIT (both before any crypto) ===
// Classification is two map lookups; the second is O(peers) and now
// runs on every inbound msg1, including refused ones. See the
// `is_established_link_msg1` doc comment for why the scan is still
// needed and what would retire it.
let established = self.is_established_link_msg1(packet.transport_id, &packet.remote_addr);
let class = if established {
Msg1Class::EstablishedLink
} else {
Msg1Class::Stranger
};
let _slot = match self.msg1_rate_limiter.start_handshake(class) {
Ok(slot) => slot,
Err(reason) => {
debug!(
transport_id = %packet.transport_id,
remote_addr = %packet.remote_addr,
refused_by = %reason,
"Msg1 rate limited"
);
return;
}
};
// accept_connections gate. Rekey/restart msg1 on an existing link
// is always admitted; the gate only filters truly-fresh connections
// from strangers. Without this carve-out, the dual-init tie-breaker
// deadlocks when the larger-NodeAddr side has accept_connections=false.
if !self.should_admit_msg1(packet.transport_id, &packet.remote_addr) {
self.msg1_rate_limiter.complete_handshake();
//
// `!established &&` is not a behaviour change: `should_admit_msg1`
// is `is_established_link_msg1() || accept_connections()`, so the
// short-circuit only skips a second evaluation of the `peers` scan
// on the hot path. The call is left in place so the two predicates
// cannot drift apart.
if !established && !self.should_admit_msg1(packet.transport_id, &packet.remote_addr) {
self.stats_mut()
.record_reject(RejectReason::Handshake(HandshakeReject::BadState));
return;
@@ -88,7 +139,6 @@ impl Node {
let header = match Msg1Header::parse(&packet.data) {
Some(h) => h,
None => {
self.msg1_rate_limiter.complete_handshake();
debug!("Invalid msg1 header");
self.stats_mut()
.record_reject(RejectReason::Handshake(HandshakeReject::BadState));
@@ -146,7 +196,6 @@ impl Node {
HandshakeReject::UnknownConnection,
));
}
self.msg1_rate_limiter.complete_handshake();
return;
}
} else {
@@ -187,7 +236,6 @@ impl Node {
) {
Ok(m) => m,
Err(e) => {
self.msg1_rate_limiter.complete_handshake();
debug!(
error = %e,
"Failed to process msg1"
@@ -202,7 +250,6 @@ impl Node {
let peer_identity = match conn.expected_identity() {
Some(id) => *id,
None => {
self.msg1_rate_limiter.complete_handshake();
warn!("Identity not learned from msg1");
self.stats_mut()
.record_reject(RejectReason::Handshake(HandshakeReject::BadState));
@@ -243,7 +290,6 @@ impl Node {
// `link_id` was allocated above but `conn` is still a local
// (not yet inserted into self.connections / self.links /
// self.addr_to_link), so the local drop suffices.
self.msg1_rate_limiter.complete_handshake();
self.stats_mut()
.record_reject(RejectReason::Handshake(HandshakeReject::BadState));
return;
@@ -300,7 +346,6 @@ impl Node {
);
self.connections.remove(&link_id);
self.links.remove(&link_id);
self.msg1_rate_limiter.complete_handshake();
self.stats_mut()
.record_reject(RejectReason::Handshake(HandshakeReject::BadState));
return;
@@ -321,7 +366,6 @@ impl Node {
);
self.connections.remove(&link_id);
self.links.remove(&link_id);
self.msg1_rate_limiter.complete_handshake();
self.stats_mut().record_reject(RejectReason::Handshake(
HandshakeReject::BadState,
));
@@ -350,7 +394,6 @@ impl Node {
Ok(idx) => idx,
Err(e) => {
warn!(error = %e, "Failed to allocate index for rekey");
self.msg1_rate_limiter.complete_handshake();
self.stats_mut().record_reject(RejectReason::Handshake(
HandshakeReject::BadState,
));
@@ -363,7 +406,6 @@ impl Node {
None => {
warn!("Rekey msg1: no session from handshake");
let _ = self.index_allocator.free(our_new_index);
self.msg1_rate_limiter.complete_handshake();
self.stats_mut().record_reject(RejectReason::Handshake(
HandshakeReject::BadState,
));
@@ -390,7 +432,6 @@ impl Node {
"Failed to send rekey msg2"
);
let _ = self.index_allocator.free(our_new_index);
self.msg1_rate_limiter.complete_handshake();
self.stats_mut().record_reject(RejectReason::Handshake(
HandshakeReject::BadState,
));
@@ -422,7 +463,6 @@ impl Node {
self.connections.remove(&link_id);
self.links.remove(&link_id);
self.msg1_rate_limiter.complete_handshake();
return;
}
@@ -442,7 +482,6 @@ impl Node {
),
}
}
self.msg1_rate_limiter.complete_handshake();
return;
}
}
@@ -459,7 +498,6 @@ impl Node {
)
.is_err()
{
self.msg1_rate_limiter.complete_handshake();
self.stats_mut()
.record_reject(RejectReason::Handshake(HandshakeReject::BadState));
return;
@@ -472,7 +510,6 @@ impl Node {
let our_index = match self.index_allocator.allocate() {
Ok(idx) => idx,
Err(e) => {
self.msg1_rate_limiter.complete_handshake();
warn!(error = %e, "Failed to allocate session index for inbound");
self.stats_mut()
.record_reject(RejectReason::Handshake(HandshakeReject::BadState));
@@ -525,7 +562,6 @@ impl Node {
self.addr_to_link
.remove(&(packet.transport_id, packet.remote_addr));
let _ = self.index_allocator.free(our_index);
self.msg1_rate_limiter.complete_handshake();
self.stats_mut()
.record_reject(RejectReason::Handshake(HandshakeReject::BadState));
return;
@@ -618,8 +654,6 @@ impl Node {
.record_reject(RejectReason::Handshake(HandshakeReject::BadState));
}
}
self.msg1_rate_limiter.complete_handshake();
}
/// Find stored msg2 bytes for a given link (pre- or post-promotion).