Skip to content

moq-relay: scope mTLS grants to the connection URL path#1535

Merged
kixelated merged 3 commits into
mainfrom
claude/upbeat-roentgen-bc33d1
May 29, 2026
Merged

moq-relay: scope mTLS grants to the connection URL path#1535
kixelated merged 3 commits into
mainfrom
claude/upbeat-roentgen-bc33d1

Conversation

@kixelated

@kixelated kixelated commented May 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • mTLS-authenticated peers were granted AuthToken::unrestricted() with an empty root, so a publisher dialing /demo announced at the cluster root (e.g. tos.hang) instead of demo/tos.hang. Path-scoped subscribers (demo/*) never saw the broadcast.
  • AuthToken::unrestricted(root) now takes a root derived from the connection URL path, the same scoping a JWT's root claim gets. All four mTLS call sites pass the request path: QUIC (connection.rs), WebSocket (websocket.rs), and the HTTP serve_announced / serve_fetch handlers (web.rs).
  • Dropped stale doc/comment claims that node identity comes from the cert's DNS SAN and that ?register= is consulted. The mTLS path validates only the cert chain; node identity comes from --cluster-mesh.
  • Simplified the mTLS presence check in moq-native: the empty PeerIdentity struct was a data-less presence flag and peer_identity() -> Result<Option<PeerIdentity>> only existed to guard a downcast that can't fail with rustls. Replaced it with Request::has_peer_certificate() -> bool, removing the struct, the downcast, and the Result plumbing at the call site.

Why it's safe for the cluster mesh

Cluster peers dial https://{remote}/, and / normalizes to an empty root, so relay-to-relay keeps unscoped, cluster-wide access exactly as before. Only path-bearing dials get scoped, which is the fix.

Branch targeting

Targeting main: this is a bug fix that preserves the wire protocol and cluster behavior. The public AuthToken::unrestricted() and Request::peer_identity() signatures change, but both are relay/native-internal with no external consumers. Happy to retarget dev if a reviewer prefers.

Test plan

  • cargo test -p moq-relay --lib (93 passed; added unrestricted_scopes_to_root and unrestricted_empty_root_is_unscoped)
  • cargo clippy -p moq-native -p moq-relay --all-targets clean
  • cargo build -p moq-native -p moq-relay clean
  • Manual: confirm an mTLS publisher dialing /demo announces under demo/ and a demo/* subscriber sees it; confirm cluster mesh (dialing /) still gets cluster-wide access

🤖 Generated with Claude Code

(Written by Claude)

kixelated and others added 2 commits May 28, 2026 19:43
mTLS-authenticated peers were granted AuthToken::unrestricted() with an
empty root, so a publisher dialing /demo announced at the cluster root
(tos.hang) instead of demo/tos.hang. Path-scoped subscribers (demo/*)
never saw it.

Make unrestricted() take a root derived from the URL path, the same
scoping a JWT's root claim gets. Cluster peers dial "/", which normalizes
to an empty root, so they keep unscoped cluster-wide access. All four
mTLS call sites (QUIC, WebSocket, serve_announced, serve_fetch) now pass
the request path.

Also drop the stale doc/comment claims that node identity comes from the
cert's DNS SAN and that ?register= is consulted: the mTLS path validates
only the cert chain (PeerIdentity is empty) and node identity comes from
--cluster-mesh.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… bool

PeerIdentity was a data-less struct used only as a presence flag, and the
Option<PeerIdentity> return existed solely to guard a downcast that can't
fail with rustls. Since the only signal we need is "did the peer present a
CA-validated client cert", collapse it to a plain bool and drop the struct,
the downcast, and the now-unused Result plumbing at the call site.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4cc38c4d-242e-43ea-8b57-172103bdfc56

📥 Commits

Reviewing files that changed from the base of the PR and between 13f9d54 and c00ae1d.

📒 Files selected for processing (1)
  • doc/bin/relay/auth.md
✅ Files skipped from review due to trivial changes (1)
  • doc/bin/relay/auth.md

Walkthrough

This PR refactors mTLS authentication to scope access by URL path instead of granting unrestricted cluster access. The moq-native request interface is simplified from extracting a PeerIdentity type to a direct boolean has_peer_certificate() check. The AuthToken::unrestricted() method now requires an explicit path parameter, enabling path-scoped tokens. The relay's connection handling and service endpoints (web, WebSocket) are updated to derive and propagate the request URL path when constructing mTLS authentication tokens. Documentation is updated to clarify the new path-scoped access model and remove references to certificate-based peer identity extraction.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title directly matches the main objective: scoping mTLS grants to the connection URL path to fix the bug where publishers dialing non-root paths announced at the cluster root instead.
Description check ✅ Passed The PR description comprehensively explains the bug, fix, design rationale, safety considerations for cluster mesh, test coverage, and explicitly references all modified files with clear intent.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch claude/upbeat-roentgen-bc33d1

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

Comment @coderabbitai help to get the list of available commands and usage tips.

@kixelated kixelated enabled auto-merge (squash) May 29, 2026 02:49

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@doc/bin/relay/auth.md`:
- Around line 187-191: The phrase "cluster privileges" is ambiguous—edit the
paragraph around the URL-root explanation so it clearly states that the
CA-signed certificate grants scoped publish/subscribe access based on the
URL-derived root and sets an internal token marker, not a separate permission
model; update wording that references "cluster privileges" to explain it's an
unscoped (empty root) internal marker for cluster nodes (examples: peer dialing
"/demo" → access under "demo/"; peer dialing "/" → empty root/unscoped but still
marked internal), and remove any implication of an additional "cluster
privilege" permission layer.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 01ad818f-45f9-4ba4-882c-3816699fb941

📥 Commits

Reviewing files that changed from the base of the PR and between ad5b3ed and 13f9d54.

📒 Files selected for processing (7)
  • doc/bin/relay/auth.md
  • rs/moq-native/src/quinn.rs
  • rs/moq-native/src/server.rs
  • rs/moq-relay/src/auth.rs
  • rs/moq-relay/src/connection.rs
  • rs/moq-relay/src/web.rs
  • rs/moq-relay/src/websocket.rs

Comment thread doc/bin/relay/auth.md Outdated
"plus cluster privileges" implied a separate permission model. The token
just gets full pub/sub within the URL-derived root and an internal flag
that only selects the billing stats tier.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@kixelated kixelated merged commit b7bfc31 into main May 29, 2026
1 check passed
@kixelated kixelated deleted the claude/upbeat-roentgen-bc33d1 branch May 29, 2026 03:20
This was referenced May 29, 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.

1 participant