Skip to content

feat(node): serve authenticated browser clients over WebRTC Direct - #220

Draft
mickvandijke wants to merge 73 commits into
mainfrom
web-support
Draft

mickvandijke wants to merge 73 commits into
mainfrom
web-support

Conversation

@mickvandijke

@mickvandijke mickvandijke commented Sep 1, 2026

Copy link
Copy Markdown
Member

Summary

Adds a browser endpoint alongside native QUIC. Browser v5 sessions use the shared ML-KEM/ML-DSA handshake and encrypted records; binary chunk requests reuse ant_protocol::ChunkMessage and the ordinary storage/payment handler. Nodes remain native.

Standard builds enable the listener by default, with configuration and build-feature opt-outs. Endpoints are published through the shared V1/V2 address plane. The devnet HTTP manifest supplies bootstrap metadata; file data travels directly between clients and nodes.

Request admission survives disconnect until work drains. Binary RPCs decode once, avoid redundant PUT serialization, reserve response headroom by request type, preserve structured settlement-version refusals, and redact backend details. Certificate initialization is locked and atomic. Failed startup drains migration and background work through the normal shutdown path. The browser endpoint uses the native filesystem/SQLite storage implementation.

Linear issue

Closes V2-803 — Linear issue

Risk tier

  • T0 — docs / tooling / CI / pure UX-output. Repo CI only.
  • T1 — client-only, no network-facing behavior change. CI + prod compat smoke.
  • T2 — node/client logic with behavioral surface, no protocol/format/economics change. Dev testnet + ADR.
  • T3 — protocol / storage format / payments / routing. T2 evidence + adversarial testing.

Proposed for human review.

Compatibility

  • Wire: Adds browser v5 RPC and WebRTC Direct endpoint publication. Existing native QUIC, V1 publication, chunk wire messages, and payment proofs remain supported.
  • Storage: Browser requests use the same native filesystem/SQLite store and migration path as QUIC requests; no browser-specific storage format.
  • Api: Adds listener configuration, diagnostics, devnet endpoint artifacts, and the webrtc-direct feature. Browser sessions do not become routing-table members.

Semver impact

  • breaking
  • feature
  • fix

Test evidence

Validation of the updated EVM/protocol dependency pins during this refresh:

  • cargo check --all-targets --all-features: passed.
  • cargo test --lib web_rtc --features test-utils: 37 passed.

Earlier validation at cf8a84cc passed 1,127 library tests, four regular WebRTC integration tests, and all-target/all-feature Clippy. The coordinated Chromium seven-node Anvil upload/recovery/download check passed with that revision. This refresh changes dependency pins; it does not claim a new Chromium or full-node-suite run.

New dependency

Shared transport WebRTC feature, self_encryption for devnet fixtures, and coordinated immutable Git patches for protocol/core/PQC/EVM/transport. Published compatible dependency releases remain required for registry packaging.

ADR

Mitigation / rollback

Disable webrtc_direct.enabled or build without the WebRTC feature. Retain native clients and existing stored data.

Coordinated dependencies

Exact immutable revisions are recorded in Cargo.toml and Cargo.lock. Compatible published releases remain a release gate.

Current stack revisions

All PRs link to V2-803. Immutable Git pins are aligned with these pushed branch heads:

PR Revision
evmlib#17 cf424c04
ant-protocol#29 d557f5e1
saorsa-core#158 dc00a2c1
saorsa-transport#160 bc6dcd80
ant-node#220 4c1509dd
ant-client#186 2b0aa662
saorsa-pqc#7 29a2b272

EVM recovery now retains ambiguous journals until finalized failure/replacement evidence is available. The protocol and native client include the matching consumer changes. Existing browser wire and payment formats are retained.

@grumbach

grumbach commented Sep 8, 2026

Copy link
Copy Markdown
Member

Independent review of this PR at head f7127540, read as one stack together with saorsa-transport #160, saorsa-core #158 and ant-client #186. Everything below was verified against the source.

The server-side work is solid where it counts. The PQ handshake is genuinely unbypassable: handle_webrtc_channel establishes the session before any request is read (src/web_rtc.rs:947-951), and non-HELLO requests are refused until HELLO succeeds (src/web_rtc.rs:990-1024), with no plaintext fallback and no pre-handshake reachable handler. Browser stores and quotes go through the same ant_protocol.try_handle_request handler as native traffic (src/web_rtc.rs:1711), so there is no weaker parallel payment path, and payment_quote_from_browser_quote (src/web_rtc.rs:1763) rebuilds the native PaymentQuote, re-checks quote.hash() against the artifact and binds the content to the requested address, so the JSON projection cannot smuggle a modified quote past the signature. validate_webrtc_config is thorough about limit consistency and each invariant has a matching test. The two non-ignored devnet tests are real: a live listener, a real WebRTC dial, a real PQ session, and a check that the operator's RPC URL and credentials never reach a browser.

