From bb0710a4229acadfd869a028bfa413925824e537 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:41:37 +0200 Subject: [PATCH 1/3] sec(iac/gcp): scope cleanup-function SA to the one secret it reads `google_project_iam_member.cleanup_secrets` granted the cleanup function's service account `roles/secretmanager.secretAccessor` at PROJECT scope, ungated. That made every secret in the project readable by a session-cleanup job, including the AES-256-GCM credential-encryption key that decrypts stored customer cloud credentials, the JWT and session secrets, and the SendGrid API key. The function reads exactly one secret. `cmd/cleanup-lambda/main.go` only calls `database.OpenFromEnv`, which builds a secret resolver solely when `dbConfig.PasswordSecret` is non-empty and resolves that single value via `AccessSecretVersion`; the module wires only `DB_PASSWORD_SECRET = var.db_password_secret_id` into the function environment. No other secret is reachable from that code path. Replace the project-scope binding with a `google_secret_manager_secret_iam_member` scoped to `var.db_password_secret_id`, mirroring the migration already applied to the sibling Cloud Run module (compute/gcp/cloud-run/main.tf:262-267). The cleanup function was never migrated when Cloud Run was. Refs #1614 --- .../compute/gcp/cleanup-function/main.tf | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/terraform/modules/compute/gcp/cleanup-function/main.tf b/terraform/modules/compute/gcp/cleanup-function/main.tf index 213a6b802..ac6b777b4 100644 --- a/terraform/modules/compute/gcp/cleanup-function/main.tf +++ b/terraform/modules/compute/gcp/cleanup-function/main.tf @@ -5,11 +5,25 @@ resource "google_service_account" "cleanup" { display_name = "Service account for ${var.function_name}" } -# Grant access to Secret Manager -resource "google_project_iam_member" "cleanup_secrets" { - project = var.project_id - role = "roles/secretmanager.secretAccessor" - member = "serviceAccount:${google_service_account.cleanup.email}" +# Secret Manager access: least-privilege, single-secret binding. +# +# This previously granted the cleanup service account project-wide +# `roles/secretmanager.secretAccessor`, which made every secret in the project +# readable by a session-cleanup job. That set includes the AES-256-GCM +# credential-encryption key that decrypts stored customer cloud credentials, the +# JWT and session secrets, and the SendGrid API key. None of them are reachable +# from this function's code path. +# +# The function reads exactly one secret: the database password, wired into +# DB_PASSWORD_SECRET in service_config below and resolved once by +# database.OpenFromEnv (cmd/cleanup-lambda/main.go). Bind only that secret, +# mirroring the same migration already applied to the Cloud Run module in +# compute/gcp/cloud-run/main.tf. +resource "google_secret_manager_secret_iam_member" "cleanup_db_password" { + project = var.project_id + secret_id = var.db_password_secret_id + role = "roles/secretmanager.secretAccessor" + member = "serviceAccount:${google_service_account.cleanup.email}" } # Cloud Function (2nd gen) From cf73337af98af98672c32dab3e4fa7e0eda242ab Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:45:30 +0200 Subject: [PATCH 2/3] sec(ci): fail CI on project-scope Secret Manager grants in Terraform The over-broad grant fixed in the previous commit arrived by copy-paste and survived because nothing looked for it: `compute/gcp/cloud-run` was migrated to per-secret bindings and its sibling `compute/gcp/cleanup-function` was not, with no mechanism to notice the gap. Add `scripts/check-gcp-secret-scope.sh`, which fails when a Terraform file binds a `roles/secretmanager.*` role through a scope-wide IAM resource (`google_{project,folder,organization}_iam_{member,binding}`). Per-secret bindings and scope-wide grants of non-Secret-Manager roles are left alone, so the existing `roles/cloudsql.client` project grants stay quiet. Wire it into CI as the `gcp-secret-scope` job, following the existing azure-role-parity / aws-iam-parity shape, and add it to `ci-success.needs` so it actually gates rather than reporting alongside. `scripts/test-gcp-secret-scope.sh` exercises the guard in both directions: clean input must exit 0, each violating shape must exit 1, and usage errors must exit 2 so a broken invocation is distinguishable from a real finding. Verified the guard fires on the pre-fix `cleanup-function/main.tf` at the exact line and passes on the fixed one; the whole `terraform/` tree is clean today, so no allowlist or suppression was needed. The guard is textual, not a policy engine: a role supplied via a variable is invisible to it. That limitation is documented in the script rather than implied away. Closes #1614 --- .github/workflows/ci.yml | 20 +++ scripts/check-gcp-secret-scope.sh | 127 ++++++++++++++++++ scripts/test-gcp-secret-scope.sh | 65 +++++++++ .../gcp-secret-scope/clean.tf.fixture | 25 ++++ .../gcp-secret-scope/org-scope.tf.fixture | 15 +++ .../gcp-secret-scope/project-scope.tf.fixture | 12 ++ 6 files changed, 264 insertions(+) create mode 100755 scripts/check-gcp-secret-scope.sh create mode 100755 scripts/test-gcp-secret-scope.sh create mode 100644 scripts/testdata/gcp-secret-scope/clean.tf.fixture create mode 100644 scripts/testdata/gcp-secret-scope/org-scope.tf.fixture create mode 100644 scripts/testdata/gcp-secret-scope/project-scope.tf.fixture diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b78b1cb74..2ea4ae5a4 100644 --- a/.github/workflows/ci.yml +++ b/.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: diff --git a/scripts/check-gcp-secret-scope.sh b/scripts/check-gcp-secret-scope.sh new file mode 100755 index 000000000..b052eef25 --- /dev/null +++ b/scripts/check-gcp-secret-scope.sh @@ -0,0 +1,127 @@ +#!/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 scope-wide Secret Manager grants found. +# Exit 1 = at least one found; each is printed to stderr. +# Exit 2 = usage error. +# +# Limitation: this is a textual guard, not a policy engine. It matches a literal +# `roles/secretmanager.*` inside a scope-wide IAM resource block, so a role +# supplied indirectly (`role = var.some_role`, or built by string interpolation) +# is invisible to it. It is a ratchet against the specific regression in #1614 +# being reintroduced by copy-paste, which is how it arrived the first time; it +# is not a proof that no scope-wide grant can exist. +# +# Usage: +# scripts/check-gcp-secret-scope.sh # scan terraform/ +# 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)" +SCAN_ROOT="${REPO_ROOT}/terraform" + +# 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 + if [[ ! -d "$SCAN_ROOT" ]]; then + echo "ERROR: scan root not found: $SCAN_ROOT" >&2 + exit 2 + fi + # NUL-delimited so paths with spaces survive. + while IFS= read -r -d '' f; do + files+=("$f") + done < <(find "$SCAN_ROOT" -type f -name '*.tf' -print0 | sort -z) +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 "" "" {` +# 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\" \"\" {" + echo " project = var.project_id" + echo " secret_id = var." + 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 project/folder/organization-scoped Secret Manager grants in ${#files[@]} Terraform file(s)." diff --git a/scripts/test-gcp-secret-scope.sh b/scripts/test-gcp-secret-scope.sh new file mode 100755 index 000000000..dad490bc8 --- /dev/null +++ b/scripts/test-gcp-secret-scope.sh @@ -0,0 +1,65 @@ +#!/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)" +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 "folder/org scope and _binding variant exit 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 real tree must be clean: this is the regression half of issue #1614. +run_case "repository terraform/ tree is clean" 0 + +echo "" +echo "Results: ${pass} passed, ${fail} failed." +[[ "$fail" -eq 0 ]] diff --git a/scripts/testdata/gcp-secret-scope/clean.tf.fixture b/scripts/testdata/gcp-secret-scope/clean.tf.fixture new file mode 100644 index 000000000..2102fd16b --- /dev/null +++ b/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}" +} diff --git a/scripts/testdata/gcp-secret-scope/org-scope.tf.fixture b/scripts/testdata/gcp-secret-scope/org-scope.tf.fixture new file mode 100644 index 000000000..56d3daead --- /dev/null +++ b/scripts/testdata/gcp-secret-scope/org-scope.tf.fixture @@ -0,0 +1,15 @@ +# Fixture: the wider scopes and the _binding variant the guard also rejects. +# A folder or organization grant is strictly broader than a project grant, and +# `_binding` replaces the whole principal list rather than adding one. Must exit 1. + +resource "google_folder_iam_member" "folder_secrets" { + folder = var.folder_id + role = "roles/secretmanager.secretAccessor" + member = "serviceAccount:${google_service_account.workload.email}" +} + +resource "google_organization_iam_binding" "org_secrets" { + org_id = var.org_id + role = "roles/secretmanager.admin" + members = ["serviceAccount:${google_service_account.workload.email}"] +} diff --git a/scripts/testdata/gcp-secret-scope/project-scope.tf.fixture b/scripts/testdata/gcp-secret-scope/project-scope.tf.fixture new file mode 100644 index 000000000..232411d07 --- /dev/null +++ b/scripts/testdata/gcp-secret-scope/project-scope.tf.fixture @@ -0,0 +1,12 @@ +# Fixture: the exact anti-pattern issue #1614 fixed. Must exit 1. + +resource "google_service_account" "cleanup" { + project = var.project_id + account_id = "cleanup-sa" +} + +resource "google_project_iam_member" "cleanup_secrets" { + project = var.project_id + role = "roles/secretmanager.secretAccessor" + member = "serviceAccount:${google_service_account.cleanup.email}" +} From 7c32b01ea2077d3a3febec127bdd035bcb210c70 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 13:54:09 +0200 Subject: [PATCH 3/3] sec(ci): make the Secret Manager guard cover iac/ and stop overclaiming Adversarial review of #1682 found the guard's self-test asserting a property the guard does not check, and the scan root missing a whole tree. Both are the same defect class the guard exists to catch: a green check standing in for a condition nobody verified. Scan root now covers `iac/` as well as `terraform/`. That is 20 further files, including the customer-facing federation modules, which are exactly the code that must not ship a scope-wide grant. Verified the extension reds nothing: 177 files under terraform/ plus 20 under iac/ = 197 scanned, exit 0. The self-test previously asserted "repository terraform/ tree is clean". That was false. `terraform/environments/gcp/ci-cd-permissions/service_account.tf` grants project-scope `roles/secretmanager.admin` through `locals` + `for_each`, which the per-resource-block scanner structurally cannot see, so the guard exits 0 on it. That grant is believed legitimate (the Terraform deploy service account creates secrets), so the fix is to the claim, not the grant: the case now asserts only what the guard verifies, and names the known gap. Resolving `locals`/`for_each` is tracked in #1686. An allowlist entry was considered and rejected: an entry for a grant the parser cannot detect would never be exercised, which would be a second false assurance rather than a fix. `.tf.json` is now discovered and REJECTED with exit 2 rather than scanned. The scanner matches HCL block syntax, so reporting a JSON-encoded file as clean would be the precise failure being guarded against. None exist today; if one is added the guard stops instead of passing it over. Test gaps closed: `google_project_iam_binding` had no fixture at all (it worked, but untested), and folder and organization scope shared one fixture, so a guard catching only one of them still passed. Split into separate cases. Suite is 11/11, covering exit 0 (clean), exit 1 (each violating shape individually) and exit 2 (usage error, unreadable encoding). Follow-ups filed, deliberately not fixed here: #1686 locals/for_each blind spot, #1687 google_project_iam_policy plus data block and the unindented-resource bypass, #1688 ci-success not failing on skipped jobs. Refs #1614 --- scripts/check-gcp-secret-scope.sh | 84 +++++++++++++++---- scripts/test-gcp-secret-scope.sh | 35 +++++++- .../gcp-secret-scope/folder-scope.tf.fixture | 10 +++ .../gcp-secret-scope/json-encoding.tf.json | 20 +++++ .../gcp-secret-scope/org-scope.tf.fixture | 15 ++-- .../project-binding.tf.fixture | 12 +++ 6 files changed, 146 insertions(+), 30 deletions(-) create mode 100644 scripts/testdata/gcp-secret-scope/folder-scope.tf.fixture create mode 100644 scripts/testdata/gcp-secret-scope/json-encoding.tf.json create mode 100644 scripts/testdata/gcp-secret-scope/project-binding.tf.fixture diff --git a/scripts/check-gcp-secret-scope.sh b/scripts/check-gcp-secret-scope.sh index b052eef25..509f9917d 100755 --- a/scripts/check-gcp-secret-scope.sh +++ b/scripts/check-gcp-secret-scope.sh @@ -19,19 +19,35 @@ # 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 scope-wide Secret Manager grants found. +# 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. # -# Limitation: this is a textual guard, not a policy engine. It matches a literal -# `roles/secretmanager.*` inside a scope-wide IAM resource block, so a role -# supplied indirectly (`role = var.some_role`, or built by string interpolation) -# is invisible to it. It is a ratchet against the specific regression in #1614 -# being reintroduced by copy-paste, which is how it arrived the first time; it -# is not a proof that no scope-wide grant can exist. +# 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 terraform/ +# 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 @@ -40,7 +56,15 @@ set -euo pipefail REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" -SCAN_ROOT="${REPO_ROOT}/terraform" + +# 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; @@ -60,14 +84,35 @@ if [[ $# -gt 0 ]]; then files+=("$arg") done else - if [[ ! -d "$SCAN_ROOT" ]]; then - echo "ERROR: scan root not found: $SCAN_ROOT" >&2 - exit 2 - fi - # NUL-delimited so paths with spaces survive. - while IFS= read -r -d '' f; do - files+=("$f") - done < <(find "$SCAN_ROOT" -type f -name '*.tf' -print0 | sort -z) + 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 @@ -124,4 +169,7 @@ if [[ -n "$violations" ]]; then exit 1 fi -echo "OK: no project/folder/organization-scoped Secret Manager grants in ${#files[@]} Terraform file(s)." +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)." diff --git a/scripts/test-gcp-secret-scope.sh b/scripts/test-gcp-secret-scope.sh index dad490bc8..895739765 100755 --- a/scripts/test-gcp-secret-scope.sh +++ b/scripts/test-gcp-secret-scope.sh @@ -11,6 +11,7 @@ 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" @@ -43,7 +44,15 @@ run_case "per-secret binding and non-secret project grant exit 0" 0 \ run_case "project-scope secretAccessor exits 1" 1 \ "${FIXTURES}/project-scope.tf.fixture" -run_case "folder/org scope and _binding variant exit 1" 1 \ +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. @@ -57,8 +66,28 @@ run_case "missing file exits 2" 2 \ run_case "unknown flag exits 2" 2 --bogus-flag -# The real tree must be clean: this is the regression half of issue #1614. -run_case "repository terraform/ tree is clean" 0 +# 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." diff --git a/scripts/testdata/gcp-secret-scope/folder-scope.tf.fixture b/scripts/testdata/gcp-secret-scope/folder-scope.tf.fixture new file mode 100644 index 000000000..d0667e4bc --- /dev/null +++ b/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}" +} diff --git a/scripts/testdata/gcp-secret-scope/json-encoding.tf.json b/scripts/testdata/gcp-secret-scope/json-encoding.tf.json new file mode 100644 index 000000000..488538ddc --- /dev/null +++ b/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}" + } + } + } +} diff --git a/scripts/testdata/gcp-secret-scope/org-scope.tf.fixture b/scripts/testdata/gcp-secret-scope/org-scope.tf.fixture index 56d3daead..034e45cb3 100644 --- a/scripts/testdata/gcp-secret-scope/org-scope.tf.fixture +++ b/scripts/testdata/gcp-secret-scope/org-scope.tf.fixture @@ -1,12 +1,9 @@ -# Fixture: the wider scopes and the _binding variant the guard also rejects. -# A folder or organization grant is strictly broader than a project grant, and -# `_binding` replaces the whole principal list rather than adding one. Must exit 1. - -resource "google_folder_iam_member" "folder_secrets" { - folder = var.folder_id - role = "roles/secretmanager.secretAccessor" - member = "serviceAccount:${google_service_account.workload.email}" -} +# 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 diff --git a/scripts/testdata/gcp-secret-scope/project-binding.tf.fixture b/scripts/testdata/gcp-secret-scope/project-binding.tf.fixture new file mode 100644 index 000000000..52a91d0cd --- /dev/null +++ b/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}"] +}