Repository navigation
feat(relay): one TCP port for HTTP, RTMP, RTMPS, and tls:// - #7362
Merged
Merged
Conversation
Quest-Claim: 1b57637f4a6c0ab8
…bytes Each public web connection is classified on its own task before HTTP: a plaintext C0 (0x03) is RTMP, a TLS connection with no ALPN is sniffed again on its first decrypted byte (RTMPS), and everything else reaches the router. RTMP goes to an embedder through Relay::rtmp / Web::rtmp; untaken, it is logged and closed. The tls:// arm stays in the quest pending its config gate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Connections still classifying held sender clones, so Incoming::accept never returned None after the listeners stopped with no idle deadline. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The HTTPS acceptor reads each ClientHello. One that offers a moq ALPN finishes with the listen TLS identity and runs as a MoQ session, through the new moq_tokio::tcp::TlsAcceptor (from Server::tls_acceptor). Every other connection keeps web.https.cert. listen.tcp stays a separate optional listener, and there is no new setting. qmux gains tls::Server::accept_established and a public tcp::adopt, so the HTTPS arm shares both handshake halves with the tls:// listener. The quest is done and deleted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
changed the title
kixelated
marked this pull request as ready for review
October 30, 2026 20:24
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 9 minutes. View limit details
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. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
kixelated
commented
Oct 30, 2026
kixelated
left a comment
kixelated
left a comment
Collaborator
Author
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: b6cbee2 (base 9387174).
No actionable bugs found in this review. The direction is sound: classifying inside the existing per-connection acceptor avoids a second listener/channel layer, while sharing the qmux handshake and SETUP path avoids parallel protocol implementations. I checked TLS identity/ALPN selection, decrypted-byte replay, HTTP metadata, RTMP handoff, and admission/deadline handling around the new HTTPS qmux path.
The existing CodeQL threads concern test assertion output; I found no additional sensitive-data logging issue in those changes.
Verification limits: static review of the diff and relevant surrounding code through GitHub only. I did not run builds, tests, or live TLS/RTMP interoperability checks; the added tests were inspected, not executed.
Collaborator
Author
|
Merge summary (
(Written by Claude Opus 5.5) |
Contributor
|
quest land removed this pull request from the queue: the batch check failed (exit 1). js: selecting docs
..js: selecting reader
...
----------------------------------------------------------------------
Ran 24 tests in 0.332s
OK
python3 sh/js/audit.test.py
..
----------------------------------------------------------------------
Ran 2 tests in 0.064s
OK
python3 sh/rs/test-lints.test.py
.
----------------------------------------------------------------------
Ran 1 test in 2.344s
OK
python3 sh/gh/nightly.test.py
..............
----------------------------------------------------------------------
Ran 14 tests in 1.112s
OK
python3 sh/gh/pages-clean.test.py
....
----------------------------------------------------------------------
Ran 4 tests in 0.048s
OK
python3 sh/gh/bench-report.test.py
............
----------------------------------------------------------------------
Ran 12 tests in 0.407s
OK
bench-ffi: 2 measured entries
bench-rust: failure; no history point
bench-ffi: failure; no history point
bench-rust: failure; no history point
no completed benchmark reports; no history point
bench-rust: 1 measured entries
bench-rust: failure; no history point
no completed benchmark reports; no history point
Paired comparison of c2 against c1: 1 confirmed regressions, 1 targets not compared
- `rust/kio/lock/4`: +40.0% (rounds +30.0%, +40.0%, +50.0%)
- `rust/gst/moq-gst/catalog` not compared: base build failed
Paired comparison of c2 against c1: 0 confirmed regressions, 0 targets not compared
python3 sh/gh/ts-paths.test.py
.....
----------------------------------------------------------------------
Ran 5 tests in 0.000s
OK
sh/go/publish-wrapper.test.sh
publish-wrapper test: no rsync on PATH, skipping
bash .claude/hooks/direnv.test.sh
direnv hook: could not load the dev shell, see the direnv output above.
direnv hook: regression tests passed
python3 sh/rs/uring.test.py
jq: error (at <stdin>:36): null (null) cannot be matched, as it is not a string
rs: selecting moq-relay moq-uring
jq: error (at <stdin>:36): null (null) cannot be matched, as it is not a string
rs: selecting moq-relay
jq: error (at <stdin>:36): null (null) cannot be matched, as it is not a string
rs: selecting kio moq-relay moq-uring
jq: error (at <stdin>:36): null (null) cannot be matched, as it is not a string
rs: selecting moq-unrelated
rs: selecting the workspace
rs: selecting the workspace
rs: selecting the workspace
.
----------------------------------------------------------------------
Ran 1 test in 0.270s
OK
ci: check quest failed
ci: check rs failed
ci: check rs_shapes failed
ci: check js failed
ci: seconds per module
24 check rs (failed)
9 check rs_shapes (failed)
14 check js (failed)
19 check drafts
11 check markdown
6 check gh
36 test gh
0 check drill
2 check quest (failed)
13 check shell
0 check large_files
0 check toml
0 check nix
0 check justfile
87 wall time
error: recipe `ci` failed on line 104 with exit code 1
. |
Contributor
|
codex / Codex land fixer: (written by codex / Codex) |
Contributor
|
quest land removed this pull request from the queue: the batch check failed (exit 1). candidate #3: `std::iter::Iterator`
help: there is a method `next_run` with a similar name
|
19292 | assert_eq!(fill.next_run(), Some(1));
| ++++
error[E0599]: no method named `next` found for struct `model::track::fill::Fill` in the current scope
--> rs/moq-net/src/model/track.rs:19396:35
|
19396 | while let Some(sequence) = fill.next() {
| ^^^^
|
::: rs/moq-net/src/model/track/fill.rs:668:1
|
668 | pub struct Fill {
| --------------- method `next` not found for this struct
|
= help: items from traits can only be used if the trait is implemented and in scope
= note: the following traits define an item `next`, perhaps you need to implement one of them:
candidate #1: `futures::StreamExt`
candidate #2: `serde_json::de::Read`
candidate #3: `std::iter::Iterator`
help: there is a method `next_run` with a similar name
|
19396 | while let Some(sequence) = fill.next_run() {
| ++++
error[E0599]: no method named `next` found for struct `model::track::fill::Fill` in the current scope
--> rs/moq-net/src/model/track.rs:19456:19
|
19456 | assert_eq!(fill.next(), Some(1));
| ^^^^
|
::: rs/moq-net/src/model/track/fill.rs:668:1
|
668 | pub struct Fill {
| --------------- method `next` not found for this struct
|
= help: items from traits can only be used if the trait is implemented and in scope
= note: the following traits define an item `next`, perhaps you need to implement one of them:
candidate #1: `futures::StreamExt`
candidate #2: `serde_json::de::Read`
candidate #3: `std::iter::Iterator`
help: there is a method `next_run` with a similar name
|
19456 | assert_eq!(fill.next_run(), Some(1));
| ++++
error[E0599]: no method named `next` found for struct `model::track::fill::Fill` in the current scope
--> rs/moq-net/src/model/track.rs:19485:37
|
19485 | while let Some(sequence) = fill.next() {
| ^^^^
|
::: rs/moq-net/src/model/track/fill.rs:668:1
|
668 | pub struct Fill {
| --------------- method `next` not found for this struct
|
= help: items from traits can only be used if the trait is implemented and in scope
= note: the following traits define an item `next`, perhaps you need to implement one of them:
candidate #1: `futures::StreamExt`
candidate #2: `serde_json::de::Read`
candidate #3: `std::iter::Iterator`
help: there is a method `next_run` with a similar name
|
19485 | while let Some(sequence) = fill.next_run() {
| ++++
For more information about this error, try `rustc --explain E0599`.
error: could not compile `moq-net` (lib test) due to 4 previous errors
error: process didn't exit successfully: `/nix/store/85lxjwg78jw6rf7pqkf3vcq6291nrlfh-cargo-1.98.1-x86_64-unknown-linux-gnu/bin/cargo clippy --locked --tests --all-features --manifest-path rs/moq-net/Cargo.toml -- -D warnings` (exit status: 101)
mbx[cache]: object cache: 0 hits, 8 misses; 0 B downloaded, 0 B uploaded, 13.3 MiB stored locally
mbx[cache]: 38.84s session; 0s estimated compiler time avoided (summed across compilations)
mbx[cache]: Cargo artifact reuse and CI cache archive transfers are not included above
mbx[savings]: 2.4 TiB of outputs exist once and appear everywhere. do not tell the checkouts.
error: recipe `feature-shapes` failed on line 251 with exit code 1
error: recipe `feature-shapes-changed` failed on line 75 with exit code 1
ci: check quest failed
ci: test rs failed
ci: check rs_shapes failed
ci: seconds per module
132 check rs
54 test rs (failed)
674 check rs_shapes (failed)
194 check js
54 test js
21 check drafts
13 check markdown
3 check gh
71 test gh
0 check drill
1 check quest (failed)
12 check shell
1 check large_files
0 check toml
0 check nix
0 check justfile
861 wall time
error: recipe `ci` failed on line 106 with exit code 1
. |
Contributor
|
codex / Codex land fixer: (written by codex / Codex) |
Contributor
|
quest land removed this pull request from the queue: Conflicts with the base: doc/bin/relay/auth.md, doc/bin/relay/cluster.md, quest/m2/one-port/tcp-demux.md, quest/m2/tls-listener-mtls.md, rs/moq-tokio/src/server.rs, rs/qmux/src/tls.rs |
Contributor
|
codex / Codex land fixer: @@ -146,6 +149,10 @@ impl Server {
/// [`handshake_timeout`](crate::Config::handshake_timeout)). Drive each
/// connection with `tokio::spawn` so a slow or non-cooperative peer can't
/// stall your `listener.accept()` loop.
+ ///
+ /// The returned session reports the client's certificate chain, when the
+ /// `rustls::ServerConfig` asked for one and the client presented it, and the
+ /// SNI it sent (see [`Session::peer_certificates`] and [`Session::server_name`]).
pub async fn accept(&self, stream: TcpStream) -> Result<Session, Error> {
let socket_stats = crate::tcp::adopt(&stream)?;
let acceptor = TlsAcceptor::from(self.config.clone());
@@ -159,11 +166,53 @@ impl Server {
tracing::debug!(?version, ?protocol, "parsed ALPN");
let session_config = self.session.clone().with_negotiated(version, protocol);
+ let conn = tls_stream.get_ref().1;
+ let handshake = Handshake::new(conn.peer_certificates(), conn.server_name());
let mut transport = Stream::new(tls_stream);
if let Some(stats) = socket_stats {
transport = transport.with_socket_stats(stats);
}
// `accept` awaits the peer's transport parameters so `protocol()` is resolved.
- Session::accept(transport, session_config).await
+ Ok(Session::accept(transport, session_config).await?.with_tls(handshake))
+ }
+}
+
+/// What a session's TLS handshake learned about the peer, captured before the
+/// stream is handed to the session.
+#[derive(Debug)]
+pub(crate) struct Handshake {
+ peer_certificates: Option<Vec<CertificateDer<'static>>>,
+ server_name: Option<String>,
+}
+
+impl Handshake {
+ fn new(peer_certificates: Option<&[CertificateDer<'static>]>, server_name: Option<&str>) -> Arc<Self> {
+ Arc::new(Self {
+ peer_certificates: peer_certificates.map(<[_]>::to_vec),
+ server_name: server_name.map(str::to_owned),
+ })
+ }
+}
+
+impl Session {
+ fn with_tls(mut self, handshake: Arc<Handshake>) -> Self {
+ self.tls = Some(handshake);
+ self
+ }
+
+ /// The certificate chain the peer presented in the TLS handshake, leaf first.
+ ///
+ /// On a [`Client`] session this is the server's chain. On a [`Server`] session
+ /// it is the client's, and only when the `rustls::ServerConfig` asked for one
+ /// and the client sent it; its client-certificate verifier has already
+ /// accepted the chain. `None` on a session not carried by TLS.
+ pub fn peer_certificates(&self) -> Option<&[CertificateDer<'static>]> {
+ self.tls.as_ref()?.peer_certificates.as_deref()
+ }
+
+ /// The SNI the client sent, on a session a [`Server`] accepted. `None` when the
+ /// client sent none, on a [`Client`] session, or off TLS.
+ pub fn server_name(&self) -> Option<&str> {
+ self.tls.as_ref()?.server_name.as_deref()
}
}
codex
The conflicts involve TLS authentication behavior. Base added client-certificate and SNI reporting, while PR 7362’s new shared-listener API has no way to carry that metadata through. Preserving both requires API integration and review, beyond a mechanical fix. I’ll hand it back and leave its branch unchanged.
exec
/bin/bash -lc "git merge --abort; python3 - <<'PY'
import json
from pathlib import Path
result = [{\"number\":7362,\"action\":\"handback\",\"diagnosis\":\"Base adds TLS peer-certificate and SNI propagation in rs/qmux/src/tls.rs and rs/moq-tokio/src/server.rs, but the PR's generic accept_established/TlsAcceptor path cannot carry that metadata; resolving the conflicts requires authentication API integration beyond a mechanical fix.\",\"recommendation\":\"Rebase with author review, carry validated peer identity and SNI through the shared HTTPS qmux accept path, preserve base TLS authentication policy, and test dedicated and shared TLS listeners.\"}]
Path('quest-land-result.json').write_text(json.dumps(result, indent=2) + '\\n')
PY
git status --short; git rev-parse pr-7362; git diff --exit-code; cat quest-land-result.json" in /work
succeeded in 349ms:
?? quest-land-result.json
b6cbee2c4edce9f34ec11b378a7dfa75e725896a
[
{
"number": 7362,
"action": "handback",
"diagnosis": "Base adds TLS peer-certificate and SNI propagation in rs/qmux/src/tls.rs and rs/moq-tokio/src/server.rs, but the PR's generic accept_established/TlsAcceptor path cannot carry that metadata; resolving the conflicts requires authentication API integration beyond a mechanical fix.",
"recommendation": "Rebase with author review, carry validated peer identity and SNI through the shared HTTPS qmux accept path, preserve base TLS authentication policy, and test dedicated and shared TLS listeners."
}
]
codex
PR 7362 handed back: resolving the TLS identity and SNI conflicts requires API integration and review beyond mechanical fixes.
Branch unchanged. Result saved uncommitted in [quest-land-result.json](/work/quest-land-result.json).
tokens used
29,443
PR 7362 handed back: resolving the TLS identity and SNI conflicts requires API integration and review beyond mechanical fixes.
Branch unchanged. Result saved uncommitted in [quest-land-result.json](/work/quest-land-result.json).
fixer result written
(written by codex / Codex) |
Main verifies client certificates on tls:// listeners (#7358). The HTTPS port's tls:// arm keeps that: it serves the same verifier config, and its request reports the SNI and accepted client certificate. To read them, qmux's Server::accept_established and moq-tokio's TlsAcceptor::accept now take the rustls TlsStream itself, and share the tcp loop's Incoming::tcp. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main's lint disallows tokio::time::timeout workspace-wide. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
Merged main (b201d42) to clear the
(Written by Claude Opus 5.5) |
kixelated
enabled auto-merge (squash)
November 1, 2026 09:34
kixelated
commented
Nov 1, 2026
kixelated
left a comment
kixelated
left a comment
Collaborator
Author
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: b201d42 (base 947c91d).
Compared with my review of b6cbee2, separating merged-main changes from the substantive HTTPS qmux/mTLS integration. No actionable bugs found in this update; the previous review had no open findings.
The direction is sound: taking the established rustls stream directly preserves negotiated ALPN, verified peer certificates and SNI, while sharing Incoming::tcp keeps authority/identity construction aligned with the dedicated listener. The added pinned-peer refusal/HTTPS isolation test addresses the changed policy boundary without adding another authentication path.
Verification limits: GitHub-only static review of the integration, surrounding TLS/request construction and tests. Builds, tests and live mTLS interoperability were not run.
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
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.
Finishes quest/m2/one-port/tcp-demux.md (one-port epic). The relay's HTTPS port now carries TLS-terminated HTTP, RTMP, RTMPS, and
tls://qmux, and the quest is deleted.Summary
listener::classify(rs/moq-relay/src/listener/demux.rs) runs inside the web acceptor, whichaxum_serveralready spawns on each connection's own task. The accept loop never peeks, and a silent peer only holds up its own task, under the existing idle deadline (listen.timeout, no new setting).0x03means RTMP, on either web listener.LazyConfigAcceptor, which reads the ClientHello first:qmux-01.<protocol>), the handshake finishes with the listen TLS identity, and the connection istls://qmux.web.https.cert(or ACME), as before. A negotiated ALPN means HTTP. With no ALPN, the first decrypted byte decides:0x03is RTMPS, anything else is HTTP.tls://on HTTPS runs as a relay connection on its own task, as a WebSocket session does. Its admission carries into the session. Its request reports the SNI (server_nameandauthority), the negotiated ALPN, and a client certificate the listen verifier accepted (peer_identity), as thelisten.tcptls://listener does since moq-dev/moq#7358.tcp::TlsAcceptor, fromServer::tls_acceptor(). It builds the same moq-ALPN config as thelisten.tcpTLS listener (TcpTls::new), including its client-certificate verifier, and serves the same stream versions.TlsAcceptor::accepttakes thetokio_rustls::server::TlsStreamwhose handshake finished with that config, runs the qmux handshake and SETUP, and returns aRequest. qmux gainstls::Server::accept_established, whichtls::Server::acceptnow calls, andtcp::adoptis now public, so both handshake halves are shared with the existingtls://listener. The TCP loop andTlsAcceptorbuild the request's TLS facts with one helper (Incoming::tcp).Relay::rtmp()/Web::rtmp()→web::rtmp::Incoming. If no consumer is taken, the relay closes RTMP connections with a debug log. RTMP connections leave the drain count when they are handed off, because the embedder owns their lifetime.WebAcceptoris no longer generic. The mTLS peer, SNI, h2, and ACME checks read the stream'srustls::ServerConnectiondirectly.Impact
web::rtmp::{Incoming, Stream},Web::rtmp,Relay::rtmp, andWeb::with_tls_acceptor.tcp::TlsAcceptorandServer::tls_acceptor.tcpnow depends ontokio-rustls(no default features), which qmux'stlsalready pulled in.tls::Server::accept_established, andtcp::adoptmade public.0x03on either web port, or a TLS connection with no ALPN whose first byte is0x03, is classified as RTMP. HTTP and HTTPS clients see no change.listen.tcpstays a separate optional listener. A relay with no listen TLS identity (stream-only, withoutlisten.tcp.tls) serves no moq ALPN on HTTPS.listen.tls.rootortls.peers,tls://on HTTPS asks for a client certificate as thelisten.tcptls://listener does: optional under CA roots, required under pinned peers. HTTPS and RTMPS on the port never see that config.doc/bin/relay/{config,auth,cluster}.mdand added an embed bullet indoc/bin/relay/index.md.tcp-demux.mdand its links. The one-port epic's TCP section is dropped, since that work has landed.rate-websocket.mdkeeps its pointer tolistener::Stream::tcp.Decisions
tls://qmux, and what happens to the separatelisten.tcpTLS bind? (maintainer, 2026-10-30)listen.tcpstays a separate optional listener. No new setting.listen.tcp.tlsis set, and stop bindinglisten.tcpseparately.web.https.qmux.Relay::rtmp()/Web::rtmp()returnweb::rtmp::Incoming;Incoming::accept()returns aStreamthat hastcp().peer()and a TLS flag.Choices made without asking
axum_serveralready runs the acceptor on its own task per connection, so this meets the Plan's goal (the accept loop never peeks) with less machinery. One idle deadline covers peek, TLS, and the first request.tls://, even if it also offersh2orhttp/1.1. Only a MoQ client offersqmux-01.*.axum_server's 10s bound (TLS_HANDSHAKE), because the lazy acceptor replacesRustlsAcceptor, which applied it.tls://on HTTPS verifies client certificates aslisten.tcp.tlsdoes. moq-dev/moq#7358 landed first, and the quest recorded (2026-10-08, 2026-10-30) that whichever landed second keepstls://client-certificate verification working, optional under CA roots and required under pinned peers. Resolving the merge,accept_establishedandTlsAcceptor::accepttake the rustlsTlsStreamrather than a generic stream plus ALPN, so they read the chain, SNI, and ALPN from the handshake itself and a caller cannot drop them.debug, so peers sending0x03cannot flood the logs.Checks
just ci origin/main: passed locally on b201d42 (quest gate), 5634 tests.quest check --base origin/main --branch quest/m2/one-port/tcp-demux: ok.https_listener_serves_tls_qmux:tls://dial to the HTTPS port that trusts only thehttps.certCA is refused, because the moq arm serves the listen identity;tls://dial lands a MoQ session;GET /healthoverhttp/1.1on the same port still returns 200 withhttps.cert.https_listener_verifies_tls_qmux_client_certificates: under pinnedtls.peers, atls://dial to the HTTPS port without a certificate is refused, while HTTPS on the port still answers without one.web_listeners_hand_rtmp_to_its_consumercovers RTMP, RTMPS, and HTTP on each listener;server_name_is_the_sni_never_the_hostcovers HTTP and WebSocket.Automated review
rs/moq-relay/src/web.rs(the twoassert!messages inweb_listeners_hand_rtmp_to_its_consumer): declined. Both are test assertion messages printing a loopback socket address and the offered ALPN list; no certificate or key material reaches them.Review
gpt-6-astra, medium effort (--deep), viaquest review --base origin/main. Session started with--newafter the hand-resolved merge of main (2a7c3b4545, round 1: 0 findings). Reviewed head: b201d42 (round 2, after moving the web tests tokio::clock::timeoutfor main's lint: 0 findings).🤖 Generated with Claude Code
(Written by Claude Opus 5.5)