Repository navigation
fix(tokio): refuse SETUP paths on URI bindings and keep iroh keys private on release (backport #7206, #7213) - #7389
Conversation
…vate on release Backports #7206 and #7213 onto release. - WebTransport and WebSocket take the path and query from the request URI only; a non-empty SETUP path is refused with a protocol violation (Tokio and the io_uring relay listener) instead of being authorized in its place. - The persisted iroh secret is created exclusively with mode 0600, and an existing file that is a symlink, non-regular, or group/other readable is refused. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: 354da9523d05b5c2bf6e77b7174441d98ad1af52.
No actionable findings in this release backport. The Tokio and uring paths reject non-empty SETUP paths on URI-bearing bindings before authorization, while raw/stream bindings retain their in-band path and query. The key-file change uses exclusive creation and validates the opened existing file rather than following a final symlink; Unix private-file checks and the accompanying migration documentation are consistent.
Direction: the focused backport is reasonable. Retain the stated absent/empty SETUP-path compatibility behavior; this is not a claim that every forbidden parameter-presence case is rejected.
Verification limits: independent static review of all four changed files and surrounding binding/dependency context only. No tests, feature builds, live transport checks, or Windows reparse-point checks were executed.
(Written by OpenAI)
|
Landing summary for head
(Written by Claude Opus 5.5) |
Problem
releasehas two security bugs that moq-dev/moq#7206 and moq-dev/moq#7213 fixed onmain:Request::path()/query()used the SETUP Path whenever it was non-empty and fell back to the request URI only otherwise. A client could dial/room?jwt=…but send/admin?jwt=…in SETUP, and Tokio would authorize the SETUP path. This affects moq-lite-05+ and every moq-transport draft.--iroh-secretfile was written with the default umask, typically readable by everyone, and was read through symlinks.Tracking quest: quest/m0/backport-security-fixes.md, claimed in moq-dev/moq#7381.
Approach
This is a hand port, not a cherry-pick.
releasehas nomoq_net::transport::Address, which #7206 depends on, and nomoq-tokio/src/secret.rs.Setup::accepttakes auriflag, set when the request carries a URL: WebTransport over noq or iroh, and WebSocket. If that SETUP carries a non-empty path, the session is closed withProtocolViolationand the request is dropped.Request::path()/query()now read only the URL when there is one, as onmain. The io_uring relay listener already authorized on the URL; it now also refuses a WebTransport SETUP path, matchingmain's Tokio and io_uring refusal. Raw QUIC, TCP, and Unix bindings still take the path from SETUP.load_or_createis ported unchanged as a private function iniroh.rs, along with its four tests. A new file is created exclusively with mode0600. An existing file must be a regular non-symlink file that group and others cannot read. The relay config doc gains fix(tokio): keep persisted iroh keys private #7213's paragraph.Choices made without asking (listed for review):
mainrefuses presence even when empty.release's publicHandshake::path()returns""for both absent and empty, and telling them apart would add moq-net API, which a patch backport must not. An empty path never overrides the URI any more, so it cannot change what gets authorized.releasehas notests/integration/refusals.rsharness.websocket_refuses_setup_pathdrives moq-lite-06, moqt-18, and moqt-22 over an in-memory WebSocket. It checks that non-empty paths are refused and the client session closes, and that an absent or empty path falls back to the URL. Without the fix it fails.release's qmux 0.5.3 the client sees a bareClosedhere instead of the application close code;main's test runs on qmux 0.6+. The server side does returnProtocolViolation, and the test asserts that.Impact
cargo semver-checks -p moq-tokio --baseline-rev origin/releasereports no semver update required. Only private items changed.--iroh-secretfile that is group- or world-readable is now refused at startup. Operators needchmod 600, as the doc says.CONTRIBUTING.md.Validation
nix develop -c just ci check origin/release: passed (afterjust fixformatting).nix develop -c just ci test origin/release: passed. Rust: 3253 tests passed, 7 skipped. That includeswebsocket_refuses_setup_pathand the fourpersisted_secret_*tests.cargo semver-checks -p moq-tokio --baseline-rev origin/release: no semver update required.websocket_refuses_setup_pathfails onqmux-01.moq-lite-06 accepted SETUP path Some("/room?jwt=url-token").Review
quest review --base origin/release --deep), round 1.Follow-ups
main. It is not in this quest's Plan; once it merges, it may need its ownreleasebackport.(Written by Claude Opus 5.5)
🤖 Generated with Claude Code