From 83b2d3ea1c1fadc02ed31bc59caa0e159f079e58 Mon Sep 17 00:00:00 2001 From: defangdevs Date: Thu, 3 Sep 2026 22:36:42 +0000 Subject: [PATCH 1/2] feat(update): install a candidate release on this box before the fleet An update follows the tracked branch, and `git merge --ff-only` refuses everything else by construction, so a fix to the box's own tooling had to be MERGED before any box could run it. That made the fleet the canary for its own updater: the first machine to execute a change was every machine. "Test it here, then merge" was not expressible. It is now one verb. `agentbox update --rev` already did the work -- fetch a named ref, keep the guard, re-apply, roll back on failure -- and what was missing was a way for an agent to ASK for it. The sudo grant pins the exact argument-free update trigger, and widening it to `agentbox update *` would have let any agent here pass --repo, i.e. have root build code from any repo on GitHub. So the grant goes to a validating wrapper instead. agent-box-candidate BRANCH queues a branch and triggers an update; --status says what the box runs and off what; --reset brings it home. It refuses anything that is not a BRANCH on this box's own configured origin -- `ls-remote --heads` scoping is what keeps refs/pull/N/head out, since a public repo takes pull requests from anyone and a fetchable PR ref would be a stranger's unreviewed code built as root -- and refuses a name that is not a branch name at all: traversal, empty components, characters with no business reaching git or a path. Nothing pins the box. The marker is intent, consumed by the very next update, and an update with no marker always heads for the tracked branch -- so a candidate you forget converges back on the fleet instead of drifting from it. Two properties took some getting right. A candidate must be strictly AHEAD of the tracked branch, or "candidate" becomes a way to replay an older, possibly vulnerable rev. That check lives in agent-box-source, at the moment of the move, for two reasons: the objects are already there, and asking in the wrapper meant FETCHING into the root-owned tree that decides what root builds -- on a question, or on a mistyped branch name. Caught by running the rendered wrapper against the real repo rather than only the test fixture. Coming home needs the guard relaxed, and only that. A candidate is squash-merged, so its head is never an ancestor of the branch it landed on: --ff-only refuses the way back and the box would be stranded off-branch. But "the running rev is not an ancestor of the tracked branch" is NOT a usable signal for that -- a rewritten upstream history looks identical from the ancestry side, and refusing it is the entire point of the guard. So the marker records a FACT (`on=REV`), that fact is verified against the rev the box actually runs before it is believed, and only then does the guard step aside, toward a target that cannot be a downgrade. A stale fact -- a rebuild that failed and rolled the tree back -- is ignored, and the strict guard comes straight back. The first draft did use the ancestry signal; tests/test-source-tree.sh's pre-existing rewritten-history refusal caught it, and now asserts both directions. The grant goes to ONE user (the same one the maintainer checkout goes to, picked the same way), never all of them: an update takes every user's sessions down with it, which is what the per-user password helper and the reboot grant exist to keep out of one agent's hands. Gated on selfUpdate.enable and NOT on web.enable, so a box with no terminal still has it. Checks run: - All 31 native aarch64-linux checks build clean, including the new `candidate` check and `source-tree`'s new assertions, and both parity checks -- whose tables carry the one declared divergence: the module binds AGENT_BOX_CANDIDATE_FILE in the update UNIT because its updater is a unit script, native binds it in-process, exactly as the AGENT_BOX_SRC_* names already do (SRC_TREE_BINDING). The sudoers spelling is a SUBSTRATE normalization, not an exemption: it is the same grant of the same one-argument wrapper, in each backend's own place. - All 40 x86_64-linux checks, every VM test included, evaluate clean (`.drvPath`), so no Nix-level error is waiting for CI. - The rendered wrapper run against the REAL repo: refs/pull/557/head, a refs/heads/ path, a bare sha, and master-at-its-own-head each refused with the right reason; this branch accepted and queued. FETCH_HEAD in /var/lib/agent-box/src was untouched, confirming a question fetches nothing. - `actionlint -shellcheck=` clean; both payloads pass `sh -n`. - Module, native expected tree and golden fixture all regenerated. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TbhGuo3wu5mkgbrkW1kkXv --- .github/workflows/ci.yml | 11 + bin/agentbox | 57 ++- flake.nix | 20 + modules/agent-box.nix | 457 ++++++++++++++++++ modules/agent-box.nix.in | 90 ++++ modules/src/candidate.sh | 270 +++++++++++ modules/src/source-tree.sh | 98 ++++ nix/runtime.nix | 4 + scripts/check_backend_parity.py | 5 + scripts/check_one_spec.py | 8 + .../web/etc/agent-box-guides/AGENTS.agent.md | 27 ++ tests/golden/web/etc/sudoers | 1 + tests/golden/web/payloads/agent-box-candidate | 282 +++++++++++ .../agent-box-source/bin/agent-box-source | 98 ++++ .../golden/web/units/agent-box-update.service | 1 + tests/native/expected-modes.json | 1 + .../etc/agent-box/bin/agent-box-candidate | 279 +++++++++++ tests/native/expected/etc/sudoers.d/agent-box | 2 +- tests/test-candidate.sh | 267 ++++++++++ tests/test-source-tree.sh | 142 ++++++ tests/test_agentbox.py | 7 + 21 files changed, 2125 insertions(+), 2 deletions(-) create mode 100644 modules/src/candidate.sh create mode 100644 tests/golden/web/payloads/agent-box-candidate create mode 100755 tests/native/expected/etc/agent-box/bin/agent-box-candidate create mode 100644 tests/test-candidate.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 90f36427..244bb5bc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -195,6 +195,17 @@ jobs: - name: Check connect card checking state run: nix build -L .#checks.x86_64-linux.connect-card && cat result + # Candidate releases: the validator between an agent's sudo grant and + # what root builds, plus agent-box-source's half (the marker it + # consumes, and the one narrow case where the fast-forward guard + # relaxes). Both are seconds, and there is no VM test that can price + # a refusal. + - name: Check candidate release validator + run: nix build -L .#checks.x86_64-linux.candidate && cat result + + - name: Check source tree manager + run: nix build -L .#checks.x86_64-linux.source-tree && cat result + # Issue #374: the native backend's two flake checks, which nothing # ran until that bug was found by hand. `runtime-profile` builds # packages..runtime — the profile a native box installs at diff --git a/bin/agentbox b/bin/agentbox index 3d5f58ef..116e427b 100755 --- a/bin/agentbox +++ b/bin/agentbox @@ -87,6 +87,18 @@ UPDATE_TRIGGER = f"{SYSTEMCTL} start --no-block {UPDATE_UNIT}" # daemon's HTTP answer is already on the wire when systemd starts stopping # units, this daemon among them. REBOOT_TRIGGER = f"{SYSTEMCTL} reboot --no-block" +# Candidate releases (this box before the fleet). The wrapper is rendered +# rather than shipped as a profile payload because it runs under sudo, whose +# env_reset means it can carry no environment: its repo, tree and trigger are +# baked in by the renderer, so a caller cannot point root at another repo. +# Spelled once for the same #353 reason as the three above — the sudoers line +# and the path the guide tells the agent to run must agree byte for byte. +CANDIDATE_HELPER = "/etc/agent-box/bin/agent-box-candidate" +CANDIDATE_TRIGGER = f"{CANDIDATE_HELPER} *" +# Where that wrapper queues the branch it wants, and where agent-box-source +# consumes it. One update reads it and deletes it: a candidate lasts until +# the next update and never pins the box. +CANDIDATE_FILE = "/var/lib/agent-box/candidate" # Store housekeeping (issue #394). A full root wedges the whole box — # no journal, no profile swap, and an agent that cannot write — and on a # native box nobody is around to run `nix-collect-garbage` by hand. The @@ -2628,6 +2640,15 @@ class Renderer: # per-user password grant above exists to avoid. if self.spec.reboot_button and u is self.spec.root_user: cmds.append(REBOOT_TRIGGER) + # Installing a candidate release goes to the ROOT user alone, + # for the same reason the reboot does: it takes every user's + # sessions down with it, and one user's power over another's + # work is what the per-user grants exist to avoid. `*` allows + # exactly one argument of anything, which is all the wrapper + # accepts — and the wrapper, not sudoers, is what decides + # whether that argument names an installable branch. + if u is self.spec.root_user: + cmds.append(CANDIDATE_TRIGGER) cmds += self.spec.sudo_allowlist lines.append(f"{u.name} ALL=(root) NOPASSWD: " + ", ".join(cmds)) t.file(self.p(SUDOERS_DIR, "agent-box"), @@ -2895,6 +2916,39 @@ class Renderer: f"#!{self.bin}/agent-box-python\n" + body + tail, 0o755) + # agent-box-candidate: install a branch on THIS box before it is + # merged and the fleet takes it (see modules/src/candidate.sh for + # what it refuses). Constants are PREPENDED, not appended: this is + # shell, where the body's `case` runs the moment it is reached, so a + # tail would assign after the code that reads it. Compiled in for + # the same sudo reason as the password helper — env_reset means the + # wrapper carries no environment, and a caller-supplied repo URL + # would be root building code from anywhere. + candidate_src = self.profile / "libexec" / "agent-box" / \ + "candidate.sh" + if candidate_src.is_file(): + # spec.repo, which prefers the PROFILE's manifest over the + # config key: the manifest cannot disagree with what is + # installed, and this wrapper must fetch from the repo the box + # actually came from. No branch key exists natively — an empty + # SRC_BRANCH is what tells the wrapper and agent-box-source to + # follow the remote's own default, rather than guessing + # "master" at a repo that may have renamed it. + url = f"https://github.com/{self.spec.repo}.git" + head = ( + "# Box constants, prepended by `agentbox apply`.\n" + f"CANDIDATE_FILE={shlex.quote(CANDIDATE_FILE)}\n" + f"SRC_DIR={shlex.quote(SRC_DIR)}\n" + f"SRC_URL={shlex.quote(url)}\n" + "SRC_BRANCH=''\n" + f"SYSTEMCTL={shlex.quote(SYSTEMCTL)}\n" + f"UPDATE_TRIGGER={shlex.quote(UPDATE_TRIGGER)}\n" + "\n" + ) + t.file(self.p(CANDIDATE_HELPER), + "#!/bin/sh\n" + head + candidate_src.read_text(), + 0o755) + # Interactive CLIs beyond agent-box-session/agent-box-profile # (interactive_clis, called unconditionally from render()): # agent-box-webhook needs the Caddy route this method renders, so @@ -3840,7 +3894,8 @@ def source_tree(profile, verb, *rest, src=SRC_DIR, url=None, rev=None, the two backends cannot drift on what "fast-forward the box" means — only on what they do with the rev it returns. """ - env = dict(os.environ, AGENT_BOX_SRC_DIR=src) + env = dict(os.environ, AGENT_BOX_SRC_DIR=src, + AGENT_BOX_CANDIDATE_FILE=CANDIDATE_FILE) if url: env["AGENT_BOX_SRC_URL"] = url if rev: diff --git a/flake.nix b/flake.nix index 67544ee1..03140df2 100644 --- a/flake.nix +++ b/flake.nix @@ -1526,6 +1526,26 @@ open(sys.argv[3], "w").write(header + yaml.safe_dump(data, sort_keys=True))' \ cp log "$out" ''; + # agent-box-candidate (this box before the fleet): the validator + # standing between an agent's sudo grant and what root builds. + # Same shape and same reasoning as source-tree above -- weighted + # at the refusals, `origin` a local repository so there is no + # network, and natively runnable on every architecture. + candidate = + pkgs.runCommand "agent-box-candidate-check" + { + nativeBuildInputs = [ pkgs.bash pkgs.coreutils pkgs.git pkgs.gnugrep pkgs.gnused ]; + script = ./modules/src/candidate.sh; + tests = ./tests/test-candidate.sh; + } '' + bash "$tests" "$script" > log 2>&1 || { + cat log + exit 1 + } + cat log + cp log "$out" + ''; + # Issue #425: a box with no webhook panel used to render an # empty string, so its operator could not tell a feature that is # off from one that is wired up wrong — which is how #425 was diff --git a/modules/agent-box.nix b/modules/agent-box.nix index cb0256cf..f06904e1 100644 --- a/modules/agent-box.nix +++ b/modules/agent-box.nix @@ -299,6 +299,34 @@ let rebuild that failed and could not put the tree back, which the wall notice says - it reads ahead of the running system. + '' + lib.optionalString candidateEnabled '' + ## Trying YOUR fix on this box, before every other box takes it + + An update follows the tracked branch, so a fix used to have to be + merged before this box could run it - which made the whole fleet the + canary for its own tooling. Push the branch and try it here first: + + sudo ${candidateHelper} BRANCH # install it, then re-apply + sudo ${candidateHelper} --status # what is this box running? + sudo ${candidateHelper} --reset # go back to the tracked branch + + Same restart as an ordinary update, so save context first. Then merge + once you have SEEN it work, and the fleet takes a change that has run + somewhere. + + Three things it refuses, all deliberately. Only a BRANCH on this + box's own origin - not a tag, not a sha, and not `refs/pull/N/head`, + because a public repo takes pull requests from anyone and that would + be a stranger's unreviewed code built as root. Only a branch strictly + AHEAD of the tracked branch, so "candidate" can never become a way to + replay an older rev - rebase yours, which is also the only way what + you test here is what will land. And it never pins this box: the next + update returns to the tracked branch and says so, so a candidate you + forget converges back on the fleet instead of drifting from it. That + return needs no flag from you, and is the one case where the + fast-forward guard steps aside - a squash-merged branch is never an + ancestor of the branch it landed on, so nothing else could get home. + '' + lib.optionalString checkoutEnabled '' ## This box ships its own sources @@ -830,6 +858,315 @@ let else if names != [ ] then lib.head names else null; checkoutEnabled = checkoutMaintainer != null; + # Candidate releases: install a branch on THIS box before it is merged and + # every other box takes it. Same user the maintainer checkout goes to, and + # picked the same way, because it is the same job seen from the other end — + # that user holds the tree a fix is written in, and this is how the fix gets + # tried here first. ONE user, never all of them: an update takes every + # user's sessions down with it, which is exactly what the per-user password + # helper and the reboot grant exist to keep out of one agent's hands. + # Gated on selfUpdate.enable alone and NOT on web.enable — a box with no + # terminal still updates, and its agent still has a fix to try (the trap + # tests/memory-protection.nix exists to catch). + candidateUser = + let names = lib.attrNames cfg.users; in + if !cfg.selfUpdate.enable then null + else if cfg.users ? ${cfg.web.user} then cfg.web.user + else if names != [ ] then lib.head names + else null; + candidateEnabled = candidateUser != null; + # Beside the tree, not inside it: the tree is a checkout git rewrites, and + # a marker within it would be one `git clean` from vanishing. srcDir's own + # parent is created by agent-box-source before it clones, so this always + # lands in a directory that exists by the time anything writes it. + candidateFile = "${cfg.selfUpdate.srcDir}.candidate"; + # The wrapper an agent reaches through sudo. Constants are PREPENDED, not + # appended: this is shell, and the body acts the moment it is read. They are + # compiled into the store script rather than read from the environment + # because sudo's env_reset carries none — and a caller-supplied repo URL + # would be root building code from anywhere (issue #154 Phase 2's rule). + candidateHelper = pkgs.writeShellScript "agent-box-candidate" '' + # Box constants, prepended by the module. + CANDIDATE_FILE=${lib.escapeShellArg candidateFile} + SRC_DIR=${lib.escapeShellArg cfg.selfUpdate.srcDir} + SRC_URL=${lib.escapeShellArg "https://github.com/${cfg.selfUpdate.repo}.git"} + SRC_BRANCH=${lib.escapeShellArg (if cfg.selfUpdate.branch != null then cfg.selfUpdate.branch else "")} + SYSTEMCTL=/run/current-system/sw/bin/systemctl + UPDATE_TRIGGER=${lib.escapeShellArg updateStartCmd} + PATH=${lib.makeBinPath [ pkgs.git pkgs.coreutils pkgs.gnused pkgs.systemd ]}:$PATH + export PATH + + # agent-box-candidate — install a candidate release on THIS box, before it + # is merged and every other box gets it. + # + # The box's own fix used to have to land on master first. The only + # root-capable path an agent has is the update trigger, whose unit follows + # the tracked branch, and `git merge --ff-only` refuses everything else by + # construction — so "test it here, then merge" was not expressible and the + # fleet was the canary for its own tooling. This is the missing verb. + # + # It is a ROOT wrapper for the same reason the password helper is one: what + # writes the source tree decides what root builds, so the tree stays + # root-owned (issue #242) and an agent reaches it only through a command + # that can refuse. Every constant below is baked in by the renderer rather + # than read from the environment, because sudo scrubs the environment + # (env_reset) and a wrapper that took its target repo from the caller would + # hand root an arbitrary URL to build from. + # + # Three things it will not do, which is most of the point: + # + # * Anything but a BRANCH on this box's own configured origin. Not a + # tag, not a sha, and above all not refs/pull/N/head: on a public repo + # anyone can open a pull request, and a fetchable PR ref would mean a + # stranger's unreviewed code built as root. A branch needs push access + # to the source repo, and whoever has that can already merge. + # * A candidate that is not strictly ahead of the tracked branch. That is + # what stops an old, possibly vulnerable rev being replayed as a + # "candidate", which is the one attack the fast-forward guard exists to + # refuse and which the force this feature needs would otherwise reopen. + # The rev-equality half is here; ANCESTRY is agent-box-source's, at the + # moment of the move, because that is where the objects are and a + # question must not fetch into the tree to answer itself. + # * A permanent divergence. Nothing here pins the box: the marker it + # writes is consumed by the very next update, and an update with no + # marker always heads for the tracked branch (forcing only when the box + # is off it, since a squash-merged candidate is never an ancestor of + # master). So a candidate survives exactly until the next update, and a + # box that is forgotten converges on the fleet instead of drifting from + # it. + # + # Usage, all idempotent: + # + # agent-box-candidate BRANCH install that branch and re-apply + # agent-box-candidate --reset go back to the tracked branch now + # agent-box-candidate --status what the box is running, and off what + # + # The renderer prepends: CANDIDATE_FILE, SRC_DIR, SRC_URL, SRC_BRANCH, + # UPDATE_TRIGGER, SYSTEMCTL. + set -u + + prog=agent-box-candidate + say() { printf '%s: %s\n' "$prog" "$*" >&2; } + die() { say "$@"; exit 1; } + + # Same hardening as agent-box-source, and for the same reason: every git + # below runs as root against a tree fetched from the network, and a + # credential prompt has nobody to answer it. + export GIT_TERMINAL_PROMPT=0 + unset GIT_ASKPASS SSH_ASKPASS + export GIT_HTTP_LOW_SPEED_LIMIT=1000 GIT_HTTP_LOW_SPEED_TIME=60 + export GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.hooksPath GIT_CONFIG_VALUE_0=/dev/null + + usage() { + cat >&2 </dev/null) \ + || die "cannot reach $SRC_URL — offline?" + [ -n "$out" ] || die "origin has no branch '$b'" + else + out=$(command git ls-remote "$SRC_URL" HEAD 2>/dev/null) \ + || die "cannot reach $SRC_URL — offline?" + [ -n "$out" ] || die "$SRC_URL has no HEAD" + fi + printf '%s\n' "$out" | head -n 1 | cut -f1 + } + + running_rev() { + # The tree names the rev the box runs (the guide says so, so it has to be + # true here too). A box that has never updated has no tree yet, which is + # not an error for --status. + [ -e "$SRC_DIR/.git" ] || return 1 + command git -C "$SRC_DIR" rev-parse --verify --quiet HEAD 2>/dev/null + } + + case "''${1:-}" in + (-h|--help) + usage + exit 0 + ;; + + (--status) + rev=$(running_rev) || die "$SRC_DIR is not a checkout yet — this box has never updated" + head=$(tracked_head) + printf 'rev: %s\n' "$rev" + printf 'tracked: %s at %s\n' "''${SRC_BRANCH:-origin/HEAD}" "$head" + # The marker's own claim, believed only when it matches the rev the box + # actually runs — the same check agent-box-source makes before it + # relaxes the fast-forward guard, and for the same reason: a rebuild + # that failed and rolled back leaves a claim that is no longer true. + on="" + [ -e "$CANDIDATE_FILE" ] \ + && on=$(sed -n 's/^on=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + if [ -n "$on" ] && [ "$on" = "$rev" ]; then + printf 'state: CANDIDATE — the next update returns to %s\n' \ + "''${SRC_BRANCH:-the tracked branch}" + elif [ "$rev" = "$head" ]; then + printf 'state: on the tracked branch\n' + elif command git -C "$SRC_DIR" merge-base --is-ancestor "$rev" "$head" 2>/dev/null; then + printf 'state: behind the tracked branch — an update is available\n' + else + # Off the branch with nothing claiming a candidate: a rewritten + # upstream history, or a tree somebody moved by hand. NOT reported as + # a candidate, because agent-box-source will not force out of it + # either — it is the case --ff-only exists to refuse. + printf 'state: off %s, and not as a candidate — the fast-forward guard applies\n' \ + "''${SRC_BRANCH:-the tracked branch}" + fi + want="" + [ -e "$CANDIDATE_FILE" ] \ + && want=$(sed -n 's/^want=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + [ -n "$want" ] && printf 'pending: %s\n' "$want" + exit 0 + ;; + + (--reset) + # An update with no QUEUED candidate already heads for the tracked + # branch, so "reset" is the ordinary update — the only work here is + # dropping a `want` that has not been consumed yet. The `on` fact must + # survive: it is what tells agent-box-source this box has a way home to + # relax the guard for, and deleting it would strand the box exactly + # where the reset was meant to rescue it from. + if [ -e "$CANDIDATE_FILE" ]; then + on=$(sed -n 's/^on=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + if [ -n "$on" ]; then + printf 'on=%s\n' "$on" > "$CANDIDATE_FILE.tmp" \ + && mv "$CANDIDATE_FILE.tmp" "$CANDIDATE_FILE" + else + rm -f "$CANDIDATE_FILE" + fi + fi + say "returning to ''${SRC_BRANCH:-the tracked branch} — triggering an update" + exec $UPDATE_TRIGGER + ;; + + ("") + usage + exit 1 + ;; + + (-*) + say "unknown option '$1'" + usage + exit 1 + ;; + esac + + branch=$1 + shift + [ $# -eq 0 ] || die "one branch at a time (got extra: $*)" + + # --- validate the name ------------------------------------------------ + # + # The name reaches git and reaches a file, so it is checked before either. + # Deliberately narrower than git's own refname rules: this is an allowlist + # of what a branch in this repo actually looks like, not an attempt to + # re-implement git-check-ref-format. + case $branch in + (refs/heads/*) + die "name the branch itself: ''\'''${branch#refs/heads/}' rather than '$branch'" + ;; + (refs/*) + # refs/pull/N/head lands here, which is the whole reason this arm is + # separate: there is no branch to suggest, and suggesting one by + # stripping refs/heads/ off a path that has no such prefix printed the + # input back as its own correction. + die "'$branch' is a ref path, not a branch — only a branch of $SRC_URL can be installed" + ;; + (*..*) + die "'$branch' contains '..'" + ;; + (/*|*/) + die "'$branch' starts or ends with '/'" + ;; + (*//*) + die "'$branch' contains '//'" + ;; + (*[!A-Za-z0-9._/-]*) + die "'$branch' has characters outside A-Za-z0-9._/- — refusing" + ;; + ([!A-Za-z0-9]*) + die "'$branch' does not start with a letter or digit" + ;; + esac + [ ''${#branch} -le 200 ] || die "branch name is ''${#branch} characters — refusing" + + # --- it must be a branch on OUR origin -------------------------------- + # + # ls-remote --heads, so only refs/heads/* can match. This is the line that + # keeps refs/pull/N/head — a stranger's unreviewed pull request on a public + # repo — from ever being installed as root. + out=$(command git ls-remote --heads "$SRC_URL" "refs/heads/$branch" 2>/dev/null) \ + || die "cannot reach $SRC_URL — offline?" + [ -n "$out" ] || die "$SRC_URL has no branch '$branch' (a tag, a sha or a pull-request ref will not do)" + want=$(printf '%s\n' "$out" | head -n 1 | cut -f1) + + # --- and not the tracked branch's own head ---------------------------- + # + # The cheap half of "strictly ahead", and the only half that can be settled + # from ls-remote. ANCESTRY is checked by agent-box-source at the moment of + # the move, where the objects already are: asking here would mean fetching + # them, and a fetch is not something a refused candidate — or a mistyped + # branch name — should leave behind in the tree that decides what root + # builds. That check is also the better home for it on its own merits, since + # it then holds however the marker was written. + head=$(tracked_head) + if [ "$want" = "$head" ]; then + die "'$branch' is at ''${head} — the same rev as ''${SRC_BRANCH:-the tracked branch}, so there is nothing to try" + fi + + # --- record the intent and let the update do the work ----------------- + # + # The marker is intent, never state: the updater consumes it as it starts, + # so a candidate that fails to build is not retried forever, and "what is + # this box running" stays a question answered by the tree rather than by a + # file that can disagree with it. + umask 022 + # A box that has never updated has no tree and may have no state dir either, + # and the marker's parent is that dir. Create it rather than failing: the + # update this queues would create it anyway when it clones. + mkdir -p "$(dirname "$CANDIDATE_FILE")" \ + || die "could not create $(dirname "$CANDIDATE_FILE")" + # An existing `on` fact is carried over, not dropped: a box already running + # a candidate is off the tracked branch, so moving it to ANOTHER candidate + # needs the same relaxation coming home does. agent-box-source re-verifies + # the fact against the running rev before it believes it either way. + on="" + [ -e "$CANDIDATE_FILE" ] \ + && on=$(sed -n 's/^on=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + # A `[ ... ] && printf` as the LAST command in the group would make the + # group's status that test's — 1 whenever there is no fact to carry — and + # the `|| die` below would then report a failure on a write that worked. + if [ -n "$on" ]; then + printf 'want=refs/heads/%s\non=%s\n' "$branch" "$on" > "$CANDIDATE_FILE.tmp" \ + || die "could not write $CANDIDATE_FILE.tmp" + else + printf 'want=refs/heads/%s\n' "$branch" > "$CANDIDATE_FILE.tmp" \ + || die "could not write $CANDIDATE_FILE.tmp" + fi + mv "$CANDIDATE_FILE.tmp" "$CANDIDATE_FILE" \ + || die "could not put $CANDIDATE_FILE in place" + say "candidate $branch ($(printf '%.12s' "$want")) queued — triggering an update" + say "this box will be off ''${SRC_BRANCH:-the tracked branch}; the next update returns to it" + exec $UPDATE_TRIGGER + ''; # Always under the maintainer's home, and the assertion below enforces it. # The agent unit runs ProtectSystem=strict with ReadWritePaths=/home/%i, # so a tree anywhere else is denied with EROFS — and the obvious repair, @@ -7216,6 +7553,13 @@ ref=''${AGENT_BOX_SRC_REF:-} # (`agentbox update --force`). Non-empty means the target replaces HEAD even # when it is not a descendant. force=''${AGENT_BOX_SRC_FORCE:-} +# Where agent-box-candidate leaves the branch it wants tried on this box +# before it is merged. Read by `pull` and CONSUMED there — it is intent, not +# state: what this box runs is the tree's own HEAD, and a marker that +# outlived its update would be a second answer able to disagree with it. +# Empty (no such wiring) is a box with the feature off, which behaves +# exactly as it did before it existed. +candidate_file=''${AGENT_BOX_CANDIDATE_FILE:-} [ -n "$dir" ] || die "AGENT_BOX_SRC_DIR is unset — there is no tree to act on" case "$dir" in @@ -7362,6 +7706,56 @@ case "''${1:-}" in (pull) [ -n "$rev" ] || die "AGENT_BOX_SRC_REV is unset — refusing to move a tree with no baseline" + # A queued candidate (agent-box-candidate BRANCH) is the target for + # exactly one update. Consumed BEFORE anything can fail, so a candidate + # that does not build is not retried on every later trigger — the next + # one goes back to the tracked branch, which is the whole of the + # "nothing here pins this box" promise. An explicit --rev wins: an + # operator naming a ref is not asking about the queue. + # Whether the TARGET was named by the caller, which decides below + # whether the fast-forward guard may relax itself. Recorded before the + # queue can overwrite $ref. + explicit_ref=$ref + # The candidate marker carries two fields, and the difference between + # them is the difference between a request and a fact: + # + # want=REF agent-box-candidate asks for REF on the next update. + # on=REV this box IS running the candidate at REV. + # + # `on` is checked against the rev the box actually runs before it is + # believed, and that check is what makes it safe to relax the + # fast-forward guard. "The running rev is not an ancestor of the tracked + # branch" is NOT a usable signal on its own: a squash-merged candidate + # and a rewritten upstream history look identical from here, and the + # second is the exact attack --ff-only exists to refuse. So a stale `on` + # (a rebuild that failed and rolled the tree back, an operator moving + # the tree by hand) is ignored rather than trusted, and the strict guard + # comes straight back. + candidate="" + on_rev="" + if [ -n "$candidate_file" ] && [ -e "$candidate_file" ]; then + on_rev=$(sed -n 's/^on=//p' "$candidate_file" 2>/dev/null | head -n 1) + if [ -z "$ref" ]; then + candidate=$(sed -n 's/^want=//p' "$candidate_file" 2>/dev/null | head -n 1) + fi + if [ -n "$candidate" ]; then + # Consumed BEFORE anything can fail, so a candidate that does not + # build is not reinstalled by every later trigger. What survives is + # only the `on` fact, which the next run re-verifies. + if [ -n "$on_rev" ]; then + printf 'on=%s\n' "$on_rev" > "$candidate_file.tmp" \ + && mv "$candidate_file.tmp" "$candidate_file" + else + rm -f "$candidate_file" + fi + say "installing candidate $candidate (queued on this box, not merged)" + ref=$candidate + fi + fi + if [ -n "$on_rev" ] && [ "$on_rev" != "$rev" ]; then + say "candidate marker names $on_rev but the box runs $rev — ignoring it" + on_rev="" + fi clone_if_missing target=$(target_rev) || exit 1 resolve_branch @@ -7384,6 +7778,36 @@ case "''${1:-}" in git checkout --quiet -B "$branch" "$rev" \ || die "could not put $dir on $branch at $rev" fi + # A candidate must be strictly AHEAD of the tracked branch, and this is + # where that is settled: the fetch above already brought both objects + # in, so it costs one ancestry query, and it holds however the marker + # came to be written. It is what keeps "candidate" from becoming a way + # to replay an older, possibly vulnerable rev — the exact thing + # --ff-only refuses and the exact thing the force below would otherwise + # reopen, since a box already off the branch takes that force path. + if [ -n "$candidate" ]; then + if ! git merge-base --is-ancestor "refs/remotes/origin/$branch" \ + "$target" 2>/dev/null; then + die "refusing candidate $candidate: $target is not ahead of $branch — rebase it, so what is tested here is what will land" + fi + fi + # Coming BACK from a candidate needs the guard relaxed, and only that. + # A candidate is squash-merged, so its head is never an ancestor of the + # tracked branch: once this box has taken one, --ff-only refuses the way + # home and the box would be stuck off-branch — the exact permanent + # divergence the feature promises not to create. + # + # Narrow on purpose. It needs the marker to say this box is running a + # candidate AND that claim to match the rev it actually runs (checked + # above), so a rewritten upstream history — which looks the same from + # the ancestry side — is still refused. The target is then either the + # tracked branch's own head or a branch agent-box-candidate already + # proved is ahead of it, so neither can be a downgrade. An explicit + # --rev keeps the strict guard and still has --force of its own. + if [ -z "$force" ] && [ -z "$explicit_ref" ] && [ -n "$on_rev" ]; then + say "the box runs candidate $rev — moving to $target without the fast-forward check" + force=1 + fi if [ "$target" = "$rev" ]; then : # nothing to move to; fall through and print the rev we are on elif [ -z "$force" ] && git merge-base --is-ancestor "$target" "$rev"; then @@ -7406,6 +7830,17 @@ case "''${1:-}" in # nothing here should be resolving conflicts in. die "refusing update: $target is not a fast-forward of the running rev $rev" fi + # Record what the box is now running, so the NEXT plain update knows it + # has a way home to relax the guard for — and stop recording it once it + # is home, so nothing is left pinned or forced afterwards. + if [ -n "$candidate_file" ]; then + if [ -n "$candidate" ]; then + printf 'on=%s\n' "$(git rev-parse HEAD)" > "$candidate_file.tmp" \ + && mv "$candidate_file.tmp" "$candidate_file" + elif [ -n "$on_rev" ]; then + rm -f "$candidate_file" + fi + fi git rev-parse HEAD ;; @@ -11612,6 +12047,24 @@ in # in the store. Verifying releases against an offline signing key is # tracked upstream (defangdevs/agent-box issue 46); until then this # trusts the pinned repo as GitHub serves it. + # One user may install a candidate release (see candidateHelper). Its own + # rule rather than a line in effectiveSudoAllowlist, for the reason the + # password helper and the reboot grant are also their own: that list is + # box-wide, and this must not become every agent user's power to restart + # every other user's sessions onto a branch of their choosing. + # + # `*` allows exactly one argument of anything, which is all the wrapper + # takes. sudoers is not the validator here and should not try to be — the + # wrapper is, and it refuses everything that is not a branch on this box's + # own origin, strictly ahead of the tracked branch. + security.sudo.extraRules = lib.optional candidateEnabled { + users = [ candidateUser ]; + commands = [{ + command = "${candidateHelper} *"; + options = [ "NOPASSWD" ]; + }]; + }; + systemd.services.agent-box-update = { description = "Fast-forward agent-box to upstream HEAD and rebuild"; # No wantedBy — on-demand only, via the agents' sudo rule (or root). @@ -11631,6 +12084,10 @@ in AGENT_BOX_SRC_DIR = cfg.selfUpdate.srcDir; AGENT_BOX_SRC_URL = "https://github.com/${cfg.selfUpdate.repo}.git"; AGENT_BOX_SRC_REV = cfg.selfUpdate.rev; + # A candidate agent-box-candidate queued, if any. agent-box-source + # CONSUMES it: this update installs it, and the next one goes back to + # the tracked branch, so a candidate never becomes a pin. + AGENT_BOX_CANDIDATE_FILE = candidateFile; # git reads a config file relative to HOME and warns (or, with some # safe.directory setups, refuses) without one. The unit inherits no # HOME of its own. diff --git a/modules/agent-box.nix.in b/modules/agent-box.nix.in index 8fbca900..ae2caeed 100644 --- a/modules/agent-box.nix.in +++ b/modules/agent-box.nix.in @@ -60,6 +60,34 @@ let rebuild that failed and could not put the tree back, which the wall notice says - it reads ahead of the running system. + '' + lib.optionalString candidateEnabled '' + ## Trying YOUR fix on this box, before every other box takes it + + An update follows the tracked branch, so a fix used to have to be + merged before this box could run it - which made the whole fleet the + canary for its own tooling. Push the branch and try it here first: + + sudo ${candidateHelper} BRANCH # install it, then re-apply + sudo ${candidateHelper} --status # what is this box running? + sudo ${candidateHelper} --reset # go back to the tracked branch + + Same restart as an ordinary update, so save context first. Then merge + once you have SEEN it work, and the fleet takes a change that has run + somewhere. + + Three things it refuses, all deliberately. Only a BRANCH on this + box's own origin - not a tag, not a sha, and not `refs/pull/N/head`, + because a public repo takes pull requests from anyone and that would + be a stranger's unreviewed code built as root. Only a branch strictly + AHEAD of the tracked branch, so "candidate" can never become a way to + replay an older rev - rebase yours, which is also the only way what + you test here is what will land. And it never pins this box: the next + update returns to the tracked branch and says so, so a candidate you + forget converges back on the fleet instead of drifting from it. That + return needs no flag from you, and is the one case where the + fast-forward guard steps aside - a squash-merged branch is never an + ancestor of the branch it landed on, so nothing else could get home. + '' + lib.optionalString checkoutEnabled '' ## This box ships its own sources @@ -233,6 +261,46 @@ let else if names != [ ] then lib.head names else null; checkoutEnabled = checkoutMaintainer != null; + # Candidate releases: install a branch on THIS box before it is merged and + # every other box takes it. Same user the maintainer checkout goes to, and + # picked the same way, because it is the same job seen from the other end — + # that user holds the tree a fix is written in, and this is how the fix gets + # tried here first. ONE user, never all of them: an update takes every + # user's sessions down with it, which is exactly what the per-user password + # helper and the reboot grant exist to keep out of one agent's hands. + # Gated on selfUpdate.enable alone and NOT on web.enable — a box with no + # terminal still updates, and its agent still has a fix to try (the trap + # tests/memory-protection.nix exists to catch). + candidateUser = + let names = lib.attrNames cfg.users; in + if !cfg.selfUpdate.enable then null + else if cfg.users ? ${cfg.web.user} then cfg.web.user + else if names != [ ] then lib.head names + else null; + candidateEnabled = candidateUser != null; + # Beside the tree, not inside it: the tree is a checkout git rewrites, and + # a marker within it would be one `git clean` from vanishing. srcDir's own + # parent is created by agent-box-source before it clones, so this always + # lands in a directory that exists by the time anything writes it. + candidateFile = "${cfg.selfUpdate.srcDir}.candidate"; + # The wrapper an agent reaches through sudo. Constants are PREPENDED, not + # appended: this is shell, and the body acts the moment it is read. They are + # compiled into the store script rather than read from the environment + # because sudo's env_reset carries none — and a caller-supplied repo URL + # would be root building code from anywhere (issue #154 Phase 2's rule). + candidateHelper = pkgs.writeShellScript "agent-box-candidate" '' + # Box constants, prepended by the module. + CANDIDATE_FILE=${lib.escapeShellArg candidateFile} + SRC_DIR=${lib.escapeShellArg cfg.selfUpdate.srcDir} + SRC_URL=${lib.escapeShellArg "https://github.com/${cfg.selfUpdate.repo}.git"} + SRC_BRANCH=${lib.escapeShellArg (if cfg.selfUpdate.branch != null then cfg.selfUpdate.branch else "")} + SYSTEMCTL=/run/current-system/sw/bin/systemctl + UPDATE_TRIGGER=${lib.escapeShellArg updateStartCmd} + PATH=${lib.makeBinPath [ pkgs.git pkgs.coreutils pkgs.gnused pkgs.systemd ]}:$PATH + export PATH + + @@include:src/candidate.sh@@ + ''; # Always under the maintainer's home, and the assertion below enforces it. # The agent unit runs ProtectSystem=strict with ReadWritePaths=/home/%i, # so a tree anywhere else is denied with EROFS — and the obvious repair, @@ -2891,6 +2959,24 @@ in # in the store. Verifying releases against an offline signing key is # tracked upstream (defangdevs/agent-box issue 46); until then this # trusts the pinned repo as GitHub serves it. + # One user may install a candidate release (see candidateHelper). Its own + # rule rather than a line in effectiveSudoAllowlist, for the reason the + # password helper and the reboot grant are also their own: that list is + # box-wide, and this must not become every agent user's power to restart + # every other user's sessions onto a branch of their choosing. + # + # `*` allows exactly one argument of anything, which is all the wrapper + # takes. sudoers is not the validator here and should not try to be — the + # wrapper is, and it refuses everything that is not a branch on this box's + # own origin, strictly ahead of the tracked branch. + security.sudo.extraRules = lib.optional candidateEnabled { + users = [ candidateUser ]; + commands = [{ + command = "${candidateHelper} *"; + options = [ "NOPASSWD" ]; + }]; + }; + systemd.services.agent-box-update = { description = "Fast-forward agent-box to upstream HEAD and rebuild"; # No wantedBy — on-demand only, via the agents' sudo rule (or root). @@ -2910,6 +2996,10 @@ in AGENT_BOX_SRC_DIR = cfg.selfUpdate.srcDir; AGENT_BOX_SRC_URL = "https://github.com/${cfg.selfUpdate.repo}.git"; AGENT_BOX_SRC_REV = cfg.selfUpdate.rev; + # A candidate agent-box-candidate queued, if any. agent-box-source + # CONSUMES it: this update installs it, and the next one goes back to + # the tracked branch, so a candidate never becomes a pin. + AGENT_BOX_CANDIDATE_FILE = candidateFile; # git reads a config file relative to HOME and warns (or, with some # safe.directory setups, refuses) without one. The unit inherits no # HOME of its own. diff --git a/modules/src/candidate.sh b/modules/src/candidate.sh new file mode 100644 index 00000000..f6fcbe94 --- /dev/null +++ b/modules/src/candidate.sh @@ -0,0 +1,270 @@ +# agent-box-candidate — install a candidate release on THIS box, before it +# is merged and every other box gets it. +# +# The box's own fix used to have to land on master first. The only +# root-capable path an agent has is the update trigger, whose unit follows +# the tracked branch, and `git merge --ff-only` refuses everything else by +# construction — so "test it here, then merge" was not expressible and the +# fleet was the canary for its own tooling. This is the missing verb. +# +# It is a ROOT wrapper for the same reason the password helper is one: what +# writes the source tree decides what root builds, so the tree stays +# root-owned (issue #242) and an agent reaches it only through a command +# that can refuse. Every constant below is baked in by the renderer rather +# than read from the environment, because sudo scrubs the environment +# (env_reset) and a wrapper that took its target repo from the caller would +# hand root an arbitrary URL to build from. +# +# Three things it will not do, which is most of the point: +# +# * Anything but a BRANCH on this box's own configured origin. Not a +# tag, not a sha, and above all not refs/pull/N/head: on a public repo +# anyone can open a pull request, and a fetchable PR ref would mean a +# stranger's unreviewed code built as root. A branch needs push access +# to the source repo, and whoever has that can already merge. +# * A candidate that is not strictly ahead of the tracked branch. That is +# what stops an old, possibly vulnerable rev being replayed as a +# "candidate", which is the one attack the fast-forward guard exists to +# refuse and which the force this feature needs would otherwise reopen. +# The rev-equality half is here; ANCESTRY is agent-box-source's, at the +# moment of the move, because that is where the objects are and a +# question must not fetch into the tree to answer itself. +# * A permanent divergence. Nothing here pins the box: the marker it +# writes is consumed by the very next update, and an update with no +# marker always heads for the tracked branch (forcing only when the box +# is off it, since a squash-merged candidate is never an ancestor of +# master). So a candidate survives exactly until the next update, and a +# box that is forgotten converges on the fleet instead of drifting from +# it. +# +# Usage, all idempotent: +# +# agent-box-candidate BRANCH install that branch and re-apply +# agent-box-candidate --reset go back to the tracked branch now +# agent-box-candidate --status what the box is running, and off what +# +# The renderer prepends: CANDIDATE_FILE, SRC_DIR, SRC_URL, SRC_BRANCH, +# UPDATE_TRIGGER, SYSTEMCTL. +set -u + +prog=agent-box-candidate +say() { printf '%s: %s\n' "$prog" "$*" >&2; } +die() { say "$@"; exit 1; } + +# Same hardening as agent-box-source, and for the same reason: every git +# below runs as root against a tree fetched from the network, and a +# credential prompt has nobody to answer it. +export GIT_TERMINAL_PROMPT=0 +unset GIT_ASKPASS SSH_ASKPASS +export GIT_HTTP_LOW_SPEED_LIMIT=1000 GIT_HTTP_LOW_SPEED_TIME=60 +export GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.hooksPath GIT_CONFIG_VALUE_0=/dev/null + +usage() { + cat >&2 </dev/null) \ + || die "cannot reach $SRC_URL — offline?" + [ -n "$out" ] || die "origin has no branch '$b'" + else + out=$(command git ls-remote "$SRC_URL" HEAD 2>/dev/null) \ + || die "cannot reach $SRC_URL — offline?" + [ -n "$out" ] || die "$SRC_URL has no HEAD" + fi + printf '%s\n' "$out" | head -n 1 | cut -f1 +} + +running_rev() { + # The tree names the rev the box runs (the guide says so, so it has to be + # true here too). A box that has never updated has no tree yet, which is + # not an error for --status. + [ -e "$SRC_DIR/.git" ] || return 1 + command git -C "$SRC_DIR" rev-parse --verify --quiet HEAD 2>/dev/null +} + +case "${1:-}" in + (-h|--help) + usage + exit 0 + ;; + + (--status) + rev=$(running_rev) || die "$SRC_DIR is not a checkout yet — this box has never updated" + head=$(tracked_head) + printf 'rev: %s\n' "$rev" + printf 'tracked: %s at %s\n' "${SRC_BRANCH:-origin/HEAD}" "$head" + # The marker's own claim, believed only when it matches the rev the box + # actually runs — the same check agent-box-source makes before it + # relaxes the fast-forward guard, and for the same reason: a rebuild + # that failed and rolled back leaves a claim that is no longer true. + on="" + [ -e "$CANDIDATE_FILE" ] \ + && on=$(sed -n 's/^on=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + if [ -n "$on" ] && [ "$on" = "$rev" ]; then + printf 'state: CANDIDATE — the next update returns to %s\n' \ + "${SRC_BRANCH:-the tracked branch}" + elif [ "$rev" = "$head" ]; then + printf 'state: on the tracked branch\n' + elif command git -C "$SRC_DIR" merge-base --is-ancestor "$rev" "$head" 2>/dev/null; then + printf 'state: behind the tracked branch — an update is available\n' + else + # Off the branch with nothing claiming a candidate: a rewritten + # upstream history, or a tree somebody moved by hand. NOT reported as + # a candidate, because agent-box-source will not force out of it + # either — it is the case --ff-only exists to refuse. + printf 'state: off %s, and not as a candidate — the fast-forward guard applies\n' \ + "${SRC_BRANCH:-the tracked branch}" + fi + want="" + [ -e "$CANDIDATE_FILE" ] \ + && want=$(sed -n 's/^want=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + [ -n "$want" ] && printf 'pending: %s\n' "$want" + exit 0 + ;; + + (--reset) + # An update with no QUEUED candidate already heads for the tracked + # branch, so "reset" is the ordinary update — the only work here is + # dropping a `want` that has not been consumed yet. The `on` fact must + # survive: it is what tells agent-box-source this box has a way home to + # relax the guard for, and deleting it would strand the box exactly + # where the reset was meant to rescue it from. + if [ -e "$CANDIDATE_FILE" ]; then + on=$(sed -n 's/^on=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + if [ -n "$on" ]; then + printf 'on=%s\n' "$on" > "$CANDIDATE_FILE.tmp" \ + && mv "$CANDIDATE_FILE.tmp" "$CANDIDATE_FILE" + else + rm -f "$CANDIDATE_FILE" + fi + fi + say "returning to ${SRC_BRANCH:-the tracked branch} — triggering an update" + exec $UPDATE_TRIGGER + ;; + + ("") + usage + exit 1 + ;; + + (-*) + say "unknown option '$1'" + usage + exit 1 + ;; +esac + +branch=$1 +shift +[ $# -eq 0 ] || die "one branch at a time (got extra: $*)" + +# --- validate the name ------------------------------------------------ +# +# The name reaches git and reaches a file, so it is checked before either. +# Deliberately narrower than git's own refname rules: this is an allowlist +# of what a branch in this repo actually looks like, not an attempt to +# re-implement git-check-ref-format. +case $branch in + (refs/heads/*) + die "name the branch itself: '${branch#refs/heads/}' rather than '$branch'" + ;; + (refs/*) + # refs/pull/N/head lands here, which is the whole reason this arm is + # separate: there is no branch to suggest, and suggesting one by + # stripping refs/heads/ off a path that has no such prefix printed the + # input back as its own correction. + die "'$branch' is a ref path, not a branch — only a branch of $SRC_URL can be installed" + ;; + (*..*) + die "'$branch' contains '..'" + ;; + (/*|*/) + die "'$branch' starts or ends with '/'" + ;; + (*//*) + die "'$branch' contains '//'" + ;; + (*[!A-Za-z0-9._/-]*) + die "'$branch' has characters outside A-Za-z0-9._/- — refusing" + ;; + ([!A-Za-z0-9]*) + die "'$branch' does not start with a letter or digit" + ;; +esac +[ ${#branch} -le 200 ] || die "branch name is ${#branch} characters — refusing" + +# --- it must be a branch on OUR origin -------------------------------- +# +# ls-remote --heads, so only refs/heads/* can match. This is the line that +# keeps refs/pull/N/head — a stranger's unreviewed pull request on a public +# repo — from ever being installed as root. +out=$(command git ls-remote --heads "$SRC_URL" "refs/heads/$branch" 2>/dev/null) \ + || die "cannot reach $SRC_URL — offline?" +[ -n "$out" ] || die "$SRC_URL has no branch '$branch' (a tag, a sha or a pull-request ref will not do)" +want=$(printf '%s\n' "$out" | head -n 1 | cut -f1) + +# --- and not the tracked branch's own head ---------------------------- +# +# The cheap half of "strictly ahead", and the only half that can be settled +# from ls-remote. ANCESTRY is checked by agent-box-source at the moment of +# the move, where the objects already are: asking here would mean fetching +# them, and a fetch is not something a refused candidate — or a mistyped +# branch name — should leave behind in the tree that decides what root +# builds. That check is also the better home for it on its own merits, since +# it then holds however the marker was written. +head=$(tracked_head) +if [ "$want" = "$head" ]; then + die "'$branch' is at ${head} — the same rev as ${SRC_BRANCH:-the tracked branch}, so there is nothing to try" +fi + +# --- record the intent and let the update do the work ----------------- +# +# The marker is intent, never state: the updater consumes it as it starts, +# so a candidate that fails to build is not retried forever, and "what is +# this box running" stays a question answered by the tree rather than by a +# file that can disagree with it. +umask 022 +# A box that has never updated has no tree and may have no state dir either, +# and the marker's parent is that dir. Create it rather than failing: the +# update this queues would create it anyway when it clones. +mkdir -p "$(dirname "$CANDIDATE_FILE")" \ + || die "could not create $(dirname "$CANDIDATE_FILE")" +# An existing `on` fact is carried over, not dropped: a box already running +# a candidate is off the tracked branch, so moving it to ANOTHER candidate +# needs the same relaxation coming home does. agent-box-source re-verifies +# the fact against the running rev before it believes it either way. +on="" +[ -e "$CANDIDATE_FILE" ] \ + && on=$(sed -n 's/^on=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) +# A `[ ... ] && printf` as the LAST command in the group would make the +# group's status that test's — 1 whenever there is no fact to carry — and +# the `|| die` below would then report a failure on a write that worked. +if [ -n "$on" ]; then + printf 'want=refs/heads/%s\non=%s\n' "$branch" "$on" > "$CANDIDATE_FILE.tmp" \ + || die "could not write $CANDIDATE_FILE.tmp" +else + printf 'want=refs/heads/%s\n' "$branch" > "$CANDIDATE_FILE.tmp" \ + || die "could not write $CANDIDATE_FILE.tmp" +fi +mv "$CANDIDATE_FILE.tmp" "$CANDIDATE_FILE" \ + || die "could not put $CANDIDATE_FILE in place" +say "candidate $branch ($(printf '%.12s' "$want")) queued — triggering an update" +say "this box will be off ${SRC_BRANCH:-the tracked branch}; the next update returns to it" +exec $UPDATE_TRIGGER diff --git a/modules/src/source-tree.sh b/modules/src/source-tree.sh index 0d9ab39b..3d777ab1 100644 --- a/modules/src/source-tree.sh +++ b/modules/src/source-tree.sh @@ -46,6 +46,13 @@ ref=${AGENT_BOX_SRC_REF:-} # (`agentbox update --force`). Non-empty means the target replaces HEAD even # when it is not a descendant. force=${AGENT_BOX_SRC_FORCE:-} +# Where agent-box-candidate leaves the branch it wants tried on this box +# before it is merged. Read by `pull` and CONSUMED there — it is intent, not +# state: what this box runs is the tree's own HEAD, and a marker that +# outlived its update would be a second answer able to disagree with it. +# Empty (no such wiring) is a box with the feature off, which behaves +# exactly as it did before it existed. +candidate_file=${AGENT_BOX_CANDIDATE_FILE:-} [ -n "$dir" ] || die "AGENT_BOX_SRC_DIR is unset — there is no tree to act on" case "$dir" in @@ -192,6 +199,56 @@ case "${1:-}" in (pull) [ -n "$rev" ] || die "AGENT_BOX_SRC_REV is unset — refusing to move a tree with no baseline" + # A queued candidate (agent-box-candidate BRANCH) is the target for + # exactly one update. Consumed BEFORE anything can fail, so a candidate + # that does not build is not retried on every later trigger — the next + # one goes back to the tracked branch, which is the whole of the + # "nothing here pins this box" promise. An explicit --rev wins: an + # operator naming a ref is not asking about the queue. + # Whether the TARGET was named by the caller, which decides below + # whether the fast-forward guard may relax itself. Recorded before the + # queue can overwrite $ref. + explicit_ref=$ref + # The candidate marker carries two fields, and the difference between + # them is the difference between a request and a fact: + # + # want=REF agent-box-candidate asks for REF on the next update. + # on=REV this box IS running the candidate at REV. + # + # `on` is checked against the rev the box actually runs before it is + # believed, and that check is what makes it safe to relax the + # fast-forward guard. "The running rev is not an ancestor of the tracked + # branch" is NOT a usable signal on its own: a squash-merged candidate + # and a rewritten upstream history look identical from here, and the + # second is the exact attack --ff-only exists to refuse. So a stale `on` + # (a rebuild that failed and rolled the tree back, an operator moving + # the tree by hand) is ignored rather than trusted, and the strict guard + # comes straight back. + candidate="" + on_rev="" + if [ -n "$candidate_file" ] && [ -e "$candidate_file" ]; then + on_rev=$(sed -n 's/^on=//p' "$candidate_file" 2>/dev/null | head -n 1) + if [ -z "$ref" ]; then + candidate=$(sed -n 's/^want=//p' "$candidate_file" 2>/dev/null | head -n 1) + fi + if [ -n "$candidate" ]; then + # Consumed BEFORE anything can fail, so a candidate that does not + # build is not reinstalled by every later trigger. What survives is + # only the `on` fact, which the next run re-verifies. + if [ -n "$on_rev" ]; then + printf 'on=%s\n' "$on_rev" > "$candidate_file.tmp" \ + && mv "$candidate_file.tmp" "$candidate_file" + else + rm -f "$candidate_file" + fi + say "installing candidate $candidate (queued on this box, not merged)" + ref=$candidate + fi + fi + if [ -n "$on_rev" ] && [ "$on_rev" != "$rev" ]; then + say "candidate marker names $on_rev but the box runs $rev — ignoring it" + on_rev="" + fi clone_if_missing target=$(target_rev) || exit 1 resolve_branch @@ -214,6 +271,36 @@ case "${1:-}" in git checkout --quiet -B "$branch" "$rev" \ || die "could not put $dir on $branch at $rev" fi + # A candidate must be strictly AHEAD of the tracked branch, and this is + # where that is settled: the fetch above already brought both objects + # in, so it costs one ancestry query, and it holds however the marker + # came to be written. It is what keeps "candidate" from becoming a way + # to replay an older, possibly vulnerable rev — the exact thing + # --ff-only refuses and the exact thing the force below would otherwise + # reopen, since a box already off the branch takes that force path. + if [ -n "$candidate" ]; then + if ! git merge-base --is-ancestor "refs/remotes/origin/$branch" \ + "$target" 2>/dev/null; then + die "refusing candidate $candidate: $target is not ahead of $branch — rebase it, so what is tested here is what will land" + fi + fi + # Coming BACK from a candidate needs the guard relaxed, and only that. + # A candidate is squash-merged, so its head is never an ancestor of the + # tracked branch: once this box has taken one, --ff-only refuses the way + # home and the box would be stuck off-branch — the exact permanent + # divergence the feature promises not to create. + # + # Narrow on purpose. It needs the marker to say this box is running a + # candidate AND that claim to match the rev it actually runs (checked + # above), so a rewritten upstream history — which looks the same from + # the ancestry side — is still refused. The target is then either the + # tracked branch's own head or a branch agent-box-candidate already + # proved is ahead of it, so neither can be a downgrade. An explicit + # --rev keeps the strict guard and still has --force of its own. + if [ -z "$force" ] && [ -z "$explicit_ref" ] && [ -n "$on_rev" ]; then + say "the box runs candidate $rev — moving to $target without the fast-forward check" + force=1 + fi if [ "$target" = "$rev" ]; then : # nothing to move to; fall through and print the rev we are on elif [ -z "$force" ] && git merge-base --is-ancestor "$target" "$rev"; then @@ -236,6 +323,17 @@ case "${1:-}" in # nothing here should be resolving conflicts in. die "refusing update: $target is not a fast-forward of the running rev $rev" fi + # Record what the box is now running, so the NEXT plain update knows it + # has a way home to relax the guard for — and stop recording it once it + # is home, so nothing is left pinned or forced afterwards. + if [ -n "$candidate_file" ]; then + if [ -n "$candidate" ]; then + printf 'on=%s\n' "$(git rev-parse HEAD)" > "$candidate_file.tmp" \ + && mv "$candidate_file.tmp" "$candidate_file" + elif [ -n "$on_rev" ]; then + rm -f "$candidate_file" + fi + fi git rev-parse HEAD ;; diff --git a/nix/runtime.nix b/nix/runtime.nix index f75e6ee7..25634381 100644 --- a/nix/runtime.nix +++ b/nix/runtime.nix @@ -339,6 +339,10 @@ let # crosses sudo, so its paths must not come from env); the template it # renders from lives here. install -m444 ${src}/password-helper.py $out/libexec/agent-box/ + # agent-box-candidate, same story: `agentbox apply` renders it per box + # with the repo, tree and trigger compiled in, because it crosses sudo + # and sudo carries no environment. The module embeds this same file. + install -m444 ${src}/candidate.sh $out/libexec/agent-box/ ''; in pkgs.buildEnv { diff --git a/scripts/check_backend_parity.py b/scripts/check_backend_parity.py index 29f3ee29..782a2107 100644 --- a/scripts/check_backend_parity.py +++ b/scripts/check_backend_parity.py @@ -214,6 +214,11 @@ "AGENT_BOX_SRC_DIR": ("module", SRC_TREE_BINDING), "AGENT_BOX_SRC_URL": ("module", SRC_TREE_BINDING), "AGENT_BOX_SRC_REV": ("module", SRC_TREE_BINDING), + # The candidate marker, consumed by that same payload and bound the same + # way for the same reason. Both backends DO supply it — native in + # source_tree()'s in-process env, beside the AGENT_BOX_SRC_* names above + # — so this is a unit-line divergence and not a missing capability. + "AGENT_BOX_CANDIDATE_FILE": ("module", SRC_TREE_BINDING), } # One reason, cited by the four entries below, so it has one home rather diff --git a/scripts/check_one_spec.py b/scripts/check_one_spec.py index a086a973..7c6b23a0 100644 --- a/scripts/check_one_spec.py +++ b/scripts/check_one_spec.py @@ -61,6 +61,14 @@ (re.compile(r"(/nix/store/e+-agent-box-password-(\w+)/bin/|" r"/etc/agent-box/bin/)agent-box-password-"), "agent-box-password-"), + # agent-box-candidate, same story: the module grants its store script + # (writeShellScript, so a file and not a dir with bin/), native grants + # the copy `agentbox apply` renders into /etc. Both are the SAME grant + # of the same one-argument wrapper — a substrate spelling, not a + # difference in what the box allows. + (re.compile(r"/nix/store/e+-agent-box-candidate|" + r"/etc/agent-box/bin/agent-box-candidate"), + "agent-box-candidate"), ] # Divergences that are CORRECT. Each needs a reason a reader can check. diff --git a/tests/golden/web/etc/agent-box-guides/AGENTS.agent.md b/tests/golden/web/etc/agent-box-guides/AGENTS.agent.md index ae3fe6ae..59726bb8 100644 --- a/tests/golden/web/etc/agent-box-guides/AGENTS.agent.md +++ b/tests/golden/web/etc/agent-box-guides/AGENTS.agent.md @@ -473,6 +473,33 @@ FIRST and the rebuild follows it, so between the two - and after a rebuild that failed and could not put the tree back, which the wall notice says - it reads ahead of the running system. +## Trying YOUR fix on this box, before every other box takes it + +An update follows the tracked branch, so a fix used to have to be +merged before this box could run it - which made the whole fleet the +canary for its own tooling. Push the branch and try it here first: + + sudo /nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-agent-box-candidate BRANCH # install it, then re-apply + sudo /nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-agent-box-candidate --status # what is this box running? + sudo /nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-agent-box-candidate --reset # go back to the tracked branch + +Same restart as an ordinary update, so save context first. Then merge +once you have SEEN it work, and the fleet takes a change that has run +somewhere. + +Three things it refuses, all deliberately. Only a BRANCH on this +box's own origin - not a tag, not a sha, and not `refs/pull/N/head`, +because a public repo takes pull requests from anyone and that would +be a stranger's unreviewed code built as root. Only a branch strictly +AHEAD of the tracked branch, so "candidate" can never become a way to +replay an older rev - rebase yours, which is also the only way what +you test here is what will land. And it never pins this box: the next +update returns to the tracked branch and says so, so a candidate you +forget converges back on the fleet instead of drifting from it. That +return needs no flag from you, and is the one case where the +fast-forward guard steps aside - a squash-merged branch is never an +ancestor of the branch it landed on, so nothing else could get home. + ## This box ships its own sources A checkout of the repo this box is BUILT from lives at diff --git a/tests/golden/web/etc/sudoers b/tests/golden/web/etc/sudoers index 86c4bceb..8c67f477 100644 --- a/tests/golden/web/etc/sudoers +++ b/tests/golden/web/etc/sudoers @@ -5,6 +5,7 @@ root ALL=(ALL:ALL) SETENV: ALL %wheel ALL=(ALL:ALL) SETENV: ALL agent ALL=(ALL:ALL) NOPASSWD: /run/current-system/sw/bin/systemctl reload caddy.service, NOPASSWD: /run/current-system/sw/bin/systemctl reload caddy.service, NOPASSWD: /run/current-system/sw/bin/systemctl start --no-block agent-box-update.service robot ALL=(ALL:ALL) NOPASSWD: /run/current-system/sw/bin/systemctl reload caddy.service, NOPASSWD: /run/current-system/sw/bin/systemctl reload caddy.service, NOPASSWD: /run/current-system/sw/bin/systemctl start --no-block agent-box-update.service +agent ALL=(ALL:ALL) NOPASSWD: /nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-agent-box-candidate * agent ALL=(ALL:ALL) NOPASSWD: /nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-agent-box-password-agent/bin/agent-box-password-agent "" robot ALL=(ALL:ALL) NOPASSWD: /nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-agent-box-password-robot/bin/agent-box-password-robot "" agent ALL=(ALL:ALL) NOPASSWD: /run/current-system/sw/bin/systemctl reboot --no-block diff --git a/tests/golden/web/payloads/agent-box-candidate b/tests/golden/web/payloads/agent-box-candidate new file mode 100644 index 00000000..4c2b4be8 --- /dev/null +++ b/tests/golden/web/payloads/agent-box-candidate @@ -0,0 +1,282 @@ +#!/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-bash-5.3p9/bin/bash +# Box constants, prepended by the module. +CANDIDATE_FILE=/var/lib/agent-box/src.candidate +SRC_DIR=/var/lib/agent-box/src +SRC_URL=https://github.com/defangdevs/agent-box.git +SRC_BRANCH='' +SYSTEMCTL=/run/current-system/sw/bin/systemctl +UPDATE_TRIGGER='/run/current-system/sw/bin/systemctl start --no-block agent-box-update.service' +PATH=/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-git-2.54.0/bin:/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-coreutils-9.11/bin:/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-gnused-4.10/bin:/nix/store/eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee-systemd-261/bin:$PATH +export PATH + +# agent-box-candidate — install a candidate release on THIS box, before it +# is merged and every other box gets it. +# +# The box's own fix used to have to land on master first. The only +# root-capable path an agent has is the update trigger, whose unit follows +# the tracked branch, and `git merge --ff-only` refuses everything else by +# construction — so "test it here, then merge" was not expressible and the +# fleet was the canary for its own tooling. This is the missing verb. +# +# It is a ROOT wrapper for the same reason the password helper is one: what +# writes the source tree decides what root builds, so the tree stays +# root-owned (issue #242) and an agent reaches it only through a command +# that can refuse. Every constant below is baked in by the renderer rather +# than read from the environment, because sudo scrubs the environment +# (env_reset) and a wrapper that took its target repo from the caller would +# hand root an arbitrary URL to build from. +# +# Three things it will not do, which is most of the point: +# +# * Anything but a BRANCH on this box's own configured origin. Not a +# tag, not a sha, and above all not refs/pull/N/head: on a public repo +# anyone can open a pull request, and a fetchable PR ref would mean a +# stranger's unreviewed code built as root. A branch needs push access +# to the source repo, and whoever has that can already merge. +# * A candidate that is not strictly ahead of the tracked branch. That is +# what stops an old, possibly vulnerable rev being replayed as a +# "candidate", which is the one attack the fast-forward guard exists to +# refuse and which the force this feature needs would otherwise reopen. +# The rev-equality half is here; ANCESTRY is agent-box-source's, at the +# moment of the move, because that is where the objects are and a +# question must not fetch into the tree to answer itself. +# * A permanent divergence. Nothing here pins the box: the marker it +# writes is consumed by the very next update, and an update with no +# marker always heads for the tracked branch (forcing only when the box +# is off it, since a squash-merged candidate is never an ancestor of +# master). So a candidate survives exactly until the next update, and a +# box that is forgotten converges on the fleet instead of drifting from +# it. +# +# Usage, all idempotent: +# +# agent-box-candidate BRANCH install that branch and re-apply +# agent-box-candidate --reset go back to the tracked branch now +# agent-box-candidate --status what the box is running, and off what +# +# The renderer prepends: CANDIDATE_FILE, SRC_DIR, SRC_URL, SRC_BRANCH, +# UPDATE_TRIGGER, SYSTEMCTL. +set -u + +prog=agent-box-candidate +say() { printf '%s: %s\n' "$prog" "$*" >&2; } +die() { say "$@"; exit 1; } + +# Same hardening as agent-box-source, and for the same reason: every git +# below runs as root against a tree fetched from the network, and a +# credential prompt has nobody to answer it. +export GIT_TERMINAL_PROMPT=0 +unset GIT_ASKPASS SSH_ASKPASS +export GIT_HTTP_LOW_SPEED_LIMIT=1000 GIT_HTTP_LOW_SPEED_TIME=60 +export GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.hooksPath GIT_CONFIG_VALUE_0=/dev/null + +usage() { + cat >&2 </dev/null) \ + || die "cannot reach $SRC_URL — offline?" + [ -n "$out" ] || die "origin has no branch '$b'" + else + out=$(command git ls-remote "$SRC_URL" HEAD 2>/dev/null) \ + || die "cannot reach $SRC_URL — offline?" + [ -n "$out" ] || die "$SRC_URL has no HEAD" + fi + printf '%s\n' "$out" | head -n 1 | cut -f1 +} + +running_rev() { + # The tree names the rev the box runs (the guide says so, so it has to be + # true here too). A box that has never updated has no tree yet, which is + # not an error for --status. + [ -e "$SRC_DIR/.git" ] || return 1 + command git -C "$SRC_DIR" rev-parse --verify --quiet HEAD 2>/dev/null +} + +case "${1:-}" in + (-h|--help) + usage + exit 0 + ;; + + (--status) + rev=$(running_rev) || die "$SRC_DIR is not a checkout yet — this box has never updated" + head=$(tracked_head) + printf 'rev: %s\n' "$rev" + printf 'tracked: %s at %s\n' "${SRC_BRANCH:-origin/HEAD}" "$head" + # The marker's own claim, believed only when it matches the rev the box + # actually runs — the same check agent-box-source makes before it + # relaxes the fast-forward guard, and for the same reason: a rebuild + # that failed and rolled back leaves a claim that is no longer true. + on="" + [ -e "$CANDIDATE_FILE" ] \ + && on=$(sed -n 's/^on=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + if [ -n "$on" ] && [ "$on" = "$rev" ]; then + printf 'state: CANDIDATE — the next update returns to %s\n' \ + "${SRC_BRANCH:-the tracked branch}" + elif [ "$rev" = "$head" ]; then + printf 'state: on the tracked branch\n' + elif command git -C "$SRC_DIR" merge-base --is-ancestor "$rev" "$head" 2>/dev/null; then + printf 'state: behind the tracked branch — an update is available\n' + else + # Off the branch with nothing claiming a candidate: a rewritten + # upstream history, or a tree somebody moved by hand. NOT reported as + # a candidate, because agent-box-source will not force out of it + # either — it is the case --ff-only exists to refuse. + printf 'state: off %s, and not as a candidate — the fast-forward guard applies\n' \ + "${SRC_BRANCH:-the tracked branch}" + fi + want="" + [ -e "$CANDIDATE_FILE" ] \ + && want=$(sed -n 's/^want=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + [ -n "$want" ] && printf 'pending: %s\n' "$want" + exit 0 + ;; + + (--reset) + # An update with no QUEUED candidate already heads for the tracked + # branch, so "reset" is the ordinary update — the only work here is + # dropping a `want` that has not been consumed yet. The `on` fact must + # survive: it is what tells agent-box-source this box has a way home to + # relax the guard for, and deleting it would strand the box exactly + # where the reset was meant to rescue it from. + if [ -e "$CANDIDATE_FILE" ]; then + on=$(sed -n 's/^on=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + if [ -n "$on" ]; then + printf 'on=%s\n' "$on" > "$CANDIDATE_FILE.tmp" \ + && mv "$CANDIDATE_FILE.tmp" "$CANDIDATE_FILE" + else + rm -f "$CANDIDATE_FILE" + fi + fi + say "returning to ${SRC_BRANCH:-the tracked branch} — triggering an update" + exec $UPDATE_TRIGGER + ;; + + ("") + usage + exit 1 + ;; + + (-*) + say "unknown option '$1'" + usage + exit 1 + ;; +esac + +branch=$1 +shift +[ $# -eq 0 ] || die "one branch at a time (got extra: $*)" + +# --- validate the name ------------------------------------------------ +# +# The name reaches git and reaches a file, so it is checked before either. +# Deliberately narrower than git's own refname rules: this is an allowlist +# of what a branch in this repo actually looks like, not an attempt to +# re-implement git-check-ref-format. +case $branch in + (refs/heads/*) + die "name the branch itself: '${branch#refs/heads/}' rather than '$branch'" + ;; + (refs/*) + # refs/pull/N/head lands here, which is the whole reason this arm is + # separate: there is no branch to suggest, and suggesting one by + # stripping refs/heads/ off a path that has no such prefix printed the + # input back as its own correction. + die "'$branch' is a ref path, not a branch — only a branch of $SRC_URL can be installed" + ;; + (*..*) + die "'$branch' contains '..'" + ;; + (/*|*/) + die "'$branch' starts or ends with '/'" + ;; + (*//*) + die "'$branch' contains '//'" + ;; + (*[!A-Za-z0-9._/-]*) + die "'$branch' has characters outside A-Za-z0-9._/- — refusing" + ;; + ([!A-Za-z0-9]*) + die "'$branch' does not start with a letter or digit" + ;; +esac +[ ${#branch} -le 200 ] || die "branch name is ${#branch} characters — refusing" + +# --- it must be a branch on OUR origin -------------------------------- +# +# ls-remote --heads, so only refs/heads/* can match. This is the line that +# keeps refs/pull/N/head — a stranger's unreviewed pull request on a public +# repo — from ever being installed as root. +out=$(command git ls-remote --heads "$SRC_URL" "refs/heads/$branch" 2>/dev/null) \ + || die "cannot reach $SRC_URL — offline?" +[ -n "$out" ] || die "$SRC_URL has no branch '$branch' (a tag, a sha or a pull-request ref will not do)" +want=$(printf '%s\n' "$out" | head -n 1 | cut -f1) + +# --- and not the tracked branch's own head ---------------------------- +# +# The cheap half of "strictly ahead", and the only half that can be settled +# from ls-remote. ANCESTRY is checked by agent-box-source at the moment of +# the move, where the objects already are: asking here would mean fetching +# them, and a fetch is not something a refused candidate — or a mistyped +# branch name — should leave behind in the tree that decides what root +# builds. That check is also the better home for it on its own merits, since +# it then holds however the marker was written. +head=$(tracked_head) +if [ "$want" = "$head" ]; then + die "'$branch' is at ${head} — the same rev as ${SRC_BRANCH:-the tracked branch}, so there is nothing to try" +fi + +# --- record the intent and let the update do the work ----------------- +# +# The marker is intent, never state: the updater consumes it as it starts, +# so a candidate that fails to build is not retried forever, and "what is +# this box running" stays a question answered by the tree rather than by a +# file that can disagree with it. +umask 022 +# A box that has never updated has no tree and may have no state dir either, +# and the marker's parent is that dir. Create it rather than failing: the +# update this queues would create it anyway when it clones. +mkdir -p "$(dirname "$CANDIDATE_FILE")" \ + || die "could not create $(dirname "$CANDIDATE_FILE")" +# An existing `on` fact is carried over, not dropped: a box already running +# a candidate is off the tracked branch, so moving it to ANOTHER candidate +# needs the same relaxation coming home does. agent-box-source re-verifies +# the fact against the running rev before it believes it either way. +on="" +[ -e "$CANDIDATE_FILE" ] \ + && on=$(sed -n 's/^on=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) +# A `[ ... ] && printf` as the LAST command in the group would make the +# group's status that test's — 1 whenever there is no fact to carry — and +# the `|| die` below would then report a failure on a write that worked. +if [ -n "$on" ]; then + printf 'want=refs/heads/%s\non=%s\n' "$branch" "$on" > "$CANDIDATE_FILE.tmp" \ + || die "could not write $CANDIDATE_FILE.tmp" +else + printf 'want=refs/heads/%s\n' "$branch" > "$CANDIDATE_FILE.tmp" \ + || die "could not write $CANDIDATE_FILE.tmp" +fi +mv "$CANDIDATE_FILE.tmp" "$CANDIDATE_FILE" \ + || die "could not put $CANDIDATE_FILE in place" +say "candidate $branch ($(printf '%.12s' "$want")) queued — triggering an update" +say "this box will be off ${SRC_BRANCH:-the tracked branch}; the next update returns to it" +exec $UPDATE_TRIGGER + diff --git a/tests/golden/web/payloads/agent-box-source/bin/agent-box-source b/tests/golden/web/payloads/agent-box-source/bin/agent-box-source index 7e84cf26..70db3c24 100644 --- a/tests/golden/web/payloads/agent-box-source/bin/agent-box-source +++ b/tests/golden/web/payloads/agent-box-source/bin/agent-box-source @@ -47,6 +47,13 @@ ref=${AGENT_BOX_SRC_REF:-} # (`agentbox update --force`). Non-empty means the target replaces HEAD even # when it is not a descendant. force=${AGENT_BOX_SRC_FORCE:-} +# Where agent-box-candidate leaves the branch it wants tried on this box +# before it is merged. Read by `pull` and CONSUMED there — it is intent, not +# state: what this box runs is the tree's own HEAD, and a marker that +# outlived its update would be a second answer able to disagree with it. +# Empty (no such wiring) is a box with the feature off, which behaves +# exactly as it did before it existed. +candidate_file=${AGENT_BOX_CANDIDATE_FILE:-} [ -n "$dir" ] || die "AGENT_BOX_SRC_DIR is unset — there is no tree to act on" case "$dir" in @@ -193,6 +200,56 @@ case "${1:-}" in (pull) [ -n "$rev" ] || die "AGENT_BOX_SRC_REV is unset — refusing to move a tree with no baseline" + # A queued candidate (agent-box-candidate BRANCH) is the target for + # exactly one update. Consumed BEFORE anything can fail, so a candidate + # that does not build is not retried on every later trigger — the next + # one goes back to the tracked branch, which is the whole of the + # "nothing here pins this box" promise. An explicit --rev wins: an + # operator naming a ref is not asking about the queue. + # Whether the TARGET was named by the caller, which decides below + # whether the fast-forward guard may relax itself. Recorded before the + # queue can overwrite $ref. + explicit_ref=$ref + # The candidate marker carries two fields, and the difference between + # them is the difference between a request and a fact: + # + # want=REF agent-box-candidate asks for REF on the next update. + # on=REV this box IS running the candidate at REV. + # + # `on` is checked against the rev the box actually runs before it is + # believed, and that check is what makes it safe to relax the + # fast-forward guard. "The running rev is not an ancestor of the tracked + # branch" is NOT a usable signal on its own: a squash-merged candidate + # and a rewritten upstream history look identical from here, and the + # second is the exact attack --ff-only exists to refuse. So a stale `on` + # (a rebuild that failed and rolled the tree back, an operator moving + # the tree by hand) is ignored rather than trusted, and the strict guard + # comes straight back. + candidate="" + on_rev="" + if [ -n "$candidate_file" ] && [ -e "$candidate_file" ]; then + on_rev=$(sed -n 's/^on=//p' "$candidate_file" 2>/dev/null | head -n 1) + if [ -z "$ref" ]; then + candidate=$(sed -n 's/^want=//p' "$candidate_file" 2>/dev/null | head -n 1) + fi + if [ -n "$candidate" ]; then + # Consumed BEFORE anything can fail, so a candidate that does not + # build is not reinstalled by every later trigger. What survives is + # only the `on` fact, which the next run re-verifies. + if [ -n "$on_rev" ]; then + printf 'on=%s\n' "$on_rev" > "$candidate_file.tmp" \ + && mv "$candidate_file.tmp" "$candidate_file" + else + rm -f "$candidate_file" + fi + say "installing candidate $candidate (queued on this box, not merged)" + ref=$candidate + fi + fi + if [ -n "$on_rev" ] && [ "$on_rev" != "$rev" ]; then + say "candidate marker names $on_rev but the box runs $rev — ignoring it" + on_rev="" + fi clone_if_missing target=$(target_rev) || exit 1 resolve_branch @@ -215,6 +272,36 @@ case "${1:-}" in git checkout --quiet -B "$branch" "$rev" \ || die "could not put $dir on $branch at $rev" fi + # A candidate must be strictly AHEAD of the tracked branch, and this is + # where that is settled: the fetch above already brought both objects + # in, so it costs one ancestry query, and it holds however the marker + # came to be written. It is what keeps "candidate" from becoming a way + # to replay an older, possibly vulnerable rev — the exact thing + # --ff-only refuses and the exact thing the force below would otherwise + # reopen, since a box already off the branch takes that force path. + if [ -n "$candidate" ]; then + if ! git merge-base --is-ancestor "refs/remotes/origin/$branch" \ + "$target" 2>/dev/null; then + die "refusing candidate $candidate: $target is not ahead of $branch — rebase it, so what is tested here is what will land" + fi + fi + # Coming BACK from a candidate needs the guard relaxed, and only that. + # A candidate is squash-merged, so its head is never an ancestor of the + # tracked branch: once this box has taken one, --ff-only refuses the way + # home and the box would be stuck off-branch — the exact permanent + # divergence the feature promises not to create. + # + # Narrow on purpose. It needs the marker to say this box is running a + # candidate AND that claim to match the rev it actually runs (checked + # above), so a rewritten upstream history — which looks the same from + # the ancestry side — is still refused. The target is then either the + # tracked branch's own head or a branch agent-box-candidate already + # proved is ahead of it, so neither can be a downgrade. An explicit + # --rev keeps the strict guard and still has --force of its own. + if [ -z "$force" ] && [ -z "$explicit_ref" ] && [ -n "$on_rev" ]; then + say "the box runs candidate $rev — moving to $target without the fast-forward check" + force=1 + fi if [ "$target" = "$rev" ]; then : # nothing to move to; fall through and print the rev we are on elif [ -z "$force" ] && git merge-base --is-ancestor "$target" "$rev"; then @@ -237,6 +324,17 @@ case "${1:-}" in # nothing here should be resolving conflicts in. die "refusing update: $target is not a fast-forward of the running rev $rev" fi + # Record what the box is now running, so the NEXT plain update knows it + # has a way home to relax the guard for — and stop recording it once it + # is home, so nothing is left pinned or forced afterwards. + if [ -n "$candidate_file" ]; then + if [ -n "$candidate" ]; then + printf 'on=%s\n' "$(git rev-parse HEAD)" > "$candidate_file.tmp" \ + && mv "$candidate_file.tmp" "$candidate_file" + elif [ -n "$on_rev" ]; then + rm -f "$candidate_file" + fi + fi git rev-parse HEAD ;; diff --git a/tests/golden/web/units/agent-box-update.service b/tests/golden/web/units/agent-box-update.service index 0d914b0a..b683d510 100644 --- a/tests/golden/web/units/agent-box-update.service +++ b/tests/golden/web/units/agent-box-update.service @@ -2,6 +2,7 @@ Description=Fast-forward agent-box to upstream HEAD and rebuild [Service] +Environment="AGENT_BOX_CANDIDATE_FILE=/var/lib/agent-box/src.candidate" Environment="AGENT_BOX_SRC_DIR=/var/lib/agent-box/src" Environment="AGENT_BOX_SRC_REV=0000000000000000000000000000000000000000" Environment="AGENT_BOX_SRC_URL=https://github.com/defangdevs/agent-box.git" diff --git a/tests/native/expected-modes.json b/tests/native/expected-modes.json index 90c88069..d15d478d 100644 --- a/tests/native/expected-modes.json +++ b/tests/native/expected-modes.json @@ -2,6 +2,7 @@ "etc/agent-box-guides/AGENTS.agent.md": "0o444", "etc/agent-box-guides/AGENTS.robot.md": "0o444", "etc/agent-box/Caddyfile": "0o644", + "etc/agent-box/bin/agent-box-candidate": "0o755", "etc/agent-box/bin/agent-box-password-agent": "0o755", "etc/agent-box/bin/agent-box-password-robot": "0o755", "etc/agent-box/bin/agent-box-webhook-spawn": "0o755", diff --git a/tests/native/expected/etc/agent-box/bin/agent-box-candidate b/tests/native/expected/etc/agent-box/bin/agent-box-candidate new file mode 100755 index 00000000..8e32dd72 --- /dev/null +++ b/tests/native/expected/etc/agent-box/bin/agent-box-candidate @@ -0,0 +1,279 @@ +#!/bin/sh +# Box constants, prepended by `agentbox apply`. +CANDIDATE_FILE=/var/lib/agent-box/candidate +SRC_DIR=/var/lib/agent-box/src +SRC_URL=https://github.com/defangdevs/agent-box.git +SRC_BRANCH='' +SYSTEMCTL=/usr/bin/systemctl +UPDATE_TRIGGER='/usr/bin/systemctl start --no-block agent-box-update.service' + +# agent-box-candidate — install a candidate release on THIS box, before it +# is merged and every other box gets it. +# +# The box's own fix used to have to land on master first. The only +# root-capable path an agent has is the update trigger, whose unit follows +# the tracked branch, and `git merge --ff-only` refuses everything else by +# construction — so "test it here, then merge" was not expressible and the +# fleet was the canary for its own tooling. This is the missing verb. +# +# It is a ROOT wrapper for the same reason the password helper is one: what +# writes the source tree decides what root builds, so the tree stays +# root-owned (issue #242) and an agent reaches it only through a command +# that can refuse. Every constant below is baked in by the renderer rather +# than read from the environment, because sudo scrubs the environment +# (env_reset) and a wrapper that took its target repo from the caller would +# hand root an arbitrary URL to build from. +# +# Three things it will not do, which is most of the point: +# +# * Anything but a BRANCH on this box's own configured origin. Not a +# tag, not a sha, and above all not refs/pull/N/head: on a public repo +# anyone can open a pull request, and a fetchable PR ref would mean a +# stranger's unreviewed code built as root. A branch needs push access +# to the source repo, and whoever has that can already merge. +# * A candidate that is not strictly ahead of the tracked branch. That is +# what stops an old, possibly vulnerable rev being replayed as a +# "candidate", which is the one attack the fast-forward guard exists to +# refuse and which the force this feature needs would otherwise reopen. +# The rev-equality half is here; ANCESTRY is agent-box-source's, at the +# moment of the move, because that is where the objects are and a +# question must not fetch into the tree to answer itself. +# * A permanent divergence. Nothing here pins the box: the marker it +# writes is consumed by the very next update, and an update with no +# marker always heads for the tracked branch (forcing only when the box +# is off it, since a squash-merged candidate is never an ancestor of +# master). So a candidate survives exactly until the next update, and a +# box that is forgotten converges on the fleet instead of drifting from +# it. +# +# Usage, all idempotent: +# +# agent-box-candidate BRANCH install that branch and re-apply +# agent-box-candidate --reset go back to the tracked branch now +# agent-box-candidate --status what the box is running, and off what +# +# The renderer prepends: CANDIDATE_FILE, SRC_DIR, SRC_URL, SRC_BRANCH, +# UPDATE_TRIGGER, SYSTEMCTL. +set -u + +prog=agent-box-candidate +say() { printf '%s: %s\n' "$prog" "$*" >&2; } +die() { say "$@"; exit 1; } + +# Same hardening as agent-box-source, and for the same reason: every git +# below runs as root against a tree fetched from the network, and a +# credential prompt has nobody to answer it. +export GIT_TERMINAL_PROMPT=0 +unset GIT_ASKPASS SSH_ASKPASS +export GIT_HTTP_LOW_SPEED_LIMIT=1000 GIT_HTTP_LOW_SPEED_TIME=60 +export GIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.hooksPath GIT_CONFIG_VALUE_0=/dev/null + +usage() { + cat >&2 </dev/null) \ + || die "cannot reach $SRC_URL — offline?" + [ -n "$out" ] || die "origin has no branch '$b'" + else + out=$(command git ls-remote "$SRC_URL" HEAD 2>/dev/null) \ + || die "cannot reach $SRC_URL — offline?" + [ -n "$out" ] || die "$SRC_URL has no HEAD" + fi + printf '%s\n' "$out" | head -n 1 | cut -f1 +} + +running_rev() { + # The tree names the rev the box runs (the guide says so, so it has to be + # true here too). A box that has never updated has no tree yet, which is + # not an error for --status. + [ -e "$SRC_DIR/.git" ] || return 1 + command git -C "$SRC_DIR" rev-parse --verify --quiet HEAD 2>/dev/null +} + +case "${1:-}" in + (-h|--help) + usage + exit 0 + ;; + + (--status) + rev=$(running_rev) || die "$SRC_DIR is not a checkout yet — this box has never updated" + head=$(tracked_head) + printf 'rev: %s\n' "$rev" + printf 'tracked: %s at %s\n' "${SRC_BRANCH:-origin/HEAD}" "$head" + # The marker's own claim, believed only when it matches the rev the box + # actually runs — the same check agent-box-source makes before it + # relaxes the fast-forward guard, and for the same reason: a rebuild + # that failed and rolled back leaves a claim that is no longer true. + on="" + [ -e "$CANDIDATE_FILE" ] \ + && on=$(sed -n 's/^on=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + if [ -n "$on" ] && [ "$on" = "$rev" ]; then + printf 'state: CANDIDATE — the next update returns to %s\n' \ + "${SRC_BRANCH:-the tracked branch}" + elif [ "$rev" = "$head" ]; then + printf 'state: on the tracked branch\n' + elif command git -C "$SRC_DIR" merge-base --is-ancestor "$rev" "$head" 2>/dev/null; then + printf 'state: behind the tracked branch — an update is available\n' + else + # Off the branch with nothing claiming a candidate: a rewritten + # upstream history, or a tree somebody moved by hand. NOT reported as + # a candidate, because agent-box-source will not force out of it + # either — it is the case --ff-only exists to refuse. + printf 'state: off %s, and not as a candidate — the fast-forward guard applies\n' \ + "${SRC_BRANCH:-the tracked branch}" + fi + want="" + [ -e "$CANDIDATE_FILE" ] \ + && want=$(sed -n 's/^want=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + [ -n "$want" ] && printf 'pending: %s\n' "$want" + exit 0 + ;; + + (--reset) + # An update with no QUEUED candidate already heads for the tracked + # branch, so "reset" is the ordinary update — the only work here is + # dropping a `want` that has not been consumed yet. The `on` fact must + # survive: it is what tells agent-box-source this box has a way home to + # relax the guard for, and deleting it would strand the box exactly + # where the reset was meant to rescue it from. + if [ -e "$CANDIDATE_FILE" ]; then + on=$(sed -n 's/^on=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) + if [ -n "$on" ]; then + printf 'on=%s\n' "$on" > "$CANDIDATE_FILE.tmp" \ + && mv "$CANDIDATE_FILE.tmp" "$CANDIDATE_FILE" + else + rm -f "$CANDIDATE_FILE" + fi + fi + say "returning to ${SRC_BRANCH:-the tracked branch} — triggering an update" + exec $UPDATE_TRIGGER + ;; + + ("") + usage + exit 1 + ;; + + (-*) + say "unknown option '$1'" + usage + exit 1 + ;; +esac + +branch=$1 +shift +[ $# -eq 0 ] || die "one branch at a time (got extra: $*)" + +# --- validate the name ------------------------------------------------ +# +# The name reaches git and reaches a file, so it is checked before either. +# Deliberately narrower than git's own refname rules: this is an allowlist +# of what a branch in this repo actually looks like, not an attempt to +# re-implement git-check-ref-format. +case $branch in + (refs/heads/*) + die "name the branch itself: '${branch#refs/heads/}' rather than '$branch'" + ;; + (refs/*) + # refs/pull/N/head lands here, which is the whole reason this arm is + # separate: there is no branch to suggest, and suggesting one by + # stripping refs/heads/ off a path that has no such prefix printed the + # input back as its own correction. + die "'$branch' is a ref path, not a branch — only a branch of $SRC_URL can be installed" + ;; + (*..*) + die "'$branch' contains '..'" + ;; + (/*|*/) + die "'$branch' starts or ends with '/'" + ;; + (*//*) + die "'$branch' contains '//'" + ;; + (*[!A-Za-z0-9._/-]*) + die "'$branch' has characters outside A-Za-z0-9._/- — refusing" + ;; + ([!A-Za-z0-9]*) + die "'$branch' does not start with a letter or digit" + ;; +esac +[ ${#branch} -le 200 ] || die "branch name is ${#branch} characters — refusing" + +# --- it must be a branch on OUR origin -------------------------------- +# +# ls-remote --heads, so only refs/heads/* can match. This is the line that +# keeps refs/pull/N/head — a stranger's unreviewed pull request on a public +# repo — from ever being installed as root. +out=$(command git ls-remote --heads "$SRC_URL" "refs/heads/$branch" 2>/dev/null) \ + || die "cannot reach $SRC_URL — offline?" +[ -n "$out" ] || die "$SRC_URL has no branch '$branch' (a tag, a sha or a pull-request ref will not do)" +want=$(printf '%s\n' "$out" | head -n 1 | cut -f1) + +# --- and not the tracked branch's own head ---------------------------- +# +# The cheap half of "strictly ahead", and the only half that can be settled +# from ls-remote. ANCESTRY is checked by agent-box-source at the moment of +# the move, where the objects already are: asking here would mean fetching +# them, and a fetch is not something a refused candidate — or a mistyped +# branch name — should leave behind in the tree that decides what root +# builds. That check is also the better home for it on its own merits, since +# it then holds however the marker was written. +head=$(tracked_head) +if [ "$want" = "$head" ]; then + die "'$branch' is at ${head} — the same rev as ${SRC_BRANCH:-the tracked branch}, so there is nothing to try" +fi + +# --- record the intent and let the update do the work ----------------- +# +# The marker is intent, never state: the updater consumes it as it starts, +# so a candidate that fails to build is not retried forever, and "what is +# this box running" stays a question answered by the tree rather than by a +# file that can disagree with it. +umask 022 +# A box that has never updated has no tree and may have no state dir either, +# and the marker's parent is that dir. Create it rather than failing: the +# update this queues would create it anyway when it clones. +mkdir -p "$(dirname "$CANDIDATE_FILE")" \ + || die "could not create $(dirname "$CANDIDATE_FILE")" +# An existing `on` fact is carried over, not dropped: a box already running +# a candidate is off the tracked branch, so moving it to ANOTHER candidate +# needs the same relaxation coming home does. agent-box-source re-verifies +# the fact against the running rev before it believes it either way. +on="" +[ -e "$CANDIDATE_FILE" ] \ + && on=$(sed -n 's/^on=//p' "$CANDIDATE_FILE" 2>/dev/null | head -n 1) +# A `[ ... ] && printf` as the LAST command in the group would make the +# group's status that test's — 1 whenever there is no fact to carry — and +# the `|| die` below would then report a failure on a write that worked. +if [ -n "$on" ]; then + printf 'want=refs/heads/%s\non=%s\n' "$branch" "$on" > "$CANDIDATE_FILE.tmp" \ + || die "could not write $CANDIDATE_FILE.tmp" +else + printf 'want=refs/heads/%s\n' "$branch" > "$CANDIDATE_FILE.tmp" \ + || die "could not write $CANDIDATE_FILE.tmp" +fi +mv "$CANDIDATE_FILE.tmp" "$CANDIDATE_FILE" \ + || die "could not put $CANDIDATE_FILE in place" +say "candidate $branch ($(printf '%.12s' "$want")) queued — triggering an update" +say "this box will be off ${SRC_BRANCH:-the tracked branch}; the next update returns to it" +exec $UPDATE_TRIGGER diff --git a/tests/native/expected/etc/sudoers.d/agent-box b/tests/native/expected/etc/sudoers.d/agent-box index 677b78f0..ebbbd69f 100644 --- a/tests/native/expected/etc/sudoers.d/agent-box +++ b/tests/native/expected/etc/sudoers.d/agent-box @@ -1,3 +1,3 @@ # Generated by `agentbox apply` - do not edit. -agent ALL=(root) NOPASSWD: /etc/agent-box/bin/agent-box-password-agent "", /usr/bin/systemctl reload caddy.service, /usr/bin/systemctl start --no-block agent-box-update.service, /usr/bin/systemctl reboot --no-block, /run/current-system/sw/bin/systemctl reload caddy.service +agent ALL=(root) NOPASSWD: /etc/agent-box/bin/agent-box-password-agent "", /usr/bin/systemctl reload caddy.service, /usr/bin/systemctl start --no-block agent-box-update.service, /usr/bin/systemctl reboot --no-block, /etc/agent-box/bin/agent-box-candidate *, /run/current-system/sw/bin/systemctl reload caddy.service robot ALL=(root) NOPASSWD: /etc/agent-box/bin/agent-box-password-robot "", /usr/bin/systemctl reload caddy.service, /usr/bin/systemctl start --no-block agent-box-update.service, /run/current-system/sw/bin/systemctl reload caddy.service diff --git a/tests/test-candidate.sh b/tests/test-candidate.sh new file mode 100644 index 00000000..60154dfb --- /dev/null +++ b/tests/test-candidate.sh @@ -0,0 +1,267 @@ +#!/usr/bin/env bash +# Unit tests for `agent-box-candidate` — installing a fix on THIS box before +# it is merged and every other box takes it. +# +# Weighted almost entirely at the REFUSALS, for the same reason +# test-source-tree.sh is: this runs as root, it decides what the next +# rebuild builds, and it is reached through a sudo grant whose argument is +# whatever an agent typed. sudoers deliberately does not validate that +# argument (`* ` allows one argument of anything) — this script is the +# validator, so each thing it must refuse gets an assertion: +# +# - anything that is not a BRANCH on this box's own origin. A tag or a +# sha would be a downgrade primitive; refs/pull/N/head would be a +# stranger's unreviewed pull request built as root, which on a public +# repo is anyone at all. The `--heads` scoping is what stops it, and +# the assertion below has a real refs/pull/1/head to prove it. +# - a candidate that is the tracked branch's own head, so there is +# nothing to try. The rest of "strictly ahead" is agent-box-source's: +# ANCESTRY needs the objects, and a question — or a mistyped branch +# name — must not fetch into the tree that decides what root builds to +# answer itself. tests/test-source-tree.sh asserts that half. +# - a name that is not a branch name at all: traversal, empty +# components, characters that have no business reaching git or a path. +# +# And two things it must PRESERVE, which are less obvious than the +# refusals: the `on` fact survives both a re-queue and a --reset, because +# it is what tells agent-box-source the box has a way home. Deleting it +# would strand the box exactly where a reset is meant to rescue it from. +# +# No network and no systemd: `origin` is a local repository built here and +# the update trigger is a shim that records its own invocation, so the whole +# file runs natively on every architecture. +set -u + +SCRIPT=${1:?usage: test-candidate.sh PATH/TO/candidate.sh} +[ -f "$SCRIPT" ] || { echo "no such script: $SCRIPT" >&2; exit 2; } +SCRIPT=$(cd "$(dirname "$SCRIPT")" && pwd)/$(basename "$SCRIPT") + +work=$(mktemp -d) +trap 'rm -rf "$work"' EXIT + +export HOME="$work/home"; mkdir -p "$HOME" +export GIT_CONFIG_GLOBAL="$work/gitconfig" +export GIT_CONFIG_NOSYSTEM=1 +export GIT_AUTHOR_NAME=t GIT_AUTHOR_EMAIL=t@example.invalid +export GIT_COMMITTER_NAME=t GIT_COMMITTER_EMAIL=t@example.invalid + +# --- the "upstream" this box is built from ------------------------------ +upstream="$work/upstream" +git init --quiet --initial-branch=master "$upstream" +echo one > "$upstream/file" +git -C "$upstream" add file +git -C "$upstream" commit --quiet -m one +REV_ONE=$(git -C "$upstream" rev-parse HEAD) +echo two > "$upstream/file" +git -C "$upstream" commit --quiet -am two +REV_TWO=$(git -C "$upstream" rev-parse HEAD) +git -C "$upstream" tag v-two + +# A candidate branch, off master's tip: what a rebased branch looks like. +git -C "$upstream" checkout --quiet -b ahead +echo three > "$upstream/file" +git -C "$upstream" commit --quiet -am three +REV_AHEAD=$(git -C "$upstream" rev-parse HEAD) +# One that is NOT ahead: it branches from the first commit. +git -C "$upstream" checkout --quiet -b behind "$REV_ONE" +git -C "$upstream" checkout --quiet master +# And one that is master's head exactly, so there is nothing to try. +git -C "$upstream" branch --quiet same "$REV_TWO" +# A pull-request ref, exactly as GitHub serves one. Fetchable by name and +# NOT a branch — the refusal this repo cares about most, since a public +# repo takes pull requests from anyone. +git -C "$upstream" update-ref refs/pull/1/head "$REV_AHEAD" + +src="$work/src" +git clone --quiet "$upstream" "$src" +git -C "$src" checkout --quiet -B master "$REV_TWO" + +marker="$work/candidate" +trigger_log="$work/triggered" +trigger="$work/bin/trigger" +mkdir -p "$work/bin" +cat > "$trigger" <<'SHIM' +#!/bin/sh +# Stands in for `systemctl start --no-block agent-box-update.service`. +echo "triggered" >> "$TRIGGER_LOG" +SHIM +chmod +x "$trigger" +export TRIGGER_LOG="$trigger_log" + +# The renderer prepends the constants; do exactly that, so what is under +# test is the shipped body and not a paraphrase of it. +runner="$work/agent-box-candidate" +{ + printf 'CANDIDATE_FILE=%s\n' "$marker" + printf 'SRC_DIR=%s\n' "$src" + printf 'SRC_URL=%s\n' "$upstream" + printf "SRC_BRANCH=master\n" + printf 'SYSTEMCTL=/bin/true\n' + printf 'UPDATE_TRIGGER=%s\n' "$trigger" + cat "$SCRIPT" +} > "$runner" + +fails=0 +ok() { printf 'ok %s\n' "$1"; } +no() { printf 'FAIL %s\n %s\n' "$1" "${2:-}"; fails=$((fails + 1)); } + +# run ARG... — stdout in $work/out, stderr in $work/err, rc in $rc +run() { + rc=0 + : > "$trigger_log" + sh "$runner" "$@" > "$work/out" 2> "$work/err" || rc=$? +} +said() { grep -F "$1" "$work/err" > /dev/null; } +printed() { grep -F "$1" "$work/out" > /dev/null; } +triggered() { [ -s "$trigger_log" ]; } +queued() { sed -n 's/^want=//p' "$marker" 2>/dev/null | head -n 1; } +fact() { sed -n 's/^on=//p' "$marker" 2>/dev/null | head -n 1; } + +# --- refusals: not a branch on our origin ------------------------------- +# Every one of these must exit non-zero AND leave the queue untouched: a +# refusal that still queued something would be installed by the next +# update, which is the whole failure this validator exists to prevent. +refuses() { + what=$1; shift + rm -f "$marker" + run "$@" + if [ "$rc" != 0 ] && ! triggered && [ ! -e "$marker" ]; then + ok "refuses $what" + else + no "refuses $what" "rc=$rc triggered=$(triggered && echo yes || echo no) marker=$(cat "$marker" 2>/dev/null)" + fi +} + +refuses "a pull-request ref" refs/pull/1/head +refuses "a bare sha" "$REV_AHEAD" +refuses "a tag" v-two +refuses "a branch that does not exist" no-such-branch +refuses "a path traversal" ../../etc/passwd +refuses "an embedded .." "fix/..\\/etc" +refuses "a leading slash" /master +refuses "a trailing slash" master/ +refuses "an empty component" fix//thing +refuses "a shell metacharacter" 'fix/x;reboot' +refuses "a space" 'fix/ x' +refuses "a branch at the tracked branch's own head" same +refuses "two arguments" ahead extra + +# Usage goes to stderr, like every other diagnostic here. +run ""; [ "$rc" != 0 ] && said "usage:" \ + && ok "refuses an empty argument, with usage" \ + || no "refuses an empty argument, with usage" "rc=$rc" + +# A pull-request ref must be refused for the right REASON — because it is +# not a branch, not because the name looks odd. This is the negative +# control on the --heads scoping: the ref exists and is fetchable. +rm -f "$marker" +run refs/pull/1/head +said "is a ref path, not a branch" \ + && ok "and says so, without inventing a branch to suggest" \ + || no "and says so, without inventing a branch to suggest" "$(cat "$work/err")" +# A refs/heads/ path DOES have a branch to suggest, and says which. +run refs/heads/ahead +said "name the branch itself: 'ahead'" \ + && ok "and a refs/heads/ path is told which branch it meant" \ + || no "and a refs/heads/ path is told which branch it meant" "$(cat "$work/err")" + +# --- the accept path ---------------------------------------------------- +rm -f "$marker" +run ahead +[ "$rc" = 0 ] && ok "accepts a branch that is ahead" \ + || no "accepts a branch that is ahead" "rc=$rc $(cat "$work/err")" +[ "$(queued)" = "refs/heads/ahead" ] \ + && ok "and queues it as a refs/heads/ path" \ + || no "and queues it as a refs/heads/ path" "$(cat "$marker" 2>/dev/null)" +triggered && ok "and triggers the update" \ + || no "and triggers the update" "the trigger was not run" +said "the next update returns to it" \ + && ok "and says the candidate is temporary" \ + || no "and says the candidate is temporary" "$(cat "$work/err")" + +# --- the `on` fact survives what must not strand the box ---------------- +printf 'on=%s\n' "$REV_AHEAD" > "$marker" +run ahead +[ "$rc" = 0 ] && [ "$(fact)" = "$REV_AHEAD" ] && [ "$(queued)" = "refs/heads/ahead" ] \ + && ok "re-queueing keeps the on= fact, so a sideways move can still come home" \ + || no "re-queueing keeps the on= fact" "$(cat "$marker" 2>/dev/null)" + +printf 'want=refs/heads/ahead\non=%s\n' "$REV_AHEAD" > "$marker" +run --reset +[ "$rc" = 0 ] && [ -z "$(queued)" ] && [ "$(fact)" = "$REV_AHEAD" ] \ + && ok "--reset drops the queue and keeps the on= fact" \ + || no "--reset drops the queue and keeps the on= fact" "$(cat "$marker" 2>/dev/null)" +triggered && ok "and triggers the update that brings the box home" \ + || no "and triggers the update that brings the box home" "the trigger was not run" + +printf 'want=refs/heads/ahead\n' > "$marker" +run --reset +[ "$rc" = 0 ] && [ ! -e "$marker" ] \ + && ok "--reset with no on= fact removes the marker entirely" \ + || no "--reset with no on= fact removes the marker" "$(cat "$marker" 2>/dev/null)" + +# --- --status reports the tree, not the marker's wishes ----------------- +rm -f "$marker" +run --status +[ "$rc" = 0 ] && printed "on the tracked branch" \ + && ok "--status: a box at the tracked branch's head says so" \ + || no "--status: a box at the tracked branch's head says so" "$(cat "$work/out")" + +git -C "$src" checkout --quiet -B master "$REV_ONE" +run --status +printed "an update is available" \ + && ok "--status: a box behind the branch says an update is available" \ + || no "--status: behind the branch" "$(cat "$work/out")" + +# Off the branch and CLAIMING a candidate, with the claim matching the rev +# the tree is on: a candidate. +git -C "$src" checkout --quiet -B master "$REV_AHEAD" +printf 'on=%s\n' "$REV_AHEAD" > "$marker" +run --status +printed "CANDIDATE" \ + && ok "--status: a verified candidate is reported as one" \ + || no "--status: a verified candidate is reported as one" "$(cat "$work/out")" + +# Off the branch with a claim that does NOT match: stale, so not a +# candidate — the same judgement agent-box-source makes before it would +# relax the fast-forward guard, and it must not read as "all fine here". +printf 'on=%s\n' "$REV_ONE" > "$marker" +run --status +printed "the fast-forward guard applies" \ + && ok "--status: a stale claim is not reported as a candidate" \ + || no "--status: a stale claim is not reported as a candidate" "$(cat "$work/out")" + +# --- a question is never a change --------------------------------------- +# --status and every refusal above run `git ls-remote` and touch the tree +# not at all — no fetch, no ref, no object. An agent asking what the box +# runs, or fat-fingering a branch name, must leave nothing behind in the +# tree that decides what root builds. (This is why the ancestry half of +# "strictly ahead" lives in agent-box-source: doing it here would have +# meant fetching, on a question.) +# Every ref, not just HEAD, and FETCH_HEAD too: a fetch would show up as a +# new object and a FETCH_HEAD even when the branch pointer never moved, and +# "does not move the tree" has to mean the whole tree. +before=$(git -C "$src" for-each-ref; git -C "$src" rev-parse HEAD) +before_objs=$(find "$src/.git/objects" -type f | sort | wc -l) +rm -f "$src/.git/FETCH_HEAD" +rm -f "$marker" +run --status +run no-such-branch +run refs/pull/1/head +run behind +after=$(git -C "$src" for-each-ref; git -C "$src" rev-parse HEAD) +after_objs=$(find "$src/.git/objects" -type f | sort | wc -l) +[ "$before" = "$after" ] \ + && ok "neither a question nor a refusal moves any ref in the tree" \ + || no "neither a question nor a refusal moves any ref" "refs changed" +[ "$before_objs" = "$after_objs" ] && [ ! -e "$src/.git/FETCH_HEAD" ] \ + && ok "and fetches nothing into it" \ + || no "and fetches nothing into it" "objects $before_objs -> $after_objs, FETCH_HEAD $([ -e "$src/.git/FETCH_HEAD" ] && echo present || echo absent)" + +echo +if [ "$fails" = 0 ]; then + echo "all candidate assertions passed" +else + echo "$fails failing assertion(s)" +fi +exit "$fails" diff --git a/tests/test-source-tree.sh b/tests/test-source-tree.sh index 58f95336..081cf217 100755 --- a/tests/test-source-tree.sh +++ b/tests/test-source-tree.sh @@ -324,6 +324,148 @@ AGENT_BOX_SRC_BRANCH=no-such-branch run check && ok "a failed check prints no rev at all" \ || no "a failed check prints no rev at all" "rc=$rc out=$(out)" +# --- a queued candidate (agent-box-candidate) --------------------------- +# +# The other half of "try a fix on this box before the fleet takes it": the +# wrapper queues a branch, and THIS script is what consumes it. Three +# properties are load-bearing, and each one is the difference between a +# candidate and a pin: +# +# - it is consumed, once. A candidate that survived its own update would +# be reinstalled by every later trigger, and a box that failed to build +# it would retry forever. +# - coming home does not need the operator. A candidate is squash-merged, +# so its head is never an ancestor of the tracked branch and --ff-only +# refuses the way back — the box would be stuck off-branch, which is the +# exact permanent divergence the feature promises not to create. +# - an explicit --rev keeps the strict guard. The relaxation is for the +# queue and for the way home, not a general-purpose force. +candidate_file="$work/candidate" +export AGENT_BOX_CANDIDATE_FILE="$candidate_file" + +# A branch off the tracked branch's tip, which is what a rebased candidate +# looks like. REV_THREE is master's head throughout. +git -C "$upstream" branch --quiet cand "$REV_THREE" 2>/dev/null || \ + git -C "$upstream" branch -f cand "$REV_THREE" +git -C "$upstream" checkout --quiet cand +echo four > "$upstream/file" +git -C "$upstream" commit --quiet -am four +REV_CAND=$(git -C "$upstream" rev-parse HEAD) +git -C "$upstream" checkout --quiet master + +at "$REV_THREE" +export AGENT_BOX_SRC_REV="$REV_THREE" +printf 'want=refs/heads/cand\n' > "$candidate_file" +run pull +[ "$rc" = 0 ] && [ "$(out)" = "$REV_CAND" ] \ + && ok "a queued candidate is what the tree moves to" \ + || no "a queued candidate is what the tree moves to" "rc=$rc out=$(out) want=$REV_CAND" +said "installing candidate refs/heads/cand" \ + && ok "and it says so, naming the branch" \ + || no "and it says so, naming the branch" "$(cat "$work/err")" +grep -q "^want=" "$candidate_file" 2>/dev/null \ + && no "the queued candidate is consumed" "want= survived the pull" \ + || ok "the queued candidate is consumed, so no later update reinstalls it" +[ "$(sed -n 's/^on=//p' "$candidate_file")" = "$REV_CAND" ] \ + && ok "and the box records WHICH candidate it now runs" \ + || no "and the box records which candidate it now runs" "$(cat "$candidate_file" 2>/dev/null)" + +# A candidate that is NOT ahead of the tracked branch is refused here, +# whatever wrote the marker. This is what keeps "candidate" from being a way +# to replay an older rev, and it has to live on this side: the force path +# below is taken by any box already off the branch, so a candidate accepted +# without this check would be installed with no ancestry guard at all. +git -C "$upstream" branch --quiet stale "$REV_ONE" 2>/dev/null || \ + git -C "$upstream" branch -f stale "$REV_ONE" +at "$REV_THREE" +printf 'want=refs/heads/stale\n' > "$candidate_file" +AGENT_BOX_SRC_REV="$REV_THREE" run pull +[ "$rc" != 0 ] && said "is not ahead of master" \ + && ok "a candidate behind the tracked branch is refused" \ + || no "a candidate behind the tracked branch is refused" "rc=$rc out=$(out)" +[ "$(head_of "$dir")" = "$REV_THREE" ] \ + && ok "and the tree does not move for it" \ + || no "and the tree does not move for it" "$(head_of "$dir")" + +# Same refusal on a box that is already off the branch, which is the case +# that matters: there the guard would otherwise be forced past. +at "$REV_CAND" +printf 'want=refs/heads/stale\non=%s\n' "$REV_CAND" > "$candidate_file" +AGENT_BOX_SRC_REV="$REV_CAND" run pull +[ "$rc" != 0 ] && said "is not ahead of master" \ + && ok "and refused on a candidate box too, where force would apply" \ + || no "and refused on a candidate box too" "rc=$rc out=$(out)" + +# Now the box RUNS the candidate, and the branch has been squash-merged: +# master has a new commit that is not the candidate's parent, so the +# candidate is not an ancestor of master and --ff-only cannot get home. +echo five > "$upstream/file" +git -C "$upstream" commit --quiet -am "squash of cand" +REV_MERGED=$(git -C "$upstream" rev-parse HEAD) +at "$REV_CAND" +printf 'on=%s\n' "$REV_CAND" > "$candidate_file" +AGENT_BOX_SRC_REV="$REV_CAND" run pull +[ "$rc" = 0 ] && [ "$(out)" = "$REV_MERGED" ] \ + && ok "a plain update brings a candidate box home to the tracked branch" \ + || no "a plain update brings a candidate box home" "rc=$rc out=$(out) want=$REV_MERGED" +said "runs candidate" \ + && ok "and names why the fast-forward check was skipped" \ + || no "and names why the fast-forward check was skipped" "$(cat "$work/err")" +[ ! -e "$candidate_file" ] \ + && ok "and clears the marker, so nothing stays forced once it is home" \ + || no "and clears the marker once home" "$(cat "$candidate_file" 2>/dev/null)" + +# THE safety property. "Off the tracked branch" is not the signal — a +# rewritten upstream history looks identical from the ancestry side, and +# that is the one thing --ff-only exists to refuse. Without a marker +# claiming a candidate, an off-branch box keeps the strict guard. +at "$REV_CAND" +rm -f "$candidate_file" +AGENT_BOX_SRC_REV="$REV_CAND" AGENT_BOX_SRC_REF="$REV_ONE" run pull +[ "$rc" != 0 ] && said "refusing update" \ + && ok "an off-branch box with no candidate marker is still guarded" \ + || no "an off-branch box with no candidate marker is still guarded" "rc=$rc out=$(out)" + +# And a marker whose claim does not match the rev the box runs is stale — +# a rebuild that failed and rolled the tree back leaves exactly that — so +# it is ignored rather than trusted into a force. +at "$REV_CAND" +printf 'on=%s\n' "$REV_ONE" > "$candidate_file" +AGENT_BOX_SRC_REV="$REV_CAND" AGENT_BOX_SRC_REF="$REV_ONE" run pull +[ "$rc" != 0 ] && said "ignoring it" \ + && ok "a stale candidate marker is ignored, not trusted" \ + || no "a stale candidate marker is ignored, not trusted" "rc=$rc out=$(out)" + +# The relaxation is scoped to the way home. An operator naming an OLDER rev +# explicitly is refused even on a genuine candidate box, so "I once tried a +# candidate" never becomes "this box accepts downgrades". +at "$REV_CAND" +printf 'on=%s\n' "$REV_CAND" > "$candidate_file" +AGENT_BOX_SRC_REV="$REV_CAND" AGENT_BOX_SRC_REF="$REV_ONE" run pull +[ "$rc" != 0 ] && said "refusing update" \ + && ok "an explicit older --rev is still refused on a candidate box" \ + || no "an explicit older --rev is still refused on a candidate box" "rc=$rc out=$(out)" + +# And the queue does not override an operator who named a ref. +at "$REV_TWO" +printf 'want=refs/heads/cand\n' > "$candidate_file" +AGENT_BOX_SRC_REV="$REV_TWO" AGENT_BOX_SRC_REF="$REV_THREE" run pull +[ "$rc" = 0 ] && [ "$(out)" = "$REV_THREE" ] \ + && ok "an explicit --rev wins over a queued candidate" \ + || no "an explicit --rev wins over a queued candidate" "rc=$rc out=$(out)" +grep -q "^want=" "$candidate_file" 2>/dev/null \ + && ok "and leaves the queue alone, so the next plain update still sees it" \ + || no "and leaves the queue alone" "marker was consumed by a --rev run" +rm -f "$candidate_file" + +# A box with the feature unwired behaves exactly as it did before it +# existed: no marker path, no candidate, no change in the guard. +at "$REV_ONE" +AGENT_BOX_CANDIDATE_FILE= AGENT_BOX_SRC_REV="$REV_ONE" run pull +[ "$rc" = 0 ] && [ "$(out)" = "$REV_MERGED" ] \ + && ok "with no marker path wired, a pull is an ordinary fast-forward" \ + || no "with no marker path wired, a pull is an ordinary fast-forward" "rc=$rc out=$(out)" + # --- a clone that cannot happen ----------------------------------------- # Offline, or a repo this box's token cannot read. It must fail loudly and # leave nothing behind: a half-clone at $dir would be skipped by every later diff --git a/tests/test_agentbox.py b/tests/test_agentbox.py index 588c48a3..df5f2149 100644 --- a/tests/test_agentbox.py +++ b/tests/test_agentbox.py @@ -154,6 +154,13 @@ def build_fake_profile(root): (prof / "libexec" / "agent-box").mkdir(parents=True) shutil.copy(SRC / "password-helper.py", prof / "libexec" / "agent-box" / "password-helper.py") + # agent-box-candidate's body, rendered per box with its constants + # prepended (see Renderer's candidate block). Copied here for the same + # reason the password helper is: without it the renderer's `is_file()` + # guard skips the wrapper, and the expected tree would ratify its + # ABSENCE instead of its content. + shutil.copy(SRC / "candidate.sh", + prof / "libexec" / "agent-box" / "candidate.sh") return prof From 352a4847a4a03e9860420d8f876cf09438b912c4 Mon Sep 17 00:00:00 2001 From: defangdevs Date: Thu, 3 Sep 2026 23:06:42 +0000 Subject: [PATCH 2/2] fix(update): render agent-box-candidate on a web-disabled box too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit on #558, three findings, all valid. **The wrapper was rendered from inside `Renderer.caddy()`**, which `render()` calls only when `web.enable` is true — while the sudo GRANT for it comes from `implied_sudo_commands()`, which is not gated on web at all (native has no `selfUpdate.enable` to gate the update trigger on either). So a web-disabled native box granted a command that was not on disk: sudo finds the rule, exec fails with a bare "No such file or directory", and nothing says the box was built without it. Same trap as #403's — where `agent-box-session` and `agent-box-profile` were generated from that same method — and as the web-gated paths #198 leaked into the ungated agent unit. It now has its own `candidate_helper()`, called unconditionally, and `test_candidate_helper_generated_without_web` asserts the grant and the file together, since the pair is what matters. **`tracked_head`'s `die` only exits its command substitution.** With `set -u` and no `set -e` — deliberate, since every refusal here is a `die` with a reason rather than a bare non-zero — an unchecked `head=$(tracked_head)` carried on with `head=''`: `--status` printed an empty tracked rev, and the "is it already the tracked head" test passed by comparing against nothing, so a candidate could be queued after tracked-branch resolution had failed. Both call sites now check. **`--status` misreported an ordinary box.** The tracked head can be a rev the tree has not fetched yet, which is the normal state between updates; `merge-base --is-ancestor` then exits 128 on the missing object and fell through to the "off the branch, and not as a candidate" warning — reporting a perfectly healthy box as one the fast-forward guard has a problem with. It now checks `cat-file -e` first and says what is actually known: there is a newer rev, and this tree cannot place itself against it without a fetch, which `--status`, being a question, will not do. Checks run: - All 31 native aarch64-linux checks build clean. - `python3 tests/test_agentbox.py`: 122 tests OK (1 skipped), including the new web-off case. - tests/test-candidate.sh and tests/test-source-tree.sh both fully green. - Module, native expected tree and golden fixture regenerated. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01TbhGuo3wu5mkgbrkW1kkXv --- bin/agentbox | 85 ++++++++++++------- modules/agent-box.nix | 20 ++++- modules/src/candidate.sh | 20 ++++- tests/golden/web/payloads/agent-box-candidate | 20 ++++- .../etc/agent-box/bin/agent-box-candidate | 20 ++++- tests/test_agentbox.py | 35 ++++++++ 6 files changed, 159 insertions(+), 41 deletions(-) diff --git a/bin/agentbox b/bin/agentbox index 116e427b..bfb3d1df 100755 --- a/bin/agentbox +++ b/bin/agentbox @@ -1059,6 +1059,10 @@ class Renderer: self.memory_protection(t) else: self.memory_protection_off(t) + # Before the web branch, and outside it: the sudo grant for this is + # box-wide (implied_sudo_commands), so the wrapper must exist on a + # web-disabled box too. + self.candidate_helper(t) if self.spec.web_enable: self.caddy(t) self.fail2ban(t) @@ -2758,6 +2762,54 @@ class Renderer: + '[credential "https://gist.github.com"]\n' + f"\thelper = !{gh} auth git-credential\n") + def candidate_helper(self, t): + """agent-box-candidate: install a branch on THIS box before it is + merged and the fleet takes it (see modules/src/candidate.sh for what + it refuses). + + Called UNCONDITIONALLY, unlike the password helper it renders beside + on a web box. sudoers() grants CANDIDATE_TRIGGER from + implied_sudo_commands(), which is not gated on web.enable either -- + native has no selfUpdate.enable to gate the update trigger on, so + every native box has both. Rendering this from caddy() instead left a + web-disabled box granting a command that was not on disk: sudo would + find the rule, exec nothing, and the agent would see a bare "No such + file or directory" with no hint that the box was built without it. + Same shape as the web-gated paths #198 leaked into the ungated agent + unit, which is what tests/memory-protection.nix exists to catch. + + Constants are PREPENDED, not appended: this is shell, where the body's + `case` runs the moment it is reached, so a tail would assign after the + code that reads it. They are compiled in for the same sudo reason as + the password helper -- env_reset means the wrapper carries no + environment, and a caller-supplied repo URL would be root building + code from anywhere. + """ + candidate_src = self.profile / "libexec" / "agent-box" / \ + "candidate.sh" + if candidate_src.is_file(): + # spec.repo, which prefers the PROFILE's manifest over the + # config key: the manifest cannot disagree with what is + # installed, and this wrapper must fetch from the repo the box + # actually came from. No branch key exists natively — an empty + # SRC_BRANCH is what tells the wrapper and agent-box-source to + # follow the remote's own default, rather than guessing + # "master" at a repo that may have renamed it. + url = f"https://github.com/{self.spec.repo}.git" + head = ( + "# Box constants, prepended by `agentbox apply`.\n" + f"CANDIDATE_FILE={shlex.quote(CANDIDATE_FILE)}\n" + f"SRC_DIR={shlex.quote(SRC_DIR)}\n" + f"SRC_URL={shlex.quote(url)}\n" + "SRC_BRANCH=''\n" + f"SYSTEMCTL={shlex.quote(SYSTEMCTL)}\n" + f"UPDATE_TRIGGER={shlex.quote(UPDATE_TRIGGER)}\n" + "\n" + ) + t.file(self.p(CANDIDATE_HELPER), + "#!/bin/sh\n" + head + candidate_src.read_text(), + 0o755) + def interactive_clis(self, t): """agent-box-session and agent-box-profile, on every box. @@ -2916,39 +2968,6 @@ class Renderer: f"#!{self.bin}/agent-box-python\n" + body + tail, 0o755) - # agent-box-candidate: install a branch on THIS box before it is - # merged and the fleet takes it (see modules/src/candidate.sh for - # what it refuses). Constants are PREPENDED, not appended: this is - # shell, where the body's `case` runs the moment it is reached, so a - # tail would assign after the code that reads it. Compiled in for - # the same sudo reason as the password helper — env_reset means the - # wrapper carries no environment, and a caller-supplied repo URL - # would be root building code from anywhere. - candidate_src = self.profile / "libexec" / "agent-box" / \ - "candidate.sh" - if candidate_src.is_file(): - # spec.repo, which prefers the PROFILE's manifest over the - # config key: the manifest cannot disagree with what is - # installed, and this wrapper must fetch from the repo the box - # actually came from. No branch key exists natively — an empty - # SRC_BRANCH is what tells the wrapper and agent-box-source to - # follow the remote's own default, rather than guessing - # "master" at a repo that may have renamed it. - url = f"https://github.com/{self.spec.repo}.git" - head = ( - "# Box constants, prepended by `agentbox apply`.\n" - f"CANDIDATE_FILE={shlex.quote(CANDIDATE_FILE)}\n" - f"SRC_DIR={shlex.quote(SRC_DIR)}\n" - f"SRC_URL={shlex.quote(url)}\n" - "SRC_BRANCH=''\n" - f"SYSTEMCTL={shlex.quote(SYSTEMCTL)}\n" - f"UPDATE_TRIGGER={shlex.quote(UPDATE_TRIGGER)}\n" - "\n" - ) - t.file(self.p(CANDIDATE_HELPER), - "#!/bin/sh\n" + head + candidate_src.read_text(), - 0o755) - # Interactive CLIs beyond agent-box-session/agent-box-profile # (interactive_clis, called unconditionally from render()): # agent-box-webhook needs the Caddy route this method renders, so diff --git a/modules/agent-box.nix b/modules/agent-box.nix index f06904e1..6989a38d 100644 --- a/modules/agent-box.nix +++ b/modules/agent-box.nix @@ -975,6 +975,13 @@ let # The tracked branch's head, as a rev. Asked of the remote WITHOUT touching # the tree: --status and the validation below are questions, and a question # must not be the thing that moves what the box builds. + # + # Its `die` only exits the COMMAND SUBSTITUTION it runs in, so both callers + # must check the status: with `set -u` alone (and no `set -e`, deliberately + # — every refusal here is a `die` with a reason, not a bare non-zero), an + # unchecked `head=$(tracked_head)` would carry on with head=''', print an + # empty tracked rev from --status, and let the "is it the tracked head + # already" test below pass by comparing against nothing. tracked_head() { b=''${SRC_BRANCH:-} if [ -n "$b" ]; then @@ -1005,7 +1012,7 @@ let (--status) rev=$(running_rev) || die "$SRC_DIR is not a checkout yet — this box has never updated" - head=$(tracked_head) + head=$(tracked_head) || exit 1 printf 'rev: %s\n' "$rev" printf 'tracked: %s at %s\n' "''${SRC_BRANCH:-origin/HEAD}" "$head" # The marker's own claim, believed only when it matches the rev the box @@ -1020,6 +1027,15 @@ let "''${SRC_BRANCH:-the tracked branch}" elif [ "$rev" = "$head" ]; then printf 'state: on the tracked branch\n' + elif ! command git -C "$SRC_DIR" cat-file -e "$head^{commit}" 2>/dev/null; then + # The remote has moved on and the tree has not fetched it yet, which is + # the ordinary state of a box between updates. merge-base would exit + # 128 on the missing object and fall through to the warning below, + # reporting a perfectly normal box as one the guard has a problem with. + # Say what is actually known instead: there is a newer rev, and this + # tree cannot place itself against it without a fetch — which --status, + # being a question, will not do. + printf 'state: an update is available (%.12s); this tree has not fetched it, so it cannot say more\n' "$head" elif command git -C "$SRC_DIR" merge-base --is-ancestor "$rev" "$head" 2>/dev/null; then printf 'state: behind the tracked branch — an update is available\n' else @@ -1127,7 +1143,7 @@ let # branch name — should leave behind in the tree that decides what root # builds. That check is also the better home for it on its own merits, since # it then holds however the marker was written. - head=$(tracked_head) + head=$(tracked_head) || exit 1 if [ "$want" = "$head" ]; then die "'$branch' is at ''${head} — the same rev as ''${SRC_BRANCH:-the tracked branch}, so there is nothing to try" fi diff --git a/modules/src/candidate.sh b/modules/src/candidate.sh index f6fcbe94..aa0f5c8e 100644 --- a/modules/src/candidate.sh +++ b/modules/src/candidate.sh @@ -77,6 +77,13 @@ USAGE # The tracked branch's head, as a rev. Asked of the remote WITHOUT touching # the tree: --status and the validation below are questions, and a question # must not be the thing that moves what the box builds. +# +# Its `die` only exits the COMMAND SUBSTITUTION it runs in, so both callers +# must check the status: with `set -u` alone (and no `set -e`, deliberately +# — every refusal here is a `die` with a reason, not a bare non-zero), an +# unchecked `head=$(tracked_head)` would carry on with head='', print an +# empty tracked rev from --status, and let the "is it the tracked head +# already" test below pass by comparing against nothing. tracked_head() { b=${SRC_BRANCH:-} if [ -n "$b" ]; then @@ -107,7 +114,7 @@ case "${1:-}" in (--status) rev=$(running_rev) || die "$SRC_DIR is not a checkout yet — this box has never updated" - head=$(tracked_head) + head=$(tracked_head) || exit 1 printf 'rev: %s\n' "$rev" printf 'tracked: %s at %s\n' "${SRC_BRANCH:-origin/HEAD}" "$head" # The marker's own claim, believed only when it matches the rev the box @@ -122,6 +129,15 @@ case "${1:-}" in "${SRC_BRANCH:-the tracked branch}" elif [ "$rev" = "$head" ]; then printf 'state: on the tracked branch\n' + elif ! command git -C "$SRC_DIR" cat-file -e "$head^{commit}" 2>/dev/null; then + # The remote has moved on and the tree has not fetched it yet, which is + # the ordinary state of a box between updates. merge-base would exit + # 128 on the missing object and fall through to the warning below, + # reporting a perfectly normal box as one the guard has a problem with. + # Say what is actually known instead: there is a newer rev, and this + # tree cannot place itself against it without a fetch — which --status, + # being a question, will not do. + printf 'state: an update is available (%.12s); this tree has not fetched it, so it cannot say more\n' "$head" elif command git -C "$SRC_DIR" merge-base --is-ancestor "$rev" "$head" 2>/dev/null; then printf 'state: behind the tracked branch — an update is available\n' else @@ -229,7 +245,7 @@ want=$(printf '%s\n' "$out" | head -n 1 | cut -f1) # branch name — should leave behind in the tree that decides what root # builds. That check is also the better home for it on its own merits, since # it then holds however the marker was written. -head=$(tracked_head) +head=$(tracked_head) || exit 1 if [ "$want" = "$head" ]; then die "'$branch' is at ${head} — the same rev as ${SRC_BRANCH:-the tracked branch}, so there is nothing to try" fi diff --git a/tests/golden/web/payloads/agent-box-candidate b/tests/golden/web/payloads/agent-box-candidate index 4c2b4be8..e0163ee9 100644 --- a/tests/golden/web/payloads/agent-box-candidate +++ b/tests/golden/web/payloads/agent-box-candidate @@ -88,6 +88,13 @@ USAGE # The tracked branch's head, as a rev. Asked of the remote WITHOUT touching # the tree: --status and the validation below are questions, and a question # must not be the thing that moves what the box builds. +# +# Its `die` only exits the COMMAND SUBSTITUTION it runs in, so both callers +# must check the status: with `set -u` alone (and no `set -e`, deliberately +# — every refusal here is a `die` with a reason, not a bare non-zero), an +# unchecked `head=$(tracked_head)` would carry on with head='', print an +# empty tracked rev from --status, and let the "is it the tracked head +# already" test below pass by comparing against nothing. tracked_head() { b=${SRC_BRANCH:-} if [ -n "$b" ]; then @@ -118,7 +125,7 @@ case "${1:-}" in (--status) rev=$(running_rev) || die "$SRC_DIR is not a checkout yet — this box has never updated" - head=$(tracked_head) + head=$(tracked_head) || exit 1 printf 'rev: %s\n' "$rev" printf 'tracked: %s at %s\n' "${SRC_BRANCH:-origin/HEAD}" "$head" # The marker's own claim, believed only when it matches the rev the box @@ -133,6 +140,15 @@ case "${1:-}" in "${SRC_BRANCH:-the tracked branch}" elif [ "$rev" = "$head" ]; then printf 'state: on the tracked branch\n' + elif ! command git -C "$SRC_DIR" cat-file -e "$head^{commit}" 2>/dev/null; then + # The remote has moved on and the tree has not fetched it yet, which is + # the ordinary state of a box between updates. merge-base would exit + # 128 on the missing object and fall through to the warning below, + # reporting a perfectly normal box as one the guard has a problem with. + # Say what is actually known instead: there is a newer rev, and this + # tree cannot place itself against it without a fetch — which --status, + # being a question, will not do. + printf 'state: an update is available (%.12s); this tree has not fetched it, so it cannot say more\n' "$head" elif command git -C "$SRC_DIR" merge-base --is-ancestor "$rev" "$head" 2>/dev/null; then printf 'state: behind the tracked branch — an update is available\n' else @@ -240,7 +256,7 @@ want=$(printf '%s\n' "$out" | head -n 1 | cut -f1) # branch name — should leave behind in the tree that decides what root # builds. That check is also the better home for it on its own merits, since # it then holds however the marker was written. -head=$(tracked_head) +head=$(tracked_head) || exit 1 if [ "$want" = "$head" ]; then die "'$branch' is at ${head} — the same rev as ${SRC_BRANCH:-the tracked branch}, so there is nothing to try" fi diff --git a/tests/native/expected/etc/agent-box/bin/agent-box-candidate b/tests/native/expected/etc/agent-box/bin/agent-box-candidate index 8e32dd72..2543f1fa 100755 --- a/tests/native/expected/etc/agent-box/bin/agent-box-candidate +++ b/tests/native/expected/etc/agent-box/bin/agent-box-candidate @@ -86,6 +86,13 @@ USAGE # The tracked branch's head, as a rev. Asked of the remote WITHOUT touching # the tree: --status and the validation below are questions, and a question # must not be the thing that moves what the box builds. +# +# Its `die` only exits the COMMAND SUBSTITUTION it runs in, so both callers +# must check the status: with `set -u` alone (and no `set -e`, deliberately +# — every refusal here is a `die` with a reason, not a bare non-zero), an +# unchecked `head=$(tracked_head)` would carry on with head='', print an +# empty tracked rev from --status, and let the "is it the tracked head +# already" test below pass by comparing against nothing. tracked_head() { b=${SRC_BRANCH:-} if [ -n "$b" ]; then @@ -116,7 +123,7 @@ case "${1:-}" in (--status) rev=$(running_rev) || die "$SRC_DIR is not a checkout yet — this box has never updated" - head=$(tracked_head) + head=$(tracked_head) || exit 1 printf 'rev: %s\n' "$rev" printf 'tracked: %s at %s\n' "${SRC_BRANCH:-origin/HEAD}" "$head" # The marker's own claim, believed only when it matches the rev the box @@ -131,6 +138,15 @@ case "${1:-}" in "${SRC_BRANCH:-the tracked branch}" elif [ "$rev" = "$head" ]; then printf 'state: on the tracked branch\n' + elif ! command git -C "$SRC_DIR" cat-file -e "$head^{commit}" 2>/dev/null; then + # The remote has moved on and the tree has not fetched it yet, which is + # the ordinary state of a box between updates. merge-base would exit + # 128 on the missing object and fall through to the warning below, + # reporting a perfectly normal box as one the guard has a problem with. + # Say what is actually known instead: there is a newer rev, and this + # tree cannot place itself against it without a fetch — which --status, + # being a question, will not do. + printf 'state: an update is available (%.12s); this tree has not fetched it, so it cannot say more\n' "$head" elif command git -C "$SRC_DIR" merge-base --is-ancestor "$rev" "$head" 2>/dev/null; then printf 'state: behind the tracked branch — an update is available\n' else @@ -238,7 +254,7 @@ want=$(printf '%s\n' "$out" | head -n 1 | cut -f1) # branch name — should leave behind in the tree that decides what root # builds. That check is also the better home for it on its own merits, since # it then holds however the marker was written. -head=$(tracked_head) +head=$(tracked_head) || exit 1 if [ "$want" = "$head" ]; then die "'$branch' is at ${head} — the same rev as ${SRC_BRANCH:-the tracked branch}, so there is nothing to try" fi diff --git a/tests/test_agentbox.py b/tests/test_agentbox.py index df5f2149..4b977013 100644 --- a/tests/test_agentbox.py +++ b/tests/test_agentbox.py @@ -1240,6 +1240,41 @@ def test_wrappers_generated_without_web(self): profile = (out / "usr/local/bin/agent-box-profile").read_text() self.assertIn("AGENT_BOX_ENVSTORE_BIN=", profile) + def test_candidate_helper_generated_without_web(self): + """agent-box-candidate must exist on a web-disabled box, because the + GRANT for it does. sudoers() adds it from implied_sudo_commands(), + which is not gated on web.enable -- native has no selfUpdate.enable + to gate the update trigger on either -- so a box that renders the + rule and not the file leaves sudo finding a command that is not + there: exec fails with a bare "No such file or directory" and + nothing says the box was built without it. + + Exactly the trap `test_wrappers_generated_without_web` above was + written for on #403, and it caught this one too (CodeRabbit finding + on the candidate-release PR): the wrapper was first rendered from + inside self.caddy(), which a web-disabled box never calls. The pair + is what matters, so this asserts BOTH halves together.""" + data = json.loads(CONFIG_JSON.read_text()) + data["web"] = {"enable": False} + del data["webhook"] + with tempfile.TemporaryDirectory() as tmp: + cfg = Path(tmp) / "no-web.json" + cfg.write_text(json.dumps(data)) + out = render(tmp, cfg) + helper = out / "etc/agent-box/bin/agent-box-candidate" + self.assertTrue( + helper.exists(), + "web.enable: false rendered no agent-box-candidate, but " + "sudoers still grants it") + # And it is the wrapper, with its constants compiled in rather + # than left to an environment sudo will not carry. + body = helper.read_text() + self.assertIn("CANDIDATE_FILE=", body) + self.assertIn("SRC_URL=", body) + self.assertIn("UPDATE_TRIGGER=", body) + sudoers = (out / "etc/sudoers.d/agent-box").read_text() + self.assertIn("/etc/agent-box/bin/agent-box-candidate *", sudoers) + def test_every_user_gets_their_own_canonical_guide(self): """Issue #394. Two things were wrong at once here.