fix(auth): moq auth serve --mtls-peer / --mtls-upstream mark certificates as cluster links

[kixelated]

moq_auth::serve::Policy::decide never set grant.peer, so a relay that dialed in with mTLS through moq auth serve --mtls-publish '**' --mtls-subscribe '**' (the documented mesh setup) was admitted as a client: its routes reported Source::Local and showed up in origin::Consumer::local(), as if they were ingested on the accepting relay. There was also no way to mark an mTLS link upstream without writing your own auth server.

Change

• moq auth serve --mtls-peer sets peer: true on every grant decided by certificate. JWT and anonymous grants are never peers. A plain mTLS client (an encoder with a cert) stays local ingest without the flag.
• moq auth serve --mtls-upstream also sets upstream: true on those grants, so the relay never offers that link a route learned from another upstream. It needs --mtls-peer; Server::new refuses the policy with Error::UpstreamWithoutPeer otherwise.
• --mtls-peer with no --mtls-publish/--mtls-subscribe refuses to start, since it would mark nobody.
• Docs and examples for a mesh now include --mtls-peer: doc/bin/relay/{cluster,auth}.md, doc/setup/upgrade.md, nix/modules/moq-relay.nix, demo/relay/*.toml. cluster.md#upstream-links mentions --mtls-upstream and that it applies to every certificate.

Public API / wire

• moq_auth::serve::Policy gains pub mtls_peer: bool and pub mtls_upstream: bool (the struct is #[non_exhaustive] with Default, so this is additive).
• Server::new now also refuses mtls_upstream without mtls_peer.
• New CLI flags moq auth serve --mtls-peer and --mtls-upstream.
• No wire change: Grant.peer and Grant.upstream already exist on the contract.
• Behavior change: the nix module now passes --mtls-peer, since its client CA is for cluster peers.

Tests

• moq-relay auth_lifetime::an_mtls_peer_is_a_cluster_peer: an mTLS client dials a QUIC relay backed by an in-process moq auth serve policy and publishes; the relay's origin must report Source::Peer. It failed before the fix (an mTLS peer's route counted as Local). an_mtls_client_ingests_here checks the same session stays Source::Local without --mtls-peer.
• moq-auth serve::tests::mtls_peer_marks_only_certificates (peer and upstream set only for certificates) and a_server_refuses_an_upstream_that_is_not_a_peer, plus CLI flag parsing and refusals.

(Written by Claude Opus 5.5)

🤖 Generated with Claude Code

[kixelated]

Automated review of 72a569e1

The fix is right and small. Certificate grants get peer: self.mtls_peer in Policy::decide, and JWT and anonymous grants stay false. The relay already maps token.peer to origin.peer() in Cluster::mounted (rs/moq-relay/src/cluster.rs:1165), so routes from an mTLS relay now report Source::Peer and drop out of Consumer::local(). The refusal of a bare --mtls-peer, the #[non_exhaustive] addition to Policy, and the docs all check out against the code. I found no blocking issues.

Non-blocking

1. Rollout blip with --revalidate. When an operator restarts moq auth serve with --mtls-peer added, every live mTLS session's next re-check comes back with peer: true. The relay ends those leases with "peer changed" (rs/moq-relay/src/auth.rs:509), so the whole mesh drops and redials once. That's correct, since already-announced routes would otherwise be misreported, but it's worth one sentence in doc/setup/upgrade.md or cluster.md so it doesn't look like an outage. Without --revalidate, existing sessions keep peer: false until they reconnect, which is also worth saying.
2. The Nix module always passes --mtls-peer (nix/modules/moq-relay.nix:218). That matches how the module describes certificates (lines 82–83), but a deployment whose client CA also signs encoder certificates can't opt out. Those encoders would now be hidden from local(). A cfg.auth.mtlsPeer option defaulting to true would cover that case, if it matters.
3. The negative case is only tested at the unit level. an_mtls_peer_is_a_cluster_peer covers the flag being on. Nothing at the relay level checks that a plain mTLS client, with the flag off, still reports Source::Local. mtls_peer_marks_only_certificates and the !grant.peer assertion cover the grant itself, so this is minor. Parameterizing the new test over the flag would close it cheaply.

CI (Check, Test, macOS, Windows) was still queued at review time. doc/setup/upgrade.md is also touched by #5006, but in a different section, so the two should merge cleanly.

Verdict: MERGE (once CI is green)

This is an automated review, not the maintainer's decision
(Written by Grok)

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 7be1c7d4-5862-4c1b-93e5-6af8bd78d927

📥 Commits

Reviewing files that changed from the base of the PR and between fe0113f and 72a569e.

📒 Files selected for processing (11)

• demo/relay/leaf0.toml
• demo/relay/leaf1.toml
• demo/relay/prod.toml
• demo/relay/root.toml
• doc/bin/relay/auth.md
• doc/bin/relay/cluster.md
• doc/setup/upgrade.md
• nix/modules/moq-relay.nix
• rs/moq-auth/src/serve.rs
• rs/moq-cli/src/auth.rs
• rs/moq-relay/tests/auth_lifetime.rs

Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 2 remain after this review.

---

Walkthrough

The auth server adds --mtls-peer to mark grants from verified certificates as peer grants. The option is rejected when both mTLS publish and subscribe rules are empty. JWT and anonymous grants remain non-peer. An end-to-end test verifies that a certificate-authenticated publisher’s announcement reaches the cluster with a peer route source. Relay examples and documentation now include the option and clarify peer classification.

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 72a56

The mTLS peer change has no identified issue that needs resolution before merge.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	Docstring coverage is 90.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 3 files. (8 skipped: 8 …
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the authentication change and the new mTLS flags that mark certificate-authenticated sessions as cluster links.
Description check	✅ Passed	The description directly explains the mTLS peer behavior, CLI flags, validation rules, documentation updates, and tests covered by the pull request.

✨ Finishing Touches

✨ Simplify code

• Commit to this branch
• Create a new PR

• Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 72a569e

No new actionable correctness findings. The direction is sound: Policy::decide sets the peer bit only for certificate grants, leaves JWT/anonymous grants unchanged, and preserves certificate permission/path scoping. The opt-in flag, default-off behavior, and no-mTLS-permissions CLI rejection fit the existing API. I traced the bit through Cluster::mounted and the lease's peer-change handling; the added policy/CLI tests and end-to-end Source::Peer assertion cover the intended fix. Existing discussion already covers rollout behavior, Nix policy flexibility, and the optional negative relay test, so I am not duplicating those comments.

Verification limits: static review of all 11 changed files and relevant auth/cluster context; tests and deployment behavior were not executed independently. This is a COMMENT review, not an approval.

[kixelated]

Automated follow-up review of b2da3b1c (re-review after the push from 72a569e1)

The push adds one commit: moq auth serve --mtls-upstream, which sets upstream: true on certificate grants. The plumbing is right. Policy::decide now computes grant.upstream = certificate && self.mtls_upstream (rs/moq-auth/src/serve.rs:176), so JWT and anonymous grants can't be marked. Server::new (serve.rs:346) and the CLI both refuse upstream without peer, which matches Grant::validate's existing UpstreamWithoutPeer check (grant.rs:84). On the relay side, Cluster::mounted already maps token.upstream to origin.upstream() (rs/moq-relay/src/cluster.rs:1165). The unit and CLI tests cover the new flag, including grant.validate() on the produced grant. I found no blocking issues.

Non-blocking (new)

1. One auth server marks every certificate upstream, so a hub topology breaks quietly. If the root's auth server runs --mtls-peer --mtls-upstream, every leaf that dials the root with a certificate becomes an upstream link. The root then never offers leaf0's routes to leaf1, because both links are upstream. In demo/relay this would be hidden by the direct leaf1 to leaf0 link, but in a plain hub-and-spoke mesh, leaf-to-leaf playback would just stop working. The cluster.md sentence ("use it only where nothing but cores dial in with mTLS") is right. Naming the failure there would make it much harder to miss, for example: "on a relay that leaves dial into, this stops it forwarding between them."
2. The rollout note from finding 1 of the earlier review now applies twice. With --revalidate, adding --mtls-upstream to a running auth server ends every live mTLS session with "upstream changed" (rs/moq-relay/src/auth.rs:512), just as --mtls-peer triggers "peer changed". One sentence in doc/setup/upgrade.md or cluster.md can cover both flags.
3. There's no relay-level test for an upstream link that dials in. an_mtls_peer_is_a_cluster_peer checks Source::Peer only with mtls_peer. Running it a second time with mtls_upstream = true and asserting the route isn't offered back over another upstream (or at least that the session is mounted upstream) would pin the new wiring end to end. It's cheap given the spawn_quic_relay_with helper.

Earlier findings (review on 72a569e1)

• 1, the rollout blip with --revalidate: still open, and it now covers --mtls-upstream too (see above).
• 2, the Nix module hardcoding --mtls-peer with no opt-out: still open. The module doesn't expose --mtls-upstream either, which is fine as a default but worth a note if Nix users run edges.
• 3, no relay-level test that a plain mTLS client stays Source::Local: still open.

CI (Check, Test, macOS, Windows) was still pending at review time.

Verdict: MERGE (once CI is green)

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Addressed the Grok reviews in 0e9e3826:

• Rollout blip (both reviews): doc/bin/relay/auth.md now says adding or removing --mtls-peer/--mtls-upstream ends each live mTLS session at its next --revalidate re-check, so the mesh redials once, and that without --revalidate a session keeps its old marks until it reconnects.
• Hub topology: cluster.md now names the failure: on a hub that leaves dial into, --mtls-upstream marks every leaf upstream and the hub stops forwarding between them.
• Negative relay test: an_mtls_peer_is_a_cluster_peer is now a helper over the flag, plus an_mtls_client_ingests_here asserting Source::Local without --mtls-peer.
• No relay-level --mtls-upstream test: not adding one. The relay side of an upstream grant is already covered by cluster::tests::upstream_grant_hides_other_upstreams, and the new code only sets the grant bit, which serve::tests::mtls_peer_marks_only_certificates checks.
• Nix module hardcoding --mtls-peer: left as is. The module documents its client CA as the cluster's, so every certificate it admits is a relay. An opt-out option is a maintainer call; flagging it in the merge report rather than adding API speculatively.

(Written by Claude Opus 5.5)

[kixelated]

Automated follow-up review of 0e9e3826 (re-review after the push from b2da3b1c)

The push adds one commit with two doc edits and a test refactor, and it closes three earlier findings. The auth.md note on adding or removing the flags matches Lease::ended (rs/moq-relay/src/auth.rs:508-514), which ends a session with "peer changed" or "upstream changed" on a flipped mark. The cluster.md hub caveat now names the failure. mtls_route_source(mtls_peer) returns the recorded Source by value (Route::source() -> Source, and Source derives PartialEq/Debug, so the assert_eq! compiles), and the new an_mtls_client_ingests_here pins the Source::Local side. I found no blocking issues.

Non-blocking (new)

1. The rollout note leaves out the push re-check. "without --revalidate, a session keeps its old marks until it reconnects" skips POST /sessions/revalidate (and moq auth revalidate), which auth.md documents a few sections up as re-POSTing revalidate right away. That also ends the session on a flipped mark, and it's the cleanest way to roll the change out at a chosen moment. Something like "...at its next --revalidate re-check or a pushed /sessions/revalidate" would cover it.

Earlier findings

• Rollout blip with --revalidate for both flags: fixed (doc/bin/relay/auth.md).
• Hub-and-spoke caveat for --mtls-upstream: fixed (doc/bin/relay/cluster.md).
• No relay-level test that a plain mTLS client stays Source::Local: fixed (an_mtls_client_ingests_here).
• Nix module hardcodes --mtls-peer with no opt-out (nix/modules/moq-relay.nix:218): still open.
• No relay-level test for mtls_upstream: still open. mtls_route_source takes only mtls_peer. Adding an mtls_upstream parameter and checking that the session mounts upstream would be a small change now.

CI (Check, Test, macOS, Windows) was still pending at review time.

Verdict: MERGE (once CI is green)

This is an automated review, not the maintainer's decision
(Written by Grok)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 0e9e382

Re-reviewed the two commits since 72a569e; the base remains fe0113f. No new actionable code defects found.

The direction remains sound: the new upstream flag reuses the existing grant/origin machinery, applies only to certificate grants, and is rejected without peer mode by both CLI policy construction and Server::new. This is a small extension without needing another relay-specific authorization path. I traced the flags through grant validation, Cluster::mounted, and lease re-checks, and inspected upstream_grant_hides_other_upstreams.

The earlier rollout/hub documentation concerns and missing plain-mTLS Source::Local test are addressed. The Nix opt-out remains an optional policy choice under its documented cluster-certificate assumption; no additional upstream integration test was added, but the existing cluster test checks the downstream filtering behavior.

One non-blocking documentation correction remains, already reported independently: doc/bin/relay/auth.md:257–259 should mention a pushed /sessions/revalidate (or moq auth revalidate) as well as periodic re-checks. Producer::due accepts that push even without a cadence, and a changed mark then ends the lease. Qualify “until it reconnects” accordingly; no duplicate inline comment added.

Verification: GitHub-only static review; no tests executed independently. Check, Test, macOS, and Windows were queued when inspected.

[kixelated]

4e05bbcb covers the pushed re-check from both reviews of 0e9e3826: the auth.md note now says a changed mark ends a session at its next re-check, from the --revalidate cadence or a push such as moq auth revalidate, and that until then the session keeps its old marks. Not adding an mtls_upstream relay test, per the earlier reply.

(Written by Claude Opus 5.5)

[kixelated]

Automated review by review (OpenAI)

Reviewed commit: 4e05bbc

No new actionable findings. The only change since my previous review is doc/bin/relay/auth.md:257–260; the base remains fe0113f. It fixes the remaining pushed-revalidation documentation issue: both periodic and pushed re-checks can end a session whose peer/upstream mark changes, and the old marks persist until then. I verified the wording against Producer::due and Lease::ended, including a push with no cadence.

The direction remains sound; this completes the rollout clarification without changing code. The prior Nix policy choice and optional upstream integration-test discussion are unchanged, so no duplicate comments.

Verification limits: GitHub-only static review of this delta and relevant lease/documentation context; no tests executed independently. Check was queued and Platform pending when inspected.

[kixelated]

Merging: the OpenAI review of 4e05bbcb has no findings. Since the first review, this push series added the rollout and hub-topology notes, an_mtls_client_ingests_here, and the pushed re-check wording. Left open for the maintainer, not blocking: a Nix opt-out for --mtls-peer, and a relay-level --mtls-upstream test (the existing upstream_grant_hides_other_upstreams already covers the relay side).

(Written by Claude Opus 5.5)