Files
PSProxmoxVE/tests/infrastructure/scripts/prepare-test-environment.test.sh
T
goodolclint-claude[bot] def8dc6b67 fix: verify checksums for downloaded ISOs/images, keep sshpass off argv (#166)
* fix: verify checksums for downloaded ISOs/images, keep sshpass off argv

ensure-base-iso.sh downloaded the PVE install ISO over plain HTTP with no
checksum, caching it on the persistent /opt/pve-integration mount and
booting it as the nested trust root the integration suite relies on.
ensure-cloud-images.sh fetched the Ubuntu cloud image and OVA over HTTPS
but never checked them either. prepare-test-environment.sh and
diagnose-cluster.sh passed the nested root password to sshpass via -p,
putting it in the process table. create-api-token.sh, unused anywhere in
the repo, minted a privsep=0 root token and echoed the secret unmasked.

- ensure-base-iso.sh now downloads from https://enterprise.proxmox.com/iso
  and verifies against its SHA256SUMS on every run, including a cache hit.
  download.proxmox.com's own TLS cert does not list download.proxmox.com in
  its SAN (confirmed with curl/openssl from this environment), so https to
  that name fails certificate validation; enterprise.proxmox.com serves the
  identical ISO tree over a valid cert. Verification happens before the
  downloaded file is moved to its canonical cache path.
- ensure-cloud-images.sh verifies the cloud image and OVA against Ubuntu's
  published SHA256SUMS the same way, matching by upstream filename since
  the cloud image is cached locally under a different extension (.img
  upstream, .qcow2 cached — the bytes are already qcow2-formatted).
- prepare-test-environment.sh and diagnose-cluster.sh now export SSHPASS
  and call sshpass -e, keeping the password out of argv/ps. This also fixes
  a latent bug: the old unquoted `sshpass -p ${ROOT_PASS}` word-split any
  password containing whitespace.
- create-api-token.sh deleted; grep across the repo found no caller.

Reviewers (codex:codex-rescue, correctness-reviewer, security-reviewer) all
independently found the same blocking bug in the first pass: when a cached
file failed verification and the subsequent redownload then failed,
ensure-cloud-images.sh fell through to a "keep the stale copy" branch and
returned that same known-bad file with exit 0 — verification could be
bypassed by inducing one failed redownload. Fixed by deleting the file
immediately on a failed verification, before the redownload is attempted,
so the later "is there a safe stale copy" check can no longer find it.
Added a test case (case 5) that reproduces this exact sequence and
mutation-tested it against the unfixed code. The three reviews also
flagged a real but separate bug already fixed in this same change: `trap
... RETURN` inside a function nested in another function is not scoped to
that function in bash — it re-fires on the OUTER function's return,
referencing an out-of-scope local. Both verify_checksum() helpers now
clean up their temp file explicitly instead of via trap.

Findings not acted on, judged out of scope for this fix:
- SHA256SUMS-fetch failures are treated the same as a checksum mismatch
  (delete + fail) rather than left untouched — a transient network blip
  destroys a good multi-GB cached ISO. This is the safer failure direction
  (never silently trust unverified bytes) and was a deliberate trade-off,
  not a defect.
- ensure-cloud-images.sh's 7-day cache window can span an upstream
  republish of noble/current, causing a legitimate re-verification churn
  (not a security issue, a cache-hit-rate one). Pre-existing cache design,
  unrelated to adding verification.
- wait-for-pve.sh (curl -d with the password on argv) and
  prepare-test-environment.sh's own positional password argument (from
  run-integration.sh) carry the same password-on-argv pattern this issue
  targeted in create-api-token.sh, sshpass -p and diagnose-cluster.sh, but
  neither script nor run-integration.sh was named in the issue. Left
  untouched per scope; worth a follow-up issue.
- GPG/detached-signature verification of the upstream SHA256SUMS was not
  added — the new checks defend against cache poisoning and transit
  corruption, not a compromised origin. Worth a follow-up issue.
- The two new self-checks (ensure-base-iso.test.sh,
  ensure-cloud-images.test.sh) are not wired into
  .github/workflows/unit-tests.yml's shell-selfchecks job. That file is
  code-owned and out of scope for this change; needs an operator follow-up.

Password rotation (the Testpass123! value from before it moved to a
secret) is unaddressed here per the contract — flagged for the operator.

Mutation-tested: broke the post-download checksum check in
ensure-base-iso.sh, confirmed the affected test cases failed, restored it.
Broke the sshpass -e change back to -p, confirmed the new assertions in
prepare-test-environment.test.sh failed, restored it. Broke the fail-open
fix in ensure-cloud-images.sh, confirmed case 5 failed, restored it.

Closes #149

* fix: also verify the stale-by-age fallback copy in ensure-cloud-images.sh

