From 94d2acca77670773dd3b89185e7937b049fefbf0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 04:01:47 +0200 Subject: [PATCH 1/2] fix(ci): key rollback-aws-fargate on the Fargate Terraform state (#1811) `rollback.yml`'s `rollback-aws-fargate` built its backend key from the Lambda state namespace, `github-/terraform.tfstate`, and then applied `-var="compute_platform=fargate"` into it. Every other Fargate writer (deploy-aws-fargate.yml, cleanup-staging.yml's destroy-aws-fargate, destroy-fargate-dev.yml) uses `github-fargate-/`, and `rollback-aws-lambda` uses the bare key correctly, so the two rollback jobs were writing the same object. Against a populated Lambda state a Fargate rollback is a platform swap of the live Lambda stack recorded in the wrong state file, while the real Fargate state is left describing resources nobody reconciles. The concurrency group moves with the key in the same commit, per #1806: the group has to name the object the job locks, and the note left on the job when #1806 landed said it would move when this key did. Nothing in Terraform ties the backend key to the platform. The key is a string a step builds by hand and the platform is a `-var` passed several steps later, so a wrong pairing initialises, plans and applies cleanly. scripts/test-aws-tfstate-platform-key.sh asserts the pairing as text: - the seven real AWS jobs that apply a `compute_platform` are named and checked first, since a scan recognizing nothing has no violations either - the negative half sweeps a GLOB of .github/workflows and scripts/, so a job added later is covered without anyone listing it - the concurrency group is checked on the same axis, so it cannot drift back from the key alone Sites are delimited by job, not by step: deploy-aws-fargate.yml writes the backend file in "Terraform Init" and passes the platform in "Terraform Plan", and a step-delimited scan would pair neither. Verified by mutating a copy of the tree, seven cases, each requiring the specific FAIL line rather than a non-zero exit: rollback.yml reverted wholesale to origin/main; the key alone reverted; the concurrency group alone reverted; the same defect introduced in deploy-aws-fargate.yml; a brand-new workflow file carrying it; the fargate job silently becoming a lambda apply; and an emptied workflow directory. Runs clean under both bash 3.2 with BWK awk and mawk, and shellcheck reports nothing. --- .github/workflows/ci.yml | 35 ++ .github/workflows/rollback.yml | 28 +- scripts/lib/code-scan-awk.sh | 28 +- scripts/test-aws-tfstate-platform-key.sh | 542 +++++++++++++++++++++++ 4 files changed, 607 insertions(+), 26 deletions(-) create mode 100755 scripts/test-aws-tfstate-platform-key.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 78bf0d988..db253ee91 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -824,6 +824,40 @@ jobs: - name: Run RDS scope self-tests run: bash scripts/test-rds-deletion-protection-scope.sh + # Assert that every job applying a `compute_platform` writes the Terraform + # state namespace that platform owns: `compute_platform=lambda` into + # github-/, `compute_platform=fargate` into github-fargate-/. The + # AWS environment is one Terraform root applied twice into two state objects, + # nothing in Terraform ties the backend key to the platform, and a job that + # pairs them wrongly initialises, plans and applies cleanly while rewriting + # the other platform's stack and recording it in the wrong state file. That + # was #1811, where rollback.yml's Fargate rollback keyed on the Lambda + # namespace, so both rollback jobs wrote the same object. A workflow run + # proves nothing about this, so the pairing is asserted as text. + # Both directions are asserted, and the positive one first: the seven real + # pairings are named and checked before any absence, since a scan that + # recognizes no state-writing job has no violations either. The negative half + # is checked over a GLOB of .github/workflows and scripts/, not a list of + # known files, so a job added later is covered without anyone naming it. + # Fast (shell only), so it always runs. + aws-tfstate-platform-key: + name: AWS Terraform state namespace per platform + runs-on: ubuntu-latest + # Same shape as ecr-delete-selection above: this job checks out the tree and + # runs a shell script against it, so the repository-default read/write token + # is narrowed to `contents: read`. + permissions: + contents: read + + steps: + - name: Checkout code + uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 + with: + persist-credentials: false + + - name: Run state namespace self-tests + run: bash scripts/test-aws-tfstate-platform-key.sh + # Assert that no Terraform file declares an azurerm_key_vault_access_policy # resource. This project's only Key Vault sets enable_rbac_authorization = # true, and an RBAC-enabled vault ignores access policies entirely, so such a @@ -863,6 +897,7 @@ jobs: - gcp-secret-scope - ecr-delete-selection - rds-deletion-protection-scope + - aws-tfstate-platform-key - azure-kv-access-policy if: always() diff --git a/.github/workflows/rollback.yml b/.github/workflows/rollback.yml index 37f6fd7ad..6da5d4e27 100644 --- a/.github/workflows/rollback.yml +++ b/.github/workflows/rollback.yml @@ -273,19 +273,21 @@ jobs: timeout-minutes: 30 needs: validate if: inputs.cloud == 'aws-fargate' - # NOTE the group is `aws-tfstate-*`, NOT `aws-fargate-tfstate-*`. Despite - # the job name, the state key this job builds below is - # `github-/terraform.tfstate` -- the LAMBDA namespace -- not - # `github-fargate-/` as deploy-aws-fargate.yml uses. The group - # must name the object this job actually locks, or it would serialize - # against a state file it never touches while writing one unguarded, which - # is #1806 reproduced in a new place. That namespace mismatch is a real - # pre-existing defect (a Fargate rollback applies into the Lambda state), - # tracked in #1811 rather than changed here, because moving the key changes - # which infrastructure a rollback rewrites. When #1811 lands, this group - # moves to `aws-fargate-tfstate-*` in the same commit as the key. + # `terraform apply` against + # s3:///github-fargate-/terraform.tfstate, the same + # object deploy-aws-fargate.yml, cleanup-staging.yml and + # destroy-fargate-dev.yml write, so it takes the same concurrency group + # (#1806). Until #1811 this job built the key from the LAMBDA namespace + # (`github-/`) and so carried `aws-tfstate-*` to match; the + # group and the key move together, because a group that does not name the + # object the job locks serializes against a state file it never touches + # while writing one unguarded. `inputs.environment` is a required `choice` + # constrained to dev|staging|prod, so the suffix is never empty; it is the + # same value this job interpolates into the state key below. + # scripts/test-aws-tfstate-platform-key.sh is what keeps the key and the + # `compute_platform` this job applies from drifting apart again. concurrency: - group: aws-tfstate-${{ inputs.environment }} + group: aws-fargate-tfstate-${{ inputs.environment }} cancel-in-progress: false permissions: id-token: write @@ -343,7 +345,7 @@ jobs: IMAGE_URI: ${{ needs.validate.outputs.image_uri }} run: | set -euo pipefail - printf '%s\nkey = "github-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend + printf '%s\nkey = "github-fargate-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend cd terraform/environments/aws terraform init -backend-config=/tmp/backend.tfbackend diff --git a/scripts/lib/code-scan-awk.sh b/scripts/lib/code-scan-awk.sh index 0b34e2a59..af96619a0 100644 --- a/scripts/lib/code-scan-awk.sh +++ b/scripts/lib/code-scan-awk.sh @@ -2,13 +2,14 @@ # code-scan-awk.sh # # Shared awk helper functions for the guard suites that scan workflow and shell -# sources for destructive commands: test-ecr-delete-selection.sh and -# test-rds-deletion-protection-scope.sh. Sourced, not executed; it defines one -# variable, AWK_CODE_FUNCS, to be prepended to an awk program. +# sources for what a step actually runs: test-ecr-delete-selection.sh, +# test-rds-deletion-protection-scope.sh and test-aws-tfstate-platform-key.sh. +# Sourced, not executed; it defines one variable, AWK_CODE_FUNCS, to be +# prepended to an awk program. # # Shared rather than copied because these functions encode the rule that -# separates code that RUNS a command from prose that only mentions it, and both -# suites are wrong in the same way if that rule drifts in one of them. A guard +# separates code that RUNS a command from prose that only mentions it, and every +# suite is wrong in the same way if that rule drifts in one of them. A guard # that fires on a comment constrains what may be WRITTEN about a command, which # is the "the string is present somewhere" mistake the selector these suites # guard exists to remove, one level up. @@ -53,19 +54,19 @@ # build_swept_scripts SCRIPTS_DIR # # Sets SWEPT_SCRIPTS to every `*.sh` directly under SCRIPTS_DIR and under -# SCRIPTS_DIR/lib, excluding the guard suites themselves. +# SCRIPTS_DIR/lib, excluding the three guard suites themselves. # -# Globbed rather than named file by file, in both suites, because naming the two +# Globbed rather than named file by file, in every suite, because naming the # scripts already known to be guarded is the same defect the suites exist to # catch, one level up: a NEW script running the dangerous command without the # selector is invisible to a sweep that only ever opens the files someone # remembered to list, which is how a guard fails to reach a sibling site. # -# The guard suites are excluded by basename because each carries both its -# dangerous command and the selector as fixture data and inside awk programs, so -# sweeping them reports a suite as a violation of itself. Matching on basename -# rather than on a path fragment keeps the exclusion from exempting a real -# script that merely sits beside them. +# The guard suites are excluded by basename because each carries the very +# pattern it looks for as fixture data and inside awk programs, so sweeping them +# reports a suite as a violation of itself. Matching on basename rather than on +# a path fragment keeps the exclusion from exempting a real script that merely +# sits beside them. # # `nullglob` so a pattern matching nothing expands to nothing rather than to the # literal pattern text. Without it an unmatched glob becomes a nonexistent path, @@ -81,7 +82,8 @@ build_swept_scripts() { shopt -s nullglob for candidate in "$dir"/*.sh "$dir"/lib/*.sh; do case "$(basename "$candidate")" in - test-rds-deletion-protection-scope.sh | test-ecr-delete-selection.sh) continue ;; + test-rds-deletion-protection-scope.sh | test-ecr-delete-selection.sh | \ + test-aws-tfstate-platform-key.sh) continue ;; esac SWEPT_SCRIPTS+=("$candidate") done diff --git a/scripts/test-aws-tfstate-platform-key.sh b/scripts/test-aws-tfstate-platform-key.sh new file mode 100755 index 000000000..16ad56639 --- /dev/null +++ b/scripts/test-aws-tfstate-platform-key.sh @@ -0,0 +1,542 @@ +#!/usr/bin/env bash +# test-aws-tfstate-platform-key.sh +# +# Asserts that every job which applies a `compute_platform` writes the Terraform +# state namespace that platform owns, and is serialized by the concurrency group +# that names the object it writes. +# +# The AWS environment is one Terraform root applied twice into two different +# state objects, one per compute platform: +# +# compute_platform=lambda s3:///github-/terraform.tfstate +# compute_platform=fargate s3:///github-fargate-/terraform.tfstate +# +# Nothing in Terraform ties those together. The backend key is a string a +# workflow step builds by hand, the platform is a `-var` passed several steps +# later, and a job that pairs them wrongly initialises fine, plans fine and +# applies fine. It just rewrites the other platform's stack and records the +# result in a state file that platform's own deploys then disagree with, while +# the state it was supposed to write is left describing resources nobody +# reconciles. That was #1811: `rollback.yml`'s `rollback-aws-fargate` built the +# key from the LAMBDA namespace and applied `compute_platform=fargate` into it, +# so the two rollback jobs wrote the same object. +# +# The `concurrency` group is checked on the same axis. #1806 keyed every group +# on the state object its job locks, so a group naming a different namespace +# from the key means one of the two moved alone: the job then serializes against +# a state file it never touches while writing one unguarded. Only groups in the +# `aws-tfstate-*` / `aws-fargate-tfstate-*` families are checked; the Azure and +# GCP groups guard state objects that have no platform split. +# +# A green workflow run proves nothing about this, which is why the pairing is +# asserted here as text rather than left to a live rollback to discover. +# +# Both directions are asserted, and the positive one first: a sweep that +# recognizes no state-writing job at all has no violations either, and would +# pass while every pairing in the repository was wrong. +# +# Exits 0 when all cases pass; exits 1 on any failure. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)" +WORKFLOW_DIR="${REPO_ROOT}/.github/workflows" + +# code_of(), plus build_swept_scripts() for the scripts/ half of the sweep. +# Shared with test-ecr-delete-selection.sh and +# test-rds-deletion-protection-scope.sh so the rule separating code that RUNS a +# command from prose that mentions it has one definition rather than three that +# drift. +# shellcheck source=scripts/lib/code-scan-awk.sh +. "${SCRIPT_DIR}/lib/code-scan-awk.sh" + +pass=0 +fail=0 + +note_pass() { + echo "PASS: $1" + ((pass++)) || true +} + +note_fail() { + echo "FAIL: $1" + ((fail++)) || true +} + +# --- The scanner ------------------------------------------------------------- +# +# scan_platform_keys MODE DIR [FILE...] +# +# Sites are delimited by top-level YAML job headers -- a line at exactly two +# spaces of indent holding an identifier and nothing else. NOT by `- name:` +# steps: deploy-aws-fargate.yml writes the backend file in "Terraform Init" and +# passes `-var="compute_platform=fargate"` in "Terraform Plan", so a +# step-delimited scan would see a key with no platform and a platform with no +# key and pair neither. A job is the smallest unit that always holds both, and +# nothing inside a job body sits at two-space indent, so no job can be split in +# half by a spurious boundary. A shell script has no such header and is scanned +# as one site, reported as "whole file". +# +# MODE=violations prints one line per job whose key disagrees with its platform +# or with its concurrency group, plus a line of its own when the scanned set +# holds no platform-applying job at all. No output means the scanned set is +# clean. +# +# MODE=census prints `file|job|key-namespace|platform|group-namespace` for every +# job that applies a platform, whatever the verdict. It is what the positive +# assertions read, so "clean" cannot mean "recognized nothing". +# +# Kept as one awk program in two modes rather than two, because a census that +# recognized jobs differently from the sweep would assert the wrong thing is +# covered. +scan_platform_keys() { + local mode="$1" + local dir="$2" + shift 2 + local files=() + local extra + + shopt -s nullglob + files=("${dir}"/*.yml "${dir}"/*.yaml) + shopt -u nullglob + + if [[ ${#files[@]} -eq 0 ]]; then + echo "no workflow files found under ${dir}" + return + fi + + for extra in "$@"; do + if [[ ! -f "$extra" ]]; then + echo "swept file not found: ${extra}" + return + fi + files+=("$extra") + done + + # Ternaries are parenthesized wherever they appear in an argument list or a + # `return`: unparenthesized, BWK awk (macOS) rejects them at parse time while + # mawk (CI) accepts them, so an unparenthesized one passes locally under mawk + # and never runs on a developer machine, or the reverse. + # + # The key is matched on code_of() alone, with string literals left intact -- + # unlike invokes(), which empties them. The key IS a quoted literal + # (`key = "github-fargate-%s/..."` inside a printf format), so emptying + # literals would erase the only thing being asserted. + # + # A site is reported from site_file rather than FILENAME: a site running to + # the end of its file is flushed by the next file's first line, by which point + # FILENAME has advanced and the report would name an innocent file. + awk -v SQ="'" -v mode="$mode" "$AWK_CODE_FUNCS"' + function finish( expected, shown) { + if (platform != "") { + total++ + shown = (ns == "" ? "NONE" : ns) + if (mode == "census") { + printf "%s|%s|%s|%s|%s\n", basename(site_file), site, shown, platform, (grp == "" ? "NONE" : grp) + } else if (platform == "MIXED") { + printf "%s: %s: more than one compute_platform value in one job\n", basename(site_file), label() + } else if (ns == "") { + printf "%s: %s: compute_platform=%s with no AWS github-* backend state key in the same job\n", basename(site_file), label(), platform + } else if (ns == "MIXED") { + printf "%s: %s: more than one AWS state namespace in one job\n", basename(site_file), label() + } else { + expected = (platform == "fargate") ? "fargate" : ((platform == "lambda") ? "lambda" : "") + if (expected == "") + printf "%s: %s: unrecognized compute_platform=%s\n", basename(site_file), label(), platform + else if (ns != expected) + printf "%s: %s: compute_platform=%s applies into the %s state namespace\n", basename(site_file), label(), platform, nsdesc(ns) + } + } + if (mode != "census" && grp != "" && ns != "" && ns != "MIXED" && grp != ns) + printf "%s: %s: concurrency group aws%s-tfstate-* guards a job writing the %s state namespace\n", basename(site_file), label(), (grp == "fargate" ? "-fargate" : ""), nsdesc(ns) + site = ""; ns = ""; platform = ""; grp = "" + } + function label() { return (site == "" ? "whole file" : ("job \"" site "\"")) } + function nsdesc(n) { return (n == "fargate" ? "github-fargate-/ (fargate)" : "github-/ (lambda)") } + function basename(p) { sub(/^.*\//, "", p); return p } + function note_ns(n) { ns = ((ns == "" || ns == n) ? n : "MIXED") } + function note_grp(n) { grp = ((grp == "" || grp == n) ? n : "MIXED") } + + FNR == 1 { if (NR > 1) finish(); site_file = FILENAME } + + { + code = code_of($0) + + # The job boundary is tested on the comment-stripped line so a header + # carrying a trailing comment still ends the previous job. Missed, the + # key and platform of the job after it merge into the previous one. + if (code ~ /^ [A-Za-z0-9_.-]+:[[:space:]]*$/) { + finish() + site = code + sub(/^[[:space:]]*/, "", site) + sub(/:[[:space:]]*$/, "", site) + next + } + + # AWS S3 keys end in `/terraform.tfstate`; the Azure blob is + # `github-.terraform.tfstate`, one namespace with no platform split. + # Requiring the slash keeps the Azure jobs out of both checks below rather + # than classifying them as the Lambda namespace and then demanding an + # `aws-tfstate-*` group for them. + if (code ~ /key[[:space:]]*=[[:space:]]*"github-fargate-[^"]*\/terraform\.tfstate/) note_ns("fargate") + else if (code ~ /key[[:space:]]*=[[:space:]]*"github-[^"]*\/terraform\.tfstate/) note_ns("lambda") + if (code ~ /^[[:space:]]*group:[[:space:]]*aws-fargate-tfstate-/) note_grp("fargate") + else if (code ~ /^[[:space:]]*group:[[:space:]]*aws-tfstate-/) note_grp("lambda") + if (match(code, /compute_platform=[A-Za-z0-9_-]+/)) { + found = substr(code, RSTART + 17, RLENGTH - 17) + platform = ((platform == "" || platform == found) ? found : "MIXED") + } + } + + END { + finish() + if (total == 0 && mode != "census") + print "no job applying a compute_platform found at all" + } + ' "${files[@]}" +} + +# assert_scan LABEL EXPECTED DIR [FILE...] +# +# EXPECTED empty asserts the sweep finds nothing; otherwise it asserts EXPECTED +# appears in the report, so a fixture pins WHICH job was flagged rather than +# only that something was. A syntax error in the awk above also produces a +# non-empty report, and matching the text is what tells the two apart. +assert_scan() { + local label="$1" + local expected="$2" + local dir="$3" + shift 3 + local report line + + report="$(scan_platform_keys violations "$dir" "$@")" + + if [[ -z "$expected" && -z "$report" ]] || [[ -n "$expected" && "$report" == *"$expected"* ]]; then + note_pass "$label" + else + note_fail "$label" + if [[ -z "$expected" ]]; then + echo " expected no findings, got:" + else + echo " expected a finding containing '${expected}', got:" + fi + if [[ -z "$report" ]]; then + echo " (no findings)" + else + while IFS= read -r line; do + echo " ${line}" + done <<<"$report" + fi + fi +} + +# --- Positive direction FIRST: the real pairings are seen and are right ------- +# +# Every AWS job that applies a compute_platform, with the namespace it must +# write. Named explicitly, and asserted BEFORE any absence: if the scanner stops +# recognizing these, the sweep below reports a clean result for a repository it +# no longer understands. The sweep is the half that covers jobs nobody named. +EXPECTED_PAIRS=$( + cat <<'EOF' +deploy-aws-fargate.yml|deploy|fargate|fargate|fargate +deploy-aws-lambda.yml|build-and-deploy|lambda|lambda|lambda +cleanup-staging.yml|destroy-aws-fargate|fargate|fargate|fargate +cleanup-staging.yml|destroy-aws-lambda|lambda|lambda|lambda +destroy-fargate-dev.yml|destroy|fargate|fargate|fargate +rollback.yml|rollback-aws-fargate|fargate|fargate|fargate +rollback.yml|rollback-aws-lambda|lambda|lambda|lambda +EOF +) + +CENSUS="$(scan_platform_keys census "$WORKFLOW_DIR")" + +# Counted by the `file|job|namespace|platform` shape rather than by line, so the +# scanner's own error lines ("no workflow files found under ...") are not +# counted as jobs. A plain `wc -l` reported an emptied workflow directory as one +# recognized job, which is the "no violations by looking at nothing" reading +# this count exists to close. +CENSUS_COUNT=$(printf '%s\n' "$CENSUS" | grep -cE '^[^|]+\|[^|]*(\|[^|]+){3}$' || true) + +if [[ "$CENSUS_COUNT" -eq 0 ]]; then + note_fail "the scan recognizes no state-writing job under ${WORKFLOW_DIR}" + echo " a scan that recognizes nothing has no violations either, so every" + echo " assertion below would pass over a repository it never read" +else + note_pass "the scan recognizes ${CENSUS_COUNT} job(s) that apply a compute_platform" +fi + +while IFS= read -r expected_pair; do + [[ -z "$expected_pair" ]] && continue + if printf '%s\n' "$CENSUS" | grep -Fxq "$expected_pair"; then + note_pass "${expected_pair} pairs as expected" + else + note_fail "expected pairing not found: ${expected_pair}" + echo " format is file|job|state-namespace|compute_platform|concurrency-namespace;" + echo " the census holds:" + if [[ -z "$CENSUS" ]]; then + echo " (nothing)" + else + while IFS= read -r line; do + echo " ${line}" + done <<<"$CENSUS" + fi + fi +done <<<"$EXPECTED_PAIRS" + +# --- Negative direction: nothing anywhere pairs them wrongly ------------------ +# +# The assertions above name the jobs they know about, so a NEW job in a new +# workflow is invisible to them, which is the mode that produced #1592, then +# #1820, then #1821: the guard landed on one site and not its sibling. This +# sweep is keyed on the pairing instead of on a name, over a GLOB of +# .github/workflows plus every script under scripts/, so a file added later is +# covered without anyone remembering to list it. Both workflow extensions GitHub +# accepts are swept, so a new `.yaml` cannot slip past. +build_swept_scripts "$SCRIPT_DIR" + +if [[ ${#SWEPT_SCRIPTS[@]} -eq 0 ]]; then + note_fail "the scripts/ half of the swept set is empty" + echo " ${SCRIPT_DIR}/*.sh matched nothing, so the sweep below would report a" + echo " clean result for files it never opened" + assert_scan "every compute_platform job writes the state namespace that platform owns" \ + "" "$WORKFLOW_DIR" +else + note_pass "the swept set holds ${#SWEPT_SCRIPTS[@]} script(s) under scripts/" + assert_scan "every compute_platform job writes the state namespace that platform owns" \ + "" "$WORKFLOW_DIR" "${SWEPT_SCRIPTS[@]}" +fi + +# --- The scanner itself, in both directions, over fixtures ------------------- +# +# The sweep is the only assertion covering jobs nobody has named, so a scanner +# that quietly stops recognizing them fails open. These fixtures pin both +# directions of that recognition, including the split-step case the real +# workflows rely on and the prose case, where a comment describing the defect +# must not be read as the defect. +FIXTURE_DIR="$(mktemp -d)" +trap 'rm -rf "$FIXTURE_DIR"' EXIT +mkdir -p "${FIXTURE_DIR}/split" "${FIXTURE_DIR}/bug1811" "${FIXTURE_DIR}/prose" \ + "${FIXTURE_DIR}/unpaired" "${FIXTURE_DIR}/unknown" "${FIXTURE_DIR}/nostate" \ + "${FIXTURE_DIR}/misattrib" + +# The shape deploy-aws-fargate.yml has: key and platform in different steps of +# one job, and a second job in the same file whose key must not leak into it. +cat >"${FIXTURE_DIR}/split/ok.yml" <<'EOF' +jobs: + deploy-lambda: + steps: + - name: Terraform Init + run: | + printf '%s\nkey = "github-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend + - name: Terraform Plan + run: | + terraform plan -var="compute_platform=lambda" + deploy-fargate: + steps: + - name: Terraform Init + run: | + printf '%s\nkey = "github-fargate-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend + - name: Terraform Plan + run: | + terraform plan -var="compute_platform=fargate" +EOF + +# #1811 itself: the fargate rollback keyed on the lambda namespace. This is the +# exact text that shipped, so a revert of the fix reproduces this fixture. +cat >"${FIXTURE_DIR}/bug1811/rollback.yml" <<'EOF' +jobs: + rollback-aws-fargate: + steps: + - name: Rollback with Terraform + run: | + printf '%s\nkey = "github-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend + terraform apply -auto-approve \ + -var="compute_platform=fargate" +EOF + +# The mirror image, which is the same defect the other way round: a lambda apply +# into the fargate namespace. +cat >"${FIXTURE_DIR}/bug1811/inverse.yml" <<'EOF' +jobs: + rollback-aws-lambda: + steps: + - name: Rollback with Terraform + run: | + printf '%s\nkey = "github-fargate-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend + terraform apply -auto-approve -var="compute_platform=lambda" +EOF + +# A comment describing the mismatch is not the mismatch. A guard that fired on +# this would police what may be WRITTEN about the defect, including the comment +# on rollback.yml recording that #1811 was fixed. +cat >"${FIXTURE_DIR}/prose/mentions.yml" <<'EOF' +jobs: + rollback-aws-fargate: + # Until #1811 this job wrote key = "github-%s/terraform.tfstate" while + # applying -var="compute_platform=fargate". + steps: + - name: Terraform Init + run: | + printf '%s\nkey = "github-fargate-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend + terraform apply -var="compute_platform=fargate" # not key = "github-%s/x" +EOF + +# Splitting the key out of the job is not a way to be unverifiable: a platform +# with no key in the same job is reported rather than skipped. +cat >"${FIXTURE_DIR}/unpaired/nokey.yml" <<'EOF' +jobs: + rollback-aws-fargate: + steps: + - name: Rollback with Terraform + run: | + terraform apply -auto-approve -var="compute_platform=fargate" +EOF + +# An unrecognized platform value is reported rather than passed: the namespace +# it should write is not known, so "no violation" would be a guess. +cat >"${FIXTURE_DIR}/unknown/newplatform.yml" <<'EOF' +jobs: + deploy-eks: + steps: + - name: Terraform Init + run: | + printf '%s\nkey = "github-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend + terraform apply -var="compute_platform=eks" +EOF + +assert_scan "key and platform in different steps of one job are paired" \ + "" "${FIXTURE_DIR}/split" + +assert_scan "the #1811 pairing is flagged, naming the job" \ + 'rollback.yml: job "rollback-aws-fargate": compute_platform=fargate applies into the github-/ (lambda) state namespace' \ + "${FIXTURE_DIR}/bug1811" + +assert_scan "the inverse pairing is flagged too" \ + 'inverse.yml: job "rollback-aws-lambda": compute_platform=lambda applies into the github-fargate-/ (fargate) state namespace' \ + "${FIXTURE_DIR}/bug1811" + +assert_scan "a comment describing the mismatch is not the mismatch" \ + "" "${FIXTURE_DIR}/prose" + +assert_scan "a platform with no state key in the same job is reported" \ + 'nokey.yml: job "rollback-aws-fargate": compute_platform=fargate with no AWS github-* backend state key in the same job' \ + "${FIXTURE_DIR}/unpaired" + +assert_scan "an unrecognized compute_platform value is reported" \ + 'newplatform.yml: job "deploy-eks": unrecognized compute_platform=eks' \ + "${FIXTURE_DIR}/unknown" + +# --- The concurrency axis ---------------------------------------------------- +# +# The group and the key both name the state object, and #1811 was the state +# where they disagreed. A guard on the key alone lets the group drift back. +mkdir -p "${FIXTURE_DIR}/group" "${FIXTURE_DIR}/othercloud" + +cat >"${FIXTURE_DIR}/group/drift.yml" <<'EOF' +jobs: + rollback-aws-fargate: + concurrency: + group: aws-tfstate-${{ inputs.environment }} + cancel-in-progress: false + steps: + - name: Rollback with Terraform + run: | + printf '%s\nkey = "github-fargate-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend + terraform apply -var="compute_platform=fargate" +EOF + +# The Azure and GCP jobs pair a `github-` key with a group in another +# family. Reading the Azure blob as the Lambda namespace would demand an +# `aws-tfstate-*` group for it, so the AWS key is recognized by its +# `/terraform.tfstate` suffix and the Azure `.terraform.tfstate` one is not. +cat >"${FIXTURE_DIR}/othercloud/azure.yml" <<'EOF' +jobs: + destroy-azure: + concurrency: + group: azure-tfstate-staging + steps: + - name: Terraform Init + run: | + printf '%s\nkey = "github-%s.terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend + deploy-gcp: + concurrency: + group: gcp-tfstate-staging + steps: + - name: Terraform Init + run: | + printf '%s\nprefix = "github-%s"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend +EOF + +assert_scan "a concurrency group naming the other namespace is flagged" \ + 'drift.yml: job "rollback-aws-fargate": concurrency group aws-tfstate-* guards a job writing the github-fargate-/ (fargate) state namespace' \ + "${FIXTURE_DIR}/group" + +assert_scan "the Azure and GCP jobs are not read as AWS namespaces" \ + 'no job applying a compute_platform found at all' "${FIXTURE_DIR}/othercloud" + +# A workflow that writes a state key but applies no platform, which is what the +# Azure and GCP jobs and the fargate lock-clearing step look like. Those are not +# violations, but a directory holding only them is also not evidence of a clean +# repository, so the scan says it found nothing rather than staying silent. +cat >"${FIXTURE_DIR}/nostate/plain.yml" <<'EOF' +jobs: + destroy-azure: + steps: + - name: Terraform Init + run: | + printf '%s\nkey = "github-%s.terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend + terraform destroy -auto-approve +EOF + +assert_scan "a directory holding no compute_platform job says so rather than passing" \ + 'no job applying a compute_platform found at all' "${FIXTURE_DIR}/nostate" + +assert_scan "a directory that does not exist is reported, not passed" \ + 'no workflow files found under' "${FIXTURE_DIR}/does-not-exist" + +assert_scan "a swept file that does not exist is reported, not passed" \ + 'swept file not found' "${FIXTURE_DIR}/split" "${FIXTURE_DIR}/scripts/does-not-exist.sh" + +# A site running to the end of its file is only flushed once the next file +# starts, so the report has to remember which file the site came from. Reported +# from FILENAME it would name the innocent file scanned next, and every fixture +# above scans one offending file at a time, so none of them can catch it. +cat >"${FIXTURE_DIR}/misattrib/a-bad.yml" <<'EOF' +jobs: + rollback-aws-fargate: + steps: + - name: Rollback with Terraform + run: | + printf '%s\nkey = "github-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend + terraform apply -var="compute_platform=fargate" +EOF + +cat >"${FIXTURE_DIR}/misattrib/b-innocent.yml" <<'EOF' +jobs: + summary: + steps: + - name: Applies nothing + run: echo "clean" +EOF + +assert_scan "a finding names the file it came from, not the file scanned after it" \ + 'a-bad.yml: job "rollback-aws-fargate"' "${FIXTURE_DIR}/misattrib" + +# A shell script has no job header, so it is scanned as one site. Nothing under +# scripts/ pairs a key with a platform today; this pins that the scan would see +# it if one did. +mkdir -p "${FIXTURE_DIR}/scripts" +cat >"${FIXTURE_DIR}/scripts/deploy.sh" <<'EOF' +#!/usr/bin/env bash +printf '%s\nkey = "github-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend +terraform apply -var="compute_platform=fargate" +EOF + +assert_scan "a script pairing them wrongly is flagged as a whole file" \ + 'deploy.sh: whole file: compute_platform=fargate applies into the github-/ (lambda) state namespace' \ + "${FIXTURE_DIR}/split" "${FIXTURE_DIR}/scripts/deploy.sh" + +echo +echo "passed: ${pass}, failed: ${fail}" +[[ "$fail" -eq 0 ]] From f1a45e35e73e964a25f4ddcbd02b52a272e1af21 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 07:11:05 +0200 Subject: [PATCH 2/2] fix(scripts): close two fail-open gaps in the state-namespace guard Both raised by review on #1857, both Major, both real. 1. The concurrency check read absence as permission. It only compared a group against the key namespace when a group was present, so a job with the correct platform and the correct backend key but NO `aws-*-tfstate-*` group passed cleanly and could then apply shared state with nothing serializing it. That is #1806 with the guard watching. A missing group is now a violation for any job applying a platform into a known AWS namespace. The mismatch arm deliberately still runs without a platform, so a read-only job locking the wrong object stays caught, while the missing-group arm requires one, since a job that only reads state needs no lock. 2. `build_swept_scripts` was not recursive. It globbed `$dir/*.sh` and `$dir/lib/*.sh`, which names two directories the same way naming files would, and is the defect the helper exists to prevent one level up: a script at `scripts/aws/rollback.sh` was opened by no suite at all while each reported coverage of every script under `scripts/`. Discovery is now `find -print0` piped through `sort -z`, read with `read -d ''` rather than a `**` glob, since `globstar` is bash 4 and this runs on the bash 3.2 that ships with macOS. Fixed in the shared helper rather than locally, because test-ecr-delete-selection.sh and test-rds-deletion-protection-scope.sh call the same function and carry the identical blind spot on main today. One change closes it in all three, which is why the helper was extracted. Verified by mutation against all three suites, not just this one. A nested `scripts/aws/` directory holding one offender per guard (an unguarded `aws ecr delete-repository` by prefix, an unguarded `aws rds modify-db-instance`, and a fargate apply into the lambda namespace) makes each of the three fail naming its own file; removing the directory returns all three to green. The counter-check runs the same fixture against `scripts/lib/code-scan-awk.sh` as currently merged on main, taken from `git show origin/main:`, and both the ECR and RDS suites pass with the offender present, so the gap on main is demonstrated rather than asserted. Also mutation-verified on the real tree: deleting the fargate rollback's concurrency block outright is now caught, where before it was not. The suite grows from 23 to 26 cases, including a fixture tree that proves the recursion reaches `aws/deep/nested.sh` and that the nested file is actually scanned rather than merely discovered. Runs clean under bash 3.2 with both BWK awk and mawk; shellcheck reports nothing. --- scripts/lib/code-scan-awk.sh | 36 +++++--- scripts/test-aws-tfstate-platform-key.sh | 109 +++++++++++++++++++++-- 2 files changed, 125 insertions(+), 20 deletions(-) diff --git a/scripts/lib/code-scan-awk.sh b/scripts/lib/code-scan-awk.sh index af96619a0..b39a18ee8 100644 --- a/scripts/lib/code-scan-awk.sh +++ b/scripts/lib/code-scan-awk.sh @@ -53,41 +53,53 @@ # build_swept_scripts SCRIPTS_DIR # -# Sets SWEPT_SCRIPTS to every `*.sh` directly under SCRIPTS_DIR and under -# SCRIPTS_DIR/lib, excluding the three guard suites themselves. +# Sets SWEPT_SCRIPTS to every `*.sh` anywhere under SCRIPTS_DIR, at any depth, +# excluding the three guard suites themselves. # -# Globbed rather than named file by file, in every suite, because naming the +# Discovered rather than named file by file, in every suite, because naming the # scripts already known to be guarded is the same defect the suites exist to # catch, one level up: a NEW script running the dangerous command without the # selector is invisible to a sweep that only ever opens the files someone # remembered to list, which is how a guard fails to reach a sibling site. # +# RECURSIVE, via `find`, and this is the second half of that same defect. The +# original form globbed `$dir/*.sh` and `$dir/lib/*.sh`, which names two +# directories the way the thing above names two files: a script added at +# `scripts/aws/rollback.sh` was never opened by ANY of the three suites, while +# each went on reporting coverage of "every script under scripts/". Raised by +# review on this suite and fixed here in the shared helper rather than locally, +# because the RDS and ECR guards call this same function and carried the +# identical blind spot. +# +# `find -print0` with `read -d ''` rather than a `**` glob: `globstar` is bash 4, +# and this must run on the bash 3.2 that ships with macOS. `sort -z` so the swept +# order is deterministic across platforms, since `find` order is not defined. +# The loop runs in the current shell (process substitution, not a pipe), because +# a pipeline subshell would build the array and then discard it. +# # The guard suites are excluded by basename because each carries the very # pattern it looks for as fixture data and inside awk programs, so sweeping them # reports a suite as a violation of itself. Matching on basename rather than on # a path fragment keeps the exclusion from exempting a real script that merely # sits beside them. # -# `nullglob` so a pattern matching nothing expands to nothing rather than to the -# literal pattern text. Without it an unmatched glob becomes a nonexistent path, -# the sweep bails out early, and it covers no scripts at all. Callers must still -# assert SWEPT_SCRIPTS is non-empty and contains the script that actually runs -# their command: an empty swept set satisfies every "no violations" reading. +# A directory with no `*.sh` yields an empty SWEPT_SCRIPTS rather than a +# nonexistent path. Callers must still assert SWEPT_SCRIPTS is non-empty and +# contains the script that actually runs their command: an empty swept set +# satisfies every "no violations" reading. # # Returns through a global because bash 3.2, which this must run on, has no # namerefs. build_swept_scripts() { local dir="$1" candidate SWEPT_SCRIPTS=() - shopt -s nullglob - for candidate in "$dir"/*.sh "$dir"/lib/*.sh; do + while IFS= read -r -d '' candidate; do case "$(basename "$candidate")" in test-rds-deletion-protection-scope.sh | test-ecr-delete-selection.sh | \ test-aws-tfstate-platform-key.sh) continue ;; esac SWEPT_SCRIPTS+=("$candidate") - done - shopt -u nullglob + done < <(find "$dir" -type f -name '*.sh' -print0 2>/dev/null | sort -z) } # shellcheck disable=SC2034 # read by the suites that source this file diff --git a/scripts/test-aws-tfstate-platform-key.sh b/scripts/test-aws-tfstate-platform-key.sh index 16ad56639..b3f0616d6 100755 --- a/scripts/test-aws-tfstate-platform-key.sh +++ b/scripts/test-aws-tfstate-platform-key.sh @@ -21,12 +21,17 @@ # key from the LAMBDA namespace and applied `compute_platform=fargate` into it, # so the two rollback jobs wrote the same object. # -# The `concurrency` group is checked on the same axis. #1806 keyed every group -# on the state object its job locks, so a group naming a different namespace -# from the key means one of the two moved alone: the job then serializes against -# a state file it never touches while writing one unguarded. Only groups in the -# `aws-tfstate-*` / `aws-fargate-tfstate-*` families are checked; the Azure and -# GCP groups guard state objects that have no platform split. +# The `concurrency` group is checked on the same axis, in two ways. #1806 keyed +# every group on the state object its job locks, so a group naming a different +# namespace from the key means one of the two moved alone: the job then +# serializes against a state file it never touches while writing one unguarded. +# A job with NO group is a violation too, not an exemption: it applies shared +# state with nothing serializing it at all, which is the same hazard without +# even a wrong answer to notice. Gating that check on the group being present +# read absence as permission and let exactly that job through. +# +# Only groups in the `aws-tfstate-*` / `aws-fargate-tfstate-*` families are +# checked; the Azure and GCP groups guard state objects with no platform split. # # A green workflow run proves nothing about this, which is why the pairing is # asserted here as text rather than left to a live rollback to discover. @@ -148,8 +153,22 @@ scan_platform_keys() { printf "%s: %s: compute_platform=%s applies into the %s state namespace\n", basename(site_file), label(), platform, nsdesc(ns) } } - if (mode != "census" && grp != "" && ns != "" && ns != "MIXED" && grp != ns) - printf "%s: %s: concurrency group aws%s-tfstate-* guards a job writing the %s state namespace\n", basename(site_file), label(), (grp == "fargate" ? "-fargate" : ""), nsdesc(ns) + # Two independent conditions, not an if/else on `grp != ""`. Gating the + # whole check on a non-empty group made ABSENCE read as permission: a job + # with the right key and the right platform but no group at all passed, + # and then applies shared state with nothing serializing it, which is + # #1806 with the guard watching. Empty is a missing requirement here, not + # an exemption. + # + # The mismatch arm does not require a platform, so a read-only job that + # locks the wrong object is still caught; the missing-group arm does, + # because a job that only reads state needs no lock. + if (mode != "census" && ns != "" && ns != "MIXED") { + if (grp == "" && platform != "") + printf "%s: %s: no AWS concurrency group serializes a job applying compute_platform=%s into the %s state namespace\n", basename(site_file), label(), platform, nsdesc(ns) + else if (grp != "" && grp != ns) + printf "%s: %s: concurrency group aws%s-tfstate-* guards a job writing the %s state namespace\n", basename(site_file), label(), (grp == "fargate" ? "-fargate" : ""), nsdesc(ns) + } site = ""; ns = ""; platform = ""; grp = "" } function label() { return (site == "" ? "whole file" : ("job \"" site "\"")) } @@ -325,6 +344,9 @@ mkdir -p "${FIXTURE_DIR}/split" "${FIXTURE_DIR}/bug1811" "${FIXTURE_DIR}/prose" cat >"${FIXTURE_DIR}/split/ok.yml" <<'EOF' jobs: deploy-lambda: + concurrency: + group: aws-tfstate-${{ inputs.environment }} + cancel-in-progress: false steps: - name: Terraform Init run: | @@ -333,6 +355,9 @@ jobs: run: | terraform plan -var="compute_platform=lambda" deploy-fargate: + concurrency: + group: aws-fargate-tfstate-${{ inputs.environment }} + cancel-in-progress: false steps: - name: Terraform Init run: | @@ -375,6 +400,8 @@ jobs: rollback-aws-fargate: # Until #1811 this job wrote key = "github-%s/terraform.tfstate" while # applying -var="compute_platform=fargate". + concurrency: + group: aws-fargate-tfstate-${{ inputs.environment }} steps: - name: Terraform Init run: | @@ -468,10 +495,28 @@ jobs: printf '%s\nprefix = "github-%s"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend EOF +# The fail-open the group check originally had: key right, platform right, no +# group at all. Nothing about the pairing is wrong, so every other assertion in +# this suite stays green while that job applies shared state with nothing +# serializing it. Absence is a missing requirement, not an exemption. +cat >"${FIXTURE_DIR}/group/nogroup.yml" <<'EOF' +jobs: + rollback-aws-fargate: + steps: + - name: Rollback with Terraform + run: | + printf '%s\nkey = "github-fargate-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend + terraform apply -auto-approve -var="compute_platform=fargate" +EOF + assert_scan "a concurrency group naming the other namespace is flagged" \ 'drift.yml: job "rollback-aws-fargate": concurrency group aws-tfstate-* guards a job writing the github-fargate-/ (fargate) state namespace' \ "${FIXTURE_DIR}/group" +assert_scan "a job with no concurrency group at all is flagged, not passed" \ + 'nogroup.yml: job "rollback-aws-fargate": no AWS concurrency group serializes a job applying compute_platform=fargate into the github-fargate-/ (fargate) state namespace' \ + "${FIXTURE_DIR}/group" + assert_scan "the Azure and GCP jobs are not read as AWS namespaces" \ 'no job applying a compute_platform found at all' "${FIXTURE_DIR}/othercloud" @@ -537,6 +582,54 @@ assert_scan "a script pairing them wrongly is flagged as a whole file" \ 'deploy.sh: whole file: compute_platform=fargate applies into the github-/ (lambda) state namespace' \ "${FIXTURE_DIR}/split" "${FIXTURE_DIR}/scripts/deploy.sh" +# --- build_swept_scripts reaches nested directories -------------------------- +# +# The helper is shared with test-ecr-delete-selection.sh and +# test-rds-deletion-protection-scope.sh, and until this was fixed it globbed +# `$dir/*.sh` and `$dir/lib/*.sh` only. That names two directories the same way +# naming files would: a script at `scripts/aws/rollback.sh` was invisible to all +# three suites while each reported coverage of every script under `scripts/`. +# +# Asserted on a fixture tree rather than on the real `scripts/`, which has no +# nested `*.sh` today: the recursion has to be proven where a nested file +# actually exists, or the assertion passes for a directory that could not have +# failed it. +mkdir -p "${FIXTURE_DIR}/tree/lib" "${FIXTURE_DIR}/tree/aws/deep" +: >"${FIXTURE_DIR}/tree/top.sh" +: >"${FIXTURE_DIR}/tree/lib/helper.sh" +: >"${FIXTURE_DIR}/tree/aws/rollback.sh" +: >"${FIXTURE_DIR}/tree/aws/deep/nested.sh" +: >"${FIXTURE_DIR}/tree/aws/not-a-script.txt" +: >"${FIXTURE_DIR}/tree/test-ecr-delete-selection.sh" + +build_swept_scripts "${FIXTURE_DIR}/tree" +swept_list="$(printf '%s\n' "${SWEPT_SCRIPTS[@]}" | sed "s#^${FIXTURE_DIR}/tree/##" | sort | tr '\n' ' ')" +expected_list="aws/deep/nested.sh aws/rollback.sh lib/helper.sh top.sh " + +if [[ "$swept_list" == "$expected_list" ]]; then + note_pass "build_swept_scripts reaches nested directories and still excludes the guard suites" +else + note_fail "build_swept_scripts did not sweep the expected set" + echo " expected: ${expected_list}" + echo " actual: ${swept_list}" +fi + +# The nested script is not merely discovered, it is actually scanned: discovery +# that does not reach the scanner is the same gap one step later. +cat >"${FIXTURE_DIR}/tree/aws/rollback.sh" <<'EOF' +#!/usr/bin/env bash +printf '%s\nkey = "github-%s/terraform.tfstate"\n' "$TF_BACKEND" "$ENVIRONMENT" > /tmp/backend.tfbackend +terraform apply -var="compute_platform=fargate" +EOF + +build_swept_scripts "${FIXTURE_DIR}/tree" +assert_scan "an invalid pairing in a NESTED script directory is scanned and flagged" \ + 'rollback.sh: whole file: compute_platform=fargate applies into the github-/ (lambda) state namespace' \ + "${FIXTURE_DIR}/split" "${SWEPT_SCRIPTS[@]}" + +# Restore the real swept set: the assertions above reassigned the global. +build_swept_scripts "$SCRIPT_DIR" + echo echo "passed: ${pass}, failed: ${fail}" [[ "$fail" -eq 0 ]]