I also classified every src/replication/ and src/upgrade/ edit as behaviour-preserving, with one item I am less sure about: src/replication/mod.rs:3934 turns a closure returning an async block into an async closure, which looks equivalent but I could not type-check it against the pinned graph.

The listener is enabled by default, and its failure stops the node

Cargo.toml:186 has default = ["logging", "webrtc-direct"] and src/config.rs sets enabled: cfg!(feature = "webrtc-direct"), so every stock binary opens a second inbound UDP port, runs ICE/DTLS/SCTP and serves anonymous RPC. saorsa-transport's ADR-015 describes an opt-in listener, which is true of the cargo feature and not of the shipped node, and this repo's own ADR-0009 says production promotion is still gated on browser interop that has not happened.

Four things make that hard to back out of:

  1. A listener failure aborts startup. In RunningNode::run (src/node.rs:549-582), any error from web_rtc::spawn calls p2p_node.shutdown() and returns Err. A UDP bind failure on the derived port, a truncated or unreadable certificate PEM, or on custom EVM networks a failed eth_chainId call now stops a healthy storage node from starting. Before this it would simply run.
  2. No CLI kill switch. All three new flags set enabled = true (src/bin/ant-node/cli.rs:254-263). docs/WEBRTC_DIRECT_TESTNET.md:163 documents webrtc_direct.enabled = false in a config file and that does work, but a fleet driven by flags would need a config file introduced to turn this off.
  3. --no-default-features does not isolate. It omits src/web_rtc.rs, but saorsa-webrtc stays an unconditional dependency (Cargo.toml:38-43), src/browser.rs:8 imports it unconditionally, and the [patch.crates-io] block (Cargo.toml:202-204) redirects saorsa-core and saorsa-transport to the unreleased PR revisions even in a disabled build. So rebuilding to roll back browser risk still ships the unreleased native core and transport lineage, which is not what docs/WEBRTC_DIRECT_TESTNET.md:162-164 promises.
  4. The upgrader can leave a node offline. stop_on_upgrade defaults to false (src/config.rs:514). In that mode src/upgrade/apply.rs:628-674 spawns the replacement before graceful shutdown and reports success with no readiness handshake, and the parent then cancels (src/node.rs:656). If the child reaches the same derived UDP port before the old listener releases it, the child hits the fail-closed path above and dies while the parent exits anyway. Rollback only covers binary replacement, not child startup.

Certificate persistence is also not atomic: load_or_generate_certificate (src/web_rtc.rs:668-690) writes the PEM to its final path and chmods 0600 in a later await, so a crash in between leaves the private key at the process umask, and a partial write leaves a corrupt PEM which, given (1), then prevents the node from booting. Existing files never have their permissions repaired. Writing a mode-0600 temp file and renaming would fix both halves.

Worth knowing on the dependency side: Cargo.lock grows from 653 to 718 packages in every default build, including turn, webrtc-mdns, webrtc-media, webrtc-srtp, rtp, rtcp and sdp. ADR-015 says the design does not introduce TURN or STUN servers, yet a TURN implementation and an mDNS responder are now linked in. It is worth confirming webrtc-mdns does not emit multicast traffic in this configuration, because an unexpected mDNS responder on production hosts would surprise operators.

Two CI checks are red

The ADR governance job fails because docs/adr/ADR-0009-direct-browser-clients-over-webrtc-direct.md collides with ADR-0009-audit-proof-shape-and-protocol-families.md already on main, and docs/adr/README.md is not updated so the new ADR is absent from the index. Clippy fails with five denied lints, all in files this PR touched only mechanically: src/replication/config.rs:246, 255, 1663, src/replication/slice.rs:345 and src/replication/protocol.rs:1677.

Relatedly, the MSRV move from 1.75 to 1.91 exists to allow Duration::from_mins and from_hours, and 16 unrelated files across replication/ and upgrade/ were rewritten to use them. That enlarges the diff, couples a browser feature to a toolchain bump, and constrains every build environment. It would read better as its own PR.

Worth fixing before this ships

