Repository navigation
T124 — A box's package build fetches its declared web sources under the box's egress verdict - #2795
Conversation
A web-source fetch for a build placed for a box opens each connection from a socket created in that box's classifier leaf, through a loopback relay, so the box's egress verdict decides the fetch (NET-155). The cache lookup and the content-hash check stay as they are. A failed fetch is written to the build's stderr with its URL.
A box build's web-source fetch that the box's rules refuse fails with the error that names the source host and the box (NET-155). The node-plane fetch record no longer lists a box build's web sources, which the build now fetches in the box's own classifier leaf.
NET-155 now says a package build run for a box fetches its declared web sources in the box's classifier leaf. NET-080 lists the node plane's own lookups as the registry, the cache and the control plane.
|
Important Review skippedBot user detected. To trigger a single review, invoke the
 |
|
Reviewed: 1 finding, none blocking at Address the comments on the diff. Clobber: none found. |
Spec lint
Advisory. None of this blocks the merge; the review decides. Advisory, and never a merge gate. Linter: |
…ckage-build-fetches-its-declared-web-s
There was a problem hiding this comment.
arch-review: CONDITIONS · 4 conditions · the full ruling
The connection of a placed web-source fetch now leaves from the box's leaf, and that conforms to §4.1 line 54. The DNS lookup does not: Relay::connect resolves the source host in the daemon. §5.3 line 145 limits the daemon's own lookups to registry, cache and control plane. C1 moves the lookup into the leaf. record_fetch now drops web sources that a leaf-less install still fetches as node-plane traffic. C2 restores the record for those installs. C3 files the own-address gap, and C4 adds the live root-lane proof that the PR body says is not run.
Ruled at c7f2edc against gominimal/arch@8640f85e193d by claude-opus-5-5.
A placed download for a leaf in the deny subtree now refuses each connection before the relay resolves the host. The node's resolver is not under the box's rules. The line a failed web-source fetch writes to the build's stderr now strips credentials and queries from every URL that its error quotes.
A failed web-source fetch names a refused destination only when its error says the download could not connect. A bad hash or a failed extract no longer reads as an egress refusal.
…ckage-build-fetches-its-declared-web-s
There was a problem hiding this comment.
arch-review: CONDITIONS · 2 conditions · the full ruling
The deny-leaf lookup fix and the narrower refusal mapping close part of the earlier ruling. Two items stay open. For an allow leaf, placed::Relay::connect still resolves the box's source host in the daemon's cgroup. Section 5.3 line 145 keeps that path for the daemon's own lookups only. C1 moves the lookup into the leaf. The live test that the spec names never runs a web-source fetch, and section 4.1 line 54 requires a measured placement. C2 adds that root-lane test.
Ruled at a033216 against gominimal/arch@8640f85e193d by claude-opus-5-5.
|
Round 1 of 10 on the runtime's review findings, pushed as
Verify Review threads the coder judged:
A deny-all box whose package declares a web source sees its build fail with "the build of fetcher could not reach downloads.example.org: the egress rules of box sealed do not allow it". A box whose leaf allows that host builds the package. The node now resolves no host name for a deny-all box's fetch. Notes from the agent: Revise round 1. The download_web_with_verification_and_caching signature gained an admits_nothing bool, so common's lib.rs needs no new export. Thread 1 is fixed only for deny leaves. A box whose rules allow some destinations still has its source host resolved by the node's resolver. To resolve inside the leaf, the fork child must run a DNS query, and that is not async-signal-safe. That case needs its own task. clippy -p common -p op -p minimald --all-targets is clean. I did not run just clippy-strict. |
There was a problem hiding this comment.
Review of head a033216 against T124's plan block (#2730) and NET-155/NET-080. Approve.
Checked: both SourceLoad sites (build_deps_mapped, materialize_prebuilt) receive the box's classifier_leaf on the in-box build path (env.rs → PatchedBuild → SpecBuild) and the side-op path (orchestrator/local_backend.rs); RemoteStorage::download_web places each connection's socket in the leaf (forked child writes cgroup.procs, creates the socket, passes it back over SCM_RIGHTS, async-signal-safe calls only); a deny leaf is refused before resolution; cache lookup and content-hash check unchanged; registry and index fetches stay node-plane; the refusal text matches T104's exactly, names the source host, and appears only on a connect refusal, not a hash mismatch or a 404; credentials and queries are stripped from the stderr line; both named tests exist and assert behaviour; the spec edits match the layer.
Non-blocking:
crates/common/src/remote_storage.rs:611:a_placed_download_connects_from_the_placementhas no#[cfg(target_os = "linux")], unlike its neighbour; on macOSsocket_in_cgroupreturnsUnsupportedand the test panics undercargo test -p common. No lane runs that today.crates/minimald/src/sandbox_progress.rs:213:record_fetchnow drops every scheme-bearingFetchSource, including fetches made withclassifier_leaf: None(own-address boxes; host-address boxes withhost_ip_enforcement = none). Those fetches leave from the daemon's cgroup and are now neither placed nor recorded. This matches NET-155's scope; the record gap needs a follow-up.
|
/revise Architecture review on a033216: CONFIRM with four conditions. The task's layers now include
Also (my review, non-blocking): |
…ckage-build-fetches-its-declared-web-s
The relay of a placed web download now asks the first resolv.conf name server from a UDP socket that a child creates in the cgroup, so the box's egress rules decide the query as well as the connection.
Only an http or https source of an install placed in the box's leaf is the box's traffic. A gs:// source, and a web source of a box with no leaf, still leave from the daemon, so NET-080 records them.
Add the root-lane proof over a loaded table: a deny leaf refuses the placed download, and an allow leaf fetches the source.
…ckage-build-fetches-its-declared-web-s
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare closes the items from the last two rounds. The name query now leaves from the box's leaf, the NET-080 record is exact, and the root lane carries a live proof. One gap stays, and this PR did not cause it. An own-address box has no leaf, so the daemon fetches its web sources with the node-plane address. Section 4.1 line 54 says box traffic and node-plane traffic never share an identity. C1 files an issue for that gap and names it in the NET-155 comment. Then it is a filed residual.
Ruled at d4cbd3e against gominimal/arch@8640f85e193d by claude-opus-5-5.
|
The runtime will not revise this pull request again on its own: the diff is +2108/-53 (2161 lines), past 3x the plan's estimate of 420 lines (est: 200 code, 220 tests, as #2730 reads now), at round 1 (the last trigger was a /revise note). A person decides now. Remove |
|
/revise Last round on 44d23de. The cap is lifted for this round. Change only these three, all in the task's layers. Do not edit the PR body.
|
…ckage-build-fetches-its-declared-web-s
The placed download's relay now records whether its client asked it for a connection. download_web returns that flag beside the path, so the log line says either that the fetch ran in the box's classifier leaf or that the download cache served the source. The relay's accept loop pauses 50 ms after an accept error, so a lasting error such as EMFILE does not spin. No test forces an accept error: a TcpListener gives no cheap way to make accept fail on demand.
A source that redirects to a CDN host fails on the CDN host. The refusal now reads the host from the download's could not connect to <host>:<port> text, and falls back to the source URL's host only when that text names no host.
There was a problem hiding this comment.
Babysitter review of 528dc85 against T124. All three items of the last /revise are closed:
528dc85d: the refusal takes the host from the relay'scould not connect to <host>:<port>text (connect_host), and falls back to the source host only when that text is missing. Tests cover the redirect and the fallback.df81191c:servepauses 50 ms (ACCEPT_RETRY) after an accept error. The log line now comes after the download and says which happened, a fetch in the leaf or a cache hit with no connection, from the relay'saskedflag.
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The compare conforms. The fetch log line now says exactly what happened, and the refusal names the host that the box's rules refused. One item is still open. Section 4.1 line 54 says the cgroup placement holds only when it is measured. The live proof web_source_fetch_in_a_box_leaf_follows_its_verdict_over_a_loaded_table did not run on this head. Run it on the root lane as C1 says, and record the result in the PR.
Ruled at 528dc85 against gominimal/arch@8640f85e193d by claude-opus-5-5.
|
Round 1 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
A deny-all box whose package declares a web source sees its build fail, and the error names the box and the host the download could not reach. For a redirect, that is the redirect target's host. A box allowed that host builds the package. Notes from the agent: Revise round: addressed the three directive items. (1) refused_destination reads the host from the 'could not connect to :' text and falls back to the source URL host. New test box_build_web_source_under_box_verdict_names_the_host_it_could_not_reach covers redirect, IPv6 and fallback. (2) The relay accept loop sleeps 50 ms after an accept error. No test was added, because accept cannot be made to fail cheaply. The commit message gives this reason. (3) The relay records whether its client asked for a connection. SourceFetcher::download_web now returns (PathBuf, bool), and sources.rs logs after the fetch: fetched in the leaf, served by the cache, or failed. A tuple is used in place of a named type, because a named type needs an export in crates/op/src/lib.rs and crates/common/src/lib.rs, which are outside the layers. RemoteStorage gets a new download_web_placed method that returns the flag. Also ran cargo nextest run -p common placed (11 passed) and -p minimald box_package_build_refusal (5 passed). Deferred (lane: root): cargo nextest run -p minimald --run-ignored only web_source_fetch_in_a_box_leaf_follows_its_verdict_over_a_loaded_table. It now also asserts that the allow-leaf fetch reports a connection. It was not run. |
|
The runtime will not revise this pull request again on its own: the diff is +2244/-64 (2308 lines), past 3x the plan's estimate of 420 lines (est: 200 code, 220 tests, as #2730 reads now), at round 1 (the last trigger was a /revise note). A person decides now. Remove |
|
/revise One fix on 528dc85. The cap is lifted for this round. Change only this. Do not edit the PR body. Credentials in the fetch failure log ( Leave the existing |
…ckage-build-fetches-its-declared-web-s
The info line for a box's failed web-source fetch logged the whole error, and a reqwest error quotes the request URL with its user, password and signed query. Run it through strip_urls, as the build's stderr report does. The debug line after a fetch logs the stripped URL too.
There was a problem hiding this comment.
arch-review: PASS_WITH_CONDITIONS · 1 condition · the full ruling
The change conforms to §4.1 line 54 and §5.3 line 145. A box's web-source fetch and its name query now leave from the box's classifier leaf, and the node-plane record lists only the daemon's own fetches. The new commit removes URL credentials from the fetch log, and a test pins that. The live proof against a real cgroup and a loaded table is still open. C1 requires the root-integration job to run it green on this head before merge. Residual #2802 covers own-address boxes and gs:// sources.
Ruled at 1620413 against gominimal/arch@8640f85e193d by claude-opus-5-5.
There was a problem hiding this comment.
Babysitter review of 1620413. 16204138 runs the failure log's error through strip_urls, and logs the stripped URL in the later debug! line. A test with a fetcher whose error quotes https://user:secret@host/x?sig=abc checks that neither secret reaches the log. This closes both HIGH threads.
|
Round 1 of 10 on a /revise note, pushed as
Verify Review threads the coder judged:
A deny-all box whose package declares a web source sees its build fail naming the source host and the box, and a box allowed that host builds it. In this round, the daemon log line for that failed fetch also no longer carries the user, password or signed query of any URL the fetch error quotes. Notes from the agent: Revise round 1: only the change the note asked for. strip_urls and stripped_url in crates/op/src/specs.rs are now pub(crate). The failed-fetch info! line in SourceLoad::run passes its error through strip_urls. The debug! line after a successful fetch printed the full Url, credentials and query included, so it now logs stripped_url(url) as the note asked. New test web_source_fetch_failure_log_strips_url_credentials captures events with a small in-test tracing Subscriber, because op has no tracing-subscriber dev-dependency and crates/op/Cargo.toml is not in the layers. I checked that it fails with the old line. I did not re-run the minimald test line this round, because no minimald file changed. Deferred (lane: root): cargo nextest run -p minimald --run-ignored only web_source_fetch_in_a_box_leaf_follows_its_verdict_over_a_loaded_table. |
|
The runtime will not revise this pull request again on its own: the diff is +2366/-65 (2431 lines), past 3x the plan's estimate of 420 lines (est: 200 code, 220 tests, as #2730 reads now), at round 1 (the last trigger was a /revise note). A person decides now. Remove |
A web-source fetch for a build placed for a box opens each connection from a socket created in that box's classifier leaf, through a loopback relay, and asks its name query from the same leaf. The box's egress verdict then decides the fetch (NET-155). On a VM host the frames carry the box mark, so the host gate decides them by the box's rules and not by the node-plane baseline. The cache lookup and the content-hash check stay as they are. A fetch that the box's rules refuse fails the build with the source host. The node-plane fetch record no longer lists a web source fetched in a box's leaf. A gs:// source, and a web source of a box with no leaf, still leave from the daemon and are recorded. Backports [#2795](#2795) to the 0.7 line. The change is T124 from main with these names adapted, because the line predates two moves on main: - sandbox::config is sandbox2::config, in op and in minimald's tests. The line predates the crate rename in [#2106](#2106). - box_ops::{BuildPlacement, BuildTap} is session_sop::{BuildPlacement, BuildTap}, in the new test in minimald's session/tests.rs. main's box_actor/tests.rs is that file on the line. (cherry picked from commit c7f6ddd)
T124 — A box's package build fetches its declared web sources under the box's egress verdict
slice: S16
kind: task
covers: NET-155
size: M
est: 200 code, 220 tests
executor: agent
difficulty: needs care
model: strongest
diagnostics: when a box's rules refuse a web source its package declares, the build fails and its error names the source host and the box, in the words T104 pins: "the build of could not reach : the egress rules of box do not allow it"
observability: the daemon logs one line per web-source fetch it makes for a box's build, naming the box, the source host and the classifier leaf the fetch ran in; the NET-080 node-plane fetch record no longer lists a box's web-source fetch
layers:
- op:
- crates/op/src/sources.rs: a web-source fetch for a build placed for a box runs in 8000 that box's classifier leaf, so the box's egress verdict decides it; the cache lookup and the content-hash check stay as they are
- crates/common/src/remote_storage.rs: the web download takes the placement it runs under and opens its connection in it
- crates/op/src/specs.rs: both
SourceLoadsites (SpecBuild::build_deps_mappedandmaterialize_prebuilt) pass the build'sclassifier_leafto the source load; its test fetchers follow the new signature- crates/op/src/patched.rs: its test
UnusedFetcherfollows the newdownload_websignaturetest: cargo nextest run -p op web_source_fetch_runs_in_the_given_classifier_leaf
- daemon:
- crates/minimald/src/sandbox_progress.rs:
record_fetchstops recording a box build'sFetchSourcewith a scheme as node-plane traffic; registry, cache and index fetches stay node-plane (§5.3 line 145)- crates/minimald/src/box_actor/tests.rs: a deny-all box whose package declares a web source fails its build naming the host and the box, and a box allowed that host builds it
- crates/minimald/src/env.rs: a box build's web-source fetch refused by the box's rules maps to the refusal T104 pins, "the build of could not reach : the egress rules of box do not allow it", with the source host as the destination
- crates/minimald/tests/classifier_root_integration.rs: its
NoSourcesfetcher follows the newdownload_websignaturetest: cargo nextest run -p minimald box_build_web_source_under_box_verdict
- spec:
- docs/specs/18-spec-box-networking/18-spec-box-networking.md: NET-155 says a package build run for a box fetches its declared web sources under that box's egress verdict, and NET-080 lists the node plane's own lookups as registry, cache and control plane only
produces: none
consumes: none
proof: A deny-all box whose package declares a web source sees its build fail naming the source host and the box, and a box allowed that host builds it.
depends on:
- blocked by #2691
Generated by Claude Code
Proof
A deny-all box whose package declares a web source now has its build fail with "the build of could not reach: the egress rules of box do not allow it". A box whose leaf allows that host builds the package. box_build_web_source_under_box_verdict shows both: it runs a real op::SpecBuild of a prebuilt package with a web source in a deny leaf and in an allow leaf, through the build side operation and through the in-box patched build. The fetcher in that test stands in for the kernel verdict.
Notes from the agent: Mechanism: a socket belongs to the cgroup of the process that creates it, and one thread cannot join a leaf alone. So a placed download in common::RemoteStorage sends reqwest through a loopback relay that requires a per-download token. reqwest uses the relay as its proxy, so https stays end to end through CONNECT. For each connection, a forked child writes itself into the leaf's cgroup.procs, creates the socket and passes it back over SCM_RIGHTS. The child uses only write, socket, sendmsg and _exit. The relay then connects that socket. common tests drive this end to end against a local HTTP server, with a plain file in place of cgroup.procs. No test here runs it against a real cgroup and nft table, because this box has no cgroup2 mount and runs without root. The root lane (just test-root-integration) still owes that live proof. DNS for the source host still resolves in the daemon, not in the leaf. Only the TCP connection is under the box's verdict. The daemon finds the refused host because op's spec build now writes 'could not fetch the source : ' to the build's stderr when a web-source load fails, and env.rs refused_destination reads that line. I used this route so box_ops.rs (BuildTap) did not change, because it is outside the layers. The observability log line is in op/src/sources.rs: one info line per web-source load placed in a leaf, with box_id (the leaf's name), host and …[truncated]
Residuals
#[tracing::instrument]spans ondownload_web*record the source URL, with any credentials.gs://source, from its own network namespace with the node's address. No box leaf decides those fetches.Tests
cargo nextest run -p op web_source_fetch_runs_in_the_given_classifier_leafcargo nextest run -p minimald box_build_web_source_under_box_verdictVerify
min task run agent-verifypassed ([tasks.verify]).Closes #2730