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" +}