Fold the internal listener into --server-bind (one authenticated accept path)#1974
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR adds TCP and Unix stream listeners to moq-native, with stream-only operation when no QUIC bind is configured. URL-less relay requests now parse auth parameters from the SETUP path, and stream connections use the same JWT/public-access rules as QUIC. The relay’s internal listener module and 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches✨ Simplify code
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. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kixelated, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
… fallback) The internal TCP/Unix listeners no longer grant unauthenticated full access. Every accepted connection is authorized through the relay's Auth: - a JWT carried in the moq-lite-05 SETUP path (`/broadcast?jwt=<token>`) is verified and scopes the session, exactly as a native QUIC client; - a no-JWT connection with NO path gets a fixed `anon` subtree if one is configured (--internal-anon, e.g. `.stats` for a local telemetry publisher), else it is rejected; - a no-JWT connection WITH a path resolves anonymous/public access for that path (tokenless public playback). This is the per-user authorization boundary for out-of-process workers (the RTMP/SRT/WHIP/WHEP gateways): the relay, not the worker, enforces the scope, so a memory-safety bug in a gateway parser can reach only what its users' tokens permit. There is no default unauthenticated full-access path anymore -- the old "unrestricted internal" behaviour is reproducible only by explicitly setting `--internal-anon ""` (the empty root). - internal.rs: collapse the prior unrestricted + separate-authenticated listeners into one authenticated path. `run_internal` takes the relay's Auth; a single `spawn_session` verifies/anon-scopes. The listeners offer the default versions plus moq-lite-05 (whose SETUP carries the request path); JWT clients negotiate lite-05, no-JWT/anon clients work on any version. - auth.rs: `AuthParams::from_path` (splits `/broadcast?jwt=<token>`) and `AuthToken::anon(prefix)` (relay-fixed narrow scope), with tests. - smoke tests: the internal round-trip helpers set `--internal-anon ""` to keep exercising the no-JWT transport path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kixelated
force-pushed
the
claude/uds-jwt-auth
branch
from
a93105c to
68e7e08
Compare
Replace the separate `--internal-*` listeners with additional `--server-bind`
schemes, so there is one authenticated accept path for every transport.
`moq_native::ServerConfig::bind` becomes a repeatable list. Each entry selects a
transport by URL scheme: `udp://` (QUIC, the default), `tcp://` and `unix://`
(plaintext qmux for trusted local workers). `moq_native::Server` now owns the
stream listeners too, exposing the SETUP path, peer credentials, and the bind
URL query on `Request` so the application authenticates them exactly like QUIC.
The relay drops `internal.rs`/`InternalConfig` entirely; `Connection` resolves a
token for stream transports from the in-band `path?jwt=`, with the per-listener
anon scope and peer-credential allowlist parsed from the bind URL query
(`?anon=.stats&allow-uid=1001`). Since the internal listener is already
authenticated, it no longer needs its own door.
Breaking change to `moq-native`: `ServerConfig.bind` is now `Vec<String>` (a
single string / the `listen` alias still deserialize via OneOrMany), and
`quic_bind()` / `Request::{path,peer_cred,listen_query}` / `PeerCred` are new
public surface.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kixelated
changed the title
Manual patch bump: the --internal -> --server-bind refactor changes the CLI (release-plz can't detect binary CLI changes), and moq-relay is patch-only with no external consumers. The moq-native library bump is left to release-plz. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@rs/moq-native/src/error.rs`:
- Around line 49-56: The exported Error variants InvalidBind, MultipleQuicBinds,
and UnsupportedBind are public API symbols but currently lack rustdoc. Add doc
comments to each variant in error.rs, using their variant names as the anchor,
and describe the meaning of each error in a concise, user-facing way; keep the
existing #[error(...)] strings unchanged since they are not a substitute for
documentation.
In `@rs/moq-native/src/quinn.rs`:
- Around line 377-381: `QuinnServer::new` is swallowing `quic_bind()` failures
by using `config.quic_bind().ok().flatten()`, which causes invalid bind input to
fall back to `DEFAULT_BIND`. Update the bind resolution path in
`QuinnServer::new` to preserve and return the `quic_bind()` error instead of
converting it to `None`, so direct callers of the public constructor cannot
bypass validation or bind the wrong address.
In `@rs/moq-native/src/server.rs`:
- Around line 284-288: `server.rs` is choosing `backend` before it knows whether
QUIC is actually needed, which makes the no-QUIC fallback panic in stream-only
builds. In `run_server` (or the surrounding backend चयन logic), move the
`config.quic_bind()` / `build_quic` decision ahead of backend selection, then
gate the QUIC backend requirement on `build_quic` being true. Keep the
stream-only `tcp`/`uds` path able to proceed without selecting or requiring a
QUIC backend, and use the existing `quic_bind` and `build_quic` symbols to drive
the conditional.
- Around line 640-648: The new public PeerCred type is currently exhaustive,
which would make adding more platform-specific credential fields later a
breaking change for external struct literals. Update the PeerCred definition
itself to be non-exhaustive while keeping its existing API shape and fields
intact, so future credential expansion can be done safely without breaking
callers that use PeerCred directly.
- Around line 816-842: The stream SETUP handshake in spawn_stream_request can
wait forever on server.accept_request(session).await, so wrap that await in a
timeout and treat timeout as a handshake failure. Add the timeout handling
inside the tokio::spawn block around accept_request, and keep the existing
tracing::debug! failure path for both timeout and other errors so slow peers are
dropped before they can accumulate tasks and sockets.
- Around line 730-764: The ensure_started() flow in server.rs is spawning accept
loops too early, so a later bind failure leaves already-started listeners
running with rx unset. Change ensure_started() to first bind and collect all
listener/loop state for every StreamTransport case, and only after all binds
succeed assign self.rx and spawn the tcp/unix loops. Keep the fix localized
around ensure_started(), spawn_tcp_loop, and spawn_unix_loop so no partial
startup can occur.
In `@rs/moq-relay/Cargo.toml`:
- Line 8: Update the moq-relay package version to the next minor release because
removing Config.internal and the internal listener/config CLI surface is a
breaking API change. Change the version in the Cargo.toml for moq-relay from the
current 0.13.1 to a 0.14.0-style release so crates.io consumers see the correct
compatibility signal; use the package metadata entry that defines moq-relay’s
version.
In `@rs/moq-relay/src/connection.rs`:
- Around line 232-247: The allowlist parsing in the query handling logic
silently ignores malformed `allow-uid`, `allow-gid`, and `allow-pid` entries, so
update the parser in `connection.rs` to fail closed instead of using
`filter_map(...parse().ok())`. In the query-processing path that builds `policy`
(the `for (key, value) in url::form_urlencoded::parse(...)` match), validate
each comma-separated value and return an auth/config error if any entry is
invalid or if an allowlist key is present but yields no valid IDs. Add a
regression test covering malformed and empty allowlist values to ensure
peer-credential enforcement is not bypassed.
- Around line 146-165: The URL-first branch in connection::authenticate skips
mTLS for URL-less QUIC requests because raw QUIC url() can be None, causing
peer_identity() to be ignored and falling through to authenticate_stream().
Update the dispatch in the authenticate flow to derive auth params from the
SETUP path when no URL is present, then still apply the mTLS check/override via
verify_mtls and peer_identity() before falling back to JWT/public verification.
In `@rs/moq-relay/src/main.rs`:
- Around line 28-29: The server address handling in main currently uses
server.local_addr().ok(), which suppresses all errors and makes real backend or
address failures look like “stream transports only.” Update the logic around
server.local_addr() to pattern-match the result: return None only for
moq_native::Error::NoBackend("no QUIC listener configured"), and preserve/report
all other errors instead of converting them to None.
🪄 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: c9e05efb-98e3-457e-b6b9-c6e26ff247ef
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
doc/bin/relay/auth.mddoc/bin/relay/config.mdrs/moq-cli/src/main.rsrs/moq-ffi/src/server.rsrs/moq-native/src/error.rsrs/moq-native/src/noq.rsrs/moq-native/src/quiche.rsrs/moq-native/src/quinn.rsrs/moq-native/src/server.rsrs/moq-native/src/tls.rsrs/moq-native/src/unix.rsrs/moq-native/tests/alpn.rsrs/moq-native/tests/backend.rsrs/moq-native/tests/broadcast.rsrs/moq-relay/Cargo.tomlrs/moq-relay/src/auth.rsrs/moq-relay/src/config.rsrs/moq-relay/src/connection.rsrs/moq-relay/src/internal.rsrs/moq-relay/src/lib.rsrs/moq-relay/src/main.rsrs/moq-relay/tests/smoke.rsrs/moq-rtmp/bin/moq-rtmp.rs
💤 Files with no reviewable changes (2)
- rs/moq-relay/src/internal.rs
- rs/moq-relay/src/lib.rs
Comment on lines
+49
to
+56
| #[error("invalid --server-bind entry: {0}")] | ||
| InvalidBind(String), | ||
|
|
||
| #[error("more than one QUIC (udp://) --server-bind entry; Quinn binds a single address")] | ||
| MultipleQuicBinds, | ||
|
|
||
| #[error("--server-bind {0} requested a transport whose feature is not compiled in")] | ||
| UnsupportedBind(String), |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add rustdoc for the new public error variants.
The #[error(...)] text is not rustdoc, so these exported Error variants are currently undocumented.
As per coding guidelines, “Document every exported public API symbol: each exported Rust item and each exported JS/TS symbol, including notable public members, must have a doc comment.”
Proposed fix
+ /// A `--server-bind` entry could not be parsed.
#[error("invalid --server-bind entry: {0}")]
InvalidBind(String),
+ /// More than one QUIC bind address was configured.
#[error("more than one QUIC (udp://) --server-bind entry; Quinn binds a single address")]
MultipleQuicBinds,
+ /// A `--server-bind` entry requested a transport disabled at compile time.
#[error("--server-bind {0} requested a transport whose feature is not compiled in")]
UnsupportedBind(String),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[error("invalid --server-bind entry: {0}")] | |
| InvalidBind(String), | |
| #[error("more than one QUIC (udp://) --server-bind entry; Quinn binds a single address")] | |
| MultipleQuicBinds, | |
| #[error("--server-bind {0} requested a transport whose feature is not compiled in")] | |
| UnsupportedBind(String), | |
| /// A `--server-bind` entry could not be parsed. | |
| #[error("invalid --server-bind entry: {0}")] | |
| InvalidBind(String), | |
| /// More than one QUIC bind address was configured. | |
| #[error("more than one QUIC (udp://) --server-bind entry; Quinn binds a single address")] | |
| MultipleQuicBinds, | |
| /// A `--server-bind` entry requested a transport disabled at compile time. | |
| #[error("--server-bind {0} requested a transport whose feature is not compiled in")] | |
| UnsupportedBind(String), |
🤖 Prompt for 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.
In `@rs/moq-native/src/error.rs` around lines 49 - 56, The exported Error variants
InvalidBind, MultipleQuicBinds, and UnsupportedBind are public API symbols but
currently lack rustdoc. Add doc comments to each variant in error.rs, using
their variant names as the anchor, and describe the meaning of each error in a
concise, user-facing way; keep the existing #[error(...)] strings unchanged
since they are not a substitute for documentation.
Source: Coding guidelines
Comment on lines
+730
to
+764
| /// Bind the configured listeners and spawn their accept loops, once. | ||
| async fn ensure_started(&mut self) -> crate::Result<()> { | ||
| if self.rx.is_some() || self.binds.is_empty() { | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let (tx, rx) = tokio::sync::mpsc::channel(16); | ||
| for bind in self.binds.drain(..) { | ||
| let versions = self.versions.clone(); | ||
| match bind.transport { | ||
| #[cfg(feature = "tcp")] | ||
| StreamTransport::Tcp(addr) => { | ||
| if !addr.ip().is_loopback() { | ||
| tracing::warn!(%addr, "tcp listener bound to a non-loopback address; qmux is UNENCRYPTED, ensure the network is trusted"); | ||
| } | ||
| let listener = crate::tcp::Listener::bind(addr).await?.with_protocols(versions.alpns()); | ||
| tracing::info!(%addr, "listening (tcp)"); | ||
| spawn_tcp_loop(listener, versions, bind.query, tx.clone()); | ||
| } | ||
| #[cfg(all(feature = "uds", unix))] | ||
| StreamTransport::Unix(path) => { | ||
| let listener = crate::unix::Listener::bind(&path) | ||
| .await? | ||
| .with_protocols(versions.alpns()); | ||
| // Loose file perms: callers gate via peer credentials, and the | ||
| // worker usually runs as a different user than the server. | ||
| listener.set_mode(0o666)?; | ||
| tracing::info!(path = %path.display(), "listening (unix)"); | ||
| spawn_unix_loop(listener, versions, bind.query, tx.clone()); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| self.rx = Some(rx); | ||
| Ok(()) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t spawn partial stream listeners before all binds succeed.
ensure_started() spawns each listener as it is bound, but if a later bind fails, earlier spawned loops keep running while rx is dropped and accept() returns None. Bind all listeners first, then publish rx and spawn accept loops only after every bind succeeds.
🤖 Prompt for 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.
In `@rs/moq-native/src/server.rs` around lines 730 - 764, The ensure_started()
flow in server.rs is spawning accept loops too early, so a later bind failure
leaves already-started listeners running with rx unset. Change ensure_started()
to first bind and collect all listener/loop state for every StreamTransport
case, and only after all binds succeed assign self.rx and spawn the tcp/unix
loops. Keep the fix localized around ensure_started(), spawn_tcp_loop, and
spawn_unix_loop so no partial startup can occur.
Comment on lines
+816
to
+842
| /// Read the SETUP from an accepted stream session (concurrently, so one slow or | ||
| /// malicious peer doesn't stall the listener) and forward the resulting request. | ||
| #[cfg(any(feature = "tcp", all(feature = "uds", unix)))] | ||
| fn spawn_stream_request( | ||
| session: qmux::Session, | ||
| peer_cred: Option<PeerCred>, | ||
| transport: &'static str, | ||
| query: Option<String>, | ||
| versions: moq_net::Versions, | ||
| tx: tokio::sync::mpsc::Sender<Request>, | ||
| ) { | ||
| tokio::spawn(async move { | ||
| let server = moq_net::Server::new().with_versions(versions); | ||
| match server.accept_request(session).await { | ||
| Ok(request) => { | ||
| let request = Request { | ||
| server: moq_net::Server::new(), | ||
| kind: RequestKind::Stream(Box::new(StreamRequest { | ||
| request, | ||
| transport, | ||
| peer_cred, | ||
| query, | ||
| })), | ||
| }; | ||
| let _ = tx.send(request).await; | ||
| } | ||
| Err(err) => tracing::debug!(%err, "stream SETUP handshake failed"), |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout around the stream SETUP handshake.
Each accepted TCP/Unix connection spawns a task that can wait indefinitely in accept_request(session).await. Slow peers can accumulate unbounded tasks and sockets before authentication.
Proposed fix
tokio::spawn(async move {
let server = moq_net::Server::new().with_versions(versions);
- match server.accept_request(session).await {
+ match tokio::time::timeout(std::time::Duration::from_secs(10), server.accept_request(session)).await {
+ Ok(Ok(request)) => {
- Ok(request) => {
let request = Request {
server: moq_net::Server::new(),
kind: RequestKind::Stream(Box::new(StreamRequest {
request,
transport,
peer_cred,
query,
})),
};
let _ = tx.send(request).await;
}
- Err(err) => tracing::debug!(%err, "stream SETUP handshake failed"),
+ Ok(Err(err)) => tracing::debug!(%err, "stream SETUP handshake failed"),
+ Err(_) => tracing::debug!("stream SETUP handshake timed out"),
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Read the SETUP from an accepted stream session (concurrently, so one slow or | |
| /// malicious peer doesn't stall the listener) and forward the resulting request. | |
| #[cfg(any(feature = "tcp", all(feature = "uds", unix)))] | |
| fn spawn_stream_request( | |
| session: qmux::Session, | |
| peer_cred: Option<PeerCred>, | |
| transport: &'static str, | |
| query: Option<String>, | |
| versions: moq_net::Versions, | |
| tx: tokio::sync::mpsc::Sender<Request>, | |
| ) { | |
| tokio::spawn(async move { | |
| let server = moq_net::Server::new().with_versions(versions); | |
| match server.accept_request(session).await { | |
| Ok(request) => { | |
| let request = Request { | |
| server: moq_net::Server::new(), | |
| kind: RequestKind::Stream(Box::new(StreamRequest { | |
| request, | |
| transport, | |
| peer_cred, | |
| query, | |
| })), | |
| }; | |
| let _ = tx.send(request).await; | |
| } | |
| Err(err) => tracing::debug!(%err, "stream SETUP handshake failed"), | |
| /// Read the SETUP from an accepted stream session (concurrently, so one slow or | |
| /// malicious peer doesn't stall the listener) and forward the resulting request. | |
| #[cfg(any(feature = "tcp", all(feature = "uds", unix)))] | |
| fn spawn_stream_request( | |
| session: qmux::Session, | |
| peer_cred: Option<PeerCred>, | |
| transport: &'static str, | |
| query: Option<String>, | |
| versions: moq_net::Versions, | |
| tx: tokio::sync::mpsc::Sender<Request>, | |
| ) { | |
| tokio::spawn(async move { | |
| let server = moq_net::Server::new().with_versions(versions); | |
| match tokio::time::timeout(std::time::Duration::from_secs(10), server.accept_request(session)).await { | |
| Ok(Ok(request)) => { | |
| let request = Request { | |
| server: moq_net::Server::new(), | |
| kind: RequestKind::Stream(Box::new(StreamRequest { | |
| request, | |
| transport, | |
| peer_cred, | |
| query, | |
| })), | |
| }; | |
| let _ = tx.send(request).await; | |
| } | |
| Ok(Err(err)) => tracing::debug!(%err, "stream SETUP handshake failed"), | |
| Err(_) => tracing::debug!("stream SETUP handshake timed out"), | |
| } | |
| }); |
🤖 Prompt for 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.
In `@rs/moq-native/src/server.rs` around lines 816 - 842, The stream SETUP
handshake in spawn_stream_request can wait forever on
server.accept_request(session).await, so wrap that await in a timeout and treat
timeout as a handshake failure. Add the timeout handling inside the tokio::spawn
block around accept_request, and keep the existing tracing::debug! failure path
for both timeout and other errors so slow peers are dropped before they can
accumulate tasks and sockets.
| license = "MIT OR Apache-2.0" | ||
|
|
||
| version = "0.13.0" | ||
| version = "0.13.1" |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'name = "moq-relay"|version = "|publish|workspace.package|moq-relay' Cargo.toml rs -C 2Repository: moq-dev/moq
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- root Cargo.toml ---'
sed -n '1,220p' Cargo.toml
echo
echo '--- rs/moq-relay/Cargo.toml ---'
sed -n '1,220p' rs/moq-relay/Cargo.toml
echo
echo '--- publish/version hints ---'
rg -n '^\s*publish\s*=|^\s*version\s*=|workspace\.package|release policy|semver|crates\.io|not published' Cargo.toml rs/moq-relay -C 2Repository: moq-dev/moq
Length of output: 6842
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- root Cargo.toml size ---'
wc -l Cargo.toml rs/moq-relay/Cargo.toml
echo
echo '--- root Cargo.toml workspace/package section ---'
sed -n '1,120p' Cargo.toml
echo
echo '--- rs/moq-relay/Cargo.toml ---'
sed -n '1,120p' rs/moq-relay/Cargo.tomlRepository: moq-dev/moq
Length of output: 5136
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- moq-relay README publish references ---'
rg -n 'crates\.io|publish|Semantic Versioning|version' rs/moq-relay/README.md rs/moq-relay/CHANGELOG.md -C 2
echo
echo '--- moq-relay public API / config surface references ---'
rg -n 'struct Config|pub struct Config|internal|listener|publish|subscribe|clap::|derive\(Parser\)|#[[:space:]]*arg|#[[:space:]]*command' rs/moq-relay/src -C 2Repository: moq-dev/moq
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Config definition ---'
rg -n 'pub struct Config|struct Config|impl Config|internal' rs/moq-relay/src -C 3
echo
echo '--- main CLI entrypoints ---'
rg -n 'Parser|Subcommand|Args|Config::|parse\(\)' rs/moq-relay/src/main.rs rs/moq-relay/src -C 2Repository: moq-dev/moq
Length of output: 41637
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- rs/moq-relay/src/main.rs outline ---'
ast-grep outline rs/moq-relay/src/main.rs --view expanded
echo
echo '--- Config / listener / internal definitions ---'
rg -n 'pub struct Config|struct Config|internal|listener|publish|subscribe|Parser|Subcommand|Args' rs/moq-relay/src -C 2Repository: moq-dev/moq
Length of output: 50367
Bump moq-relay to the next minor version.
Removing Config.internal and the internal listener/config CLI surface is a breaking change, so 0.13.1 understates the compatibility impact for crates.io consumers. Use the next minor release, e.g. 0.14.0.
🤖 Prompt for 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.
In `@rs/moq-relay/Cargo.toml` at line 8, Update the moq-relay package version to
the next minor release because removing Config.internal and the internal
listener/config CLI surface is a breaking API change. Change the version in the
Cargo.toml for moq-relay from the current 0.13.1 to a 0.14.0-style release so
crates.io consumers see the correct compatibility signal; use the package
metadata entry that defines moq-relay’s version.
Comment on lines
+146
to
+165
| if let Some(url) = self.request.url() { | ||
| let params = self.auth.params_from_url(url); | ||
|
|
||
| if let Some(identity) = self.request.peer_identity() { | ||
| tracing::debug!("mTLS peer authenticated"); | ||
| // Scope the grant to the canonical root. An mTLS publisher dialing a | ||
| // vanity alias lands on the same tree a JWT would; cluster peers dial | ||
| // "/", which the API resolves (typically to an unscoped root). The API | ||
| // also returns the billing tier (defaulting to internal for trusted peers). | ||
| let mut token = self.auth.verify_mtls(¶ms.path).await?; | ||
| // Close the session when the client certificate expires, mirroring | ||
| // the JWT `exp` handling. Validated once at the TLS handshake otherwise. | ||
| token.expires = identity.expiry(); | ||
| return Ok(token); | ||
| if let Some(identity) = self.request.peer_identity() { | ||
| tracing::debug!("mTLS peer authenticated"); | ||
| // Scope the grant to the canonical root. An mTLS publisher dialing a | ||
| // vanity alias lands on the same tree a JWT would; cluster peers dial | ||
| // "/", which the API resolves (typically to an unscoped root). The API | ||
| // also returns the billing tier (defaulting to internal for trusted peers). | ||
| let mut token = self.auth.verify_mtls(¶ms.path).await?; | ||
| // Close the session when the client certificate expires, mirroring | ||
| // the JWT `exp` handling. Validated once at the TLS handshake otherwise. | ||
| token.expires = identity.expiry(); | ||
| return Ok(token); | ||
| } | ||
|
|
||
| return Ok(self.auth.verify(¶ms).await?); | ||
| } | ||
|
|
||
| self.authenticate_stream().await |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Preserve mTLS auth for URL-less QUIC requests.
Raw QUIC request url() returns None, so this URL-first dispatch falls through to authenticate_stream() and never checks peer_identity(). That can reject or downgrade mTLS-authenticated QUIC peers that connect without a WebTransport URL. Derive params from the SETUP path for URL-less requests and apply the mTLS override before JWT/public verification.
🤖 Prompt for 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.
In `@rs/moq-relay/src/connection.rs` around lines 146 - 165, The URL-first branch
in connection::authenticate skips mTLS for URL-less QUIC requests because raw
QUIC url() can be None, causing peer_identity() to be ignored and falling
through to authenticate_stream(). Update the dispatch in the authenticate flow
to derive auth params from the SETUP path when no URL is present, then still
apply the mTLS check/override via verify_mtls and peer_identity() before falling
back to JWT/public verification.
…st as a flag Follow the review: drop the per-listener `?anon=` and `?allow-uid=` query parameters on stream `--server-bind` entries. - No-JWT stream connections now resolve through the relay's existing public auth (`--auth-public` / `[auth] public`), exactly like a tokenless QUIC client, instead of a bespoke anon scope. `Connection::authenticate` collapses to one path (URL or SETUP -> `Auth::verify`), and `AuthToken::anon` is gone. - The Unix peer-credential allowlist becomes server-level flags (`--server-unix-allow-uid` / `-gid` / `-pid`), enforced in the `unix://` listener before the SETUP is read, rather than a bind-URL query. TCP/QUIC ignore them (no peer credentials). Stream bind URLs are now plain `tcp://host:port` / `unix:///path` with no query. `Request` drops `peer_cred()` / `listen_query()` (the relay no longer needs them); `path()` remains. `PeerCred` moves back to `moq_native::unix`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
rs/moq-native/src/server.rs (2)
665-666: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake
close()a no-op for stream-only builds instead of panicking.In a
tcp/uds-only build thisunreachable!still compiles, andaccept()callsself.close().awaiton Ctrl-C, so stream-only shutdown panics.Proposed fix
- #[cfg(not(any(feature = "noq", feature = "quinn", feature = "quiche", feature = "iroh")))] - unreachable!("no QUIC backend compiled"); + #[cfg(not(any( + feature = "noq", + feature = "quinn", + feature = "quiche", + feature = "iroh", + feature = "websocket", + feature = "tcp", + all(feature = "uds", unix) + )))] + unreachable!("no transport compiled");🤖 Prompt for 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. In `@rs/moq-native/src/server.rs` around lines 665 - 666, The `close()` path in `server.rs` currently uses an `unreachable!` fallback for builds without any QUIC backend, but `accept()` still invokes `self.close().await` during shutdown in tcp/uds-only builds. Update the `close()` implementation so the no-QUIC case becomes a harmless no-op instead of panicking, keeping the existing QUIC-backend branches intact and making sure the `Server` shutdown flow remains safe for stream-only builds.
226-248: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject stream bind URL parts that are currently ignored.
tcp://.../path?jwt=...andunix://...?anon=...parse successfully, but only the TCP host/port or Unix path are retained. That silently drops auth/policy-looking configuration; reject query/fragment and TCP path components, or carry them through deliberately. The?anon=fixture should be updated to match the intended behavior.Proposed guard
let url = Url::parse(entry).map_err(|_| Error::InvalidBind(entry.to_string()))?; +if !matches!(url.path(), "" | "/") || url.query().is_some() || url.fragment().is_some() { + return Err(Error::InvalidBind(entry.to_string())); +} let host = url.host_str().ok_or_else(|| Error::InvalidBind(entry.to_string()))?;let url = Url::parse(entry).map_err(|_| Error::InvalidBind(entry.to_string()))?; if url.path().is_empty() { return Err(Error::InvalidBind(entry.to_string())); } +if url.query().is_some() || url.fragment().is_some() { + return Err(Error::InvalidBind(entry.to_string())); +} Ok(BindScheme::Unix(PathBuf::from(url.path())))Also applies to: 1226-1231
🤖 Prompt for 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. In `@rs/moq-native/src/server.rs` around lines 226 - 248, The stream bind parsing in the BindScheme/URL parsing path is currently accepting extra URL parts that get ignored, so tighten the validation in the bind parser around the Tcp and Unix branches. In the Url::parse-based handling, reject any query or fragment for both schemes, and also reject TCP paths (only host/port should be allowed) instead of silently discarding them; if Unix is meant to support only a filesystem path, keep that explicit and reject unexpected components there too. Update the related bind fixture/test case using the ?anon= form to match the intended behavior.
🧹 Nitpick comments (2)
doc/bin/relay/auth.md (2)
316-322: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSoften the absolute claim about moq-lite-05 being the only version with in-band request path.
The phrasing "the only version that carries a request path in-band" is an absolute statement about the protocol that may become incorrect as MoQ evolves. Consider rephrasing to reflect the implementation scope:
-the listener offers moq-lite-05, the only version that carries a request -path in-band, so a JWT/path can ride the SETUP. +the listener offers moq-lite-05, currently the only supported version that +carries a request path in-band, so a JWT/path can ride the SETUP.🤖 Prompt for 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. In `@doc/bin/relay/auth.md` around lines 316 - 322, Soften the absolute wording in the Notes section of auth.md around moq-lite-05; the sentence in the stream transport / qmux explanation currently claims it is “the only version that carries a request path in-band.” Rephrase this in the surrounding prose to describe the current implementation scope instead of a permanent protocol guarantee, and keep the reference tied to the plain-stream/qmux setup description.
285-315: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse consistent unix allowlist naming here
The prose uses CLI-style names (
--server-unix-allow-*), while the example andconfig.mddocument TOML keys (unix_allow_*). Add a brief note mapping the two forms, or stick to one naming scheme throughout this section.🤖 Prompt for 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. In `@doc/bin/relay/auth.md` around lines 285 - 315, The Unix socket allowlist section mixes CLI-style flags with TOML config keys, so make the naming consistent in this block. Update the prose in auth.md around the Unix socket allowlist to either stick to the TOML field names used by the server config (`unix_allow_uid`, `unix_allow_gid`, `unix_allow_pid`) or add a short mapping note that explicitly ties them to the corresponding `--server-unix-allow-*` flags, and ensure the wording around the server-level setting matches the symbols shown in the example.
🤖 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 `@rs/moq-native/src/server.rs`:
- Around line 789-800: The spawned listener tasks in Server are detached, so
closing or dropping Server can leave the TCP/Unix accept loops and bound sockets
running. Update the server lifecycle handling in server.rs around
spawn_tcp_loop, spawn_unix_loop, and StreamListeners to keep abort/cancellation
handles alongside rx, then have close() and/or Drop signal shutdown and stop
those tasks before Server goes away.
---
Outside diff comments:
In `@rs/moq-native/src/server.rs`:
- Around line 665-666: The `close()` path in `server.rs` currently uses an
`unreachable!` fallback for builds without any QUIC backend, but `accept()`
still invokes `self.close().await` during shutdown in tcp/uds-only builds.
Update the `close()` implementation so the no-QUIC case becomes a harmless no-op
instead of panicking, keeping the existing QUIC-backend branches intact and
making sure the `Server` shutdown flow remains safe for stream-only builds.
- Around line 226-248: The stream bind parsing in the BindScheme/URL parsing
path is currently accepting extra URL parts that get ignored, so tighten the
validation in the bind parser around the Tcp and Unix branches. In the
Url::parse-based handling, reject any query or fragment for both schemes, and
also reject TCP paths (only host/port should be allowed) instead of silently
discarding them; if Unix is meant to support only a filesystem path, keep that
explicit and reject unexpected components there too. Update the related bind
fixture/test case using the ?anon= form to match the intended behavior.
---
Nitpick comments:
In `@doc/bin/relay/auth.md`:
- Around line 316-322: Soften the absolute wording in the Notes section of
auth.md around moq-lite-05; the sentence in the stream transport / qmux
explanation currently claims it is “the only version that carries a request path
in-band.” Rephrase this in the surrounding prose to describe the current
implementation scope instead of a permanent protocol guarantee, and keep the
reference tied to the plain-stream/qmux setup description.
- Around line 285-315: The Unix socket allowlist section mixes CLI-style flags
with TOML config keys, so make the naming consistent in this block. Update the
prose in auth.md around the Unix socket allowlist to either stick to the TOML
field names used by the server config (`unix_allow_uid`, `unix_allow_gid`,
`unix_allow_pid`) or add a short mapping note that explicitly ties them to the
corresponding `--server-unix-allow-*` flags, and ensure the wording around the
server-level setting matches the symbols shown in the example.
🪄 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: 4cacb74e-49fe-439c-84b1-22375f61ba3c
📒 Files selected for processing (7)
doc/bin/relay/auth.mddoc/bin/relay/config.mdrs/moq-native/src/server.rsrs/moq-relay/src/auth.rsrs/moq-relay/src/config.rsrs/moq-relay/src/connection.rsrs/moq-relay/tests/smoke.rs
✅ Files skipped from review due to trivial changes (1)
- doc/bin/relay/config.md
🚧 Files skipped from review as they are similar to previous changes (2)
- rs/moq-relay/tests/smoke.rs
- rs/moq-relay/src/config.rs
kixelated
commented
Jul 1, 2026
| value_delimiter = ',' | ||
| )] | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub unix_allow_uid: Vec<u32>, |
Collaborator
Author
There was a problem hiding this comment.
Make a Unix struct instead, and feature/os gate this?
Collaborator
Author
There was a problem hiding this comment.
Done — the unix knobs are now a feature/os-gated UnixConfig struct (#[cfg(all(feature = "uds", unix))]), and there's a matching #[cfg(feature = "tcp")] TcpConfig. Both flatten into ServerConfig.
(Written by Claude Opus 4.8)
| Unix(PathBuf), | ||
| /// A `tcp://`/`unix://` entry whose transport feature is not compiled in. | ||
| #[cfg(not(all(feature = "tcp", feature = "uds", unix)))] | ||
| Unsupported(String), |
Collaborator
Author
There was a problem hiding this comment.
Why not return an error instead?
Collaborator
Author
There was a problem hiding this comment.
Moot now — with separate flags there's no scheme classification, so BindScheme/quic_bind() (and the Unsupported-then-error dance) are deleted. An unknown flag is just a clap parse error.
(Written by Claude Opus 4.8)
| }; | ||
|
|
||
| match scheme { | ||
| "udp" | "quic" => Ok(BindScheme::Quic(rest.to_string())), |
Collaborator
Author
There was a problem hiding this comment.
I guess add https too
Collaborator
Author
There was a problem hiding this comment.
Also moot — no URL scheme parsing anymore, so there's nothing to add https to. (The web/wss listener stays its own --web-https-* config.)
(Written by Claude Opus 4.8)
| /// QUIC over UDP. Carries the address with any `udp://` scheme stripped. | ||
| Quic(String), | ||
| /// Plaintext qmux over TCP. Carries the resolved address. | ||
| #[cfg(feature = "tcp")] |
Collaborator
Author
There was a problem hiding this comment.
IDK I would just use separate flags for these protocols. It feels like we're overloading --bind especially since we only support one QUIC listener.
--server-tcp-bind
--server-unix-bind
--server-unix-allow-uid
Collaborator
Author
There was a problem hiding this comment.
Done — switched to separate flags in 874fdae. --server-bind is QUIC-only again; the stream transports now have --server-tcp-bind and --server-unix-bind (+ --server-unix-allow-{uid,gid,pid}). The whole BindScheme URL-scheme parser and quic_bind() are gone.
(Written by Claude Opus 4.8)
… flags Address review: stop overloading `--server-bind` with URL schemes. `--server-bind` is QUIC-only again (`Option<String>`), and the stream transports get their own flags via feature/os-gated config structs: - `TcpConfig` (`--server-tcp-bind`), gated on the `tcp` feature. - `UnixConfig` (`--server-unix-bind` + `--server-unix-allow-uid`/`-gid`/`-pid`), gated on `uds` + unix. This deletes the `BindScheme` URL parser, `quic_bind()`, and the related bind errors. QUIC still runs by default; it's skipped only when a stream listener is configured and `--server-bind` is unset (`has_stream_listener()`). Also from review: - Tie the spawned stream accept-loops to the `Server`: `StreamListeners` keeps their `JoinHandle`s and aborts them on `Drop`, so bound sockets don't linger. - `main.rs` only maps `Error::NoBackend` from `local_addr()` to "no QUIC"; other errors propagate instead of being swallowed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
kixelated
commented
Jul 1, 2026
| value_delimiter = ',' | ||
| )] | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub allow_uid: Vec<u32>, |
Collaborator
Author
There was a problem hiding this comment.
Make struct Option ? If None then we don't enforce anything.
Collaborator
Author
There was a problem hiding this comment.
Done — the listener now takes Option<UnixAllow>; None (the default, built as (!allow.is_empty()).then(...)) enforces nothing, so there's no per-connection is_empty() check anymore.
(Written by Claude Opus 4.8)
Collaborator
Author
There was a problem hiding this comment.
Also made the config field itself Option<UnixAllow> now (not just the enforcement path), in 9f3004c's follow-up: None = no allowlist. clap's flatten sets it to Some only when a --server-unix-allow-* flag is present, and the Option survives the TOML→CLI re-parse (the clobber test covers it).
(Written by Claude Opus 4.8)
| /// [`UnixConfig::allow_uid`] and friends. | ||
| #[cfg(all(feature = "uds", unix))] | ||
| #[derive(Clone, Default)] | ||
| struct UnixAllow { |
Collaborator
Author
There was a problem hiding this comment.
Lul just make this use clap
Collaborator
Author
There was a problem hiding this comment.
Done in 9f3004c — UnixAllow is now a clap::Args struct with the --server-unix-allow-* flags, flattened into UnixConfig as allow and reused for enforcement. No more hand-rolled duplicate.
(Written by Claude Opus 4.8)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
rs/moq-native/src/server.rs (2)
606-631: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
close()still assumes a QUIC backend.In a supported
tcp/unix-only build, the cfg on Line 630 compiles theunreachable!()path, soclose()panics instead of shutting the server down.self.streamsis also never stopped here, so the new listeners remain live until drop.🤖 Prompt for 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. In `@rs/moq-native/src/server.rs` around lines 606 - 631, The close() implementation in Server still assumes a QUIC backend, causing tcp/unix-only builds to hit unreachable!() and panic instead of shutting down cleanly. Update Server::close to handle the non-QUIC configuration without panicking, and make sure self.streams is explicitly stopped/closed there as part of shutdown. Keep the existing backend-specific close paths for noq/quinn/quiche/iroh, but remove the fallback unreachable branch from the tcp/unix-only case.
423-428: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDon't collapse stream-listener bind failures into
None.Lines 426-428 turn a fatal first-poll bind error into an ordinary end-of-stream. Downstream callers treat
Noneas shutdown; for example,rs/moq-relay/src/main.rsonly escalatesserve(server, ...)when it returnsErr, so the process can stay up without an accepting server.🤖 Prompt for 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. In `@rs/moq-native/src/server.rs` around lines 423 - 428, The first-poll stream-listener startup path in server.rs is swallowing a fatal bind failure by returning None from the server poll logic. Update the code around self.streams.ensure_started() so a bind error is propagated as an actual failure result instead of being converted into end-of-stream, and keep the existing tracing::error! logging for context. Make the fix in the server’s poll/accept path so callers like serve(...) in moq-relay can observe the error and abort startup correctly.rs/moq-relay/tests/smoke.rs (1)
403-405: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDon't swallow
Connection::run()failures.If stream auth/setup regresses, this helper turns the real error into a later timeout. Fail the spawned task immediately so the smoke test points at the root cause.
Suggested change
tokio::spawn(async move { - let _ = conn.run().await; + if let Err(err) = conn.run().await { + panic!("stream relay connection failed: {err:?}"); + } });🤖 Prompt for 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. In `@rs/moq-relay/tests/smoke.rs` around lines 403 - 405, The spawned helper in the smoke test is swallowing failures from Connection::run(), which delays the real setup/auth error until a later timeout. Update the tokio::spawn block around conn.run() to surface the error immediately instead of assigning it to _, and make sure the spawned task fails fast with a clear panic or equivalent so the test reports the root cause. Use the existing conn.run() call site in the smoke test to locate the change.
♻️ Duplicate comments (2)
rs/moq-native/src/server.rs (2)
727-755: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBind every stream listener before spawning any accept loop.
If a later bind fails, earlier loops are already running,
self.rxis never installed, and the server reports startup failure while those sockets keep accepting untilServeris dropped.🤖 Prompt for 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. In `@rs/moq-native/src/server.rs` around lines 727 - 755, Bind setup currently starts accept loops inside the `Server::start` bind loop, so earlier listeners can keep running if a later bind fails. Change the flow to first bind all entries from `self.binds` and collect the resulting listeners (for both `StreamBind::Tcp` and `StreamBind::Unix`), then assign `self.rx` and spawn the accept loops only after every bind succeeds. Keep the existing `spawn_tcp_loop` and `spawn_unix_loop` calls, but move them to a second phase after successful listener creation.
243-265: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDefer QUIC backend selection until
build_quicis known.Line 257 can still panic before Line 265 decides whether QUIC is needed, so a
tcp/unix-only build cannot initialize the stream-only mode this PR adds.
🤖 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/config.md`:
- Around line 40-42: The config docs currently imply mTLS works for plaintext
qmux stream listeners, which is incorrect for the new stream path. Update the
wording in the relay config documentation to scope mTLS only to TLS-based
transports in the QUIC/WebTransport settings, and describe tcp/unix stream
listeners as supporting JWT or public access, with optional Unix peer-credential
gating instead of TLS identity.
---
Outside diff comments:
In `@rs/moq-native/src/server.rs`:
- Around line 606-631: The close() implementation in Server still assumes a QUIC
backend, causing tcp/unix-only builds to hit unreachable!() and panic instead of
shutting down cleanly. Update Server::close to handle the non-QUIC configuration
without panicking, and make sure self.streams is explicitly stopped/closed there
as part of shutdown. Keep the existing backend-specific close paths for
noq/quinn/quiche/iroh, but remove the fallback unreachable branch from the
tcp/unix-only case.
- Around line 423-428: The first-poll stream-listener startup path in server.rs
is swallowing a fatal bind failure by returning None from the server poll logic.
Update the code around self.streams.ensure_started() so a bind error is
propagated as an actual failure result instead of being converted into
end-of-stream, and keep the existing tracing::error! logging for context. Make
the fix in the server’s poll/accept path so callers like serve(...) in moq-relay
can observe the error and abort startup correctly.
In `@rs/moq-relay/tests/smoke.rs`:
- Around line 403-405: The spawned helper in the smoke test is swallowing
failures from Connection::run(), which delays the real setup/auth error until a
later timeout. Update the tokio::spawn block around conn.run() to surface the
error immediately instead of assigning it to _, and make sure the spawned task
fails fast with a clear panic or equivalent so the test reports the root cause.
Use the existing conn.run() call site in the smoke test to locate the change.
---
Duplicate comments:
In `@rs/moq-native/src/server.rs`:
- Around line 727-755: Bind setup currently starts accept loops inside the
`Server::start` bind loop, so earlier listeners can keep running if a later bind
fails. Change the flow to first bind all entries from `self.binds` and collect
the resulting listeners (for both `StreamBind::Tcp` and `StreamBind::Unix`),
then assign `self.rx` and spawn the accept loops only after every bind succeeds.
Keep the existing `spawn_tcp_loop` and `spawn_unix_loop` calls, but move them to
a second phase after successful listener creation.
🪄 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: e7c4bcb2-7dca-450f-a032-59d0ce992d90
📒 Files selected for processing (6)
doc/bin/relay/auth.mddoc/bin/relay/config.mdrs/moq-native/src/server.rsrs/moq-relay/src/config.rsrs/moq-relay/src/main.rsrs/moq-relay/tests/smoke.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- rs/moq-relay/src/main.rs
- rs/moq-relay/src/config.rs
…orcement
Address review:
- Merge the hand-rolled internal allowlist with the config: `UnixAllow` is now a
`clap::Args` struct (`--server-unix-allow-{uid,gid,pid}`) reused for both config
and enforcement, flattened into `UnixConfig` as `allow`. `[server.unix.allow]`
in TOML (matching the old `[internal.uds.allow]` shape).
- The listener takes `Option<UnixAllow>`: `None` (the default, when nothing is
configured) enforces nothing, replacing the always-present + is_empty() check.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The config field itself is now `Option<UnixAllow>` (None = no allowlist, allow everything) rather than an always-present struct. clap's `#[command(flatten)]` populates it to Some only when a `--server-unix-allow-*` flag is given, and the value survives the relay's TOML->CLI re-parse (verified by the clobber test). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mTLS is a QUIC/WebTransport credential; plaintext tcp/unix stream listeners have no TLS peer identity. Reword so mTLS isn't implied for every transport. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
What
The relay no longer has a separate
--internal-*listener. Its trusted-local-worker transports (plaintext qmux over TCP / Unix sockets) become their own--server-*flags alongside the QUIC--server-bind, and every transport shares one authenticated accept path:Why
The internal listener originally existed because it was unauthenticated full access — a different trust model that deserved its own door. The first commit on this branch made it authenticate through the relay's
Auth. Once it's authenticated, it differs from the normal server only in transport, not in auth — so a separate subsystem, config namespace, and accept loop no longer earn their keep.Authentication
Every transport goes through the same
Auth; only the source of the path + JWT differs:?jwt=from the request URL; a valid mTLS client cert (QUIC only) stands in for a JWT.tcp/unix: path +?jwt=ride the moq-lite-05 SETUP in-band. A no-JWT stream connection resolves through the relay's existing public auth (--auth-public/[auth] public), exactly like a tokenless QUIC client — no listener-specific scope. A local helper that must publish without a token is granted publicly (e.g.--auth-public-publish .stats).Unix sockets can additionally be gated by peer credentials via
--server-unix-allow-uid/-gid/-pid, enforced in the listener before the SETUP is read.Changes
moq-native:Servergains TCP (TcpConfig) and Unix (UnixConfig) stream listeners alongside QUIC/WebSocket, each a feature/os-gated config struct flattened intoServerConfig(--server-bindstays QUIC-only,Option<String>). The listeners bind lazily on firstaccept()and feed one accept loop; their accept-loop tasks are owned byStreamListenersand aborted onDrop.Requestgainspath()(in-band SETUP path). Theunix://listener enforces the peer-cred allowlist. A stream-only server (no--server-bind) doesn't open QUIC.moq-relay: deleteinternal.rs/InternalConfig.Connection::authenticateis one path: params from the URL (QUIC/WS) or the SETUP (stream), thenAuth::verify.main.rsdrops therun_internalarm and only treatsError::NoBackendfromlocal_addr()as "no QUIC".doc/bin/relay/{config,auth}.mdrewritten (the[internal]section becomes "Stream Listeners").Breaking changes
moq-native(0.x):Servergainstcp/unixconfig +Request::path(). Kept onmainper maintainer call — all consumers are in-tree.moq-relay:--internal-*flags /[internal]TOML removed, replaced by--server-tcp-bind/--server-unix-bind/--server-unix-allow-*. The old unauthenticated behaviour is reproducible with--server-tcp-bind 127.0.0.1:4444 --auth-public "".Tests
cargo nextest --all-featuresgreen (incl. theinternal_tcp_round_trip/internal_unix_round_tripsmoke tests, now stream-only servers driven throughServer+Connectionwith public auth) plus config/allowlist unit tests.cargo clippy --workspace --all-targets --all-features -- -D warnings,cargo doc --all-features,cargo check --no-default-features, and shear/sort/taplo all pass.🤖 Generated with Claude Code
(Written by Claude Opus 4.8)