diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2c0b2cb21..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 @@ -781,7 +785,44 @@ 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. + # 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 + 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 +862,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..2ee1daf99 --- /dev/null +++ b/scripts/disable-owned-rds-deletion-protection.sh @@ -0,0 +1,148 @@ +#!/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. +# +# 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 +# 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 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 + +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}' 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 \ + | 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..0b34e2a59 --- /dev/null +++ b/scripts/lib/code-scan-awk.sh @@ -0,0 +1,107 @@ +#!/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. + +# 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) { + 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 88% rename from scripts/test-select-ecr-repos-to-delete.sh rename to scripts/test-ecr-delete-selection.sh index 0c80e7381..9e552ac7e 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" @@ -423,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 @@ -473,7 +450,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 +466,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[@]}" } @@ -529,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 --------------------- # @@ -566,7 +572,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 +587,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 +615,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..9b10de762 --- /dev/null +++ b/scripts/test-rds-deletion-protection-scope.sh @@ -0,0 +1,836 @@ +#!/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" + +# --- 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 +# 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 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...] +# +# 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 +} + +# 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 --------------------- +# +# 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)" +# 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' + - 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