-
-
Notifications
You must be signed in to change notification settings - Fork 251
quest: watch refusal, auth outage clock, relay auth client CA #4367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
+141
−0
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4e81502
quest: watch refusal, auth outage clock, and relay auth client CA
kixelated 3b0a768
quest: address review on watch refusal, auth outage clock, client CA
kixelated f88701d
quest: keep the LAN-only empty-auth fallback in relay-auth-client-ca
kixelated 5bc5ab1
Merge origin/main into quest/audit-followups
kixelated 255023e
Merge origin/main into quest/audit-followups
kixelated File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| # [M] Auth outage tests on a paused clock | ||
|
|
||
| ## Goal | ||
|
|
||
| The moq-relay and moq-auth outage tests run on tokio's paused clock again and | ||
| assert both bounds: a session (or grant) survives an auth outage until its | ||
| `expires`, and closes at `expires`, not later. No wall-clock sleeps, no | ||
| widened timeouts, and no dependence on how fast the OS delivers loopback. | ||
|
|
||
| ## Plan | ||
|
|
||
| - The tests: `an_outage_keeps_the_session_until_expires` in | ||
| `rs/moq-relay/tests/auth_lifetime.rs` | ||
| ([#4244](https://github.com/moq-dev/moq/pull/4244)) and | ||
| `an_outage_keeps_the_grant_until_expires` in `rs/moq-auth/src/client.rs` | ||
| ([#4291](https://github.com/moq-dev/moq/pull/4291)). Both moved to the real | ||
| clock because a paused clock auto-advances while the runtime waits on a | ||
| real socket, so a virtual timer fired before macOS delivered loopback. That | ||
| swapped one violation of "unit tests mock time" for another, and #4244 | ||
| dropped the upper bound. Read both PR descriptions: they list what was | ||
| tried and why it failed (restoring a listener probe, pausing after setup, | ||
| waiting on the log). | ||
| - The race is real sockets under virtual time, so fix it by taking the | ||
| sockets out of these tests. Look at what the codebase already offers | ||
| before building anything: `rs/moq-net/tests/support/mock.rs` (an in-memory | ||
| session pair), `moq_relay::auth::Auth::embedded` with its `Admissions` | ||
| (decides leases in-process), and the lease driver in moq-auth, which could | ||
| be exercised against an in-process answer source instead of HTTP. If the | ||
| relay's `Connection` or moq-auth's `Client` cannot take such a transport, | ||
| prefer the small seam that lets them over a test-only shim. | ||
| - Decide where each assertion belongs. The outage semantics (a 503 keeps the | ||
| grant until `expires`) are moq-auth's; the relay test may only need to show | ||
| that a lease reaching `expires` closes the session as `Expired` and reports | ||
| `end`. Don't keep two tests proving the same thing. | ||
| - Measure against tokio's clock, not `SystemTime`: the grant still carries a | ||
| wall-clock `expires`, so pin how it maps onto the paused clock. | ||
| - Other paused-clock tests touch real sockets and would share the hazard | ||
| once a timeout lands on their path. #4291's audit named | ||
| `a_grant_within_clock_skew_stays_live` (moq-auth) and | ||
| `fixed_addresses_keep_tls_name_and_request_host` (moq-tokio websocket); | ||
| moq-auth's `clock_server` helper exists only to keep axum on the paused | ||
| clock. Move those onto the same seam if it is cheap. | ||
| - Prove it: loop the tests with every core loaded, on macOS if available, | ||
| and mutate the deadline both ways (close early, close late) to see each | ||
| bound fail. | ||
|
|
||
| Public API: none unless a transport seam is needed; report it if so. Wire: | ||
| none. | ||
|
|
||
| ## Related | ||
|
|
||
| - [More tests under load](/quest/m1/test-flakes-2.md) - the same rule | ||
| applied to other load-only failures | ||
| - [moq-shaper virtual time](/quest/m1/shaper-virtual-time.md) - the same | ||
| paused-clock-versus-real-socket fight in moq-shaper | ||
| - [#4280](https://github.com/moq-dev/moq/pull/4280) - moq-archive and | ||
| moq-hls tests poll with real-clock sleeps, on the archive track-timeline | ||
| line | ||
| - [#4281](https://github.com/moq-dev/moq/pull/4281) - OBS `WaitFor` polling, | ||
| on the C++ line | ||
| - [Nightly 2026-09-26](https://github.com/moq-dev/moq/actions/runs/36240326747/job/108399481809) - | ||
| the macOS relay tarball job failed this test with "publisher connect | ||
| timeout", before #4244 landed |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| # [S] moq-relay auth validate takes the client-CA flag | ||
|
|
||
| ## Goal | ||
|
|
||
| On dev, no caller of `moq_relay::auth::Config` can start with `--auth-public` | ||
| rules alongside a listener TLS client CA by forgetting a check. | ||
| `Config::validate` takes the client-CA flag, `init` requires it too, and | ||
| `validate_client_ca` is gone. The `moq` CLI fails loud on an invalid auth | ||
| config instead of quietly refusing every session. | ||
|
|
||
| ## Plan | ||
|
|
||
| - [#4364](https://github.com/moq-dev/moq/pull/4364) adds an additive | ||
| `Config::validate_client_ca(&self, client_ca: bool)` that `Relay::load` and | ||
| the CLI must each remember to call; the CLI missing the original check is | ||
| the bug it fixes. Fold it into `validate(&self, client_ca: bool)` so every | ||
| caller has to answer, and have `init` take the same answer so a caller that | ||
| skips `validate` still cannot start. `init` today only receives the | ||
| outbound auth TLS, so the listener's client-CA answer is a new input; its | ||
| shape (a bool, or the listener TLS config) is open. Prefer whatever makes | ||
| the wrong call unrepresentable. | ||
| - `spawn_server` in `rs/moq-cli/src/main.rs` maps any `auth.validate()` error | ||
| to `Auth::refuse`. That fallback is only right for a LAN-only mesh with no | ||
| auth configured, which `MoqSide::validate` permits and whose peers admit | ||
| through the cluster. Make that case explicit and let any other error stop | ||
| startup. | ||
| - Update every caller, the tests #4364 added in both `moq-cli` and | ||
| `moq-relay`, and `doc/bin/relay/auth.md` or `doc/lib/rs` wherever they name | ||
| the methods. | ||
|
|
||
| Public API: breaks `moq_relay::auth::Config::validate` and `init`, removes | ||
| `validate_client_ca`, so this targets `dev`. Wire: none. | ||
|
|
||
| ## Required | ||
|
|
||
| - #4364 merged to `main` | ||
| - `dev` has merged `main` after #4364 lands |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # [S] Watch shows a refusal | ||
|
|
||
| ## Goal | ||
|
|
||
| When the origin refuses the broadcast `<moq-watch>` asks for, the player shows | ||
| that refusal as an error instead of sitting offline as if nothing was | ||
| published yet. Refusal stays terminal, as | ||
| [#4230](https://github.com/moq-dev/moq/pull/4230) made it in `@moq/net` to | ||
| match Rust moq-net: the player never re-asks a handler that already said no. | ||
|
|
||
| ## Plan | ||
|
|
||
| - The gap is Codex's P1 on #4230 | ||
| ([r4109909244](https://github.com/moq-dev/moq/pull/4230#discussion_r4109909244)): | ||
| `js/watch/src/broadcast.ts` only watches `request.active`, so after a | ||
| `dynamic()` handler refuses, the request closes with an error that nobody | ||
| reads and `active` stays `undefined` forever. | ||
| - Observe `Requesting.closed` and carry the error into the broadcast's | ||
| state. Whether that is a new `"error"` status, a separate error signal, or | ||
| both is open; mirror how the element already surfaces other terminal | ||
| states, such as the unsupported indicator. Keep the error's message so the | ||
| UI can say why. `unroutable` is also true for a path nothing serves yet, so | ||
| it cannot tell a refusal from offline. | ||
| - What clears the error is part of the design: a fresh request (a new | ||
| `name` or origin, or re-enabling) should be the only way back. No retry | ||
| loop. | ||
| - Cover both the announced and unannounced paths in `#runBroadcast`. | ||
| - Show it in the UI, and update `demo/web` if it consumes the status. Add a | ||
| test in `js/watch` where a `dynamic()` handler refuses and the broadcast | ||
| reports the error. | ||
| - Update `doc/` wherever the watch status values are documented. | ||
|
|
||
| Public API: likely additive (a new status value or error signal on the watch | ||
| broadcast and element). Wire: none. | ||
|
|
||
| ## Related | ||
|
|
||
| - [#4230](https://github.com/moq-dev/moq/pull/4230) - made JS refusals terminal | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this quest chooses the proposed new
"error"status, it widens the publicly exposedBroadcast.out.statusunion in the published@moq/watchpackage, which can break consumers with exhaustive switches or assignments. That option must targetdev; only adding a separate optional error signal is additive, so the quest should distinguish the two instead of labeling both “likely additive.”AGENTS.md reference: AGENTS.md:L59-L61
Useful? React with 👍 / 👎.