diff --git a/CHANGELOG.md b/CHANGELOG.md index 9383b60a..aa13cfdd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -273,6 +273,35 @@ with v0.5.x or earlier peers. smaller-NodeAddr resolution rule. Mirrored to the FSP rekey msg1 path for symmetry. +- A node no longer relays away the answer to its own lookup. A request is + flooded to every tree peer whose bloom filter claims the target, so a false + positive can send a copy out into the wider network and circulate it back to + the node that originated it. The only identity test on arrival was whether + the request named this node as the target, which a lookup this node + originated never satisfies, so the copy was filed in the request dedup cache + as ordinary transit under this node's own `request_id`. When the target + answered, the reply was reverse-path forwarded to the peer that looped the + request, the pending lookup was never satisfied, and discovery reported that + its requests went unanswered while the answers were in fact arriving. An + inbound response is now matched against this node's outstanding lookups + before the transit dedup record, and a returning copy of this node's own + request is dropped as the duplicate it is rather than recorded, so that id + never enters the transit cache at all. This was a race rather than a hard + failure: a reply that beat the looped copy found a clean cache and + succeeded, and the failure grew likelier as the bloom fill ratio rose. + Contributed by Arjen. + +- A lookup request of this node's own, returning to it, is no longer counted as + a duplicate from the peer that delivered it. The fix above drops that copy, + and it recorded the drop under the existing `req_duplicate` rejection, whose + documented meaning is that a peer resent a request. A returning copy has a + nonzero floor in healthy operation and rises with the bloom fill ratio, so + folding the two together put a permanent number on a counter an operator + reads as neighbour misbehaviour, and made the two events indistinguishable. + It now has its own rejection reason and counter, `req_own_loopback`, shown in + `fipstop` as "Own Loopback". `req_duplicate` returns to meaning only what it + says. + ### Security - The peer static key is verified on both FMP handshake paths, not only at diff --git a/docs/design/fips-mesh-operation.md b/docs/design/fips-mesh-operation.md index 8ec9635b..11c5b9ef 100644 --- a/docs/design/fips-mesh-operation.md +++ b/docs/design/fips-mesh-operation.md @@ -272,6 +272,17 @@ follows the same path as the request. Greedy tree routing toward the greedy tree routing toward the origin's coordinates is used only as a fallback if the reverse-path entry has expired. +**Originator check first**: a node tests its own outstanding lookups before +consulting `recent_requests`. A request is flooded to every bloom-matching +tree peer, and a bloom false positive can send a copy out into the wider +network and back to the originator, which would otherwise file its own +`request_id` as an ordinary transit entry and relay its own answer away. The +originator arm therefore wins: a response naming a target with a lookup +outstanding, carrying a `request_id` that lookup issued, is accepted here +whatever the dedup cache holds. A returning copy of the request is likewise +dropped as a duplicate rather than recorded, so the originator's id never +enters the transit cache. + **Response-forwarded flag**: Each `recent_requests` entry tracks whether a response has already been forwarded for that `request_id`. If a second response arrives (e.g., from convergent request paths that reached the diff --git a/src/bin/fipstop/ui/routing.rs b/src/bin/fipstop/ui/routing.rs index 1961c8c5..1cae2ca5 100644 --- a/src/bin/fipstop/ui/routing.rs +++ b/src/bin/fipstop/ui/routing.rs @@ -189,6 +189,7 @@ fn draw_routing_stats( ("Deduplicated", lookup("req_deduplicated")), ("Target Is Us", lookup("req_target_is_us")), ("Duplicate", lookup("req_duplicate")), + ("Own Loopback", lookup("req_own_loopback")), ("Bloom Miss", lookup("req_bloom_miss")), ("Backoff Suppressed", lookup("req_backoff_suppressed")), ("Fwd Rate Limited", lookup("req_forward_rate_limited")), diff --git a/src/control/snapshots/show_routing.json b/src/control/snapshots/show_routing.json index 99e8ddd2..0e0685da 100644 --- a/src/control/snapshots/show_routing.json +++ b/src/control/snapshots/show_routing.json @@ -20,6 +20,7 @@ "req_forwarded": 0, "req_initiated": 0, "req_no_tree_peer": 0, + "req_own_loopback": 0, "req_received": 0, "req_sign_rate_limited": 0, "req_target_is_us": 0, @@ -96,6 +97,7 @@ "req_forwarded": 0, "req_initiated": 0, "req_no_tree_peer": 0, + "req_own_loopback": 0, "req_received": 0, "req_sign_rate_limited": 0, "req_target_is_us": 0, diff --git a/src/node/handlers/lookup.rs b/src/node/handlers/lookup.rs index 9feafbd9..7a031814 100644 --- a/src/node/handlers/lookup.rs +++ b/src/node/handlers/lookup.rs @@ -154,6 +154,23 @@ impl Node { "Duplicate LookupRequest, dropping" ); } + RequestOutcome::OwnRequestLooped => { + // Our own flooded request, come back to us through a bloom + // false positive. Counted apart from ReqDuplicate: that one + // describes a peer resending, and this one describes our own + // fan-out returning, so folding them together would put a + // permanent healthy floor on a counter an operator reads as + // neighbour misbehaviour. + self.metrics() + .lookup + .record_reject(DiscoveryReject::ReqOwnLoopback); + debug!( + request_id = request.request_id, + from = %self.peer_display_name(from), + target = %self.peer_display_name(&request.target), + "LookupRequest we originated came back to us, dropping" + ); + } RequestOutcome::RespondAsTarget => { // Answering costs a fresh Schnorr signature every time: the // proof is bound to the requester's request_id, so it cannot diff --git a/src/node/metrics.rs b/src/node/metrics.rs index 6359c5dd..c9e4576b 100644 --- a/src/node/metrics.rs +++ b/src/node/metrics.rs @@ -277,6 +277,7 @@ pub struct LookupMetrics { pub req_received: Padded, pub req_decode_error: Counter, pub req_duplicate: Counter, + pub req_own_loopback: Counter, pub req_dedup_cache_full: Counter, pub req_dedup_evicted: Counter, pub req_sign_rate_limited: Counter, @@ -309,6 +310,7 @@ impl LookupMetrics { match reason { DiscoveryReject::ReqDecodeError => self.req_decode_error.inc(), DiscoveryReject::ReqDuplicate => self.req_duplicate.inc(), + DiscoveryReject::ReqOwnLoopback => self.req_own_loopback.inc(), DiscoveryReject::ReqDedupCacheFull => self.req_dedup_cache_full.inc(), DiscoveryReject::ReqSignRateLimited => self.req_sign_rate_limited.inc(), DiscoveryReject::ReqTtlExhausted => self.req_ttl_exhausted.inc(), @@ -326,6 +328,7 @@ impl LookupMetrics { req_received: self.req_received.get(), req_decode_error: self.req_decode_error.get(), req_duplicate: self.req_duplicate.get(), + req_own_loopback: self.req_own_loopback.get(), req_dedup_cache_full: self.req_dedup_cache_full.get(), req_dedup_evicted: self.req_dedup_evicted.get(), req_sign_rate_limited: self.req_sign_rate_limited.get(), diff --git a/src/node/reject.rs b/src/node/reject.rs index 954cdae0..89287a55 100644 --- a/src/node/reject.rs +++ b/src/node/reject.rs @@ -117,6 +117,16 @@ pub enum DiscoveryReject { /// Tracked via /// [`DiscoveryStats::req_duplicate`](crate::node::stats::DiscoveryStats). ReqDuplicate, + /// A request this node originated, flooded to its bloom-matching tree + /// peers and circulated back to it by one of them. Dropped without being + /// recorded, so the answer is accepted here rather than reverse-path + /// forwarded to the peer that looped it. Tracked via + /// [`DiscoveryStats::req_own_loopback`](crate::node::stats::DiscoveryStats). + /// + /// **This has a nonzero floor in healthy operation** and rises with the + /// bloom fill ratio. It says nothing about the peer that delivered the + /// copy, which is why it is not counted as [`Self::ReqDuplicate`]. + ReqOwnLoopback, /// Request dedup cache (`recent_requests`) is at capacity, so the /// `LookupRequest` is dropped without being forwarded. Tracked via /// [`DiscoveryStats::req_dedup_cache_full`](crate::node::stats::DiscoveryStats). @@ -417,6 +427,7 @@ mod tests { let variants = [ DiscoveryReject::ReqDecodeError, DiscoveryReject::ReqDuplicate, + DiscoveryReject::ReqOwnLoopback, DiscoveryReject::ReqDedupCacheFull, DiscoveryReject::ReqTtlExhausted, DiscoveryReject::ReqSignRateLimited, diff --git a/src/node/stats.rs b/src/node/stats.rs index dda3a6fe..5fc4c702 100644 --- a/src/node/stats.rs +++ b/src/node/stats.rs @@ -340,6 +340,7 @@ pub struct LookupStatsSnapshot { pub req_received: u64, pub req_decode_error: u64, pub req_duplicate: u64, + pub req_own_loopback: u64, pub req_dedup_cache_full: u64, pub req_dedup_evicted: u64, pub req_sign_rate_limited: u64, diff --git a/src/proto/lookup/core.rs b/src/proto/lookup/core.rs index e65230d1..49214b1f 100644 --- a/src/proto/lookup/core.rs +++ b/src/proto/lookup/core.rs @@ -172,6 +172,12 @@ pub(crate) fn plan_initiate(request: &LookupRequest, rv: &impl RoutingView) -> V pub(crate) enum RequestOutcome { /// request_id already in the dedup cache — drop. Duplicate, + /// A request this node originated, looped back to it — drop without + /// recording. Kept separate from `Duplicate`, which means another node + /// resent a request, so the two do not share a rejection counter: this + /// one has a nonzero floor in healthy operation and says nothing about + /// the peer that delivered it. + OwnRequestLooped, /// We are the lookup target — the shell generates + sends the response. RespondAsTarget, /// Forward the request onward (the shell calls the forward planner). @@ -282,6 +288,40 @@ pub(crate) fn classify_request( }; } + // A request this node originated, flooded to every bloom-matching tree + // peer and circulated back to us by one of them. The only identity test + // below is `request.target == *my_addr`, and for a lookup we originated + // the target is someone else, so without this the copy is filed as an + // ordinary transit entry keyed on our own `request_id` — and the answer, + // when it comes, is reverse-path forwarded to the peer that looped the + // request instead of being accepted here. Dropping it as the duplicate it + // is also stops the copy being forwarded a second time: our flood already + // reached the peers that could carry it. + // + // `request.origin` looks like the cheaper identity test and cannot be + // used. It is unsigned and set by whoever sends the frame, so a peer + // could put this node's address on any request and make it refuse to + // transit that request. An id has to have been issued here to match, + // which is what makes this test safe to drop on. + // + // The test reaches only as far as `PendingLookup::ids`, which keeps the + // last `MAX_RECORDED_IDS` that a target's ladder issued. A ladder + // configured with more rungs than that loses its earliest ids, and a + // returning copy of one of those attempts is recorded as transit again. + // The bound is inherited from the originator test in `classify_response` + // rather than introduced here, and a reply to such an attempt was already + // being dropped as unsolicited. + if lookup + .pending_lookups + .get(&request.target) + .is_some_and(|pending| pending.matches(request.request_id)) + { + return Classification { + outcome: RequestOutcome::OwnRequestLooped, + evicted: None, + }; + } + let evicted = make_room(lookup, from, max_recent, peer_count); lookup.record_recent(request.request_id, *from, now_ms); @@ -302,8 +342,9 @@ pub(crate) fn classify_request( Classification { outcome, evicted } } -/// How an inbound LookupResponse should be routed, decided from the -/// recent-request dedup state. +/// How an inbound LookupResponse should be routed, decided from the pending +/// lookups this node has outstanding and, failing that, the recent-request +/// dedup state. pub(crate) enum ResponseRoute { /// A response for a request we forwarded, but we already reverse-forwarded /// one for this request_id — drop to prevent response routing loops. @@ -318,7 +359,8 @@ pub(crate) enum ResponseRoute { Unsolicited, } -/// Classify an inbound LookupResponse against the recent-request dedup cache. +/// Classify an inbound LookupResponse against the pending lookups and the +/// recent-request dedup cache, in that order. /// /// Pure decision over `Lookup` state: sets `response_forwarded` when this is /// the first response we transit for the request. No I/O, no view, no metrics. @@ -327,6 +369,30 @@ pub(crate) fn classify_response( request_id: u64, target: &NodeAddr, ) -> ResponseRoute { + // Originator-ness is decided first, ahead of the transit dedup record. + // The response names a target this node has a lookup outstanding for and + // carries an id that lookup issued, so it answers us whatever else the + // dedup cache happens to hold for that id. Testing `recent_requests` + // first instead made this arm unreachable whenever a copy of our own + // flooded request found its way back to us and was filed as transit: the + // reply was relayed away rather than accepted, the pending lookup was + // never satisfied, and discovery failed while the answers were arriving. + // + // The id is fresh 64-bit randomness drawn per attempt and the target + // signs over it, so a harvested response is bound to the request it + // answered and cannot be redirected or replayed: an id we never issued + // still cannot match, and preferring this arm takes nothing away from + // what the `Unsolicited` arm protects. Replies to earlier attempts of a + // still-outstanding lookup match too, which is the common case on a link + // whose round trip exceeds the first rung of the retry ladder. + if lookup + .pending_lookups + .get(target) + .is_some_and(|pending| pending.matches(request_id)) + { + return ResponseRoute::Originator; + } + match lookup.recent_requests.get_mut(&request_id) { Some(recent) => { if recent.response_forwarded { @@ -338,25 +404,9 @@ pub(crate) fn classify_response( } } } - // Not a request we transited, so it claims to answer one of ours. - // Require that it names a target with a lookup outstanding and carries - // an id issued for it. The id is fresh 64-bit randomness drawn per - // attempt and the target signs over it, so a harvested response is - // bound to the request it answered and cannot be redirected or - // replayed. Replies to earlier attempts of a still-outstanding lookup - // still match, which is the common case on a link whose round trip - // exceeds the first rung of the retry ladder. - None => { - let solicited = lookup - .pending_lookups - .get(target) - .is_some_and(|pending| pending.matches(request_id)); - if solicited { - ResponseRoute::Originator - } else { - ResponseRoute::Unsolicited - } - } + // Neither a request we transited nor an answer to a lookup we have + // outstanding for its target. Nobody asked for it. + None => ResponseRoute::Unsolicited, } } diff --git a/src/proto/lookup/tests/core.rs b/src/proto/lookup/tests/core.rs index 0c5102e3..229e0c51 100644 --- a/src/proto/lookup/tests/core.rs +++ b/src/proto/lookup/tests/core.rs @@ -234,8 +234,8 @@ fn classify_response_transit_on_fresh_forwarded_request() { .recent_requests .insert(42, RecentRequest::new(from_peer, 1000)); - // Transit is decided by the dedup record alone, so the target the response - // names plays no part here — pass one no pending lookup mentions. + // With no lookup of ours outstanding for the target, transit is decided by + // the dedup record — pass a target no pending lookup mentions. match classify_response(&mut lookup, 42, &make_node_addr(0xF1)) { ResponseRoute::Transit { from_peer: peer } => assert_eq!(peer, from_peer), _ => panic!("expected Transit"), @@ -263,6 +263,38 @@ fn classify_response_already_forwarded_on_second_call() { )); } +#[test] +fn classify_response_originator_wins_over_a_transit_dedup_record() { + // Regression. A copy of a request we originated can be flooded back to us + // and filed in `recent_requests` as ordinary transit, keyed on our own + // request_id. Consulting the dedup record first then classified the answer + // as Transit and relayed it to the peer that looped the request, so the + // pending lookup was never satisfied and discovery failed with "no reply" + // while the responses were in fact arriving. Originator-ness wins. + let target = make_node_addr(0x61); + let looping_peer = make_node_addr(0x62); + let mut lookup = empty_lookup(); + lookup + .recent_requests + .insert(4242, RecentRequest::new(looping_peer, 1000)); + let mut pending = PendingLookup::new(1000); + pending.record(4242); + lookup.pending_lookups.insert(target, pending); + + assert!(matches!( + classify_response(&mut lookup, 4242, &target), + ResponseRoute::Originator + )); + // And nothing was reverse-path forwarded on our behalf. + assert!( + !lookup + .recent_requests + .get(&4242) + .unwrap() + .response_forwarded + ); +} + #[test] fn classify_response_originator_when_a_pending_lookup_issued_the_id() { // No dedup record, so this is not a response we transit. It counts as ours @@ -524,6 +556,71 @@ fn classify_request_duplicate_on_second_call() { )); } +#[test] +fn classify_request_drops_our_own_request_looped_back_to_us() { + // Regression, request side of the same defect. Our flood reaches a peer + // that circulates it back; the only identity test is `target == my_addr`, + // which a lookup we originated never satisfies. Recording it would file + // our own request_id as a transit entry and hand the eventual answer to + // the looping peer, so the copy is dropped as the duplicate it is. + let mut lookup = empty_lookup(); + let looping_peer = make_node_addr(0x01); + let my_addr = make_node_addr(0x99); + let target = make_node_addr(0xAA); + let mut pending = PendingLookup::new(1000); + pending.record(77); + lookup.pending_lookups.insert(target, pending); + + // Built here rather than through `make_request_id`, whose origin is a + // third party: the defect is our *own* request returning, so the request + // under test has to carry our address as its origin. The guard keys on + // the id and ignores the origin, so this asserts the same behaviour the + // helper would; it asserts it on the shape the defect actually has. + let request = LookupRequest::new(77, target, my_addr, 3, 0); + let classification = classify_request( + &mut lookup, + &request, + &looping_peer, + &my_addr, + 1000, + 5000, + 4096, + 1, + ); + assert!(matches!( + classification.outcome, + RequestOutcome::OwnRequestLooped + )); + assert!(classification.evicted.is_none()); + // Crucially, our own id must not enter the transit dedup cache. + assert!(!lookup.recent_requests.contains_key(&77)); + assert!(lookup.recent_by_peer.is_empty()); + // A response for it is then ours to accept. + assert!(matches!( + classify_response(&mut lookup, 77, &target), + ResponseRoute::Originator + )); +} + +#[test] +fn classify_request_still_transits_a_foreign_id_for_a_target_we_are_looking_up() { + // The guard keys on the id, not the target: another node's lookup for the + // same target must still be transited normally while ours is outstanding. + let mut lookup = empty_lookup(); + let from = make_node_addr(0x01); + let my_addr = make_node_addr(0x99); + let target = make_node_addr(0xAA); + let mut pending = PendingLookup::new(1000); + pending.record(77); + lookup.pending_lookups.insert(target, pending); + + let request = make_request_id(88, target, 3); + let outcome = + classify_request(&mut lookup, &request, &from, &my_addr, 1000, 5000, 4096, 1).outcome; + assert!(matches!(outcome, RequestOutcome::Forward)); + assert_eq!(lookup.recent_requests.get(&88).unwrap().from_peer, from); +} + #[test] fn classify_request_evicts_rather_than_refusing_a_full_dedup_cache() { // Regression. The cache-full path used to drop the arriving request, so