Repository navigation
T36 — Apply per-box source-addressed rules outside the VM from a host-side box table - #1764
Conversation
The VM's per-tap shuttle dialled the gvproxy switch socket directly, so every per-box egress decision lived inside the guest; an escape that owns the guest also owns the policy. The decision now lives on the host, outside the VM. - egress_gate.rs (NET-081) binds a socket beside the switch socket, relays the gvproxy upgrade head verbatim, then applies the shared sessions::core::egress verdict to every Ethernet frame per source address before writing it to the switch. A frame whose source no box holds is dropped as egress-unknown-source; source-less families (IPv6, undeclared, truncated) are decided by the shared verdict's family rules. Drops are warn-logged once per source and rule per minute. - box_registry.rs (NET-138) is the host-side table of every published namespace: name, switch and loopback addresses, admitted ports, and the compiled egress rules. Rows are filled in-process on the host (the client-driven registration path is a later task) and the gate holds only the read-only table handle. The guest node's own namespace is an ordinary row, allow-all until the node-plane baseline lands. - net.rs runs the gate on the switch runtime, beside the switch socket, and it dies with the runtime. shuttle.rs names the gate socket. run.rs creates the registry, clears a stale gate socket, and hands the table to the switch runtime. sessions becomes a normal dependency for the shared verdict.
The shuttle vsock port is registered at the gate's socket instead of the switch socket, so libkrun dials the gate and a box's frames reach gvproxy only after the host-side verdict admits them. If the gate never came up, the guest's connect fails and the relay reports no egress, leaving the boot itself unaffected.
…box-source-addressed-rules-outside-the
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Approved: no findings at Nothing to do. Verdict APPROVE, posted as a comment: the App cannot review its own pull request; the check and the label carry it. Clobber: none: |
There was a problem hiding this comment.
The macOS e2e failure is a regression this PR introduces, and it is deterministic: the daemon's zone answerer and DNS proxy can no longer publish on the host loopback (malformed gvproxy status line: "", run 36672153233).
Cause: the guest's gvproxy control channel shares the shuttle vsock port with the L2 relay. expose_proxy_on_host (crates/minimald/src/server.rs:1813), provider.rs:382 and net/mod.rs:716/736 all dial ControlChannel::Vsock { port: VSOCK_GVPROXY_SHUTTLE_PORT } and speak ordinary HTTP/1.1 (POST /services/forwarder/expose, POST /services/dns/...) framed by Content-Length (policy.rs:281). This PR repoints that vsock port at the gate socket (vm.rs) and serve_connection treats every connection as a POST /connect upgrade: it forwards the head verbatim, then relay_guest_to_switch parses everything after the blank line as u16-length-prefixed frames. A JSON body's first two bytes ({") become a bogus length, the gate errors with guest frame length … exceeds max (or drops the "frame"), gvproxy never receives the body, the guest reads EOF, and the publish retries forever. On Linux DM2 the control channel is a unix socket, which is why only the macOS/VM lane sees it.
Required change: only gate connections whose request line is the POST /connect upgrade (CONNECT_REQUEST). Any other request on the shuttle port is a control exchange and must be spliced both ways verbatim, response included, without frame parsing. Add a test that a POST /services/forwarder/expose request with a JSON body reaches the switch socket intact and its response reaches the guest. Bound the head read with a timeout as the advisory review comment on serve_connection asks (the native relay uses VSOCK_CONNECT_TIMEOUT).
Plan test lines and the four named tests are otherwise in scope; this is the one blocker.
serve_connection dialed the host switch and then blocked on reading the upgrade head with no time bound, so a guest that connected and stayed silent — or sent a head it never ended — held a host gvproxy switch connection and a gate relay task for the gate's lifetime. The peer the head comes from is inside the escape boundary NET-081 exists for, so the handshake is now bounded front to back by HANDSHAKE_TIMEOUT, the same fail-fast the native relay's VSOCK_CONNECT_TIMEOUT gives its connect + /connect upgrade: a refusal inside the bound brings the switch connection and the relay task down with it.
…box-source-addressed-rules-outside-the
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
On a VM-backed host, a box's undeclared connection is dropped by the host-side gate before the switch sees it and a frame whose source address belongs to no box never leaves the VM — and since this round the gate also refuses a guest that connects and stays silent inside a 5-second handshake bound, so nothing inside the escape boundary can park a host switch connection or a gate relay task for the gate's lifetime. Notes from the agent: Revise round 1, one thread, judged valid and fixed: serve_connection's handshake (dial the switch, read the upgrade head, forward it) is now bounded by HANDSHAKE_TIMEOUT (5 s, mirroring the native relay's VSOCK_CONNECT_TIMEOUT in crates/minimald/src/net/switch.rs); on the bound elapsing the future is dropped, which closes the host switch connection and the relay task and warns once. The three per-step warn lines are kept; a timeout gets its own. The bound reaches serve_connection as a parameter so the new test can shrink it to 200 ms — every caller outside this module's tests goes through accept_loop, which passes the constant. Scope held to the one layer file the finding named; nothing outside crates/minvmd/src/net/egress_gate.rs changed. just clippy-strict exits 2 on a pre-existing hit from round 1 (egress_gate.rs:603, clippy::unwrap_in_result on CaptureWriter::write's lock expect) — per AGENTS.md a revise round's directive bounds the change, so it is reported rather than fixed; it needs either a map_err or an #[expect] in a round that owns those lines. just ci was not run whole: it is the repo's pre-PR gate but compiles the entire workspace on a box with no incremental cache; the crate this round touched was verified with the clippy recipe's pinned flags plus a full -p minvmd test run instead. |
The warn in `serve_connection` said the gate "could not read the switch upgrade head", but that read is off the guest — the switch was already dialed and nothing is read from it in that branch. An operator debugging a refused connection would be sent hunting a host-side switch read that never happened. Name the guest.
|
/revise the macOS e2e lane is still red on 5714bea (
Change: in |
|
Round 2 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
On a VM-backed host, a frame leaving the VM is decided by the host-side egress gate (crates/minvmd/src/net/egress_gate.rs) before it reaches the switch: the box's published namespace supplies the rules, an undeclared connection is dropped rather than forwarded, and a frame whose source address no namespace in the host table holds is dropped outright before any rule is consulted — never leaving the VM. The daemon's log carries one rate-limited warn line per dropped frame class naming the source address and the rule, and the warn for an unreadable upgrade head now names the guest it reads from, so an operator tracing a refused connection is not sent hunting a host-side switch read that never happened. Notes from the agent: Round 2 (revise): the branch already carried the task's substance from earlier rounds (gate, host box table, shuttle/vm/run wiring, and the four covered tests, which all pass at the new head); this round's only change is the single review finding, the guest-naming reword of one warn line. Cargo.lock needed no change: main already records minvmd's direct |
|
/revise round 2 (97e31b6) changed only the warn wording; the macOS e2e regression from the request-changes review is still unaddressed and
|
The limiter kept one map entry per distinct (source, rule) pair with no cap or eviction, and the source address it keyed by is the dropped frame's own bytes — chosen by the guest, the side the gate exists to contain. A guest flooding the shuttle with frames from ever-different spoofed addresses, each dropped as egress-unknown-source, would grow host memory one entry per frame: a memory amplification against the very path the gate protects. The window table is now capped at DROP_WARN_MAX_TRACKED_PAIRS. Past the cap a pair the table holds no window for shares one rate-limited line per rule, so a flood costs no memory and at most one line per rule per interval; a pair the table already holds keeps its own window; and stale windows are pruned before the fallback is taken, so a flood that has ended restores per-source lines within one interval.
|
Round 3 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
On a VM-backed host, a box's undeclared connection is still dropped by the host-side gate before the switch sees it and a frame whose source address belongs to no box still never leaves the VM — and the gate's drop-log window table is now bounded, so a guest flooding the shuttle with frames from ever-different spoofed source addresses can no longer grow host minvmd memory one entry per frame: past the cap the flood's drops share one rate-limited line per rule while every held source keeps its own line, and stale windows are pruned so per-source lines return within one interval of the flood ending. Notes from the agent: This round addressed the single review thread: the drop limiter's window table. The fix is contained to crates/minvmd/src/net/egress_gate.rs (a named layer): a DROP_WARN_MAX_TRACKED_PAIRS cap of 1024 on the (source, rule) window map, a per-rule DropKey::Overflow shared window for pairs past the cap (one line per rule per interval, naming the flood rather than a junk source), and a stale-window prune before the fallback so the fold is a flood's shape and not a permanent state. should_warn_at now returns a WarnDecision (Named/Overflow/Silent) so the three outcomes are testable without a clock or subscriber, as before. The strict clippy hit reported in tests is on lines an earlier round wrote in the test_support CaptureWriter (egress_gate.rs:692, clippy::unwrap_in_result on the mutex lock inside io::Write::write); this round's directive names the limiter only, so per the repo's clippy-strict contract it is reported rather than fixed — it never turns a PR red. Nothing was touched outside the task's layers; no deferred lanes this round. |
The endless-head test sent its noise after `gate_over` had already completed the /connect handshake, so the relay read the bytes as a 2-byte length prefix and the refusal came from the over-length-frame rejection, not the MAX_HEAD bound the test is named for. Split the harness so a test can hold the gate at the head-read (`gate_connected`, before any head is forwarded), send the noise as the guest's first write, and assert the refusal on the bound's own log line so the byte-count check is what fired, not the handshake timeout. The over-length-frame rejection keeps its own correctly named test, which was the only path the old test exercised.
`BoxRegistry::register` documented that a re-registration "can only ever tighten or restate, never widen behind the gate's back", but the code inserts unconditionally and compares no policy — and an absent dimension is allow-all, so a widening restatement is as expressible as a narrowing one. Say what the table actually does: the newest declaration replaces the row, whatever it holds, and whether a re-declaration may widen is the registering side's contract (T66's client-driven path), while what the table enforces is NET-138's boundary — only the host process publishes at all. Pin the semantics by restating the row wider than the declaration it replaces and asserting the gate now decides by the wider reach.
|
Round 4 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
The host-side gate still drops a box's undeclared connection before the switch sees it and never lets a frame from an unheld source leave the VM — and the tests and docs that say so now say what they mean: the endless-head test really drives the gate's MAX_HEAD bound (refused in 22 ms on the bound's own log line, not the 5 s timeout), the over-length frame claim has its own named test, and the box registry's doc states the replace-with-newest semantics its test pins instead of a monotonicity the code never had. Notes from the agent: Both findings judged valid and fixed in commits 56a5901 (egress_gate) and aedac75 (box_registry); nothing outside the layer files was touched. Chose the reviewer's second remedy for the registry: enforcing tighten-only at the table would misfire on a recycled lease (a new box on a reused address must get fresh rules) and is T66's registration-path decision; the rewritten doc says so explicitly. Also note for the next round: |
There was a problem hiding this comment.
Round 4 still lacks the control-channel passthrough; inline sketch on the exact line below.
`serve_connection` spawned `copy_switch_to_guest` and then awaited the egress leg, and only ever `abort()`ed the ingress task: its completion was never observed. So a switch that closed one connection's end — without the whole gvproxy process exiting behind it, which the supervisor catches — ended the ingress leg silently while the egress leg stayed blocked on `guest.read_exact`, holding the relay task, the gate's dial, and the switch write half open until the guest's next frame came and failed to write. A guest that was idle stayed connected to a switch that was no longer there. Race the two legs in a `select!`, because neither can see the other's end: the egress leg waits on the guest, which has no reason to speak while it is idle, so the ingress leg's completion is the only thing on this side that knows the switch hung up. Whichever leg ends first takes the relay down with it, and the switch-side ending is said in the log, so a box whose egress went dark with the guest still on the connection leaves a line in the daemon log tail explaining why. The leg that loses the race is torn down rather than left holding its half. `a_switch_that_hangs_up_takes_the_relay_down_with_it` drives it through the stand-in switch closing its end while the guest is idle, and fails against the await-then-abort shape: without the race the gate emits no line and leaves the guest's side hanging past the deadline.
|
Round 5 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
On a VM-backed host, a box's undeclared connection is dropped by the host-side gate before the switch sees it, and a frame whose source address belongs to no box never leaves the VM — and after this round the relay that carries the admitted frames ends when the switch hangs up on it: the gate closes the guest's side at once instead of holding an idle guest open against a switch that is no longer there, and the daemon log names which leg ended. Notes from the agent: Revise round 5. One new commit, d40efc1, touching only crates/minvmd/src/net/egress_gate.rs; the PR's wider set (box_registry, the net/shuttle/vm/run wiring, minvmd's Cargo.toml) is rounds 1-3's and is unchanged. Nothing new was owed there: the layers' |
|
The runtime will not revise this pull request again on its own: round 11 would exceed the cap of 10 (the last trigger was a failed check suite). A person decides now. Remove |
CI (Linux KVM) was red on every head of this PR: rule 0 dropped any frame whose source had no row, the table held only the guest daemon's own tap, and an own-address box's lease is minted inside the VM by the guest daemon's allocator, so no host-side process can name it. The box's ARP reply was dropped as egress-unknown-source, the neighbour probe gave up, and NET-001 failed with 502 / EHOSTUNREACH. The creator-side registration that supplies those rows is T66's (#1711), and the lease it would register is not a fact any host-side process holds today, so the rows cannot be filled in this round. What lands is the interim the review names: rule 0 now hard-drops sources outside the plan's lease block - the gateway, the host alias, the daemon address, every foreign address - and an in-plan source no row holds is admitted under the shipped allow-all default: the reach the box had before the gate existed, which the in-guest relay still bounds by its own declared rules. Every admit warns, rate-limited, naming T66. Registered rows are enforced exactly as before. The posture is a named phase, UNREGISTERED_SOURCE_PHASE, modelled on the guest's own EGRESS_DEFAULT_PHASE rollout: T66 flips the one constant, the start-up line logs it, and both arms are pinned by tests - unknown_source_default_deny on the interim that ships, gate_verdict_decides_by_source_and_rules on the arm it flips onto. Own-address boxes' frames are back on the wire on the VM lanes; spoofing outside the lease block stays dropped, as do IPv6 and undeclared families. Refs: #1711
|
Round 10 of 10 on a review requesting changes, pushed as
Verify Review threads the coder judged:
On a VM-backed host, a box's undeclared connection is still dropped by the host-side gate before the switch sees it, a frame whose source the address plan could never hand to a box still never leaves the VM, and a frame from an in-plan address no published row holds is admitted under a named, warned interim — so the own-address boxes whose ARP replies rule 0 was killing at the KVM lane's boots are back on the wire. Notes from the agent: Register-at-create could not be the fix this round: an own-address box's lease is minted inside the VM by the guest daemon's IpAllocator (crates/minimald/src/net/mod.rs), minvmd is not in the SSH/vsock-bridge data path (libkrun splices the UDS), and the CLI (crates/minimal) never learns the lease — no host-side process can name it, and the two crates that would have to change are outside this task's layers. So this round ships the review's admissible interim, and the account against its six conditions is: (1) met — rule 0 still hard-drops every source outside the plan's lease block (gateway, host alias, daemon address, network/broadcast, all foreign addresses) and every undeclared/IPv6/truncated family; (2) met — registered rows are enforced exactly as before (host_side_rules_applied_outside_vm unchanged and green); (3) not deliverable in scope — there is no node-attribute surface in this tree (own_ip_enforcement, per_box, LocalVM and pinned appear nowhere; the only analogous concept is sessions' EGRESS_DEFAULT_PHASE opt-out), and surfacing an attribute at session start would live in crates/minimal and crates/minimald-rpc, outside the layers; the gate instead logs its posture on its start-up line (unregistered_in_plan_sources) and warns on every admit; (4) met — a rate-limited warn (60 s per source/rule, 1024-pair bound) naming T66 (#1711) on every admitted-unregistered source, plus an overflow line at flood; (5) met — enforcement is pinned by tests with the flip being the one constant UNREGISTERED_SOURCE_PHASE (unknown_…[truncated] |
|
The runtime will not revise this pull request again on its own: round 11 would exceed the cap of 10 (the last trigger was the runtime's review findings). A person decides now. Remove |
There was a problem hiding this comment.
REQUEST_CHANGES on 5d19dd1 (round 10 of 10).
Round 10 chose the observe-mode interim rather than the registration fold: gate_verdict admits any unregistered source inside the guest's lease run under UNREGISTERED_SOURCE_PHASE = Announced, warns once per source per minute naming T66 (#1711), and still hard-drops out-of-block, infrastructure and foreign-family sources; registered rows are enforced as before. That plausibly turns CI (Linux KVM) green (the own-address box's ARP reply and SYN-ACK now pass the gate; lane still running at review time).
Against the ARCH ruling this is non-conforming as it stands: conditions 1, 2 and 4 are met, 5 is a single pure-function pin with no relay-level InForce test, and 3 and 6 are absent. No own_ip_enforcement attribute exists and nothing reaches a client at session start; and the PR body still asserts the opposite of the code. The Proof section says "a frame whose source address belongs to no box never leaves the VM", and the agent's notes say an in-guest-allocated box's frames "are dropped as egress-unknown-source" and the own-IP proofs "will not pass until T66". Both are false for 100.64.0.2..100.64.255.252 at this head; the interim and the link to #1711 exist only in the commit message.
Minimum to be mergeable as an interim: rewrite the Proof and Notes to state the interim (in-block unregistered sources admitted allow-all, warned under egress-unregistered-source, until #1711 supplies rows), delete the "dropped ... will not pass until T66" sentence, add the relay-level InForce test, and either land the node attribute or record an explicit, linked deferral of it with the LocalVM pinned caveat. Those are small; the round budget is spent, so how they land is the owner's call.
The default-deny thread from the previous review stays open, marked carried; four threads here (1 BLOCKING, 1 carried, 1 MEDIUM, 1 NIT).
|
The runtime will not revise this pull request again on its own: round 11 would exceed the cap of 10 (the last trigger was a review requesting changes). A person decides now. Remove |
|
/revise |
…box-source-addressed-rules-outside-the
…interim admits The egress gate's unregistered-source decision is now taken from a phase handed down the relay path instead of read from a constant mid-relay: spawn keeps the shipped announced interim and is the only constructor production callers reach, while a new spawn_with_phase lets a test build the gate with the per-box default in force. The relay-level test unregistered_sources_dropped_when_default_in_force drives that built gate end to end — a made-up lease, a withdrawn lease, and an ARP announcing an unpublished in-block address are all dropped outside the VM, the published box's frame is the only one relayed, no interim warn fires, and the table is untouched — so the flip T66 (#1711) makes has its proof standing rather than tests to rewrite. Docs are reconciled with what ships: the gate's module posture, the phase enum's InForce arm, the vm.rs frame-path comment, and the run.rs registry comment now say that under the announced interim an in-plan source no row holds is admitted with a rate-limited warn naming T66, and that a registered box can be escaped by sourcing frames from any unregistered in-block address — NET-081's host-side guarantee is deferred to T66 until its creator-side rows land and it flips the constant.
|
Round 1 of 7 on a /revise note, pushed as
Verify Review threads the coder judged:
On a VM-backed host, a box's undeclared connection is still dropped by the host-side gate before the switch sees it, and the gate's unregistered-source decision is now a phase handed down the relay path instead of a constant read mid-relay: production spawns keep the shipped announced interim — an in-plan source no published row holds is admitted with a rate-limited warn naming T66 (#1711) — while a new tests-only constructor (spawn_with_phase) builds the gate with the per-box default in force. The new relay-level test drives that built gate end to end: a made-up lease, a withdrawn lease, and an ARP announcing an unpublished in-block address are each dropped outside the VM, the published box's frame is the only one relayed, and no interim warn fires — so T66's flip has its proof standing rather than tests to rewrite. The second half of the task's proof is NOT whole today and the code now says so plainly: the gate module doc, the phase-arm docs, and the vm.rs/run.rs comments state that 'a frame whose source address belongs to no box never leaves the VM' holds outside the plan's lease block today and everywhere once T66 flips UNREGISTERED_SOURCE_PHASE, and that under the shipped interim a registered box can be escaped by sourcing frames from any unregistered in-block address. Notes from the agent: Round work: condition (5) of the interim is complete in-layer — the phase is injected at spawn (spawn_with_phase; production callers reach only spawn, which cannot choose a phase) and unregistered_sources_dropped_when_default_in_force carries the two removed relay-level pins. Condition (6) is stated in run.rs's registry comment and in proof above for the PR body. T66 handoff: after its flip the dead variant in non-test builds becomes Announced and the dead_code expectation moves to it (now documented on the InForce arm too); T66 should also carry the test that its flip closes the registered-box escape. Condition (3) is outside this task's layers (minvmd only): a…[truncated] |
Superseded: ARCH-EXPERT accepted the observe-mode interim on 548710d with condition 3 as an explicit linked deferral; a fresh review follows on the round-2 head.
The head-read's MAX_HEAD bound was checked only after each read, but a read_buf into a growable buffer doubles its reserve read by read: a guest talking past the bound without ending a head drove each read's own allocation up with it and crossed MAX_HEAD within one read (measured 6.8 KiB of transient host memory on a 1 MiB write, whatever the bound says). The guest is the untrusted peer, and every other guest-scaled cost on this path is capped deliberately. The head is now read into a fixed 512-byte scratch, so each read adds a bounded step and the buffer's high-water mark is MAX_HEAD plus one chunk whatever the guest writes. Semantics are unchanged: a head that ends is still accepted with its carry, a head that never ends is still refused with the same message, and the honest heads the gate sees (the shuttle's 43-byte upgrade, the daemon's few-hundred-byte control verbs) read in one read as before.
|
Round 2 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
On a VM-backed host, the host-side egress gate decides every frame the guest sends before the switch sees it: a published box's undeclared connection is dropped by its row's rules, and a frame whose source address belongs to no box is dropped — today outside the plan's lease block, everywhere once T66 (#1711) flips UNREGISTERED_SOURCE_PHASE, whose in-force arm is pinned at relay level by unregistered_sources_dropped_when_default_in_force. This round adds: the gate's first-request head read is capped at a fixed 512-byte scratch, so a guest cannot drive the read's own host-memory growth past the MAX_HEAD bound with one read. Notes from the agent: Revise round 2: one code change, for the new LOW finding (thread 4152281181) — read_request_head now reads into a fixed HEAD_READ_CHUNK (512 B) scratch instead of a read_buf'd Vec, so the head buffer's high-water mark is MAX_HEAD plus one chunk whatever the guest writes; semantics, error messages and honest-peer behaviour unchanged (commit ab5355e). Basis: measured tokio read_buf growth empirically (scratch probe, tokio 1.53.1) — it doubles its reserve read by read, so one read carried head past MAX_HEAD to 6848 B on a 1 MiB write; the finding's 'roughly the socket's read size' magnitude is overstated but the bound-after-growth mechanism held. clippy-strict: the indexing_slicing hit on my new line was fixed (take(n)); one hit remains at egress_gate.rs:1642 (unwrap_in_result, CaptureWriter::contents) — code from the round-1 feature commit 5acc96b, test support outside this round's directive, so reported here rather than fixed. Thread rulings against the branch head: the interim's in-layer conditions are all met (548710d) — spawn_with_phase injects the phase, unregistered_sources_dropped_when_default_in_force pins default deny at relay level, run.rs:438-446 and the module doc state the provision; what remains of the blocking findings (register each own-address box's lease from the host-side creator; the own_ip_e…[truncated] |
There was a problem hiding this comment.
Approved at ab5355e. Round 1 (548710d) discharged the observe-mode interim's in-layer conditions: rule 0 and registered rows enforced, spawn_with_phase tests-only with production fixed on Announced, unregistered_sources_dropped_when_default_in_force pinning the in-force default end to end through the relay, and the register-at-create dispute verified against the tree (minvmd has no host-side path that learns a box's address before its first frame). ARCH-EXPERT: PASS as interim, condition 3 as the explicit linked deferral #1775 under the guard that no surface claims pinned or enforcement for own-IP boxes on the local profile (grep-clean on this head); condition 6 is in the PR body. Round 2 (ab5355e) caps the head read with a fixed scratch. All threads resolved. Merge once the KVM and e2e lanes and minimal-review report green on this head.
A guest that speaks its control request and then stays connected holds the relay for the gate's lifetime when the switch neither answers nor closes: the request leg's probe read waits on a guest whose silence is its normal posture while it waits for the answer, the response leg waits on a switch that never speaks, and the drain bound — added to release a relay on a switch that will not answer — was reachable only through the request leg's end, which neither shape supplies. The probe read now runs under the same drain bound the answer's drain runs under. Past it the leg ends as a new `GuestSilent` end and the release runs through the existing bounded drain, which half-closes the switch's side and drains the answer so an exchange that completes within the bounds is delivered as before. The bound wraps only the probe, not the whole exchange, so a body relayed slowly is not cut off mid-body.
|
Round 3 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
On a VM-backed host, a box's undeclared connection is still dropped by the host-side gate before the switch sees it and a frame whose source address belongs to no box never leaves the VM. New this round: a control relay whose guest speaks its one request and then waits — its normal posture while waiting for the answer — with a switch that neither answers nor closes, is released at the drain bound instead of holding the relay task, the guest socket and the switch dial for the gate's lifetime, and a test pins that silent-guest shape beside the guest-closes shape the existing drain test covered. Notes from the agent: One finding, judged valid against the code and fixed: relay_control's select reached finish_control's drain_timeout only through the request leg's end, and a guest silent after its request (probe read pending) with a wedged switch (response leg pending) left both arms pending forever. The fix bounds the probe read with drain_timeout — not the whole select, which would cut a legitimate slow or large body relay mid-body — and adds ControlEnd::GuestSilent, which releases through the existing bounded drain in finish_control, so an exchange that completes within the bounds is still delivered whole. Tests: cargo clippy -p minvmd --all-targets clean; just clippy-strict reports one hit, crates/minvmd/src/net/egress_gate.rs:1681 (unwrap_in_result on CaptureWriter::write's lock expect) — a line from an earlier round of this branch, outside this round's delta, so reported here and not fixed. |
There was a problem hiding this comment.
Round 3 at 3509367 reviewed: the request leg's probe read is bounded by drain_timeout, a guest that only waits ends the leg as GuestSilent and the answer still drains under the same bound, so a wedged switch can no longer hold the relay for the gate's lifetime. Control exchanges slower than the handshake bound plus the drain bound now end unanswered; gvproxy's control endpoints are local and answer in milliseconds, so that is the right trade. Approval re-issued on this head; merge once build-linux and minimal-review conclude.
T36 — Apply per-box source-addressed rules outside the VM from a host-side box table
slice: S9
kind: task
covers: NET-081, NET-138
size: L
est: 650 code, 180 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: the bundle's daemon log tail carries the VM host daemon's rate-limited drop lines by source address and rule
observability: one rate-limited warn log line from the VM host daemon per dropped frame class with the source address and the rule
layers:
- vmhost:
- crates/minvmd/src/net/egress_gate.rs (new): sits between the guest's shuttle and the switch socket, applies the shared frame verdict per source address and drops frames from any source no box holds
- crates/minvmd/src/net.rs: declares the gate module and runs it on the switch runtime
- crates/minvmd/src/net/shuttle.rs: names the gate's socket beside the switch socket
- crates/minvmd/src/vm.rs: points the shuttle vsock port at the gate's socket
- crates/minvmd/src/cmd/run.rs: starts the gate before the VM
- crates/minvmd/src/box_registry.rs (new): the host-side table of every published namespace (NET-138): each box's switch and loopback addresses, name, admitted ports and egress rules, filled from the client on the host and never from the guest (the registration path is T66); rows are filled in-process here and the gate reads them
- crates/minvmd/src/lib.rs: declares the registry module
- crates/minvmd/Cargo.toml: depends on sessions directly for the verdict
- Cargo.lock: records the direct dependency
test: cargo nextest run -p minvmd host_side_rules_applied_outside_vm
test: cargo nextest run -p minvmd unknown_source_default_deny
test: cargo nextest run -p minvmd host_table_holds_every_published_namespace
test: cargo nextest run -p minvmd host_table_never_sourced_from_guest
produces:
- crates/minvmd/src/box_registry.rs
- crates/minvmd/src/net/egress_gate.rs
consumes:
- crates/sessions/src/core/egress.rs
proof: On a VM-backed host, a box's undeclared connection is dropped by the host-side gate before the switch sees it, and a frame whose source address belongs to no box never leaves the VM.
depends on:
- blocked by #1629
Proof
On a VM-backed host, a box's undeclared connection is dropped by the host-side gate before the switch sees it: the guest's shuttle vsock port dials the gate's socket, the gate applies the shared
sessions::core::egressverdict per source address against a host-side table no guest can write, every registered row is enforced, and every drop leaves one rate-limited warn line naming the source address and the rule in the daemon log the bundle's tail carries. Rule 0 hard-drops every source outside the plan's lease block, every infrastructure address, and every foreign address family.Interim, stated. The second half of the task's proof, "a frame whose source address belongs to no box never leaves the VM", is not whole at this head. The gate's unregistered-source phase ships as
Announced(crates/minvmd/src/net/egress_gate.rs,UNREGISTERED_SOURCE_PHASE): a frame from a source inside the lease block that no published row holds is admitted allow-all with a rate-limited warn naming T66. The reason is sequencing: the lease is minted inside the VM by the guest daemon, minvmd has no host-side path that learns a box's address before its first frame, and the client-driven registration that fills the table is T66's (#1711), which this task blocks. Consequences while the interim holds: a registered box can escape its own row by sending from an unregistered in-block source, and a LocalVM host must not claimpinnedfor own-IP boxes. The per-box default is already proved:unregistered_sources_dropped_when_default_in_forcebuilds the gate withInForcethrough a tests-only constructor and drives a made-up lease, a withdrawn lease, and an ARP announcing an unpublished address through the relay end to end, each dropped outside the VM. T66 flips the shipped constant and inherits that test; thedead_codeexpectation moves toAnnouncedwith it.Deferred out of this task's layers. The node attribute
own_ip_enforcement = per_box | none, surfaced at session start (ARCH ruling, condition 3), lives in sessions/minimald/minimal and is not reachable from minvmd. It is tracked as #1775, blocked by T66, per the ARCH-EXPERT verdict on the review thread; until it lands no surface claimspinnedor enforcement for own-IP boxes on the local profile.Notes from the agent: Cargo.lock is unchanged on purpose: its per-crate dependency list already records sessions for minvmd because the lock holds the union of normal and dev dependencies, so moving sessions from dev-dependencies to dependencies produces no lock diff (verified against git status); the layer line 'records the direct dependency' is satisfied by the existing entry. The gate is table-pure: the guest node's own namespace is an ordinary row (name "minimald", switch_addr = the subnet's daemon IP, loopback = localhost, egress = None, i.e. allow-all) registered in-process in run.rs, which keeps the daemon's own package fetches and HostNet sessions working until NET-130's node-plane baseline lands; the client-driven registration path is T66's, so a box whose address is allocated in-guest is unknown to the host table in the interim and its frames are dropped as egress-unknown-source — the plan's accepted sequencing, and it means own-IP e2e proofs riding real guest traffic will not pass until T66. vm.rs and run.rs's VM-spawn regions sit behind #[cfg(minvmd_libkrun)], which is off on this box, so they are parsed but not type-checked here; their shape was checked against the identical sibling call sites, and full verification of the VM path needs a KVM host (just test-cross / just e2e), which this box cannot run. just clippy-strict was not run: it is a local-only gate with no CI lane, the new code was written strict-clean by construction (underscore-bound RAII…[truncated]
Tests
cargo nextest run -p minvmd host_side_rules_applied_outside_vmcargo nextest run -p minvmd unknown_source_default_denycargo nextest run -p minvmd host_table_holds_every_published_namespacecargo nextest run -p minvmd host_table_never_sourced_from_guestVerify
min task run agent-verifypassed ([tasks.verify]).Base
The base branch moved while the coder worked; it was merged in before the tests ran.
Closes #1640