From 2f2f0bb231ce9917358a232162970353b057ca05 Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Fri, 31 Jul 2026 08:03:55 -0600 Subject: [PATCH 1/2] dns: distinguish "this name does not resolve" from "this resolver failed" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unblocks the re-selection follow-up recorded in spark's ADR 0014. The proxyless transport wants to drop a strategy whose resolver has died, but could not: every resolve failure arrived as `io::Error::other`, so NXDOMAIN for a mistyped host was indistinguishable from a blackholed resolver. Acting on that signal would have discarded a working strategy — and forced a full verified search — every time a user visited a domain that does not exist, which is why the earlier attempt at self-healing had to be reverted. The information was always there, just flattened. `codec::parse_response` already returns `Rcode(3)` for NXDOMAIN and `validate_answers` already separates `Empty` from `Poisoned`; `map_err(io::Error::other)` threw the distinction away at the last step. Now each failure maps onto a meaningful `io::ErrorKind`: - **NotFound** — NXDOMAIN, or an answer with no usable address. The resolver did its job. - **InvalidData** — SERVFAIL/REFUSED, truncated, not-a-response, mismatched transaction ID, or bogon-only records. The resolver answered and the answer cannot be believed. Poisoned belongs here rather than with NotFound: bogons mean something is lying, which is very much about the resolver. - **InvalidInput** — an unencodable name. Our bug; nothing was ever sent. - transport kinds pass through untouched, which is the clearest case of all. `indicts_resolver(&io::Error)` is the predicate callers actually want — "should I stop trusting this resolver?" — so they need not learn the mapping. No signatures changed, so existing callers are unaffected; they simply get better-kinded errors. `Kind::System` is documented as the exception: `getaddrinfo` reports "no such host" and "the network is down" through the same opaque failure, so it is reported as indicting. That errs toward re-selecting, the safe direction — moving off the OS resolver is cheap and is the right move if the network really did change. Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace (flint-dns 33, three new covering both sides of the distinction), and cargo doc with zero warnings. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX --- crates/flint-dns/src/lib.rs | 109 +++++++++++++++++++++++++++++++++--- 1 file changed, 100 insertions(+), 9 deletions(-) diff --git a/crates/flint-dns/src/lib.rs b/crates/flint-dns/src/lib.rs index 1f9eae3..b14a327 100644 --- a/crates/flint-dns/src/lib.rs +++ b/crates/flint-dns/src/lib.rs @@ -70,6 +70,56 @@ const DEFAULT_WINDOW: usize = 16; /// instead of hanging on the slowest resolver. const ATTEMPT_TIMEOUT: Duration = Duration::from_secs(5); +/// Whether a resolve error indicts the **resolver** rather than the name. +/// +/// `false` means the resolver did its job and the name simply does not resolve — NXDOMAIN, or an +/// answer carrying no usable address. A caller must not read that as reason to distrust the resolver: +/// users mistype hosts and visit dead domains constantly, and discarding a working resolver every time +/// one does would throw away a good strategy for an entirely normal event. +/// +/// `true` means the resolver could not be reached, timed out, or answered in a way that cannot be +/// believed (SERVFAIL, a malformed or mismatched response, bogon-only records). Those *are* grounds to +/// stop using it and pick another. +/// +/// This distinction is the whole reason the resolve paths map failures onto meaningful +/// [`io::ErrorKind`]s instead of flattening everything to `Other` — without it a caller sees one +/// undifferentiated error and cannot tell a broken network from a typo. +/// +/// **[`Kind::System`] is the exception.** `getaddrinfo` reports "no such host" and "the network is +/// down" through the same opaque failure, so a system-resolver error is reported as indicting. That +/// errs toward re-selecting, which is the safe direction: moving off the OS resolver is cheap, and it +/// is the right move whenever the network really has changed. +pub fn indicts_resolver(err: &io::Error) -> bool { + err.kind() != io::ErrorKind::NotFound +} + +/// Map a wire/codec failure onto an [`io::Error`] whose kind carries the distinction +/// [`indicts_resolver`] depends on. +fn codec_err(e: codec::DnsError) -> io::Error { + let kind = match e { + // The resolver answered authoritatively: this name has no address. Not its fault. + codec::DnsError::Rcode(3) => io::ErrorKind::NotFound, + // A name we could not even encode is our bug, not the resolver's. + codec::DnsError::BadName => io::ErrorKind::InvalidInput, + // Everything else — SERVFAIL/REFUSED, truncated, not-a-response, a mismatched transaction ID — + // is the resolver failing to give a usable answer. + _ => io::ErrorKind::InvalidData, + }; + io::Error::new(kind, e) +} + +/// Map an answer-validation failure the same way. +fn validate_err(e: validate::ValidateError) -> io::Error { + let kind = match e { + // Answered, but with nothing usable — the name, not the resolver. + validate::ValidateError::Empty => io::ErrorKind::NotFound, + // Bogons only. The resolver answered and the answer is a lie, which is very much about the + // resolver (or whoever is speaking for it). + validate::ValidateError::Poisoned => io::ErrorKind::InvalidData, + }; + io::Error::new(kind, e) +} + /// Resolve `name`/`qtype` through a single `resolver`: reach it over whatever transport its /// [`Kind`] names, run the query, parse, and validate. Returns the validated public addresses, or an /// `io::Error` (which the smart-dialer funnels into the race's per-resolver failures). @@ -102,34 +152,34 @@ pub async fn resolve_one_with( ) -> io::Result> { let answers = match resolver.kind { Kind::Doh => { - let query = codec::build_query(name, qtype).map_err(io::Error::other)?; + let query = codec::build_query(name, qtype).map_err(codec_err)?; let stream = flint_dial::dial(&tls_strategy(resolver, policy)?).await?; let response = doh::query(stream, &resolver.host, &resolver.path, &query).await?; - codec::parse_response(&response).map_err(io::Error::other)? + codec::parse_response(&response).map_err(codec_err)? } Kind::Dot => { let id = random_id()?; - let query = codec::build_query_with_id(name, qtype, id).map_err(io::Error::other)?; + let query = codec::build_query_with_id(name, qtype, id).map_err(codec_err)?; let stream = flint_dial::dial(&tls_strategy(resolver, policy)?).await?; let response = plain::query_stream(stream, &query).await?; - codec::parse_response_with_id(&response, id).map_err(io::Error::other)? + codec::parse_response_with_id(&response, id).map_err(codec_err)? } Kind::Tcp => { let id = random_id()?; - let query = codec::build_query_with_id(name, qtype, id).map_err(io::Error::other)?; + let query = codec::build_query_with_id(name, qtype, id).map_err(codec_err)?; let stream = tokio::net::TcpStream::connect(resolver.target).await?; let response = plain::query_stream(stream, &query).await?; - codec::parse_response_with_id(&response, id).map_err(io::Error::other)? + codec::parse_response_with_id(&response, id).map_err(codec_err)? } Kind::Udp => { let id = random_id()?; - let query = codec::build_query_with_id(name, qtype, id).map_err(io::Error::other)?; + let query = codec::build_query_with_id(name, qtype, id).map_err(codec_err)?; let response = plain::query_udp(resolver.target, &query).await?; - codec::parse_response_with_id(&response, id).map_err(io::Error::other)? + codec::parse_response_with_id(&response, id).map_err(codec_err)? } Kind::System => system_lookup(name, qtype).await?, }; - validate::validate_answers(answers).map_err(io::Error::other) + validate::validate_answers(answers).map_err(validate_err) } /// The TLS strategy for a resolver whose kind is known to be TLS-based. @@ -291,6 +341,47 @@ pub async fn resolve_cached_with( mod tests { use super::*; + #[test] + fn a_name_that_does_not_resolve_does_not_indict_the_resolver() { + // The distinction that matters: a user mistyping a host must not cost a working resolver. + for e in [ + codec_err(codec::DnsError::Rcode(3)), // NXDOMAIN + validate_err(validate::ValidateError::Empty), // answered, no usable records + ] { + assert_eq!(e.kind(), io::ErrorKind::NotFound, "{e}"); + assert!(!indicts_resolver(&e), "{e} must not blame the resolver"); + } + } + + #[test] + fn a_resolver_that_misbehaves_is_indicted() { + for e in [ + codec_err(codec::DnsError::Rcode(2)), // SERVFAIL + codec_err(codec::DnsError::Truncated), + codec_err(codec::DnsError::NotAResponse), + codec_err(codec::DnsError::IdMismatch { got: 1, want: 2 }), + // Bogons only: the resolver answered, and the answer is a lie. + validate_err(validate::ValidateError::Poisoned), + ] { + assert!(indicts_resolver(&e), "{e} should blame the resolver"); + } + + // Transport failures are the clearest case of all — they never reach the codec. + for kind in [io::ErrorKind::TimedOut, io::ErrorKind::ConnectionRefused] { + assert!(indicts_resolver(&io::Error::new(kind, "unreachable"))); + } + } + + #[test] + fn an_unencodable_name_is_our_bug_not_a_resolver_failure() { + // Still "indicts" (it is not a NotFound), but the kind records who to blame: nothing was ever + // sent, so no resolver was involved. + assert_eq!( + codec_err(codec::DnsError::BadName).kind(), + io::ErrorKind::InvalidInput + ); + } + #[tokio::test] async fn resolve_on_an_empty_pool_fails() { // No network: an empty pool races nothing → AllFailed{0}. Proves resolve still funnels an From 43fb7a1cdf472d11a8285e7e4fed130950b5b083 Mon Sep 17 00:00:00 2001 From: Adam Fisk Date: Fri, 31 Jul 2026 09:50:44 -0600 Subject: [PATCH 2/2] dns: do not indict a resolver for our own bad input, and reject TC-truncated answers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catches (CodeRabbit + Copilot, PR #18). Both reviewers independently found the first, which is a logic bug of mine. **`indicts_resolver` blamed the resolver for a caller error.** `DnsError::BadName` maps to `InvalidInput` precisely because *we* could not encode the name and nothing was ever sent — the doc and the PR's own mapping table both say so — yet the predicate returned `true` for everything except `NotFound`. A caller would have discarded a perfectly good resolver over a name that never left the process, which is the same class of mistake as blaming it for a typo'd domain: exactly what this predicate exists to prevent. My own test made it easy to miss: it asserted the *kind* was `InvalidInput` without ever asking what `indicts_resolver` said about it. Now both are asserted. **The TC flag was never checked.** `parse_response` validated QR and RCODE but not TC, so a syntactically complete response the server had cut short would parse as a success and return whatever records happened to fit. That mattered little before; it matters now, because if truncation removed every address record the result would be "no addresses" → `NotFound` → "this name does not resolve" — and this PR makes callers *act* on that. The verdict would be exactly backwards: the resolver is the thing that failed to serve us, so it should be indicted, not excused. TC is now checked after the RCODE, so a truncated NXDOMAIN still reports the more specific NXDOMAIN, and maps to `InvalidData` (indicting). We do no TCP retry, so a resolver that truncates genuinely cannot answer the query and should lose to one that can. Two tests: that a TC answer is rejected rather than partially parsed (with the flag cleared again to prove TC is what rejected it, not the fixture), and that NXDOMAIN wins over TC. Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace (flint-dns 35, two more new), and cargo doc with zero warnings. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX --- crates/flint-dns/src/codec.rs | 47 ++++++++++++++++++++++++++++++++--- crates/flint-dns/src/lib.rs | 23 +++++++++++------ 2 files changed, 59 insertions(+), 11 deletions(-) diff --git a/crates/flint-dns/src/codec.rs b/crates/flint-dns/src/codec.rs index e8125a6..46f71b3 100644 --- a/crates/flint-dns/src/codec.rs +++ b/crates/flint-dns/src/codec.rs @@ -101,9 +101,10 @@ pub fn parse_response_with_id(buf: &[u8], expected_id: u16) -> Result Result, DnsError> { if rcode != 0 { return Err(DnsError::Rcode(rcode)); } + // TC: the answer did not fit and the server cut it short (RFC 1035 §4.1.1). Checked *after* the + // RCODE so a truncated NXDOMAIN still reports the more specific NXDOMAIN. + // + // Rejecting rather than parsing what arrived: a partial answer looks like a complete one, and if + // truncation happened to remove every address record we would report "this name has no address" + // — which callers now act on (see `flint_dns::indicts_resolver`) and would be exactly backwards, + // since the resolver is the thing that failed to serve us. We do no TCP-retry, so a resolver that + // truncates cannot answer this query and should lose to one that can. + if flags & 0x0200 != 0 { + return Err(DnsError::Truncated); + } let qdcount = u16::from_be_bytes([buf[4], buf[5]]) as usize; let ancount = u16::from_be_bytes([buf[6], buf[7]]) as usize; @@ -263,6 +275,35 @@ mod tests { assert_eq!(ips, vec!["93.184.216.34".parse::().unwrap()]); } + #[test] + fn a_truncated_answer_is_rejected_rather_than_parsed_partially() { + // A TC response is syntactically fine and carries *some* records, so without this check it + // would parse as a success and silently return a partial answer. + let mut resp = build_response( + "example.com", + TYPE_A, + 0, + &[(TYPE_A, vec![93, 184, 216, 34])], + ); + resp[2] |= 0x02; // set TC in the high flags byte + assert_eq!(parse_response(&resp), Err(DnsError::Truncated)); + + // Clearing it again parses normally — proving TC is what rejected it, not the fixture. + resp[2] &= !0x02; + assert_eq!( + parse_response(&resp).unwrap(), + vec!["93.184.216.34".parse::().unwrap()] + ); + } + + #[test] + fn a_truncated_nxdomain_still_reports_nxdomain() { + // The RCODE is the more specific fact, so it wins over truncation. + let mut resp = build_response("nope.example", TYPE_A, 3, &[]); + resp[2] |= 0x02; + assert_eq!(parse_response(&resp), Err(DnsError::Rcode(3))); + } + #[test] fn rejects_an_overlong_label() { let long = "a".repeat(64); diff --git a/crates/flint-dns/src/lib.rs b/crates/flint-dns/src/lib.rs index b14a327..ab7844d 100644 --- a/crates/flint-dns/src/lib.rs +++ b/crates/flint-dns/src/lib.rs @@ -90,7 +90,14 @@ const ATTEMPT_TIMEOUT: Duration = Duration::from_secs(5); /// errs toward re-selecting, which is the safe direction: moving off the OS resolver is cheap, and it /// is the right move whenever the network really has changed. pub fn indicts_resolver(err: &io::Error) -> bool { - err.kind() != io::ErrorKind::NotFound + !matches!( + err.kind(), + // The resolver answered and the name has no address. + io::ErrorKind::NotFound + // We could not even encode the query, so nothing was sent and no resolver was involved. + // Blaming one for our own bad input would churn selection over a caller error. + | io::ErrorKind::InvalidInput + ) } /// Map a wire/codec failure onto an [`io::Error`] whose kind carries the distinction @@ -373,13 +380,13 @@ mod tests { } #[test] - fn an_unencodable_name_is_our_bug_not_a_resolver_failure() { - // Still "indicts" (it is not a NotFound), but the kind records who to blame: nothing was ever - // sent, so no resolver was involved. - assert_eq!( - codec_err(codec::DnsError::BadName).kind(), - io::ErrorKind::InvalidInput - ); + fn an_unencodable_name_is_our_bug_and_indicts_nobody() { + // Nothing was ever sent, so no resolver was involved. Blaming one would discard a working + // strategy over a caller error — the same class of mistake as blaming it for a typo'd domain, + // which is the whole point of this predicate. + let e = codec_err(codec::DnsError::BadName); + assert_eq!(e.kind(), io::ErrorKind::InvalidInput); + assert!(!indicts_resolver(&e), "a caller-input error blames nobody"); } #[tokio::test]