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) <noreply@anthropic.com>
2026-09-23 15:25:51 -07:00
parent 75c2f1adbf
commit c3aaecdae2
19 changed files with 37 additions and 5 deletions
@@ -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<OsString> {
    [
+        // 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",
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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<String> {
    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(),
@@ -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",
@@ -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 |
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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",
@@ -150,7 +150,7 @@ The container is named `boxmaker-<session>-<call>-<n>`, where `n` counts calls w
 (never a shell string):

 ```
-run --rm -i --name=<name> --label=boxmaker=tool --network=none --read-only --cap-drop=all
+run --rm -i --name=<name> --label=boxmaker=tool --network=none --pull=never --read-only --cap-drop=all
    --security-opt=no-new-privileges --userns=keep-id --pids-limit=<pids> --memory=<memory>
    --tmpfs=/tmp:rw,size=64m,mode=1777
    [--volume=<path>:<path>: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 `<home>/run/egress/<name>/` (mode 0700) and starts the
 proxy:

 ```
-run -d --rm --name=<name>-egress --label=boxmaker=egress --network=<egress_network> --read-only
+run -d --rm --name=<name>-egress --label=boxmaker=egress --network=<egress_network> --pull=never --read-only
    --cap-drop=all --security-opt=no-new-privileges --userns=keep-id --pids-limit=64
    --memory=128m --volume=<egress dir>:/run/egress:rw
    <image> /bin/toolkit egress-proxy --socket /run/egress/egress.sock --allow <hosts joined by ,>
@@ -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 <url>