dns: resolver-transport axis (DoT/TCP/UDP/system), orthogonal to wire shaping - #13
Merged
Conversation
… to wire shaping Realizes the "diversity across ... AND transports" spread that design §6 already called for but only implemented for DoH. `pool::Kind` now names the DNS axis of a proxyless strategy: Doh, Dot, Tcp, Udp, System. Keeps that axis independent of the wire-shaping axis (§5) instead of folding shaping into the resolver. `Resolver::strategy_with(wire)` and `resolve_one_shaped` compose the two, so a DoH lookup can itself be carried over a fragmented, jittered ClientHello — the same shaping vocabulary used for a destination dial, aimed at the DNS dial. That makes the strategy space a product (`resolver × wire`) a search can enumerate, rather than a fixed list of hand-written combinations. flint-dial re-exports WirePlan/RecordFragment/ SegmentSplit/DelaySpec so a consumer can express that without taking a direct flint-shaping dependency. Trust across the axis is not uniform, and the code is explicit about it: - DoH/DoT are encrypted; only they are shapeable (no ClientHello otherwise). - Plaintext TCP/UDP and the system resolver are poisonable. `validate` rejects bogons but cannot prove an answer is *correct*, so these kinds are excluded from `default_pool()` and are sound only inside a search that verifies the answer end-to-end via a valid-certificate TLS handshake to the resolved address. - Where plaintext is used, the query carries a CSPRNG transaction ID that is verified on return (`build_query_with_id` / `parse_response_with_id`), and UDP `connect`s its socket on a random ephemeral port so the kernel drops datagrams from any other source. That is the standard bar against off-path injection, which censors including the GFW perform by blasting forged answers without ever seeing the query. DoH keeps ID 0 per RFC 8484 §4.1, since its own framing binds the response. Stream framing (2-byte length prefix, RFC 1035 §4.2.2) is shared by DoT and plaintext TCP, which differ only in how the stream was obtained; both cap the response so a hostile length prefix cannot drive a large allocation. POOL_MAGIC FRP1 → FRP2: signed pools are postcard, which is not self-describing, so `Resolver`'s new field shifts every following byte. An FRP1 client handed an FRP2 pool would mis-parse the whole list rather than merely miss `kind` — hence a magic bump, which is exactly what the constant documents itself as being for. Also fixes a pre-existing clippy failure (useless_borrows_in_formatting under clippy 1.97) in flint-fronted's meek_live test, which was blocking the workspace `-D warnings` gate. Gate: cargo fmt --all --check, cargo clippy --workspace --all-targets -D warnings, and cargo test --workspace all clean; flint-dns 25 tests (7 new, covering stream framing round-trip, zero/oversized length prefixes, UDP round trip, off-path-source rejection, and transaction-ID set/verify). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughDNS resolution now supports DoH, DoT, TCP, UDP, and system transports. Resolver construction composes transport selection with optional TLS wire shaping, while plaintext queries use randomized transaction IDs and validated responses. ChangesDNS transport resolution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Resolver
participant Transport
participant Codec
participant Validator
Caller->>Resolver: resolve_one_shaped(name, qtype, WirePlan)
Resolver->>Transport: send query using selected Kind
Transport-->>Resolver: return DNS response
Resolver->>Codec: parse and verify transaction ID
Codec-->>Resolver: return addresses
Resolver->>Validator: validate answers
Validator-->>Caller: return validated addresses
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/design.md (1)
250-258: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winScope transaction-ID and UDP protections to plaintext socket transports.
Kind::Systemhas no application-controlled socket, so it cannot attach/check a DNS transaction ID or use UDPconnectsource filtering. Clarify that those mitigations apply only to plaintext TCP/UDP, while system resolution relies on the documented end-to-end validation path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/design.md` around lines 250 - 258, Update the trust explanation around the plaintext resolver discussion to scope CSPRNG transaction-ID checks and UDP socket connect/source filtering explicitly to plaintext TCP/UDP transports. Clarify that Kind::System cannot use these socket-level protections and is trusted only through the documented end-to-end TLS certificate validation path.crates/flint-dns/src/signed.rs (1)
18-26: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a regression test for rejecting FRP1 artifacts.
The schema bump is correct, but the tests only exercise FRP2. Add coverage that a valid legacy-magic artifact is rejected, so future verifier changes cannot accidentally reintroduce cross-version decoding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/flint-dns/src/signed.rs` around lines 18 - 26, Add a regression test near the existing pool artifact verification tests that constructs an otherwise valid artifact using the legacy FRP1 magic and asserts verification rejects it. Reuse the existing artifact-building and verifier APIs rather than altering POOL_MAGIC or production decoding behavior, and keep FRP2 acceptance coverage unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/flint-dns/src/pool.rs`:
- Around line 24-30: The DNS strategy documentation around the Doh and Dot
descriptions incorrectly claims encrypted resolvers can never have answers
poisoned. Update the relevant doc comment to acknowledge that unverified TLS
permits on-path interception and answer injection when certificate or hostname
verification is disabled, without changing the documented behavior of Tcp, Udp,
or default_pool.
---
Nitpick comments:
In `@crates/flint-dns/src/signed.rs`:
- Around line 18-26: Add a regression test near the existing pool artifact
verification tests that constructs an otherwise valid artifact using the legacy
FRP1 magic and asserts verification rejects it. Reuse the existing
artifact-building and verifier APIs rather than altering POOL_MAGIC or
production decoding behavior, and keep FRP2 acceptance coverage unchanged.
In `@docs/design.md`:
- Around line 250-258: Update the trust explanation around the plaintext
resolver discussion to scope CSPRNG transaction-ID checks and UDP socket
connect/source filtering explicitly to plaintext TCP/UDP transports. Clarify
that Kind::System cannot use these socket-level protections and is trusted only
through the documented end-to-end TLS certificate validation path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 236eb015-aea5-4b07-8b4e-ef46aa8c21a8
📒 Files selected for processing (9)
crates/flint-dial/src/lib.rscrates/flint-dns/Cargo.tomlcrates/flint-dns/src/codec.rscrates/flint-dns/src/lib.rscrates/flint-dns/src/plain.rscrates/flint-dns/src/pool.rscrates/flint-dns/src/signed.rscrates/flint-fronted/tests/meek_live.rsdocs/design.md
Contributor
Author
|
Confirmed with @afisk: no signed pool artifact is deployed yet, so the FRP1 → FRP2 magic bump is a no-op in practice. Resolving that pre-merge check — no coordination needed. |
…is unauthenticated Review catch (CodeRabbit, PR #13), verified and correct. The docs added in this PR asserted that DoH/DoT answers "can only be blocked, never poisoned". That is false as implemented: `Resolver::strategy` builds on `BootstrapStrategy::boring_chrome`, which sets `CertVerification::None`, and flint-tls's connector enables `SslVerifyMode::PEER` + `set_verify_hostname` only in the `Roots` branch. So neither the chain nor the hostname is checked, and an on-path attacker can complete the handshake with any certificate and hand back forged answers over a properly encrypted channel. `validate` would catch a clumsy sentinel like 10.10.34.x but not a plausible attacker-chosen address. Corrects the claim everywhere it appeared (`Kind`, `Kind::is_encrypted`, `default_pool`, the crate docs, design §6.1): encrypted kinds resist passive reading and *off-path* forgery, not an active on-path MITM, until a caller supplies `CertVerification::Roots` via `with_verification`. No behavior change here. The underlying gap is pre-existing — it predates this PR and affects the existing DoH path — and closing it alters a live bootstrap path, so it is recorded as an explicit §11 open item rather than folded into a PR about adding the resolver axis. That item also captures the part that needs a decision: verifying the real host is straightforward for `sni == host` plain/CDN-edge entries, but a fronted entry presents a cert matching the camouflage SNI rather than the DoH `:authority`, so those need a distinct policy or exclusion. flint-fronted already dials with `Roots`, so the pattern exists. Also notes in §6.1 that this undercuts the "raw-IP works because the cert carries IP SANs" rationale, which only means anything when the cert is verified. Gate: cargo fmt --all --check, cargo clippy --workspace --all-targets -D warnings, cargo test --workspace (25 flint-dns tests), and cargo doc -p flint-dns all clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
There was a problem hiding this comment.
Pull request overview
This PR expands flint-dns from DoH-only into a resolver model with an explicit resolver-transport axis (Kind: DoH/DoT/TCP/UDP/system) that composes orthogonally with wire shaping (WirePlan). It also hardens plaintext DNS paths against classic off-path response injection by adding randomized transaction IDs and connected UDP sockets.
Changes:
- Introduces
pool::Kindand updatesResolverto represent DoH/DoT/TCP/UDP/system transports, plus composition viaresolve_one_shaped. - Adds plaintext DNS query implementations (DoT/TCP stream framing + UDP) and transaction-ID set/verify APIs in the codec.
- Bumps signed pool magic (
FRP1→FRP2), updates crate exports/deps, and fixes a workspace clippy failure inflint-frontedtests.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/design.md | Documents the new resolver-transport axis and its orthogonality to wire shaping. |
| crates/flint-fronted/tests/meek_live.rs | Fixes a clippy warning (useless_borrows_in_formatting). |
| crates/flint-dns/src/signed.rs | Bumps pool update magic to FRP2 and updates tests to use new constructors. |
| crates/flint-dns/src/pool.rs | Adds Kind, extends Resolver, and introduces typed constructors and strategy composition. |
| crates/flint-dns/src/plain.rs | Implements DoT/TCP stream framing and UDP query support with safety caps and tests. |
| crates/flint-dns/src/lib.rs | Adds resolve_one_shaped, system resolver path, transaction ID generation, and re-exports. |
| crates/flint-dns/src/codec.rs | Adds transaction ID support (build_query_with_id, parse_response_with_id) and tests. |
| crates/flint-dns/Cargo.toml | Updates crate description and dependencies (adds ring, enables tokio net). |
| crates/flint-dial/src/lib.rs | Re-exports shaping types so consumers can express BootstrapStrategy::wire without a direct flint-shaping dependency. |
Comments suppressed due to low confidence (1)
crates/flint-dns/src/pool.rs:108
Resolver::strategy_with()similarly ignoresself.kindand always builds a TLSBootstrapStrategy. With the newKindvariants, this can lead consumers to accidentally apply TLS/wire shaping to plaintext/system resolvers. Consider restricting this API to TLS-based kinds (returnOption/Resultor add a newtls_strategy_with), and/or add an explicit guard (debug_assert!(self.kind.is_shapeable())) plus docs stating it is only meaningful forDoh/Dot.
/// The bootstrap-dial strategy with opening-handshake shaping `wire` composed onto it.
///
/// This is the composition seam between the two axes: the resolver says *where and how* to reach
/// DNS, `wire` says *how to shape the opening handshake* getting there. That is what makes
/// "DoH lookups carried over a fragmented, jittered ClientHello" expressible — the same shaping
/// vocabulary used for a destination dial, applied to the DNS dial itself.
///
/// Shaping is only meaningful for the TLS-based kinds ([`Kind::is_shapeable`]); for plaintext or
/// system resolvers there is no ClientHello and `wire` is ignored by the query path.
pub fn strategy_with(&self, wire: WirePlan) -> BootstrapStrategy {
BootstrapStrategy::boring_chrome(self.target, self.sni.clone()).with_wire(wire)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review catch (Copilot, PR #13). `Resolver::strategy()` returned a TLS `BootstrapStrategy` for every kind, including the ones that have no TLS dial at all: plaintext TCP/UDP, and `System`, whose `target` is an unused placeholder — so it cheerfully described a TLS dial to 0.0.0.0:0. Internally the `resolve_one_shaped` match never took that path, but both methods are `pub`, so nothing stopped a consumer from dialing TLS at a UDP resolver on port 53. Renamed to `tls_strategy` / `tls_strategy_with` and returns `Option`, `None` for any kind where `!kind.is_shapeable()`. The name now says what it builds, and the `Option` keeps the invalid combination out of the type at the call site instead of trusting each caller to check `kind` first — the same reasoning behind the typed constructors added earlier in this PR. The two internal call sites are the `Doh`/`Dot` match arms, which have already established the kind is TLS-based, so `None` there would mean `is_shapeable` and the dispatch disagree — a bug rather than a runtime condition. A small helper converts it to an `io::Error` naming the resolver and kind, so that impossible case cannot become a panic (no `unwrap` on a production path). Adds `only_tls_kinds_have_a_dial_strategy`: DoH/DoT yield a strategy carrying the right SNI, shaping composes through `tls_strategy_with`, and Tcp/Udp/System all yield `None`. Also fixes the `querying host path` doc typo Copilot flagged, which rendered as one identifier. Gate: fmt, clippy --workspace --all-targets -D warnings, and cargo test --workspace all clean; flint-dns now 26 tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
Contributor
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 44 minutes. |
This was referenced Jul 29, 2026
myleshorton
added a commit
that referenced
this pull request
Jul 29, 2026
…es the destination (#14) New `flint-proxyless` crate — the Rust counterpart to the Outline SDK smart dialer's proxyless path (`x/smart`, findDNS × findTLS): reach the real destination with no proxy and no exit hop by finding a `(resolver, wire)` pairing the local network does not block. Chunk 2 of the proxyless port; #13 made the two axes orthogonal, this consumes them as a cartesian product. The certificate is the oracle. A censor can block a connection, and can answer a plaintext query with anything it likes, but cannot produce a valid certificate for the destination — so success is not "DNS answered" or "TCP connected" but a TLS handshake to the resolved address completing against a verified chain and hostname. That is what admits the poisonable resolver kinds here even though `default_pool()` excludes them: a forged answer cannot survive the handshake, so a poisoned resolver loses the race instead of silently winning it. Search behaviour: candidates enumerate wire-major (every resolver against the no-op plan first, then each shaping plan), so plain dials are ruled out before paying for shaping. `race_windowed` keeps 4 probes in flight — each is a real lookup plus a real handshake, and firing dozens at once would be slow, wasteful, and conspicuous exactly where someone is watching. A candidate must reach ALL test domains, not any, so one accidentally-open path cannot vouch for a hostile network; an empty domain list is rejected rather than trivially succeeding. A cached winner is re-verified rather than trusted, so a strategy the censor has since caught self-heals into a fresh search. The verification step is injectable, so the search logic — enumeration order, the all-domains rule, concurrency bounds, cache hit/miss/self-heal — is unit-tested with no network and no TLS engine. That matters because dialing needs the `boring` feature, which the default CI job does not enable. Scope, stated in the crate docs and design §6.2: no exit hop means traffic leaves the user's own address for the real destination. This defeats blocking, not observation, and does nothing against IP blackholing — a reachability tool, not an anonymity one. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
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
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.
Adds the resolver-transport axis to
flint-dns— DoT, plaintext TCP/UDP, and the system resolver alongside DoH — and keeps it orthogonal to wire shaping so the two compose.This realizes the "diversity across providers AND ASNs AND jurisdictions AND transports" spread that design §6 already called for but only implemented for DoH. It is chunk 1 of porting outline-sdk's proxyless circumvention (
x/smart:findDNS × findTLS) to Rust.Why orthogonal axes
A resolver contributes addressing — where and by what protocol an answer is obtained. A
WirePlancontributes opening-handshake shape — record fragmentation, segment splitting, inter-segment jitter. Composing them at the seam rather than folding shaping into the resolver is what lets a DoH lookup itself be carried over a fragmented, jittered ClientHello: the same shaping vocabulary used for a destination dial, aimed at the DNS dial.It also makes the strategy space a product (
resolver × wire) that a search can enumerate mechanically, instead of a hand-written list of combinations. Folding shaping in would mean every new shaping idea needs N new resolver entries.flowchart LR subgraph DNSAXIS["DNS axis — flint_dns::pool::Kind"] direction TB A1["Doh — RFC 8484 over h2"] A2["Dot — RFC 7858"] A3["Tcp / Udp — plaintext, RFC 1035"] A4["System — OS resolver"] end subgraph WIREAXIS["Shaping axis — flint_shaping::WirePlan"] direction TB B1["RecordFragment — SniStraddle / Chunks / Offsets"] B2["SegmentSplit — SniBoundary / Explicit"] B3["DelaySpec — Fixed / Jitter"] end DNSAXIS -->|"where + which protocol"| S["BootstrapStrategy<br/>Resolver::strategy_with(wire)"] WIREAXIS -->|"how the opening handshake looks"| S S --> R["resolve_one_shaped<br/>flint-dns/src/lib.rs"] R --> V["validate → public addresses"] A3 -.->|"poisonable: excluded from default_pool<br/>CSPRNG txn ID + connected UDP socket"| Rflint-dialnow re-exportsWirePlan/RecordFragment/SegmentSplit/DelaySpec, becauseBootstrapStrategy::wireis part of its public API — a consumer could not express shaping without those types and should not need a directflint-shapingdependency to get them.Trust is not uniform across the axis, and the code says so
default_pool()DohDotTcp/UdpSystemvalidaterejects bogons but cannot prove an answer is correct — a censor returning a plausible wrong address passes it. So plaintext kinds are excluded fromdefault_pool()and are sound only inside a search that verifies the answer end-to-end by completing a TLS handshake with a valid certificate against the resolved address (chunk 2). They earn a place in the strategy space at all because some networks filter encrypted DNS while leaving plaintext queries to an unfiltered resolver alone.Off-path injection fix
codec::build_queryhardcoded transaction ID0andparse_responsenever checked it. That is correct for DoH — RFC 8484 §4.1 recommends ID 0 since DoH framing binds the response — but on plaintext UDP it is the classic off-path injection hole, which censors including the GFW exploit by blasting forged answers without ever seeing the query.build_query_with_id+parse_response_with_id: plaintext paths draw aringCSPRNG ID and verify it on return. DoH keeps ID 0 and its RFC rationale.query_udpbinds a random ephemeral port andconnects the socket, so the kernel drops datagrams from any source other than the resolver.This is the standard bar against off-path forgery. It is not protection against an on-path censor — hence the
default_pool()exclusion above.POOL_MAGICFRP1 → FRP2 (needs a sanity check)Signed pools are postcard: positional, not self-describing, no serde field defaults.
Resolver's newkindfield shifts every following byte, so an FRP1 client handed an FRP2 pool would mis-parse the whole list, not merely miss the field. The constant documents itself as existing for exactly this, so it is bumped.Please confirm no FRP1 signed pool artifact is already deployed —
pool.rssays signed updates are "layered on later", so this should be a no-op, but it is worth an explicit check before merge.Framing
DoT and plaintext TCP share DNS stream framing (2-byte big-endian length prefix, RFC 1035 §4.2.2) and differ only in how the stream was obtained — DoT over
flint_dial::dial(so it composes with shaping), TCP over a bare socket. Both cap the response so a hostile length prefix cannot drive a large allocation, and reject a zero-length prefix.Verification
Matches CI exactly:
cargo fmt --all -- --check— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test --workspace— clean;flint-dnsat 25 tests, 7 new: stream framing round-trip, zero and oversized length prefixes, UDP round-trip, off-path-source rejection, and transaction-ID set/verifyAlso fixes a pre-existing clippy failure in
flint-fronted/tests/meek_live.rs(useless_borrows_in_formatting, new in clippy 1.97) that was already breaking the workspace-D warningsgate and is unrelated to this change.Not in scope
The search harness itself (
Strategy{dns, wire},find()racing theresolver × wireproduct with test-domain verification, persistable winner cache),disorder/TTL shaping, the sparkproxylesstransport, andKindling::with_proxylessfor config fetches — each a following chunk.🤖 Generated with Claude Code
https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
Summary by CodeRabbit
New Features
Bug Fixes
Documentation