Repository navigation
T1 — Route two-label box names through the proxy, own-address boxes on the VM host included, and log every refusal - #1685
Conversation
A PTask's box name is now `<name>.min.internal`; the shipped zone also answers the former three-label form `<name>.<host-id>.min.internal` (default host id `local`) with a deprecation notice naming the two-label form (NET-002). An own-address box on a VM host — where the daemon holds a tap on the switch — routes straight to its lease, translating the requested port through the ingress declaration's external-to-internal map; on a native host the published-loopback model stays, and the name registers only once the attach path has reported the lease (NET-001). The lease is kept by the stable session id, so a rename re-registers against it. Every request the proxy refuses is logged with the host asked for, the reason and the status sent; a refusal whose host resolved names the session it resolved to.
…label-box-names-through-the-proxy-own
|
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: |
…label-box-names-through-the-proxy-own
There was a problem hiding this comment.
Reviewed against the T1 plan block on #1437 at 68c336b0, ahead of revise round 1.
Plan compliance. All ten layer files touched, none outside the set. Two deviations, both declared in the PR body and sound: the "two-label form" in sessions.rs is realised as the on_switch constructor argument (a HostShuttle transport means lease routes), and the rename test needed no change. Verified: the report key equals the deregister key on both sides, register_own_ip is a no-op before a lease at spawn, Stop and Destroy pass for_good=true and rename passes false, and dns_add_body emits both labels.
Test lines. The three plan tests exist; the task status shows only the revise banner and the PR body's test tails are compiler output, so the proof is the CI tests job at this head (run 36074852128), where all three pass. Five aggregators green.
Open thread (dns.rs:144, minimal-review MEDIUM, "the Lease route passes through any requested port"): valid, and worse than stated. The ingress gate is port-only with no source exemption (switch.rs:516-535), so the daemon's own SYN to an undeclared port is silently dropped, and TcpStream::connect in proxy.rs handle_connection_io has no timeout, so the client stalls through the kernel's SYN retries (about two minutes on Linux) before the refusal warn fires. The running revise round is on this finding. For it to close the thread the code has to refuse ports outside the ingress map before connecting, or at minimum bound the upstream connect; a comment rewrite alone will get another round.
Cosmetic. The impl HostRoute for RwLock<HostnameRegistry> comment still says "two HashMaps with no cross-field invariant"; the registry now has three. The 408 timeout refusal (30 s constant) is untested, acknowledged in the PR body, acceptable.
A lease route passed any port outside the ingress declaration's external->internal map through to the box's lease, but the box's ingress gate drops an inbound SYN to any undeclared port from any switch peer, the daemon proxy included: an unpublished port did not actually pass through, it hung on a silent drop until the connect timed out, while a declared internal port was reachable through the proxy directly by its internal number, which the published-loopback model does not allow. Route only the published ports: a port outside the map now refuses at the proxy with a 403 and a warn line naming the host, the session, the port and the reason, matching the gate's deny-all posture and the native-host model.
…label-box-names-through-the-proxy-own
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A browser through the proxy reaches an own-address box on the VM host at .min.internal on a port its ingress declaration publishes, the old three-label name still routes with a deprecation notice, and the daemon log names every refused request with its reason — an unpublished port is now refused right there with a 403 and a warn line carrying the host, the session it resolved to, the port and the reason, instead of passing through to a port the box's ingress gate silently drops. Notes from the agent: Revise round 1: one review thread (PRRT_kwDOPdfcxM6l0HQl), judged valid against the code — the lease route's pass-through (ports.get(&port).copied().unwrap_or(port)) does not hold: IngressGate::for_session (switch.rs:516) rides the box's switch→tap leg with no source exemption, so an unpublished port was a silent SYN drop (connect hang → timeout, against NET-014's refused-not-timed-out) while a declared internal port was reachable through the proxy by its internal number, which the published-loopback model does not allow. Took the reviewer's option (a): Route::target is now Route::upstream(port) -> Option, a lease route carries only the published external ports, Router::route returns None for an unroutable port, and handle_connection_io refuses it with 403 Forbidden and the structured warn line (host, session, port, reason, status) before any connect is attempted. Deliberate consequence: box.min.internal: no longer routes through the proxy on a VM host, matching the native-host published-loopback model; direct peer-to-lease reach of declared internal ports is unchanged (the gate still allows it). New test proxy_refuses_an_unpublished_port_and_logs_why covers the refusal end to end; the dns.rs lease-map test now asserts the drop of both an unrelated port and the internal number. Docs updated in place (dns module doc, Target::Lease, Router::route, log_refusal) — no behavior outside the lease route changed; Loopback routes alwa…[truncated] |
There was a problem hiding this comment.
Revise round 1 closes the port pass-through finding the right way: Route::target became Route::upstream -> Option<SocketAddr>, a Lease route returns None for any port outside the ingress map (including the internal number behind the map), and the proxy refuses instead of dialing a port the switch gate would silently drop. The test own_ip_on_a_vm_host_routes_to_the_lease_through_the_ingress_map now asserts both the unrelated and the internal port route nowhere. Runtime review at e8d7ec12: approved, no findings; all lanes green.
Approving and merging; findings from my earlier pass are unchanged and non-blocking.
T1 — Route two-label box names through the proxy, own-address boxes on the VM host included, and log every refusal
slice: S0
kind: task
covers: NET-001, NET-002
size: L
est: 380 code, 180 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: the bundle's daemon log tail carries every proxy refusal with the Host asked for, the reason and the session it resolved to, and every deprecated three-label request
observability: one warn log line per refusal with the host, the reason and the status sent; one info line per deprecated three-label request naming the two-label form
layers:
- daemon:
- crates/minimald/src/net/dns.rs: names become
<name>.min.internal; the legacy<name>.<host-id>.min.internalresolves to the same entry flagged deprecated; an own-address entry carries the lease address and its external-to-internal port map- crates/minimald/src/net/proxy.rs: handle_connection_io logs each refusal (timeout, bad request, no route, connect failure) with its reason and the deprecation notice; Router::route sends an own-address box on a VM host to lease:internal-port instead of guest loopback
- crates/minimald/src/session.rs: register_hostname registers own-address boxes once a lease exists rather than at spawn to loopback; rename docs drop the
.local.form- crates/minimald/src/net/provider.rs: network_for passes a registry handle so the own-address network can report its lease
- crates/minimald/src/net/gvproxy_network.rs: finish_own_ip_attach updates the registry with the lease and registers the two-label DNS name
- crates/minimald/src/net/policy.rs: dns_add_body adds the
<name>label beside the legacy one- crates/minimald/src/session_host.rs: the network_for call follows the new signature
- crates/minimald/src/exec.rs: the network_for call and the switch test follow the new signature
- crates/minimald/src/sessions.rs: registry construction and the rename test use the two-label form
- crates/minimald/src/server.rs: start_host_proxies names the two-label form in its messages
test: cargo nextest run -p minimald proxy_routes_min_internal_for_own_ip_session_on_vm_host
test: cargo nextest run -p minimald proxy_refusal_is_logged_with_reason
test: cargo nextest run -p minimald legacy_local_zone_routes_with_deprecation
produces: none
consumes: none
proof: A browser through the proxy reaches an own-address box on the VM host at
<name>.min.internal, the old three-label name still routes with a deprecation notice, and the daemon log names every refused request with its reason.Proof
A browser through the egress proxy reaches an own-address box on the VM host at .min.internal (the requested port is translated through the box's ingress declaration to the internal listener), the old ..min.internal form still routes to the same box with an info deprecation notice naming the two-label form, and every refused request lands in the daemon log — which the diagnostics bundle carries — with the host asked for, the reason and the status sent (and the session it resolved to, when one resolved).
Notes from the agent: All ten layer files changed; no out-of-scope files touched. Two design calls worth knowing: (1) the VM-host-vs-native route form is a registry-level on_switch fact taken from the switch transport at construction (HostShuttle ⇒ lease routes, LocalSpawn ⇒ published-loopback), since Manager::init already holds the switch; the sessions.rs layer line's 'two-label form' for construction is realized as this on_switch argument plus the comment. (2) OwnIp n 8000 ames register only once a lease exists: the attach path (finish_own_ip_attach) reports lease + ingress port map through a new OwnAddressReporter threaded SandboxLauncher → network_for → OwnIpNetwork → complete/finish_own_ip_attach; the registry keeps the lease by stable SessionId so rename re-registers without a fresh report, forget_own_address drops it at Stop/Destroy (deregister_hostname gained a for_good flag — rename passes false), and tasks pass None (exec layer note: they own no route). The deprecation info line fires per legacy-pattern request, routed or not; when unrouted the refusal warn also names the host. The 408 head-timeout refusal logs like the others but is not asserted by the test (30s const bound impractical to drive in-process) — the other three refusal kinds are asserted. sessions.rs's rename test asserts no hostname strings, so it needed no change despite the layer line. diag.rs was already bundling rotated daemon logs, so the new warn/info lines r…[truncated]
Tests
cargo nextest run -p minimald proxy_routes_min_internal_for_own_ip_session_on_vm_hostcargo nextest run -p minimald proxy_refusal_is_logged_with_reasoncargo nextest run -p minimald legacy_local_zone_routes_with_deprecationVerify
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 #1611