From e9e1c6277aaa074bcf0841d527d7fb3733d7956b Mon Sep 17 00:00:00 2001
From: Astrid Gealer <astrid@gealer.email>
Date: Tue, 20 Oct 2026 14:45:48 +0100
Subject: [PATCH 1/2] sec(ci): refuse authoritative GCP IAM policies in the
 secret scope guard

check-gcp-secret-scope.sh finds Secret Manager roles by their text, but an
authoritative google_{project,folder,organization}_iam_policy can take its
policy_data from file(), a variable with no in-tree default or a module
output, so the role it grants never appears in a scanned file and the guard
reported such configurations clean. Fail closed instead: any non-comment
mention of those types, or of data "google_iam_policy", now exits 2, as the
guard already does for the JSON encoding.

CI's terraform fmt check now covers iac/ as well as terraform/, so every
root the guard scans is held to the layout its block tracking was written
for. The self-test reads SCAN_ROOTS and fails if a root has no fmt step.

Closes #150

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 .github/workflows/ci.yml                      |  7 +-
 scripts/check-gcp-secret-scope.sh             | 61 ++++++++++++++-
 scripts/test-gcp-secret-scope.sh              | 74 ++++++++++++++++++-
 .../dynamic-iam-policy.tf.fixture             |  3 +-
 .../folder-iam-policy.tf.fixture              | 11 +++
 .../iam-policy-data-output.tf.fixture         | 16 ++++
 .../iam-policy-data.tf.fixture                |  3 +-
 .../iam-policy-lookalikes.tf.fixture          | 28 +++++++
 .../iam-policy-unindented.tf.fixture          |  7 ++
 .../iam-policy-unquoted-labels.tf.fixture     |  8 ++
 .../org-iam-policy.tf.fixture                 | 13 ++++
 .../per-secret-unindented-body.tf.fixture     | 20 +++++
 .../policy-heredoc.tf.fixture                 |  4 +-
 .../project-iam-policy-file.tf.fixture        |  9 +++
 .../unindented-body.tf.fixture                | 14 ++++
 15 files changed, 270 insertions(+), 8 deletions(-)
 create mode 100644 scripts/testdata/gcp-secret-scope/folder-iam-policy.tf.fixture
 create mode 100644 scripts/testdata/gcp-secret-scope/iam-policy-data-output.tf.fixture
 create mode 100644 scripts/testdata/gcp-secret-scope/iam-policy-lookalikes.tf.fixture
 create mode 100644 scripts/testdata/gcp-secret-scope/iam-policy-unindented.tf.fixture
 create mode 100644 scripts/testdata/gcp-secret-scope/iam-policy-unquoted-labels.tf.fixture
 create mode 100644 scripts/testdata/gcp-secret-scope/org-iam-policy.tf.fixture
 create mode 100644 scripts/testdata/gcp-secret-scope/per-secret-unindented-body.tf.fixture
 create mode 100644 scripts/testdata/gcp-secret-scope/project-iam-policy-file.tf.fixture
 create mode 100644 scripts/testdata/gcp-secret-scope/unindented-body.tf.fixture

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 9b1e8106e..194d359c5 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -593,7 +593,12 @@ jobs:
           terraform_version: 1.10.5
 
       - name: Terraform Format Check
-        run: terraform fmt -check -recursive terraform/
+        # Must cover every SCAN_ROOTS entry in scripts/check-gcp-secret-scope.sh,
+        # whose block tracking was written against fmt layout (#150). The
+        # guard's self-test fails if a scan root has no line here.
+        run: |
+          terraform fmt -check -recursive terraform/
+          terraform fmt -check -recursive iac/
 
       - name: Test Azure federation identity guards
         if: matrix.cloud == 'azure'
diff --git a/scripts/check-gcp-secret-scope.sh b/scripts/check-gcp-secret-scope.sh
index 3b25f4bc5..ece200f7d 100755
--- a/scripts/check-gcp-secret-scope.sh
+++ b/scripts/check-gcp-secret-scope.sh
@@ -47,6 +47,17 @@
 # which role it becomes. Refusing it is the fail-closed answer; the fix is to
 # write the role name out.
 #
+# The one shape that breaks the premise above is refused outright (issue #150):
+# an authoritative policy (`google_project_iam_policy`, `google_folder_iam_policy`,
+# `google_organization_iam_policy`) and the `data "google_iam_policy"` source
+# that builds policy_data for one. policy_data can be read with file() from a
+# JSON file the guard does not scan, or handed in through a variable or module
+# output, so the role it grants need never appear as text here. It is also the
+# most dangerous shape: it REPLACES the scope's whole IAM policy. Any
+# non-comment mention of one of those type names exits 2 before the scan runs,
+# and no waiver applies. Correlating policy_data with its source is the work to
+# do if one is ever genuinely needed.
+#
 # WAIVERS. A deliberate scope-wide Secret Manager grant is permitted by a comment
 # on the line DIRECTLY above the mention it excuses, with a reason:
 #
@@ -61,7 +72,8 @@
 #
 # Exit 0 = no un-waived Secret Manager mention and no plan-time role name found.
 # Exit 1 = at least one found; each is printed to stderr.
-# Exit 2 = usage error, or a file this guard cannot read (JSON encoding).
+# Exit 2 = usage error, a file this guard cannot read (JSON encoding), or an
+#          authoritative IAM policy it cannot analyze (see above).
 #
 # LIMITATIONS. This is a textual guard, not a policy engine.
 #   - Values supplied from OUTSIDE the scanned tree are out of reach: `-var` on
