diff --git a/quest/m1/README.md b/quest/m1/README.md index b25e1a2af05..996d84e0b76 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 fixed by moq-dev/moq#6509. + Added 2026-10-23 (maintainer, from a linger audit): broadcasts lose their finished state and retention moves to moq-mux, a break in every binding, so it lands before the cut. @@ -168,6 +174,11 @@ audit's receive-memory, priority, and lease bugs rank ahead of that reshape. - [HLS recorder in the CLI](/quest/m1/hls-record-cli.md) - `export hls --output ` records playlists and segments to a directory - [Group storage benchmark](/quest/m1/group-storage-bench.md) - numbers pick the append-only log, a ring buffer, or sorted runs for `GroupState` before either lands - [Unbounded groups](/quest/m1/unbounded-groups/README.md) - groups and frames lose their size caps; the cache sheds parts of a group under pressure and refills the holes in place +- [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 ## Related 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..5dbd96f112f --- /dev/null +++ b/quest/m1/clean-close-reason.md @@ -0,0 +1,42 @@ +# [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` 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. + +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. + +## 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..8a8932b5d15 --- /dev/null +++ b/quest/m1/webrtc-mdns-resolver.md @@ -0,0 +1,37 @@ +# [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. 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; 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. + +## 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.