Repository navigation
fix(minimald): proxy runtime-published ports like a direct connection (0.7.0 backport) - #2724
norrietaylor wants to merge 9 commits into
Conversation
* fix(minimal,minvmd): hold the answerer channel socket owner-only The box-zone answerer's channel socket was world-connectable: the launchd plist bound it SockPathMode 0666 and the systemd socket unit SocketMode=0666, leaving the peer-uid gate the only check on who may connect. Both units now bind it owner-only -- SockPathOwner the operator, SockPathMode 0600 on launchd; SocketUser the operator, SocketMode=0600 on systemd -- with the uid gate kept as defence in depth. An install whose socket node carries group or other bits -- one that predates the mode, or a hand that relaxed it -- now fails custody through a channel_mode fact, so the advisory re-surfaces and a step re-run re-binds the socket. The accept loop's foreign-refusal warn is rate-limited to a few lines a second, so a connect storm cannot flood the log; the refusals themselves are never limited. * fix(minimal,minimald,minvmd): name the other account on a refused channel connect A daemon whose connect the owner-only channel socket refuses now says the host's DNS service belongs to another account, not a bare Permission denied. The answerer step needs the operator's account to resolve, with no stand-in uid, and custody fails a channel socket the operator does not own. * fix(minimal,minvmd): leave another operator's DNS service alone A channel socket owned by another existing, non-root account is that operator's service: the answerer step is now unavailable with the same sentence the daemons say on a refused connect, instead of a custody failure whose step would reinstall the unit and take the service over. A root-owned socket, or one whose uid names no account, still fails custody so the step re-binds it. The minvmd tests that take the process-global foreign-refusal slots now run one at a time and start and end with every slot free, and the warn-rate test bounds the burst at two windows' worth of lines, so a slow runner that spreads the burst does not fail it. * fix(minimal): offer the step over a socket another account left behind A channel socket another account owns refuses the answerer step only when a service is installed; with none, the step's install removes the leftover socket before it loads the unit, so it is offered. --------- Co-authored-by: gominimal-aw-bot[bot] <281738952+gominimal-aw-bot[bot]@users.noreply.github.com> (cherry picked from commit 746770d)
…er (#2300) * fix(net): refuse out-of-zone names at a guest deny-subtree DNS listener A deny-all host-address box on a VM-backed guest resolved out-of-zone names at the switch gateway: the interim NET-079 carve-out admitted the gateway's port 53 whole, UDP and TCP, so a box that admits no destination could still ask a resolver beyond the machine — and DNS over TCP could retry past the verdict. The guest's deny subtree now steers its UDP-53 lookups onto a loopback listener on the answerer's port (NET-141 slice 2): deny_out admits the listener beside the reply direction, the dstnat match carries no destination address (the box asks the gateway like every other host-address box; the steer is the carve-out), and the listener itself answers REFUSED for every name outside the box zone — apex and a non-lowercase zone suffix among them — NODATA for an in-zone non-A question, and forwards the zone's A questions to the switch gateway alone. DNS over TCP is neither steered nor admitted. The guest daemon starts the listener after its classifier table loads; the daemon's own lookups are untouched (cgroup-keyed match, deny path only). The interim --gateway-resolver installer flag and every test, digest and pin that spelled the interim carve-out follow the new shape. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(net): bound and pin the guest DNS listener's forwards Connect each forward socket to the gateway so a datagram from any other sender is dropped, cap forwards in flight so a flood cannot exhaust the daemon's descriptors, and drop 8000 the needless to_string calls clippy rejects in the guest classifier integration test. * fix(net): refuse questionless and non-QUERY messages at the guest DNS listener decide() forwarded a message with no question, and an UPDATE or NOTIFY whose zone section named an in-zone A, because every per-question check passed vacuously. Answer the first REFUSED and the rest NOTIMP, so the forward path carries only standard lookups. Bind the listener directly so its log does not read as the box-zone answerer, pin the integration test's boot-log check to the exact example.com REFUSED line, and cite NET-079 (the deny-all case) rather than NET-141 at the lines this change added. --------- Co-authored-by: gominimal-aw-bot[bot] <281738952+gominimal-aw-bot[bot]@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> (cherry picked from commit 2c2f8cc)
crate::traced landed on main after the release branch was cut, so the cherry-pick of #2300 did not compile here.
v0.7.2 is tagged at the branch head, so the version check requires the next patch release.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
Reviewed: no findings; approval withheld, 5 threads unresolved at APPROVE withheld: 5 unresolved threads — 0chroma crates/minimal/src/cmd/finalize_install.rs:1814, 0chroma crates/minimal/src/resolver.rs:397, 0chroma crates/minimal/src/resolver.rs:1344, 0chroma crates/minimal/src/resolver.rs:1875, 0chroma crates/minimald/src/net/dns.rs:224. Resolving the last thread releases the approval on this head. Resolve the open threads. Resolving the last one releases the approval on this head; no new review is required. Clobber: none seen. |
| ], | ||
| install: Some(AnswererInstall { | ||
| operator: "op".to_string(), | ||
| operator_uid: 1000, |
There was a problem hiding this comment.
I'm noticing lots of hard-coded 1000s, I think it's better to have this as a const
There was a problem hiding this comment.
Declining this one here. This PR is a cherry-pick of #2714 onto release/0.7.0, and a backport carries the change exactly as it landed on main, so the release branch doesn't drift from main. If you think the change is worth making, please raise it against main. It reaches the release branch only if it is itself backported.
| \t\t\t<string>Unix</string> | ||
| \t\t\t<key>SockPathName</key> | ||
| \t\t\t<string>__CHANNEL__</string> | ||
| \t\t\t<key>SockPathOwner</key> |
There was a problem hiding this comment.
I know this was already here, but I feel like having this inline is so cursed. Can use the inline_str! macro so this can be a separate file, or maybe there's a templating crate we can pull in if there's a good one
There was a problem hiding this comment.
Declining this one here. This PR is a cherry-pick of #2714 onto release/0.7.0, and a backport carries the change exactly as it landed on main, so the release branch doesn't drift from main. If you think the change is worth making, please raise it against main. It reaches the release branch only if it is itself backported.
| /// What a channel socket another operator's account owns means: the | ||
| /// host's DNS service is that account's. The same sentence the daemons | ||
| /// say when the socket refuses their connect. | ||
| const FOREIGN_OPERATOR_SERVICE: &str = "the host's DNS service belongs to another account; \ |
There was a problem hiding this comment.
seems like this is only used in a function and a test case, not sure if this needs to be a constant. If you feel it should, we should probably have all the strings as consts in a module for consistency
There was a problem hiding this comment.
Declining this one here. This PR is a cherry-pick of #2714 onto release/0.7.0, and a backport carries the change exactly as it landed on main, so the release branch doesn't drift from main. If you think the change is worth making, please raise it against main. It reaches the release branch only if it is itself backported.
| if matches!(step, AnswererStep::SourceUnavailable { .. }) { | ||
| return step; | ||
| } |
There was a problem hiding this comment.
this feels a bit silly, better to just make this a case statement and encorporate the implicit return in with _ => answerer_step_with_source(...) than to do an early return
There was a problem hiding this comment.
Declining this one here. This PR is a cherry-pick of #2714 onto release/0.7.0, and a backport carries the change exactly as it landed on main, so the release branch doesn't drift from main. If you think the change is worth making, please raise it against main. It reaches the release branch only if it is itself backported.
| impl RouteSource { | ||
| /// The value the proxy's log lines carry in their `route_source` field. | ||
| #[must_use] | ||
| pub fn as_str(self) -> &'static str { |
There was a problem hiding this comment.
better to just impl the display trait here
There was a problem hiding this comment.
Declining this one here. This PR is a cherry-pick of #2714 onto release/0.7.0, and a backport carries the change exactly as it landed on main, so the release branch doesn't drift from main. If you think the change is worth making, please raise it against main. It reaches the release branch only if it is itself backported.
… (T99) (#2714) * feat(minimald): forward runtime-published ports through the proxy The hostname proxy now reads the target box's runtime-published ports (an allowed expose, a published listener) from its live relay gate when the name resolves. The proxy verdict then admits exactly the ports a direct connection reaches. Admitted requests log their route source, and refusals record route_source=none. * test(minimald): prove an allowed expose opens through the proxy expose_allow_publishes_and_lists now registers the box's relay gate and lease, then shows the port refused through the proxy before the expose and reachable through it right after. * style(minimald): format the proxy and expose tests * fix(minimald): log admitted proxy requests at info The daemon's default filter is info, so a debug line never reached the log a min bug bundle carries. --------- Co-authored-by: minimal-foundry[bot] <327719912+minimal-foundry[bot]@users.noreply.github.com> (cherry picked from commit 7097b96)
The cherry-picked change names main's types and helpers. Release/0.7.0 predates the box renames and the traced spawn wrapper, so its tests and one doc link use the release's own names: - BoxGate is SessionGate, BoxPolicy is SessionPolicy, and boxes::DynamicIngress is sessions::DynamicIngress. - crate::traced does not exist here, so the new tests spawn with tokio::spawn, as the release's other proxy tests do. - The release's spawn_backend_on takes a full address and returns nothing, so proxy_forwards_runtime_published_port picks the backend port with a probe listener, the pattern the release's tests use. - The proptest's no-wider-reach check uses the outer gate, which now carries the runtime publishes, and asks admits_tcp. On this branch admits_direct_tcp covers the declared ports only, while admits_tcp also covers the listen-published and exposed ports. That is the set main's admits_tcp covers. The behaviour is #2714's: runtime-published ports join the proxy's route set, and the proxy's verdict equals the direct connection's.
f15bf61 to
a69ee07
Compare
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 5 unresolved threads — 0chroma crates/minimal/src/cmd/finalize_install.rs:1750, 0chroma crates/minimal/src/resolver.rs:397, 0chroma crates/minimal/src/resolver.rs:1344, 0chroma crates/minimal/src/resolver.rs:1875, 0chroma crates/minimald/src/net/dns.rs:224. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
… a direct connection NET-069 covers ports a box publishes while it runs. The new test publishes a listener's port through the listen watcher's step (the gate's admission), sees it forwarded, withdraws it, and checks that the proxied request then meets the direct connection's refusal: the same verdict and the same logged rule as a port the box never published. Adapted from main to release/0.7.0: SessionGate and SessionPolicy for BoxGate and BoxPolicy, tokio::spawn for crate::traced::spawn, a probe listener for the backend port because this branch's spawn_backend_on takes a full address, and no registry dynamic hold, which this branch's listen watcher does not record.
There was a problem hiding this comment.
minimal-review — approval withheld
Verdict COMMENT — APPROVE withheld: 5 unresolved threads — 0chroma crates/minimal/src/cmd/finalize_install.rs:1750, 0chroma crates/minimal/src/resolver.rs:397, 0chroma crates/minimal/src/resolver.rs:1344, 0chroma crates/minimal/src/resolver.rs:1875, 0chroma crates/minimald/src/net/dns.rs:224. No findings.
Panel: 3 of 3 personas ran (correctness, security, supply-chain).
…y-runtime-ports-0.7.0 # Conflicts: # scripts/install-host-classifier.sh
…#2714) T99. The hostname proxy reads the target box's runtime-published ports (an allowed expose, a published listener) from its live relay gate when the name resolves, so its verdict admits exactly the ports a direct connection reaches. Admitted requests log their route source at info, and refusals record route_source=none. Backport of #2714, folded from #2724. Adapted to the line: - BoxGate, BoxPolicy and boxes::DynamicIngress become SessionGate, sessions::SessionPolicy and sessions::DynamicIngress. - crate::traced::spawn becomes tokio::spawn in the new tests. - The line's spawn_backend_on takes a full address, so proxy_forwards_runtime_published_port picks its port with a probe listener. - The parity proptest checks the gate that carries the runtime publishes and asks admits_tcp, since admits_direct_tcp covers only the declared ports here. Also keeps proxy_refuses_a_withdrawn_port_like_direct (NET-069, from #2732): it publishes and withdraws through the gate only, because the line's listen watcher has no record_dynamic_hold. (cherry picked from commit 7097b96)
|
Folded into #3019, which landed on |
Backports #2714 (task T99, #2688) to
release/0.7.0. Through the hostname proxy, a port published inside a box at runtime (min net exposeor listen-publish) now opens as it does on a direct connection. The proxy's verdict equals the direct connection's in every network mode.Stacked on #2678; merge that first. #2678 carries the 0.7.3 version bump the release branch's version check needs. Until it merges, this diff also shows #2678's commits.
Commits
f614510c6is the cherry-pick, with conflicts resolved tomain's text. It does not compile alone. Squash-merge the pair.a69ee0758adapts it to the release branch:BoxGate,BoxPolicyandboxes::DynamicIngressbecomeSessionGate,sessions::SessionPolicyandsessions::DynamicIngress.crate::traced::spawnbecomestokio::spawn, sincetracedis not on this branch.spawn_backend_ontakes a fullSocketAddr, so the new test picks its port with a probe listener.admits_tcp.All of #2714's tests are kept.
43742d694addsproxy_refuses_a_withdrawn_port_like_direct(from test(minimald): prove the proxy refuses a withdrawn runtime port like a direct connection #2732), adapted to the release branch: it has norecord_dynamic_hold, so the test publishes and withdraws through the gate only.Verification (Linux, Lima)
cargo clippy -p minimald --all-targets --locked -- -D warningsis clean.just test-one proxy_ -p minimald: 43 passed. The runtime-publish, expose and parity tests also pass.minimaldsuite: 997 passed, 1 failed. The failure issession_host::tests::a_hook_launch_does_not_advise_unenforced_placement, which fails identically on the unmodified fix(net): backport the deny-all DNS refusal and the owner-only answerer socket to 0.7.0 #2678 head in the same VM: the VM's cgroup tree refuses placement with EACCES. It is not caused by this change.just fmt-checkpasses.