[wip] Replace hickory with simpledns based DNS resolver#4036
Draft
dignifiedquire wants to merge 41 commits intomainfrom
Draft
[wip] Replace hickory with simpledns based DNS resolver#4036dignifiedquire wants to merge 41 commits intomainfrom
dignifiedquire wants to merge 41 commits intomainfrom
Conversation
Contributor
|
I don't remember the exact detalis, but there was something that hickory could do that simple-dns can't. @Frando ? |
dignifiedquire
force-pushed
the
refactor-hickory
branch
from
4f62d53 to
f6c6c09
Compare
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/4036/docs/iroh/ Last updated: 2026-04-15T10:51:51Z |
Recursive resolvers commonly return CNAME records when a queried name
is an alias. The previous code only looked for the exact queried record
type (A/AAAA/TXT) in the answer section, silently returning empty
results for any name behind a CNAME (common with CDNs, cloud LBs).
This adds two levels of CNAME following:
(a) In-response: resolve_cname_chain() walks CNAME records within a
single response packet to find the canonical name, then collects
records matching either the original or canonical name.
(b) Recursive: send_query_following_cnames() detects when a response
contains only a CNAME with no target records, and issues a new
query for the CNAME target. Limited to 8 hops to prevent loops.
Without EDNS(0), well-behaved DNS servers limit UDP responses to 512 bytes (RFC 1035). Iroh endpoint TXT records with multiple addresses can easily exceed this, forcing a TCP fallback round-trip on every endpoint discovery query. Add an OPT pseudo-record to all outgoing queries advertising 1232-byte UDP payload support (the recommended safe value per RFC 6891 and DNS flag day 2020). This avoids unnecessary TCP fallbacks while staying under common path MTU limits.
Previously, send_query tried nameservers sequentially with a 5-second per-nameserver timeout. Since the outer DNS_TIMEOUT is 3 seconds, only the first nameserver was ever reached. A single UDP packet loss meant immediate failure. Now: - All nameservers are queried in parallel via FuturesUnorderedBounded, with staggered starts (100ms between each) so the preferred nameserver gets a head start. - UDP queries retry once per nameserver (2 attempts total) before giving up, matching hickory-resolver's default attempts:2 behavior. - Per-nameserver timeout reduced to 2s so individual attempts complete quickly and don't block the overall query. - First successful response from any nameserver wins. Also fixes CNAME name-matching to gracefully handle responses without a question section (accept all records of the target type).
Use recv_from instead of recv to verify that the DNS response came from the expected nameserver. Without this check, a local network attacker could race a spoofed response before the real one arrives. The random 16-bit query ID provides some defense, but source address validation is standard defense-in-depth against cache poisoning.
DNS-over-TLS and DNS-over-HTTPS currently derive the TLS server name from the IP address. This works for providers with IP SANs in their certificates (Google, Cloudflare) but will fail for servers with hostname-only certificates. Document this as a known limitation.
TxtRecordData was changed from Box<[Box<[u8]>]> to Box<[String]>, which is lossy for non-UTF-8 TXT record content and breaks the public API. Restore the original bytes representation to preserve binary TXT record fidelity. Display still uses from_utf8_lossy for rendering. Keep From<Vec<String>> for convenience at construction sites.
The previous extract_txt_record_data used TXT::attributes() which returns a HashMap, losing ordering and deduplicating keys. This is destructive for iroh's endpoint records which publish multiple addr= entries as separate TXT records. Replace with String::try_from(txt) which preserves the raw concatenated content of each TXT record faithfully.
The resolv.conf parser previously only extracted nameserver lines, ignoring search and domain directives. This means search domain completion for short hostnames was silently broken. Parse both directives per resolv.conf(5) semantics: search and domain are mutually exclusive, last one wins. Introduce SystemDnsConfig struct to carry both nameservers and search domains. The resolver does not yet apply search domains to queries -- this commit just ensures the configuration is read and available.
Some setups (Docker, VPNs, custom resolvers) use non-standard DNS ports. Previously, entries like "nameserver 8.8.8.8:5353" would silently fail to parse as IpAddr and be skipped. Now try parsing as SocketAddr first (which supports port), falling back to IpAddr with the default port 53.
NXDOMAIN (domain doesn't exist) and SERVFAIL (server error) were lumped into the same ServerError variant. Add a dedicated NxDomain variant so callers can distinguish "this domain doesn't exist" from "DNS is broken" and skip retries for definitive NXDOMAIN responses.
dedup_by_key only removes consecutive duplicates, so if the same DNS server appears on non-adjacent network adapters, it would survive deduplication. Use a HashSet to properly deduplicate regardless of ordering.
Document two known limitations: 1. No negative caching: NXDOMAIN/NODATA responses are never cached, which can cause thundering herd under high concurrency for non-existent domains. This matches the old hickory-resolver config. 2. No TCP/TLS connection reuse: each query opens a fresh connection, adding a full TLS handshake per DoT query. Only affects non-default DoT/DoH configurations.
The field is intentionally parsed from resolv.conf but not yet consumed by the resolver. Add allow(dead_code) with a comment explaining why.
Implement resolv.conf search domain semantics per resolv.conf(5): - Short hostnames (fewer dots than ndots, default 1) try each search domain suffix first, then the bare name. - Names with enough dots try the bare name first, then search domains. - FQDNs (trailing dot) bypass search domains entirely. - NXDOMAIN responses advance to the next candidate name rather than failing immediately. This makes the custom resolver behave like system resolvers for short hostnames, which matters for Docker, Kubernetes, and corporate network setups where search domains are commonly configured.
- Collapse nested if/if-let into if-let chains (resolve_cname_chain) - Use .then() and .ok()? for cname_target - Use is_ok_and for is_truncated - Use matches! for record type checking - Use bytes() instead of chars() for dot counting - Extract with_timeout helper to deduplicate timeout wrapping - Simplify stagger logging in send_query - Use elapsed() instead of manual Instant arithmetic in cache - Remove dead system_nameservers() wrapper - Remove stale #[allow(dead_code)] on search_domains
- with_timeout: use `?` on Elapsed to get DnsError::Timeout directly instead of wrapping in a fake io::Error -> DnsError::Transport - InvalidPacket: use `?` on SimpleDnsError directly (from_sources generates the From impl) - UDP source validation: use `?` on io::Error instead of e!() wrapper - TLS config missing: use io::Error::other for brevity - Remove unused n0_error::e import from transport.rs
- send_query_following_cnames now takes String by value instead of &str, avoiding a clone since callers (search_names) already produce owned Strings. - Removed redundant trace! logs that referenced name after move (use host instead, which is the user-facing name anyway). - Document remaining allocations in DnsCache (lookup key String, record clone on cache hit, insert key) -- these are inherent to the LruCache<(String, QueryType), _> design and negligible vs network I/O.
Replace LruCache<(String, QueryType), _> with LruCache<u64, _> using pre-hashed keys. This removes a String allocation on every cache get() call, which was the only per-lookup heap allocation on the cache-hit fast path. Hash collisions are benign for a cache (worst case: stale entry returned, triggers a re-query). With 64-bit hashes and 512 max entries the collision probability is negligible (~3e-14 per lookup).
- Run rustfmt on test code - Collapse nested if/if-let into let-chains (clippy::collapsible_if) - Remove needless Ok(x??) wrapping (clippy::needless_question_mark) - Move NxDomain variant after InvalidResponse to preserve discriminant ordering (semver check) - cfg-gate tls_client_config field and access behind with_crypto_provider to fix builds without a crypto provider
Move the Mutex from SimpleDnsResolver into DnsCache itself, so the resolver calls self.cache.get()/insert()/clear() directly without external locking. This simplifies all call sites. Extract a generic lookup() method that captures the shared pattern across lookup_ipv4/ipv6/txt: cache check, search name iteration, query with CNAME following, parse, and cache insert. The per-type differences (QueryType, TYPE, parse function, cache variant) are passed as function pointers.
- Validate QR bit and question section in check_response per RFC 5452 - Check raw header bytes for truncation instead of parsing full packet (truncated packets may fail to parse, missing TCP fallback) - Check HTTP status code in DoH responses before parsing body - Guard against TCP/TLS length prefix overflow with u16::try_from - Clamp cache TTL to 1 day to prevent malicious permanent entries Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Parse `options ndots:N` from /etc/resolv.conf and use it for search domain expansion instead of hardcoding ndots=1. This is important for Kubernetes environments where ndots:5 is the default. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
A SERVFAIL for one search name doesn't mean other names will also fail. Continue to the next search domain instead of returning immediately. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Read the DNS suffix search list from the Windows registry via ipconfig::computer::get_search_list(). Falls back to the primary domain (get_domain()) if no search list is configured. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace three near-identical parse functions with a generic parse_response that takes an extraction closure. Each public function is now a thin wrapper providing the rdata pattern match. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
11 tasks
Return 127.0.0.1 / ::1 immediately for localhost and *.localhost names without hitting the network, matching hickory-resolver behavior. Fixes test_https_clients_and_server on macOS where localhost is only in /etc/hosts, not resolvable via DNS. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Log nameservers and search domains at debug (not just count) - Use %name for unquoted string output in traces - Add trace for NXDOMAIN/SERVFAIL with the queried name - Add debug log on hard lookup failures - Remove redundant per-transport trace (addr already in UDP trace) - Fix bug: NXDOMAIN from parse() was propagated via ? instead of being caught by the search-domain retry match arm - Add resolve_success_and_nxdomain test for log inspection - show resolved addresses in logs, restore cache hit trace
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
3 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.
No description provided.