diff --git a/scripts/probe-image-signature-enforcement.sh b/scripts/probe-image-signature-enforcement.sh index 8f4c9c5458..d0b8b71f38 100755 --- a/scripts/probe-image-signature-enforcement.sh +++ b/scripts/probe-image-signature-enforcement.sh @@ -360,11 +360,24 @@ printf 'probe: on node %s the unsigned ref matches rule %s and the signed contro image_list="$("${talosctl_bin}" -n "${node}" image list --namespace "${ns}" 2>/dev/null)" || fail_inconclusive "could not list images on node ${node} — cannot establish that the probe refs are absent" +# Substring match on purpose: `image list` renders a ref plus its digest and +# size, so an exact line match would miss it. +# +# The text goes in through a here-string, NEVER a pipe. `grep -q` stops reading +# at its first match. GitHub Actions ignores SIGPIPE, so a piped `printf` still +# writing a large list then fails with "write error: Broken pipe", and under +# `pipefail` that failure becomes the pipeline's status: a MATCH is reported as +# NO match. On the cache guard that let a ref the node already held through as +# absent. Measured in scripts/tests: restoring the pipe turns the cached-ref +# INCONCLUSIVE into a FAIL. +list_contains() { + grep -qF -- "$2" <<<"$1" +} + ref_is_cached() { - # Substring match on purpose: `image list` renders a ref plus its digest and - # size, so an exact line match would miss it. The consequence of a false - # positive here is a fail-closed INCONCLUSIVE, never a false PASS. - printf '%s' "${image_list}" | grep -qF -- "$1" + # The consequence of a false positive here is a fail-closed INCONCLUSIVE, + # never a false PASS. + list_contains "${image_list}" "$1" } for ref in "${unsigned_image}" "${signed_image}"; do @@ -414,9 +427,18 @@ cleanup() { return "${rc}" fi + local remaining for ref in ${pulled_refs[@]+"${pulled_refs[@]}"}; do if "${talosctl_bin}" -n "${node}" image remove --namespace "${ns}" "${ref}" >/dev/null 2>&1; then printf 'probe: cleaned up %s\n' "${ref}" >&2 + continue + fi + # A refused pull stores nothing, so removal fails because there is nothing + # to remove. Only a fresh listing that succeeds and no longer shows the ref + # counts as absent; anything else keeps the manual-cleanup warning. + if remaining="$("${talosctl_bin}" -n "${node}" image list --namespace "${ns}" 2>/dev/null)" && + ! list_contains "${remaining}" "${ref}"; then + printf 'probe: %s not present on node %s, nothing to clean up\n' "${ref}" "${node}" >&2 else printf 'probe: WARNING could not remove %s from node %s — remove it by hand\n' "${ref}" "${node}" >&2 fi @@ -450,10 +472,11 @@ trap cleanup EXIT # allowlist does not recognise yields INCONCLUSIVE — the safe direction for a # probe whose job is never to report an unfounded PASS. is_verification_refusal() { - if printf '%s' "$1" | grep -qiE 'x509|certificate|tls:'; then + # Here-strings, not pipes, for the reason given at list_contains. + if grep -qiE 'x509|certificate|tls:' <<<"$1"; then return 1 fi - printf '%s' "$1" | grep -qiE 'image verification|signature verification|no valid signature|no matching signature|not signed|unsigned image|cosign|sigstore' + grep -qiE 'image verification|signature verification|no valid signature|no matching signature|not signed|unsigned image|cosign|sigstore' <<<"$1" } # Pull errors routinely REPEAT the ref, and the recommended negative-control name diff --git a/scripts/tests/test-probe-image-signature-enforcement.sh b/scripts/tests/test-probe-image-signature-enforcement.sh index b046683cd8..765836bb2d 100755 --- a/scripts/tests/test-probe-image-signature-enforcement.sh +++ b/scripts/tests/test-probe-image-signature-enforcement.sh @@ -115,6 +115,16 @@ case "${verb}" in fi case "${sub}" in list) + # Once a removal has been attempted, a case can stage what the node + # holds afterwards, so cleanup's re-check sees a different store from + # the cache guard's first read. + if [[ -e "${fixtures}/removed.txt" ]]; then + [[ -e "${fixtures}/imagelist_after_remove_fails" ]] && exit 1 + if [[ -e "${fixtures}/imagelist_after_remove.txt" ]]; then + cat "${fixtures}/imagelist_after_remove.txt" + exit 0 + fi + fi [[ -e "${fixtures}/imagelist_fails" ]] && exit 1 if [[ -e "${fixtures}/imagelist.txt" ]]; then cat "${fixtures}/imagelist.txt" @@ -136,6 +146,7 @@ case "${verb}" in remove) ref="${1:-}" printf '%s\n' "${ref}" >>"${fixtures}/removed.txt" + [[ -e "${fixtures}/remove_fails" ]] && exit 1 exit 0 ;; esac @@ -691,4 +702,84 @@ run_probe [[ ! -e "${fixtures}/removed.txt" ]] || fail 'probe removed an image despite never pulling one' check 'a failure before any pull removes nothing' +# A refused pull stores nothing, so removing that ref fails simply because it is +# not there. That must not tell the operator to clean a production node by hand: +# a warning on every passing run hides the run where removal genuinely failed. +reset_fixtures +stage_pull "${unsigned}" 1 'image verification failed: no valid signature found' +stage_pull "${signed}" 0 '' +: >"${fixtures}/remove_fails" +: >"${fixtures}/imagelist_after_remove.txt" +run_probe +[[ ${probe_rc} -eq 0 ]] || fail "a refused, absent ref should still PASS, got ${probe_rc}: ${probe_out}" +refute_text "${probe_out}" 'WARNING could not remove' 'refused, absent ref' +require_text "${probe_out}" 'not present' 'refused, absent ref' +check 'a refused ref that was never stored is reported not present, without a manual-cleanup warning' + +# The control for the case above: the same failed removal while the node still +# holds the ref IS a real cleanup failure, and still warns. The verdict is +# unchanged either way. +reset_fixtures +stage_pull "${unsigned}" 1 'image verification failed: no valid signature found' +stage_pull "${signed}" 0 '' +: >"${fixtures}/remove_fails" +printf '%s sha256:%s 1.0 MB\n' "${unsigned}" "${unsigned_hex}" >"${fixtures}/imagelist_after_remove.txt" +run_probe +[[ ${probe_rc} -eq 0 ]] || fail "a failed cleanup must not change the PASS verdict, got ${probe_rc}: ${probe_out}" +require_text "${probe_out}" 'WARNING could not remove' 'present ref whose removal fails' +check 'a ref still present after a failed removal warns, and the verdict is unchanged' + +# If the node cannot be listed after a failed removal, absence is unproven, so +# the warning stays. +reset_fixtures +stage_pull "${unsigned}" 1 'image verification failed: no valid signature found' +stage_pull "${signed}" 0 '' +: >"${fixtures}/remove_fails" +: >"${fixtures}/imagelist_after_remove_fails" +run_probe +[[ ${probe_rc} -eq 0 ]] || fail "an unlistable node must not change the PASS verdict, got ${probe_rc}: ${probe_out}" +require_text "${probe_out}" 'WARNING could not remove' 'unlistable node after failed removal' +refute_text "${probe_out}" 'not present' 'unlistable node after failed removal' +check 'a failed removal on a node that cannot be re-listed still warns' + +# --- A large image list under the runner's signal disposition ---------------- +# GitHub Actions runs steps with SIGPIPE ignored, so a writer whose reader exits +# early gets EPIPE instead of dying silently. A presence check that pipes a large +# image list into `grep -q` then logs "write error: Broken pipe" as soon as grep +# matches, and under `pipefail` the writer's failure becomes the pipeline's +# status: the match is reported as NO match. The cache guard then passes a ref +# the node already holds, which is exactly the false result it exists to +# prevent. Reproduce the runner's disposition and a list larger than a pipe +# buffer, with the ref on the first line. +run_probe_sigpipe_ignored() { + local out rc=0 + set +e + out="$( + trap '' PIPE + "${script}" --confirm --node "${node}" \ + --unsigned-image "${unsigned}" --signed-image "${signed}" "$@" 2>&1 + )" + rc=$? + set -e + probe_out="${out}" + probe_rc="${rc}" +} + +large_image_list() { + local i + printf '%s sha256:%s 1.0 MB\n' "$1" "${unsigned_hex}" + for ((i = 0; i < 3000; i++)); do + printf 'ghcr.io/example/filler-%05d:v1 sha256:%s 1.0 MB\n' "${i}" "${unsigned_hex}" + done +} + +reset_fixtures +large_image_list "${unsigned}" >"${fixtures}/imagelist.txt" +run_probe_sigpipe_ignored +[[ ${probe_rc} -eq 3 ]] || fail "a cached ref in a large list should exit 3, got ${probe_rc}: ${probe_out}" +refute_text "${probe_out}" 'both refs confirmed absent' 'large image list with an early match' +refute_text "${probe_out}" 'Broken pipe' 'large image list with an early match' +refute_text "${probe_out}" 'write error' 'large image list with an early match' +check 'a large image list with an early match produces no broken-pipe write error' + printf '\nAll %d case(s) passed: probe-image-signature-enforcement.sh behaviour is pinned.\n' "${cases_run}"