[IAmJSD]

Root cause

scripts/check-gcp-secret-scope.sh (as rewritten for #149 in #995) treats every secretmanager mention outside a per-secret IAM resource as a finding. That works because a Secret Manager role normally has to appear as text somewhere in the scanned tree. The authoritative-policy shape breaks that assumption. google_{project,folder,organization}_iam_policy takes policy_data that can come from file("policy.json") (a file the guard doesn't scan), from a variable with no in-tree default, or from a data "google_iam_policy" exported through a module output. In those cases no secretmanager text reaches the guard, and it reported the configuration clean (exit 0). That shape is also the most dangerous one, because it replaces the scope's whole IAM policy.

Part 2 of the issue (the unindented-body bypass) no longer reproduces on main. Since #995, block tracking only narrows the per-secret exemption and never decides whether a role is reported, so a body that starts in column 0 can only cause over-reporting. The issue's suggested fmt gate still had a gap: CI ran terraform fmt -check -recursive over terraform/ but not over iac/, which the guard also scans.

Fix

The fix follows the issue's simplified remedy: fail closed instead of correlating policy_data.

1. Authoritative policies exit 2. A new pass runs before the main scan. It exits 2 on any non-comment mention of google_project_iam_policy, google_folder_iam_policy, google_organization_iam_policy or google_iam_policy, matched as a whole token. That covers quoted or bare block labels, any indentation, and references such as data.google_iam_policy.x.policy_data, moved and import. This mirrors the existing .tf.json handling. No waiver applies, and the message points to this issue and to the _iam_member alternatives. Lookalike types (google_secret_manager_secret_iam_policy, google_storage_bucket_iam_policy) and comments are not affected.
2. fmt gate covers every scan root. The CI step Terraform Format Check now also runs terraform fmt -check -recursive iac/. iac/ is already formatted: terraform fmt -check -recursive iac/ exits 0 locally on Terraform 1.15.8; CI pins 1.10.5. The self-test reads SCAN_ROOTS out of the guard and fails if ci.yml has no matching terraform fmt -check -recursive <root>/ line, so a root added later can't skip the gate.
3. The header's Exit 2 line and LIMITATIONS now say the authoritative-policy shape is rejected, not analyzed. LIMITATIONS also notes the fmt gate and that modules sourced from outside the tree aren't scanned.

Three existing fixtures (iam-policy-data, policy-heredoc, dynamic-iam-policy) contain google_project_iam_policy and now expect exit 2 instead of 1. That is intended: both codes fail CI, and 2 is the more accurate answer. policy-heredoc was the only test of heredoc-body scanning, so to keep that covered the self-test also runs a copy retyped to google_secret_manager_secret_iam_policy and asserts that the role in the JSON heredoc is still reported (exit 1).

Verification

All of this is fixture and self-test evidence from a local run on macOS (BWK awk). CI runs mawk on Linux, so I also ran it in an ubuntu:24.04 container (mawk): the self-test gives 53 passed, 0 failed, and the guard exits 0 on the real repo. No Go code changed, so Go build, vet, golangci-lint and gocyclo don't apply. Nothing here touches real cloud APIs.

New fixtures run against main's guard and against this branch (bash <guard> <fixture>):

Fixture	main	branch
project-iam-policy-file (policy_data = file(...))	0 (missed)	2
folder-iam-policy (policy_data from a var with no default)	0 (missed)	2
org-iam-policy (jsonencode with a role var)	0 (missed)	2
iam-policy-data-output (data source exported as an output)	0 (missed)	2
iam-policy-unquoted-labels (resource google_project_iam_policy project {)	0 (missed)	2
iam-policy-unindented (column-0 body)	0 (missed)	2
unindented-body (scope-wide grant, column-0 nested } before the role)	1	1 (pins part 2; already caught since #995)
per-secret-unindented-body (column-0 per-secret body followed by a project grant)	1	1 (pins part 2)
iam-policy-lookalikes (negative direction)	0	0

Full self-test, branch version, run against main's guard and main's ci.yml: Results: 42 passed, 11 failed. The 10 authoritative-policy cases fail, plus CI has no 'terraform fmt -check -recursive iac/' step for scan root iac/.

The same self-test on the branch: bash scripts/test-gcp-secret-scope.sh gives Results: 53 passed, 0 failed.

Real repo, no new false positives: bash scripts/check-gcp-secret-scope.sh gives OK: ... in 227 Terraform file(s) (exit 0). The tree has no google_*_iam_policy or google_iam_policy.

Mutation probes. Each one was applied to the branch, the self-test was run, and the file was restored. Every probe was caught:

• dropping folder from the type regex fails google_folder_iam_policy fed by a variable exits 2
• dropping the google_iam_policy match fails google_iam_policy data source exported as an output exits 2
• removing the comment skip fails lookalike IAM policy types and comments exit 0
• disabling the exit-2 branch causes 10 failures
• deleting the new iac/ fmt line from ci.yml fails the fmt-coverage check

Other checks:

• shellcheck scripts/check-gcp-secret-scope.sh scripts/test-gcp-secret-scope.sh: clean. One SC2016 is disabled inline because ${REPO_ROOT} is matched literally there.
• actionlint .github/workflows/ci.yml: clean.
• terraform fmt -check -recursive terraform/ and terraform fmt -check -recursive iac/: both exit 0.
• git-secrets 1.3.0 (setup-git-secrets.sh patterns plus .gitallowed), run with git secrets --scan --cached over scripts/ and .github/ in a fresh git init repo with an isolated HOME: clean. bash scripts/test-git-secrets-allowlist.sh: 42 PASS, 0 FAIL.
• git diff --check: clean.

Remaining blind spots (regex-level guard, not an HCL parser)

The issue explicitly rejected a fully correct fix, which would mean parsing HCL and following values from where they are written to the resource that grants them. This PR is the regex-level hardening the issue asks for. These gaps remain:

• Values from outside the tree (-var, tfvars kept elsewhere, environment variables, external callers of iac/ modules) were already listed under LIMITATIONS and are unchanged.
• Modules sourced from a registry or git, such as terraform-google-modules/iam in authoritative mode, are not scanned. This is now listed under LIMITATIONS.
• /* ... */ block comments are treated as code by both passes. That only causes over-reporting.
• The new pass also fires on the read-only data "google_project_iam_policy" and on any google_iam_policy data source, including one that feeds a per-secret _iam_policy. That is deliberate fail-closed over-reporting: the guard can't tell where policy_data flows. None of these exist in the tree.
• ephemeral (Terraform 1.10+) is not in the scanner's list of construct openers. It can't grant IAM, so I left it alone and am noting it here.

Out of scope

• sec(ci): check-gcp-secret-scope guard is blind to roles supplied via locals/for_each #149 (locals/for_each) is already fixed on main by fix(ci): make check-gcp-secret-scope see roles supplied via locals/for_each #995 and wasn't touched.
• ci(gcp): secret scope guard does not check the role on secret-level Secret Manager grants #505 (role allowlist on secret-level grants) isn't done here. Its change belongs in taint() in the main awk scan, at the allowed && s ~ /^...role...="roles\/secretmanager\.[A-Za-z]+"/ exemption: narrow [A-Za-z]+ to the expected roles, for example (secretAccessor|secretVersionAdder). This PR doesn't touch that code; the new pass is separate and runs before it. One input for ci(gcp): secret scope guard does not check the role on secret-level Secret Manager grants #505's open question about _policy resources: after this PR, a per-secret _iam_policy fed by data "google_iam_policy" already exits 2.
• The Makefile's terraform-fmt-check and terraform-fmt targets still cover only terraform/. The issue asked only for the CI gate.

Frontend follow-up for the UI owner

None. This PR has no frontend impact.

Labels: could a maintainer please mirror the issue's triage labels onto this PR: impact/internal, triaged, priority/p3, severity/low, urgency/eventually, effort/m, type/security.

Closes #150

🤖 Generated with Claude Code

[coderabbitai]

Caution

Review failed

The pull request is closed.

Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

Independent review 2 (claude-fleet, cold): APPROVE e9e1c62

Findings (none blocking):

• [non-blocking] scripts/testdata/gcp-secret-scope/iam-policy-lookalikes.tf.fixture - Neither token boundary on the new pass is pinned by a test.
  • Removing the left boundary on google_iam_policy (M2), or the right boundary on google_(project|folder|organization)_iam_policy (M3), still gives 53/53.
  • my_google_iam_policy_name is stopped by the right boundary alone.
  • No real provider type hits either gap today, but a later edit could quietly over-report or under-report.
  • Fix: add xgoogle_iam_policy and google_project_iam_policy_x lines.
• [non-blocking] scripts/test-gcp-secret-scope.sh (the SCAN_ROOTS sed) - The parser reads only entries written as "${REPO_ROOT}/<dir>".
  • A root added in any other form is skipped silently.
  • The check fails only when the whole list comes back empty.
• [non-blocking, disclosed] The Makefile's terraform-fmt-check and pre-commit still cover only terraform/.

Against #150: the PR implements the issue's simplified remedy, and all three acceptance criteria hold.

• It fails closed with exit 2 before the main scan (check-gcp-secret-scope.sh, in a new block after the files-empty check).
• It adds the iac/ line to ci.yml's "Terraform Format Check" step.
• Part 2 no longer reproduces on base: the base guard returns 1 for unindented-body and for per-secret-unindented-body.
• There is no Go code, migration or frontend change.

Blast radius:

• Only the ci.yml gcp-secret-scope job and the self-test call the guard, and exit 2 also fails CI.
• The real tree has no google_*_iam_policy or google_iam_policy, so there are no new false positives: exit 0 on 227 files.
• The run: | block runs under bash -e, so the first fmt failure fails the step.

Drift: main has no changes to the guard, its tests, testdata, ci.yml or iac/ since the base. merge-tree --write-tree origin/main head is clean, and everything was re-run on the merged export.

Attacked:

• Mutation probes on a scratch copy, each restored and confirmed identical with cmp:
  • M1, organization dropped from the type regex: 1 FAIL (the org case). Caught.
  • M2, the left boundary dropped on google_iam_policy: 53/53. Gap.
  • M3, the right boundary dropped on the scope types: 53/53. Gap.
  • M4, the new pass exits 1 instead of 2: 10 FAIL. Caught.
  • M5, the comment skip removed: the lookalikes case FAILS. Caught.
  • M6, the iac fmt line commented out in ci.yml: the fmt-coverage check FAILS. Caught.
  • M7, that line retargeted to iac/federation/: FAILS. Caught.
• Other inputs: CRLF, /* */ prefixes and bare labels, each reasoned through or covered by fixtures.

Verified locally:

• bash scripts/test-gcp-secret-scope.sh: 53 passed, 0 failed on head with both BWK awk and mawk, and on the merged export.
• bash scripts/check-gcp-secret-scope.sh on the real tree: exit 0 on head (both awks) and on the merged tree.
• Fail-before: the head's tests and fixtures over the base guard and ci.yml give 42 passed and 11 failed. That is exactly the 10 sec(ci): check-gcp-secret-scope misses google_project_iam_policy and unindented resource blocks #150 exit-2 cases plus the iac fmt-coverage check.
• terraform fmt -check -recursive on terraform/ and iac/ with Terraform v1.10.5 (the CI pin): exit 0 on head and merged.
• shellcheck, actionlint on ci.yml and git diff --check: clean.

Reviewed by an independent agent with no author context (second reviewer). Review only: nothing was pushed, and the GitHub review state is unchanged.

[cristim]

VERDICT: APPROVE e9e1c62

Reviewed head e9e1c62 against origin/main 6776e27. git merge-tree is clean (merged tree 19fa7fd2). All checks below ran on the merged tree.

Findings (none blocking):

• nit .github/workflows/ci.yml:968-970 (merged tree) - the comment says the guard's block tracking "was written against fmt layout". The guard header added in the same PR (scripts/check-gcp-secret-scope.sh, LIMITATIONS) says tracking "does not need terraform fmt layout to stay closed". The two statements should agree.
• nit scripts/test-gcp-secret-scope.sh (fmt-coverage block) - the comment line "every root the guard scans. Read the roots ... fmt gate" is about 100 columns wide, while the rest of the file wraps at 80.
• nit scripts/test-gcp-secret-scope.sh fmt-coverage grep - any matching terraform fmt -check -recursive <root>/ line anywhere in ci.yml satisfies it, including one in a step gated by if:. That is acceptable for a self-test.

Coverage: the issue's acceptance criteria are met. google_{project,folder,organization}_iam_policy and google_iam_policy exit 2, with a token-bounded match that skips comments. Every other *_iam_policy type (bucket, service account) is per-resource and correctly not refused. Per-secret _iam_policy is the sanctioned shape. *_iam_binding needs no new handling because its role must appear as text, so the main scan catches it the same way it catches _member. .tf.json and .tfvars.json are rejected (exit 2) before the new pass in both default-root and explicit-file modes. The existing tf.json case still passes.

Attacked:

• Self-test: 53 passed, 0 failed under macOS BWK awk, and the same under mawk 1.3.4 via an awk->mawk PATH shim.
• The guard on the real tree exits 0 (227 files). A grep of the tree outside fixtures finds no google_*_iam_policy or google_iam_policy, so there are no false positives.
• shellcheck on both scripts: clean. actionlint on ci.yml: clean.
• Probe fixtures: a tab-separated resource "google_organization_iam_policy" exits 2; a CRLF google_folder_iam_policy file exits 2.
• Mutation probes (scratch copy, each one restored), all caught:
  • dropping organization from the type regex fails 1 case;
  • deleting the comment skip fails the lookalikes case;
  • neutralising exit 2 fails 10 cases;
  • deleting the iac/ fmt line from ci.yml fails the fmt-coverage check;
  • breaking the google_iam_policy alternative fails the data-output case.
• CI on e9e1c62: every check passes (Snyk skipped), including "GCP Secret Manager grant scope" and all three "Validate Terraform" legs, so iac/ passes fmt under Terraform 1.10.5.
• Commit size: one commit with 278 changed lines, under the 400 cap.

[IAmJSD]

Independent review (IAmJSD's agent, fresh context, given only the diff, the issue and the PR text): VERDICT: APPROVE e9e1c6277aaa074bcf0841d527d7fb3733d7956b

Blocking:

• none

Non-blocking:

• scripts/test-gcp-secret-scope.sh (fmt-coverage check): the grep accepts a terraform fmt -check -recursive <root>/ line anywhere in ci.yml. That includes a line in a step with an if: that skips it, or in a job that doesn't gate merges. It's good enough as a tripwire, but it doesn't prove the gate runs.
• The two comments disagree. ci.yml:596 says the guard's "block tracking was written against fmt layout". The new LIMITATIONS bullet says block tracking "does not need terraform fmt layout to stay closed". One of them should change so a later reader isn't misled about what the fmt gate protects.
• Makefile terraform-fmt-check still covers only terraform/ (the PR body notes this). A one-line follow-up would keep local and CI behaviour the same.

Attacked:

• Acceptance criteria for sec(ci): check-gcp-secret-scope misses google_project_iam_policy and unindented resource blocks #150: a google_project_iam_policy fixture exits 2; CI fmt covers both SCAN_ROOTS (terraform/, iac/); iac/ passes terraform fmt -check -recursive on TF 1.10.5, the CI pin; LIMITATIONS records the rejection. All met, so Closes is right.
• Bypass probes against the new awk pass. Not skipped (still caught): bare and quoted labels, data.google_iam_policy.x references, moved/import targets, and a type name after an inline /* */. Lookalikes (google_secret_manager_secret_iam_policy, google_storage_bucket_iam_policy) don't match because of the token boundary. Only whole-line #/// comments are skipped. Anything else can only over-report, which is the safe direction.
• Fail-before: with main's check-gcp-secret-scope.sh and ci.yml restored and the branch self-test kept, the result is 42 passed, 11 failed. That's the 10 authoritative-policy cases plus the iac/ fmt check; 6 of them exit 0 on main, which is the false green. With the files restored to the branch: 53 passed, 0 failed under both gawk and mawk (CI's awk, via a PATH shim).
• bash scripts/check-gcp-secret-scope.sh on the real tree: exit 0 over 227 files, no new false positives. shellcheck is clean on both scripts. No Go code changed.

Body-stale:

• none

_{Reviewer: rA2. Bar while the maintainers are away (set by IAmJSD as acting owner): one approving independent review on the head, two for money, auth and security paths.}

[IAmJSD]

Local verification at e9e1c6277aaa074bcf0841d527d7fb3733d7956b merged onto main (2514c01a), together with #1203, #1207, #1211, #1214, #1216 in that order (combined tree 995207c1, Linux, Go 1.26.10): go build ./..., go vet with and without -tags integration, the full go test -race -short -count=1 ./..., and go test -race -tags integration -count=1 on every changed package, against Docker Postgres. All pass. CI Success and pre-commit are green on this head, with 2 approving independent review(s) on it. Nothing was run against any cloud.

[IAmJSD]

Merged by IAmJSD's agent at IAmJSD's request (IAmJSD is acting owner while the maintainers are away), under the bar IAmJSD set: 2 approving independent review(s) on e9e1c627 (two required: money, auth or security path), green CI, and the combined-tree verification above. Squash-merged at exactly e9e1c6277aaa074bcf0841d527d7fb3733d7956b.