From 75c2f1adbff99661f01b5a2bb421376742244577 Mon Sep 17 00:00:00 2001 From: "K. Isom" Date: Wed, 23 Sep 2026 11:48:31 -0700 Subject: [PATCH] Review M3b: accept with follow-ups; add the Nix expression for the image On straylight, with real containers from deploy/tools-image.nix, every claim held: no network without a grant, the limits, the file tools, http_fetch's host checks including a redirect and a tailnet name, and no leftovers. Two plan defects found there (curl globbing, podman pulling a missing image), five lower findings. Co-Authored-By: Claude Opus 5.5 (1M context) --- deploy/tools-image.nix | 46 +++++++++++++++++++++++++++++++++++++ docs/implementer-log.md | 50 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+) create mode 100644 deploy/tools-image.nix diff --git a/deploy/tools-image.nix b/deploy/tools-image.nix new file mode 100644 index 0000000..bb60217 --- /dev/null +++ b/deploy/tools-image.nix @@ -0,0 +1,46 @@ +# The image every tool container runs from (M3b spec, section 7; the brief's authority contract 4). +# Built from this repository's source by Nix on the host that runs brokerd, loaded with +# `podman load`, and named by digest in brokerd.toml's `[runner] image`. Nothing is pulled at call +# time. +# +# nix-build deploy/tools-image.nix # from the repository root; result is an image tarball +# podman load < result +# podman image inspect --format '{{.Digest}}' localhost/boxmaker-tools:latest +# +# Contents: /bin/toolkit (static, musl), busybox with every applet (so /bin/sh is busybox's), +# /bin/curl (static), the CA bundle. No package manager, no compiler, nothing else. +{ pkgs ? import { } }: +let + static = pkgs.pkgsStatic; + src = pkgs.lib.cleanSourceWith { + src = ../.; + # Only what the build reads: no target/, no .state/, no docs. + filter = path: type: + let rel = pkgs.lib.removePrefix (toString ../. + "/") (toString path); + in rel == "Cargo.toml" || rel == "Cargo.lock" + || rel == "crates" || pkgs.lib.hasPrefix "crates/" rel; + }; + toolkit = static.rustPlatform.buildRustPackage { + pname = "boxmaker-toolkit"; + version = "0.1.0"; + inherit src; + cargoLock.lockFile = ../Cargo.lock; + cargoBuildFlags = [ "-p" "toolkit" ]; + doCheck = false; # the tests run in `make gate`, on the development machine + }; + root = pkgs.runCommand "boxmaker-tools-root" { } '' + mkdir -p $out/bin $out/etc/ssl/certs $out/tmp $out/run/egress + # busybox first: `cp -a` keeps the store's read-only modes, so make the copies writable after. + cp -a ${static.busybox}/bin/. $out/bin/ + chmod -R u+w $out/bin + cp ${toolkit}/bin/toolkit $out/bin/toolkit + cp ${static.curl.bin}/bin/curl $out/bin/curl + cp ${pkgs.cacert}/etc/ssl/certs/ca-bundle.crt $out/etc/ssl/certs/ca-certificates.crt + ''; +in +pkgs.dockerTools.buildLayeredImage { + name = "boxmaker-tools"; + tag = "latest"; + contents = [ root ]; + config.WorkingDir = "/tmp"; +} diff --git a/docs/implementer-log.md b/docs/implementer-log.md index 5c6dd35..a486d3e 100644 --- a/docs/implementer-log.md +++ b/docs/implementer-log.md @@ -431,3 +431,53 @@ task 01's stopped row carried the notes of M2b task 11. Each is moved back or re bracketed mark, and pipes inside code are escaped so every row has its eight cells. | M3b/03-brokerd-grant-mount-rule | 2026-09-22 | done | 1 | pass | none | Copied `tests/grants_mount.rs` from the plan's `files/`. Added a third arm to the path loop in `check_grant` (grants.rs:263), an `else if path.contains([':', ','])` checked only when the first two arms did not apply, reporting `"{:?} cannot be mounted: it contains ':' or ','"`. The `else if` chain means a path already reported as invalid is not reported twice. `cargo fmt --all` kept the `push` multi-line (the single-line form in the task exceeds 100 columns); the wording matches the task verbatim. Both suites pass (17 grants, 2 mount); `make gate` prints `gate: ok` on the first run. | ? | +### M3b, tasks 01 to 13 — reviewed 2026-09-23 by the design model (Claude) + +Accepted, with follow-ups. All work by Ornith-1.5-35B-A3B through `tools/run-plan.sh`. The code +does what the spec says, and on straylight, with real containers from the Nix-built image, every +claim of the milestone held. + +| Check | Result | +|---|---| +| 13 task commits, each with the trailer; 6 plan commits by the design model during the run | pass | +| All 28 given test and fixture files identical to the plan | pass (`container.rs` differs from its skeleton, as intended) | +| `make gate` on Talos | `gate: ok`, 638 tests, the same count as the reference | +| `toolkit` and `brokerd` suites ten times in a row | no failure | +| Banned constructs in new library code | `thread::spawn` four times, `as u64` on two constants (findings 3 and 7) | +| Independent review by a separate agent, given only the code, the spec and the plan | no serious defect; its points are below | + +**On straylight** (2026-09-23; image `localhost/boxmaker-tools@sha256:04459bec…`, 16 MB, built by +`deploy/tools-image.nix`; `brokerd` from this branch with `[runner]`): + +| Claim | Seen | +|---|---| +| A granted file is read; a symlink in the granted directory to `~/.ssh/id_ed25519` is not | `hello from the notes`; `read_file: …/notes/key: no such file` | +| `write_file` writes as the owner | the file is owned by uid 1000 | +| No network without a grant | from `shell`: `100.100.100.100` unreachable, `1.1.1.1` unreachable, no DNS, only `lo` | +| Hardening | no capabilities, read-only root, writable `/tmp` | +| Limits | 8 s limit stopped `sleep 60` at 8 s; 64 processes stopped a fork loop; 256m killed a memory hog | +| `http_fetch` reaches only allowed hosts | `example.com` 200; redirect `google.com` → `www.google.com` refused at the proxy (reply 2); an allowed name resolving to the tailnet (`100.88.197.9`) refused (reply 4); a host with no grant denied before any container | +| No container outlives its call | `podman ps -a --filter label=boxmaker` empty after every call; egress directories removed | + +| # | 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 | +| 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 | +| 6 | low | spec (section 5) | `is_public` passes local-use NAT64 `64:ff9b:1::/48` and 6to4 `2002::/16` with a private IPv4 inside. Neither is in use on straylight. A host with its own public address would be reachable by an allowed name that points at it; straylight has none (its addresses are LAN, tailnet and Tailscale's ULA, all refused). | Spec, when next touched | +| 7 | nit | implementer | The runtime notice lacks its `brokerd:` prefix; two config messages use `{}` where the task gave `{:?}`; `chunk[..take]` where the skeleton said `get`; `egress-proxy` accepts trailing arguments; `as u64` on two constants. | When next touched | + +What was good: the proxy handshake reads exactly what the protocol gives against one deadline, has +no panic path, and tries only public addresses (the independent review probed it and found it +sound); the egress guard is created before anything can fail and cleans up on every path, a panic +included; nothing from the model reaches `podman`'s command line. + +**The run.** 11 of 13 tasks committed on the first attempt. Task 04 committed but left `Cargo.lock` +out (the plan's `git add` line). Task 08 needed two attempts (the first did not skip `argv[0]` and +joined each handler thread). Task 11 needed four: the task could not be written as given (a field +nothing read, and `#[allow]` forbidden), then two sessions ran out of room planning the whole file +in one turn; a skeleton, and then a finer one with `run` as glue over small helpers, got it done. +Every stop was a task-writing problem or a turn-size problem, not a wrong implementation. +