mirror of
https://github.com/GoodOlClint/PSProxmoxVE.git
synced 2026-09-04 03:05:32 +00:00
def8dc6b67
* 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>
121 lines
4.7 KiB
Bash
Executable File
121 lines
4.7 KiB
Bash
Executable File
#!/usr/bin/env bash
|
|
# Downloads cloud image and OVA to the cache directory if not already present
|
|
# or if the cached copy is older than 7 days. Verifies both against the
|
|
# upstream Ubuntu SHA256SUMS on every check, including a fresh cache hit.
|
|
#
|
|
# Usage: ensure-cloud-images.sh <cache-dir>
|
|
#
|
|
# Outputs (for use in GITHUB_OUTPUT):
|
|
# CLOUD_IMAGE_PATH=<path>
|
|
# OVA_PATH=<path>
|
|
set -euo pipefail
|
|
|
|
CACHE_DIR="${1:?Usage: ensure-cloud-images.sh <cache-dir>}"
|
|
MAX_AGE_DAYS=7
|
|
|
|
CLOUD_IMAGE_URL="https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img"
|
|
CLOUD_IMAGE_SUMS_URL="https://cloud-images.ubuntu.com/noble/current/SHA256SUMS"
|
|
CLOUD_IMAGE_FILENAME="noble-server-cloudimg-amd64.qcow2"
|
|
|
|
OVA_URL="https://cloud-images.ubuntu.com/releases/24.04/release/ubuntu-24.04-server-cloudimg-amd64.ova"
|
|
OVA_SUMS_URL="https://cloud-images.ubuntu.com/releases/24.04/release/SHA256SUMS"
|
|
OVA_FILENAME="ubuntu-24.04-server-cloudimg-amd64.ova"
|
|
|
|
mkdir -p "${CACHE_DIR}"
|
|
|
|
# Ubuntu's SHA256SUMS lists the upstream filename, which is not always the
|
|
# name we cache under (the cloud image is published as .img and cached as
|
|
# .qcow2 — Ubuntu's .img is already qcow2-formatted). Match by upstream name,
|
|
# then check the bytes under the name they actually have on disk.
|
|
verify_checksum() {
|
|
local filepath="$1" sums_url="$2" upstream_name="$3"
|
|
local sums_file hash dir base
|
|
|
|
sums_file="$(mktemp)"
|
|
|
|
if ! curl -fsSL -o "${sums_file}" "${sums_url}"; then
|
|
echo " ERROR: failed to download ${sums_url}" >&2
|
|
rm -f "${sums_file}"
|
|
return 1
|
|
fi
|
|
|
|
hash="$(awk -v f="${upstream_name}" '$2 == f || $2 == "*" f {print $1; exit}' "${sums_file}")"
|
|
rm -f "${sums_file}"
|
|
|
|
if [ -z "${hash}" ]; then
|
|
echo " ERROR: ${upstream_name} not listed in ${sums_url}" >&2
|
|
return 1
|
|
fi
|
|
|
|
dir="$(dirname "${filepath}")"
|
|
base="$(basename "${filepath}")"
|
|
|
|
if ! printf '%s %s\n' "${hash}" "${base}" | (cd "${dir}" && sha256sum -c -); then
|
|
echo " ERROR: checksum mismatch for ${filepath}" >&2
|
|
return 1
|
|
fi
|
|
}
|
|
|
|
download_if_stale() {
|
|
local url="$1"
|
|
local filepath="$2"
|
|
local description="$3"
|
|
local sums_url="$4"
|
|
local upstream_name
|
|
upstream_name="$(basename "${url}")"
|
|
|
|
if [ -f "${filepath}" ] && [ -s "${filepath}" ]; then
|
|
# Check age
|
|
local age_days
|
|
age_days=$(( ( $(date +%s) - $(stat -c %Y "${filepath}" 2>/dev/null || stat -f %m "${filepath}" 2>/dev/null) ) / 86400 ))
|
|
if [ "${age_days}" -lt "${MAX_AGE_DAYS}" ]; then
|
|
if verify_checksum "${filepath}" "${sums_url}" "${upstream_name}"; then
|
|
echo "${description} cached and fresh (${age_days}d old): ${filepath}"
|
|
return 0
|
|
fi
|
|
# Remove it now, not just on a redownload's own failure below —
|
|
# otherwise a redownload that then fails falls through to the
|
|
# "keep the stale copy" branch and hands back these same
|
|
# known-bad bytes with exit 0.
|
|
echo "${description} cached copy failed checksum verification, removing and re-downloading..." >&2
|
|
rm -f "${filepath}"
|
|
else
|
|
echo "${description} is ${age_days}d old, re-downloading..."
|
|
fi
|
|
else
|
|
echo "Downloading ${description}..."
|
|
fi
|
|
|
|
local tmp_path="${filepath}.downloading"
|
|
if curl -fSL --progress-bar -o "${tmp_path}" "${url}"; then
|
|
if ! verify_checksum "${tmp_path}" "${sums_url}" "${upstream_name}"; then
|
|
rm -f "${tmp_path}"
|
|
echo "ERROR: ${description} failed checksum verification" >&2
|
|
return 1
|
|
fi
|
|
mv "${tmp_path}" "${filepath}"
|
|
echo "Downloaded ${description}: $(du -h "${filepath}" | cut -f1)"
|
|
else
|
|
rm -f "${tmp_path}"
|
|
# A copy that failed verification above is already gone by this
|
|
# point; a copy that's here because it was merely stale-by-age was
|
|
# never re-verified this run. Verify it now, at the point we'd
|
|
# actually hand it back — checking only here, not proactively before
|
|
# the redownload attempt, avoids deleting a copy the redownload was
|
|
# about to replace anyway.
|
|
if [ -f "${filepath}" ] && verify_checksum "${filepath}" "${sums_url}" "${upstream_name}"; then
|
|
echo "WARNING: Download failed, using stale cached copy" >&2
|
|
return 0
|
|
fi
|
|
rm -f "${filepath}"
|
|
echo "ERROR: Failed to download ${description}" >&2
|
|
return 1
|
|
fi
|
|
}
|
|
|
|
download_if_stale "${CLOUD_IMAGE_URL}" "${CACHE_DIR}/${CLOUD_IMAGE_FILENAME}" "Ubuntu cloud image" "${CLOUD_IMAGE_SUMS_URL}"
|
|
download_if_stale "${OVA_URL}" "${CACHE_DIR}/${OVA_FILENAME}" "Ubuntu OVA" "${OVA_SUMS_URL}"
|
|
|
|
echo "CLOUD_IMAGE_PATH=${CACHE_DIR}/${CLOUD_IMAGE_FILENAME}"
|
|
echo "OVA_PATH=${CACHE_DIR}/${OVA_FILENAME}"
|