diff --git a/.github/workflows/cd.yaml b/.github/workflows/cd.yaml index bfc0c5b70..a6f38505f 100644 --- a/.github/workflows/cd.yaml +++ b/.github/workflows/cd.yaml @@ -162,9 +162,18 @@ jobs: kubectl version --client - name: 🔐 Validate static production authorization controls + # Both commands run unconditionally, and the step fails if either did. + # They are independent outputs over the same surface: `go test` is the + # verdict, `go run` is the only caller of the moved-entry diagnostics. A + # real aggregate mismatch fails BOTH, so short-circuiting on the first + # left the one failure that most needs explaining with no entry names at + # all (#3879). Covered by scripts/tests/test-authorization-gate-diagnostics.sh. run: | - go test ./scripts/validate-eks-ci-role-policy - go run ./scripts/validate-eks-ci-role-policy . + set -euo pipefail + status=0 + go test ./scripts/validate-eks-ci-role-policy || status=1 + go run ./scripts/validate-eks-ci-role-policy . || status=1 + exit "$status" - name: ⚙️ Setup KSail for effective authorization validation shell: bash diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index f5e583072..b0fb6a8f9 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -230,6 +230,19 @@ jobs: shellcheck scripts/tests/test-regenerate-publish-workflow-approved-revisions.sh bash scripts/tests/test-regenerate-publish-workflow-approved-revisions.sh + # The authorization gate's verdict and its moved-entry diagnostics are two + # commands in one step, and a real aggregate mismatch fails BOTH — so a step + # that stopped at the first never ran the diagnostics on the one failure they + # exist to explain (#3879). That is a property of the STEP's shape, which no + # script it calls can enforce, so the test executes the step against stub + # binaries and ablates each assertion. Unconditional for the same reason as + # the steps above: its subjects are ci.yaml and cd.yaml themselves, which sit + # outside the k8s paths. + - name: 🔐 Validate the authorization gate reports its diagnostics + run: | + shellcheck scripts/tests/test-authorization-gate-diagnostics.sh + bash scripts/tests/test-authorization-gate-diagnostics.sh + # Both subject guards above examine a file only when it already carries a # shared-publish-workflow subject, so an OCIRepository pointed at a devantler-tech # artifact with NO spec.verify at all is invisible to them — and refused by nothing @@ -906,16 +919,24 @@ jobs: # The HEAD^1 checkout is diagnostic only: on an unapproved aggregate the # validator names the surface entries that differ from it. Without it the # verdict is identical and the moved entries are reported as unknown. + # Both commands run unconditionally, and the step fails if either did. + # They are independent outputs over the same surface: `go test` is the + # verdict, `go run` is the only caller of the moved-entry diagnostics. A + # real aggregate mismatch fails BOTH, so short-circuiting on the first + # left the one failure that most needs explaining with no entry names at + # all (#3879). Covered by scripts/tests/test-authorization-gate-diagnostics.sh. run: | set -euo pipefail - go test ./scripts/validate-eks-ci-role-policy + status=0 + go test ./scripts/validate-eks-ci-role-policy || status=1 base_root="$RUNNER_TEMP/approval-base-tree" if git worktree add --detach "$base_root" HEAD^1; then - go run ./scripts/validate-eks-ci-role-policy . "$base_root" + go run ./scripts/validate-eks-ci-role-policy . "$base_root" || status=1 else echo "::warning::approval base checkout failed; an aggregate mismatch will not name moved entries" - go run ./scripts/validate-eks-ci-role-policy . + go run ./scripts/validate-eks-ci-role-policy . || status=1 fi + exit "$status" - name: ⚙️ Setup KSail for effective authorization validation shell: bash diff --git a/scripts/tests/test-authorization-gate-diagnostics.sh b/scripts/tests/test-authorization-gate-diagnostics.sh new file mode 100755 index 000000000..8d6584f38 --- /dev/null +++ b/scripts/tests/test-authorization-gate-diagnostics.sh @@ -0,0 +1,231 @@ +#!/usr/bin/env bash +# Prove the authorization gate reports BOTH of its outputs on a real mismatch +# (devantler-tech/platform#3879). +# +# WHY THIS EXISTS. The "🔐 Validate static production authorization controls" +# step runs two independent commands over the same surface: +# +# * `go test ./scripts/validate-eks-ci-role-policy` — the verdict. On an +# unapproved aggregate TestValidateAuthorizationAcceptsCommittedPolicy +# t.Fatalf's with the bare fingerprint. +# * `go run ./scripts/validate-eks-ci-role-policy . "$base_root"` — the +# diagnostics. This is the ONLY caller of surfaceMismatchReport, the code +# #3836 added to name the surface entries that moved. +# +# A real aggregate mismatch fails BOTH. So a step that short-circuits on the +# first command never reaches the second, and the failure that most needs an +# explanation is the one reported with no entry names at all — measured on +# #3878 (CI run 35257229458, job 105324573289): the approval base checked out +# fine, `go test` failed with `a4781e58…`, and zero moved-entry lines were +# emitted. +# +# The step's script is EXECUTED here against stub `go` and `git` binaries rather +# than pattern-matched, so the assertions are about behaviour: which commands +# ran, and what the step exited with. Every assertion is ABLATED against the +# short-circuiting form the fix replaces; a check that cannot fail is not a +# check. +# +# yq (mikefarah v4) reads the YAML. No network, no secrets, no Go toolchain. +# Bash 3.2 compatible. +set -euo pipefail + +root_dir="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +readonly root_dir +readonly job='validate-eks-authorization' +readonly step='🔐 Validate static production authorization controls' +readonly diagnostic_line='changed helm.toolkit.fluxcd.io/v2|HelmRelease|kube-system|hcloud-csi' + +work_dir="$(mktemp -d)" +readonly work_dir +trap 'rm -rf "${work_dir}"' EXIT + +failures=0 + +fail() { + printf 'FAIL: %s\n' "$1" >&2 + failures=$((failures + 1)) +} + +# extract_step prints the step's `run:` script for one workflow. +extract_step() { + local workflow="$1" + yq -r \ + ".jobs[\"${job}\"].steps[] | select(.name == \"${step}\") | .run" \ + "${workflow}" +} + +# step_shell_flags prints the flags the runner gives the step's interpreter, so +# the script is exercised the way GitHub actually runs it rather than under a +# bare `bash`. A step with no `shell:` key gets `bash -e {0}`; an explicit +# `shell: bash` gets `bash --noprofile --norc -eo pipefail {0}`. The difference +# decides whether a failing first command aborts the step, which is the very +# property under test — running these under a plain `bash` reports a swallowed +# failure that the runner would never produce. +step_shell_flags() { + local workflow="$1" declared + declared="$(yq -r \ + ".jobs[\"${job}\"].steps[] | select(.name == \"${step}\") | .shell // \"\"" \ + "${workflow}")" + case "${declared}" in + bash) printf -- '-e -o pipefail' ;; + '') printf -- '-e' ;; + *) printf -- '-e' ;; + esac +} + +# make_stubs builds a PATH directory whose `go` records each invocation and +# exits with the caller's chosen status, and whose `git` always succeeds so the +# approval-base branch is taken. +# +# $1 work root $2 `go test` exit status $3 `go run` exit status +make_stubs() { + local dir="$1" test_status="$2" run_status="$3" + mkdir -p "${dir}/bin" + cat >"${dir}/bin/go" <>"${dir}/invocations" +case "\$1" in + test) printf 'FAIL\tgithub.com/devantler-tech/platform/scripts/validate-eks-ci-role-policy\n'; exit ${test_status} ;; + run) printf 'EKS CI role policy: ${diagnostic_line}\n'; exit ${run_status} ;; +esac +exit 0 +EOF + cat >"${dir}/bin/git" <<'EOF' +#!/usr/bin/env bash +exit 0 +EOF + chmod +x "${dir}/bin/go" "${dir}/bin/git" + : >"${dir}/invocations" +} + +# run_step executes a step script under the stubs and prints +# "||". +# +# $1 script path $2 `go test` exit status $3 `go run` exit status +# $4 the runner's shell flags for that step +run_step() { + local script="$1" test_status="$2" run_status="$3" shell_flags="$4" + local sandbox status output + sandbox="$(mktemp -d "${work_dir}/sandbox.XXXXXX")" + make_stubs "${sandbox}" "${test_status}" "${run_status}" + set +e + # shellcheck disable=SC2086 # shell_flags is a controlled flag list, not a path. + output="$(PATH="${sandbox}/bin:${PATH}" RUNNER_TEMP="${sandbox}" \ + bash ${shell_flags} "${script}" 2>&1)" + status=$? + set -e + printf '%s|%s|%s\n' \ + "${status}" \ + "$(tr '\n' ' ' <"${sandbox}/invocations" | sed 's/ *$//')" \ + "$(printf '%s' "${output}" | tr '\n' ' ')" +} + +# field reads one of run_step's three fields. The captured output is LAST and is +# read with an open-ended range, because a surface entry is itself pipe-separated +# (`apiVersion|Kind|namespace|name`) — a fixed `-f3` truncates the diagnostics at +# their first separator and reports a line that was printed as missing. +field() { + case "$2" in + 3) printf '%s' "$1" | cut -d'|' -f3- ;; + *) printf '%s' "$1" | cut -d'|' -f"$2" ;; + esac +} + +# assert_gate_behaviour runs the four cases every copy of the step must satisfy. +# +# $1 label $2 script path $3 shell flags +# $4 "diagnostics" to require the moved-entry line +assert_gate_behaviour() { + local label="$1" script="$2" shell_flags="$3" mode="${4:-}" + local result + + # Case A — a real aggregate mismatch: both commands fail. The step must run + # the diagnostics anyway, and must still fail. + result="$(run_step "${script}" 1 1 "${shell_flags}")" + case " $(field "${result}" 2) " in + *' run '*) : ;; + *) fail "${label}: a failing 'go test' short-circuited the step before 'go run' — the moved-entry diagnostics never execute on the one failure they exist to explain (got invocations: '$(field "${result}" 2)')" ;; + esac + [ "$(field "${result}" 1)" != "0" ] || + fail "${label}: the step exited 0 while both commands failed" + if [ "${mode}" = "diagnostics" ]; then + case "$(field "${result}" 3)" in + *"${diagnostic_line}"*) : ;; + *) fail "${label}: the moved-entry diagnostics were not reported (got: '$(field "${result}" 3)')" ;; + esac + fi + + # Case B — negative control: `go test` fails for a reason unrelated to the + # aggregate, so `go run` passes. Running both must not swallow the failure. + result="$(run_step "${script}" 1 0 "${shell_flags}")" + [ "$(field "${result}" 1)" != "0" ] || + fail "${label}: a failing 'go test' was swallowed when 'go run' passed — the step reports success on a broken validator" + + # Case C — the mirror of B, and the half of "the step fails if either did" that + # nothing else here covers: the verdict passes and only the diagnostics fail. + # Cases A, B and D are all satisfied by a step that ignores `go run` entirely, + # so dropping `|| status=1` from either `go run` line would otherwise land + # silently — and the diagnostics this gate exists for would stop being checked. + result="$(run_step "${script}" 0 1 "${shell_flags}")" + [ "$(field "${result}" 1)" != "0" ] || + fail "${label}: a failing 'go run' was swallowed when 'go test' passed — the step reports success although the moved-entry diagnostics did not run" + + # Case D — control: both pass, so the step passes. + result="$(run_step "${script}" 0 0 "${shell_flags}")" + [ "$(field "${result}" 1)" = "0" ] || + fail "${label}: the step failed while both commands passed (exit $(field "${result}" 1): '$(field "${result}" 3)')" +} + +for workflow_name in ci cd; do + workflow="${root_dir}/.github/workflows/${workflow_name}.yaml" + [ -f "${workflow}" ] || { fail "${workflow_name}.yaml is missing"; continue; } + + script="${work_dir}/${workflow_name}-step.sh" + extract_step "${workflow}" >"${script}" + shell_flags="$(step_shell_flags "${workflow}")" + if [ ! -s "${script}" ] || grep -qx 'null' "${script}"; then + fail "${workflow_name}.yaml: job '${job}' has no step named '${step}' — the gate this test pins was renamed or removed" + continue + fi + grep -q 'validate-eks-ci-role-policy' "${script}" || + fail "${workflow_name}.yaml: the extracted step does not invoke the authorization validator" + + # ci.yaml checks out the approval base, so its diagnostics name the entries. + if grep -q 'approval-base-tree' "${script}"; then + assert_gate_behaviour "${workflow_name}.yaml" "${script}" "${shell_flags}" diagnostics + else + assert_gate_behaviour "${workflow_name}.yaml" "${script}" "${shell_flags}" + fi +done + +# ABLATION. The short-circuiting form this fix replaces must fail Case A, and +# must fail naming the short-circuit rather than something incidental. A test +# that passes against the defect proves nothing. +ablation="${work_dir}/ablation-step.sh" +cat >"${ablation}" <<'EOF' +set -euo pipefail +go test ./scripts/validate-eks-ci-role-policy +base_root="$RUNNER_TEMP/approval-base-tree" +if git worktree add --detach "$base_root" HEAD^1; then + go run ./scripts/validate-eks-ci-role-policy . "$base_root" +else + go run ./scripts/validate-eks-ci-role-policy . +fi +EOF +ablation_result="$(run_step "${ablation}" 1 1 '-e')" +case " $(field "${ablation_result}" 2) " in + *' run '*) + fail "ABLATION: the short-circuiting step reached 'go run', so Case A cannot detect the defect it is written for" + ;; + *) : ;; +esac +[ "$(field "${ablation_result}" 1)" != "0" ] || + fail "ABLATION: the short-circuiting step exited 0, so Case B's control is not exercised" + +if [ "${failures}" -ne 0 ]; then + printf '%s: %d assertion(s) failed\n' "$(basename "$0")" "${failures}" >&2 + exit 1 +fi + +printf '%s: authorization gate reports its verdict AND its diagnostics in ci.yaml and cd.yaml\n' \ + "$(basename "$0")"