Repository navigation
T32 — Complete the toolchain under a hostname-only allowlist - #1771
norrietaylor merged 22 commits into
Conversation
…he-toolchain-under-a-hostname-only-all
|
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. Clobber: none: |
There was a problem hiding this comment.
REQUEST_CHANGES on b02b5c3.
Three compile errors put the new harness on the floor wherever minvmd's tests build with libkrun (the macOS unit job on this head is already red: E0433, E0603, E0283; the KVM archive step will follow). Past compiling, the test cannot pass where it runs: the lanes never export MINVMD_VM_OWN_IP or MINVMD_GVPROXY_BIN before the harness step, the box is created with only the launcher baseline packages so none of the five tools exist, and the fixture omits hosts the exact-match gate needs (files.pythonhosted.org, auth.docker.io, production.cloudflare.docker.com). The plan's diagnostics line reads a log file nothing writes.
ARCH-EXPERT design verdict: PASS on the gate semantics the unit test proves (§5.3 l145/l149); the five-name fixture does not by itself discharge NET-068, and the design has no redirect-follow to close that gap. allow_protocols has no counterpart in the design's Box Spec (see the dns_gate thread).
The daemon unit test is not vacuous (it drives the real relay) but proves nothing toolchain-specific beyond "five names pin". Scope is clean: only the two layer files changed.
Threads: 6 BLOCKING, 3 MEDIUM, 1 LOW. Also flagged by the strict gate: last_err assigned but never read, unused AsyncReadExt/AsyncWriteExt in run_session_exec.
The integration test crate cannot resolve crate::cmd::own_ip_requested. Use the public minvmd::cmd::own_ip_requested path instead. Review: PRRT_kwDOPdfcxM6ngj6Y
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
The review finding is fixed: crates/minvmd/tests/egress_allowlist_integration.rs now calls minvmd::cmd::own_ip_requested() so it compiles on hosts where minvmd_libkrun is set. Notes from the agent: The deferred VM test (cargo nextest run -p minvmd hostname_allowlist_toolchain_completes) is written but cannot run in this sandbox; it remains lane:vm per the survey. No other paths were touched. |
…ration test - Move the toolchain hostname list into sessions::NET068_TOOLCHAIN_EGRESS_HOSTS so the minimald unit fixture and the minvmd integration test share it. - Fix integration-test compile issues: use sessions::SessionId, remove private minimald_rpc::SessionId, gate on MINVMD_GVPROXY_BIN instead of MINVMD_VM_OWN_IP, and fix FinalizeSession error typing. - Upload a minimal.toml with toolchain packages before ConfigureLoadout; replace podman pull with skopeo copy; add per-tool timeouts and small git clone target. - Read MINVMD_BOOT_LOG (falling back to boot.log) and boot with RUST_LOG=info,minimald::net::dns_gate=debug for diagnostics. - Document that the DNS gate lives in the guest minimald and that the host gvproxy is the L2 switch. - In dns_gate.rs, reword the negative assertion to name the design divergence and note that the TCP-only fixture still resolves because of the UDP/53 resolver carve-out.
|
Round 2 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
The hostname-only egress allowlist integration test is updated to compile on libkrun hosts, gates on MINVMD_GVPROXY_BIN, uploads a minimal.toml so the box has toolchain packages, replaces podman with skopeo, adds per-tool timeouts, reads the guest boot log with dns-gate debug logging, and shares the toolchain host list with the minimald DNS-gate unit fixture. The unit test that exercises the gate against that host list still passes. Notes from the agent: The VM integration test itself (cargo nextest run -p minvmd hostname_allowlist_toolchain_completes) is deferred to the libkrun VM lanes/MINVMD_E2E=1 environment, which this sandbox cannot run. I verified type-correctness by building with RUSTFLAGS='--cfg minvmd_libkrun'. I did not edit any workflow files; the reviewer noted CI needs a CODEOWNER change to fetch gvproxy before the harness step for this test to run automatically, and I left that for a follow-up. The shared host list is best-effort based on the review threads; the real set should be recorded from an integration run. |
|
Round 3 of 10 on the runtime's review findings, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
The hostname-only toolchain allowlist is shared between the minimald DNS-gate unit test and the minvmd VM integration test via Notes from the agent: This was a revise round. The only remaining open review thread was a one-word doc-comment typo in |
There was a problem hiding this comment.
Round-3 head 5ee1fd5 (round 2 plus the one-line EgressPolicy doc fix) reviewed.
Round-2 head 3c06ac2 reviewed. The round did real work and eight of the ten threads are resolved below: the harness now compiles cleanly under minvmd_libkrun (checked with cargo check --tests -p minvmd --locked at this commit: exit 0, no warnings), the three compile errors are gone, the gvproxy gate skips with a reason, the boot-log tail plus dns_gate=debug meets the diagnostics line, every exec is bounded, the allowlist is one shared 11-name exact-match fixture, and the TCP-only/DNS carve-out claim checks out against egress::verdict_ipv4.
Still REQUEST_CHANGES, because the proof NET-068 needs cannot pass as written, for reasons visible without a VM:
- Three of the five legs exit non-zero on tool absence, not on egress (inline, HIGH ×3):
apt-getdoes not exist in the composed box (noaptpackage ingominimal/pkgs@f4de33d0, no Debian userland), thepythonpackage shipspip3and no barepip, andskopeo copyaborts on the missing/etc/containers/policy.jsonbefore it resolves a single name. A green run is impossible and a red run says nothing about the allowlist. The fixes are one-liners (pip3,--insecure-policy, drop-or-justify apt); thread 5 asked for the apt leg to be decided honestly and that part was skipped. - Thread 4 stays open (PARTLY): no
justfileorscripts/path exportsMINVMD_GVPROXY_BINtojust test-vm(justfile:582 / :608), so the harness is skipped onci-linux-kvm,ci-macosandjust test-vmalike, and neither the body nor the round comment records that the lane needs a CODEOWNER change. The deferred line still has nothing that can produce its evidence. - Thread 5 stays open (PARTLY): the
minimal.tomlupload and the package set are right; the apt leg is item 1. - LOW/MEDIUM inline: the exec budget vs the
vmprofile's 6-minute kill,read_to_stringon a console log, and the fixture insessions' public API without#[doc(hidden)].
Draft: the out-of-layer edit to crates/sessions/src/lib.rs is the one this review asked for, so it should be un-drafted by a person rather than reverted. Allowlist note for the first real run: Docker Hub also serves layer blobs from its r2.cloudflarestorage.com host, which is not in the fixture; the boot-log tail will name it if it is hit.
The composed box has no Debian userland or apt package, so drop the apt-get leg and document the NET-068 deviation. Use pip3 (the only pip in the python package) and skopeo --insecure-policy (no containers policy.json ships). Cap per-tool exec at 60s and make the boot-log tail robust to invalid UTF-8 while filtering to dns-gate/policy lines so a chatty tool cannot push the admission evidence out of the cap.
… a gvproxy export Hide the shared NET068_TOOLCHAIN_EGRESS_HOSTS constant from public docs. Export MINVMD_GVPROXY_BIN in both test-vm recipes so the own-IP integration tests can run, and add a vm-profile override for hostname_allowlist_toolchain_completes so the 6-minute default kill does not cut off a real toolchain run.
…he-toolchain-under-a-hostname-only-all
|
Round 4 of 10 on a review requesting changes, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
The minimald unit fixture Notes from the agent: The apt leg required by NET-068 is intentionally dropped from the integration test: |
There was a problem hiding this comment.
Round 4 addresses the toolchain legs it could (pip3, skopeo --insecure-policy, the 60 s per-tool bound with a vm profile override, lossy UTF-8 tail, #[doc(hidden)], and the gvproxy export in both test-vm recipes; the KVM lane already exports MINVMD_GVPROXY_BIN, so the test now runs there). Not settled: the apt leg was removed rather than made to pass, and NET-068 names apt explicitly; the PR body still says apt is asserted, contradicting the new comments. That is a requirement-level decision, raised to the walk owner separately. The KVM run on a8582b4 (36732132542) is the first to execute hostname_allowlist_toolchain_completes with gvproxy; its result decides whether the four remaining legs complete from the runner.
|
(see above) |
Spec PR #1707 amended NET-068 to the four-tool toolchain (git clone, npm install, pip install, a container pull): the composed box has no Debian userland and no upstream package carries apt, so an apt leg can never exit 0 there. - Module doc and in-file comments now state the four-tool toolchain and no longer claim apt is asserted or recorded as a deviation; the spec matches the test. - `deb.debian.org` and `security.debian.org` leave `sessions::NET068_TOOLCHAIN_EGRESS_HOSTS`; the only other user, the `minimald` DNS-gate unit test `toolchain_hosts_admitted_by_gate`, drops the same two rows. - The boot-log tail no longer keys on the `dns-gate` component string, which the guest console need not print; it matches the gate's message text alone (`admitted`, `refused`, `rule_matched`) and, when nothing matches, returns the last 40 unfiltered lines so the console stays visible.
`minvmd boot --foreground` never spawns the host gvproxy switch (only the `run` supervisor does), so the shuttle bridge dialled a socket nothing served and the guest had no egress: the composer could not fetch the upstream packages and every toolchain leg failed to spawn before the allowlist was exercised. - `Guest::boot` now runs `minvmd run --detach --timeout 150`, polls `status --json` until Running, and stops the VM on drop, bounded like the sibling harnesses; boot failures quote the supervisor's `run.log`. - With the packages composed, `ConfigureLoadout` routes their env wiring back as `Pending`; the harness gates it with the `sessions` client handler and an approve-all `PolicyHooks`, then `SubmitVerdict`. An `Approved` patch would need the patches upload the CLI performs, so it is reported instead. - `camino` joins minvmd's dev-dependencies: the `PolicyHooks` trait names `Utf8Path` in its signatures.
…NET-068 run The container pull stalled under the hostname-only allowlist: Docker Hub's blob fetch 307-redirects to `production.cloudfront.docker.com` (18.238.0.0/16), which the list did not name, so the relay refused the flow as `egress-undeclared-subnet`. Docker Hub has served blobs from either of its CDNs, so the list now carries both `production.cloudflare.docker.com` and `production.cloudfront.docker.com`, with a doc comment saying why; the `minimald` DNS-gate unit table mirrors the row. Also read the status JSON with `.get(..)` in the harness's Running poll instead of indexing, clearing the strict-lint hit the previous commit introduced.
|
Round 5 of 10 on a failed check suite, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
At this branch a person can see the NET-068 harness end to end: crates/minvmd/tests/egress_allowlist_integration.rs boots a VM box through the supervisor path (the only path that stands up the host gvproxy switch), gives it a hostname-only egress allowlist shared with the minimald unit fixture, and asserts git clone, npm install, pip install and a container pull (skopeo) all exit 0 inside the box, printing each tool's exit status and the boot-log admissions/drops that arrived during it; the module doc now also states the no-apt facts. The daemon-side half of the same proof, that the gate admits exactly that host list, passed here (cargo nextest run -p minimald toolchain_hosts_admitted_by_gate: 1 passed). The vm-lane line remains deferred on this box (no KVM); the harness was last exercised against a real boot in round 4, where the run surfaced Docker Hub's CloudFront blob CDN, which the allowlist now names. Notes from the agent: Deferred (lane: vm): cargo nextest run -p minvmd hostname_allowlist_toolchain_completes — written, not run here; this box has no KVM. The module doc now answers thread PRRT_kwDOPdfcxM6nlsFW item (2): no package would carry apt — gominimal/pkgs@f4de33d0 is not a Debian userland and no upstream package carries apt, so the pre-#1707 apt leg could never exit 0; restoring it reintroduces the ENOENT→127 failure the reviewer diagnosed. NET068_TOOLCHAIN_EGRESS_HOSTS carries no Debian hosts, per the reviewer's 'fine either way'. commitlint check: the sole failing commit is 3c06ac2 (verified from the check-run annotations), whose five body lines exceed 100 chars; fixing it requires rewording an existing commit — a history rewrite this round must never do, new commits only — so the check stays red on that commit until a maintainer rewords it on rebase or accepts it (commitlint is not one of the branch-protection aggregators). The stale 'apt is asserted' claim lives in the existing PR body (lines 14/21/27), which only the runner can regene…[truncated] |
Main's box registration (#1789) added a `box_addresses` field to `SessionConfig`, and the NET-068 harness's `CreateSession` initializer went without it. The macOS lane is the only one that compiles the harness (the `minvmd_libkrun` cfg the file is gated on is unset on stock Linux CI, where the file compiles to nothing), so the Linux lanes stayed green while the macOS `unit` job failed at clippy with E0063, reding `ci-macos-success` with it. Send `None`: no registration happens on this path, the box attaches as an unregistered one always has, drawing its own switch lease — the same value every sibling minvmd harness sends.
|
Round 6 of 10 on a failed check suite, pushed as
Verify Outside the task's
Converted to a draft: something above needs a person before review. Review threads the coder judged:
The NET-068 proof is whole again at this branch: the VM harness that boots a box under a hostname-only allowlist and drives git clone, npm install, pip install and a container pull to exit 0 compiles on the one lane that builds it (macOS, cfg minvmd_libkrun) after sending the SessionConfig Notes from the agent: The three failed checks. (1) unit + ci-macos-success: main's box registration (#1789, merged in b2b585a) added SessionConfig.box_addresses; the NET-068 harness's CreateSession initializer went without it and E0063 failed the macOS lane's clippy step - the only lane that compiles the harness, because the file is #![cfg(minvmd_libkrun)] and stock Linux CI builds the stub, so the Linux lanes never saw it. Fixed with box_addresses: None (no registration on this path; the box attaches as an unregistered one always has, the same value every sibling harness sends), committed as 500085c. This box is Linux without libkrun, so macOS stood in via RUSTFLAGS=--cfg minvmd_libkrun on the unit job's exact commands (clippy, nextest 744/744, doctests, both builds - all green); a real macOS confirmation stays with the runner. (2) commitlint: it reds on commits already on the branch - 3c06ac2 (five body lines of 124-197 chars) and c6ec0ec (one 338-char body line) exceed config-conventional's body-max-line-length 100; the merge commits are skipped by commitlint's default ignores. A revise round may not rewrite commits already on the branch, so this check cannot go green from here: it needs the runtime or a person to rebase/rewrite those bodies. This round's commit keeps every body line at 72 columns. (3) Deferred: cargo nextest run -p minvmd hostname_allowlist_toolchain_completes (lane: vm) - wr…[truncated] |
…the NET-068 run The compose gate now approves exactly NPM_CONFIG_CACHE, the one variable the node package wires through env_state_wiring, and fails the test naming any other var, patch, or hook the daemon routes back. A missing precondition (MINVMD_E2E != 1 or no MINVMD_GVPROXY_BIN) prints one "SKIPPED: hostname_allowlist_toolchain_completes: ..." stderr line and returns; under MINVMD_VM_LANE it panics instead, so a VM lane cannot go green without the host switch.
The gate test pairs each host with a representative address, so it cannot read its names off the shared constant. It now asserts its names equal the constant, so a host added there fails here until the test exercises it.
|
Round 8 of 10 on a failed check suite: the coder changed nothing. Revise rou 10BC8 nd 8 finding: the 'commitlint' check on PR #1771 is the only red check at branch head 85304e9 (all 31 other runs green, verified via the GitHub API for this head and the previous head 732a12b; both heads fail commitlint identically, so the head's own merge message is not the cause). The lane lints the PR's commit list, and the offenders are two immutable commits from earlier rounds: 3c06ac2 has 5 body lines over 100 characters (150/197/160/124/169) and c6ec0ec has 1 (338) — commitlint's body-max-line-length [2, always, 100]. Approximating config-conventional over all 18 PR commits (merge commits excluded by commitlint's built-in ignore patterns) marks exactly these two; every other commit, headers included, passes. I cannot fix this without rewriting branch history, which the revise rules forbid ('never rewrite the commits already on the branch; new commits only'), and new commits cannot affect it: the lane grades the PR's past messages, so no future commit in…[truncated] |
The offset was taken from a lossy-decoded copy, where each invalid byte becomes a three-byte replacement character, so the next read could skip new bytes or slice inside a character. Slice the raw bytes, then decode.
…he-toolchain-under-a-hostname-only-all
|
Round 9 of 10 on a failed check suite: the coder changed nothing. every commit this round put on the branch above |
The detach timeout bounds only the poll for Running; the VMM parent's READY wait reads MINVMD_READY_TIMEOUT_SECS (60 s default). Set it in the harness so a bare nextest run tolerates a 40-70 s cold boot.
…he-toolchain-under-a-hostname-only-all
|
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 |
|
Round 10 of 10 on a failed check suite: the coder changed nothing. every commit this round put on the branch above |
There was a problem hiding this comment.
Approved on 0d0b678: the MINVMD_VM_OWN_IP and READY-timeout pins fix the T32 VM lane, minimal-review APPROVE on this head, no open threads, and every lane is green except commitlint. That one fails on old commit subjects, which the squash merge replaces.
T32 — Complete the toolchain under a hostname-only allowlist
slice: S7
kind: task
covers: NET-068
size: M
est: 60 code, 360 tests
executor: agent
difficulty: needs care
model: standard
diagnostics: the test prints the daemon log's admissions after each tool, so a stalled fetch names the host that was not admitted
observability: the test records each tool's exit status and the drops during it
layers:
- vmhost:
- crates/minvmd/tests/egress_allowlist_integration.rs (new): boots a VM box with the toolchain's packages and a hostname-only allowlist, then runs git clone, npm install, pip install and a container pull (the four tools NET-068 names since #1707; the composed base ships no apt)
test (deferred: vm): cargo nextest run -p minvmd hostname_allowlist_toolchain_completes
- daemon:
- crates/minimald/src/net/dns_gate.rs: a fixture allowlist naming the toolchain's hosts
test: cargo nextest run -p minimald toolchain_hosts_admitted_by_gate
produces: none
consumes: none
proof: With an allowlist naming only hostnames, git clone, npm install, pip install and a container pull all complete inside a VM box.
depends on:
- blocked by #1638
Proof
With an allowlist naming only hostnames, git clone, npm install, pip install and a container pull all complete inside a VM box. The harness composes the box with the toolchain's packages, boots it through the supervisor path (
minvmd run --detach, the only path that stands up the host switch), gives it an egress policy whose only entries are the ten names insessions::NET068_TOOLCHAIN_EGRESS_HOSTS(GitHub, the npm registry, PyPI and its file host, Docker Hub's registry and auth hosts, and both CDNs Docker Hub's blob redirect lands on), runs the four tools with a 60 s cap each, and asserts every one exits 0. The daemon's DNS gate admitted exactly the names the tools resolved, every connection the tools opened was to an admitted address, and the only refusal in the run was the box's AAAA attempt (NET-136). The composed base ships noapt; the four tools are the toolchain NET-068 names, as amended by #1707.Design review: ARCH-EXPERT CONFIRM-with-conditions (the CDN union is the required shape; the harness's loadout approval names its one item; the skip-versus-fail guard and #1809 carry the CI proof).
Run on macOS (libkrun) at 96f62c9,
MINIMALD_BIN=<prebuilt guest minimald> just test-vm:Per-tool exit statuses and the admissions, from the same test re-run with
--no-capturefrom the built archive:What the run found before it passed. The harness as first written booted with
minvmd boot --foreground, which never spawns the host switch, so the guest had no egress and every tool failed at spawn; it now runs through the supervisor path. Docker Hub's blob fetch redirects toproduction.cloudfront.docker.comtoday, not theproduction.cloudflare.docker.comthe first list named; the list carries both. The box's loadout configure returns pending for one package variable; the harness answers it the way the CLI's no-prompt hook does and refuses to continue if a patch would need an upload.Lane coverage. The two VM lanes do not export the gvproxy binary to the harness step, so there the harness's precondition skips it as passed; the evidence for this requirement is the local run above until #1809 lands. Its fix, #1815, provisions the pinned switch inside the VM test harnesses under
MINVMD_E2E=1(the marker both VM lanes already set) and fails rather than skips there; this harness adopts that helper in a follow-up once #1815 merges. Until then the harness prints aSKIPPED:line and never reports a pass with the switch present but unused; its ownMINVMD_VM_LANEmarker is set by no lane today.just test-vmon both hosts now fetches the switch and exportsMINVMD_GVPROXY_BIN, so a local run is the real thing.Out of layer
crates/sessions/src/lib.rsholds the host list the harness and the daemon fixture share (one constant,#[doc(hidden)]), so both layers read one list;.config/nextest.tomlgives this one test a longer slow-timeout leash in the vm profile (compose plus boot plus four network downloads); thejustfile'stest-vmrecipes fetch the switch and export it;crates/minvmd/Cargo.tomlandCargo.lockaddcaminoas a dev-dependency because the policy-hook trait names its path type. No production code outside the plan's layers changed.Tests
cargo nextest run -p minvmd hostname_allowlist_toolchain_completescargo nextest run -p minimald toolchain_hosts_admitted_by_gateVerify
Local on macOS at 96f62c9:
just fixclean,just test(minvmd and sessions) 715 passed,just test-vm17 passed with the log at hand. Theminimaldline runs in the Linux lanes. The commitlint check is red on an earlier coder commit's body line length; the squash merge replaces it.Base
The base branch moved while the coder worked; it was merged in before the tests ran.
Closes #1648