From e2731a9432989fad64ec4dd54ecbbe7db1ed8511 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 15:37:08 -0700 Subject: [PATCH 1/4] quest: plan the 2026-10-22 security audit follow-ups Adds m1 quests for a clean close misreported as a revocation, metric-safe tiers, the ACME renewal failure budget, a per-peer WebRTC mDNS resolver, and the stats group linger contract. The server_name finding is already owned by the auth contract quest. Co-Authored-By: Claude Opus 5.5 --- quest/m1/README.md | 11 ++++++++ quest/m1/acme-renewal-budget.md | 33 ++++++++++++++++++++++++ quest/m1/clean-close-reason.md | 35 +++++++++++++++++++++++++ quest/m1/metric-tier.md | 40 +++++++++++++++++++++++++++++ quest/m1/stats-group-linger.md | 31 ++++++++++++++++++++++ quest/m1/webrtc-mdns-resolver.md | 33 ++++++++++++++++++++++++ quest/m2/relay-prometheus-client.md | 7 +++-- 7 files changed, 186 insertions(+), 4 deletions(-) create mode 100644 quest/m1/acme-renewal-budget.md create mode 100644 quest/m1/clean-close-reason.md create mode 100644 quest/m1/metric-tier.md create mode 100644 quest/m1/stats-group-linger.md create mode 100644 quest/m1/webrtc-mdns-resolver.md diff --git a/quest/m1/README.md b/quest/m1/README.md index ecdc6d286f6..0d7c72a1020 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -68,6 +68,12 @@ announce churn on today's hop-list mesh, shipping with the next cut from route layer ships; and stats groups as tracks, so per-group stats stop costing cluster announces. +Added 2026-10-22 (maintainer, security audit of the relay, auth, stats, +ACME, and WebRTC): a clean close misreported as a revocation, injectable +metric tiers, an unbudgeted ACME renewal, unbounded mDNS work per WebRTC +offer, and an undocumented stats linger cut. The audit's `server_name` +finding was already owned by the auth contract quest. + ## Required - [Epoch correctness](/quest/m0/broadcast-epoch/README.md) - publisher identity, duplicate suppression, and catalog pinning gate the breaking cut @@ -172,6 +178,11 @@ tracks, so per-group stats stop costing cluster announces. - [Peer count bounds](/quest/m1/peer-count-bounds.md) - namespace interests, a legacy announce list, a WebSocket upgrade, and public HTTP admission are bounded by what the peer sent or by an existing budget - [Caption store bounds](/quest/m1/caption-store-bounds.md) - a hostile caption track cannot grow a viewer's cue and region store or its per-frame work without bound - [Auth contract](/quest/m1/auth-contract.md) - the auth server receives the certificate-selecting SNI, documented as client-chosen, and a certificate grants nothing alone +- [Clean close reason](/quest/m1/clean-close-reason.md) - a session that closes on its own is never recorded as `Unauthorized` because its in-band credentials closed with it +- [Metric-safe tiers](/quest/m1/metric-tier.md) - tiers are `[A-Za-z0-9._-]` paths refused where they are set, and every metric label value is escaped +- [ACME renewal budget](/quest/m1/acme-renewal-budget.md) - renewal counts failed validations against the startup budget and pauses instead of locking the hostname out +- [WebRTC mDNS resolver](/quest/m1/webrtc-mdns-resolver.md) - each peer resolves its `.local` candidates in one owned task, one distinct name at a time +- [Stats group linger](/quest/m1/stats-group-linger.md) - at depth >= 1 a lingered group ends its held tracks, documented and tested - [Nightly benchmark regressions](/quest/m1/bench-ci.md) - each nightly compares the Rust Criterion benches against the previous nightly's commit on the same hosted runner, publishes it to the git.moq.dev report, and alerts on a regression - [Performance audit](/quest/m1/audit-perf.md) - benchmarks that regressed since the m0 run are explained, fixed, or planned, and unbenched hot paths get coverage diff --git a/quest/m1/acme-renewal-budget.md b/quest/m1/acme-renewal-budget.md new file mode 100644 index 00000000000..bca46b97ae4 --- /dev/null +++ b/quest/m1/acme-renewal-budget.md @@ -0,0 +1,33 @@ +# [S] ACME renewal honors the failed-validation budget + +## Goal + +A persistent renewal misconfiguration cannot lock the relay's hostname out of +Let's Encrypt. Renewal counts failed validations against the same budget +startup uses, and pauses once it is spent while the previous certificate +keeps serving. + +## Plan + +Decided 2026-10-22 from the security audit, with the maintainer. +`Acme::issue` (`rs/moq-relay/src/acme.rs`) records each +`Order(BadAuth)` and refuses after `FAILURE_LIMIT` within `FAILURE_WINDOW`, +which keeps the relay under five failed validations per hostname per hour. +`Acme::run` only logs errors, and rustls-acme 0.15 retries a failed order +after 1, 2, 4, 8, and 16 seconds, so renewal can exceed the limit within a +minute. Renewal failures are also not persisted, so a restart after them +misjudges the budget. + +- In `run`, record each `BadAuth` through `self.failures`, as `issue` + does. +- Once the recent failures reach the limit, stop polling the ACME state until + the oldest failure leaves the window, then resume. Not polling the + stream pauses rustls-acme's order loop without forking it. Log the pause + with the time renewal resumes. +- A successful renewal clears the failures, as issuance does. + +Test (mocked time): repeated renewal `BadAuth` failures stop at the limit, +resume after the window, and are seen by a later `issue` call. + +Docs: `doc/bin/relay/config.md` says the budget covers renewal. Public API: +none. Wire: none. diff --git a/quest/m1/clean-close-reason.md b/quest/m1/clean-close-reason.md new file mode 100644 index 00000000000..d5e06a4e87e --- /dev/null +++ b/quest/m1/clean-close-reason.md @@ -0,0 +1,35 @@ +# [XS] A clean close is never reported as a revocation + +## Goal + +A session that closes on its own is recorded as a session close, never as +`Unauthorized` with an application failure, even when the close also +withdraws every in-band credential at the same moment. + +## Plan + +Decided 2026-10-22 from the security audit, with the maintainer. +`supervise` (`rs/moq-relay/src/connection.rs`) selects `session.closed()` +and `lease.ended()` without bias. A transport close also closes each in-band +credential's issued stream, so `Refresh::ended` +(`rs/moq-relay/src/auth/refresh.rs`) can report `Reason::Dropped` in the same +poll. If the lease wins, the relay aborts with `Unauthorized` +(`auth::error`) and `Lease::close` records `sessions_failed.application`, +contradicting `Lease::close`'s own rule that `Dropped` is an effect of the +close, not a verdict. + +- Add `biased;` to that `select!` and poll `session.closed()` first. + `Lease::close` already folds in a real, non-`Dropped` revocation, so none is + lost. +- A peer that withdraws its last credential on a live session still ends it + as today. + +Test (mocked time, as the refresh tests do): a session whose only remaining +credential is in band closes cleanly; the lease reports the session reason +and no application failure. + +Public API: none. Wire: none. + +## Related + +- [In-band auth](/quest/m1/auth/README.md) - owns the refresh and lease contract diff --git a/quest/m1/metric-tier.md b/quest/m1/metric-tier.md new file mode 100644 index 00000000000..ccc7c265c9c --- /dev/null +++ b/quest/m1/metric-tier.md @@ -0,0 +1,40 @@ +# [S] Tiers are metric-safe names + +## Goal + +A stats tier from an auth server cannot break or forge `/metrics` series. Every +tier the relay accepts is a slash-separated path of `[A-Za-z0-9._-]` segments, +and the relay escapes every label value it renders. + +## Plan + +Decided 2026-10-22 from the security audit, with the maintainer. `Grant.tier` +and `Refusal.tier` are unvalidated (`rs/moq-auth/src/grant.rs`, +`rs/moq-auth/src/refusal.rs`). The relay turns them into a `Tier` +(`rs/moq-relay/src/auth.rs`, `rs/moq-relay/src/refusals.rs`), and +`render_metrics` (`rs/moq-relay/src/internal.rs`) interpolates the tier raw +into `tier="..."`. An auth server that derives the tier from a token claim +can therefore inject `"`, `\`, or a newline. Release has the same +interpolation, but this is not backported: only an auth server sets the tier. + +- ✅ Both validate and escape. Validation fails loud at the source; + escaping keeps exposition correct for any label value, now or later. +- ✅ The rule: tier segments of ASCII letters, digits, `.`, `_`, and `-`, + joined by `/`. Enforce it in `Grant::validate`, when a `Refusal` is + parsed, in `moq-auth serve --tier`, and in the relay's tier configuration, + so a bad tier is refused where it is set. Mirror the check in `js/auth`'s + contract schemas. +- Escape `\`, `"`, and newline in every label value `render_metrics` and + `rs/moq-relay/src/stun.rs` write, via one helper. + +Tests: a grant and a refusal with a quote, backslash, or newline in the tier +are refused; a rendered label containing each character is escaped per the +Prometheus text format. + +Docs: `doc/bin/relay/auth.md` states the tier rule. Public API: `Grant` and +`Refusal` refuse tiers they accepted before, so add a `MIGRATE.md` entry for +moq-auth and @moq/auth. Wire: the auth server's JSON accepts fewer tiers. + +## Related + +- [Relay metrics through prometheus-client](/quest/m2/relay-prometheus-client.md) - replaces hand-written exposition later; escaping lands here first diff --git a/quest/m1/stats-group-linger.md b/quest/m1/stats-group-linger.md new file mode 100644 index 00000000000..a8e00af4777 --- /dev/null +++ b/quest/m1/stats-group-linger.md @@ -0,0 +1,31 @@ +# [XS] Stats group linger ends held tracks + +## Goal + +At `stats.depth >= 1`, the documented contract matches the code: a group's +broadcast lives while it has traffic, and once it lingers out, every track it +carries ends, including presence, prefix, and requested tier tracks a +subscriber still holds. A reader follows the group's next announcement. + +## Plan + +Decided 2026-10-22 from the security audit, with the maintainer. +`GroupPublisher::lingered` (`rs/moq-stats/src/produce.rs`) counts only +traffic and session rows, and `finish` ends every track. This is the intended +behavior. Subscribers do not keep a group announced, which matches announcements +being online or offline and keeps a reader from pinning a group's announcement +across a cluster. `doc/concept/stats.md` describes held presence and prefix +tracks as lasting until the last subscriber leaves, which is true only for +depth 0's permanent node broadcast. + +- Update `doc/concept/stats.md`: at depth >= 1, held and requested tracks + end with their group's linger. +- Add depth-1 tests (mocked time): a held idle presence track, a held + prefix track, and a requested tier track each finish when the group + lingers out, and are served again after the group's next announcement. + +Public API: none. Wire: none. + +## Related + +- [Stats groups as tracks](/quest/m1/stats-group-tracks.md) - the new layout keeps the node broadcast stable instead diff --git a/quest/m1/webrtc-mdns-resolver.md b/quest/m1/webrtc-mdns-resolver.md new file mode 100644 index 00000000000..3d5b088892b --- /dev/null +++ b/quest/m1/webrtc-mdns-resolver.md @@ -0,0 +1,33 @@ +# [S] One mDNS resolver per WebRTC peer + +## Goal + +An untrusted offer or answer cannot make the native WebRTC transport spawn +unbounded work. Each peer resolves its `.local` candidates in one task it +owns, one distinct name at a time, and that task ends with the peer. + +## Plan + +Decided 2026-10-22 from the security audit, with the maintainer. +`split_mdns` (`rs/moq-tokio/src/webrtc.rs`) collects every `.local` candidate +line, and `Peer::resolve` spawns one detached task per line. Each task waits up +to `MDNS_TIMEOUT` (5 s) and multicasts a query. One description can therefore +create any number of tasks and queries, and they outlive the peer. + +- ✅ Bound the cost by design, not with a candidate cap: the `Peer` owns one + resolver task, aborted on drop. That task dedupes lines by host name, resolves + each distinct name once, sequentially, and sends every line that shares + the name. Trickled `.local` candidates join the same queue rather than + resolving inline. +- The cost is one mDNS query in flight per peer, so total work scales with + the peers signaling already admits. + +Tests: a description with many `.local` lines sharing one name issues one +query; many distinct names resolve one at a time; dropping the peer cancels +pending resolution. Use a fake resolver rather than real multicast. + +Public API: none. Wire: none. + +## Related + +- [P2P](/quest/m2/p2p/README.md) - the signaling that exposes this transport to untrusted peers diff --git a/quest/m2/relay-prometheus-client.md b/quest/m2/relay-prometheus-client.md index a1189e335d3..826a003944b 100644 --- a/quest/m2/relay-prometheus-client.md +++ b/quest/m2/relay-prometheus-client.md @@ -11,9 +11,8 @@ exposition, with the same metric names, labels, and types. Found by the 2026-10-19 dependency audit; adding the dependency is the maintainer's call. `render_metrics` in `rs/moq-relay/src/internal.rs` writes its metric families by hand (about 430 lines, histograms included), and -`rs/moq-relay/src/stun.rs` adds more. Label values are interpolated without -escaping: safe while every label is an enum, wrong as soon as one carries a -broadcast path or peer string. +`rs/moq-relay/src/stun.rs` adds more. [Metric-safe tiers](/quest/m1/metric-tier.md) +adds label escaping by hand first. `prometheus-client` 0.25.1 (released 2026-09) needs only `dtoa`, `itoa`, `parking_lot`, and its derive crate without the protobuf feature. Keep the @@ -24,4 +23,4 @@ before switching, so a renamed or retyped series fails loudly; `doc/bin/relay/http.md` documents the endpoint. A no-go is valid if the crate cannot reproduce the existing series without -renames; then add label escaping and a test instead. +renames. From 9f125c649dbaa1295f1e3cdd91943562324a0bb7 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Thu, 22 Oct 2026 15:43:17 -0700 Subject: [PATCH 2/4] quest: backpressure trickled mDNS candidates Co-Authored-By: Claude Opus 5.5 --- quest/m1/webrtc-mdns-resolver.md | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/quest/m1/webrtc-mdns-resolver.md b/quest/m1/webrtc-mdns-resolver.md index 3d5b088892b..8a8932b5d15 100644 --- a/quest/m1/webrtc-mdns-resolver.md +++ b/quest/m1/webrtc-mdns-resolver.md @@ -17,13 +17,17 @@ create any number of tasks and queries, and they outlive the peer. - ✅ Bound the cost by design, not with a candidate cap: the `Peer` owns one resolver task, aborted on drop. That task dedupes lines by host name, resolves each distinct name once, sequentially, and sends every line that shares - the name. Trickled `.local` candidates join the same queue rather than - resolving inline. -- The cost is one mDNS query in flight per peer, so total work scales with - the peers signaling already admits. + the name. A description's lines are already held, bounded by its size. +- Trickled `.local` candidates reach the resolver through a bounded channel, + so `add_candidate` waits for resolver capacity rather than queueing. Held + candidates are then bounded by what the peer has sent, and the peer's + signaling stalls behind its own slow names. +- The cost is one mDNS query in flight per peer, and the work queued behind it + is bounded by the peer's descriptions and backpressured trickle. Tests: a description with many `.local` lines sharing one name issues one -query; many distinct names resolve one at a time; dropping the peer cancels +query; many distinct names resolve one at a time; sustained trickled distinct +names make `add_candidate` wait instead of queueing; dropping the peer cancels pending resolution. Use a fake resolver rather than real multicast. Public API: none. Wire: none. From 9d95b9902259bd5507092bcb2b090307545b1de9 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 23 Oct 2026 17:07:32 -0700 Subject: [PATCH 3/4] quest: a pending revocation outranks the close that withdrew it Co-Authored-By: Claude Opus 5.5 --- quest/m1/clean-close-reason.md | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/quest/m1/clean-close-reason.md b/quest/m1/clean-close-reason.md index d5e06a4e87e..5dbd96f112f 100644 --- a/quest/m1/clean-close-reason.md +++ b/quest/m1/clean-close-reason.md @@ -19,14 +19,21 @@ contradicting `Lease::close`'s own rule that `Dropped` is an effect of the close, not a verdict. - Add `biased;` to that `select!` and poll `session.closed()` first. - `Lease::close` already folds in a real, non-`Dropped` revocation, so none is - lost. +- `Lease::close` folds in a non-`Dropped` reason from one poll of + `Refresh::ended`, but `Credential::changed` polls `issued.closed()` ahead + of expiry and `consumer.changed()`, so after a close it reports `Withdrawn` + even when the decider's revocation is already pending. Poll expiry and + `consumer.changed()` ahead of the withdrawal: a verdict the decider already + gave outranks the close that withdrew the credential, and a credential that + merely refreshed is still withdrawn on the next poll. - A peer that withdraws its last credential on a live session still ends it as today. -Test (mocked time, as the refresh tests do): a session whose only remaining -credential is in band closes cleanly; the lease reports the session reason -and no application failure. +Tests (mocked time, as the refresh tests do): a session whose only remaining +credential is in band closes cleanly, and the lease reports the session reason +and no application failure. A revocation of that credential already pending +when the transport closes is still recorded as the revocation, with its +failure kind. Public API: none. Wire: none. From e39dd678a5f9f469fdaf409fd9de3991093ffc41 Mon Sep 17 00:00:00 2001 From: Luke Curley Date: Fri, 23 Oct 2026 17:10:48 -0700 Subject: [PATCH 4/4] quest: the server_name finding landed in #6509 Co-Authored-By: Claude Opus 5.5 --- quest/m1/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quest/m1/README.md b/quest/m1/README.md index 376c14d9298..552e4825fa9 100644 --- a/quest/m1/README.md +++ b/quest/m1/README.md @@ -72,7 +72,7 @@ Added 2026-10-22 (maintainer, security audit of the relay, auth, stats, ACME, and WebRTC): a clean close misreported as a revocation, injectable metric tiers, an unbudgeted ACME renewal, unbounded mDNS work per WebRTC offer, and an undocumented stats linger cut. The audit's `server_name` -finding was already owned by the auth contract quest. +finding was already fixed by moq-dev/moq#6509. ## Required