diff --git a/VERSION b/VERSION index 1892b92..31e5c84 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.3.2 +1.3.3 diff --git a/deploy/install-agent.sh b/deploy/install-agent.sh index bb2f47c..a29d228 100755 --- a/deploy/install-agent.sh +++ b/deploy/install-agent.sh @@ -21,26 +21,88 @@ id -u "$APP_USER" >/dev/null 2>&1 || { echo "No such account: $APP_USER" >&2; ex install -o "$APP_USER" -g "$APP_USER" -d "$ROOT/storage/app/deploy" chmod +x "$ROOT/deploy/update-agent.sh" -# rsync, for the invoice archive. +# ── The few things an update needs root for ────────────────────────────────── +# An update runs as the service account. Everything root-owned on the host is +# therefore out of its reach, and each such thing turns into "log into every +# server once and paste this" — which is exactly the work this project exists to +# remove. # -# It has to be on the HOST, not in the image: a backup that collects the archive -# connects by ssh to this machine, and sshd then starts `rsync --server` here. -# Without it the pull fails with "command not found" from a NAS whose own setup -# is perfectly correct — which is a bad afternoon for whoever is looking. +# The obvious fix is to let the service account run THIS script under sudo. That +# would be handing it root outright: it owns the checkout, so it can rewrite the +# very script it is allowed to run privileged. A grant is only worth anything if +# the holder cannot change what it grants. # -# Installed from here because this is the entry point that already runs as root, -# once per machine, and because the alternative is an operator logging into a -# server to install a package by hand every time they set up a new one. rrsync -# comes with it, and that is what locks the collecting key to one directory. +# So the privileged part is written OUT of the checkout, to a root-owned path, +# from a here-document rather than copied — the service account has no way to +# influence a single byte of it. What it may then do as root is one fixed +# command line, enumerated in sudoers, and nothing else. +# +# CONTRACT is the version of that fixed set. deploy/update.sh compares it and +# says so when this script has to be run again — a new step added here is +# useless on a server still carrying the old helper, and silently useless is the +# failure mode worth spending a line on. +HOST_STEP=/usr/local/sbin/clupilot-host-step + +cat > "$HOST_STEP" <<'EOF' +#!/usr/bin/env bash +# +# The root-owned half of a CluPilot update. Installed by deploy/install-agent.sh +# and never read from the application checkout — see the note there. +# +# One argument, from the list below. Nothing takes free-form input. +set -euo pipefail + +CONTRACT=1 + +case "${1:-}" in + contract) + echo "$CONTRACT" + ;; + + ensure-rsync) + # rsync has to be on the HOST: a backup that collects the invoice + # archive connects by ssh, and sshd starts `rsync --server` out here, + # not in a container. rrsync comes with it, and that is what locks the + # collecting key to one directory. + command -v rsync >/dev/null 2>&1 && exit 0 + command -v apt-get >/dev/null 2>&1 || { echo "no apt-get on this host" >&2; exit 3; } + export DEBIAN_FRONTEND=noninteractive + apt-get update -qq >/dev/null 2>&1 || true + apt-get install -y -qq rsync >/dev/null + command -v rsync >/dev/null 2>&1 || { echo "rsync still missing after install" >&2; exit 4; } + ;; + + *) + # Refused by name. sudoers already permits only the exact command lines + # above, so this is the second of two locks, not the only one. + echo "unknown step: ${1:-}" >&2 + exit 64 + ;; +esac +EOF + +chown root:root "$HOST_STEP" +chmod 0755 "$HOST_STEP" + +# The command line in full, not just the path: sudoers matches arguments too, so +# written this way the grant is for `clupilot-host-step ensure-rsync` and for +# nothing else the file might ever learn to do. +cat > /etc/sudoers.d/clupilot-host-step </dev/null || { + echo " ! sudoers fragment rejected — removing it. Updates will ask for rsync by hand." + rm -f /etc/sudoers.d/clupilot-host-step +} + +# And straight away, through the same helper rather than beside it — one +# implementation, exercised on every install instead of only when an update +# happens to find rsync missing. if ! command -v rsync >/dev/null 2>&1; then - echo " Installing rsync (needed by the invoice archive)" - if command -v apt-get >/dev/null 2>&1; then - DEBIAN_FRONTEND=noninteractive apt-get update -qq >/dev/null 2>&1 || true - DEBIAN_FRONTEND=noninteractive apt-get install -y -qq rsync >/dev/null 2>&1 \ - || echo " ! Could not install rsync — the invoice archive cannot be collected until it is there." - else - echo " ! No apt-get here. Install rsync yourself, or the invoice archive cannot be collected." - fi + echo " Installing rsync (the invoice archive is collected over ssh)" + "$HOST_STEP" ensure-rsync \ + || echo " ! Could not install rsync — the invoice archive cannot be collected until it is there." fi cat > /etc/systemd/system/clupilot-update-agent.service </dev/null 2>&1; then + if [[ -x "$HOST_STEP" ]] && sudo -n "$HOST_STEP" ensure-rsync >/dev/null 2>&1; then + log "Installed rsync on the host (the invoice archive is collected over ssh)" + else + warn "rsync is not installed on this host — the invoice archive cannot be collected." + fi +fi + phase maintenance_off "Leaving maintenance mode" in_app php artisan up >/dev/null down=0 @@ -492,6 +516,20 @@ fi # The proxy's console allowlist has to be wired once, as root, and an update # never runs as root — so an existing installation would keep its hard-coded # list and everything the owner changes in the console would do nothing. +# A host helper from before a step was added to it. Nothing is broken, but the +# new step silently does nothing — which is worse than an error, because the +# next person to look assumes it ran. +if [[ -z "$agent_hint" ]]; then + # `|| true` on the read, not on the whole test: an installation that has + # never had the helper reports 0 and lands in the same hint. + host_step_have="$( [[ -x "$HOST_STEP" ]] && "$HOST_STEP" contract 2>/dev/null || true )" + [[ "$host_step_have" =~ ^[0-9]+$ ]] || host_step_have=0 + + if (( host_step_have < HOST_STEP_NEEDS )); then + agent_hint="the update cannot install host packages such as rsync by itself" + fi +fi + if [[ -z "$agent_hint" ]] && command -v caddy >/dev/null 2>&1 \ && [[ -f /etc/caddy/Caddyfile ]] \ && ! grep -q 'clupilot-console-allow.conf' /etc/caddy/Caddyfile 2>/dev/null; then diff --git a/tests/Feature/HostStepTest.php b/tests/Feature/HostStepTest.php new file mode 100644 index 0000000..5e165c8 --- /dev/null +++ b/tests/Feature/HostStepTest.php @@ -0,0 +1,183 @@ +toContain("cat > \"\$HOST_STEP\" <<'EOF'"); + + // betweenFirst, NOT between: `between` cuts at the LAST delimiter, and this + // installer has several here-documents after this one — so it handed back + // the helper plus everything down to the final EOF, and the extra `EOF` + // line then ran as a command. The helper was fine; the measurement was not. + $body = Str::betweenFirst($installer, "cat > \"\$HOST_STEP\" <<'EOF'\n", "\nEOF\n"); + + // The extraction is part of what these tests trust, so it is checked at + // both ends rather than only at the start. + expect($body)->toContain('CONTRACT=') + ->and(rtrim($body))->toEndWith('esac') + ->and($body)->not->toContain("\nEOF"); + + return $body; +} + +function runHostStep(string $args, array $onPath = []): Process +{ + $dir = sys_get_temp_dir().'/clupilot-host-step-'.bin2hex(random_bytes(6)); + mkdir($dir.'/bin', 0755, true); + file_put_contents($dir.'/clupilot-host-step', hostStepScript()); + chmod($dir.'/clupilot-host-step', 0755); + + // A stub apt-get that fails loudly. Any test that does not expect an + // install will see it in the output rather than silently tolerating one. + file_put_contents($dir.'/bin/apt-get', "#!/bin/sh\necho 'APT-GET WAS CALLED' >&2\nexit 1\n"); + chmod($dir.'/bin/apt-get', 0755); + + foreach ($onPath as $name => $exit) { + file_put_contents($dir."/bin/{$name}", "#!/bin/sh\nexit {$exit}\n"); + chmod($dir."/bin/{$name}", 0755); + } + + $process = Process::fromShellCommandline( + 'PATH='.escapeshellarg($dir.'/bin').':/usr/bin:/bin ' + .escapeshellarg($dir.'/clupilot-host-step').' '.$args + ); + $process->run(); + + exec('rm -rf '.escapeshellarg($dir)); + + return $process; +} + +it('refuses a step it does not know by name', function () { + // The whole safety argument rests on this. sudoers permits one exact + // command line, and this is the second of the two locks: even reached some + // other way, the helper does nothing it was not built to do. + $process = runHostStep('install-whatever-i-like'); + + expect($process->getExitCode())->toBe(64) + ->and($process->getErrorOutput())->toContain('unknown step') + ->and($process->getErrorOutput())->not->toContain('APT-GET WAS CALLED'); +}); + +it('refuses being run with no step at all', function () { + // `set -u` would otherwise end this as an unbound-variable error, which is + // the right outcome by accident and reads as a bug in the helper. + $process = runHostStep(''); + + expect($process->getExitCode())->toBe(64) + ->and($process->getErrorOutput())->toContain('unknown step'); +}); + +it('does not run a package manager when rsync is already there', function () { + $process = runHostStep('ensure-rsync', ['rsync' => 0]); + + expect($process->getExitCode())->toBe(0) + ->and($process->getErrorOutput())->not->toContain('APT-GET WAS CALLED'); +}); + +it('says so rather than pretending, on a host with no apt-get', function () { + // Nothing here is Debian-only by design, but the install step is. Silence + // would leave an operator believing rsync is on a machine where it is not, + // and they would find out from a backup that collected nothing. + $dir = sys_get_temp_dir().'/clupilot-host-step-'.bin2hex(random_bytes(6)); + mkdir($dir.'/bin', 0755, true); + file_put_contents($dir.'/clupilot-host-step', hostStepScript()); + chmod($dir.'/clupilot-host-step', 0755); + + // An empty PATH, and bash named outright — the helper's own `#!/usr/bin/env + // bash` would not find an interpreter otherwise, and that 127 looks exactly + // like the refusal being tested here. + $process = new Process( + [trim((string) shell_exec('command -v bash')), $dir.'/clupilot-host-step', 'ensure-rsync'], + null, + ['PATH' => $dir.'/bin'], + ); + $process->run(); + exec('rm -rf '.escapeshellarg($dir)); + + expect($process->getExitCode())->toBe(3) + ->and($process->getErrorOutput())->toContain('no apt-get'); +}); + +it('reports the contract version the updater compares against', function () { + $process = runHostStep('contract'); + + expect($process->getExitCode())->toBe(0) + ->and(trim($process->getOutput()))->toMatch('/^\d+$/'); + + // And the two halves agree. A helper written by an older installer is the + // failure mode this number exists for; if update.sh asked for a version the + // current installer never writes, every up-to-date server would be told to + // run the installer again forever. + $needs = Str::between(file_get_contents(base_path('deploy/update.sh')), 'HOST_STEP_NEEDS=', "\n"); + + expect((int) trim($process->getOutput()))->toBe((int) trim($needs)); +}); + +it('grants one command line, not a script the service account can rewrite', function () { + $installer = file_get_contents(base_path('deploy/install-agent.sh')); + + expect($installer) + // Root's, at a path under root's — the service account cannot edit what + // it is allowed to run. This is the entire safety argument. + ->toContain('HOST_STEP=/usr/local/sbin/clupilot-host-step') + ->toContain('chown root:root "$HOST_STEP"') + ->toContain('chmod 0755 "$HOST_STEP"') + // Validated before it counts. A malformed fragment in /etc/sudoers.d + // breaks sudo for everyone on the machine, not only for this grant. + ->toContain('visudo -cf /etc/sudoers.d/clupilot-host-step') + // Both halves must mean the same path. + ->and(file_get_contents(base_path('deploy/update.sh'))) + ->toContain('HOST_STEP=/usr/local/sbin/clupilot-host-step'); + + // Every sudo grant this installer writes, read as sudoers reads it. + preg_match_all('/^\S+\s+ALL=\(root\)\s+NOPASSWD:\s*(.+)$/m', $installer, $m); + + expect($m[1])->not->toBeEmpty(); + + foreach ($m[1] as $granted) { + // An argument, always. A grant on a bare path permits that path with + // ANY arguments — so every step ever added to the helper would be + // covered by a decision taken before it existed. + expect(trim($granted))->toContain(' ') + // And never anything inside the checkout, which the service account + // owns: it could rewrite the file and the grant would follow. + ->not->toContain('$ROOT') + ->not->toContain('deploy/'); + } + + expect($m[1])->toContain('$HOST_STEP ensure-rsync'); +}); + +it('never lets a host package stop a deployment', function () { + // rsync is wanted by whoever collects the invoice archive, minutes or hours + // later. An update that died because a package manager was busy would be a + // far bigger problem than a backup waiting for the next run. + $update = file_get_contents(base_path('deploy/update.sh')); + + $call = Str::between($update, 'if ! command -v rsync', 'phase maintenance_off'); + + expect($call)->toContain('sudo -n "$HOST_STEP" ensure-rsync >/dev/null 2>&1') + ->and($call)->toContain('warn '); +});