From a78a00023f06acb83608f4297d656451fda8001e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 18 Aug 2026 23:58:17 +0200 Subject: [PATCH 1/3] sec(ci): unprotect only the RDS instance each state owns before destroy Three destroy steps selected RDS instances with starts_with(DBInstanceIdentifier,'cudly-dev') or the 'cudly-staging' equivalent and stripped deletion protection from every match. That prefix also matches cudly-dev-prod-mirror, cudly-dev--postgres-replica and any operator-named instance sharing it, and the staging prefix additionally spans both staging states, so either cleanup job unprotected the other's database. Deletion protection is the last line of defence on a database: removing it from an instance the workflow does not own does not delete anything by itself, it leaves that database exposed to the next destroy that does match it. Selection now resolves the owned identifier from `terraform output` on the state being torn down and compares by exact equality, using the selector added for #1592 rather than a second implementation of the same idea. The selector was ECR-specific in name only, so it is now scripts/select-owned-name.sh and both resources share one comparison; two copies would have to be hardened in lockstep, which is the failure mode that turned #1592 into #1820 and then this. The identifier is published by a new database_instance_identifier output, since aws_db_instance.main.identifier carries local.stack_name's random suffix and no prefix describes it uniquely. Nothing is swallowed. `2>/dev/null` on the listing made a failed call indistinguishable from an account with no instances, and `|| true` on the modify reported success after a failed strip, so the step reported success and `terraform destroy` then failed downstream on an instance that was still protected. A state that publishes outputs but not the identifier now fails with the remedy named, rather than falling back to the prefix this removes. All three sites are wired, not the two that are most visible: leaving one behind would have destroy-fargate-dev.yml select ECR by equality and RDS by prefix in the same job. The guard suite asserts both directions, because a selector that matches nothing passes every refusal assertion while leaving the destroy broken: a non-zero match count is asserted before any absence. Each assertion is proven to bite by mutation, against a copy of the tree, and each was required to produce its specific FAIL line since a syntax error also exits non-zero. Reverting any one of the three sites to a prefix loop fails, including reverting only the second staging site while the first stays wired; a selector that matches by prefix fails on the near-miss table; one that matches nothing fails on the count; re-adding `|| true` fails; keeping the selector as a live call while the modify reads the raw listing fails, which is the case a "the selector is still called" check waves through; and renaming or repointing either terraform output fails. The ECR suite was refactored onto the shared scan helpers, so reverting an ECR site and reintroducing a prefix filter in the ECR script were both re-confirmed to fail. --- .github/workflows/ci.yml | 34 +- .github/workflows/cleanup-staging.yml | 44 +- .github/workflows/destroy-fargate-dev.yml | 22 +- .../disable-owned-rds-deletion-protection.sh | 102 +++ scripts/force-delete-owned-ecr-repo.sh | 4 +- scripts/lib/code-scan-awk.sh | 69 ++ scripts/select-ecr-repos-to-delete.sh | 73 -- scripts/select-owned-name.sh | 82 +++ ...delete.sh => test-ecr-delete-selection.sh} | 75 +-- scripts/test-rds-deletion-protection-scope.sh | 623 ++++++++++++++++++ terraform/environments/aws/outputs.tf | 11 + terraform/modules/database/aws/outputs.tf | 5 + 12 files changed, 989 insertions(+), 155 deletions(-) create mode 100755 scripts/disable-owned-rds-deletion-protection.sh create mode 100644 scripts/lib/code-scan-awk.sh delete mode 100755 scripts/select-ecr-repos-to-delete.sh create mode 100755 scripts/select-owned-name.sh rename scripts/{test-select-ecr-repos-to-delete.sh => test-ecr-delete-selection.sh} (90%) create mode 100755 scripts/test-rds-deletion-protection-scope.sh diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2c0b2cb21..a8ece349a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -781,7 +781,38 @@ jobs: persist-credentials: false - name: Run selector self-tests - run: bash scripts/test-select-ecr-repos-to-delete.sh + run: bash scripts/test-ecr-delete-selection.sh + + # Assert that the RDS instance selector used by destroy-fargate-dev.yml and + # cleanup-staging.yml unprotects the instance each state owns and nothing + # else. Deletion protection is the last line of defence on a database, so + # stripping it from an instance a state does not own leaves that database + # exposed to the next destroy that does match it. Both directions are + # asserted: a prefix near-miss is refused, and the owned instance is still + # selected, since a selector matching nothing passes every refusal assertion + # while leaving the destroy broken. The suite also asserts the wiring, which + # is what #1592, #1820 and #1821 each escaped: all three destroy steps call + # scripts/disable-owned-rds-deletion-protection.sh, that script unprotects + # only what the selector yields and swallows nothing, and nothing else under + # .github/workflows or scripts/ runs `aws rds modify-db-instance` unguarded. + # Fast (shell only), so it always runs. + rds-deletion-protection-scope: + name: RDS deletion protection scope + 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 RDS scope self-tests + run: bash scripts/test-rds-deletion-protection-scope.sh # Assert that no Terraform file declares an azurerm_key_vault_access_policy # resource. This project's only Key Vault sets enable_rbac_authorization = @@ -821,6 +852,7 @@ jobs: - aws-iam-parity - gcp-secret-scope - ecr-delete-selection + - rds-deletion-protection-scope - azure-kv-access-policy if: always() diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index 586160736..368d06147 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -151,15 +151,19 @@ jobs: - name: Force-delete the ECR repo this state owns run: ./scripts/force-delete-owned-ecr-repo.sh terraform/environments/aws - - name: Disable RDS deletion protection before destroy - run: | - for INSTANCE_ID in $(aws rds describe-db-instances \ - --query "DBInstances[?starts_with(DBInstanceIdentifier,'cudly-staging')].DBInstanceIdentifier" \ - --output text 2>/dev/null); do - echo "Disabling deletion protection on $INSTANCE_ID..." - aws rds modify-db-instance --db-instance-identifier "$INSTANCE_ID" \ - --no-deletion-protection --apply-immediately 2>/dev/null || true - done + # Runs before `terraform destroy`: an instance applied with + # deletion_protection = true blocks the destroy. Unprotects only the + # instance THIS state owns, by exact identifier. The + # `starts_with(DBInstanceIdentifier,'cudly-staging')` filter this step used + # to run also matched `cudly-staging-prod-mirror`, + # `cudly-staging--postgres-replica` and the sibling staging state's + # instance, and stripped the last line of defence from them, leaving them + # exposed to the next destroy that did match (#1821). Rationale and why + # nothing here is swallowed: the script's header. Both staging jobs and + # destroy-fargate-dev.yml call the same script, so the guard cannot land in + # one workflow and not its sibling -- which is how #1592 became #1820. + - name: Disable RDS deletion protection on the instance this state owns + run: ./scripts/disable-owned-rds-deletion-protection.sh terraform/environments/aws - name: Terraform Destroy env: @@ -229,15 +233,19 @@ jobs: - name: Force-delete the ECR repo this state owns run: ./scripts/force-delete-owned-ecr-repo.sh terraform/environments/aws - - name: Disable RDS deletion protection before destroy - run: | - for INSTANCE_ID in $(aws rds describe-db-instances \ - --query "DBInstances[?starts_with(DBInstanceIdentifier,'cudly-staging')].DBInstanceIdentifier" \ - --output text 2>/dev/null); do - echo "Disabling deletion protection on $INSTANCE_ID..." - aws rds modify-db-instance --db-instance-identifier "$INSTANCE_ID" \ - --no-deletion-protection --apply-immediately 2>/dev/null || true - done + # Runs before `terraform destroy`: an instance applied with + # deletion_protection = true blocks the destroy. Unprotects only the + # instance THIS state owns, by exact identifier. The + # `starts_with(DBInstanceIdentifier,'cudly-staging')` filter this step used + # to run also matched `cudly-staging-prod-mirror`, + # `cudly-staging--postgres-replica` and the sibling staging state's + # instance, and stripped the last line of defence from them, leaving them + # exposed to the next destroy that did match (#1821). Rationale and why + # nothing here is swallowed: the script's header. Both staging jobs and + # destroy-fargate-dev.yml call the same script, so the guard cannot land in + # one workflow and not its sibling -- which is how #1592 became #1820. + - name: Disable RDS deletion protection on the instance this state owns + run: ./scripts/disable-owned-rds-deletion-protection.sh terraform/environments/aws - name: Terraform Destroy env: diff --git a/.github/workflows/destroy-fargate-dev.yml b/.github/workflows/destroy-fargate-dev.yml index a259f01b0..6bc98a04b 100644 --- a/.github/workflows/destroy-fargate-dev.yml +++ b/.github/workflows/destroy-fargate-dev.yml @@ -141,15 +141,19 @@ jobs: - name: Force-delete ECR repo run: ./scripts/force-delete-owned-ecr-repo.sh terraform/environments/aws - - name: Disable RDS deletion protection - run: | - for INSTANCE_ID in $(aws rds describe-db-instances \ - --query "DBInstances[?starts_with(DBInstanceIdentifier,'cudly-dev')].DBInstanceIdentifier" \ - --output text 2>/dev/null); do - echo "Disabling deletion protection on $INSTANCE_ID..." - aws rds modify-db-instance --db-instance-identifier "$INSTANCE_ID" \ - --no-deletion-protection --apply-immediately 2>/dev/null || true - done + # Runs before `terraform destroy`: an instance applied with + # deletion_protection = true blocks the destroy. Unprotects only the + # instance THIS state owns, by exact identifier. The + # `starts_with(DBInstanceIdentifier,'cudly-dev')` filter this step used to + # run also matched `cudly-dev-prod-mirror` and + # `cudly-dev--postgres-replica` and stripped the last line of defence + # from them, leaving them exposed to the next destroy that did match + # (#1821). Rationale and why nothing here is swallowed: the script's + # header. This job and both cleanup-staging.yml staging jobs call the same + # script, so the guard cannot land in one workflow and not its sibling -- + # which is how #1592 became #1820. + - name: Disable RDS deletion protection on the instance this state owns + run: ./scripts/disable-owned-rds-deletion-protection.sh terraform/environments/aws - name: Terraform Destroy env: diff --git a/scripts/disable-owned-rds-deletion-protection.sh b/scripts/disable-owned-rds-deletion-protection.sh new file mode 100755 index 000000000..7c781fac6 --- /dev/null +++ b/scripts/disable-owned-rds-deletion-protection.sh @@ -0,0 +1,102 @@ +#!/usr/bin/env bash +# disable-owned-rds-deletion-protection.sh +# +# Removes deletion protection from the one RDS instance a Terraform state owns, +# and from nothing else. +# +# Usage: disable-owned-rds-deletion-protection.sh TERRAFORM_STATE_DIR +# +# Run before `terraform destroy` on a state whose instance was applied with +# deletion_protection = true (terraform/modules/database/aws/main.tf): the +# destroy otherwise fails on the protected instance. +# +# The state directory is a required argument rather than a constant because it +# is the identity of what gets modified. Every caller happens to pass +# terraform/environments/aws today, but which state the identifier is read from +# is the whole safety property here, so it stays visible at each call site. +# +# The owned identifier is read from `terraform output` on the state the destroy +# is about to tear down and compared by exact equality against every instance in +# the account, by scripts/select-owned-name.sh -- the same selector +# force-delete-owned-ecr-repo.sh uses, so the comparison is hardened in one +# place for both resources. +# +# The callers used to select by the `cudly-dev*` / `cudly-staging*` identifier +# prefix, which also matches `cudly-dev-prod-mirror`, +# `cudly-dev--postgres-replica` and any operator-named instance sharing the +# prefix, and stripped deletion protection from every one of them (#1821). +# Deletion protection is the last line of defence on a database, so an instance +# this state does not own must never be touched: it would be left exposed to the +# next destroy that does match it. The `cudly-staging` prefix also spanned both +# staging states, whose instances are each +# `cudly-staging--postgres`, so either cleanup job +# unprotected the other's database. +# +# Reads the identifier through `output -json`, not `output -raw`: on a state +# with no outputs at all, `output -raw ` exits 0 and writes its "No +# outputs found" warning to STDOUT, so the identifier would become that warning +# text. `output -json` returns `{}` for that state and the cases separate +# cleanly: +# no outputs at all -> already destroyed, skip +# outputs but not this one -> fail loudly, with the remedy named +# +# Nothing is swallowed. The `2>/dev/null || true` the callers used to carry +# reported success after a failed listing or a failed modify, and a failed +# listing is indistinguishable from an account with no instances, so the step +# did nothing and `terraform destroy` then failed on an instance that was still +# protected. "Already gone" needs no swallowing: the instance is simply absent +# from the listing, the selector prints nothing and exits 0, and the loop body +# never runs. +# +# Exit codes: +# 0 completed, including the "state already destroyed" and "instance already +# gone" cases, which are normal outcomes and not errors +# 1 the state publishes outputs but not the owned identifier +# 2 usage error (wrong arity, or a state directory that does not exist) +# * anything the AWS CLI, terraform, jq or the selector fails with, unmasked + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +if [[ $# -ne 1 ]]; then + echo "usage: $(basename "$0") TERRAFORM_STATE_DIR" >&2 + exit 2 +fi + +STATE_DIR="$1" + +if [[ ! -d "$STATE_DIR" ]]; then + echo "error: terraform state directory '${STATE_DIR}' does not exist" >&2 + exit 2 +fi + +OUTPUTS_JSON="$(terraform -chdir="$STATE_DIR" output -json)" +if [[ "$(jq -r 'length' <<<"$OUTPUTS_JSON")" -eq 0 ]]; then + echo "State has no outputs; the stack is already destroyed and there is no RDS instance to unprotect." + exit 0 +fi + +# `database_instance_identifier` was added with #1821, so a state last applied +# before it does not publish it yet. That is a loud failure with a named remedy +# rather than a fallback: the only fallback available is the identifier prefix +# this script exists to remove. +if ! OWNED_INSTANCE="$(jq -er '.database_instance_identifier.value' <<<"$OUTPUTS_JSON")"; then + echo "error: state '${STATE_DIR}' publishes outputs but not 'database_instance_identifier'," >&2 + echo " so the instance this state owns cannot be identified. Re-apply the state to" >&2 + echo " publish the output, or remove deletion protection on that one instance by" >&2 + echo " hand, then re-run the destroy. Refusing to fall back to an identifier" >&2 + echo " prefix, which strips protection from instances this state does not own." >&2 + exit 1 +fi + +echo "This state owns RDS instance '$OWNED_INSTANCE'" + +aws rds describe-db-instances --query 'DBInstances[].DBInstanceIdentifier' --output text \ + | tr '\t' '\n' \ + | "${SCRIPT_DIR}/select-owned-name.sh" "$OWNED_INSTANCE" \ + | while IFS= read -r INSTANCE_ID; do + echo "Disabling deletion protection on $INSTANCE_ID..." + aws rds modify-db-instance --db-instance-identifier "$INSTANCE_ID" \ + --no-deletion-protection --apply-immediately + done diff --git a/scripts/force-delete-owned-ecr-repo.sh b/scripts/force-delete-owned-ecr-repo.sh index 045b7f271..2148cf2d7 100755 --- a/scripts/force-delete-owned-ecr-repo.sh +++ b/scripts/force-delete-owned-ecr-repo.sh @@ -17,7 +17,7 @@ # # The owned name is read from `terraform output` on the state the destroy is # about to tear down and compared by exact equality against every repository in -# the account, by scripts/select-ecr-repos-to-delete.sh. The callers used to +# the account, by scripts/select-owned-name.sh. The callers used to # select by the `cudly-dev*` / `cudly-staging*` prefix, which also matches # `cudly-staging-prod-mirror` and `cudly-staging--backup` and force-deleted # every image in them (#1592, #1820). The prefix also spans both staging states: @@ -77,7 +77,7 @@ echo "This state owns ECR repository '$OWNED_REPO'" aws ecr describe-repositories --query 'repositories[].repositoryName' --output text \ | tr '\t' '\n' \ - | "${SCRIPT_DIR}/select-ecr-repos-to-delete.sh" "$OWNED_REPO" \ + | "${SCRIPT_DIR}/select-owned-name.sh" "$OWNED_REPO" \ | while IFS= read -r REPO; do echo "Force-deleting ECR repo $REPO..." aws ecr delete-repository --repository-name "$REPO" --force diff --git a/scripts/lib/code-scan-awk.sh b/scripts/lib/code-scan-awk.sh new file mode 100644 index 000000000..2fa47568c --- /dev/null +++ b/scripts/lib/code-scan-awk.sh @@ -0,0 +1,69 @@ +#!/usr/bin/env bash +# 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. +# +# 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 +# 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. +# +# Callers must pass -v SQ="'": the single quote cannot be written inside a +# single-quoted awk program, so it reaches awk as a variable. +# +# code_of(line) drops a trailing comment. A `#` that starts a word +# ends the line in both YAML and shell, and in +# neither is the `#` of `a#b` a comment, so the +# word-start rule is the one rule both languages +# already use. +# invokes(line, cmdre) code_of() with quoted string literals emptied, +# matched against cmdre. A command named inside +# `"..."` or `'...'` is prose, not an invocation. +# pipes_to_selector(line, whether the line pipes into +# argre) scripts/select-owned-name.sh with an argument +# matching argre. Runs on code_of() alone, because +# it asserts the literal text of the selector's +# quoted argument and emptying literals would erase +# it. Running it on the raw line instead would let a +# commented-out selector stage mask an unguarded +# destructive call in the same step, which fails +# open. +# +# The path is matched as "anything with no pipe or +# space in it, ending in a slash" so both call forms +# are recognised: a workflow's relative +# `./scripts/select-owned-name.sh` and a sibling +# script's `"${SCRIPT_DIR}/select-owned-name.sh"`, +# which resolves from BASH_SOURCE rather than from +# the caller's working directory. Requiring the +# slash immediately before the file name keeps +# `| cat select-owned-name.sh "$X"` from counting. +# +# `[|]` and `[$]` rather than `\|` and `\$` in the regexes here and in the +# callers: escaping those is undefined in POSIX ERE, and CI's awk is mawk rather +# than the awk this was written on. `^#` and ` #` as two subs rather than one +# `(^|[[:space:]])#` alternation, for the same reason: anchors inside a group +# are not portable across awk implementations. + +# shellcheck disable=SC2034 # read by the suites that source this file +AWK_CODE_FUNCS=' + function code_of(line) { + sub(/^#.*$/, "", line) + sub(/[[:space:]]#.*$/, "", line) + return line + } + function invokes(line, cmdre) { + line = code_of(line) + gsub(/"[^"]*"/, "", line) + gsub(SQ "[^" SQ "]*" SQ, "", line) + return line ~ cmdre + } + function pipes_to_selector(line, argre) { + return code_of(line) ~ ("[|][[:space:]]*\"?[^|[:space:]]*/select-owned-name\\.sh\"?[[:space:]]+" argre) + } +' diff --git a/scripts/select-ecr-repos-to-delete.sh b/scripts/select-ecr-repos-to-delete.sh deleted file mode 100755 index 21b9d2de1..000000000 --- a/scripts/select-ecr-repos-to-delete.sh +++ /dev/null @@ -1,73 +0,0 @@ -#!/usr/bin/env bash -# select-ecr-repos-to-delete.sh -# -# Selects which ECR repositories a destroy workflow may force-delete. -# -# Reads the account's repository names on stdin, one per line, and prints back -# only the ones that are byte-for-byte identical to the owned name passed as the -# single argument. Nothing else is ever printed, so the caller can pipe the -# output straight into `aws ecr delete-repository --force`. -# -# The owned name comes from `terraform output -raw ecr_repository_name`, i.e. -# from the state the destroy is about to tear down, so it is the name this -# environment actually created rather than a pattern someone hopes only matches -# that name. The repository is `local.stack_name` -# (terraform/environments/aws/main.tf), which carries a random suffix, so no -# literal list can be hardcoded here and no prefix describes it uniquely: -# `cudly-dev--backup` shares every prefix the real repository has. -# -# The previous filter was `contains(repositoryName,'cudly-dev')` evaluated -# inside the destroy workflow, which also selected `backup-cudly-dev` and -# `cudly-dev-prod-mirror` and force-deleted every image in them. A -# `starts_with`/`case cudly-dev*` allow-list, the shape cleanup-staging.yml -# uses, still selects `cudly-dev-prod-mirror`; equality is the only comparison -# that does not. -# -# Exit codes: -# 0 selection completed (an empty selection is normal -- the repository may -# already be gone, and the caller must not treat that as an error, so it -# is reported on stderr rather than through the exit code) -# 2 usage error, including an empty or whitespace-bearing owned name, which -# is what a failed `terraform output` looks like. Never degrades into -# "select nothing" or "select everything". - -set -euo pipefail - -if [[ $# -ne 1 ]]; then - echo "usage: $(basename "$0") OWNED_REPOSITORY_NAME < repository-names" >&2 - echo " reads candidate repository names on stdin, one per line" >&2 - exit 2 -fi - -owned="$1" - -# ECR repository names contain no whitespace, so anything that does is not a -# name this script was handed on purpose -- most likely an empty or -# warning-polluted `terraform output`. Refuse rather than guess. -case "$owned" in - '' | *[![:graph:]]*) - echo "error: owned repository name must be non-empty and free of whitespace; got '${owned}'" >&2 - exit 2 - ;; -esac - -found=0 - -# `|| [[ -n "$candidate" ]]` so a final line with no trailing newline is still -# compared. `read` returns non-zero on such a line, which would otherwise drop -# the one entry the caller is looking for and report an empty selection. -while IFS= read -r candidate || [[ -n "$candidate" ]]; do - # Quoted right-hand side: [[ ]] would otherwise treat it as a glob pattern, - # which is the same class of over-matching this script exists to remove. - if [[ "$candidate" == "$owned" ]]; then - printf '%s\n' "$candidate" - found=1 - fi -done - -# An empty selection is a normal outcome (exit 0 above), but it is also what a -# listing taken from the wrong region or account looks like. Say which name was -# looked for so the two are distinguishable in the caller's log. -if [[ "$found" -eq 0 ]]; then - echo "note: '${owned}' is not present in the listing on stdin; nothing to delete (already deleted, or the listing came from a different region or account)" >&2 -fi diff --git a/scripts/select-owned-name.sh b/scripts/select-owned-name.sh new file mode 100755 index 000000000..3265e8b3f --- /dev/null +++ b/scripts/select-owned-name.sh @@ -0,0 +1,82 @@ +#!/usr/bin/env bash +# select-owned-name.sh +# +# Selects which AWS resources a destroy workflow may act destructively on. +# +# Reads the account's resource names on stdin, one per line, and prints back +# only the ones that are byte-for-byte identical to the owned name passed as the +# single argument. Nothing else is ever printed, so the caller can pipe the +# output straight into the destructive command. +# +# The owned name comes from `terraform output` on the state the destroy is about +# to tear down, so it is the name this environment actually created rather than +# a pattern someone hopes only matches that name. Both current callers pass a +# name derived from `local.stack_name` +# (terraform/environments/aws/main.tf), which carries a random suffix, so no +# literal list can be hardcoded here and no prefix describes it uniquely: +# `cudly-dev--backup` shares every prefix the real name has. +# +# The comparison is deliberately resource-agnostic, and shared rather than +# copied per resource: two copies of it would have to be hardened in lockstep, +# and a guard landing on one resource and not its sibling is precisely how #1592 +# became #1820 and then #1821. The callers are: +# +# force-delete-owned-ecr-repo.sh `aws ecr delete-repository --force` +# disable-owned-rds-deletion-protection.sh `aws rds modify-db-instance +# --no-deletion-protection` +# +# The filters this replaced were `contains(repositoryName,'cudly-dev')` and +# `starts_with(DBInstanceIdentifier,'cudly-dev')` evaluated inside the destroy +# workflows. `contains` also selected `backup-cudly-dev` and +# `cudly-dev-prod-mirror`; `starts_with` still selects `cudly-dev-prod-mirror` +# and `cudly-dev--replica`. Equality is the only comparison that does not. +# +# Exit codes: +# 0 selection completed (an empty selection is normal -- the resource may +# already be gone, and the caller must not treat that as an error, so it +# is reported on stderr rather than through the exit code) +# 2 usage error, including an empty or whitespace-bearing owned name, which +# is what a failed `terraform output` looks like. Never degrades into +# "select nothing" or "select everything". + +set -euo pipefail + +if [[ $# -ne 1 ]]; then + echo "usage: $(basename "$0") OWNED_NAME < candidate-names" >&2 + echo " reads candidate resource names on stdin, one per line" >&2 + exit 2 +fi + +owned="$1" + +# Neither an ECR repository name nor an RDS DBInstanceIdentifier may contain +# whitespace, so anything that does is not a name this script was handed on +# purpose -- most likely an empty or warning-polluted `terraform output`. Refuse +# rather than guess. +case "$owned" in + '' | *[![:graph:]]*) + echo "error: owned name must be non-empty and free of whitespace; got '${owned}'" >&2 + exit 2 + ;; +esac + +found=0 + +# `|| [[ -n "$candidate" ]]` so a final line with no trailing newline is still +# compared. `read` returns non-zero on such a line, which would otherwise drop +# the one entry the caller is looking for and report an empty selection. +while IFS= read -r candidate || [[ -n "$candidate" ]]; do + # Quoted right-hand side: [[ ]] would otherwise treat it as a glob pattern, + # which is the same class of over-matching this script exists to remove. + if [[ "$candidate" == "$owned" ]]; then + printf '%s\n' "$candidate" + found=1 + fi +done + +# An empty selection is a normal outcome (exit 0 above), but it is also what a +# listing taken from the wrong region or account looks like. Say which name was +# looked for so the two are distinguishable in the caller's log. +if [[ "$found" -eq 0 ]]; then + echo "note: '${owned}' is not present in the listing on stdin; nothing to act on (already deleted, or the listing came from a different region or account)" >&2 +fi diff --git a/scripts/test-select-ecr-repos-to-delete.sh b/scripts/test-ecr-delete-selection.sh similarity index 90% rename from scripts/test-select-ecr-repos-to-delete.sh rename to scripts/test-ecr-delete-selection.sh index 0c80e7381..43afe3047 100755 --- a/scripts/test-select-ecr-repos-to-delete.sh +++ b/scripts/test-ecr-delete-selection.sh @@ -1,7 +1,8 @@ #!/usr/bin/env bash -# test-select-ecr-repos-to-delete.sh +# test-ecr-delete-selection.sh # -# Exercises select-ecr-repos-to-delete.sh in BOTH directions over a table of +# Exercises select-owned-name.sh, as the ECR destroy path uses it, in BOTH +# directions over a table of # real and adversarial repository names. Both directions matter because the # consumer force-deletes what this selector prints: a selector that matches # nothing passes every "no longer over-matches" assertion while silently @@ -13,7 +14,7 @@ set -euo pipefail SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" -SELECT="${SCRIPT_DIR}/select-ecr-repos-to-delete.sh" +SELECT="${SCRIPT_DIR}/select-owned-name.sh" # The repository terraform/environments/aws creates for the dev stack: # local.stack_name = "${project_name}-${environment}-${random_id.suffix.hex}". @@ -264,47 +265,17 @@ CLEANUP_SCRIPT="${SCRIPT_DIR}/force-delete-owned-ecr-repo.sh" # ABOUT the command, which is the same "the string is present somewhere" mistake # the selector itself exists to remove, one level up. # -# So the awk programs match the shell code on a line rather than the raw line: -# -# code_of() drops a trailing comment. A `#` that starts a word ends the -# line in both YAML and shell, and in neither is the `#` of -# `a#b` a comment, so the word-start rule is the one rule -# both languages already use. -# invokes_delete() additionally empties quoted string literals, so a command -# named inside `"..."` or `'...'` is prose, not an invocation. -# -# pipes_to_selector() runs on code_of() alone, because it asserts the literal -# text of the selector's quoted argument and emptying literals would erase it. -# Running it on the raw line instead would let a commented-out selector stage -# mask an unguarded delete in the same step, which fails open. -# -# Its path is matched as "anything with no pipe or space in it, ending in a -# slash" so both call forms are recognised: the workflow fixtures' relative -# `./scripts/select-...` and the shared script's `"${SCRIPT_DIR}/select-..."`, -# which resolves the sibling script from BASH_SOURCE rather than from the -# caller's working directory. Requiring the slash immediately before the file -# name keeps `| cat select-ecr-repos-to-delete.sh "$X"` from counting. -# -# `^#` and ` #` as two subs rather than one `(^|[[:space:]])#` alternation: -# anchors inside a group are not portable across awk implementations, and CI's -# awk is mawk rather than the awk this was written on. The single quote reaches -# awk through -v because it cannot be written inside the single-quoted program. -AWK_CODE_FUNCS=' - function code_of(line) { - sub(/^#.*$/, "", line) - sub(/[[:space:]]#.*$/, "", line) - return line - } - function invokes_delete(line) { - line = code_of(line) - gsub(/"[^"]*"/, "", line) - gsub(SQ "[^" SQ "]*" SQ, "", line) - return line ~ /aws[[:space:]]+ecr[[:space:]]+delete-repository/ - } - function pipes_to_selector(line, argre) { - return code_of(line) ~ ("[|][[:space:]]*\"?[^|[:space:]]*/select-ecr-repos-to-delete\\.sh\"?[[:space:]]+" argre) - } -' +# So the awk programs match the shell code on a line rather than the raw line, +# through code_of() / invokes() / pipes_to_selector() in +# scripts/lib/code-scan-awk.sh. That file holds the rule and the reasoning; it +# is shared with test-rds-deletion-protection-scope.sh, which guards the same +# shape on RDS and would otherwise carry a second copy of it to drift against. +# shellcheck source=scripts/lib/code-scan-awk.sh +. "${SCRIPT_DIR}/lib/code-scan-awk.sh" + +# The command whose invocation makes a site dangerous here. Passed to invokes() +# rather than baked into it, because the RDS suite scans for a different one. +DELETE_CMD_RE='aws[[:space:]]+ecr[[:space:]]+delete-repository' # assert_step_wiring WORKFLOW_FILE STEP_NAME EXPECTED_STEPS # @@ -389,13 +360,13 @@ assert_script_wiring() { return fi - if awk -v SQ="'" "$AWK_CODE_FUNCS"' + if awk -v SQ="'" -v cmdre="$DELETE_CMD_RE" "$AWK_CODE_FUNCS"' code_of($0) ~ /OWNED_REPO=.*jq[[:space:]]+-er[[:space:]]+.*\.ecr_repository_name\.value/ { owned++ } pipes_to_selector($0, "\"[$]OWNED_REPO\"") { piped++ } code_of($0) ~ /[|][[:space:]]*while[[:space:]]+IFS=[[:space:]]*read[[:space:]]+-r[[:space:]]+REPO/ { if (pipes_to_selector(prev, "\"[$]OWNED_REPO\"")) fed++ } - invokes_delete($0) { deletes++ } + invokes($0, cmdre) { deletes++ } code_of($0) ~ /aws[[:space:]]+ecr[[:space:]]+delete-repository[[:space:]]+--repository-name[[:space:]]+"[$]REPO"/ { by_loop_var++ } { prev = $0 } END { exit !(owned == 1 && piped == 1 && fed == 1 && deletes == 1 && by_loop_var == 1) } @@ -404,7 +375,7 @@ assert_script_wiring() { ((pass++)) || true else echo "FAIL: $(basename "$script") no longer reads the owned name from 'terraform" - echo " output', pipes it to scripts/select-ecr-repos-to-delete.sh, and" + echo " output', pipes it to scripts/select-owned-name.sh, and" echo " force-deletes exactly the repositories that pipeline yields -- expected" echo " one of each. This is where the deletion body lives now, so a prefix" echo " filter reintroduced here is the #1592/#1820 over-match, whatever the" @@ -473,7 +444,7 @@ sweep_unwired() { # file boundary is flushed by the next file's first line, by which point # FILENAME has already advanced and the report would send the reader to an # innocent file. Pinned by the two-file fixture below. - awk -v SQ="'" "$AWK_CODE_FUNCS"' + awk -v SQ="'" -v cmdre="$DELETE_CMD_RE" "$AWK_CODE_FUNCS"' function finish() { if (has_delete && !has_selector) { if (step_name == "") printf "%s: whole file\n", site_file @@ -489,7 +460,7 @@ sweep_unwired() { sub(/^[[:space:]]*-[[:space:]]+name:[[:space:]]*/, "", step_name) } pipes_to_selector($0, "\"[$][A-Za-z_][A-Za-z0-9_]*\"") { has_selector = 1 } - invokes_delete($0) { has_delete = 1 } + invokes($0, cmdre) { has_delete = 1 } END { finish(); if (total == 0) print "no `aws ecr delete-repository` step found at all" } ' "${files[@]}" } @@ -566,7 +537,7 @@ cat >"${FIXTURE_DIR}/wired/deletes.yml" <<'EOF' - name: Force-delete the ECR repo this state owns run: | aws ecr describe-repositories --query 'repositories[].repositoryName' --output text \ - | ./scripts/select-ecr-repos-to-delete.sh "$OWNED_REPO" \ + | ./scripts/select-owned-name.sh "$OWNED_REPO" \ | while IFS= read -r REPO; do aws ecr delete-repository --repository-name "$REPO" --force done @@ -581,7 +552,7 @@ cat >"${FIXTURE_DIR}/unwired/deletes.yml" <<'EOF' for REPO in $(aws ecr describe-repositories \ --query "repositories[?starts_with(repositoryName,'cudly-staging')].repositoryName" \ --output text); do - # | ./scripts/select-ecr-repos-to-delete.sh "$OWNED_REPO" + # | ./scripts/select-owned-name.sh "$OWNED_REPO" aws ecr delete-repository --repository-name "$REPO" --force done EOF @@ -609,7 +580,7 @@ mkdir -p "${FIXTURE_DIR}/scripts" cat >"${FIXTURE_DIR}/scripts/wired.sh" <<'EOF' aws ecr describe-repositories --query 'repositories[].repositoryName' --output text \ | tr '\t' '\n' \ - | "${SCRIPT_DIR}/select-ecr-repos-to-delete.sh" "$OWNED_REPO" \ + | "${SCRIPT_DIR}/select-owned-name.sh" "$OWNED_REPO" \ | while IFS= read -r REPO; do aws ecr delete-repository --repository-name "$REPO" --force done diff --git a/scripts/test-rds-deletion-protection-scope.sh b/scripts/test-rds-deletion-protection-scope.sh new file mode 100755 index 000000000..826136b88 --- /dev/null +++ b/scripts/test-rds-deletion-protection-scope.sh @@ -0,0 +1,623 @@ +#!/usr/bin/env bash +# test-rds-deletion-protection-scope.sh +# +# Asserts that the destroy workflows strip RDS deletion protection from the +# instance each Terraform state owns and from nothing else. +# +# Both directions matter, and the negative direction alone is worthless here: a +# selector that matches nothing passes every "no longer over-matches" assertion +# while silently leaving the owned instance protected, so `terraform destroy` +# fails on it. So the owned instance is asserted to be selected out of a full, +# hostile account listing BEFORE any absence is asserted. +# +# Deletion protection is the last line of defence on a database. Stripping it +# from an instance the state does not own does not delete anything by itself -- +# it leaves that database exposed to the next destroy that does match it, which +# is why #1821 is a security issue and not a tidiness one. +# +# 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" +SELECT="${SCRIPT_DIR}/select-owned-name.sh" +UNPROTECT_SCRIPT="${SCRIPT_DIR}/disable-owned-rds-deletion-protection.sh" + +# code_of() / invokes() / pipes_to_selector(), shared with +# test-ecr-delete-selection.sh so the rule separating code that RUNS a command +# from prose that mentions it has one definition rather than two that drift. +# shellcheck source=scripts/lib/code-scan-awk.sh +. "${SCRIPT_DIR}/lib/code-scan-awk.sh" + +# The command whose invocation makes a site dangerous here. +MODIFY_CMD_RE='aws[[:space:]]+rds[[:space:]]+modify-db-instance' + +# The instance terraform/environments/aws creates: +# aws_db_instance.main.identifier = "${local.stack_name}-postgres", and +# local.stack_name = "${project_name}-${environment}-${random_id.suffix.hex}" +# (terraform/modules/database/aws/main.tf:121, environments/aws/main.tf:55). +# The random suffix is why no literal list can be hardcoded and no prefix +# describes the instance uniquely. +OWNED="cudly-dev-1a2b3c4d-postgres" + +# A plausible `aws rds describe-db-instances` listing in the target account. +# Every entry other than $OWNED is an instance this workflow does not own. +ACCOUNT_LISTING=$( + cat <<'EOF' +cudly-dev +cudly-dev-postgres +cudly-dev-1a2b3c4d-postgres +cudly-dev-1a2b3c4d-postgres-replica +cudly-dev-1a2b3c4d-replica +cudly-dev-prod-mirror +cudly-dev-dba-scratch +backup-cudly-dev-postgres +cudly-staging-9f8e7d6c-postgres +cudly-prod-0badc0de-postgres +EOF +) + +pass=0 +fail=0 + +# assert_case LABEL EXPECTED_EXIT EXPECTED_STDOUT ACTUAL_EXIT ACTUAL_STDOUT +assert_case() { + local label="$1" + local expected_exit="$2" + local expected_out="$3" + local actual_exit="$4" + local actual_out="$5" + + if [[ "$actual_exit" -eq "$expected_exit" && "$actual_out" == "$expected_out" ]]; then + echo "PASS: $label" + ((pass++)) || true + else + echo "FAIL: $label" + echo " expected exit $expected_exit, got $actual_exit" + echo " expected stdout: '${expected_out}'" + echo " actual stdout: '${actual_out}'" + ((fail++)) || true + fi +} + +# run_case LABEL EXPECTED_EXIT EXPECTED_STDOUT STDIN [ARGS...] +run_case() { + local label="$1" + local expected_exit="$2" + local expected_out="$3" + local stdin_data="$4" + shift 4 + + local actual_out actual_exit=0 + actual_out="$("$SELECT" "$@" <<<"$stdin_data" 2>/dev/null)" || actual_exit=$? + + assert_case "$label" "$expected_exit" "$expected_out" "$actual_exit" "$actual_out" +} + +# --- Positive direction FIRST: the owned instance is still selected ---------- +# +# Everything below this point asserts that some identifier is NOT selected, and +# every one of those assertions is satisfied by a selector that selects nothing +# at all. These two run first and are counted, so the negative table can never +# be the only thing holding. + +selected="$("$SELECT" "$OWNED" <<<"$ACCOUNT_LISTING" 2>/dev/null)" + +# Counted in the shell rather than with `grep -c`, which prints 0 and exits 1 on +# an empty selection: the exact case this assertion exists to catch, arriving as +# a non-zero exit that a `|| true` would then have to launder. +selected_count=0 +while IFS= read -r line; do + if [[ -n "$line" ]]; then + ((selected_count++)) || true + fi +done <<<"$selected" + +if [[ "$selected_count" -eq 1 ]]; then + echo "PASS: exactly one instance is selected out of the hostile account listing" + ((pass++)) || true +else + echo "FAIL: expected exactly 1 selected instance out of the account listing, got ${selected_count}" + echo " a selection of 0 leaves the owned instance protected and the destroy fails;" + echo " a selection of >1 strips protection from a database this state does not own" + ((fail++)) || true +fi + +run_case "owned instance is selected out of the full account listing" \ + 0 "$OWNED" "$ACCOUNT_LISTING" "$OWNED" + +run_case "selection is not tied to one suffix (different random_id)" \ + 0 "cudly-dev-deadbeef-postgres" \ + "$(printf 'cudly-dev\ncudly-dev-deadbeef-postgres\ncudly-dev-deadbeef-postgres-replica\n')" \ + "cudly-dev-deadbeef-postgres" + +run_case "owned instance already destroyed selects nothing, exit 0" \ + 0 "" \ + "$(printf 'cudly-staging-9f8e7d6c-postgres\ncudly-prod-0badc0de-postgres\n')" \ + "$OWNED" + +# --- Negative direction: every prefix near-miss is refused ------------------- +# +# Each on its own line so a failure names the database that would have been +# stripped of its protection. The trailing comment is the filter that selects +# it: `starts_with cudly-dev` is what shipped at all three sites, and +# `cudly-dev-1a2b3c4d*` is the tightest prefix that still describes the real +# instance. +while IFS='|' read -r instance caught_by; do + [[ -n "$instance" ]] || continue + run_case "refused: ${instance} (selected by ${caught_by})" \ + 0 "" "$instance" "$OWNED" +done <<'EOF' +cudly-dev|starts_with cudly-dev +cudly-dev-postgres|starts_with cudly-dev +cudly-dev-prod-mirror|starts_with cudly-dev +cudly-dev-1a2b3c4d-replica|starts_with cudly-dev +cudly-dev-1a2b3c4d-postgres-replica|starts_with cudly-dev, prefix of the real instance +cudly-dev-dba-scratch|starts_with cudly-dev, an operator-named instance +backup-cudly-dev-postgres|contains cudly-dev +cudly-staging-9f8e7d6c-postgres|no filter, regression guard +cudly-prod-0badc0de-postgres|no filter, regression guard +EOF + +# --- The staging pair: each state owns one instance, and only its own -------- +# +# cleanup-staging.yml destroys two AWS states from the same +# terraform/environments/aws directory (github-staging and +# github-fargate-staging), and each creates its own +# `cudly-staging--postgres` instance. The `cudly-staging` +# prefix both jobs selected by therefore matched the sibling job's database as +# well, so either job unprotected a database the other's state still owned. +STAGING_LAMBDA_OWNED="cudly-staging-9f8e7d6c-postgres" +STAGING_FARGATE_OWNED="cudly-staging-5e4d3c2b-postgres" + +STAGING_LISTING=$( + cat <<'EOF' +cudly-staging +cudly-staging-9f8e7d6c-postgres +cudly-staging-5e4d3c2b-postgres +cudly-staging-9f8e7d6c-postgres-replica +cudly-staging-prod-mirror +backup-cudly-staging-postgres +cudly-prod-0badc0de-postgres +EOF +) + +run_case "staging lambda state selects its own instance out of the staging listing" \ + 0 "$STAGING_LAMBDA_OWNED" "$STAGING_LISTING" "$STAGING_LAMBDA_OWNED" + +run_case "staging fargate state selects its own instance out of the staging listing" \ + 0 "$STAGING_FARGATE_OWNED" "$STAGING_LISTING" "$STAGING_FARGATE_OWNED" + +run_case "staging lambda state does not unprotect the fargate state's database" \ + 0 "" "$STAGING_FARGATE_OWNED" "$STAGING_LAMBDA_OWNED" + +run_case "staging fargate state does not unprotect the lambda state's database" \ + 0 "" "$STAGING_LAMBDA_OWNED" "$STAGING_FARGATE_OWNED" + +# A failed `terraform output` hands the selector an empty string. That must be a +# loud failure and not a silent empty selection, which looks identical to "the +# instance is already gone" and lets the destroy report success. +run_case "empty owned identifier exits 2" 2 "" "$ACCOUNT_LISTING" "" + +# --- Terraform: the state actually publishes the identifier ------------------ +# +# The script resolves the owned instance from `terraform output`, so the whole +# guard rests on that output existing and being the instance's real identifier. +# Delete the output and every case above stays green while the destroy fails at +# runtime on a state that cannot name what it owns. + +# assert_file_matches LABEL FILE AWK_CONDITION +assert_file_matches() { + local label="$1" + local file="$2" + local condition="$3" + + if [[ ! -f "$file" ]]; then + echo "FAIL: ${label} -- file not found at ${file}" + ((fail++)) || true + return + fi + + if awk -v SQ="'" "$AWK_CODE_FUNCS"' + '"$condition"' + END { exit !(hits == 1) } + ' "$file"; then + echo "PASS: $label" + ((pass++)) || true + else + echo "FAIL: $label" + echo " in $(basename "$file") -- expected exactly one match" + ((fail++)) || true + fi +} + +# Matched across the whole `output` block rather than on one line: the value is +# on the line after the block header, so the two are correlated by remembering +# which block is open. `[{]` and `[}]` rather than bare braces, which start an +# interval expression in ERE -- CI's awk is mawk rather than the awk this was +# written on, the same reason the shared helpers avoid `\|` and `\$`. +assert_file_matches "the aws environment publishes database_instance_identifier from the database module" \ + "${REPO_ROOT}/terraform/environments/aws/outputs.tf" \ + 'code_of($0) ~ /^output[[:space:]]+"database_instance_identifier"[[:space:]]*[{]/ { in_block = 1; next } + in_block && code_of($0) ~ /^[}]/ { in_block = 0 } + in_block && code_of($0) ~ /value[[:space:]]*=[[:space:]]*module\.database\.instance_identifier[[:space:]]*$/ { hits++ }' + +assert_file_matches "the aws database module publishes the real aws_db_instance identifier" \ + "${REPO_ROOT}/terraform/modules/database/aws/outputs.tf" \ + 'code_of($0) ~ /^output[[:space:]]+"instance_identifier"[[:space:]]*[{]/ { in_block = 1; next } + in_block && code_of($0) ~ /^[}]/ { in_block = 0 } + in_block && code_of($0) ~ /value[[:space:]]*=[[:space:]]*aws_db_instance\.main\.identifier[[:space:]]*$/ { hits++ }' + +# --- Wiring: the destroy steps route through the shared script --------------- +# +# Every case above exercises the selector standalone. Revert a step to a +# `starts_with` loop and all of them stay green while that step strips +# protection from whatever the prefix matches. The recurrence mode that produced +# #1592, then #1820, then #1821 was exactly that: the guard landed on one +# resource or one workflow and not its sibling. + +# assert_step_wiring WORKFLOW_FILE STEP_NAME EXPECTED_STEPS +# +# Asserts WORKFLOW_FILE contains exactly EXPECTED_STEPS steps named STEP_NAME +# and that EVERY one runs the shared script against terraform/environments/aws. +# The count matters where a file holds more than one such step +# (cleanup-staging.yml destroys two AWS states): without it, a run where one +# step keeps the call and the other drops it satisfies a per-file flag. +# +# The state directory is pinned rather than accepted as any argument because it +# decides which instance the call may unprotect. The regex ends at end-of-line, +# so a `|| true` appended to the call fails this too. +assert_step_wiring() { + local workflow="$1" + local step="$2" + local expected="$3" + + if [[ ! -f "$workflow" ]]; then + echo "FAIL: consumer workflow not found at ${workflow}" + ((fail++)) || true + return + fi + + if awk -v SQ="'" -v step="$step" -v expected="$expected" "$AWK_CODE_FUNCS"' + function finish() { + if (in_step) { + steps++ + if (!has_call) unwired++ + } + in_step = 0; has_call = 0 + } + $0 ~ ("^[[:space:]]*-[[:space:]]+name:[[:space:]]*" step "[[:space:]]*$") { + finish(); in_step = 1; next + } + /^[[:space:]]*-[[:space:]]+name:/ { finish() } + in_step && code_of($0) ~ /[[:space:]]\.\/scripts\/disable-owned-rds-deletion-protection\.sh[[:space:]]+terraform\/environments\/aws[[:space:]]*$/ { has_call = 1 } + END { finish(); exit !(steps == expected && unwired == 0) } + ' "$workflow"; then + echo "PASS: all ${expected} '${step}' step(s) in $(basename "$workflow") call the shared script" + ((pass++)) || true + else + echo "FAIL: $(basename "$workflow") does not have exactly ${expected} step(s) named" + echo " '${step}' that each run" + echo " './scripts/disable-owned-rds-deletion-protection.sh terraform/environments/aws'" + echo " (call removed, state directory changed, failure swallowed with a trailing" + echo " '|| true', step renamed, or a step added/deleted). The cases above only" + echo " exercise the selector standalone, so they stay green while the #1821" + echo " prefix over-match returns" + ((fail++)) || true + fi +} + +RDS_STEP="Disable RDS deletion protection on the instance this state owns" +assert_step_wiring "${WORKFLOW_DIR}/destroy-fargate-dev.yml" "$RDS_STEP" 1 +assert_step_wiring "${WORKFLOW_DIR}/cleanup-staging.yml" "$RDS_STEP" 2 + +# assert_script_wiring SCRIPT +# +# The other half: the script those steps call must still unprotect only what the +# exact-match selector yields. Five counts, each exactly one, so a second +# unguarded modify added beside the guarded one is caught, and so is a guard +# removed entirely: +# +# owned the identifier is read from `terraform output`, not hardcoded +# and not derived from a prefix +# piped that identifier reaches the selector +# fed the modify loop reads the selector's output, so the selector +# cannot be reduced to a no-op stage beside a modify driven by +# some other listing +# modifies there is exactly one `aws rds modify-db-instance` +# by_loop_var the instance it modifies is the one the loop read, not some +# other identifier that happened to be in scope +# +# `modifies` is also what keeps the rest from passing vacuously: a script that +# modifies nothing at all satisfies every "is guarded" reading of them. +assert_script_wiring() { + local script="$1" + + if [[ ! -f "$script" ]]; then + echo "FAIL: shared script not found at ${script}" + ((fail++)) || true + return + fi + + if awk -v SQ="'" -v cmdre="$MODIFY_CMD_RE" "$AWK_CODE_FUNCS"' + code_of($0) ~ /OWNED_INSTANCE=.*jq[[:space:]]+-er[[:space:]]+.*\.database_instance_identifier\.value/ { owned++ } + pipes_to_selector($0, "\"[$]OWNED_INSTANCE\"") { piped++ } + code_of($0) ~ /[|][[:space:]]*while[[:space:]]+IFS=[[:space:]]*read[[:space:]]+-r[[:space:]]+INSTANCE_ID/ { + if (pipes_to_selector(prev, "\"[$]OWNED_INSTANCE\"")) fed++ + } + invokes($0, cmdre) { modifies++ } + code_of($0) ~ /aws[[:space:]]+rds[[:space:]]+modify-db-instance[[:space:]]+--db-instance-identifier[[:space:]]+"[$]INSTANCE_ID"/ { by_loop_var++ } + { prev = $0 } + END { exit !(owned == 1 && piped == 1 && fed == 1 && modifies == 1 && by_loop_var == 1) } + ' "$script"; then + echo "PASS: $(basename "$script") unprotects only what the exact-match selector yields" + ((pass++)) || true + else + echo "FAIL: $(basename "$script") no longer reads the owned identifier from 'terraform" + echo " output', pipes it to scripts/select-owned-name.sh, and modifies exactly the" + echo " instances that pipeline yields -- expected one of each. This is where the" + echo " body lives, so a prefix filter reintroduced here is the #1821 over-match," + echo " whatever the call sites look like" + ((fail++)) || true + fi +} + +assert_script_wiring "$UNPROTECT_SCRIPT" + +# assert_nothing_swallowed SCRIPT +# +# The second half of #1821: the sites carried `2>/dev/null` on the listing and +# `|| true` on the modify, so a failed listing (indistinguishable from an +# account with no instances) and a failed modify both reported success, and +# `terraform destroy` then failed downstream on an instance that was still +# protected. Asserted on code_of() so the header, which quotes both to explain +# why they are gone, is not itself a violation. +assert_nothing_swallowed() { + local script="$1" + + if awk -v SQ="'" "$AWK_CODE_FUNCS"' + code_of($0) ~ /2>[[:space:]]*\/dev\/null/ { print " swallowed stderr: " FNR; bad++ } + code_of($0) ~ /[|][|][[:space:]]*true/ { print " swallowed exit status: " FNR; bad++ } + END { exit !(bad == 0) } + ' "$script"; then + echo "PASS: $(basename "$script") swallows neither a failed listing nor a failed modify" + ((pass++)) || true + else + echo "FAIL: $(basename "$script") suppresses an error on the line(s) above. A partial or" + echo " failed strip must fail loudly (#1821), not report success and let" + echo " 'terraform destroy' hit a confusing downstream error" + ((fail++)) || true + fi +} + +assert_nothing_swallowed "$UNPROTECT_SCRIPT" + +# The assertions above name the files and steps they know about, so a NEW modify +# site in a new step, workflow or script is invisible to them -- which is how +# #1820 outlived #1592 and #1821 outlived both. This sweep is keyed on the +# dangerous call instead of on a name: everything anywhere in the swept set that +# runs `aws rds modify-db-instance` must pipe through the selector, whatever it +# is called. +# +# It also reports when it finds no modify site at all, because that is what a +# wrong directory or an unmatched glob looks like, and an empty sweep would +# otherwise read as a clean result for files it never opened. Both workflow +# extensions GitHub accepts are swept, so a new `.yaml` file cannot slip past. +# scripts/ is named file by file rather than globbed because this suite itself +# lives there and quotes both the command and the selector, as data. + +# sweep_unwired DIR [FILE...] +# +# Prints one line per modify site in DIR (plus each named FILE) that does not +# pipe through the selector, plus a line of its own when the swept set holds no +# modify site at all. No output means the swept set is clean. +# +# Sites are delimited by workflow `- name:` lines. A shell script has none, so +# it is swept as a single site and reported as "whole file". +sweep_unwired() { + local dir="$1" + shift + 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 + + # The site is reported from site_file, not FILENAME: a site that ends at a + # file boundary is flushed by the next file's first line, by which point + # FILENAME has already advanced and the report would send the reader to an + # innocent file. Pinned by the two-file fixture below. + awk -v SQ="'" -v cmdre="$MODIFY_CMD_RE" "$AWK_CODE_FUNCS"' + function finish() { + if (has_modify && !has_selector) { + if (step_name == "") printf "%s: whole file\n", site_file + else printf "%s: step \"%s\"\n", site_file, step_name + } + if (has_modify) total++ + has_modify = 0; has_selector = 0; step_name = "" + } + FNR == 1 { if (NR > 1) finish(); site_file = FILENAME } + /^[[:space:]]*-[[:space:]]+name:/ { + finish() + step_name = $0 + sub(/^[[:space:]]*-[[:space:]]+name:[[:space:]]*/, "", step_name) + } + pipes_to_selector($0, "\"[$][A-Za-z_][A-Za-z0-9_]*\"") { has_selector = 1 } + invokes($0, cmdre) { has_modify = 1 } + END { finish(); if (total == 0) print "no `aws rds modify-db-instance` step found at all" } + ' "${files[@]}" +} + +# assert_sweep LABEL DIR EXPECTED [FILE...] +# +# EXPECTED empty asserts the sweep finds nothing; otherwise it asserts EXPECTED +# appears in the report, so a fixture pins WHICH site was flagged rather than +# only that something was. +assert_sweep() { + local label="$1" + local dir="$2" + local expected="$3" + shift 3 + local report + + report="$(sweep_unwired "$dir" "$@")" + + if [[ -z "$expected" && -z "$report" ]] || [[ -n "$expected" && "$report" == *"$expected"* ]]; then + echo "PASS: $label" + ((pass++)) || true + else + echo "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 + ((fail++)) || true + fi +} + +assert_sweep "every 'aws rds modify-db-instance' site in .github/workflows and scripts/ pipes through the selector" \ + "$WORKFLOW_DIR" "" "$UNPROTECT_SCRIPT" "$SELECT" + +# --- The sweep itself, in both directions, over fixtures --------------------- +# +# The sweep is the only assertion covering modify sites nobody has named, so a +# sweep that quietly stops recognizing them fails open. These fixtures pin both +# directions of that recognition, including the prose case: a guard that fired +# on ci.yml's comment describing this assertion would police what may be written +# rather than what is run. +FIXTURE_DIR="$(mktemp -d)" +trap 'rm -rf "$FIXTURE_DIR"' EXIT +mkdir -p "${FIXTURE_DIR}/prose" "${FIXTURE_DIR}/wired" "${FIXTURE_DIR}/unwired" "${FIXTURE_DIR}/scripts" "${FIXTURE_DIR}/misattrib" + +cat >"${FIXTURE_DIR}/prose/mentions.yml" <<'EOF' + - name: Describes the command without running it + run: | + # asserts every `aws rds modify-db-instance` step is wired + echo "would run aws rds modify-db-instance if it were wired" + echo 'aws rds modify-db-instance is named here too' +EOF + +cat >"${FIXTURE_DIR}/wired/modifies.yml" <<'EOF' + - name: Describes the command without running it + run: | + # asserts every `aws rds modify-db-instance` step is wired + echo "would run aws rds modify-db-instance if it were wired" + + - name: Disable RDS deletion protection on the instance this state owns + run: | + aws rds describe-db-instances --query 'DBInstances[].DBInstanceIdentifier' --output text \ + | ./scripts/select-owned-name.sh "$OWNED_INSTANCE" \ + | while IFS= read -r INSTANCE_ID; do + aws rds modify-db-instance --db-instance-identifier "$INSTANCE_ID" \ + --no-deletion-protection --apply-immediately + done +EOF + +# The #1821 shape: a prefix filter, no selector. A commented-out selector stage +# in the same step must not satisfy the wiring, which is why the selector match +# runs on the comment-stripped line. +cat >"${FIXTURE_DIR}/unwired/modifies.yml" <<'EOF' + - name: Disable RDS deletion protection + run: | + for INSTANCE_ID in $(aws rds describe-db-instances \ + --query "DBInstances[?starts_with(DBInstanceIdentifier,'cudly-staging')].DBInstanceIdentifier" \ + --output text 2>/dev/null); do + # | ./scripts/select-owned-name.sh "$OWNED_INSTANCE" + aws rds modify-db-instance --db-instance-identifier "$INSTANCE_ID" \ + --no-deletion-protection --apply-immediately 2>/dev/null || true + done +EOF + +assert_sweep "a step that only mentions the command is not a modify site" \ + "${FIXTURE_DIR}/prose" 'no `aws rds modify-db-instance` step found at all' + +assert_sweep "a wired modify step alongside prose mentions is not flagged" \ + "${FIXTURE_DIR}/wired" "" + +assert_sweep "an unwired modify step is flagged, past a commented-out selector stage" \ + "${FIXTURE_DIR}/unwired" 'step "Disable RDS deletion protection"' + +assert_sweep "a directory holding no workflow file is reported, not passed" \ + "${FIXTURE_DIR}/empty-does-not-exist" 'no workflow files found under' + +# The body lives in a shell script rather than a workflow step, so the sweep has +# to recognise a modify site in a file with no `- name:` lines at all, and has +# to accept the sibling-script call form that resolves the selector from +# BASH_SOURCE. Both directions, over a file swept by name the way the real one +# is. The `prose` dir supplies the workflow half and contributes no modify site. +cat >"${FIXTURE_DIR}/scripts/wired.sh" <<'EOF' +aws rds describe-db-instances --query 'DBInstances[].DBInstanceIdentifier' --output text \ + | tr '\t' '\n' \ + | "${SCRIPT_DIR}/select-owned-name.sh" "$OWNED_INSTANCE" \ + | while IFS= read -r INSTANCE_ID; do + aws rds modify-db-instance --db-instance-identifier "$INSTANCE_ID" \ + --no-deletion-protection --apply-immediately + done +EOF + +cat >"${FIXTURE_DIR}/scripts/unwired.sh" <<'EOF' +aws rds describe-db-instances \ + --query "DBInstances[?starts_with(DBInstanceIdentifier,'cudly-staging')].DBInstanceIdentifier" \ + --output text \ + | while IFS= read -r INSTANCE_ID; do + aws rds modify-db-instance --db-instance-identifier "$INSTANCE_ID" \ + --no-deletion-protection --apply-immediately + done +EOF + +assert_sweep "a script calling the selector through \${SCRIPT_DIR} is not flagged" \ + "${FIXTURE_DIR}/prose" "" "${FIXTURE_DIR}/scripts/wired.sh" + +assert_sweep "an unwired script is flagged as a whole-file modify site" \ + "${FIXTURE_DIR}/prose" 'unwired.sh: whole file' "${FIXTURE_DIR}/scripts/unwired.sh" + +assert_sweep "a swept file that does not exist is reported, not passed" \ + "${FIXTURE_DIR}/prose" 'swept file not found' "${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 named the innocent file swept next, and every fixture above +# sweeps one file at a time, so none of them can catch it. +cat >"${FIXTURE_DIR}/misattrib/a-unwired.yml" <<'EOF' + - name: Disable RDS deletion protection + run: | + aws rds modify-db-instance --db-instance-identifier "$INSTANCE_ID" \ + --no-deletion-protection --apply-immediately +EOF + +cat >"${FIXTURE_DIR}/misattrib/b-innocent.yml" <<'EOF' + - name: Modifies nothing + run: | + echo "clean" +EOF + +assert_sweep "a finding names the file it came from, not the file swept after it" \ + "${FIXTURE_DIR}/misattrib" 'a-unwired.yml: step "Disable RDS deletion protection"' + +echo +echo "passed: ${pass}, failed: ${fail}" +[[ "$fail" -eq 0 ]] diff --git a/terraform/environments/aws/outputs.tf b/terraform/environments/aws/outputs.tf index 87608603e..5908c2bd1 100644 --- a/terraform/environments/aws/outputs.tf +++ b/terraform/environments/aws/outputs.tf @@ -45,6 +45,17 @@ output "database_endpoint" { value = module.database.proxy_endpoint != null ? module.database.proxy_endpoint : module.database.instance_address } +# The identifier of the one RDS instance this state owns. Read by +# scripts/disable-owned-rds-deletion-protection.sh, which strips deletion +# protection before `terraform destroy`: the destroy steps used to select by the +# `cudly-dev` / `cudly-staging` identifier prefix, which also matches +# `cudly-dev-prod-mirror` and the sibling staging state's instance (#1821). +# Publishing the identifier is what lets that selection be an equality test. +output "database_instance_identifier" { + description = "RDS instance identifier owned by this state" + value = module.database.instance_identifier +} + output "database_instance_endpoint" { description = "RDS instance endpoint" value = module.database.instance_endpoint diff --git a/terraform/modules/database/aws/outputs.tf b/terraform/modules/database/aws/outputs.tf index 3bd0f3658..3f0f50f60 100644 --- a/terraform/modules/database/aws/outputs.tf +++ b/terraform/modules/database/aws/outputs.tf @@ -1,3 +1,8 @@ +output "instance_identifier" { + description = "RDS instance identifier (DBInstanceIdentifier)" + value = aws_db_instance.main.identifier +} + output "instance_endpoint" { description = "RDS instance endpoint" value = aws_db_instance.main.endpoint From 2c217309dd61ae30124a7a8435c2414f9eeae79b Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 00:43:04 +0200 Subject: [PATCH 2/3] fix(ci): tell a state predating the RDS output apart from an empty one The five states `terraform output -json` can be in were measured rather than reasoned about, because they do not behave alike. Measured on terraform 1.10.0, the version TF_VERSION pins in the workflows that call this, and on 1.14.4; identical on both. It exits 0 in all five, so the exit code carries no information and every branch has to be driven by the payload: no state file | {} | jq length 0 | jq -er exit 1 state, no outputs | {} | jq length 0 | jq -er exit 1 key absent | {...} w/o key | jq length>0 | jq -er exit 1 key present, null | {"value":null} | jq length 1 | jq -er exit 1 key present, "" | {"value":""} | jq length 1 | jq -er exit 0, "" The last row is the trap: an empty string is neither null nor false, so `jq -er` accepts it and hands back a valid-looking empty identifier. Nothing was ever unprotected by it, since the selector refuses an empty owned name with exit 2 and that propagates through pipefail, but it got there only after `describe-db-instances` had already run, it announced "This state owns RDS instance ''" on the way, and the error named the selector's contract rather than the operator's problem. It now has its own check, before anything is announced as owned and before any AWS call. The two causes get distinct messages because they need different fixes: an absent or null key means the state predates the output and wants an apply, while a present-but-empty key means the output is there and resolved to nothing, which an apply will not fix and which wants the state inspected before anything is destroyed. The behaviour these branches describe was previously verified only outside the tree, so the suite now runs the script end to end against stubbed terraform and aws, with every AWS invocation logged so the assertions are about WHICH instance was unprotected rather than only about an exit code: the golden path issues exactly one modify and it names the owned instance, each of the four hostile neighbours keeps its protection, all five output payloads take their stated branch with no AWS call on the failing ones, and a failed listing and a failed modify each fail the step. That moves the half of #1821 that is about not swallowing failures from a claim into a CI assertion. 33 cases to 51. Removing the new guard is confirmed to fail by the specific behaviour assertion rather than by a bare non-zero exit. --- .../disable-owned-rds-deletion-protection.sh | 66 ++++++- scripts/test-rds-deletion-protection-scope.sh | 173 +++++++++++++++++- 2 files changed, 228 insertions(+), 11 deletions(-) diff --git a/scripts/disable-owned-rds-deletion-protection.sh b/scripts/disable-owned-rds-deletion-protection.sh index 7c781fac6..2ee1daf99 100755 --- a/scripts/disable-owned-rds-deletion-protection.sh +++ b/scripts/disable-owned-rds-deletion-protection.sh @@ -35,10 +35,33 @@ # Reads the identifier through `output -json`, not `output -raw`: on a state # with no outputs at all, `output -raw ` exits 0 and writes its "No # outputs found" warning to STDOUT, so the identifier would become that warning -# text. `output -json` returns `{}` for that state and the cases separate -# cleanly: -# no outputs at all -> already destroyed, skip -# outputs but not this one -> fail loudly, with the remedy named +# text. +# +# The five states `terraform output -json` can be in were measured rather than +# reasoned about, because they do not behave alike and only one of them is loud +# by default. Measured on terraform 1.10.0, the version TF_VERSION pins in the +# workflows that call this, and on 1.14.4; identical on both. `output -json` +# exits 0 in ALL five, so the exit code carries no information here and every +# branch below is driven by the payload: +# +# state | -json | jq length | jq -er .value | handled as +# -------------------|---------------|-----------|---------------|------------ +# no state file | {} | 0 | exit 1 | skip, exit 0 +# state, no outputs | {} | 0 | exit 1 | skip, exit 0 +# key absent | {...} w/o key | >=1 | exit 1 | exit 1, remedy +# key present, null | {"value":null}| 1 | exit 1 | exit 1, remedy +# key present, "" | {"value":""} | 1 | exit 0, "" | exit 1, remedy +# +# The last row is the trap: an empty string is neither null nor false, so `jq +# -er` accepts it and hands the caller a valid-looking empty identifier. It is +# caught by its own check rather than left to the selector, for two reasons. The +# selector does refuse it (exit 2, so nothing is ever unprotected), but only +# after `describe-db-instances` has already run, and its message names the +# selector's contract rather than the operator's actual problem. The two causes +# also need different fixes, so they get different messages: "key absent" means +# the state predates the output and wants an apply, while "empty" means the +# state HAS the output and it resolved to nothing, which is a real defect in the +# state or the module and an apply will not fix it. # # Nothing is swallowed. The `2>/dev/null || true` the callers used to carry # reported success after a failed listing or a failed modify, and a failed @@ -51,7 +74,9 @@ # Exit codes: # 0 completed, including the "state already destroyed" and "instance already # gone" cases, which are normal outcomes and not errors -# 1 the state publishes outputs but not the owned identifier +# 1 the owned identifier cannot be resolved from a state that has outputs: +# the key is absent or null (state predates the output), or it is present +# and empty (state or module defect). Distinct messages, distinct remedies. # 2 usage error (wrong arity, or a state directory that does not exist) # * anything the AWS CLI, terraform, jq or the selector fails with, unmasked @@ -82,14 +107,35 @@ fi # rather than a fallback: the only fallback available is the identifier prefix # this script exists to remove. if ! OWNED_INSTANCE="$(jq -er '.database_instance_identifier.value' <<<"$OUTPUTS_JSON")"; then - echo "error: state '${STATE_DIR}' publishes outputs but not 'database_instance_identifier'," >&2 - echo " so the instance this state owns cannot be identified. Re-apply the state to" >&2 - echo " publish the output, or remove deletion protection on that one instance by" >&2 - echo " hand, then re-run the destroy. Refusing to fall back to an identifier" >&2 - echo " prefix, which strips protection from instances this state does not own." >&2 + echo "error: state '${STATE_DIR}' has outputs, but 'database_instance_identifier' is" >&2 + echo " absent or null, so the instance this state owns cannot be identified." >&2 + echo " This state was last applied before that output existed (#1821)." >&2 + echo " Fix: re-apply this state to publish the output, or remove deletion" >&2 + echo " protection on that one instance by hand, then re-run the destroy." >&2 + echo " Refusing to fall back to an identifier prefix, which strips protection" >&2 + echo " from instances this state does not own." >&2 exit 1 fi +# An empty string is neither null nor false, so `jq -er` above accepts it. Its +# own check, before anything is echoed as owned and before any AWS call, because +# it means something different from the branch above and an apply will not fix +# it. Whitespace is refused for the same reason the selector refuses it: no +# DBInstanceIdentifier contains any, so it is not a value this was handed on +# purpose. +case "$OWNED_INSTANCE" in + '' | *[![:graph:]]*) + echo "error: state '${STATE_DIR}' publishes 'database_instance_identifier', but it" >&2 + echo " resolved to '${OWNED_INSTANCE}', which is not an instance identifier." >&2 + echo " Unlike an absent output, re-applying will NOT fix this: the output is" >&2 + echo " there and empty, so either the state is corrupt or the module stopped" >&2 + echo " populating it. Inspect 'terraform -chdir=${STATE_DIR} output -json'" >&2 + echo " before destroying anything. Refusing to fall back to an identifier" >&2 + echo " prefix, which strips protection from instances this state does not own." >&2 + exit 1 + ;; +esac + echo "This state owns RDS instance '$OWNED_INSTANCE'" aws rds describe-db-instances --query 'DBInstances[].DBInstanceIdentifier' --output text \ diff --git a/scripts/test-rds-deletion-protection-scope.sh b/scripts/test-rds-deletion-protection-scope.sh index 826136b88..6cfd04338 100755 --- a/scripts/test-rds-deletion-protection-scope.sh +++ b/scripts/test-rds-deletion-protection-scope.sh @@ -394,6 +394,175 @@ assert_nothing_swallowed() { assert_nothing_swallowed "$UNPROTECT_SCRIPT" +# --- Behaviour: the script run end to end against stubbed terraform and aws --- +# +# Everything above is static. None of it can show that the pipeline actually +# unprotects the right instance, that a failed call is really not swallowed, or +# that the `terraform output` branches behave as their table claims -- a script +# can satisfy every text assertion and still do the wrong thing at runtime. +# +# `terraform` and `aws` are stubbed on PATH, and every aws invocation is logged +# so the assertions can be made about WHICH instance was unprotected rather than +# only about the exit code. The stub ends in an explicit `exit 0`: written as a +# trailing `[[ guard ]] && { ... }` it returns 1 whenever the guard is false, so +# every modify "failed" and the golden path looked like a script bug. +# +# The five `terraform output -json` payloads are the measured ones from the +# script's header table. The empty-string row is the one that matters: it is +# neither null nor false, so `jq -er` accepts it, and without its own check the +# script would announce an empty identifier as owned and call AWS before the +# selector refused it. +STUB_DIR="$(mktemp -d)" +STUB_STATE="$(mktemp -d)" +STUB_CALLS="$(mktemp)" +trap 'rm -rf "$STUB_DIR" "$STUB_STATE" "$STUB_CALLS"' EXIT + +cat >"${STUB_DIR}/terraform" <<'EOF' +#!/usr/bin/env bash +printf '%s' "$TF_OUTPUT_JSON" +EOF + +cat >"${STUB_DIR}/aws" <<'EOF' +#!/usr/bin/env bash +echo "aws $*" >>"$AWS_CALLS" +case "$2" in + describe-db-instances) + [[ "${DESCRIBE_FAILS:-0}" == "1" ]] && { echo "describe failed" >&2; exit 255; } + printf '%s\n' "$LISTING" + ;; + modify-db-instance) + [[ "${MODIFY_FAILS:-0}" == "1" ]] && { echo "modify failed" >&2; exit 254; } + ;; +esac +exit 0 +EOF +chmod +x "${STUB_DIR}/terraform" "${STUB_DIR}/aws" + +export AWS_CALLS="$STUB_CALLS" + +# The real `--output text` shape: one tab-separated line. The neighbours are the +# ones the prefix filter used to match. +STUB_LISTING=$'cudly-dev\tcudly-dev-1a2b3c4d-postgres\tcudly-dev-1a2b3c4d-postgres-replica\tcudly-dev-prod-mirror\tcudly-dev-dba-scratch\tcudly-staging-9f8e7d6c-postgres' + +# run_script -> STUB_EXIT, STUB_ERR, and a truncated call log +run_script() { + : >"$STUB_CALLS" + local errfile + errfile="$(mktemp)" + STUB_EXIT=0 + PATH="${STUB_DIR}:${PATH}" "$UNPROTECT_SCRIPT" "$@" >/dev/null 2>"$errfile" || STUB_EXIT=$? + STUB_ERR="$(cat "$errfile")" + rm -f "$errfile" +} + +# assert_behaviour LABEL CONDITION_RESULT DETAIL +assert_behaviour() { + if [[ "$2" == "0" ]]; then + echo "PASS: $1" + ((pass++)) || true + else + echo "FAIL: $1" + echo " $3" + ((fail++)) || true + fi +} + +count_calls() { grep -c "$1" "$STUB_CALLS" 2>/dev/null || true; } + +export LISTING="$STUB_LISTING" + +# A state that is already destroyed is a normal outcome, not an error, and must +# not reach AWS at all. +export TF_OUTPUT_JSON='{}' +run_script "$STUB_STATE" +assert_behaviour "behaviour: a state with no outputs exits 0 without calling aws" \ + "$([[ "$STUB_EXIT" -eq 0 && ! -s "$STUB_CALLS" ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$STUB_CALLS")" + +# A state that predates the output. Distinct from the empty case below, because +# the remedies differ: this one wants an apply. +export TF_OUTPUT_JSON='{"ecr_repository_name":{"value":"cudly-dev-1a2b3c4d"}}' +run_script "$STUB_STATE" +assert_behaviour "behaviour: a state missing the output exits 1 without calling aws" \ + "$([[ "$STUB_EXIT" -eq 1 && ! -s "$STUB_CALLS" ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$STUB_CALLS")" +assert_behaviour "behaviour: the missing-output error tells the operator to re-apply" \ + "$([[ "$STUB_ERR" == *"absent or null"* && "$STUB_ERR" == *"re-apply this state"* ]] && echo 0 || echo 1)" \ + "stderr: ${STUB_ERR}" + +# `jq -er` accepts an empty string, so without its own check this reaches AWS. +export TF_OUTPUT_JSON='{"database_instance_identifier":{"value":""}}' +run_script "$STUB_STATE" +assert_behaviour "behaviour: an empty identifier exits 1 without calling aws" \ + "$([[ "$STUB_EXIT" -eq 1 && ! -s "$STUB_CALLS" ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$STUB_CALLS")" +assert_behaviour "behaviour: the empty-identifier error is distinct and says an apply will not fix it" \ + "$([[ "$STUB_ERR" == *"will NOT fix this"* && "$STUB_ERR" != *"re-apply this state"* ]] && echo 0 || echo 1)" \ + "stderr: ${STUB_ERR}" + +# A null value takes the jq branch, not the empty branch. +export TF_OUTPUT_JSON='{"database_instance_identifier":{"value":null}}' +run_script "$STUB_STATE" +assert_behaviour "behaviour: a null identifier exits 1 without calling aws" \ + "$([[ "$STUB_EXIT" -eq 1 && ! -s "$STUB_CALLS" ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$STUB_CALLS")" + +# The golden path, against the hostile listing. +export TF_OUTPUT_JSON='{"database_instance_identifier":{"value":"cudly-dev-1a2b3c4d-postgres"}}' +run_script "$STUB_STATE" +assert_behaviour "behaviour: the golden path exits 0" \ + "$([[ "$STUB_EXIT" -eq 0 ]] && echo 0 || echo 1)" "exit ${STUB_EXIT}" +assert_behaviour "behaviour: exactly one instance is unprotected out of the hostile listing" \ + "$([[ "$(count_calls 'modify-db-instance')" -eq 1 ]] && echo 0 || echo 1)" \ + "modify calls: $(count_calls 'modify-db-instance')" +assert_behaviour "behaviour: the unprotected instance is the one the state owns" \ + "$(grep -q 'modify-db-instance --db-instance-identifier cudly-dev-1a2b3c4d-postgres --no-deletion-protection --apply-immediately' "$STUB_CALLS" && echo 0 || echo 1)" \ + "calls: $(grep modify "$STUB_CALLS" || echo none)" + +# Asserted per neighbour so a failure names the database that would have lost +# its protection. +while IFS= read -r neighbour; do + [[ -n "$neighbour" ]] || continue + assert_behaviour "behaviour: neighbour ${neighbour} keeps its deletion protection" \ + "$(grep -q -- "--db-instance-identifier ${neighbour} " "$STUB_CALLS" && echo 1 || echo 0)" \ + "calls: $(grep modify "$STUB_CALLS" || echo none)" +done <<'EOF' +cudly-dev-1a2b3c4d-postgres-replica +cudly-dev-prod-mirror +cudly-dev-dba-scratch +cudly-staging-9f8e7d6c-postgres +EOF + +# Re-running a cleanup after a completed one is normal, not an error. +export LISTING=$'cudly-staging-9f8e7d6c-postgres\tcudly-prod-0badc0de-postgres' +run_script "$STUB_STATE" +assert_behaviour "behaviour: an instance already gone exits 0 and modifies nothing" \ + "$([[ "$STUB_EXIT" -eq 0 && "$(count_calls 'modify-db-instance')" -eq 0 ]] && echo 0 || echo 1)" \ + "exit ${STUB_EXIT}, calls: $(cat "$STUB_CALLS")" + +# The two halves of the swallowing bug, as behaviour rather than as text. A +# failed listing is what `2>/dev/null` made indistinguishable from an empty +# account; a failed modify is what `|| true` reported as success. +export LISTING="$STUB_LISTING" MODIFY_FAILS=1 +run_script "$STUB_STATE" +assert_behaviour "behaviour: a failed modify fails the step" \ + "$([[ "$STUB_EXIT" -ne 0 ]] && echo 0 || echo 1)" "exit ${STUB_EXIT}" +unset MODIFY_FAILS + +export DESCRIBE_FAILS=1 +run_script "$STUB_STATE" +assert_behaviour "behaviour: a failed listing fails the step" \ + "$([[ "$STUB_EXIT" -ne 0 ]] && echo 0 || echo 1)" "exit ${STUB_EXIT}" +unset DESCRIBE_FAILS + +run_script +assert_behaviour "behaviour: no argument exits 2" \ + "$([[ "$STUB_EXIT" -eq 2 ]] && echo 0 || echo 1)" "exit ${STUB_EXIT}" + +run_script "${STUB_STATE}/does-not-exist" +assert_behaviour "behaviour: a missing state directory exits 2" \ + "$([[ "$STUB_EXIT" -eq 2 ]] && echo 0 || echo 1)" "exit ${STUB_EXIT}" + # The assertions above name the files and steps they know about, so a NEW modify # site in a new step, workflow or script is invisible to them -- which is how # #1820 outlived #1592 and #1821 outlived both. This sweep is keyed on the @@ -510,7 +679,9 @@ assert_sweep "every 'aws rds modify-db-instance' site in .github/workflows and s # on ci.yml's comment describing this assertion would police what may be written # rather than what is run. FIXTURE_DIR="$(mktemp -d)" -trap 'rm -rf "$FIXTURE_DIR"' EXIT +# Replaces the stub trap set above rather than adding to it, so it has to clean +# up both sets. A second `trap ... EXIT` silently discards the first. +trap 'rm -rf "$FIXTURE_DIR" "$STUB_DIR" "$STUB_STATE" "$STUB_CALLS"' EXIT mkdir -p "${FIXTURE_DIR}/prose" "${FIXTURE_DIR}/wired" "${FIXTURE_DIR}/unwired" "${FIXTURE_DIR}/scripts" "${FIXTURE_DIR}/misattrib" cat >"${FIXTURE_DIR}/prose/mentions.yml" <<'EOF' From 5dc4d01cbaf536f21caf173bfc4e8ed5d37b3703 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 19 Aug 2026 01:35:52 +0200 Subject: [PATCH 3/3] fix(ci): sweep every script for unguarded destroy calls, not two named ones The sweep globbed .github/workflows but named only the two scripts already known to be guarded, so a NEW script running `aws rds modify-db-instance` without the selector passed the suite. That is the defect these suites exist to catch, one level up: the guard reached the sites someone remembered to list and not the sibling site nobody did, which is how #1592 became #1820 and then #1821. It also left ci.yml asserting that nothing else under scripts/ runs the command unguarded, which nothing actually checked. scripts/ and scripts/lib/ are now globbed. The two guard suites are excluded by basename, since each carries its dangerous command and the selector as fixture data and inside awk programs and would otherwise report itself as a violation. The swept set goes from 2 scripts to 23. Fixed on the ECR suite as well as the RDS one it was raised against. Fixing one and leaving the other is the same asymmetry, and the ECR sweep had the identical narrow list. An empty swept set has no violations, so two assertions stand in front of the sweep: the set is non-empty, and it contains the script that actually runs the command. `nullglob` keeps an unmatched pattern from expanding to its own literal text, which would otherwise become a nonexistent path that makes the sweep bail out early and report a clean result for files it never opened. The set builder is shared rather than copied into both suites, for the reason the selector is: two copies of it drift, and a sweep that quietly stops recognising sites fails open. Proven by six mutations, each against a copy and each required to produce its specific FAIL line: a new unguarded script under scripts/ is flagged and named, so is one under scripts/lib/, so is an unguarded ECR script; pointing the glob at a directory with no scripts fails the non-empty assertion; dropping nullglob fails the containment assertion, with the literal unexpanded pattern visible in the report; and widening the exclusion list to cover the guarded script fails containment and then reports finding no site at all. --- .github/workflows/ci.yml | 10 ++++ scripts/lib/code-scan-awk.sh | 38 ++++++++++++++ scripts/test-ecr-delete-selection.sh | 47 ++++++++++++++--- scripts/test-rds-deletion-protection-scope.sh | 50 +++++++++++++++++-- 4 files changed, 135 insertions(+), 10 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a8ece349a..78bf0d988 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -762,6 +762,10 @@ jobs: # destroy steps still call scripts/force-delete-owned-ecr-repo.sh, that script # still deletes only what the selector yields, and nothing else under # .github/workflows or scripts/ runs `aws ecr delete-repository` unguarded. + # That last claim is checked over a GLOB of both directories, not a list of + # known files, so a script added later is covered without anyone remembering + # to name it; the sweep is asserted to have opened a non-zero number of files + # first, since an empty swept set has no violations either. # Fast (shell only), so it always runs. ecr-delete-selection: name: ECR delete selection scope @@ -795,6 +799,12 @@ jobs: # scripts/disable-owned-rds-deletion-protection.sh, that script unprotects # only what the selector yields and swallows nothing, and nothing else under # .github/workflows or scripts/ runs `aws rds modify-db-instance` unguarded. + # That last claim is checked over a GLOB of both directories, not a list of + # known files, so a script added later is covered without anyone remembering + # to name it; the sweep is asserted to have opened a non-zero number of files + # first, since an empty swept set has no violations either. + # It additionally runs the script end to end against stubbed terraform and aws, + # so "a failed strip is not swallowed" is asserted as behaviour, not as text. # Fast (shell only), so it always runs. rds-deletion-protection-scope: name: RDS deletion protection scope diff --git a/scripts/lib/code-scan-awk.sh b/scripts/lib/code-scan-awk.sh index 2fa47568c..0b34e2a59 100644 --- a/scripts/lib/code-scan-awk.sh +++ b/scripts/lib/code-scan-awk.sh @@ -50,6 +50,44 @@ # `(^|[[:space:]])#` alternation, for the same reason: anchors inside a group # are not portable across awk implementations. +# 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. +# +# Globbed rather than named file by file, in both suites, because naming the two +# 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. +# +# `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. +# +# 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 + case "$(basename "$candidate")" in + test-rds-deletion-protection-scope.sh | test-ecr-delete-selection.sh) continue ;; + esac + SWEPT_SCRIPTS+=("$candidate") + done + shopt -u nullglob +} + # shellcheck disable=SC2034 # read by the suites that source this file AWK_CODE_FUNCS=' function code_of(line) { diff --git a/scripts/test-ecr-delete-selection.sh b/scripts/test-ecr-delete-selection.sh index 43afe3047..9e552ac7e 100755 --- a/scripts/test-ecr-delete-selection.sh +++ b/scripts/test-ecr-delete-selection.sh @@ -394,13 +394,19 @@ assert_script_wiring "$CLEANUP_SCRIPT" # argument only has to be a quoted variable here; the named assertions pin it to # "$OWNED_REPO". # -# The swept set is .github/workflows plus the two production scripts. Extracting +# The swept set is .github/workflows plus every script under scripts/. Extracting # the deletion body out of the workflow steps moved the only real delete call # into scripts/, so a sweep that still looked only at .github/workflows would # report a clean result for a directory that no longer contains the thing it is -# looking for. scripts/ is named file by file rather than globbed because this -# suite itself lives there and quotes both the command and the selector, in -# fixtures and in awk programs, as data. +# looking for. +# +# scripts/ is GLOBBED, not named file by file. Naming only the two scripts +# already known to be guarded left a NEW script running `aws ecr +# delete-repository` unswept, which is the "the guard did not reach the sibling +# site" mode this suite exists to catch, one level up. Raised by review on the +# RDS suite (#1821) and fixed on both, since fixing one and not the other is +# that same mode again. Rationale and the nullglob reasoning: +# build_swept_scripts in scripts/lib/code-scan-awk.sh. # # It also reports when it finds no delete site at all, because that is what a # wrong directory or an unmatched glob looks like, and an empty sweep would @@ -500,8 +506,37 @@ assert_sweep() { fi } -assert_sweep "every 'aws ecr delete-repository' site in .github/workflows and scripts/ pipes through the selector" \ - "$WORKFLOW_DIR" "" "$CLEANUP_SCRIPT" "$SELECT" +build_swept_scripts "$SCRIPT_DIR" + +# "Found no violations" must not be reachable by looking at nothing, so the +# swept set is asserted non-empty and asserted to contain the one script that +# actually runs the command. Guarding the expansion too: under `set -u`, bash +# 3.2 treats "${arr[@]}" on an empty array as an unbound variable. +if [[ ${#SWEPT_SCRIPTS[@]} -eq 0 ]]; then + echo "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" + ((fail++)) || true +else + echo "PASS: the swept set holds ${#SWEPT_SCRIPTS[@]} script(s) under scripts/" + ((pass++)) || true + + swept_has_guarded=0 + for swept_candidate in "${SWEPT_SCRIPTS[@]}"; do + [[ "$swept_candidate" == "$CLEANUP_SCRIPT" ]] && swept_has_guarded=1 + done + if [[ "$swept_has_guarded" -eq 1 ]]; then + echo "PASS: the swept set includes $(basename "$CLEANUP_SCRIPT"), the script that runs the command" + ((pass++)) || true + else + echo "FAIL: the swept set does not include $(basename "$CLEANUP_SCRIPT")" + echo " the sweep would then find no delete site at all and pass vacuously" + ((fail++)) || true + fi + + assert_sweep "every 'aws ecr delete-repository' site in .github/workflows and scripts/ pipes through the selector" \ + "$WORKFLOW_DIR" "" "${SWEPT_SCRIPTS[@]}" +fi # --- The sweep itself, in both directions, over fixtures --------------------- # diff --git a/scripts/test-rds-deletion-protection-scope.sh b/scripts/test-rds-deletion-protection-scope.sh index 6cfd04338..9b10de762 100755 --- a/scripts/test-rds-deletion-protection-scope.sh +++ b/scripts/test-rds-deletion-protection-scope.sh @@ -574,8 +574,18 @@ assert_behaviour "behaviour: a missing state directory exits 2" \ # wrong directory or an unmatched glob looks like, and an empty sweep would # otherwise read as a clean result for files it never opened. Both workflow # extensions GitHub accepts are swept, so a new `.yaml` file cannot slip past. -# scripts/ is named file by file rather than globbed because this suite itself -# lives there and quotes both the command and the selector, as data. +# +# scripts/ is GLOBBED, not named file by file. Naming the two known files was +# the same defect this PR fixes, one level up: a NEW script running `aws rds +# modify-db-instance` without the selector was invisible to a sweep that only +# ever opened the two scripts already known to be guarded, so the guard did not +# reach the sibling site. It also made ci.yml's claim that nothing else under +# scripts/ runs the command unguarded an assertion nobody was checking. +# +# The two guard suites are excluded by name because they carry both the command +# and the selector as fixture data and in awk programs; sweeping them would +# report this file as a violation of itself. That exclusion is by basename, so +# it cannot accidentally exempt a real script that merely sits near them. # sweep_unwired DIR [FILE...] # @@ -668,8 +678,40 @@ assert_sweep() { fi } -assert_sweep "every 'aws rds modify-db-instance' site in .github/workflows and scripts/ pipes through the selector" \ - "$WORKFLOW_DIR" "" "$UNPROTECT_SCRIPT" "$SELECT" +# The scripts/ half of the swept set is globbed, so a script added later is +# swept without anyone remembering to name it here. Rationale and the nullglob +# reasoning: build_swept_scripts in scripts/lib/code-scan-awk.sh. +build_swept_scripts "$SCRIPT_DIR" + +# "Found no violations" must not be reachable by looking at nothing, so the +# swept set is asserted non-empty and asserted to contain the one script that +# actually runs the command. Guarding the expansion too: under `set -u`, bash +# 3.2 treats "${arr[@]}" on an empty array as an unbound variable. +if [[ ${#SWEPT_SCRIPTS[@]} -eq 0 ]]; then + echo "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" + ((fail++)) || true +else + echo "PASS: the swept set holds ${#SWEPT_SCRIPTS[@]} script(s) under scripts/" + ((pass++)) || true + + swept_has_guarded=0 + for swept_candidate in "${SWEPT_SCRIPTS[@]}"; do + [[ "$swept_candidate" == "$UNPROTECT_SCRIPT" ]] && swept_has_guarded=1 + done + if [[ "$swept_has_guarded" -eq 1 ]]; then + echo "PASS: the swept set includes $(basename "$UNPROTECT_SCRIPT"), the script that runs the command" + ((pass++)) || true + else + echo "FAIL: the swept set does not include $(basename "$UNPROTECT_SCRIPT")" + echo " the sweep would then find no modify site at all and pass vacuously" + ((fail++)) || true + fi + + assert_sweep "every 'aws rds modify-db-instance' site in .github/workflows and scripts/ pipes through the selector" \ + "$WORKFLOW_DIR" "" "${SWEPT_SCRIPTS[@]}" +fi # --- The sweep itself, in both directions, over fixtures --------------------- #