From c3aaecdae287460340ea52c4580d16ef7df19b6b Mon Sep 17 00:00:00 2001 From: "K. Isom" Date: Wed, 23 Sep 2026 15:25:51 -0700 Subject: [PATCH] M3b review fixes by the design model: curl --globoff, podman --pull=never Review findings 1 and 2, both plan defects. curl gains --globoff and a leading --disable; both podman runs gain --pull=never. The given fetch.rs and the six golden files change with them. Checked on straylight with a rebuilt image: a glob URL is one request, a missing image fails at once. Co-Authored-By: Claude Opus 5.5 (1M context) --- crates/brokerd/src/podman.rs | 3 +++ crates/brokerd/tests/fixtures/podman/egress.args | 1 + crates/brokerd/tests/fixtures/podman/http_fetch.args | 1 + crates/brokerd/tests/fixtures/podman/read_file.args | 1 + crates/brokerd/tests/fixtures/podman/shell.args | 1 + crates/brokerd/tests/fixtures/podman/shell_no_paths.args | 1 + crates/brokerd/tests/fixtures/podman/write_file.args | 1 + crates/toolkit/src/fetch.rs | 4 ++++ crates/toolkit/tests/fetch.rs | 2 ++ docs/implementer-log.md | 4 ++-- docs/plans/M3b/README.md | 6 ++++++ .../crates/brokerd/tests/fixtures/podman/egress.args | 1 + .../crates/brokerd/tests/fixtures/podman/http_fetch.args | 1 + .../crates/brokerd/tests/fixtures/podman/read_file.args | 1 + .../crates/brokerd/tests/fixtures/podman/shell.args | 1 + .../brokerd/tests/fixtures/podman/shell_no_paths.args | 1 + .../crates/brokerd/tests/fixtures/podman/write_file.args | 1 + docs/plans/M3b/files/crates/toolkit/tests/fetch.rs | 2 ++ docs/specs/2026-09-22-m3b-runner.md | 9 ++++++--- 19 files changed, 37 insertions(+), 5 deletions(-) diff --git a/crates/brokerd/src/podman.rs b/crates/brokerd/src/podman.rs index 430cbd5..d6c1056 100644 --- a/crates/brokerd/src/podman.rs +++ b/crates/brokerd/src/podman.rs @@ -23,6 +23,9 @@ pub fn container_name(session: &SessionId, call: CallId, n: u64) -> String { /// takes the runner's, the proxy its fixed limits. fn hardening(pids: u32, memory: &str) -> Vec { [ + // A missing image is an error at once, never a pull: a pull is egress, and what runs must + // be exactly the image built for it. + "--pull=never", "--read-only", "--cap-drop=all", "--security-opt=no-new-privileges", diff --git a/crates/brokerd/tests/fixtures/podman/egress.args b/crates/brokerd/tests/fixtures/podman/egress.args index 7f70a7d..364c619 100644 --- a/crates/brokerd/tests/fixtures/podman/egress.args +++ b/crates/brokerd/tests/fixtures/podman/egress.args @@ -4,6 +4,7 @@ run --name=boxmaker-s1-1-7-egress --label=boxmaker=egress --network=pasta +--pull=never --read-only --cap-drop=all --security-opt=no-new-privileges diff --git a/crates/brokerd/tests/fixtures/podman/http_fetch.args b/crates/brokerd/tests/fixtures/podman/http_fetch.args index 7763d10..a05a247 100644 --- a/crates/brokerd/tests/fixtures/podman/http_fetch.args +++ b/crates/brokerd/tests/fixtures/podman/http_fetch.args @@ -4,6 +4,7 @@ run --name=boxmaker-s1-1-7 --label=boxmaker=tool --network=none +--pull=never --read-only --cap-drop=all --security-opt=no-new-privileges diff --git a/crates/brokerd/tests/fixtures/podman/read_file.args b/crates/brokerd/tests/fixtures/podman/read_file.args index 4fd89e9..efd7551 100644 --- a/crates/brokerd/tests/fixtures/podman/read_file.args +++ b/crates/brokerd/tests/fixtures/podman/read_file.args @@ -4,6 +4,7 @@ run --name=boxmaker-s1-1-7 --label=boxmaker=tool --network=none +--pull=never --read-only --cap-drop=all --security-opt=no-new-privileges diff --git a/crates/brokerd/tests/fixtures/podman/shell.args b/crates/brokerd/tests/fixtures/podman/shell.args index 7d85f69..683240d 100644 --- a/crates/brokerd/tests/fixtures/podman/shell.args +++ b/crates/brokerd/tests/fixtures/podman/shell.args @@ -4,6 +4,7 @@ run --name=boxmaker-s1-1-7 --label=boxmaker=tool --network=none +--pull=never --read-only --cap-drop=all --security-opt=no-new-privileges diff --git a/crates/brokerd/tests/fixtures/podman/shell_no_paths.args b/crates/brokerd/tests/fixtures/podman/shell_no_paths.args index 5d26eea..e7abd6d 100644 --- a/crates/brokerd/tests/fixtures/podman/shell_no_paths.args +++ b/crates/brokerd/tests/fixtures/podman/shell_no_paths.args @@ -4,6 +4,7 @@ run --name=boxmaker-s1-1-7 --label=boxmaker=tool --network=none +--pull=never --read-only --cap-drop=all --security-opt=no-new-privileges diff --git a/crates/brokerd/tests/fixtures/podman/write_file.args b/crates/brokerd/tests/fixtures/podman/write_file.args index 015b5d5..c25848f 100644 --- a/crates/brokerd/tests/fixtures/podman/write_file.args +++ b/crates/brokerd/tests/fixtures/podman/write_file.args @@ -4,6 +4,7 @@ run --name=boxmaker-s1-1-7 --label=boxmaker=tool --network=none +--pull=never --read-only --cap-drop=all --security-opt=no-new-privileges diff --git a/crates/toolkit/src/fetch.rs b/crates/toolkit/src/fetch.rs index 7d50772..868410c 100644 --- a/crates/toolkit/src/fetch.rs +++ b/crates/toolkit/src/fetch.rs @@ -22,8 +22,12 @@ const MAX_STDERR: usize = 64 * 1024; /// `curl`'s arguments for `url`, in order, without the program name. pub fn curl_args(url: &str) -> Vec { vec![ + // First, or it has no effect: never read a `.curlrc`. + "--disable".to_string(), "--silent".to_string(), "--show-error".to_string(), + // `[1-99999999]` in a URL is text, not millions of requests to the allowed host. + "--globoff".to_string(), "--proto".to_string(), "=https".to_string(), "--proto-redir".to_string(), diff --git a/crates/toolkit/tests/fetch.rs b/crates/toolkit/tests/fetch.rs index a95978f..73c06e7 100644 --- a/crates/toolkit/tests/fetch.rs +++ b/crates/toolkit/tests/fetch.rs @@ -39,8 +39,10 @@ fn args() -> HttpFetchArgs { #[test] fn the_argument_list_is_fixed_and_ends_with_the_url() { let expected: Vec<&str> = vec![ + "--disable", "--silent", "--show-error", + "--globoff", "--proto", "=https", "--proto-redir", diff --git a/docs/implementer-log.md b/docs/implementer-log.md index a486d3e..e646e0e 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -461,8 +461,8 @@ claim of the milestone held. | # | Severity | Owner | Finding | Fix | |---|---|---|---|---| -| 1 | medium | plan (task 06, spec 6) | `curl` expands globs in the URL: `https://example.com/[1-3]` made three requests (seen on straylight), so `[1-99999999]` would hammer an allowed host and buffer every body. The fixed argument list lacks `--globoff`. | Follow-up | -| 2 | low | plan (task 10, spec 6) | `podman run` has no `--pull=never`: with an image that is not loaded, Podman tries to pull it (seen on straylight). Here the name starts `localhost/`, so the pull fails, but a pull is unlisted egress and the call should fail at once. | Follow-up | +| 1 | medium | plan (task 06, spec 6) | `curl` expands globs in the URL (documented behaviour: `https://example.com/[1-3]` is three requests; the review first wrote "seen on straylight", but only the last response was seen, not counted), so `[1-99999999]` would hammer an allowed host and buffer every body. After the fix, the same URL is one request, counted on straylight. The fixed argument list lacks `--globoff`. | Fixed by the design model (`--globoff`, and `--disable` first) | +| 2 | low | plan (task 10, spec 6) | `podman run` has no `--pull=never`: with an image that is not loaded, Podman tries to pull it (seen on straylight). Here the name starts `localhost/`, so the pull fails, but a pull is unlisted egress and the call should fail at once. | Fixed by the design model (`--pull=never`); a missing image now fails in 46 ms, no pull | | 3 | low | implementer (11), plan | `std::thread::spawn` in `container.rs` (three) and `toolkit/src/fetch.rs` panics if a thread cannot be made; after the spawn of the container, a panic drops the `Child` without `podman kill`, so the container runs on without its limit. The reference had the same; the task did not say. | Follow-up | | 4 | low | implementer (11) | The time limit bounds the wait, not the joins after it: if another process held the pipes, `run` would block until it let go (shown with a fake `podman` without `exec`: 6 s for a 0.3 s limit). Real Podman released them at the kill (8 s limit, 8 s seen). | Follow-up: join with a grace deadline | | 5 | low | implementer (11) | Podman's standard error, which the tool can write to, goes into `brokerd`'s log unescaped, so a tool can forge log lines (a fake runbook pointer). It never reaches a `RunError`. | Follow-up: escape | diff --git a/docs/plans/M3b/README.md b/docs/plans/M3b/README.md index a5b2cdd..82b297c 100644 --- a/docs/plans/M3b/README.md +++ b/docs/plans/M3b/README.md @@ -82,6 +82,12 @@ At the end: `make gate` prints `gate: ok` with about 638 tests. pass (11 of 11, five runs, clippy clean) and removed that. Task 12 now calls `run_container`. Resume from task 11. +- 2026-09-23, after the review: two plan defects found on straylight were fixed by the design + model: `curl` gains `--globoff` (and a leading `--disable`), since `[1-3]` in a URL made three + requests; both `podman run`s gain `--pull=never`, since a missing image made Podman try to pull. + The given `fetch.rs` and the six golden files changed with them. Follow-up tasks 14 to 16 are for + the implementer. + ## Running it ```sh diff --git a/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/egress.args b/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/egress.args index 7f70a7d..364c619 100644 --- a/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/egress.args +++ b/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/egress.args @@ -4,6 +4,7 @@ run --name=boxmaker-s1-1-7-egress --label=boxmaker=egress --network=pasta +--pull=never --read-only --cap-drop=all --security-opt=no-new-privileges diff --git a/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/http_fetch.args b/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/http_fetch.args index 7763d10..a05a247 100644 --- a/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/http_fetch.args +++ b/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/http_fetch.args @@ -4,6 +4,7 @@ run --name=boxmaker-s1-1-7 --label=boxmaker=tool --network=none +--pull=never --read-only --cap-drop=all --security-opt=no-new-privileges diff --git a/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/read_file.args b/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/read_file.args index 4fd89e9..efd7551 100644 --- a/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/read_file.args +++ b/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/read_file.args @@ -4,6 +4,7 @@ run --name=boxmaker-s1-1-7 --label=boxmaker=tool --network=none +--pull=never --read-only --cap-drop=all --security-opt=no-new-privileges diff --git a/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/shell.args b/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/shell.args index 7d85f69..683240d 100644 --- a/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/shell.args +++ b/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/shell.args @@ -4,6 +4,7 @@ run --name=boxmaker-s1-1-7 --label=boxmaker=tool --network=none +--pull=never --read-only --cap-drop=all --security-opt=no-new-privileges diff --git a/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/shell_no_paths.args b/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/shell_no_paths.args index 5d26eea..e7abd6d 100644 --- a/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/shell_no_paths.args +++ b/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/shell_no_paths.args @@ -4,6 +4,7 @@ run --name=boxmaker-s1-1-7 --label=boxmaker=tool --network=none +--pull=never --read-only --cap-drop=all --security-opt=no-new-privileges diff --git a/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/write_file.args b/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/write_file.args index 015b5d5..c25848f 100644 --- a/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/write_file.args +++ b/docs/plans/M3b/files/crates/brokerd/tests/fixtures/podman/write_file.args @@ -4,6 +4,7 @@ run --name=boxmaker-s1-1-7 --label=boxmaker=tool --network=none +--pull=never --read-only --cap-drop=all --security-opt=no-new-privileges diff --git a/docs/plans/M3b/files/crates/toolkit/tests/fetch.rs b/docs/plans/M3b/files/crates/toolkit/tests/fetch.rs index a95978f..73c06e7 100644 --- a/docs/plans/M3b/files/crates/toolkit/tests/fetch.rs +++ b/docs/plans/M3b/files/crates/toolkit/tests/fetch.rs @@ -39,8 +39,10 @@ fn args() -> HttpFetchArgs { #[test] fn the_argument_list_is_fixed_and_ends_with_the_url() { let expected: Vec<&str> = vec![ + "--disable", "--silent", "--show-error", + "--globoff", "--proto", "=https", "--proto-redir", diff --git a/docs/specs/2026-09-22-m3b-runner.md b/docs/specs/2026-09-22-m3b-runner.md index ab9dd50..c5484b2 100644 --- a/docs/specs/2026-09-22-m3b-runner.md +++ b/docs/specs/2026-09-22-m3b-runner.md @@ -150,7 +150,7 @@ The container is named `boxmaker---`, where `n` counts calls w (never a shell string): ``` -run --rm -i --name= --label=boxmaker=tool --network=none --read-only --cap-drop=all +run --rm -i --name= --label=boxmaker=tool --network=none --pull=never --read-only --cap-drop=all --security-opt=no-new-privileges --userns=keep-id --pids-limit= --memory= --tmpfs=/tmp:rw,size=64m,mode=1777 [--volume=::ro | :rw for each mount, in RunSpec order] @@ -175,13 +175,16 @@ polling. Then: Every text above is fixed: tool output never reaches the model through a `RunError`. +`--pull=never` (added after the M3b review): an image that is not loaded fails the call at once +(exit 125) instead of Podman trying to pull it, which would be unlisted egress. + ### `http_fetch` Before the tool container, `brokerd` makes `/run/egress//` (mode 0700) and starts the proxy: ``` -run -d --rm --name=-egress --label=boxmaker=egress --network= --read-only +run -d --rm --name=-egress --label=boxmaker=egress --network= --pull=never --read-only --cap-drop=all --security-opt=no-new-privileges --userns=keep-id --pids-limit=64 --memory=128m --volume=:/run/egress:rw /bin/toolkit egress-proxy --socket /run/egress/egress.sock --allow @@ -196,7 +199,7 @@ out is the socket. `curl`'s arguments inside the tool container are fixed: ``` -/bin/curl --silent --show-error --proto =https --proto-redir =https --location --max-redirs 5 +/bin/curl --disable --silent --show-error --globoff --proto =https --proto-redir =https --location --max-redirs 5 --max-time 50 --max-filesize 8388608 --cacert /etc/ssl/certs/ca-certificates.crt --proxy socks5h://localhost/run/egress/egress.sock --write-out "\n[http %{response_code}]" --url