Repository navigation
Add insecure demo flag so client_demo can succeed locally - #5
Merged
Merged
Conversation
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 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up to a question raised while trying the example client:
cargo run -p binda --example client_demoalways showsregister -> Error { ... }because its hardcodedrdnsclaim genuinely can't forward-confirm against wherever it's run — that's the FCrDNS check working as intended, not a bug, but it meant the showcased demo could never actually demonstrate a successful registration without real reverse-DNS infrastructure.binda_core::fcrdns::AllowAllVerifier: anRdnsVerifierthat accepts any claim unchecked. Heavily documented as demo/test-only, and it's never the default.Nodegainsnew_allowing_any_rdnsalongside the existing (still-default)new, and anrdns_verifierfield sorun_client_apiasks whichever verifier the node was actually built with, instead of hardcodingFcrdnsVerifier.--insecure-skip-rdns-verificationCLI flag on thebindabinary, which prints a loud warning to stderr when used.client_demo.rs's own doc comment to start the node with that flag, so the showcased walkthrough now actually completes end to end (Registered/RecordsSet/ populated resolves) instead of stopping at a correct-but-demo-defeating refusal.Notes for reviewers
Node::new(used whenever the flag is omitted) still requires real FCrDNS verification, matching every other test and the documented security model.register_via_client_api_succeeds_with_rdns_verification_disabled, alongside the existing refusal test, so both the "real check refuses a fake claim" and "insecure mode accepts it" paths are covered.--insecure-skip-rdns-verificationand the example client against it —register/set_records/both resolves all succeed now.🤖 Generated with Claude Code