The derived port is not stable by default. NodeConfig::default() sets the native port to 0 (src/config.rs:438), and the browser port is derived from whatever ephemeral port the OS assigns (src/node.rs:539, src/web_rtc.rs:82-120). A node started without an explicit --port publishes a different WebRTC address after every restart, invalidating cached endpoints even though it reuses its certificate. ADR-0009:483-488 and docs/WEBRTC_DIRECT_TESTNET.md:132-137 both claim the complete address stays stable, which holds only for nodes given a fixed native port, which is what the testnet harness does.

A valid max_request_bytes makes the browser pay and then be rejected. The shared contract fixes the header bound at 64 KiB, but this node lets an operator configure max_request_bytes anywhere in 1..=65536 (src/web_rtc.rs:647) and enforces the smaller value at parse time (src/web_rtc.rs:959), before any request processing, and HELLO never advertises the node's real limit. A PUT header always exceeds 10 KiB, because the quote carries a 1,952-byte ML-DSA public key and a 3,309-byte signature hex-encoded, before the optional commitment sidecar. So with the limit set below about 11 KiB, HELLO and quote_chunk succeed, the browser pays on-chain, and put_chunk is rejected before storage. The 64 KiB default is safe with roughly 27 KiB of margin. Pinning the limit to the shared contract, or advertising it in HELLO, would close it.

A partial config section inverts the default. WebRtcDirectConfig has no container-level #[serde(default)], and enabled carries a plain #[serde(default)] (src/config.rs:161-163) resolving to false, while every other field has an explicit default function. So a TOML file with no [webrtc_direct] section enables the listener, and one with the section but without enabled disables it:

[webrtc_direct]
max_connections = 48

default_webrtc_listener_tracks_compile_time_feature (src/config.rs:776) only exercises WebRtcDirectConfig::default(), never the deserialised path, so it cannot catch this.

Storage errors reach anonymous browsers. src/web_rtc.rs:1503 returns "chunk read failed: {error}" and :1598, :1684 return {other:?}. src/browser.rs:76-84 goes to real trouble to avoid leaking RPC provider detail; this path does not, and with the file-per-chunk store those strings can carry filesystem paths.

Per-IP limits do not aggregate IPv6. canonical_source_ip (src/web_rtc.rs:474) only unmaps IPv4-mapped v6, so a routine /64 gives an attacker a practically unlimited number of distinct sources, each with its own connection slot, rate bucket and byte budget.

cache_insert_browser_devnet_seed is behind the wrong feature. src/payment/verifier.rs:1244 marks content as prepaid and is gated on webrtc-direct, which is on by default, so it ships in production binaries. It is pub(crate) and only reachable through Devnet::publish_public_file, which operates on its own verifier, so I do not believe it is exploitable, but a payment bypass belongs behind test-utils or a dedicated devnet feature.

resolve_automatic_config falls back to loopback. src/web_rtc.rs:99-108 uses 127.0.0.1 when the route probe fails, and the node then publishes that as its browser endpoint. saorsa-core drops loopback on ingest, so the publish is silently wasted rather than an error.

Test fidelity and validation gaps

The five-node devnet test is valuable native interoperability coverage, but it is not browser coverage and does not test the browser client. BrowserRpcClient (tests/webrtc_direct_devnet.rs:426) is a native Rust client built on saorsa_transport::webrtc_direct::WebRtcDirectClient, with hand-written framing and PQ logic using the same saorsa-webrtc crate and constants as the server. If node framing and this adapter drift together while the deployed WASM client does not, CI stays green and every real browser fails.

Three more specific gaps:

  • Nothing moves a large payload. The whole suite transfers 31 bytes (tests/webrtc_direct_devnet.rs:206) and 32 bytes (:349); MAX_CHUNK_SIZE appears only as a read bound. So the 16 KiB DataChannel chunking loop, multi-fragment reassembly, byte reservations at scale, the size-scaled write deadlines and SCTP backpressure are never exercised, even though ant-devnet's own default public file is 5 MiB (src/bin/ant-devnet/main.rs:201) and the runbook implies automated verification publishes it.
  • The admission-timeout test is a mock. first_data_channel_timeout_releases_connection_admission (src/web_rtc.rs:1874-1915) manually takes an admission guard, hands pending() to the timeout helper and lets the guard drop out of local scope. It never drives handle_connection, a real association or accept_data_channel, so it would still pass if production stopped wrapping the first channel in that helper. That is the invariant the listener's availability depends on.
  • The five-node test bypasses discovery. It takes its initial endpoints straight from Devnet::browser_endpoints() (:203-220) and makes two manually scripted one-hop requests, with responses augmented by the in-process endpoint catalog, so it cannot catch a failure of V2 DHT publication, mixed-fleet propagation, alpha/K convergence, retained routing state, failure cooldowns or production seed bootstrapping.

