sec(iac/gcp): scope cleanup-function SA to the one secret it reads

.github/workflows/ci.yml

@@ -499,6 +499,25 @@ jobs:
       - name: Run parity script self-tests
         run: bash scripts/test-aws-iam-parity.sh
 
+  # Assert that no Terraform file grants a Secret Manager role at project,
+  # folder or organization scope; those scopes hand the member every secret in
+  # the scope. Fast (shell only), so it always runs.
+  gcp-secret-scope:
+    name: GCP Secret Manager grant scope
+    runs-on: ubuntu-latest
+
+    steps:
+      - name: Checkout code
+        uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1
+        with:
+          persist-credentials: false
+
+      - name: Assert no scope-wide Secret Manager grants
+        run: bash scripts/check-gcp-secret-scope.sh
+
+      - name: Run guard script self-tests
+        run: bash scripts/test-gcp-secret-scope.sh
+
   # Summary job - all checks must pass
   ci-success:
     name: CI Success
@@ -513,6 +532,7 @@ jobs:
       - e2e-tests
       - azure-role-parity
       - aws-iam-parity
+      - gcp-secret-scope
     if: always()
 
     steps:

scripts/check-gcp-secret-scope.sh

@@ -0,0 +1,175 @@
+#!/usr/bin/env bash
+# check-gcp-secret-scope.sh
+#
+# Fails when any Terraform file grants a Secret Manager role at project, folder
+# or organization scope. Those scopes hand the member EVERY secret in the scope,
+# so a workload that needs one secret ends up able to read the credential
+# encryption key, the JWT/session secrets and the SendGrid API key alongside it.
+#
+# The supported pattern is a per-secret binding instead:
+#
+#   resource "google_secret_manager_secret_iam_member" "..." {
+#     project   = var.project_id
+#     secret_id = var.some_secret_id
+#     role      = "roles/secretmanager.secretAccessor"
+#     member    = "serviceAccount:${...}"
+#   }
+#
+# This guard exists because compute/gcp/cloud-run was migrated to per-secret
+# bindings while its sibling compute/gcp/cleanup-function was left on a
+# project-wide grant for as long as it took someone to notice (issue #1614).
+#
+# Exit 0 = no DIRECTLY EXPRESSED scope-wide Secret Manager grant found. Read the
+#          limitations below before treating this as "the tree is clean".
+# Exit 1 = at least one found; each is printed to stderr.
+# Exit 2 = usage error.
+#
+# LIMITATIONS. This is a textual guard, not a policy engine. It matches a literal
+# `roles/secretmanager.*` appearing inside a scope-wide IAM resource block, and
+# only that. Exit 0 means "nothing of that exact shape", NOT "no scope-wide grant
+# exists". Known blind spots, each tracked:
+#
+#   - Roles reaching the resource through `locals` + `for_each` (issue #1686).
+#     There is a live instance today:
+#     terraform/environments/gcp/ci-cd-permissions/service_account.tf grants
+#     project-scope `roles/secretmanager.admin` to the Terraform deploy service
+#     account through `local.deploy_roles`, and this guard exits 0 on it. That
+#     grant is believed legitimate (the deploy SA creates secrets); it is called
+#     out here so nobody reads a green run as proof of its absence.
+#   - `google_project_iam_policy` fed by a `data "google_iam_policy"` block, and
+#     resource bodies not indented the way `terraform fmt` produces (issue #1687).
+#   - The Terraform JSON encoding (`.tf.json`) is not parsed. Rather than report
+#     such a file as clean, the guard exits 2 if it finds one. None exist today.
+#   - Any role that is only knowable after variable resolution
+#     (`role = var.some_role`, string interpolation).
+#
+# It is a ratchet against the #1614 regression being reintroduced by copy-paste,
+# which is how it arrived the first time. It is not a proof of absence.
+#
+# Usage:
+#   scripts/check-gcp-secret-scope.sh              # scan the default roots
+#   scripts/check-gcp-secret-scope.sh FILE...      # scan exactly these files
+#
+# Passing explicit files is how the test harness points the check at fixtures
+# without touching the real sources.
+
+set -euo pipefail
+
+REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
+
+# Every directory holding Terraform this guard should cover. `iac/` carries the
+# customer-facing federation modules, which are exactly the kind of thing that
+# must not ship a scope-wide grant; scanning only `terraform/` missed all 20 of
+# its .tf files.
+SCAN_ROOTS=(
+  "${REPO_ROOT}/terraform"
+  "${REPO_ROOT}/iac"
+)
+
+# Resources that bind an IAM role across a whole project/folder/organization.
+# `_member` adds one principal, `_binding` replaces the whole principal list;
+# both are scope-wide and both are wrong for Secret Manager here.
+SCOPE_WIDE_RESOURCES='google_(project|folder|organization)_iam_(member|binding)'
+
+files=()
+if [[ $# -gt 0 ]]; then
+  for arg in "$@"; do
+    case "$arg" in
+      -*) echo "Unknown flag: $arg" >&2; exit 2 ;;
+    esac
+    if [[ ! -f "$arg" ]]; then
+      echo "ERROR: file not found: $arg" >&2
+      exit 2
+    fi
+    files+=("$arg")
+  done
+else
+  for root in "${SCAN_ROOTS[@]}"; do
+    if [[ ! -d "$root" ]]; then
+      echo "ERROR: scan root not found: $root" >&2
+      exit 2
+    fi
+    # NUL-delimited so paths with spaces survive. `.tf.json` is collected so it
+    # can be REJECTED below, not because it can be parsed: the scanner below
+    # matches HCL block syntax, and silently returning "clean" for a file it
+    # cannot read is the exact failure mode this guard exists to prevent.
+    while IFS= read -r -d '' f; do
+      files+=("$f")
+    done < <(find "$root" -type f \( -name '*.tf' -o -name '*.tf.json' \) -print0 | sort -z)
+  done
+fi
+
+# Fail closed on the JSON encoding rather than under-reporting it. None exist in
+# the repo today; if one is ever added, this stops rather than passing it over.
+json_files=()
+for f in "${files[@]}"; do
+  [[ "$f" == *.tf.json ]] && json_files+=("$f")
+done
+if [[ ${#json_files[@]} -gt 0 ]]; then
+  {
+    echo "ERROR: this guard cannot parse the Terraform JSON encoding, and will not"
+    echo "       report a file it cannot read as clean. Found:"
+    printf '         %s\n' "${json_files[@]}"
+    echo "       Extend the scanner to handle .tf.json (see issue #1687) before adding one."
+  } >&2
+  exit 2
+fi
+
+if [[ ${#files[@]} -eq 0 ]]; then
+  echo "ERROR: no Terraform files to scan" >&2
+  exit 2
+fi
+
+# Walk each resource block. A block opens on a `resource "<type>" "<name>" {`
+# line at column 0 and closes on a `}` at column 0, which `terraform fmt`
+# guarantees for top-level blocks. Report a block only when it is BOTH
+# scope-wide AND carries a roles/secretmanager.* role, so per-secret bindings
+# and non-secret project grants (cloudsql.client, logging, etc.) stay quiet.
+violations=$(
+  awk -v pattern="^resource[[:space:]]+\"${SCOPE_WIDE_RESOURCES}\"" '
+    $0 ~ pattern {
+      in_block = 1
+      header = $0
+      header_line = FNR
+      role = ""
+      next
+    }
+    in_block && /roles\/secretmanager\./ {
+      role = $0
+      sub(/^[[:space:]]+/, "", role)
+    }
+    in_block && /^}/ {
+      if (role != "") {
+        printf "%s:%d: %s\n      %s\n", FILENAME, header_line, header, role
+      }
+      in_block = 0
+      role = ""
+    }
+  ' "${files[@]}"
+)
+
+if [[ -n "$violations" ]]; then
+  {
+    echo "FAILED: scope-wide Secret Manager grant(s) found."
+    echo ""
+    echo "$violations"
+    echo ""
+    echo "A project/folder/organization-scoped Secret Manager role grants the member"
+    echo "every secret in that scope. Replace it with a per-secret binding:"
+    echo ""
+    echo "  resource \"google_secret_manager_secret_iam_member\" \"<name>\" {"
+    echo "    project   = var.project_id"
+    echo "    secret_id = var.<the_one_secret_this_workload_reads>"
+    echo "    role      = \"roles/secretmanager.secretAccessor\""
+    echo "    member    = \"serviceAccount:\${...}\""
+    echo "  }"
+    echo ""
+    echo "See terraform/modules/compute/gcp/cloud-run/main.tf for the reference shape."
+  } >&2
+  exit 1
+fi
+
+echo "OK: no directly expressed project/folder/organization-scoped Secret Manager" \
+     "role found in ${#files[@]} Terraform file(s). See the LIMITATIONS header:" \
+     "this does not cover roles reaching a resource via locals/for_each (#1686)" \
+     "or via a google_iam_policy data block (#1687)."

scripts/test-gcp-secret-scope.sh

@@ -0,0 +1,94 @@
+#!/usr/bin/env bash
+# test-gcp-secret-scope.sh
+#
+# Exercises check-gcp-secret-scope.sh against testdata fixtures, in BOTH
+# directions: a clean input must pass and a violating input must fail. A guard
+# that only ever returns "clean" is indistinguishable from a broken one, so the
+# failing cases are the ones that give this check its value.
+#
+# 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)"
+CHECK="${SCRIPT_DIR}/check-gcp-secret-scope.sh"
+FIXTURES="${SCRIPT_DIR}/testdata/gcp-secret-scope"
+
+pass=0
+fail=0
+
+run_case() {
+  local label="$1"
+  local expected_exit="$2"
+  shift 2
+
+  local actual_exit=0
+  "$CHECK" "$@" >/dev/null 2>&1 || actual_exit=$?
+
+  if [[ "$actual_exit" -eq "$expected_exit" ]]; then
+    echo "PASS: $label"
+    (( pass++ )) || true
+  else
+    echo "FAIL: $label (expected exit $expected_exit, got $actual_exit)"
+    (( fail++ )) || true
+  fi
+}
+
+# Negative direction: the supported shapes must not be reported. This is what
+# catches a guard so blunt it fires on every per-secret binding.
+run_case "per-secret binding and non-secret project grant exit 0" 0 \
+  "${FIXTURES}/clean.tf.fixture"
+
+# Positive direction: the anti-pattern must be caught.
+run_case "project-scope secretAccessor exits 1" 1 \
+  "${FIXTURES}/project-scope.tf.fixture"
+
+run_case "project-scope _binding variant exits 1" 1 \
+  "${FIXTURES}/project-binding.tf.fixture"
+
+# Folder and organization scope are asserted separately. They used to share one
+# fixture, which meant a guard that caught only one of them still passed.
+run_case "folder-scope _member variant exits 1" 1 \
+  "${FIXTURES}/folder-scope.tf.fixture"
+
+run_case "organization-scope _binding variant exits 1" 1 \
+  "${FIXTURES}/org-scope.tf.fixture"
+
+# A violation must still be found when mixed in with clean files.
+run_case "violation alongside a clean file exits 1" 1 \
+  "${FIXTURES}/clean.tf.fixture" "${FIXTURES}/project-scope.tf.fixture"
+
+# Usage errors are exit 2, distinct from "found a violation" (exit 1), so CI can
+# tell a broken invocation apart from a real finding.
+run_case "missing file exits 2" 2 \
+  "${FIXTURES}/does-not-exist.tf.fixture"
+
+run_case "unknown flag exits 2" 2 --bogus-flag
+
+# The JSON encoding is not parseable by this scanner, so it must be rejected
+# rather than reported clean. Exit 2 (cannot check) rather than 0 (checked and
+# clean) is the whole point.
+run_case "tf.json is rejected rather than silently passed" 2 \
+  "${FIXTURES}/json-encoding.tf.json"
+
+# The regression half of issue #1614: the module that carried the over-broad
+# grant must stay free of one. This is a claim the guard can actually check.
+run_case "cleanup-function module has no scope-wide Secret Manager grant" 0 \
+  "${REPO_ROOT}/terraform/modules/compute/gcp/cleanup-function/main.tf"
+
+# The default scan roots (terraform/ + iac/) must hold no violation of the shape
+# this guard detects.
+#
+# Deliberately NOT phrased as "the tree is clean". It is not:
+# terraform/environments/gcp/ci-cd-permissions/service_account.tf grants
+# project-scope roles/secretmanager.admin through `locals` + `for_each`, which
+# this guard structurally cannot see (issue #1686). That grant is believed
+# legitimate, but the distinction matters: this case asserts what the guard
+# verifies, not a broader property it never inspects. An earlier version of this
+# suite claimed the tree was clean, which was false as written.
+run_case "default scan roots hold no directly expressed scope-wide grant" 0
+
+echo ""
+echo "Results: ${pass} passed, ${fail} failed."
+[[ "$fail" -eq 0 ]]

scripts/testdata/gcp-secret-scope/clean.tf.fixture

@@ -0,0 +1,25 @@
+# Fixture: the supported shape. check-gcp-secret-scope.sh must exit 0 on this.
+#
+# Covers the two shapes that must NOT be reported:
+#   1. a per-secret Secret Manager binding (carries a roles/secretmanager.* role
+#      but is not scope-wide)
+#   2. a project-scope grant of a NON-Secret-Manager role (scope-wide but not
+#      Secret Manager)
+
+resource "google_service_account" "workload" {
+  project    = var.project_id
+  account_id = "workload-sa"
+}
+
+resource "google_secret_manager_secret_iam_member" "db_password_reader" {
+  project   = var.project_id
+  secret_id = var.db_password_secret_id
+  role      = "roles/secretmanager.secretAccessor"
+  member    = "serviceAccount:${google_service_account.workload.email}"
+}
+
+resource "google_project_iam_member" "cloud_sql_client" {
+  project = var.project_id
+  role    = "roles/cloudsql.client"
+  member  = "serviceAccount:${google_service_account.workload.email}"
+}

scripts/testdata/gcp-secret-scope/folder-scope.tf.fixture

@@ -0,0 +1,10 @@
+# Fixture: folder scope, `_member` variant. Must exit 1.
+#
+# Split out from the former bundled folder+organization fixture so each scope is
+# asserted on its own.
+
+resource "google_folder_iam_member" "folder_secrets" {
+  folder = var.folder_id
+  role   = "roles/secretmanager.secretAccessor"
+  member = "serviceAccount:${google_service_account.workload.email}"
+}

scripts/testdata/gcp-secret-scope/json-encoding.tf.json

@@ -0,0 +1,20 @@
+{
+  "_comment": [
+    "Fixture: the Terraform JSON encoding. Must exit 2 (cannot check), never 0.",
+    "This file DOES contain a project-scope Secret Manager grant. The HCL scanner",
+    "matches block syntax and would find nothing here, so without the explicit",
+    "reject it would report clean, which is the exact failure this guard exists",
+    "to prevent. Kept as .tf.json so it is discovered by the same find as a real",
+    "one would be; it lives outside terraform/ and iac/, so it is only reachable",
+    "when passed explicitly by this test."
+  ],
+  "resource": {
+    "google_project_iam_member": {
+      "project_secrets": {
+        "project": "${var.project_id}",
+        "role": "roles/secretmanager.admin",
+        "member": "serviceAccount:${google_service_account.workload.email}"
+      }
+    }
+  }
+}

scripts/testdata/gcp-secret-scope/org-scope.tf.fixture

@@ -0,0 +1,12 @@
+# Fixture: organization scope, `_binding` variant. Must exit 1.
+#
+# Organization scope is the broadest of the three, and `_binding` replaces the
+# whole principal list rather than adding one principal. Split from the former
+# bundled folder+organization fixture so each is asserted on its own: a guard
+# catching only one of them would still have passed the bundled case.
+
+resource "google_organization_iam_binding" "org_secrets" {
+  org_id  = var.org_id
+  role    = "roles/secretmanager.admin"
+  members = ["serviceAccount:${google_service_account.workload.email}"]
+}

scripts/testdata/gcp-secret-scope/project-binding.tf.fixture

@@ -0,0 +1,12 @@
+# Fixture: project scope, `_binding` variant. Must exit 1.
+#
+# The guard's resource pattern covers `_binding` as well as `_member`, but only
+# the `_member` form had a fixture, so this arm was working-but-untested.
+# `google_project_iam_binding` is authoritative for the role: it replaces the
+# entire principal list for that role at project scope.
+
+resource "google_project_iam_binding" "project_secrets" {
+  project = var.project_id
+  role    = "roles/secretmanager.secretAccessor"
+  members = ["serviceAccount:${google_service_account.workload.email}"]
+}