PR review on #166 (COMMENTED, non-blocking) found the sibling of the
fail-open bug already fixed in this branch: when the cached cloud image
is stale by *age* (>= 7 days) rather than failed verification, the
redownload-failure fallback could hand back that file with exit 0
without ever re-verifying it in this run. A file that failed the
earlier verification is already deleted by the time the fallback runs,
but a stale-by-age file skips verification entirely on the way in.

Fixed by verifying the stale-by-age file at the point of actual
fallback use — after the redownload has failed, not proactively before
it's attempted, so a copy the redownload was about to replace anyway
isn't deleted along a path that would have succeeded. Added two test
cases (6, 7): a still-verifying stale-by-age copy is used as a
fallback; one that no longer verifies is not. Mutation-tested by
reverting to the unfixed fallback and confirming case 7 fails, then
restored.

---------

Co-authored-by: goodolclint-claude[bot] <323206664+goodolclint-claude[bot]@users.noreply.github.com>
2026-09-02 17:20:36 +00:00

180 lines
6.9 KiB
Bash
Executable File

#!/usr/bin/env bash
# Self-check for prepare-test-environment.sh's opt-in dist-upgrade branch.
#
# Stubs sshpass/curl/sleep on PATH so every path runs offline in ~0s, then
# asserts on the commands the script actually issued.
#
# The stub is deliberately stateful: boot_id must differ across the reboot, and
# case 3 pins the failure by returning the SAME boot_id twice.
#
# Run: bash tests/infrastructure/scripts/prepare-test-environment.test.sh
set -euo pipefail
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
TARGET="$SCRIPT_DIR/prepare-test-environment.sh"
TMP="$(mktemp -d)"
trap 'rm -rf "$TMP"' EXIT
mkdir -p "$TMP/bin"
# Fake sshpass. Logs every invocation; emits plausible output per command.
# BOOT_ID_STUCK=1 makes it return an unchanging boot_id, simulating a node that
# never rebooted.
cat > "$TMP/bin/sshpass" <<'STUB'
#!/usr/bin/env bash
echo "SSHPASS_ENV=${SSHPASS:-<unset>} ARGS=$*" >> "$STUB_LOG"
case "$*" in
*boot_id*)
if [[ "${BOOT_ID_STUCK:-0}" == "1" ]]; then
echo "11111111-1111-1111-1111-111111111111"
else
n=0
[[ -f "$STUB_STATE/boot_calls" ]] && n=$(cat "$STUB_STATE/boot_calls")
n=$((n + 1))
echo "$n" > "$STUB_STATE/boot_calls"
if [[ "$n" -le 1 ]]; then
echo "11111111-1111-1111-1111-111111111111"
else
echo "22222222-2222-2222-2222-222222222222"
fi
fi
;;
*dpkg-query*)
# DPKG_EMPTY=1 simulates a failed query whose output is swallowed by
# the remote `| sort`, which succeeds on empty input.
[[ "${DPKG_EMPTY:-0}" == "1" ]] || printf 'proxmox-kernel-6.14\t6.14.11-1\npve-manager\t9.2.1\n'
;;
*uname*)
printf '# running-kernel\t6.14.11-1-pve\n'
;;
esac
exit 0
STUB
# wait-for-api.sh greps curl output for "version"; log the call so the test can
# assert the wait actually ran. sleep must not really sleep.
cat > "$TMP/bin/curl" <<'STUB'
#!/usr/bin/env bash
echo "curl $*" >> "$STUB_LOG"
echo '{"data":{"version":"9.2.1"}}'
STUB
cat > "$TMP/bin/sleep" <<'STUB'
#!/usr/bin/env bash
exit 0
STUB
chmod +x "$TMP/bin/"*
export PATH="$TMP/bin:$PATH"
export STUB_STATE="$TMP"
fail=0
check() {
local desc="$1" haystack="$2" needle="$3" want="$4"
local found=no
grep -q -- "$needle" "$haystack" && found=yes
if [[ "$found" == "$want" ]]; then
echo " ok: $desc"
else
echo " FAIL: $desc (expected present=$want, got present=$found)"
fail=1
fi
}
pass() { echo " ok: $1"; }
fatal() { echo " FAIL: $1"; fail=1; }
echo "case 1: no dist-upgrade argument — lane 1 path must be untouched"
export STUB_LOG="$TMP/log1"
: > "$STUB_LOG"; rm -f "$TMP/boot_calls"
bash "$TARGET" 10.0.0.1 secret > "$TMP/out1" 2>&1
check "no dist-upgrade issued" "$STUB_LOG" "dist-upgrade" no
check "no reboot issued" "$STUB_LOG" "systemctl reboot" no
check "no package set recorded" "$STUB_LOG" "dpkg-query" no
check "no boot_id probe" "$STUB_LOG" "boot_id" no
check "storage still configured" "$STUB_LOG" "pvesm set local" yes
check "password not passed as -p arg" "$STUB_LOG" "ARGS=-p" no
check "sshpass invoked with -e" "$STUB_LOG" "ARGS=-e" yes
check "password reaches sshpass via SSHPASS env" "$STUB_LOG" "SSHPASS_ENV=secret" yes
echo "case 2: dist-upgrade requested"
export STUB_LOG="$TMP/log2"
: > "$STUB_LOG"; rm -f "$TMP/boot_calls"
bash "$TARGET" 10.0.0.1 secret 1 "$TMP/packages.txt" > "$TMP/out2" 2>&1
check "dist-upgrade issued" "$STUB_LOG" "dist-upgrade" yes
check "reboot issued" "$STUB_LOG" "systemctl reboot" yes
check "package set recorded" "$STUB_LOG" "dpkg-query" yes
check "boot_id checked" "$STUB_LOG" "boot_id" yes
check "waited for the API" "$STUB_LOG" "api2/json/version" yes
check "waited for pmxcfs" "$STUB_LOG" "/etc/pve/storage.cfg" yes
check "storage still configured" "$STUB_LOG" "pvesm set local" yes
# The stub replaces the remote shell, so it cannot observe what that shell does
# with a command — only which command was sent. These two assert at that level,
# because both defects live in the command string itself:
# - without `set -o pipefail`, a failed remote dpkg-query is masked by `sort`,
# which succeeds on empty input and makes ssh return 0.
# - bash's builtin `echo` does not interpret \t without -e, so `echo` here
# would write the one row in the file lacking a real tab.
check "query sets remote pipefail" "$STUB_LOG" "set -o pipefail" yes
check "kernel capture uses printf" "$STUB_LOG" "printf '# running-kernel" yes
# The reboot must be issued after the upgrade, or the node records a package
# set it never booted.
upgrade_line=$(grep -n "dist-upgrade" "$STUB_LOG" | head -1 | cut -d: -f1)
reboot_line=$(grep -n "systemctl reboot" "$STUB_LOG" | head -1 | cut -d: -f1)
api_line=$(grep -n "api2/json/version" "$STUB_LOG" | head -1 | cut -d: -f1)
[[ "$reboot_line" -gt "$upgrade_line" ]] \
&& pass "reboot ordered after dist-upgrade" \
|| fatal "reboot ordered before dist-upgrade"
[[ "$api_line" -gt "$reboot_line" ]] \
&& pass "API wait ordered after reboot" \
|| fatal "API wait ordered before reboot"
# The running-kernel row must carry a REAL tab, like every dpkg-query row.
# `echo "...\t..."` in bash emits a literal backslash-t and would fail here.
if grep -q '^# running-kernel' "$TMP/packages.txt"; then
pass "running kernel recorded"
if grep -qP '^# running-kernel\t' "$TMP/packages.txt" 2>/dev/null \
|| awk -F'\t' '/^# running-kernel/ && NF == 2 {found=1} END {exit !found}' "$TMP/packages.txt"; then
pass "running-kernel row uses a real tab"
else
fatal "running-kernel row has a literal backslash-t, not a tab"
fi
else
fatal "running kernel not recorded"
fi
[[ -s "$TMP/packages.txt" ]] && pass "package file non-empty" || fatal "package file empty or missing"
echo "case 3: node never rebooted — must be fatal"
export STUB_LOG="$TMP/log3"
: > "$STUB_LOG"; rm -f "$TMP/boot_calls"
if BOOT_ID_STUCK=1 bash "$TARGET" 10.0.0.1 secret 1 "$TMP/packages3.txt" > "$TMP/out3" 2>&1; then
fatal "script exited 0 despite an unchanged boot_id"
else
pass "unchanged boot_id fails the run"
grep -q "did not reboot" "$TMP/out3" \
&& pass "failure names the cause" \
|| fatal "failure message does not mention the reboot"
fi
echo "case 4: dpkg-query produced nothing — must be fatal"
export STUB_LOG="$TMP/log4"
: > "$STUB_LOG"; rm -f "$TMP/boot_calls"
if DPKG_EMPTY=1 bash "$TARGET" 10.0.0.1 secret 1 "$TMP/packages4.txt" > "$TMP/out4" 2>&1; then
fatal "script exited 0 despite an empty package set"
else
pass "empty package set fails the run"
grep -q "empty package set" "$TMP/out4" \
&& pass "failure names the cause" \
|| fatal "failure message does not mention the package set"
fi
if [[ "$fail" -eq 0 ]]; then
echo "PASS"
else
echo "FAILED"
exit 1
fi