@@ -75,6 +87,17 @@
 #   - The Terraform JSON encoding (`.tf.json`, `.tfvars.json`) is not parsed.
 #     Rather than report such a file as clean, the guard exits 2 if it finds one.
 #     None exist today.
+#   - The authoritative-policy shape (`google_{project,folder,organization}
+#     _iam_policy`, `data "google_iam_policy"`) is REJECTED with exit 2, not
+#     analyzed (issue #150). That includes a `google_iam_policy` data source
+#     feeding a per-secret `_iam_policy`, since the guard cannot tell where
+#     policy_data goes. None exist today.
+#   - Block tracking (which decides the per-secret exemption) counts braces and
+#     does not need `terraform fmt` layout to stay closed, but CI also runs
+#     `terraform fmt -check` over every SCAN_ROOTS entry, so unformatted code
+#     cannot land in the first place.
+#   - Resources inside modules sourced from outside the tree (registry, git)
+#     are not scanned.
 #
 # Usage:
 #   scripts/check-gcp-secret-scope.sh              # scan the default roots
@@ -148,6 +171,42 @@ if [[ ${#files[@]} -eq 0 ]]; then
   exit 2
 fi
 
+# Fail closed on the authoritative-policy shape (issue #150; see the header).
+# Matches the type name as a token anywhere in a non-comment line, so a quoted
+# or bare block label, any indentation, and references (`data.google_iam_policy.
+# x`, `moved`, `import`) are all caught. Heredoc bodies and descriptions are
+# not exempt here: over-reporting a name that should not be in the tree at all
+# is the safe direction.
+unsupported=$(
+  awk '
+    /^[[:space:]]*(#|\/\/)/ { next }
+    /(^|[^A-Za-z0-9_])google_(project|folder|organization)_iam_policy([^A-Za-z0-9_]|$)/ ||
+    /(^|[^A-Za-z0-9_])google_iam_policy([^A-Za-z0-9_]|$)/ {
+      t = $0
+      sub(/^[[:space:]]+/, "", t)
+      printf "%s:%d: %s\n", FILENAME, FNR, t
+    }
+  ' "${files[@]}"
+)
+if [[ -n "$unsupported" ]]; then
+  {
+    echo "ERROR: this guard cannot analyze an authoritative IAM policy, and will not"
+    echo "       report one as clean (issue #150). Found:"
+    while IFS= read -r finding; do
+      printf '         %s\n' "$finding"
+    done <<<"$unsupported"
+    echo ""
+    echo "       google_{project,folder,organization}_iam_policy replaces the scope's"
+    echo "       whole IAM policy with policy_data, which may come from file(), a"
+    echo "       variable or a module output the guard cannot follow, so it cannot say"
+    echo "       which roles are granted. Use google_{project,folder,organization}"
+    echo "       _iam_member for non-secret roles and google_secret_manager_secret"
+    echo "       _iam_member for secrets, or extend the guard to correlate policy_data"
+    echo "       with its source before adding one."
+  } >&2
+  exit 2
+fi
+
 # Line scanner. Portable awk only (CI runs mawk, macOS runs BWK awk): no gawk
 # extensions, no regex intervals, braces matched via bracket expressions.
 #
diff --git a/scripts/test-gcp-secret-scope.sh b/scripts/test-gcp-secret-scope.sh
index 62bfe7ab1..8790f12b1 100755
--- a/scripts/test-gcp-secret-scope.sh
+++ b/scripts/test-gcp-secret-scope.sh
@@ -90,13 +90,15 @@ run_case "role passed as a module argument exits 1" 1 \
 run_case "role supplied by a tfvars file exits 1" 1 \
   "${FIXTURES}/roles.tfvars.fixture"
 
-run_case "dynamic binding in google_iam_policy over a local exits 1" 1 \
+# These three carried the authoritative-policy shape and exited 1 through the
+# role text alone. Since issue #150 that shape is refused outright (exit 2).
+run_case "dynamic binding in google_iam_policy over a local exits 2 (#150)" 2 \
   "${FIXTURES}/dynamic-iam-policy.tf.fixture"
 
-run_case "literal role in google_iam_policy data block exits 1" 1 \
+run_case "literal role in google_iam_policy data block exits 2 (#150)" 2 \
   "${FIXTURES}/iam-policy-data.tf.fixture"
 
-run_case "JSON heredoc policy_data on google_project_iam_policy exits 1" 1 \
+run_case "JSON heredoc policy_data on google_project_iam_policy exits 2 (#150)" 2 \
   "${FIXTURES}/policy-heredoc.tf.fixture"
 
 run_case "custom role carrying secretmanager.* permissions exits 1" 1 \
@@ -105,6 +107,42 @@ run_case "custom role carrying secretmanager.* permissions exits 1" 1 \
 run_case "block not laid out the way terraform fmt does exits 1" 1 \
   "${FIXTURES}/unformatted-block.tf.fixture"
 
+# Issue #150, part 1: the authoritative-policy shape is refused (exit 2) rather
+# than analyzed. Each of the first six exited 0 before #150, because the roles
+# it grants never appear as `secretmanager` text in a scanned file.
+run_case "google_project_iam_policy fed by file() exits 2 (#150)" 2 \
+  "${FIXTURES}/project-iam-policy-file.tf.fixture"
+
+run_case "google_folder_iam_policy fed by a variable exits 2 (#150)" 2 \
+  "${FIXTURES}/folder-iam-policy.tf.fixture"
+
+run_case "google_organization_iam_policy fed by jsonencode exits 2 (#150)" 2 \
+  "${FIXTURES}/org-iam-policy.tf.fixture"
+
+run_case "google_iam_policy data source exported as an output exits 2 (#150)" 2 \
+  "${FIXTURES}/iam-policy-data-output.tf.fixture"
+
+run_case "authoritative policy with bare block labels exits 2 (#150)" 2 \
+  "${FIXTURES}/iam-policy-unquoted-labels.tf.fixture"
+
+run_case "authoritative policy with a column-0 body exits 2 (#150)" 2 \
+  "${FIXTURES}/iam-policy-unindented.tf.fixture"
+
+run_case "rejected shape alongside a clean file still exits 2 (#150)" 2 \
+  "${FIXTURES}/clean.tf.fixture" "${FIXTURES}/project-iam-policy-file.tf.fixture"
+
+# Negative direction: lookalike type names and comments are not refused.
+run_case "lookalike IAM policy types and comments exit 0 (#150)" 0 \
+  "${FIXTURES}/iam-policy-lookalikes.tf.fixture"
+
+# Issue #150, part 2: a body laid out in column 0 must not end a block early
+# enough to drop a role, nor carry the per-secret exemption onward.
+run_case "scope-wide grant with a column-0 body exits 1 (#150)" 1 \
+  "${FIXTURES}/unindented-body.tf.fixture"
+
+run_case "column-0 per-secret body does not leak its exemption (#150)" 1 \
+  "${FIXTURES}/per-secret-unindented-body.tf.fixture"
+
 # Fail closed on role names the scanner cannot resolve.
 run_case "role assembled by interpolation exits 1" 1 \
   "${FIXTURES}/interpolated-role.tf.fixture"
@@ -170,6 +208,36 @@ fi
 run_case "ci-cd-permissions deploy SA grant fails without its waiver" 1 \
   "${WORK}/service_account.tf"
 
+# policy-heredoc now exits 2 on its resource type alone, which would hide a
+# regression in how heredoc bodies are scanned. Retyped to a per-secret
+# _iam_policy (not refused), the role inside the JSON heredoc must still be
+# reported: only a plain `role = "roles/secretmanager.X"` line is exempt.
+sed 's/google_project_iam_policy/google_secret_manager_secret_iam_policy/g' \
+  "${FIXTURES}/policy-heredoc.tf.fixture" > "${WORK}/per-secret-heredoc.tf"
+run_case "JSON heredoc role on a per-secret _iam_policy exits 1" 1 \
+  "${WORK}/per-secret-heredoc.tf"
+
+# Issue #150, part 2: the guard's block tracking was written against
+# `terraform fmt` layout, so CI must run `terraform fmt -check -recursive` over
+# every root the guard scans. Read the roots from the guard itself so adding one without a fmt gate
+# fails here.
+CI_YML="${REPO_ROOT}/.github/workflows/ci.yml"
+# shellcheck disable=SC2016  # ${REPO_ROOT} is matched literally, not expanded.
+roots=$(sed -n '/^SCAN_ROOTS=(/,/^)/s|^ *"\${REPO_ROOT}/\([^"]*\)"$|\1|p' "$CHECK")
+if [[ -z "$roots" ]]; then
+  echo "FAIL: could not read SCAN_ROOTS from $CHECK"
+  (( fail++ )) || true
+fi
+for root in $roots; do
+  if grep -Eq "^[[:space:]]*(run:[[:space:]]*)?terraform fmt -check -recursive ${root}/?[[:space:]]*$" "$CI_YML"; then
+    echo "PASS: CI runs terraform fmt -check over scan root ${root}/"
+    (( pass++ )) || true
+  else
+    echo "FAIL: CI has no 'terraform fmt -check -recursive ${root}/' step for scan root ${root}/"
+    (( fail++ )) || true
+  fi
+done
+
 # Default-root discovery: run a copy of the guard from a throwaway tree laid out
 # like the repo, so the cases below exercise which files it COLLECTS, not only
 # how it scans files it is handed.
diff --git a/scripts/testdata/gcp-secret-scope/dynamic-iam-policy.tf.fixture b/scripts/testdata/gcp-secret-scope/dynamic-iam-policy.tf.fixture
index 314bbfe8f..59d69fed8 100644
--- a/scripts/testdata/gcp-secret-scope/dynamic-iam-policy.tf.fixture
+++ b/scripts/testdata/gcp-secret-scope/dynamic-iam-policy.tf.fixture
@@ -1,6 +1,7 @@
 # Fixture: an authoritative project policy built from a `google_iam_policy` data
 # source whose bindings come from a `dynamic` block over a local list. Neither
-# the data block nor the policy resource carries the literal. Must exit 1.
+# the data block nor the policy resource carries the literal. Since issue #150
+# the guard refuses the authoritative-policy shape outright. Must exit 2.
 
 locals {
   policy_roles = ["roles/viewer", "roles/secretmanager.secretAccessor"]
diff --git a/scripts/testdata/gcp-secret-scope/folder-iam-policy.tf.fixture b/scripts/testdata/gcp-secret-scope/folder-iam-policy.tf.fixture
new file mode 100644
index 000000000..c48b8f50f
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/folder-iam-policy.tf.fixture
@@ -0,0 +1,11 @@
+# Fixture (issue #150): an authoritative folder policy whose policy_data comes
+# from a variable with no in-tree default. Must exit 2.
+
+variable "folder_policy_data" {
+  type = string
+}
+
+resource "google_folder_iam_policy" "folder" {
+  folder      = var.folder_id
+  policy_data = var.folder_policy_data
+}
diff --git a/scripts/testdata/gcp-secret-scope/iam-policy-data-output.tf.fixture b/scripts/testdata/gcp-secret-scope/iam-policy-data-output.tf.fixture
new file mode 100644
index 000000000..27b8cefca
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/iam-policy-data-output.tf.fixture
@@ -0,0 +1,16 @@
+# Fixture (issue #150): a `google_iam_policy` data source on its own, exported as
+# a module output. Its policy_data can be applied by a scope-wide policy
+# resource in whichever configuration consumes the output, which the guard
+# cannot follow. The role comes from a variable with no in-tree default.
+# Must exit 2.
+
+data "google_iam_policy" "workload" {
+  binding {
+    role    = var.workload_role
+    members = ["serviceAccount:workload@example.iam.gserviceaccount.com"]
+  }
+}
+
+output "workload_policy_data" {
+  value = data.google_iam_policy.workload.policy_data
+}
diff --git a/scripts/testdata/gcp-secret-scope/iam-policy-data.tf.fixture b/scripts/testdata/gcp-secret-scope/iam-policy-data.tf.fixture
index 3d09906cf..29eede8cb 100644
--- a/scripts/testdata/gcp-secret-scope/iam-policy-data.tf.fixture
+++ b/scripts/testdata/gcp-secret-scope/iam-policy-data.tf.fixture
@@ -1,6 +1,7 @@
 # Fixture: a literal role inside a `google_iam_policy` data block feeding
 # google_project_iam_policy. The data block is not a scope-wide IAM resource, so
-# a scanner keyed on resource type misses it. Must exit 1.
+# a scanner keyed on resource type misses it. Since issue #150 the guard refuses
+# the authoritative-policy shape outright rather than analyzing it. Must exit 2.
 
 data "google_iam_policy" "project" {
   binding {
diff --git a/scripts/testdata/gcp-secret-scope/iam-policy-lookalikes.tf.fixture b/scripts/testdata/gcp-secret-scope/iam-policy-lookalikes.tf.fixture
new file mode 100644
index 000000000..cb1d01eb0
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/iam-policy-lookalikes.tf.fixture
@@ -0,0 +1,28 @@
+# Fixture (issue #150, negative direction): types whose names share words with
+# the rejected authoritative-policy types, and a comment naming one, must not
+# be rejected. Only google_{project,folder,organization}_iam_policy and
+# google_iam_policy are. Must exit 0.
+
+# A comment may say google_project_iam_policy or data "google_iam_policy".
+// So may a // comment: google_organization_iam_policy.
+
+resource "google_secret_manager_secret_iam_policy" "secret" {
+  project     = var.project_id
+  secret_id   = var.secret_id
+  policy_data = var.secret_policy_data
+}
+
+resource "google_storage_bucket_iam_policy" "bucket" {
+  bucket      = var.bucket_name
+  policy_data = var.bucket_policy_data
+}
+
+resource "google_project_iam_member" "viewer" {
+  project = var.project_id
+  role    = "roles/viewer"
+  member  = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+locals {
+  my_google_iam_policy_name = "not-a-data-source"
+}
diff --git a/scripts/testdata/gcp-secret-scope/iam-policy-unindented.tf.fixture b/scripts/testdata/gcp-secret-scope/iam-policy-unindented.tf.fixture
new file mode 100644
index 000000000..b33294e99
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/iam-policy-unindented.tf.fixture
@@ -0,0 +1,7 @@
+# Fixture (issue #150): an authoritative project policy whose body starts in
+# column 0, the layout `terraform fmt` would rewrite. Must exit 2.
+
+resource "google_project_iam_policy" "project" {
+project = var.project_id
+policy_data = var.project_policy_data
+}
diff --git a/scripts/testdata/gcp-secret-scope/iam-policy-unquoted-labels.tf.fixture b/scripts/testdata/gcp-secret-scope/iam-policy-unquoted-labels.tf.fixture
new file mode 100644
index 000000000..b39efb926
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/iam-policy-unquoted-labels.tf.fixture
@@ -0,0 +1,8 @@
+# Fixture (issue #150): HCL accepts bare identifiers as block labels, so the
+# resource type need not be quoted. Matching only `resource "google_..._policy"`
+# would miss this. Must exit 2.
+
+resource google_project_iam_policy project {
+  project     = var.project_id
+  policy_data = var.project_policy_data
+}
diff --git a/scripts/testdata/gcp-secret-scope/org-iam-policy.tf.fixture b/scripts/testdata/gcp-secret-scope/org-iam-policy.tf.fixture
new file mode 100644
index 000000000..8b4faedb2
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/org-iam-policy.tf.fixture
@@ -0,0 +1,13 @@
+# Fixture (issue #150): an authoritative organization policy built with
+# jsonencode() from a role supplied by a variable with no in-tree default.
+# Must exit 2.
+
+resource "google_organization_iam_policy" "org" {
+  org_id = var.org_id
+  policy_data = jsonencode({
+    bindings = [{
+      role    = var.org_role
+      members = ["serviceAccount:workload@example.iam.gserviceaccount.com"]
+    }]
+  })
+}
diff --git a/scripts/testdata/gcp-secret-scope/per-secret-unindented-body.tf.fixture b/scripts/testdata/gcp-secret-scope/per-secret-unindented-body.tf.fixture
new file mode 100644
index 000000000..b5cd8c103
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/per-secret-unindented-body.tf.fixture
@@ -0,0 +1,20 @@
+# Fixture (issue #150): a per-secret binding whose body starts in column 0, with
+# a column-0 `}` closing a nested block before a second resource. The
+# per-secret exemption must not reach the project grant that follows, however
+# the closing braces are laid out. Must exit 1.
+
+resource "google_secret_manager_secret_iam_member" "unindented" {
+project   = var.project_id
+secret_id = var.secret_id
+condition {
+title      = "always"
+expression = "true"
+}
+role   = "roles/secretmanager.secretAccessor"
+member = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+resource "google_project_iam_member" "after" {
+project = var.project_id
+role    = "roles/secretmanager.secretAccessor"
+member  = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
diff --git a/scripts/testdata/gcp-secret-scope/policy-heredoc.tf.fixture b/scripts/testdata/gcp-secret-scope/policy-heredoc.tf.fixture
index 901bbde48..4f6b869ea 100644
--- a/scripts/testdata/gcp-secret-scope/policy-heredoc.tf.fixture
+++ b/scripts/testdata/gcp-secret-scope/policy-heredoc.tf.fixture
@@ -1,6 +1,8 @@
 # Fixture: an authoritative project policy given as a JSON heredoc. The role is
 # heredoc text, and the JSON's own column-0 braces must not end the scan of it.
-# Must exit 1.
+# Since issue #150 the guard refuses google_project_iam_policy outright, so this
+# file exits 2; test-gcp-secret-scope.sh also runs a copy retyped to a per-secret
+# _iam_policy, which keeps the heredoc scan itself covered (exit 1).
 
 resource "google_project_iam_policy" "project" {
   project     = var.project_id
diff --git a/scripts/testdata/gcp-secret-scope/project-iam-policy-file.tf.fixture b/scripts/testdata/gcp-secret-scope/project-iam-policy-file.tf.fixture
new file mode 100644
index 000000000..66f774a5b
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/project-iam-policy-file.tf.fixture
@@ -0,0 +1,9 @@
+# Fixture (issue #150): an authoritative project policy whose policy_data is read
+# from a JSON file. The roles live in the .json file, which the guard does not
+# scan, so no `secretmanager` text appears anywhere it looks. The guard cannot
+# analyze this shape and must refuse it rather than report it clean. Must exit 2.
+
+resource "google_project_iam_policy" "project" {
+  project     = var.project_id
+  policy_data = file("${path.module}/project-policy.json")
+}
diff --git a/scripts/testdata/gcp-secret-scope/unindented-body.tf.fixture b/scripts/testdata/gcp-secret-scope/unindented-body.tf.fixture
new file mode 100644
index 000000000..ca5e85855
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/unindented-body.tf.fixture
@@ -0,0 +1,14 @@
+# Fixture (issue #150): a scope-wide grant whose body starts in column 0. The
+# nested condition block's closing brace is in column 0 too, so a scanner that
+# treats `^}` as the end of the resource stops attributing lines to it before
+# the role. The role must still be reported. Must exit 1.
+
+resource "google_project_iam_member" "unindented" {
+project = var.project_id
+condition {
+title      = "always"
+expression = "true"
+}
+role   = "roles/secretmanager.secretAccessor"
+member = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}

From 5edd45650fec283ea8ee3b9475b5e31263d1b84a Mon Sep 17 00:00:00 2001
From: Astrid Gealer <astrid@gealer.email>
Date: Tue, 20 Oct 2026 15:09:45 +0100
Subject: [PATCH 2/2] sec(ci): allowlist the roles a per-secret Secret Manager
 grant may carry

check-gcp-secret-scope.sh exempted a per-secret binding's `role =` line for
any roles/secretmanager.* name, so widening a per-secret grant to
roles/secretmanager.admin (the #504 review's mutation) still exited 0. Inside
a per-secret IAM resource every `role =` line must now name
roles/secretmanager.secretAccessor or roles/secretmanager.secretVersionAdder
as a literal, matched case-sensitively. Any other role there, including one
from another service such as roles/owner or a reference the guard cannot
resolve, is a finding unless waived with a reason. Those two are the only
roles the repo grants per secret and the only ones the issue names.

Inline policy_data role lines on a per-secret _iam_policy get the same check.
One built from data "google_iam_policy" already exits 2 after #150; one fed
from file() or a variable remains out of reach and is listed under
LIMITATIONS.

Closes #505

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---
 scripts/check-gcp-secret-scope.sh             |  48 +++++--
 scripts/test-gcp-secret-scope.sh              |  58 +++++++++
 .../per-secret-admin.tf.fixture               |  11 ++
 .../per-secret-allowed-roles.tf.fixture       |  63 +++++++++
 .../per-secret-role-lookalikes.tf.fixture     | 123 ++++++++++++++++++
 .../per-secret-role-waived.tf.fixture         |  12 ++
 6 files changed, 305 insertions(+), 10 deletions(-)
 create mode 100644 scripts/testdata/gcp-secret-scope/per-secret-admin.tf.fixture
 create mode 100644 scripts/testdata/gcp-secret-scope/per-secret-allowed-roles.tf.fixture
 create mode 100644 scripts/testdata/gcp-secret-scope/per-secret-role-lookalikes.tf.fixture
 create mode 100644 scripts/testdata/gcp-secret-scope/per-secret-role-waived.tf.fixture

diff --git a/scripts/check-gcp-secret-scope.sh b/scripts/check-gcp-secret-scope.sh
index ece200f7d..ac5313411 100755
--- a/scripts/check-gcp-secret-scope.sh
+++ b/scripts/check-gcp-secret-scope.sh
@@ -5,6 +5,8 @@
 # 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.
+# It also fails when a per-secret grant carries a role wider than a workload
+# needs on one secret (issue #505): see shape 1 below.
 #
 # The supported pattern is a per-secret binding instead:
 #
@@ -32,7 +34,17 @@
 #   1. Inside a per-secret IAM resource
 #      (google_secret_manager_secret_iam_{member,binding,policy} and the
 #      _regional_ variants), and only as a plain `role = "roles/secretmanager.X"`
-#      attribute line. That is the supported pattern above.
+#      attribute line where X is secretAccessor (read a version) or
+#      secretVersionAdder (add one). That is the supported pattern above.
+#      Those are the two roles a workload needs on a single secret and the
+#      only two this repo grants. Any other `role =` line in such a resource
+#      is a finding (issue #505): a wider Secret Manager role such as admin or
+#      secretVersionManager (destroy versions, delete the secret, rewrite its
+#      IAM policy), a role from another service such as roles/owner, a
+#      reference whose value the guard cannot see, or a name that differs from
+#      an allowed one in case or suffix. Inside `_policy`, this covers role
+#      lines written inline in policy_data (e.g. jsonencode); see LIMITATIONS
+#      for policy_data from elsewhere.
 #   2. A comment line (`#` or `//` as the first non-blank characters). A trailing
 #      comment on a code line is NOT exempt; that is deliberate, so a `#` inside
 #      a string can never hide the role written after it.
@@ -58,8 +70,9 @@
 # and no waiver applies. Correlating policy_data with its source is the work to
 # do if one is ever genuinely needed.
 #
-# WAIVERS. A deliberate scope-wide Secret Manager grant is permitted by a comment
-# on the line DIRECTLY above the mention it excuses, with a reason:
+# WAIVERS. A deliberate scope-wide Secret Manager grant, or a per-secret grant
+# of a role outside the allowed two, is permitted by a comment on the line
+# DIRECTLY above the mention it excuses, with a reason:
 #
 #   # gcp-secret-scope: allow - <why this grant must be scope-wide>
 #   "roles/secretmanager.admin",
@@ -92,6 +105,11 @@
 #     analyzed (issue #150). That includes a `google_iam_policy` data source
 #     feeding a per-secret `_iam_policy`, since the guard cannot tell where
 #     policy_data goes. None exist today.
+#   - A per-secret `_iam_policy` whose policy_data is NOT written inline, e.g.
+#     from file(), a variable or a module output, is not analyzed: its roles
+#     never appear as `role =` lines here. One built from `data
+#     "google_iam_policy"` exits 2 (above). None exist today; the allowlist in
+#     shape 1 applies only to role lines the guard can see.
 #   - Block tracking (which decides the per-secret exemption) counts braces and
 #     does not need `terraform fmt` layout to stay closed, but CI also runs
 #     `terraform fmt -check` over every SCAN_ROOTS entry, so unformatted code
@@ -243,10 +261,17 @@ violations=$(
     function taint(s,    t) {
       t = tolower(s)
       gsub(/secretmanager\.googleapis\.com/, "", t)
-      if (t ~ /secretmanager/) {
-        if (allowed && s ~ /^[[:space:]]*role[[:space:]]*=[[:space:]]*"roles\/secretmanager\.[A-Za-z]+"[[:space:]]*$/) {
+      # Inside a per-secret IAM resource every `role =` line must name one of
+      # the two allowed roles as a literal (issue #505). Matched case-sensitively
+      # because IAM role names are. Anything else there, a wider Secret Manager
+      # role, a role from another service or a reference, is a finding.
+      if (allowed && s ~ /^[[:space:]]*role[[:space:]]*=/) {
+        if (s ~ /^[[:space:]]*role[[:space:]]*=[[:space:]]*"roles\/secretmanager\.(secretAccessor|secretVersionAdder)"[[:space:]]*$/) {
           return ""
         }
+        return "per-secret IAM role is not roles/secretmanager.secretAccessor or roles/secretmanager.secretVersionAdder"
+      }
+      if (t ~ /secretmanager/) {
         return "Secret Manager role or permission outside a per-secret IAM resource"
       }
       if (t ~ /"roles\/[^"]*([$][{]|%)/ || t ~ /"roles\/?"/) {
@@ -352,7 +377,7 @@ violations=$(
 
 if [[ -n "$violations" ]]; then
   {
-    echo "FAILED: possible scope-wide Secret Manager grant(s) found."
+    echo "FAILED: possible scope-wide or over-broad Secret Manager grant(s) found."
     echo ""
     echo "$violations"
     echo ""
@@ -367,10 +392,12 @@ if [[ -n "$violations" ]]; then
     echo "  }"
     echo ""
     echo "See terraform/modules/compute/gcp/cloud-run/main.tf for the reference shape."
-    echo "If the grant must be scope-wide, put a waiver with the reason on the line"
-    echo "directly above it:"
+    echo "A per-secret binding may grant only roles/secretmanager.secretAccessor or"
+    echo "roles/secretmanager.secretVersionAdder, written as a literal on its role line."
+    echo "If the grant must be scope-wide or needs another role, put a waiver with the"
+    echo "reason on the line directly above it:"
     echo ""
-    echo "  # gcp-secret-scope: allow - <why this grant must be scope-wide>"
+    echo "  # gcp-secret-scope: allow - <why this grant must be this wide>"
     echo ""
     echo "See the header of scripts/check-gcp-secret-scope.sh for what is exempt."
   } >&2
@@ -378,5 +405,6 @@ if [[ -n "$violations" ]]; then
 fi
 
 echo "OK: no un-waived Secret Manager role or permission outside a per-secret IAM" \
-     "resource, and no plan-time role name, in ${#files[@]} Terraform file(s)." \
+     "resource, no un-waived per-secret role other than secretAccessor or" \
+     "secretVersionAdder, and no plan-time role name, in ${#files[@]} Terraform file(s)." \
      "Values supplied from outside the scanned tree are not covered; see LIMITATIONS."
diff --git a/scripts/test-gcp-secret-scope.sh b/scripts/test-gcp-secret-scope.sh
index 8790f12b1..4f7895690 100755
--- a/scripts/test-gcp-secret-scope.sh
+++ b/scripts/test-gcp-secret-scope.sh
@@ -35,6 +35,28 @@ run_case() {
   fi
 }
 
+# Like run_case, but also asserts WHICH lines are reported: exactly the
+# `role =` lines of the file, in order. One fixture can then hold many
+# variants without a variant the guard misses hiding behind another it catches.
+run_role_lines_case() {
+  local label="$1"
+  local file="$2"
+  local expected actual out actual_exit=0
+  expected=$(grep -n '^[[:space:]]*role[[:space:]]*=' "$file" | cut -d: -f1)
+  out=$("$CHECK" "$file" 2>&1 >/dev/null) || actual_exit=$?
+  actual=$(awk -v p="${file}:" 'index($0, p) == 1 {
+             s = substr($0, length(p) + 1); sub(/:.*/, "", s); print s
+           }' <<<"$out")
+  if [[ "$actual_exit" -eq 1 && -n "$expected" && "$actual" == "$expected" ]]; then
+    echo "PASS: $label"
+    (( pass++ )) || true
+  else
+    echo "FAIL: $label (exit $actual_exit; role lines: $(tr '\n' ' ' <<<"$expected")" \
+         "; reported: $(tr '\n' ' ' <<<"$actual"))"
+    (( 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 \
@@ -183,6 +205,22 @@ run_case "waiver above a line with nothing to excuse exits 1" 1 \
 run_case "waiver covers only the next line, not a second role" 1 \
   "${FIXTURES}/waiver-one-line-only.tf.fixture"
 
+# Issue #505: a per-secret binding is exempt only for the two roles a workload
+# needs on one secret, secretAccessor and secretVersionAdder. Before #505 any
+# roles/secretmanager.* name passed there, so the #504 review's mutation of a
+# version-adder grant to roles/secretmanager.admin exited 0.
+run_case "allowed per-secret roles in every per-secret resource type exit 0 (#505)" 0 \
+  "${FIXTURES}/per-secret-allowed-roles.tf.fixture"
+
+run_case "per-secret roles/secretmanager.admin exits 1 (#505)" 1 \
+  "${FIXTURES}/per-secret-admin.tf.fixture"
+
+run_role_lines_case "every disallowed or lookalike per-secret role is reported (#505)" \
+  "${FIXTURES}/per-secret-role-lookalikes.tf.fixture"
+
+run_case "waived per-secret role outside the allowed pair exits 0 (#505)" 0 \
+  "${FIXTURES}/per-secret-role-waived.tf.fixture"
+
 # The regression half of issue #1614: the module that carried the over-broad
 # grant must stay free of one.
 run_case "cleanup-function module has no scope-wide Secret Manager grant" 0 \
@@ -208,6 +246,26 @@ fi
 run_case "ci-cd-permissions deploy SA grant fails without its waiver" 1 \
   "${WORK}/service_account.tf"
 
+# Issue #505 acceptance, against the real module the #504 review mutated: the
+# GKE workload SA's version-adder grant on the admin password secret passes as
+# written and fails once its role is widened to roles/secretmanager.admin.
+GKE="${REPO_ROOT}/terraform/modules/compute/gcp/gke/main.tf"
+sed 's|"roles/secretmanager.secretVersionAdder"|"roles/secretmanager.admin"|' \
+  "$GKE" > "${WORK}/gke-main.tf"
+if cmp -s "$GKE" "${WORK}/gke-main.tf"; then
+  echo "FAIL: gke module carries no secretVersionAdder grant to widen"
+  (( fail++ )) || true
+fi
+run_case "gke module per-secret grants pass as written (#505)" 0 "$GKE"
+run_case "gke module version-adder widened to secretmanager.admin exits 1 (#505)" 1 \
+  "${WORK}/gke-main.tf"
+
+# The waiver is what lets per-secret-role-waived pass; without it, it fails.
+grep -v 'gcp-secret-scope: allow' "${FIXTURES}/per-secret-role-waived.tf.fixture" \
+  > "${WORK}/per-secret-role-unwaived.tf"
+run_case "per-secret role outside the allowed pair fails without its waiver (#505)" 1 \
+  "${WORK}/per-secret-role-unwaived.tf"
+
 # policy-heredoc now exits 2 on its resource type alone, which would hide a
 # regression in how heredoc bodies are scanned. Retyped to a per-secret
 # _iam_policy (not refused), the role inside the JSON heredoc must still be
diff --git a/scripts/testdata/gcp-secret-scope/per-secret-admin.tf.fixture b/scripts/testdata/gcp-secret-scope/per-secret-admin.tf.fixture
new file mode 100644
index 000000000..8723620f1
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/per-secret-admin.tf.fixture
@@ -0,0 +1,11 @@
+# Fixture (issue #505): the mutation from the #504 review. A per-secret binding
+# that should be roles/secretmanager.secretVersionAdder grants
+# roles/secretmanager.admin instead, which lets the member read every version,
+# destroy versions, delete the secret and rewrite its IAM policy. The grant is
+# per-secret, so it is not scope-wide, but it is still an over-grant. Must exit 1.
+
+resource "google_secret_manager_secret_iam_member" "admin_password_writer" {
+  secret_id = var.admin_password_secret_name
+  role      = "roles/secretmanager.admin"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
diff --git a/scripts/testdata/gcp-secret-scope/per-secret-allowed-roles.tf.fixture b/scripts/testdata/gcp-secret-scope/per-secret-allowed-roles.tf.fixture
new file mode 100644
index 000000000..17edea2ae
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/per-secret-allowed-roles.tf.fixture
@@ -0,0 +1,63 @@
+# Fixture (issue #505, negative direction): the two roles a per-secret Secret
+# Manager IAM resource may grant, roles/secretmanager.secretAccessor (read a
+# version) and roles/secretmanager.secretVersionAdder (add a version), in every
+# per-secret resource type the guard exempts. Must exit 0.
+
+resource "google_secret_manager_secret_iam_member" "reader" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.secretAccessor"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_secret_iam_member" "writer" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.secretVersionAdder"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+
+  condition {
+    title      = "until-rotation"
+    expression = "request.time < timestamp(\"2030-01-01T00:00:00Z\")"
+  }
+}
+
+resource "google_secret_manager_secret_iam_binding" "readers" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.secretAccessor"
+  members   = ["serviceAccount:workload@example.iam.gserviceaccount.com"]
+}
+
+resource "google_secret_manager_regional_secret_iam_member" "regional_writer" {
+  project   = var.project_id
+  location  = var.region
+  secret_id = var.regional_secret_id
+  role      = "roles/secretmanager.secretVersionAdder"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_regional_secret_iam_binding" "regional_readers" {
+  project   = var.project_id
+  location  = var.region
+  secret_id = var.regional_secret_id
+  role      = "roles/secretmanager.secretAccessor"
+  members   = ["serviceAccount:workload@example.iam.gserviceaccount.com"]
+}
+
+resource "google_secret_manager_secret_iam_policy" "inline" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  policy_data = jsonencode({
+    bindings = [
+      {
+        role    = "roles/secretmanager.secretAccessor"
+        members = ["serviceAccount:workload@example.iam.gserviceaccount.com"]
+      },
+      {
+        role    = "roles/secretmanager.secretVersionAdder"
+        members = ["serviceAccount:workload@example.iam.gserviceaccount.com"]
+      },
+    ]
+  })
+}
diff --git a/scripts/testdata/gcp-secret-scope/per-secret-role-lookalikes.tf.fixture b/scripts/testdata/gcp-secret-scope/per-secret-role-lookalikes.tf.fixture
new file mode 100644
index 000000000..e413737dc
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/per-secret-role-lookalikes.tf.fixture
@@ -0,0 +1,123 @@
+# Fixture (issue #505): roles a per-secret Secret Manager IAM resource must NOT
+# grant without a waiver, including names that differ from an allowed role only
+# slightly. Every `role =` line in this file must be reported, one finding per
+# line; test-gcp-secret-scope.sh compares the reported line numbers with the
+# `role =` lines here, so a miss in any one block fails the test. Must exit 1.
+#
+# Roles are case-sensitive in IAM, so a differently cased allowed name is not
+# the allowed role. A role that is not a Secret Manager role at all
+# (roles/owner) and a role that is not a literal (a local) are reported too:
+# the guard cannot tell what either grants on the secret.
+
+resource "google_secret_manager_secret_iam_member" "lookalike_1" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.admin"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_secret_iam_member" "lookalike_2" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.secretVersionManager"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_secret_iam_member" "lookalike_3" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.viewer"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_secret_iam_member" "lookalike_4" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.secretaccessor"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_secret_iam_member" "lookalike_5" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.SecretAccessor"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_secret_iam_member" "lookalike_6" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.secretAccessorAdmin"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_secret_iam_member" "lookalike_7" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.secretVersionAdderX"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_secret_iam_member" "lookalike_8" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.secretAccessor.x"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_secret_iam_member" "lookalike_9" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.secretAccessor "
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_secret_iam_member" "lookalike_10" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/owner"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_secret_iam_member" "lookalike_11" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = local.writer_role
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_secret_iam_binding" "binding_admin" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  role      = "roles/secretmanager.admin"
+  members   = ["serviceAccount:workload@example.iam.gserviceaccount.com"]
+}
+
+resource "google_secret_manager_regional_secret_iam_member" "regional_version_manager" {
+  project   = var.project_id
+  location  = var.region
+  secret_id = var.regional_secret_id
+  role      = "roles/secretmanager.secretVersionManager"
+  member    = "serviceAccount:workload@example.iam.gserviceaccount.com"
+}
+
+resource "google_secret_manager_regional_secret_iam_binding" "regional_binding_admin" {
+  project   = var.project_id
+  location  = var.region
+  secret_id = var.regional_secret_id
+  role      = "roles/secretmanager.admin"
+  members   = ["serviceAccount:workload@example.iam.gserviceaccount.com"]
+}
+
+resource "google_secret_manager_secret_iam_policy" "inline_admin" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  policy_data = jsonencode({
+    bindings = [
+      {
+        role    = "roles/secretmanager.admin"
+        members = ["serviceAccount:workload@example.iam.gserviceaccount.com"]
+      },
+    ]
+  })
+}
diff --git a/scripts/testdata/gcp-secret-scope/per-secret-role-waived.tf.fixture b/scripts/testdata/gcp-secret-scope/per-secret-role-waived.tf.fixture
new file mode 100644
index 000000000..3112489bc
--- /dev/null
+++ b/scripts/testdata/gcp-secret-scope/per-secret-role-waived.tf.fixture
@@ -0,0 +1,12 @@
+# Fixture (issue #505): a per-secret role outside the allowed pair is accepted
+# when a waiver with a reason sits on the line directly above it, the same
+# escape hatch scope-wide grants have. Must exit 0, and exit 1 once the waiver
+# is removed (test-gcp-secret-scope.sh asserts both).
+
+resource "google_secret_manager_secret_iam_member" "metadata_viewer" {
+  project   = var.project_id
+  secret_id = var.secret_id
+  # gcp-secret-scope: allow - reads secret metadata only, never a payload (fixture)
+  role   = "roles/secretmanager.viewer"
+  member = "serviceAccount:auditor@example.iam.gserviceaccount.com"
+}