feat: certificate pinning for native and browser clients#1698
Conversation
Verifying a self-signed peer previously meant either disabling TLS verification entirely or letting the client fetch the certificate hash over an insecure http:// request. Neither the PEM-root knob nor the fingerprint path was reachable from the FFI, so Python/Swift/Kotlin callers were stuck with set_tls_disable_verify. Add a tls.Client.fingerprint field (--tls-fingerprint, MOQ_CLIENT_TLS_FINGERPRINT) that pins the peer's leaf certificate to one or more hex SHA-256 hashes, bypassing the CA chain. This is the native equivalent of WebTransport serverCertificateHashes and accepts the exact values a server already reports via cert_fingerprints(). FingerprintVerifier is ungated from the quinn/noq backends so it applies to every transport, and now matches against a set to support rotation. Expose both knobs through moq-ffi (MoqClient.set_tls_roots / set_tls_fingerprints) and the Python wrapper (Client tls_roots / tls_fingerprints), so callers can trust a self-signed cert without turning verification off. moq-cli picks up --tls-fingerprint / --tls-root for free via the flattened config. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR implements SHA-256 certificate fingerprint pinning and optional TLS root overrides across native Rust, FFI, Python, and JS surfaces. The Rust TLS layer accepts multiple hex fingerprints, validates/decodes them in Client::build(), and installs a verifier that accepts any matching fingerprint. Connection code paths now pass fingerprints as vectors. UniFFI exposes set_tls_roots and set_tls_fingerprints. The Python Client gains tls_roots/tls_fingerprints parameters and README docs were updated. JS WebTransport gained certificate hashing, pin normalization, types, and tests. 🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rs/moq-native/src/tls.rs (1)
210-218: 💤 Low valueConsider validating fingerprint length during decode.
The code correctly decodes hex fingerprints and installs the verifier, but it doesn't validate that each decoded fingerprint is exactly 32 bytes (SHA-256). A fingerprint of the wrong length would never match at handshake time, potentially confusing users who made a typo.
💡 Optional: Add length validation
let fingerprints = self .fingerprint .iter() - .map(|fp| hex::decode(fp.trim()).map_err(Error::Fingerprint)) + .map(|fp| { + let bytes = hex::decode(fp.trim()).map_err(Error::Fingerprint)?; + if bytes.len() != 32 { + // Could add a new error variant, or reuse Fingerprint with a custom message + return Err(Error::Fingerprint(hex::FromHexError::InvalidStringLength)); + } + Ok(bytes) + }) .collect::<Result<Vec<_>>>()?;🤖 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 210 - 218, The hex-decoding pipeline for self.fingerprint should validate each decoded fingerprint is exactly 32 bytes (SHA-256) before constructing the FingerprintVerifier; after hex::decode(...) (or immediately after collecting the Vec<u8>), check each decoded value's len() == 32 and return an appropriate error (e.g., extend or reuse Error::Fingerprint to indicate bad length) if any fingerprint is the wrong size, then only call FingerprintVerifier::new(provider, fingerprints) and tls.dangerous().set_certificate_verifier(...) when all entries pass validation.
🤖 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.
Nitpick comments:
In `@rs/moq-native/src/tls.rs`:
- Around line 210-218: The hex-decoding pipeline for self.fingerprint should
validate each decoded fingerprint is exactly 32 bytes (SHA-256) before
constructing the FingerprintVerifier; after hex::decode(...) (or immediately
after collecting the Vec<u8>), check each decoded value's len() == 32 and return
an appropriate error (e.g., extend or reuse Error::Fingerprint to indicate bad
length) if any fingerprint is the wrong size, then only call
FingerprintVerifier::new(provider, fingerprints) and
tls.dangerous().set_certificate_verifier(...) when all entries pass validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c5da6f3-99d2-42a9-8ea3-3315abbc523f
📒 Files selected for processing (7)
py/moq-rs/README.mdpy/moq-rs/moq/client.pyrs/moq-ffi/src/session.rsrs/moq-native/src/client.rsrs/moq-native/src/noq.rsrs/moq-native/src/quinn.rsrs/moq-native/src/tls.rs
Pinning a self-signed server from the browser meant hand-building the DOM
`serverCertificateHashes` struct: `{ algorithm: "sha-256", value: <bytes> }`
with a manual hex decode. And there was no way to pin from the certificate
itself, only from a precomputed hash.
Widen `connect()`'s WebTransport options so `serverCertificateHashes`
values accept a hex string (the format moq servers report), decoded
automatically, and add `serverCertificate` to pin from a PEM/DER cert with
the SHA-256 computed in JS. Export `certificateHash()` for callers that want
the hash directly, and add `Hex.fromBytes` as the counterpart to `toBytes`.
This mirrors the native client's `tls_fingerprints` (both now take hex
strings) so the same value a server reports via its certificate fingerprints
works in either client without conversion.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A typo'd or truncated --tls-fingerprint would decode fine as hex but never match at handshake time, leaving the user with a confusing connection failure. Validate the SHA-256 length at config-build time instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the fingerprint-length nitpick in 34239b1: (Written by Claude) |
Address review feedback on certificate pinning: - pemToDer now requires the -----BEGIN/END CERTIFICATE----- armor and throws a clear error instead of silently atob-ing whatever it's given. - connectWebTransport accumulates caller-provided pins into a single array and appends the http:// fetched fingerprint, so a fetched hash can never clobber hashes passed via options (it was already additive via concat, but two assignment sites made it non-obvious). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
Trusting a self-signed or privately-issued peer previously meant disabling TLS verification entirely. This adds proper pinning to both the native and browser clients so callers can trust a specific certificate without turning verification off.
The unifying idea: a server already reports its certificate's SHA-256 fingerprint as hex (native
cert_fingerprints(), the/certificate.sha256endpoint). Now both clients accept that same hex value directly — no insecure runtime fetch, no manual byte-wrangling.Native (
moq-native/moq-ffi/py)tls::Client.fingerprintfield (--tls-fingerprint,MOQ_CLIENT_TLS_FINGERPRINT): pin the peer's leaf certificate to one or more hex SHA-256 hashes, bypassing the CA chain, hostname match, and expiry. The native equivalent of WebTransportserverCertificateHashes.FingerprintVerifieris ungated from the quinn/noq backends so it applies to every transport, and matches against a set to support rotation.--tls-root) is now reachable from the FFI too.MoqClient.set_tls_roots(paths)andset_tls_fingerprints(hashes)alongsideset_tls_disable_verify.Client(url, tls_roots=..., tls_fingerprints=...).moq-clipicks up--tls-root/--tls-fingerprintfor free via the flattened config.Browser (
js/net)connect()'swebtransport.serverCertificateHashesvalues now accept a hex string (decoded automatically), not just raw bytes.webtransport.serverCertificateoption: pin from a PEM string or DER bytes with the SHA-256 computed in JS.certificateHash(cert)helper andHex.fromBytes(counterpart totoBytes).Motivation / scope
This came out of looking at pipecat#4629. Worth being precise about what helps where:
serverCertificateHashes) is a browser-side flow. The native client change doesn't touch it; the JS ergonomics here do (hex-string +serverCertificateinstead of hand-built hash structs).moq.Client(tls_verify=...)) only had a bool.tls_fingerprints/tls_rootsis the upgrade if that relay is ever self-signed.So neither half is strictly required by that PR, but both close a real "verify or disable, nothing in between" gap on their respective clients.
Notes for reviewers
#[non_exhaustive]config field + new FFI methods (native); widened option types (existing{value: BufferSource}callers still compile) + new exports (JS). Non-breaking, hencemain.cert_fingerprints()//certificate.sha256.setTlsRoots/setTlsFingerprintson the nextmoq-ffi-v*mirror. libmoq exposes no client TLS-verify surface today, so nothing to mirror there.Test plan
cargo test -p moq-native—--tls-fingerprintTOML/CLI parsing, the TOML→CLI merge-clobber regression, and aFingerprintVerifierroundtrip (match / mismatch / invalid-hex)bun testinjs/net—certificateHashover DER and PEM against a known SHA-256 vector, hex round-trip (145/145 pass)moq-native+moq-ffi; tsc + biome onjs/netmoq_ffi)🤖 Generated with Claude Code
(Written by Claude)