feat(moq-native): unified client TLS verification + quiche backend support#1902
Conversation
Wire the quiche QUIC backend up to the same TLS verification options the quinn backend already honors, so it can reach relays without disabling verification: - `--tls-fingerprint`: pin the leaf certificate by SHA-256 (the native serverCertificateHashes equivalent), bypassing the CA chain. - `--tls-root` / `--tls-system-roots`: verify against explicit and/or system roots instead of only the (empty) BoringSSL default store. - `http://` scheme: fetch `/certificate.sha256` over an insecure request and pin it, replacing the previous hard `FingerprintUnsupported` error. Adds `tls::Client::root_certs()` / `fingerprints()` helpers (shared resolution logic for non-rustls backends) and pulls `reqwest` into the `quiche` feature for the fingerprint fetch. Depends on unreleased web-transport-quiche APIs (`with_server_certificate_hashes` / `with_root_certificates`, plus the live `Session::stats()` and Linux client GSO landing in the web-transport repo). CI will not compile until a web-transport release ships those and the dependency is bumped. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Picks up the published quiche backend APIs the cert-verification wiring depends on (server certificate hashes / root certificates, live stats, Linux client GSO). Unblocks compiling the `quiche` feature against crates.io instead of a local checkout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 21 minutes and 37 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThe quiche backend now supports 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches✨ Simplify code
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: 2
🧹 Nitpick comments (1)
rs/moq-native/src/quiche.rs (1)
24-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the new public error variants.
These variants are part of a public enum, so each new public variant needs a short doc comment. As per coding guidelines, “Public API symbols must be documented.”
🤖 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 `@rs/moq-native/src/quiche.rs` around lines 24 - 34, The public error variants in the quiche error enum are missing API documentation. Add short doc comments to each newly exposed variant in the error enum (FetchFingerprint, ReadFingerprint, InvalidFingerprint, and FingerprintLength) so the public enum complies with the documentation guideline; keep the comments brief and place them directly above each variant definition.Source: Coding guidelines
🤖 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 `@rs/moq-native/src/quiche.rs`:
- Around line 236-239: The fingerprint fetch in quiche’s `fetch_fingerprint`
path is decoding response bodies even when the HTTP request failed, which can
turn 404/500 responses into misleading decode or length errors. Update the
`reqwest::get(...).await` flow to call `error_for_status()` on the response
before reading `text`, so non-success `/certificate.sha256` responses are
surfaced as `Error::FetchFingerprint` failures instead of being passed to
`hex::decode` and `try_into`.
- Around line 138-147: The pin handling in quiche.rs allows the insecure HTTP
bootstrap fingerprint to be added even when explicit fingerprints are already
configured, which weakens certificate pinning. Update the logic in the
fingerprint setup around the URL handling so fetch_fingerprint is only called
when self.fingerprints is empty, and ensure the insecure bootstrap path is
skipped whenever verification is disabled. Apply the same fix to the other
matching URL/bootstrap block referenced by the comment so both paths preserve
explicit pins first.
---
Nitpick comments:
In `@rs/moq-native/src/quiche.rs`:
- Around line 24-34: The public error variants in the quiche error enum are
missing API documentation. Add short doc comments to each newly exposed variant
in the error enum (FetchFingerprint, ReadFingerprint, InvalidFingerprint, and
FingerprintLength) so the public enum complies with the documentation guideline;
keep the comments brief and place them directly above each variant definition.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a596c41-bf2d-4ab6-a1b0-7fd637fe9ad4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
rs/moq-native/Cargo.tomlrs/moq-native/src/quiche.rsrs/moq-native/src/tls.rs
…recated) Re-add the variant removed when the http:// scheme switched to fetching and pinning the fingerprint, marked #[deprecated] instead. Removing a variant from this public enum was breaking; keeping it deprecated makes the change additive so it can target main. It is never constructed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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 (2)
rs/moq-native/src/quiche.rs (2)
17-17: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMake the new public API surface extensible and documented.
This PR adds public error variants and public fields, but
Errorremains exhaustive and the new members lack doc comments. Add#[non_exhaustive]and document the new public members. Also rewrite the deprecated variant doc in present tense.As per coding guidelines, “Public API symbols must be documented” and “Public APIs must be marked
#[non_exhaustive]to allow for future extension without breaking changes.”Proposed fix
+#[non_exhaustive] pub enum Error { @@ - /// No longer returned: the `http://` scheme now fetches and pins the - /// certificate fingerprint instead of failing. + /// Deprecated. The `http://` scheme fetches and pins the certificate fingerprint. #[deprecated(note = "fingerprint verification over http:// is now supported; this is never returned")] #[error("fingerprint verification (http:// scheme) is not supported with the quiche backend")] FingerprintUnsupported, + /// The certificate fingerprint request failed. #[error("failed to fetch certificate fingerprint")] FetchFingerprint(#[source] reqwest::Error), + /// The certificate fingerprint response body could not be read. #[error("failed to read certificate fingerprint")] ReadFingerprint(#[source] reqwest::Error), + /// The certificate fingerprint was not valid hex. #[error("invalid certificate fingerprint")] InvalidFingerprint(#[source] hex::FromHexError), + /// The certificate fingerprint was not a SHA-256 digest. #[error("certificate fingerprint must be 32 bytes (SHA-256), got {0}")] FingerprintLength(usize), @@ + /// Configured certificate fingerprints trusted by this client. pub fingerprints: Vec<[u8; 32]>, @@ + /// Root certificates trusted by this client. pub roots: Vec<CertificateDer<'static>>,Also applies to: 24-40, 106-108
🤖 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 `@rs/moq-native/src/quiche.rs` at line 17, The public Error API in quiche.rs is currently exhaustive and its new public variants/fields are undocumented, so update the Error enum to be #[non_exhaustive] and add doc comments to every newly exposed member. Also revise the deprecated variant’s doc comment to present tense, and make sure the public fields and variants introduced around Error are documented consistently so the API remains extensible and compliant with the public API guidelines.Source: Coding guidelines
42-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInclude
httpin the invalid scheme error.
connectnow acceptshttp://as a bootstrap scheme, so this user-facing error should list it too.Proposed fix
- #[error("url scheme must be 'https', 'moqt', or 'moql'")] + #[error("url scheme must be 'http', 'https', 'moqt', or 'moql'")] InvalidScheme,🤖 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 `@rs/moq-native/src/quiche.rs` around lines 42 - 43, Update the user-facing invalid-scheme message in the Quiche-related error enum so it matches the schemes accepted by connect; the InvalidScheme variant currently omits http even though connect now allows http:// as a bootstrap scheme. Adjust the error text in the enum definition and any related scheme validation in connect to list http alongside https, moqt, and moql.
🤖 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 `@rs/moq-native/src/quiche.rs`:
- Line 17: The public Error API in quiche.rs is currently exhaustive and its new
public variants/fields are undocumented, so update the Error enum to be
#[non_exhaustive] and add doc comments to every newly exposed member. Also
revise the deprecated variant’s doc comment to present tense, and make sure the
public fields and variants introduced around Error are documented consistently
so the API remains extensible and compliant with the public API guidelines.
- Around line 42-43: Update the user-facing invalid-scheme message in the
Quiche-related error enum so it matches the schemes accepted by connect; the
InvalidScheme variant currently omits http even though connect now allows
http:// as a bootstrap scheme. Adjust the error text in the enum definition and
any related scheme validation in connect to list http alongside https, moqt, and
moql.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf7b80fb-ab9f-47fb-a320-0cc7901a7612
📒 Files selected for processing (1)
rs/moq-native/src/quiche.rs
Centralize the server-verification policy in `tls::Client::verification()`, returning a shared `Verification` enum (Disabled / Fingerprints / Roots) consumed by both the rustls backends (quinn/noq via `build()`) and quiche. This removes the duplicated root/fingerprint resolution the quiche backend had and makes all three agree on precedence and flag validity. Fixes two issues: - Fingerprint pinning + custom/system roots was silently resolved to "fingerprint only" (roots dropped). It's now a hard error (`FingerprintWithRoots`): pinning bypasses the CA chain, so combining it with roots is a misconfiguration. Applies to quinn/noq/quiche alike. - The system-roots default is now computed in one place: on by default, dropped once a custom root is given unless `--tls-system-roots` re-enables it; fingerprint mode never loads them. Full additive verification (accept if pinned OR chains to a root) was considered but not adopted: it's clean in rustls (a composite verifier) but unsafe in BoringSSL, where `set_custom_verify_callback` replaces the standard verifier and would require hand-rolling chain + hostname checks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kixelated
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rs/moq-native/src/tls.rs (1)
19-52: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMark the public
Errorenum as non-exhaustive.This enum is being extended with
FingerprintWithRoots; add#[non_exhaustive]so future error variants do not break downstream exhaustive matches.+#[non_exhaustive] pub enum Error {As per coding guidelines, “Public APIs must be marked
#[non_exhaustive]to allow for future extension without breaking changes.”🤖 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 `@rs/moq-native/src/tls.rs` around lines 19 - 52, Mark the public Error enum in tls.rs as non-exhaustive by adding the non_exhaustive attribute to the enum definition. This enum is part of the public API and now includes FingerprintWithRoots, so update Error itself to prevent downstream exhaustive match breakage when future variants are added. Keep the existing variant definitions and error messages unchanged.Source: Coding guidelines
🤖 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 `@rs/moq-native/src/quiche.rs`:
- Around line 111-115: QuicheClient::new() is eagerly calling
config.tls.verification()? and failing with NoRoots before connect() has a
chance to handle the http:// bootstrap pin flow. Adjust the initialization so
root validation is deferred until after connect() can detect and apply a fetched
/certificate.sha256 pin, or otherwise allow QuicheClient::new() to succeed
without resolved roots when HTTP pin bootstrap is expected. Use the existing
QuicheClient::new, connect, and config.tls.verification logic to keep the
pin-based connection path usable even in environments without system or custom
roots.
---
Outside diff comments:
In `@rs/moq-native/src/tls.rs`:
- Around line 19-52: Mark the public Error enum in tls.rs as non-exhaustive by
adding the non_exhaustive attribute to the enum definition. This enum is part of
the public API and now includes FingerprintWithRoots, so update Error itself to
prevent downstream exhaustive match breakage when future variants are added.
Keep the existing variant definitions and error messages unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dd4a290d-d290-469b-99d9-f66288d87afa
📒 Files selected for processing (2)
rs/moq-native/src/quiche.rsrs/moq-native/src/tls.rs
| pub fn new(config: &ClientConfig) -> Result<Self> { | ||
| if !config.tls.root.is_empty() { | ||
| tracing::warn!("--tls-root is not supported with the quiche backend; system roots will be used"); | ||
| } | ||
|
|
||
| Ok(Self { | ||
| bind: config.bind, | ||
| disable_verify: config.tls.disable_verify.unwrap_or_default(), | ||
| verification: config.tls.verification()?, | ||
| max_streams: config.max_streams.unwrap_or(crate::DEFAULT_MAX_STREAMS), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Don’t resolve roots before knowing whether HTTP bootstrap will supply a pin.
QuicheClient::new() calls config.tls.verification()?, which can return NoRoots before connect() sees an http:// URL and fetches /certificate.sha256. That makes the new HTTP pin bootstrap unusable in environments with no system/custom roots, even though the fetched pin should bypass CA roots for that connection.
🤖 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 `@rs/moq-native/src/quiche.rs` around lines 111 - 115, QuicheClient::new() is
eagerly calling config.tls.verification()? and failing with NoRoots before
connect() has a chance to handle the http:// bootstrap pin flow. Adjust the
initialization so root validation is deferred until after connect() can detect
and apply a fetched /certificate.sha256 pin, or otherwise allow
QuicheClient::new() to succeed without resolved roots when HTTP pin bootstrap is
expected. Use the existing QuicheClient::new, connect, and
config.tls.verification logic to keep the pin-based connection path usable even
in environments without system or custom roots.
There was a problem hiding this comment.
Valid, and it applies to all three backends: client.rs calls config.tls.build() (→ verification()) eagerly for every backend, so the NoRoots error fires before any connect() sees an http:// URL. This is fail-closed (it errors rather than connecting insecurely) and only bites the niche combination --tls-system-roots=false + no roots + http:// bootstrap. Fixing it properly means deferring root resolution to per-connection (where the scheme is known) across all backends and the websocket path — a real refactor. Tracking as a follow-up rather than expanding this PR; flagging to the maintainer for the call. (Written by Claude)
There was a problem hiding this comment.
@kixelated, thanks for the thorough analysis. The fail-closed behavior means there's no security regression here — it's purely a usability gap for that niche configuration combination.
Would you like me to open a GitHub issue to track the follow-up refactor? I can capture the relevant context: deferring verification() / root resolution to per-connection time across all three backends (quiche, noq, quinn) and the WebSocket path so that http:// bootstrap remains usable even when no system or custom roots are configured.
🧠 Learnings used
Learnt from: kixelated
Repo: moq-dev/moq PR: 1663
File: rs/moq-relay/src/auth.rs:3036-3038
Timestamp: 2026-06-09T15:16:38.658Z
Learning: In Rust, a `matches!(value, SomeEnum::Variant(_))`-style check where every inner sub-pattern is a wildcard (`_`) does not bind/move any data from `value` (no variables are captured). Therefore, if `value` is still usable after the `matches!` call, suggestions to change the argument to `&value` are likely incorrect/false positives. Only recommend borrowing if the pattern actually binds data (e.g., `Variant(v)`), or if the surrounding code would otherwise cause a move.
Learnt from: kixelated
Repo: moq-dev/moq PR: 1901
File: rs/moq-native/src/tls.rs:116-121
Timestamp: 2026-06-24T15:58:02.567Z
Learning: When reviewing Rust diffs that change a `#[arg(...)]` attribute (e.g., Clap/derive-style CLI args), don’t treat an `env` value rename as a breaking change based solely on reformatting. If a single-line attribute like `#[arg(id = "x", long = "x", env = "MOQ_CLIENT_TLS_ROOT")]` becomes a multiline block, the diff may show the `env = "..."` only on the `+` side. Before flagging a breaking `env`-var rename, compare both the pre- and post-change sides of the diff and confirm the `env` value is identical; only flag if the actual `env` string differs.
The `http://` scheme fetches a certificate fingerprint over plaintext and
pins it. Previously this ran unconditionally and could override stronger,
explicitly-configured verification:
- quiche appended the fetched hash to any configured `--tls-fingerprint`
pins ("trusted pin OR attacker-controlled pin").
- quinn/noq replaced the verifier outright, silently dropping an explicit
pin (or re-enabling verification under `--tls-disable-verify`).
An attacker who controls the plaintext fetch could thus substitute their
own certificate. Gate the bootstrap on a shared
`tls::Client::allows_http_bootstrap()` (true only when no explicit
`--tls-fingerprint` is set and verification isn't disabled), so the fetch
is honored only when there's nothing stronger to undermine. With CA roots
(the default) `http://` still pins a self-signed relay as before.
Also: quiche's fetch now calls `error_for_status()` so a 404/500 reports
as a request failure instead of a misleading decode error (matching
quinn/noq).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…verification() rustdoc denies public docs linking to private items; verification() is pub(crate). Reword the build() doc to plain text. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Summary
Brings the quiche QUIC backend up to TLS-verification parity with quinn/noq, and centralizes the verification policy so all three backends agree.
Verification (shared)
tls::Client::verification()is the single source of truth, returning aVerificationenum (Disabled/Fingerprints/Roots) consumed by the rustls backends (quinn/noq viabuild()) and quiche directly:--tls-fingerprint→ pins the leaf by SHA-256 (nativeserverCertificateHashes), bypassing the CA chain.--tls-root/--tls-system-roots→ verify against system and/or custom roots. System roots are on by default, dropped once a custom root is given unless re-enabled; the two sets are additive.http://scheme → fetch/certificate.sha256and pin it.Behavior fix (quinn/noq/quiche): fingerprint + roots used to silently resolve to "fingerprint only" (roots dropped). It's now a hard error (
FingerprintWithRoots) — pinning bypasses the CA chain, so combining it with roots is a misconfiguration. The quiche backend's previously-duplicated root/fingerprint resolution is gone; it now calls the sharedverification().quiche backend
Wires the above into quiche (
with_server_certificate_hashes/with_root_certificates, thehttp://fetch).quiche::Error::FingerprintUnsupportedis kept as#[deprecated](never constructed).Backend pieces live in moq-dev/web-transport#261, shipped as
web-transport-quiche0.4.2, which this PR bumps to.API note (additive — safe for main)
quiche::Errorgains fetch variants;FingerprintUnsupportedis deprecated, not removed.tls::ErrorgainsFingerprintWithRoots. No public removals.Test plan
tlsunit tests incl. newbuild_rejects_fingerprint_with_roots(fingerprint + system/custom roots both error).cargo check/clippyformoq-native(quinn default,noq,quiche) andmoq-cli/moq-relaywithmoq-native/quiche, against publishedweb-transport-quiche0.4.2 — clean.🤖 Generated with Claude Code