ADR-0009 is admirably candid that there is no automated real-browser v5 flow and that Chrome, Firefox and Safari interop remain unmet acceptance criteria rather than claimed results. That is the right way to write it up. The tension is that an explicitly unmet acceptance criterion currently ships enabled by default.

For operations, what exists is startup logging, an endpoint file, static resource knobs, fail-fast startup and shutdown ordering. There is no listener health or readiness metric, no DHT reachability probe, no saturation alert, no runtime disable flag, no certificate backup or rotation procedure, no child-readiness handshake in the upgrader, no browser-aware rollback test and no firewall or NAT verification workflow.

Smaller things

  • ADR-0009 contradicts itself on the connection profile: :507-517 says the implemented profile is v2 with no v1 fallback, :742-746 says the implementation currently uses v1. The code uses v2, so :742 is stale.
  • docs/WEBRTC_DIRECT_TESTNET.md:8-10 says Rust 1.88 or newer while the package now requires 1.91, so following the runbook fails before you reach any test. The ADR also names ant-protocol 2.3.1 while Cargo uses 2.3.5, and says saorsa-transport owns persisted certificates while this repo loads and writes them.
  • --webrtc-direct-advertised-addr has requires = "webrtc_direct_bind" (src/bin/ant-node/cli.rs:44), so overriding only the advertised address means redundantly repeating the bind.
  • saorsa-webrtc is a git dependency with no version key in [dependencies], which cargo publish refuses, and the crate name is already taken on crates.io at 0.1.0 through 0.1.2 by a different crate. That blocks releasing this repo.

What I could not check

Nothing was built or run: the pinned cross-repo revisions are not in a local cargo cache here. No real browser, no NAT or firewall path, no mixed-version fleet, and no measurement of the resource envelope under load.

@dirvine dirvine left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 3ce1e9b147aa9074300c861aee7c1c7400710c24.

Advisory verdict: REQUEST_CHANGES before this leaves draft. The core request path looks substantially stronger than the earlier review, but one current code issue and the declared release gates remain.

Material finding

Persist the DTLS private key atomically with private permissions (src/web_rtc.rs:821-837).

load_or_generate_certificate writes the certificate and private key directly to the final path with tokio::fs::write. On Unix that normally creates the file as 0666 & umask (commonly 0644), and a crash or concurrent start can leave a truncated PEM. Because the listener is enabled by default and certificate-load failure aborts node startup, this is both key-material handling and boot-availability risk. Please create a same-directory temporary file with mode 0600, flush/sync as appropriate, atomically rename it, and repair/reject unsafe permissions on an existing file. Add tests for mode and interrupted/corrupt persistence.

Confirmed draft/release gates

  • The current v4/v5 evidence is native Rust against Rust. The ADR explicitly says current Chrome, Firefox and Safari interoperability is still unproven, while webrtc-direct is enabled in the default build. This is already acknowledged in the PR's remaining work and should stay blocking until real-browser evidence exists and the ADR has human owners/reviewers/acceptance.
  • The Git dependency stack is not publishable as declared. Locally, cargo package --allow-dirty --locked --no-verify fails because crates.io saorsa-transport 0.36.3 lacks the requested webrtc feature. This matches the stated requirement to replace draft pins with reviewed releases before merge.
  • The PR body gives cargo test --test webrtc_direct_devnet --features webrtc-direct -- --ignored --nocapture as the five-node command, but that executes 0 tests because the five-node test is gated on test-utils. The CI/README form using --features test-utils is the effective command.

Verification

  • cargo test --lib web_rtc --features webrtc-direct --no-fail-fast: 29 passed
  • cargo test --lib config::tests:: --features webrtc-direct: 50 passed
  • cargo test --test webrtc_direct_devnet --features test-utils -- --ignored --test-threads=1 --nocapture: 1 passed
  • cargo check --no-default-features --locked: passed
  • Current GitHub CI: build/test/clippy/security/devnet jobs green; linear-link and pr-template red, consistent with the declared draft work.

Review-team split: the focused request-path/security review found no remotely exploitable or availability-blocking flaw in src/web_rtc.rs; the architecture/release-readiness review requested changes for the browser-evidence, ADR and dependency gates. Four other review seats timed out and supplied no opinion, so I am not counting them as evidence.

@mickvandijke mickvandijke changed the title feat: add WebRTC-Direct support feat(node): serve authenticated browser clients over WebRTC Direct Sep 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants