From d91d1a560c6684c7b07cd958ffe79c29d77ece27 Mon Sep 17 00:00:00 2001 From: Jeremy Carter Date: Sat, 19 Sep 2026 11:58:30 -0400 Subject: [PATCH 1/3] Add an explicit insecure mode so the demo can succeed locally client_demo.rs's register call was always refused, since its hardcoded rdns claim can't genuinely forward-confirm against wherever the demo happens to run -- correct behavior, but it meant the showcased demo could never actually demonstrate a successful registration without real reverse-DNS infrastructure. Adds binda_core::fcrdns::AllowAllVerifier: an RdnsVerifier that accepts any claim unchecked, heavily documented as demo/test-only and never safe for a reachable deployment. Node gains new_allowing_any_rdns (alongside the existing, still-default new) and an rdns_verifier field so run_client_api asks whichever verifier the node was built with instead of hardcoding FcrdnsVerifier. Wires this up behind an explicit `--insecure-skip-rdns-verification` CLI flag, which prints a loud warning to stderr when used. Updates the README's demo command and client_demo.rs's own doc comment to start the node with that flag, so the showcased walkthrough now actually completes (Registered/RecordsSet/populated resolves) instead of stopping at a (correct, but demo-defeating) refusal. Co-Authored-By: Claude Sonnet 5 --- README.md | 14 +++- crates/binda-core/src/fcrdns.rs | 28 +++++++ crates/binda/examples/client_demo.rs | 22 +++-- crates/binda/src/main.rs | 36 ++++++++- crates/binda/src/node.rs | 117 ++++++++++++++++++++++++--- 5 files changed, 194 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index f5daee3..c4220b1 100644 --- a/README.md +++ b/README.md @@ -111,6 +111,13 @@ binda --gossip 0.0.0.0:9530 --resolver 0.0.0.0:9531 --api 0.0.0.0:9532 \ - `--peer` — a known peer's gossip address; repeatable. Peers also learn about each other dynamically from inbound gossip, so only one bootstrap peer per new node is typically needed. +- `--insecure-skip-rdns-verification` — accept every claimed `rdns` + hostname without checking it (see + [`fcrdns::AllowAllVerifier`](crates/binda-core/src/fcrdns.rs)). This is + for running a node locally, for demos and manual testing, where no + claimed hostname could ever genuinely forward-confirm. **Never pass + this on a node anyone else can reach** — it turns the "5 domains per + live socket" cap back into "5 domains per free keypair." A node's clock is disciplined against public NTP servers ([`ntp::NtpTimeSource`](crates/binda-core/src/ntp.rs)) rather than trusting @@ -121,10 +128,13 @@ See [`examples/client_demo.rs`](crates/binda/examples/client_demo.rs) for a full walkthrough of a client: probe liveness, register a Unicode domain, publish an A record, then resolve it both via BINDA's own resolver protocol and via BINDA's native, DNS-shaped protocol — proving -the label crosses the wire as raw UTF-8, not Punycode. +the label crosses the wire as raw UTF-8, not Punycode. The node below is +started with `--insecure-skip-rdns-verification` so the demo's made-up +`rdns` claim is accepted and `register` actually succeeds; drop that flag +to see the real FCrDNS check correctly refuse it instead. ```bash -cargo run -p binda --bin binda -- --gossip 127.0.0.1:9530 --resolver 127.0.0.1:9531 --api 127.0.0.1:9532 --dns 127.0.0.1:9533 & +cargo run -p binda --bin binda -- --gossip 127.0.0.1:9530 --resolver 127.0.0.1:9531 --api 127.0.0.1:9532 --dns 127.0.0.1:9533 --insecure-skip-rdns-verification & cargo run -p binda --example client_demo ``` diff --git a/crates/binda-core/src/fcrdns.rs b/crates/binda-core/src/fcrdns.rs index d3ffeab..c82df0c 100644 --- a/crates/binda-core/src/fcrdns.rs +++ b/crates/binda-core/src/fcrdns.rs @@ -54,6 +54,27 @@ impl RdnsVerifier for FcrdnsVerifier { } } +/// A verifier that accepts every claimed hostname without checking it +/// against anything. +/// +/// **Never use this for a real deployment.** It exists purely so a node +/// can be run locally (on a laptop with no reverse DNS delegation, behind +/// NAT, on 127.0.0.1) for demos, examples, and manual testing, where no +/// claimed `rdns` could ever forward-confirm for real. With this +/// verifier installed, the "5 domains per live socket" cap once again +/// means nothing more than "5 domains per free keypair" — exactly the +/// gap [`FcrdnsVerifier`] exists to close. A node only uses this if it +/// was explicitly started with a flag that says so out loud (see the +/// `binda` binary's `--insecure-skip-rdns-verification`). +#[derive(Debug, Default, Clone, Copy)] +pub struct AllowAllVerifier; + +impl RdnsVerifier for AllowAllVerifier { + fn verify(&self, _source_ip: IpAddr, _claimed_rdns: &str) -> bool { + true + } +} + /// Full FCrDNS check: `claimed_rdns` must appear in `source_ip`'s PTR /// records, and a forward lookup of `claimed_rdns` must resolve back to /// `source_ip`. @@ -358,6 +379,13 @@ mod tests { assert!(!verifier.verify(ip, "someone-else.example.net")); } + #[test] + fn allow_all_verifier_accepts_any_claim() { + let ip: IpAddr = "203.0.113.7".parse().unwrap(); + assert!(AllowAllVerifier.verify(ip, "literally-anything.invalid")); + assert!(AllowAllVerifier.verify(ip, "")); + } + fn encode_name(name: &str) -> Vec { let mut buf = Vec::new(); for label in name.split('.') { diff --git a/crates/binda/examples/client_demo.rs b/crates/binda/examples/client_demo.rs index d066733..78641ba 100644 --- a/crates/binda/examples/client_demo.rs +++ b/crates/binda/examples/client_demo.rs @@ -3,18 +3,24 @@ //! resolver protocol and via BINDA's native DNS-shaped wire protocol //! (which carries the label as raw UTF-8 — no Punycode involved). //! -//! Note: the `rdns` claimed below (`client-demo.example.net`) almost -//! certainly won't forward-confirm against wherever you actually run -//! this from (see `binda_core::fcrdns`), so `register` is expected to -//! come back as an `Error` unless you edit `rdns` to a hostname that -//! genuinely resolves back to your machine's real source IP. -//! -//! Run a node first, then this example against it: +//! The `rdns` claimed below (`client-demo.example.net`) is made up and +//! won't forward-confirm against wherever you actually run this from +//! (see `binda_core::fcrdns`) — that check needs a real, reverse-DNS- +//! delegated host, which a laptop or CI runner doesn't have. To see the +//! full flow succeed locally, start the node with +//! `--insecure-skip-rdns-verification` (see +//! [`binda_core::fcrdns::AllowAllVerifier`] for exactly what that turns +//! off, and why it must never be used on a node anyone else can reach): //! //! ```bash -//! cargo run -p binda --bin binda -- --gossip 127.0.0.1:9530 --resolver 127.0.0.1:9531 --api 127.0.0.1:9532 --dns 127.0.0.1:9533 & +//! cargo run -p binda --bin binda -- --gossip 127.0.0.1:9530 --resolver 127.0.0.1:9531 --api 127.0.0.1:9532 --dns 127.0.0.1:9533 --insecure-skip-rdns-verification & //! cargo run -p binda --example client_demo //! ``` +//! +//! Run it *without* that flag instead if you want to see the FCrDNS +//! check do its job and correctly refuse the made-up `rdns` — `register` +//! will come back as an `Error` in that case, which is expected, not a +//! bug. use std::net::UdpSocket; use std::time::{SystemTime, UNIX_EPOCH}; diff --git a/crates/binda/src/main.rs b/crates/binda/src/main.rs index c66824d..63ce47a 100644 --- a/crates/binda/src/main.rs +++ b/crates/binda/src/main.rs @@ -15,6 +15,15 @@ //! protocol, not RFC 1035; a production deployment might still bind it to //! port 53 as a convenient, familiar-looking address, which needs //! elevated privilege on most systems). +//! +//! `--insecure-skip-rdns-verification` disables the FCrDNS check a +//! `Register` request's claimed `rdns` normally has to pass (see +//! [`binda_core::fcrdns`]). It exists so a node can be run locally — on +//! a laptop, behind NAT, with no real reverse-DNS delegation — for +//! demos and manual testing, where no claimed hostname could ever +//! genuinely forward-confirm. **Never pass this flag on a node anyone +//! else can reach**: it turns the "5 domains per live socket" cap back +//! into "5 domains per free keypair." // `main` itself is excluded from coverage accounting (see the attribute // on it below): it's pure top-level wiring — parse args, construct a @@ -40,6 +49,7 @@ struct Args { api_addr: SocketAddr, dns_addr: Option, peers: Vec, + insecure_skip_rdns_verification: bool, } fn parse_args() -> Args { @@ -55,6 +65,7 @@ fn parse_args_from(args: impl Iterator) -> Args { let mut api_addr: SocketAddr = "0.0.0.0:9532".parse().unwrap(); let mut dns_addr: Option = None; let mut peers = Vec::new(); + let mut insecure_skip_rdns_verification = false; let mut args = args; while let Some(flag) = args.next() { @@ -96,6 +107,9 @@ fn parse_args_from(args: impl Iterator) -> Args { .expect("invalid --peer address"); peers.push(addr); } + "--insecure-skip-rdns-verification" => { + insecure_skip_rdns_verification = true; + } other => { eprintln!("binda: ignoring unrecognized argument {other}"); } @@ -108,6 +122,7 @@ fn parse_args_from(args: impl Iterator) -> Args { api_addr, dns_addr, peers, + insecure_skip_rdns_verification, } } @@ -171,7 +186,16 @@ async fn main() -> std::io::Result<()> { let args = parse_args(); println!("binda (bind10) — starting node"); - let node = Node::new(args.peers.clone()); + let node = if args.insecure_skip_rdns_verification { + eprintln!( + "binda: WARNING — running with --insecure-skip-rdns-verification: \ + every claimed rdns hostname is accepted unchecked. Never run a \ + node reachable by anyone else this way." + ); + Node::new_allowing_any_rdns(args.peers.clone()) + } else { + Node::new(args.peers.clone()) + }; node.spawn_rate_limiter_maintenance(); println!( "binda: known peers at startup: {:?}", @@ -203,6 +227,13 @@ mod tests { assert_eq!(parsed.api_addr, "0.0.0.0:9532".parse().unwrap()); assert_eq!(parsed.dns_addr, None); assert!(parsed.peers.is_empty()); + assert!(!parsed.insecure_skip_rdns_verification); + } + + #[test] + fn insecure_skip_rdns_verification_flag_is_recognized() { + let parsed = args(&["--insecure-skip-rdns-verification"]); + assert!(parsed.insecure_skip_rdns_verification); } #[test] @@ -273,6 +304,7 @@ mod tests { api_addr: "127.0.0.1:29592".parse().unwrap(), dns_addr: Some("127.0.0.1:29593".parse().unwrap()), peers: Vec::new(), + insecure_skip_rdns_verification: false, }; let tasks = spawn_listener_tasks(&node, &parsed_args); assert!(tasks.dns.is_some()); @@ -291,6 +323,7 @@ mod tests { api_addr: "127.0.0.1:29596".parse().unwrap(), dns_addr: None, peers: Vec::new(), + insecure_skip_rdns_verification: false, }; let tasks = spawn_listener_tasks(&node, &parsed_args); assert!(tasks.dns.is_none()); @@ -321,6 +354,7 @@ mod tests { api_addr, dns_addr: Some(dns_addr), peers: Vec::new(), + insecure_skip_rdns_verification: false, }; let tasks = spawn_listener_tasks(&node, &parsed_args); diff --git a/crates/binda/src/node.rs b/crates/binda/src/node.rs index a94154e..b414e88 100644 --- a/crates/binda/src/node.rs +++ b/crates/binda/src/node.rs @@ -75,14 +75,34 @@ pub struct Node { /// correctly in the matching [`GossipMessage::Rumors`] reply, keyed /// by the peer address the request was sent to. pending_challenges: Arc>>, + /// What checks a `Register` request's claimed `rdns` hostname against + /// its actual source address. Real deployments must use the default + /// ([`fcrdns::FcrdnsVerifier`]); swapping in + /// [`fcrdns::AllowAllVerifier`] (via + /// [`Node::new_allowing_any_rdns`]) is for local demos and testing + /// only, and is never the default. + rdns_verifier: Arc, } impl Node { /// Construct a node whose clock is disciplined against public NTP - /// servers. This blocks briefly (bounded by the per-server query - /// timeout) to get an initial offset before the node starts trusting - /// it for liveness decisions. + /// servers, and whose registrations are gated by real FCrDNS + /// verification. This blocks briefly (bounded by the per-server + /// query timeout) to get an initial NTP offset before the node + /// starts trusting it for liveness decisions. pub fn new(peers: Vec) -> Self { + Self::new_with_verifier(peers, Arc::new(fcrdns::FcrdnsVerifier)) + } + + /// Construct a node that accepts *any* claimed `rdns` hostname + /// without checking it — see [`fcrdns::AllowAllVerifier`] for why + /// this only ever belongs in a local demo or test, never a real + /// deployment reachable by anyone else. + pub fn new_allowing_any_rdns(peers: Vec) -> Self { + Self::new_with_verifier(peers, Arc::new(fcrdns::AllowAllVerifier)) + } + + fn new_with_verifier(peers: Vec, rdns_verifier: Arc) -> Self { let time: Arc = Arc::new(NtpTimeSource::spawn_default()); Self { store: Arc::new(Mutex::new(RegistryStore::new())), @@ -93,6 +113,7 @@ impl Node { api_limiter: Arc::new(new_rate_limiter()), dns_limiter: Arc::new(new_rate_limiter()), pending_challenges: Arc::new(Mutex::new(HashMap::new())), + rdns_verifier, } } @@ -351,19 +372,19 @@ impl Node { }; // Only a Register actually needs this (it's what the "5 - // domains per live socket" cap gates), and it's a real, - // blocking network round-trip against the public DNS system, - // so it runs off the async runtime's worker threads and - // (crucially) before the store's lock is ever taken. + // domains per live socket" cap gates), and (with the real + // FcrdnsVerifier) it's a blocking network round-trip against + // the public DNS system, so it runs off the async runtime's + // worker threads and (crucially) before the store's lock is + // ever taken. let rdns_verified = match &request { ClientRequest::Register(envelope) => { let source_ip = from.ip(); let claimed_rdns = envelope.rdns.clone(); - tokio::task::spawn_blocking(move || { - fcrdns::FcrdnsVerifier.verify(source_ip, &claimed_rdns) - }) - .await - .unwrap_or(false) + let verifier = self.rdns_verifier.clone(); + tokio::task::spawn_blocking(move || verifier.verify(source_ip, &claimed_rdns)) + .await + .unwrap_or(false) } _ => true, }; @@ -451,6 +472,19 @@ mod tests { api_limiter: Arc::new(new_rate_limiter()), dns_limiter: Arc::new(new_rate_limiter()), pending_challenges: Arc::new(Mutex::new(HashMap::new())), + rdns_verifier: Arc::new(fcrdns::FcrdnsVerifier), + } + } + + /// Same as [`test_node`], but with FCrDNS verification switched off, + /// so `Register` requests succeed without needing real reverse-DNS + /// infrastructure. Used only to test the `Register` happy path + /// itself; every other test uses the real verifier so a wrongly + /// "always succeeds" verifier can't mask a real regression. + fn test_node_allowing_any_rdns(peers: Vec) -> Node { + Node { + rdns_verifier: Arc::new(fcrdns::AllowAllVerifier), + ..test_node(peers) } } @@ -896,4 +930,63 @@ mod tests { other => panic!("expected Error (no real host can forward-confirm this made-up rdns), got {other:?}"), } } + + #[tokio::test(flavor = "multi_thread")] + async fn register_via_client_api_succeeds_with_rdns_verification_disabled() { + use binda_core::client_api::{ + probe_message, register_message, ClientRequest, ClientResponse, ProbeBody, + RegisterBody, SignedEnvelope, + }; + use ed25519_dalek::Signer; + + let api_addr: SocketAddr = "127.0.0.1:29562".parse().unwrap(); + let node = test_node_allowing_any_rdns(Vec::new()); + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_client_api(api_addr).await; + }); + tokio::time::sleep(StdDuration::from_millis(100)).await; + + let client_socket = std::net::UdpSocket::bind("127.0.0.1:0").unwrap(); + client_socket + .set_read_timeout(Some(StdDuration::from_secs(3))) + .unwrap(); + let signing_key = SigningKey::generate(&mut OsRng); + let rdns = "this-would-never-forward-confirm.invalid".to_string(); + + let t = node.time.now_millis(); + let probe_msg = probe_message(t); + let probe_sig = signing_key.sign(&probe_msg); + let probe_req = ClientRequest::Probe(SignedEnvelope { + verifying_key: signing_key.verifying_key().to_bytes().to_vec(), + rdns: rdns.clone(), + timestamp_millis: t, + signature: probe_sig.to_bytes().to_vec(), + body: ProbeBody, + }); + client_socket + .send_to(&wire::encode(&probe_req).unwrap(), api_addr) + .unwrap(); + let mut buf = vec![0u8; wire::MAX_DATAGRAM_BYTES]; + let (len, _) = client_socket.recv_from(&mut buf).unwrap(); + let _: ClientResponse = wire::decode(&buf[..len]).unwrap(); + + let domain = DomainName::new("demo-succeeds.binda").unwrap(); + let t = node.time.now_millis(); + let register_msg = register_message(&domain, t); + let register_sig = signing_key.sign(®ister_msg); + let register_req = ClientRequest::Register(SignedEnvelope { + verifying_key: signing_key.verifying_key().to_bytes().to_vec(), + rdns, + timestamp_millis: t, + signature: register_sig.to_bytes().to_vec(), + body: RegisterBody { domain }, + }); + client_socket + .send_to(&wire::encode(®ister_req).unwrap(), api_addr) + .unwrap(); + let (len, _) = client_socket.recv_from(&mut buf).unwrap(); + let response: ClientResponse = wire::decode(&buf[..len]).unwrap(); + assert!(matches!(response, ClientResponse::Registered { .. })); + } } From 0de0ca6cf2def7331cb4999009a2982aaf8e0baa Mon Sep 17 00:00:00 2001 From: Jeremy Carter Date: Sat, 19 Sep 2026 12:08:10 -0400 Subject: [PATCH 2/3] Cover the new rdns_verifier paths and node.rs's listener defenses The insecure-demo-flag work added new_allowing_any_rdns and an rdns_verifier field with no direct test coverage of its own, and node.rs's per-listener rate limiting, malformed-input handling, and no-peers idling had never been exercised end to end despite being real defensive behavior. Adds: - new_allowing_any_rdns_constructs_a_working_node: calls the public constructor directly (main() is otherwise the only caller, and it's excluded from coverage). - rate_limiter_maintenance_prunes_stale_pending_challenges_but_keeps_fresh_ones: seeds a fresh and a stale pending challenge and confirms the maintenance task's retain logic actually discriminates between them (previously only smoke-tested with an empty map). - gossip_push_loop_idles_safely_with_no_known_peers: a node with data but no peers doesn't panic or block. - gossip_recv_loop_ignores_garbage_bytes_and_keeps_working, resolver_ignores_undecodable_bytes_and_keeps_working, client_api_ignores_undecodable_bytes_and_keeps_working: each listener survives undecodable input and still answers a valid follow-up message. - {gossip,resolver,client_api,dns}_rate_limiter_drops_traffic_past_the_burst: 40 rapid requests from one source get measurably fewer than 40 replies, exercising each listener's own rate-limit `continue` branch end to end rather than only unit-testing RateLimiter in isolation. - native_dns_loop_returns_nxdomain_for_unknown_domain: the DNS-shaped loop's "no such domain" path, alongside the existing "known domain" and "unparseable message" tests. Also drops a test that tried to send an over-MAX_DIGEST_ENTRIES Digest through the real wire: encoding that many entries costs far more than MAX_DATAGRAM_BYTES in JSON overhead alone, so wire::decode's own size cap always rejects it before is_well_formed ever runs -- that specific rejection path is structurally unreachable via a real round trip with today's constants, and is already covered directly in binda_core::gossip's own unit tests. binda binary crate: 27 -> 38 tests. Workspace line coverage: ~98.4% -> ~98.7%. Co-Authored-By: Claude Sonnet 5 --- crates/binda/src/node.rs | 383 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 383 insertions(+) diff --git a/crates/binda/src/node.rs b/crates/binda/src/node.rs index b414e88..9e6e76a 100644 --- a/crates/binda/src/node.rs +++ b/crates/binda/src/node.rs @@ -989,4 +989,387 @@ mod tests { let response: ClientResponse = wire::decode(&buf[..len]).unwrap(); assert!(matches!(response, ClientResponse::Registered { .. })); } + + #[tokio::test(flavor = "multi_thread")] + async fn new_allowing_any_rdns_constructs_a_working_node() { + // Exercises the public constructor directly (main() is the only + // other caller, and it's excluded from coverage), rather than + // going through the test-only struct-literal helper. + let node = Node::new_allowing_any_rdns(Vec::new()); + assert!(node + .store + .lock() + .await + .lookup(&DomainName::new("nobody.binda").unwrap()) + .is_none()); + } + + #[tokio::test(flavor = "multi_thread")] + async fn rate_limiter_maintenance_prunes_stale_pending_challenges_but_keeps_fresh_ones() { + let node = test_node(Vec::new()); + let fresh_peer: SocketAddr = "127.0.0.1:1".parse().unwrap(); + let stale_peer: SocketAddr = "127.0.0.1:2".parse().unwrap(); + { + let mut pending = node.pending_challenges.lock().await; + pending.insert( + fresh_peer, + (ConformanceChallenge::random(0), Instant::now()), + ); + let long_ago = Instant::now() + .checked_sub(PENDING_CHALLENGE_TIMEOUT + Duration::from_secs(1)) + .expect("test process has been up long enough for this"); + pending.insert(stale_peer, (ConformanceChallenge::random(0), long_ago)); + } + + node.spawn_rate_limiter_maintenance(); + // The maintenance loop's first tick fires immediately (tokio + // interval semantics), so a short wait is enough to observe it. + tokio::time::sleep(StdDuration::from_millis(50)).await; + + let pending = node.pending_challenges.lock().await; + assert!(pending.contains_key(&fresh_peer)); + assert!(!pending.contains_key(&stale_peer)); + } + + #[tokio::test(flavor = "multi_thread")] + async fn gossip_push_loop_idles_safely_with_no_known_peers() { + let gossip_addr: SocketAddr = "127.0.0.1:29610".parse().unwrap(); + let node = test_node(Vec::new()); + seed_registration( + &node, + &DomainName::new("has-data-but-no-peers.binda").unwrap(), + ) + .await; + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_gossip(gossip_addr).await; + }); + // Long enough for several push-loop ticks; nothing to assert on + // directly (there's no peer to send to), this just proves the + // loop doesn't panic or block when self.peers.random_peer() + // returns None every time. + tokio::time::sleep(GOSSIP_INTERVAL * 2 + StdDuration::from_millis(200)).await; + } + + #[tokio::test(flavor = "multi_thread")] + async fn gossip_recv_loop_ignores_garbage_bytes_and_keeps_working() { + let gossip_addr: SocketAddr = "127.0.0.1:29611".parse().unwrap(); + let node = test_node(Vec::new()); + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_gossip(gossip_addr).await; + }); + tokio::time::sleep(StdDuration::from_millis(100)).await; + + let socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); + socket + .send_to(b"not a gossip message", gossip_addr) + .await + .unwrap(); + tokio::time::sleep(StdDuration::from_millis(100)).await; + + // The loop must have survived the garbage: a well-formed message + // sent right after should still get a real reply. + let digest = GossipMessage::Digest { + rumors: vec![DigestEntry { + domain: DomainName::new("after-garbage.binda").unwrap(), + issued_at_millis: u64::MAX, + }], + }; + socket + .send_to(&wire::encode(&digest).unwrap(), gossip_addr) + .await + .unwrap(); + let mut buf = vec![0u8; wire::MAX_DATAGRAM_BYTES]; + let (len, _) = tokio::time::timeout(StdDuration::from_secs(5), socket.recv_from(&mut buf)) + .await + .expect("node should still respond after ignoring garbage") + .unwrap(); + assert!(wire::decode::(&buf[..len]).is_ok()); + } + + // A dedicated "oversized Digest ignored by the running gossip loop" + // integration test isn't practical: encoding MAX_DIGEST_ENTRIES + 1 + // entries (even with minimal one-character domain names) costs far + // more than MAX_DATAGRAM_BYTES in JSON overhead alone, so + // wire::decode's own size cap always rejects such a message before + // is_well_formed ever gets to. That rejection (not is_well_formed) + // is exactly what gossip_recv_loop_ignores_garbage_bytes_and_keeps_working + // exercises. is_well_formed's own oversized-rejection logic is + // covered directly in binda_core::gossip's unit tests instead, where + // it doesn't have to survive a real wire encode/decode round trip. + + #[tokio::test(flavor = "multi_thread")] + async fn gossip_rate_limiter_drops_traffic_past_the_burst() { + let gossip_addr: SocketAddr = "127.0.0.1:29613".parse().unwrap(); + let node = test_node(Vec::new()); + seed_registration(&node, &DomainName::new("rate-limit-gossip.binda").unwrap()).await; + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_gossip(gossip_addr).await; + }); + tokio::time::sleep(StdDuration::from_millis(100)).await; + + let socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); + let request = GossipMessage::Request { + domains: vec![DomainName::new("rate-limit-gossip.binda").unwrap()], + challenge: ConformanceChallenge::random(0), + }; + let bytes = wire::encode(&request).unwrap(); + + // Fire well past the burst size in a tight loop from the same + // source address; some of these must be dropped outright. + for _ in 0..40 { + let _ = socket.send_to(&bytes, gossip_addr).await; + } + + let mut received = 0; + loop { + let mut buf = vec![0u8; wire::MAX_DATAGRAM_BYTES]; + match tokio::time::timeout(StdDuration::from_millis(300), socket.recv_from(&mut buf)) + .await + { + Ok(Ok(_)) => received += 1, + _ => break, + } + } + assert!( + received < 40, + "expected the rate limiter to drop some of 40 rapid requests, got {received} replies" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn resolver_rate_limiter_drops_traffic_past_the_burst() { + use binda_core::resolver::ResolveQuery; + + let resolver_addr: SocketAddr = "127.0.0.1:29614".parse().unwrap(); + let node = test_node(Vec::new()); + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_resolver(resolver_addr).await; + }); + tokio::time::sleep(StdDuration::from_millis(100)).await; + + let socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); + let query = ResolveQuery { + domain: DomainName::new("whatever.binda").unwrap(), + }; + let bytes = wire::encode(&query).unwrap(); + for _ in 0..40 { + let _ = socket.send_to(&bytes, resolver_addr).await; + } + + let mut received = 0; + loop { + let mut buf = vec![0u8; wire::MAX_DATAGRAM_BYTES]; + match tokio::time::timeout(StdDuration::from_millis(300), socket.recv_from(&mut buf)) + .await + { + Ok(Ok(_)) => received += 1, + _ => break, + } + } + assert!( + received < 40, + "expected the rate limiter to drop some of 40 rapid queries, got {received} replies" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn resolver_ignores_undecodable_bytes_and_keeps_working() { + use binda_core::resolver::{ResolveAnswer, ResolveQuery}; + + let resolver_addr: SocketAddr = "127.0.0.1:29615".parse().unwrap(); + let node = test_node(Vec::new()); + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_resolver(resolver_addr).await; + }); + tokio::time::sleep(StdDuration::from_millis(100)).await; + + let client_socket = std::net::UdpSocket::bind("127.0.0.1:0").unwrap(); + client_socket + .set_read_timeout(Some(StdDuration::from_secs(3))) + .unwrap(); + client_socket.send_to(b"garbage", resolver_addr).unwrap(); + std::thread::sleep(StdDuration::from_millis(100)); + + let query = ResolveQuery { + domain: DomainName::new("after-garbage.binda").unwrap(), + }; + client_socket + .send_to(&wire::encode(&query).unwrap(), resolver_addr) + .unwrap(); + let mut buf = vec![0u8; wire::MAX_DATAGRAM_BYTES]; + let (len, _) = client_socket.recv_from(&mut buf).unwrap(); + let answer: ResolveAnswer = wire::decode(&buf[..len]).unwrap(); + assert_eq!(answer.owner_client_key, None); + } + + #[tokio::test(flavor = "multi_thread")] + async fn client_api_rate_limiter_drops_traffic_past_the_burst() { + use binda_core::client_api::{probe_message, ClientRequest, ProbeBody, SignedEnvelope}; + use ed25519_dalek::Signer; + + let api_addr: SocketAddr = "127.0.0.1:29616".parse().unwrap(); + let node = test_node(Vec::new()); + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_client_api(api_addr).await; + }); + tokio::time::sleep(StdDuration::from_millis(100)).await; + + let socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); + let signing_key = SigningKey::generate(&mut OsRng); + let t = node.time.now_millis(); + let msg = probe_message(t); + let sig = signing_key.sign(&msg); + let req = ClientRequest::Probe(SignedEnvelope { + verifying_key: signing_key.verifying_key().to_bytes().to_vec(), + rdns: "host.example.net".into(), + timestamp_millis: t, + signature: sig.to_bytes().to_vec(), + body: ProbeBody, + }); + let bytes = wire::encode(&req).unwrap(); + for _ in 0..40 { + let _ = socket.send_to(&bytes, api_addr).await; + } + + let mut received = 0; + loop { + let mut buf = vec![0u8; wire::MAX_DATAGRAM_BYTES]; + match tokio::time::timeout(StdDuration::from_millis(300), socket.recv_from(&mut buf)) + .await + { + Ok(Ok(_)) => received += 1, + _ => break, + } + } + assert!( + received < 40, + "expected the rate limiter to drop some of 40 rapid requests, got {received} replies" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn client_api_ignores_undecodable_bytes_and_keeps_working() { + use binda_core::client_api::{ + probe_message, ClientRequest, ClientResponse, ProbeBody, SignedEnvelope, + }; + use ed25519_dalek::Signer; + + let api_addr: SocketAddr = "127.0.0.1:29617".parse().unwrap(); + let node = test_node(Vec::new()); + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_client_api(api_addr).await; + }); + tokio::time::sleep(StdDuration::from_millis(100)).await; + + let client_socket = std::net::UdpSocket::bind("127.0.0.1:0").unwrap(); + client_socket + .set_read_timeout(Some(StdDuration::from_secs(3))) + .unwrap(); + client_socket.send_to(b"garbage", api_addr).unwrap(); + std::thread::sleep(StdDuration::from_millis(100)); + + let signing_key = SigningKey::generate(&mut OsRng); + let t = node.time.now_millis(); + let msg = probe_message(t); + let sig = signing_key.sign(&msg); + let req = ClientRequest::Probe(SignedEnvelope { + verifying_key: signing_key.verifying_key().to_bytes().to_vec(), + rdns: "host.example.net".into(), + timestamp_millis: t, + signature: sig.to_bytes().to_vec(), + body: ProbeBody, + }); + client_socket + .send_to(&wire::encode(&req).unwrap(), api_addr) + .unwrap(); + let mut buf = vec![0u8; wire::MAX_DATAGRAM_BYTES]; + let (len, _) = client_socket.recv_from(&mut buf).unwrap(); + let response: ClientResponse = wire::decode(&buf[..len]).unwrap(); + assert_eq!(response, ClientResponse::ProbeAck); + } + + #[tokio::test(flavor = "multi_thread")] + async fn dns_rate_limiter_drops_traffic_past_the_burst() { + let dns_addr: SocketAddr = "127.0.0.1:29618".parse().unwrap(); + let node = test_node(Vec::new()); + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_dns(dns_addr).await; + }); + tokio::time::sleep(StdDuration::from_millis(100)).await; + + let socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); + let mut query = Vec::new(); + query.extend(0x1234u16.to_be_bytes()); + query.extend(0x0100u16.to_be_bytes()); + query.extend(1u16.to_be_bytes()); + query.extend([0u8; 6]); + query.extend(4u32.to_be_bytes()); + query.extend(b"test"); + query.extend(0u32.to_be_bytes()); + query.extend(1u16.to_be_bytes()); + query.extend(1u16.to_be_bytes()); + + for _ in 0..40 { + let _ = socket.send_to(&query, dns_addr).await; + } + + let mut received = 0; + loop { + let mut buf = vec![0u8; wire::MAX_DATAGRAM_BYTES]; + match tokio::time::timeout(StdDuration::from_millis(300), socket.recv_from(&mut buf)) + .await + { + Ok(Ok(_)) => received += 1, + _ => break, + } + } + assert!( + received < 40, + "expected the rate limiter to drop some of 40 rapid queries, got {received} replies" + ); + } + + #[tokio::test(flavor = "multi_thread")] + async fn native_dns_loop_returns_nxdomain_for_unknown_domain() { + let dns_addr: SocketAddr = "127.0.0.1:29619".parse().unwrap(); + let node = test_node(Vec::new()); + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_dns(dns_addr).await; + }); + tokio::time::sleep(StdDuration::from_millis(100)).await; + + let client_socket = std::net::UdpSocket::bind("127.0.0.1:0").unwrap(); + client_socket + .set_read_timeout(Some(StdDuration::from_secs(3))) + .unwrap(); + let mut query = Vec::new(); + query.extend(0x4321u16.to_be_bytes()); + query.extend(0x0100u16.to_be_bytes()); + query.extend(1u16.to_be_bytes()); + query.extend([0u8; 6]); + for label in "nobody-registered-this.binda".split('.') { + let bytes = label.as_bytes(); + query.extend((bytes.len() as u32).to_be_bytes()); + query.extend(bytes); + } + query.extend(0u32.to_be_bytes()); + query.extend(1u16.to_be_bytes()); + query.extend(1u16.to_be_bytes()); + + client_socket.send_to(&query, dns_addr).unwrap(); + let mut buf = vec![0u8; wire::MAX_DATAGRAM_BYTES]; + let (len, _) = client_socket.recv_from(&mut buf).unwrap(); + let response = &buf[..len]; + let flags = u16::from_be_bytes([response[2], response[3]]); + assert_eq!(flags & 0x000F, 3); // RCODE_NXDOMAIN + } } From bfb3790eb8bc06c6433497634c7a8c48fc18cb22 Mon Sep 17 00:00:00 2001 From: Jeremy Carter Date: Sat, 19 Sep 2026 12:18:05 -0400 Subject: [PATCH 3/3] Fix codecov/patch failure: exclude structurally-uncoverable spawn lines CI reported "97.36% of diff hit (target 98.21%)" on codecov/patch: 10 new lines in node.rs's test module were flagged uncovered. All 10 turned out to be the same pattern repeated across the new tests -- `tokio::spawn(async move { let _ = n.run_x(addr).await; })` -- where the spawned task is an infinite listener loop that, by design, never returns. Source-based coverage instrumentation can't mark that spawned block's own completion region as hit no matter how thoroughly the listener's actual behavior is exercised afterward (which every one of these tests does, via real socket round-trips) -- the task gets torn down by the test process exiting, not by returning, so that specific region is structurally unreachable regardless of test quality. The same phenomenon already affects the file's original, pre-existing spawn sites for this exact reason. Consolidates the pattern into four small helpers (spawn_gossip_task, spawn_resolver_task, spawn_client_api_task, spawn_dns_task) marked #[coverage(off)], the same nightly attribute already used to exclude main() for an analogous reason -- the thing being tested is the listener's behavior, not whether a background-task spawn call "book completes," which it never does. Reduces the diff's own uncovered line count from 10 to effectively 0, since the four helper definitions replace what used to be ten separate inline copies of an uncoverable line. Verified locally: workspace coverage 98.30% region / 98.96% line, both above the 98.21% target; all 38 binda-binary tests still pass (both --test-threads=1 and default parallelism). Co-Authored-By: Claude Sonnet 5 --- crates/binda/src/node.rs | 94 +++++++++++++++++++++++----------------- 1 file changed, 54 insertions(+), 40 deletions(-) diff --git a/crates/binda/src/node.rs b/crates/binda/src/node.rs index 9e6e76a..5d164c3 100644 --- a/crates/binda/src/node.rs +++ b/crates/binda/src/node.rs @@ -488,6 +488,50 @@ mod tests { } } + // These four helpers all share one shape: spawn a listener loop that, + // by design, never returns (it's a `loop { ... }` bound to a real UDP + // socket) and run it in the background for a test to send traffic + // at. Source-based coverage instrumentation can't mark a spawned + // async block's own "completed" region as hit when it's an infinite + // loop that gets torn down by the test process exiting rather than + // by returning — that's a property of *how the test drives the + // listener*, not of whether the listener's behavior is actually + // exercised (every test using these asserts on real responses over a + // real socket). `#[coverage(off)]` excludes exactly that + // structurally-uncoverable line, the same way `main` is excluded for + // an analogous reason. + #[coverage(off)] + fn spawn_gossip_task(node: &Node, addr: SocketAddr) { + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_gossip(addr).await; + }); + } + + #[coverage(off)] + fn spawn_resolver_task(node: &Node, addr: SocketAddr) { + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_resolver(addr).await; + }); + } + + #[coverage(off)] + fn spawn_client_api_task(node: &Node, addr: SocketAddr) { + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_client_api(addr).await; + }); + } + + #[coverage(off)] + fn spawn_dns_task(node: &Node, addr: SocketAddr) { + let n = node.clone(); + tokio::spawn(async move { + let _ = n.run_dns(addr).await; + }); + } + /// Receive datagrams on `socket` until one decodes as a /// `GossipMessage::Request`, ignoring anything else (e.g. the /// victim's own routine, unsolicited `Digest` pushes to this address, @@ -941,10 +985,7 @@ mod tests { let api_addr: SocketAddr = "127.0.0.1:29562".parse().unwrap(); let node = test_node_allowing_any_rdns(Vec::new()); - let n = node.clone(); - tokio::spawn(async move { - let _ = n.run_client_api(api_addr).await; - }); + spawn_client_api_task(&node, api_addr); tokio::time::sleep(StdDuration::from_millis(100)).await; let client_socket = std::net::UdpSocket::bind("127.0.0.1:0").unwrap(); @@ -1040,10 +1081,7 @@ mod tests { &DomainName::new("has-data-but-no-peers.binda").unwrap(), ) .await; - let n = node.clone(); - tokio::spawn(async move { - let _ = n.run_gossip(gossip_addr).await; - }); + spawn_gossip_task(&node, gossip_addr); // Long enough for several push-loop ticks; nothing to assert on // directly (there's no peer to send to), this just proves the // loop doesn't panic or block when self.peers.random_peer() @@ -1055,10 +1093,7 @@ mod tests { async fn gossip_recv_loop_ignores_garbage_bytes_and_keeps_working() { let gossip_addr: SocketAddr = "127.0.0.1:29611".parse().unwrap(); let node = test_node(Vec::new()); - let n = node.clone(); - tokio::spawn(async move { - let _ = n.run_gossip(gossip_addr).await; - }); + spawn_gossip_task(&node, gossip_addr); tokio::time::sleep(StdDuration::from_millis(100)).await; let socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); @@ -1104,10 +1139,7 @@ mod tests { let gossip_addr: SocketAddr = "127.0.0.1:29613".parse().unwrap(); let node = test_node(Vec::new()); seed_registration(&node, &DomainName::new("rate-limit-gossip.binda").unwrap()).await; - let n = node.clone(); - tokio::spawn(async move { - let _ = n.run_gossip(gossip_addr).await; - }); + spawn_gossip_task(&node, gossip_addr); tokio::time::sleep(StdDuration::from_millis(100)).await; let socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); @@ -1145,10 +1177,7 @@ mod tests { let resolver_addr: SocketAddr = "127.0.0.1:29614".parse().unwrap(); let node = test_node(Vec::new()); - let n = node.clone(); - tokio::spawn(async move { - let _ = n.run_resolver(resolver_addr).await; - }); + spawn_resolver_task(&node, resolver_addr); tokio::time::sleep(StdDuration::from_millis(100)).await; let socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); @@ -1182,10 +1211,7 @@ mod tests { let resolver_addr: SocketAddr = "127.0.0.1:29615".parse().unwrap(); let node = test_node(Vec::new()); - let n = node.clone(); - tokio::spawn(async move { - let _ = n.run_resolver(resolver_addr).await; - }); + spawn_resolver_task(&node, resolver_addr); tokio::time::sleep(StdDuration::from_millis(100)).await; let client_socket = std::net::UdpSocket::bind("127.0.0.1:0").unwrap(); @@ -1214,10 +1240,7 @@ mod tests { let api_addr: SocketAddr = "127.0.0.1:29616".parse().unwrap(); let node = test_node(Vec::new()); - let n = node.clone(); - tokio::spawn(async move { - let _ = n.run_client_api(api_addr).await; - }); + spawn_client_api_task(&node, api_addr); tokio::time::sleep(StdDuration::from_millis(100)).await; let socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); @@ -1262,10 +1285,7 @@ mod tests { let api_addr: SocketAddr = "127.0.0.1:29617".parse().unwrap(); let node = test_node(Vec::new()); - let n = node.clone(); - tokio::spawn(async move { - let _ = n.run_client_api(api_addr).await; - }); + spawn_client_api_task(&node, api_addr); tokio::time::sleep(StdDuration::from_millis(100)).await; let client_socket = std::net::UdpSocket::bind("127.0.0.1:0").unwrap(); @@ -1299,10 +1319,7 @@ mod tests { async fn dns_rate_limiter_drops_traffic_past_the_burst() { let dns_addr: SocketAddr = "127.0.0.1:29618".parse().unwrap(); let node = test_node(Vec::new()); - let n = node.clone(); - tokio::spawn(async move { - let _ = n.run_dns(dns_addr).await; - }); + spawn_dns_task(&node, dns_addr); tokio::time::sleep(StdDuration::from_millis(100)).await; let socket = UdpSocket::bind("127.0.0.1:0").await.unwrap(); @@ -1341,10 +1358,7 @@ mod tests { async fn native_dns_loop_returns_nxdomain_for_unknown_domain() { let dns_addr: SocketAddr = "127.0.0.1:29619".parse().unwrap(); let node = test_node(Vec::new()); - let n = node.clone(); - tokio::spawn(async move { - let _ = n.run_dns(dns_addr).await; - }); + spawn_dns_task(&node, dns_addr); tokio::time::sleep(StdDuration::from_millis(100)).await; let client_socket = std::net::UdpSocket::bind("127.0.0.1:0").unwrap();