From d4ab3629645c9597363bde31e3f7cdd77977186d Mon Sep 17 00:00:00 2001 From: claude-hdb Date: Mon, 13 Jul 2026 14:57:28 +0000 Subject: [PATCH] fix(runner): install refuses a box registered to another repo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `rig runner install --repo ` on a box already registered to repo A treated the mere existence of .runner as "already registered", skipped configure, restarted the service still pointed at A, and reported success. --repo was accepted, validated, and then ignored — leaving B with zero runners and its `runs-on` jobs queued against one that will never come. This is the natural next command after a partial `repoint`, and the failure is worse than a no-op: moving a runner between repos is a trust-boundary act, so quietly putting it back on the old one defeats the point of the move. Gate install on the repo .runner actually names. Convergence — the property worth keeping — is untouched: re-running against the repo the box is already on still skips registration, never prompts for a token, and exits 0. Skipping when the repo *differs* was never convergence, only a silently ignored argument, so it now fails and names both repos, pointing at `runner repoint` (move) or `runner remove` (start over). An unreadable .runner is refused too — it is no licence to assume a match. The .runner reader that `status` and `repoint` each carried is lifted into commands/lib/runner-config.sh, which now also holds the guard. Its json_field no longer dies bare under `set -o pipefail` when a key is missing, which is what `status`'s own ${REPO_URL:-unknown} fallback always assumed. Tests: the guard is exercised against a fixture .runner (refuses another repo naming both, points at repoint, no-ops on the same repo, passes an unregistered box, refuses an unreadable one) plus an ordering assertion that it precedes svc.sh start — reaching it through the CLI would need root and a really-registered runner, which the dependency-free harness cannot fabricate. All three mutants (guard deleted, guard comparing nothing, guard moved below the service start) go red. Closes #13 --- README.md | 17 ++++++--- commands/lib/runner-config.sh | 71 +++++++++++++++++++++++++++++++++++ commands/runner-install.sh | 27 +++++++++++-- commands/runner-repoint.sh | 10 ++--- commands/runner-status.sh | 15 +++----- test/cli.sh | 45 ++++++++++++++++++++++ 6 files changed, 161 insertions(+), 24 deletions(-) create mode 100644 commands/lib/runner-config.sh diff --git a/README.md b/README.md index 8114e7f..36e0fd1 100644 --- a/README.md +++ b/README.md @@ -168,7 +168,14 @@ from stale runners, so freezing it would just make it silently stop taking work. The install-time version is a starting point either way; `--version` exists for when you want that starting point deterministic and auditable. -Convergent — safe to re-run; an already-registered runner is left alone. +Convergent **toward `--repo`** — re-running against the repo this box is +already on re-uses the binary, skips registration, and never asks for a token. +Pointed at a *different* repo it **refuses**, and names both: skipping there +would not be convergence, it would be ignoring the argument — restarting the +runner on the **old** repo while reporting success, leaving the repo you asked +for with no runner and its `runs-on` jobs queued forever. Moving a runner +between repos is a trust-boundary act; that verb is +[`rig runner repoint`](#rig-runner-repoint---repo-ownerrepo). ### `rig runner status` @@ -225,10 +232,10 @@ Moves an installed runner from one repository to another: deregister, re-register, reusing the binary already on the box. It keeps the runner's existing name unless you pass `--name`. -This is the verb that was missing. `runner install` is convergent *by -skipping* — it sees a registered runner and leaves it alone — so it can -create a runner but never move one, and re-pointing a box meant hand-rolled -`config.sh`/`svc.sh` incantations against an install path only rig knew. +This is the verb that was missing. `runner install` can create a runner but +never move one — pointed at a repo the box is not on, it fails and sends you +here — and re-pointing a box otherwise meant hand-rolled `config.sh`/`svc.sh` +incantations against an install path only rig knew. Two short-lived tokens, each minted from **its own** repo — `RUNNER_REMOVE_TOKEN` for the one it's leaving, `RUNNER_TOKEN` for the one it's joining. Both are diff --git a/commands/lib/runner-config.sh b/commands/lib/runner-config.sh new file mode 100644 index 0000000..e8aaf0a --- /dev/null +++ b/commands/lib/runner-config.sh @@ -0,0 +1,71 @@ +#!/usr/bin/env bash +# Shared reader for the runner's own on-disk config ($RUNNER_DIR/.runner). +# Sourced by the runner-* commands; never executed on its own. + +# .runner is JSON, parsed here with grep/sed on purpose: a rig-bootstrapped box +# has no jq, and installing one to read two fields would be a poor trade. +# +# json_field — the first string value for , empty if absent. +# Never fails: callers run under `set -e` with pipefail, where a grep that +# matches nothing would otherwise kill the script with no message. A missing +# key is a fact to test for, not an error to die on. +json_field() { + grep -o "\"$2\"[[:space:]]*:[[:space:]]*\"[^\"]*\"" "$1" 2>/dev/null \ + | head -n1 | sed 's/.*:[[:space:]]*"//; s/"$//' || true +} + +# runner_repo_url — the repository this box's runner is registered +# to, empty when nothing is registered there. +runner_repo_url() { + [ -e "$1/.runner" ] || return 0 + json_field "$1/.runner" gitHubUrl +} + +# runner_agent_name — the runner's name, empty when unregistered. +runner_agent_name() { + [ -e "$1/.runner" ] || return 0 + json_field "$1/.runner" agentName +} + +# assert_runner_repo +# +# Returns 0 when the box has no runner, or has one already registered to +# : re-running `install` against the repo the box is already on is +# real convergence — it re-uses the binary, skips registration, exits 0. +# +# Returns 1, explaining itself on stderr, when the runner is registered to a +# DIFFERENT repo. Skipping *that* is not convergence, it is ignoring the +# argument: `install` would skip its configure step, restart the service on the +# OLD repo, and report success — leaving the repo you asked for with no runner +# and its jobs queued against one that will never come. Moving a runner between +# repos is a trust-boundary act, so it belongs to `repoint`, out loud. +assert_runner_repo() { + local dir="$1" repo="$2" current wanted + [ -e "$dir/.runner" ] || return 0 + + current="$(runner_repo_url "$dir")" + wanted="https://github.com/${repo}" + + if [ -z "$current" ]; then + printf 'rig-runner: ERROR: %s\n' \ +"${dir}/.runner exists but names no repository — this box's registration cannot +be read, so rig cannot tell whether it is already on ${wanted}. +Wipe the local registration and install again: + rig runner remove --local" >&2 + return 1 + fi + + if [ "$current" = "$wanted" ]; then + return 0 + fi + + printf 'rig-runner: ERROR: %s\n' \ +"this box's runner is already registered to ${current}, not ${wanted}. +install will not move a runner between repositories: it would leave the service +running against the OLD repo and report success. To move it in one act: + rig runner repoint --repo ${repo} +or take it off the old repo first, then install: + rig runner remove (deregisters from ${current}; needs a removal token) + rig runner remove --local (when you cannot mint one)" >&2 + return 1 +} diff --git a/commands/runner-install.sh b/commands/runner-install.sh index fac5b61..e9516ea 100755 --- a/commands/runner-install.sh +++ b/commands/runner-install.sh @@ -1,9 +1,15 @@ #!/usr/bin/env bash # rig runner install — GitHub Actions self-hosted runner as a systemd service # under an unprivileged user. Outbound-only (long-poll to GitHub), no Docker. -# Convergent: safe to re-run; an already-registered runner is left alone. +# Convergent toward --repo: re-running against the repo the box is already on +# leaves it alone; a box registered to a DIFFERENT repo is refused, never +# silently restarted on the old one (that is `repoint`'s job). set -euo pipefail +HERE="$(cd "$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" && pwd)" +# shellcheck source=SCRIPTDIR/lib/runner-config.sh +. "$HERE/lib/runner-config.sh" + log() { printf 'rig-runner: %s\n' "$*"; } warn() { printf 'rig-runner: WARNING: %s\n' "$*" >&2; } die() { printf 'rig-runner: ERROR: %s\n' "$1" >&2; exit "${2:-1}"; } @@ -33,6 +39,11 @@ the interactive prompt (get one from the repo's Settings > Actions > Runners > "New self-hosted runner", or: gh api -X POST repos//actions/runners/registration-token). It is consumed at registration and never written to disk by rig. + +Convergent toward --repo: re-running against the repo this box is already on +re-uses the binary, skips registration, and never asks for a token. A box +registered to a DIFFERENT repo is refused — moving a runner is +`rig runner repoint --repo `. EOF } @@ -88,17 +99,25 @@ else fi command -v curl >/dev/null || die "curl is required (run rig bootstrap first)" -# --- registration token — only when registration is actually pending ------- -# Pending unless the runner user already exists AND $RUNNER_DIR/.runner -# exists (user absent => nothing can be registered => pending). +# --- is this box already registered somewhere else? -------------------------- +# Before anything is prompted for, downloaded, or started: --repo must agree +# with what is already on the box. Everything below this point treats an +# existing .runner as "nothing to do" — which is right for the repo the box is +# already on, and silently wrong for any other. See assert_runner_repo. +# +# Registration is pending unless the runner user already exists AND +# $RUNNER_DIR/.runner exists (user absent => nothing can be registered). REG_PENDING=1 if id -u "$RUNNER_USER" >/dev/null 2>&1; then USER_HOME="$(getent passwd "$RUNNER_USER" | cut -d: -f6)" RUNNER_DIR="$USER_HOME/actions-runner" + assert_runner_repo "$RUNNER_DIR" "$REPO" || exit 1 if [ -e "$RUNNER_DIR/.runner" ]; then REG_PENDING=0 fi fi + +# --- registration token — only when registration is actually pending ------- if [ "$REG_PENDING" -eq 1 ]; then RUNNER_TOKEN="${RUNNER_TOKEN:-}" if [ -z "$RUNNER_TOKEN" ]; then diff --git a/commands/runner-repoint.sh b/commands/runner-repoint.sh index 8305cca..9f5463c 100755 --- a/commands/runner-repoint.sh +++ b/commands/runner-repoint.sh @@ -5,6 +5,8 @@ set -euo pipefail HERE="$(cd "$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" && pwd)" +# shellcheck source=SCRIPTDIR/lib/runner-config.sh +. "$HERE/lib/runner-config.sh" log() { printf 'rig-runner: %s\n' "$*"; } warn() { printf 'rig-runner: WARNING: %s\n' "$*" >&2; } @@ -94,11 +96,7 @@ RUNNER_DIR="$USER_HOME/actions-runner" || die "no runner registered in ${RUNNER_DIR} — use: rig runner install" # --- what is it registered to now? ------------------------------------------ -json_field() { - grep -o "\"$2\"[[:space:]]*:[[:space:]]*\"[^\"]*\"" "$1" \ - | head -n1 | sed 's/.*:[[:space:]]*"//; s/"$//' -} -CURRENT_URL="$(json_field "$RUNNER_DIR/.runner" gitHubUrl)" +CURRENT_URL="$(runner_repo_url "$RUNNER_DIR")" TARGET_URL="https://github.com/${REPO}" if [ "$CURRENT_URL" = "$TARGET_URL" ]; then @@ -108,7 +106,7 @@ fi # Keep the runner's identity across the move unless told otherwise. if [ -z "$RUNNER_NAME" ]; then - RUNNER_NAME="$(json_field "$RUNNER_DIR/.runner" agentName)" + RUNNER_NAME="$(runner_agent_name "$RUNNER_DIR")" [ -n "$RUNNER_NAME" ] || die "could not read the current runner name from ${RUNNER_DIR}/.runner" fi if [ -z "$LABELS" ]; then diff --git a/commands/runner-status.sh b/commands/runner-status.sh index c3a1ffc..72df06a 100755 --- a/commands/runner-status.sh +++ b/commands/runner-status.sh @@ -3,6 +3,10 @@ # Read-only: reports what is already on the box. No credential, no network call. set -euo pipefail +HERE="$(cd "$(dirname "$(readlink -f "${BASH_SOURCE[0]}")")" && pwd)" +# shellcheck source=SCRIPTDIR/lib/runner-config.sh +. "$HERE/lib/runner-config.sh" + log() { printf 'rig-runner: %s\n' "$*"; } die() { printf 'rig-runner: ERROR: %s\n' "$1" >&2; exit "${2:-1}"; } @@ -47,15 +51,8 @@ RUNNER_DIR="$USER_HOME/actions-runner" || die "no runner registered in ${RUNNER_DIR}" # --- read the runner's own config ------------------------------------------- -# .runner is JSON. Kept dependency-free on purpose: a rig-bootstrapped box has -# no jq, and installing one to read five fields would be a poor trade. -json_field() { - grep -o "\"$2\"[[:space:]]*:[[:space:]]*\"[^\"]*\"" "$1" \ - | head -n1 | sed 's/.*:[[:space:]]*"//; s/"$//' -} - -REPO_URL="$(json_field "$RUNNER_DIR/.runner" gitHubUrl)" -RUNNER_NAME="$(json_field "$RUNNER_DIR/.runner" agentName)" +REPO_URL="$(runner_repo_url "$RUNNER_DIR")" +RUNNER_NAME="$(runner_agent_name "$RUNNER_DIR")" # GitHub owns the labels; the runner does not persist them locally. rig records # what it registered with, so a box installed before this existed reports the diff --git a/test/cli.sh b/test/cli.sh index c9ba794..0332275 100644 --- a/test/cli.sh +++ b/test/cli.sh @@ -82,6 +82,51 @@ fi check "runner: bad subcommand exits 2" 2 "usage:" "$ROOT/bin/rig" runner frobnicate +# --- runner install: --repo must agree with what the box is already on ------- +# The bug: `install --repo B` on a box registered to repo A skipped configure, +# restarted the service on A, and reported success — --repo accepted, validated, +# then ignored. The guard is exercised here through the shared lib, against a +# fixture .runner: reaching it via the CLI needs root AND a really-registered +# runner, neither of which this harness can fabricate. +guard() { # guard + bash -c 'set -euo pipefail + . "$1/commands/lib/runner-config.sh" + assert_runner_repo "$2" "$3"' _ "$ROOT" "$1" "$2" +} +REG_DIR="$(mktemp -d)" # a box registered to acme/alpha +EMPTY_DIR="$(mktemp -d)" # a box with no runner at all +printf '%s\n' '{"agentId":7,"agentName":"ci-box","gitHubUrl":"https://github.com/acme/alpha","workFolder":"_work"}' \ + > "$REG_DIR/.runner" + +check "runner install: refuses a repo the box is not registered to" \ + 1 "already registered to https://github.com/acme/alpha" guard "$REG_DIR" acme/beta +check "runner install: the refusal names the repo that was asked for" \ + 1 "not https://github.com/acme/beta" guard "$REG_DIR" acme/beta +check "runner install: the refusal points at repoint" \ + 1 "rig runner repoint --repo acme/beta" guard "$REG_DIR" acme/beta +# Convergence is the property worth keeping: same repo stays a clean no-op. +check "runner install: the repo it is already on is a no-op" \ + 0 "" guard "$REG_DIR" acme/alpha +check "runner install: an unregistered box passes the guard" \ + 0 "" guard "$EMPTY_DIR" acme/beta +# A .runner rig cannot read is not a licence to assume it matches. +printf '%s\n' '{"agentName":"ci-box"}' > "$REG_DIR/.runner" +check "runner install: refuses an unreadable registration" \ + 1 "names no repository" guard "$REG_DIR" acme/alpha +rm -rf "$REG_DIR" "$EMPTY_DIR" + +# The guard is only worth something if it runs BEFORE the box is touched: the +# token prompt, the download, configure and svc.sh start all come after it. +# Ordering is the whole fix, so assert it rather than trust it. +# Matches the CALL, not the word: the comment above it mentions assert_runner_repo +# too, and a plain grep would keep finding that after the call itself was deleted. +# The defaults fail closed, so a guard that is gone cannot read as one that merely +# sits early in the file. +guard_at="$(grep -nE '^[[:space:]]*assert_runner_repo ' "$ROOT/commands/runner-install.sh" | head -n1 | cut -d: -f1)" +start_at="$(grep -n 'svc.sh start' "$ROOT/commands/runner-install.sh" | head -n1 | cut -d: -f1)" +check "runner install: the repo guard precedes svc.sh start" \ + 0 "" test "${guard_at:-999999}" -lt "${start_at:-0}" + check "runner status: --help exits 0" 0 "usage:" "$ROOT/commands/runner-status.sh" --help check "runner status: user needs value" 2 "needs a value" "$ROOT/commands/runner-status.sh" --user check "runner status: refuses --user root" 2 "must not be root" "$ROOT/commands/runner-status.sh" --user root