dns: authenticate the resolver dial — verify chain and hostname (closes §11 gap) - #15
Conversation
📝 WalkthroughWalkthroughChangesThe resolver stack replaces wire-only shaping with Authenticated resolver dialing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Space
participant Strategy
participant Proxyless
participant flint_dns
participant TLS
Space->>Strategy: Build DialPolicy with wire and roots
Proxyless->>flint_dns: resolve_one_with(..., policy)
flint_dns->>TLS: Create strategy with policy roots and resolver SNI
TLS-->>Proxyless: Verified DNS connection
Proxyless->>TLS: Dial using policy
TLS-->>Proxyless: Verified resolver connection
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 |
There was a problem hiding this comment.
Pull request overview
This PR closes a security gap in flint-dns where DoH/DoT resolver dials were encrypted but not authenticated, by explicitly enabling certificate chain + hostname verification and making trust configuration an explicit input to resolution/dial paths.
Changes:
- Introduces
DialPolicy { wire, roots }to bundle handshake shaping with TLS trust anchors, and threads it throughflint-dnsresolution APIs. - Updates resolver TLS strategies to always use
CertVerification::Roots(verifying the SNI as the identity), never inheritingboring_chrome’sCertVerification::None. - Updates
flint-proxylessto carry trust roots at theSpacelevel and ensure cached strategies never supply trust anchors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/design.md | Updates the design doc to reflect authenticated resolver dials, SNI identity verification, and mobile root handling. |
| crates/flint-proxyless/src/lib.rs | Switches proxyless strategies from WirePlan to DialPolicy and adds per-space trust roots. |
| crates/flint-proxyless/src/cache.rs | Ensures cached entries only select a candidate; trust anchors always come from the current Space. |
| crates/flint-dns/src/pool.rs | Adds DialPolicy and makes resolver TLS strategies explicitly verified with CertVerification::Roots. |
| crates/flint-dns/src/lib.rs | Renames/shifts resolution APIs to accept DialPolicy and adds *_with variants for explicit policy control. |
Comments suppressed due to low confidence (1)
docs/design.md:398
(spark's core/src/ca_roots.rs)looks like an in-repo path, but there is no such file here. Consider clarifying that this is a reference to the external spark embedder so the design doc remains self-contained.
- **Trust anchors on mobile.** Empty `DialPolicy::roots` falls back to OpenSSL's default paths, which
are empty on Android/iOS unless the embedder points `SSL_CERT_FILE`/`SSL_CERT_DIR` at a bundled set
(spark's `core/src/ca_roots.rs`). That coupling is implicit: flint cannot tell whether an embedder
has done it, and gets an indistinguishable "unable to get local issuer certificate" either way. Open:
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…reference Review catches (Copilot, PR #15), both verified and both correct. `DialPolicy`'s docs described `roots` as verifying "the resolver's certificate", but this same PR widened the type: flint-proxyless applies it to destination dials too, so the resolver-specific wording was already stale when written. Reworded to apply to whichever TLS peer the policy is used against, and noted explicitly that the name reflects the dial it was introduced for rather than a restriction — both legs of a proxyless connection are shaped and anchored from one value. design.md pointed at `core/src/ca_roots.rs` twice as if it lived here. It does not — flint has no `core/` directory at all; that file belongs to the spark embedder in a separate repo. Both references now say so, so a reader does not go hunting for a path this repository never contained. Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace, and cargo doc all clean. Documentation only, no behavior change. 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
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
crates/flint-dns/src/lib.rs:149
tls_strategy_withnow always buildsCertVerification::Roots { hostname: resolver.sni }, and flint-tls rejects verified connections with an empty hostname (io::ErrorKind::InvalidInput). SinceResolver::doh/dotdon’t preventsni == "", this currently turns into a late failure insidedialwith a less actionable error. Consider failing fast here with a clearer error whenresolver.sniis empty.
fn tls_strategy(
resolver: &Resolver,
policy: &DialPolicy,
) -> io::Result<flint_dial::BootstrapStrategy> {
resolver.tls_strategy_with(policy).ok_or_else(|| {
io::Error::other(format!(
"resolver {} has kind {:?}, which has no TLS dial strategy",
resolver.name, resolver.kind
))
})
…r inherit None
Closes the design §11 gap. `Resolver::tls_strategy_with` inherited
`CertVerification::None` from `BootstrapStrategy::boring_chrome`, so every DoH and
DoT resolver dial was encrypted but **unauthenticated**: an on-path attacker could
terminate the handshake with any certificate and hand back forged answers, which is
exactly the poisoning DoH exists to prevent. Bogon validation would catch a clumsy
sentinel like 10.10.34.x but never a plausible attacker-chosen address. flint-fronted
already dialed with `Roots`; DNS was the outlier.
Now every TLS resolver dial applies `CertVerification::Roots` explicitly — chain and
hostname — rather than inheriting a default. That inheritance *was* the bug, so the
application is deliberate rather than relying on a better default upstream.
**The identity is the SNI**, which resolves the open question uniformly instead of
needing a per-addressing-form policy. The SNI is by definition the name whose
certificate the server presents:
- raw-IP and CDN-edge entries have `sni == host`, so this is "verify the resolver
hostname" — and it works despite dialing a bare IP because the identity checked is
the hostname, not the address. That also makes the "raw-IP works because the cert
carries IP SANs" rationale unnecessary for our path: we never verify against the IP.
- a fronted entry (`sni != host`) authenticates the front whose certificate actually
arrives; verifying the DoH `:authority` there would simply fail. The real resolver
is addressed by `:authority` inside the verified channel — the trust model fronting
always relies on.
New `DialPolicy { wire, roots }` bundles shaping and trust, so trust is something a
caller states rather than inherits, and the resolve signatures stay stable as knobs
are added. Empty `roots` means the platform default store.
**Public signatures are unchanged.** `resolve`/`resolve_cached`/`resolve_one` keep
their shapes and gain verification for free; `*_with` variants accept a policy for
callers that need to pin anchors. That matters because flint-fronted resolves its
fronts through `resolve_cached` and spark calls `flint_dns::resolve` directly — both
get the fix without a code change. `resolve_one_shaped` becomes `resolve_one_with`
(one day old, single caller, unmerged).
Mobile is why roots are pinnable at all: empty roots fall back to
`X509_STORE_set_default_paths()`, which is empty on Android/iOS unless the embedder
points `SSL_CERT_FILE` at a bundled set. spark already does this in
`core/src/ca_roots.rs` for fronted TLS, so this change rides that same mechanism
rather than needing new platform work — recorded in §11 along with the open question
of whether flint should self-check anchors at init so a misconfigured embedder fails
loudly instead of looking like a blocked network.
flint-proxyless picks up the same treatment: its dials were already verified, but its
roots were hardcoded empty. It now carries a `DialPolicy` per strategy and
deployment-wide `Space::roots`, so both legs of a proxyless connection are
authenticated — noting the legs verify *different* identities (resolver SNI vs
destination host). A cached entry never carries trust anchors of its own: trust comes
from the current space, since a persisted (or disk-loaded) anchor set is not something
a cache should get a vote on.
Tests: verification is present on every TLS kind with hostname == SNI and empty
anchors meaning the default store; pinned anchors are carried through; a fronted entry
verifies the front rather than the `:authority`; proxyless trust comes from the space.
flint-dns 28 tests, flint-proxyless 13.
Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace
(flint-fronted's resolver path included), cargo doc with zero warnings for both
crates, and cargo check --features boring — the configuration where verification
actually executes — all clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
…reference Review catches (Copilot, PR #15), both verified and both correct. `DialPolicy`'s docs described `roots` as verifying "the resolver's certificate", but this same PR widened the type: flint-proxyless applies it to destination dials too, so the resolver-specific wording was already stale when written. Reworded to apply to whichever TLS peer the policy is used against, and noted explicitly that the name reflects the dial it was introduced for rather than a restriction — both legs of a proxyless connection are shaped and anchored from one value. design.md pointed at `core/src/ca_roots.rs` twice as if it lived here. It does not — flint has no `core/` directory at all; that file belongs to the spark embedder in a separate repo. Both references now say so, so a reader does not go hunting for a path this repository never contained. Gate: fmt, clippy --workspace --all-targets -D warnings, cargo test --workspace, and cargo doc all clean. Documentation only, no behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
f28f7dd to
f168b13
Compare
|
@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 17 minutes. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
docs/design.md:265
- In §6.1, the earlier “orthogonal to shaping” paragraph still references the pre-rename APIs (
Resolver::strategy_with(wire)/resolve_one_shaped). Since this PR introducesDialPolicyand renames the composition seam toResolver::tls_strategy_with(policy)/resolve_one_with, that paragraph should be updated to match so the design doc doesn’t point readers at non-existent functions.
**The resolver dial is authenticated, and the identity is the SNI.** Every TLS resolver dial uses
`CertVerification::Roots` — chain *and* hostname — never `None`. Encryption without authentication
would have bought very little: an active on-path MITM could terminate the handshake with any
certificate and return whatever answers it liked, and bogon validation catches a clumsy sentinel but
not a plausible attacker-chosen address. (`boring_chrome` still defaults to `CertVerification::None`,
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design.md (1)
241-249: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale API names adjacent to the updated section.
This paragraph still says
Resolver::strategy_with(wire)/resolve_one_shaped, but the actual current API (per pool.rs/lib.rs in this same PR) isResolver::tls_strategy_with(policy)/resolve_one_with. It sits immediately before the newly-added §6.1 paragraphs that correctly use the current names, so the section is now internally inconsistent.📝 Suggested fix
-inter-segment jitter). `Resolver::strategy_with(wire)` / `resolve_one_shaped` compose the two, so a +inter-segment jitter). `Resolver::tls_strategy_with(policy)` / `resolve_one_with` compose the two, so a🤖 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 241 - 249, Update the API references in the paragraph describing resolver and wire-shaping composition: replace Resolver::strategy_with(wire) with Resolver::tls_strategy_with(policy) and resolve_one_shaped with resolve_one_with. Keep the surrounding explanation of orthogonal resolver and wire strategies unchanged.
🧹 Nitpick comments (1)
crates/flint-dns/src/pool.rs (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
BootstrapStrategy+CertVerification::Rootsconstruction across crates. Both sites hand-build the same "shape withpolicy.wire, verify withCertVerification::Rootssourced frompolicy.roots" pattern, differing only in which identity string is verified. A sharedDialPolicyconstructor would centralize this security-critical pattern instead of keeping it duplicated in two crates.
crates/flint-dns/src/pool.rs#L183-195: extract aDialPolicy::strategy(&self, target: SocketAddr, identity: impl Into<String>) -> BootstrapStrategyhelper that buildsboring_chrome(...).with_wire(self.wire.clone()).with_verification(CertVerification::Roots{ roots_pem: self.roots.clone(), hostname }), and havetls_strategy_withcall it withself.sni.clone().crates/flint-proxyless/src/lib.rs#L379-386: replaceverified()'s body with a call to the sameDialPolicy::strategy(target, hostname)helper.🤖 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/pool.rs` at line 1, Add a shared DialPolicy::strategy method that accepts target and identity, constructs BootstrapStrategy using boring_chrome, self.wire, and CertVerification::Roots from self.roots, then update tls_strategy_with to call it with self.sni.clone(). Replace flint-proxyless verified() construction with DialPolicy::strategy(target, hostname), preserving each caller’s identity value.
🤖 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.
Outside diff comments:
In `@docs/design.md`:
- Around line 241-249: Update the API references in the paragraph describing
resolver and wire-shaping composition: replace Resolver::strategy_with(wire)
with Resolver::tls_strategy_with(policy) and resolve_one_shaped with
resolve_one_with. Keep the surrounding explanation of orthogonal resolver and
wire strategies unchanged.
---
Nitpick comments:
In `@crates/flint-dns/src/pool.rs`:
- Line 1: Add a shared DialPolicy::strategy method that accepts target and
identity, constructs BootstrapStrategy using boring_chrome, self.wire, and
CertVerification::Roots from self.roots, then update tls_strategy_with to call
it with self.sni.clone(). Replace flint-proxyless verified() construction with
DialPolicy::strategy(target, hostname), preserving each caller’s identity value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5004b5a4-9fd7-4e80-b198-5d252d864f1a
📒 Files selected for processing (5)
crates/flint-dns/src/lib.rscrates/flint-dns/src/pool.rscrates/flint-proxyless/src/cache.rscrates/flint-proxyless/src/lib.rsdocs/design.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design.md (1)
241-249: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale API references left over from this PR's refactor.
This paragraph (in the same §6.1 subsection updated at lines 261-285) still says
Resolver::strategy_with(wire)/resolve_one_shapedcompose the DNS and shaping axes.resolve_one_shapedwas removed by this PR's refactor ofcrates/flint-dns/src/lib.rs, and the actual method isResolver::tls_strategy_with(viaDialPolicy), notstrategy_with. Worth fixing alongside the rest of this section's update.📝 Proposed fix
It is deliberately **independent of the wire-shaping axis** (§5). A resolver contributes addressing; a `WirePlan` contributes opening-handshake shape (record fragmentation, segment splitting, -inter-segment jitter). `Resolver::strategy_with(wire)` / `resolve_one_shaped` compose the two, so a +inter-segment jitter). `Resolver::tls_strategy_with(policy)` / `resolve_one_with` 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. Keeping them orthogonal is what makes🤖 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 241 - 249, Update the §6.1 paragraph to remove the stale `resolve_one_shaped` and `Resolver::strategy_with(wire)` references, replacing them with the current `Resolver::tls_strategy_with`/`DialPolicy` composition API. Preserve the description of orthogonal resolver and wire shaping axes and the existing limitations for shapeable TLS kinds, plaintext DNS, and the system resolver.
🤖 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.
Outside diff comments:
In `@docs/design.md`:
- Around line 241-249: Update the §6.1 paragraph to remove the stale
`resolve_one_shaped` and `Resolver::strategy_with(wire)` references, replacing
them with the current `Resolver::tls_strategy_with`/`DialPolicy` composition
API. Preserve the description of orthogonal resolver and wire shaping axes and
the existing limitations for shapeable TLS kinds, plaintext DNS, and the system
resolver.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 211427cf-3372-4458-82dc-f8836231fe70
📒 Files selected for processing (5)
crates/flint-dns/src/lib.rscrates/flint-dns/src/pool.rscrates/flint-proxyless/src/cache.rscrates/flint-proxyless/src/lib.rsdocs/design.md
Closes the design §11 gap that surfaced in review on #13: every DoH and DoT resolver dial was encrypted but not authenticated.
The bug
Resolver::tls_strategy_withbuilt onBootstrapStrategy::boring_chrome, which defaults toCertVerification::None— andflint-tls's connector only enablesSslVerifyMode::PEER+set_verify_hostnamein theRootsbranch. So neither the chain nor the hostname was ever checked.An on-path attacker could therefore terminate the handshake with any certificate and hand back forged answers over a perfectly encrypted channel — precisely the poisoning DoH exists to prevent.
validatewould catch a clumsy sentinel like Iran's10.10.34.x, but never a plausible attacker-chosen address.flint-frontedalready dialed withRoots; DNS was the outlier.Verification is now applied explicitly rather than inherited. That inheritance was the bug, so relying on a better upstream default would leave the same trap for the next constructor.
The identity is the SNI
#13 left open "which identity to verify per addressing form". It resolves uniformly: the SNI is by definition the name whose certificate the server presents.
snivshostTwo consequences worth calling out:
:authoritywould simply fail; the front serves its own certificate. Authenticating the front and then addressing the real resolver via:authorityinside the verified channel is the trust model fronting always relies on.Verification failure hard-fails that resolver's attempt, which the pool race already treats as one candidate losing — so no separate down-ranking was needed.
Public signatures are unchanged
New
DialPolicy { wire, roots }bundles shaping and trust, so trust is something a caller states rather than inherits, and the resolve signatures stay stable as knobs accumulate.resolve/resolve_cached/resolve_onekeep their shapes and gain verification for free;*_withvariants take a policy for callers that need to pin anchors. That matters concretely:resolve_cached(lib.rs:1288)flint_dns::resolvedirectly (core/src/bootstrap/mod.rs:80)Both get the fix with no code change. (
resolve_one_shaped→resolve_one_withis the one rename — one day old, single caller, unmerged.)Mobile is why roots are pinnable
Empty
rootsfalls back toX509_STORE_set_default_paths()— OpenSSL's compile-time paths plusSSL_CERT_FILE/SSL_CERT_DIR. Desktop has a working store; Android and iOS do not, since they keep trust roots where those paths cannot see them.spark already solved this in
core/src/ca_roots.rs, writing the bundled Mozilla set and pointingSSL_CERT_FILEat it — added for fronted TLS, for exactly this reason. So this change rides an existing mechanism rather than needing new platform work, which is the main reason it is low-risk to turn on.I recorded the residual coupling as a new §11 item: flint cannot tell whether an embedder has done that setup, and a missing anchor set looks identical to a blocked network (
unable to get local issuer certificate). Worth considering a cheap startup self-check so a misconfigured embedder fails loudly at init instead of masquerading as censorship.flint-proxyless gets the same treatment
Its dials were already verified, but its roots were hardcoded empty — the same latent mobile problem. It now carries a
DialPolicyper strategy plus deployment-wideSpace::roots, so both legs of a proxyless connection are authenticated. The legs verify different identities: the resolver leg checks the resolver's SNI, the destination leg checks the destination host.A cached entry never carries trust anchors of its own — trust comes from the current
Space. A persisted (or disk-loaded) anchor set is not something a cache should get a vote on.Verification
cargo fmt --all -- --check— cleancargo clippy --workspace --all-targets -- -D warnings— cleancargo test --workspace— clean, flint-dns 28 / flint-proxyless 13, including flint-fronted's resolver pathcargo doc— zero warnings on both cratescargo check -p flint-dns -p flint-proxyless --features boring— clean; this is the configuration where verification actually executesNew tests: every TLS kind verifies with
hostname == sniand empty anchors meaning the default store (neverNone); pinned anchors are carried through; a fronted entry verifies the front rather than the:authority; proxyless trust comes from the space, not the cache.What this does not cover
No live assertion that each pool entry's certificate actually validates — CI has no egress, and the CDN-edge entries were "verified live" while verification was off, which proved the handshake completed and DoH answered, not that the chain was valid for
cloudflare-dns.com. The pool races, so a few entries failing verification degrades rather than breaks, and an entry that was serving an invalid chain should now fail. Worth one live run against a real network before relying on it in the field.🤖 Generated with Claude Code
https://claude.ai/code/session_01BufGsK81otiqihvrZdfoJX
Summary by CodeRabbit
Security
New Features
Documentation