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..509f9917d --- /dev/null +++ b/scripts/check-gcp-secret-scope.sh @@ -0,0 +1,175 @@ +#!/usr/bin/env bash +# check-gcp-secret-scope.sh +# +# Fails when any Terraform file grants a Secret Manager role at project, folder +# or organization scope. Those scopes hand the member EVERY secret in the scope, +# so a workload that needs one secret ends up able to read the credential +# encryption key, the JWT/session secrets and the SendGrid API key alongside it. +# +# The supported pattern is a per-secret binding instead: +# +# resource "google_secret_manager_secret_iam_member" "..." { +# project = var.project_id +# secret_id = var.some_secret_id +# role = "roles/secretmanager.secretAccessor" +# member = "serviceAccount:${...}" +# } +# +# This guard exists because compute/gcp/cloud-run was migrated to per-secret +# bindings while its sibling compute/gcp/cleanup-function was left on a +# project-wide grant for as long as it took someone to notice (issue #1614). +# +# Exit 0 = no DIRECTLY EXPRESSED scope-wide Secret Manager grant found. Read the +# limitations below before treating this as "the tree is clean". +# Exit 1 = at least one found; each is printed to stderr. +# Exit 2 = usage error. +# +# LIMITATIONS. This is a textual guard, not a policy engine. It matches a literal +# `roles/secretmanager.*` appearing inside a scope-wide IAM resource block, and +# only that. Exit 0 means "nothing of that exact shape", NOT "no scope-wide grant +# exists". Known blind spots, each tracked: +# +# - Roles reaching the resource through `locals` + `for_each` (issue #1686). +# There is a live instance today: +# terraform/environments/gcp/ci-cd-permissions/service_account.tf grants +# project-scope `roles/secretmanager.admin` to the Terraform deploy service +# account through `local.deploy_roles`, and this guard exits 0 on it. That +# grant is believed legitimate (the deploy SA creates secrets); it is called +# out here so nobody reads a green run as proof of its absence. +# - `google_project_iam_policy` fed by a `data "google_iam_policy"` block, and +# resource bodies not indented the way `terraform fmt` produces (issue #1687). +# - The Terraform JSON encoding (`.tf.json`) is not parsed. Rather than report +# such a file as clean, the guard exits 2 if it finds one. None exist today. +# - Any role that is only knowable after variable resolution +# (`role = var.some_role`, string interpolation). +# +# It is a ratchet against the #1614 regression being reintroduced by copy-paste, +# which is how it arrived the first time. It is not a proof of absence. +# +# Usage: +# scripts/check-gcp-secret-scope.sh # scan the default roots +# scripts/check-gcp-secret-scope.sh FILE... # scan exactly these files +# +# Passing explicit files is how the test harness points the check at fixtures +# without touching the real sources. + +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" + +# Every directory holding Terraform this guard should cover. `iac/` carries the +# customer-facing federation modules, which are exactly the kind of thing that +# must not ship a scope-wide grant; scanning only `terraform/` missed all 20 of +# its .tf files. +SCAN_ROOTS=( + "${REPO_ROOT}/terraform" + "${REPO_ROOT}/iac" +) + +# Resources that bind an IAM role across a whole project/folder/organization. +# `_member` adds one principal, `_binding` replaces the whole principal list; +# both are scope-wide and both are wrong for Secret Manager here. +SCOPE_WIDE_RESOURCES='google_(project|folder|organization)_iam_(member|binding)' + +files=() +if [[ $# -gt 0 ]]; then + for arg in "$@"; do + case "$arg" in + -*) echo "Unknown flag: $arg" >&2; exit 2 ;; + esac + if [[ ! -f "$arg" ]]; then + echo "ERROR: file not found: $arg" >&2 + exit 2 + fi + files+=("$arg") + done +else + for root in "${SCAN_ROOTS[@]}"; do + if [[ ! -d "$root" ]]; then + echo "ERROR: scan root not found: $root" >&2 + exit 2 + fi + # NUL-delimited so paths with spaces survive. `.tf.json` is collected so it + # can be REJECTED below, not because it can be parsed: the scanner below + # matches HCL block syntax, and silently returning "clean" for a file it + # cannot read is the exact failure mode this guard exists to prevent. + while IFS= read -r -d '' f; do + files+=("$f") + done < <(find "$root" -type f \( -name '*.tf' -o -name '*.tf.json' \) -print0 | sort -z) + done +fi + +# Fail closed on the JSON encoding rather than under-reporting it. None exist in +# the repo today; if one is ever added, this stops rather than passing it over. +json_files=() +for f in "${files[@]}"; do + [[ "$f" == *.tf.json ]] && json_files+=("$f") +done +if [[ ${#json_files[@]} -gt 0 ]]; then + { + echo "ERROR: this guard cannot parse the Terraform JSON encoding, and will not" + echo " report a file it cannot read as clean. Found:" + printf ' %s\n' "${json_files[@]}" + echo " Extend the scanner to handle .tf.json (see issue #1687) before adding one." + } >&2 + exit 2 +fi + +if [[ ${#files[@]} -eq 0 ]]; then + echo "ERROR: no Terraform files to scan" >&2 + exit 2 +fi + +# Walk each resource block. A block opens on a `resource "" "" {` +# 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 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 new file mode 100755 index 000000000..895739765 --- /dev/null +++ b/scripts/test-gcp-secret-scope.sh @@ -0,0 +1,94 @@ +#!/usr/bin/env bash +# test-gcp-secret-scope.sh +# +# Exercises check-gcp-secret-scope.sh against testdata fixtures, in BOTH +# directions: a clean input must pass and a violating input must fail. A guard +# that only ever returns "clean" is indistinguishable from a broken one, so the +# failing cases are the ones that give this check its value. +# +# Exits 0 when all cases pass; exits 1 on any failure. + +set -euo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)" +CHECK="${SCRIPT_DIR}/check-gcp-secret-scope.sh" +FIXTURES="${SCRIPT_DIR}/testdata/gcp-secret-scope" + +pass=0 +fail=0 + +run_case() { + local label="$1" + local expected_exit="$2" + shift 2 + + local actual_exit=0 + "$CHECK" "$@" >/dev/null 2>&1 || actual_exit=$? + + if [[ "$actual_exit" -eq "$expected_exit" ]]; then + echo "PASS: $label" + (( pass++ )) || true + else + echo "FAIL: $label (expected exit $expected_exit, got $actual_exit)" + (( fail++ )) || true + fi +} + +# Negative direction: the supported shapes must not be reported. This is what +# catches a guard so blunt it fires on every per-secret binding. +run_case "per-secret binding and non-secret project grant exit 0" 0 \ + "${FIXTURES}/clean.tf.fixture" + +# Positive direction: the anti-pattern must be caught. +run_case "project-scope secretAccessor exits 1" 1 \ + "${FIXTURES}/project-scope.tf.fixture" + +run_case "project-scope _binding variant exits 1" 1 \ + "${FIXTURES}/project-binding.tf.fixture" + +# Folder and organization scope are asserted separately. They used to share one +# fixture, which meant a guard that caught only one of them still passed. +run_case "folder-scope _member variant exits 1" 1 \ + "${FIXTURES}/folder-scope.tf.fixture" + +run_case "organization-scope _binding variant exits 1" 1 \ + "${FIXTURES}/org-scope.tf.fixture" + +# A violation must still be found when mixed in with clean files. +run_case "violation alongside a clean file exits 1" 1 \ + "${FIXTURES}/clean.tf.fixture" "${FIXTURES}/project-scope.tf.fixture" + +# Usage errors are exit 2, distinct from "found a violation" (exit 1), so CI can +# tell a broken invocation apart from a real finding. +run_case "missing file exits 2" 2 \ + "${FIXTURES}/does-not-exist.tf.fixture" + +run_case "unknown flag exits 2" 2 --bogus-flag + +# The JSON encoding is not parseable by this scanner, so it must be rejected +# rather than reported clean. Exit 2 (cannot check) rather than 0 (checked and +# clean) is the whole point. +run_case "tf.json is rejected rather than silently passed" 2 \ + "${FIXTURES}/json-encoding.tf.json" + +# The regression half of issue #1614: the module that carried the over-broad +# grant must stay free of one. This is a claim the guard can actually check. +run_case "cleanup-function module has no scope-wide Secret Manager grant" 0 \ + "${REPO_ROOT}/terraform/modules/compute/gcp/cleanup-function/main.tf" + +# The default scan roots (terraform/ + iac/) must hold no violation of the shape +# this guard detects. +# +# Deliberately NOT phrased as "the tree is clean". It is not: +# terraform/environments/gcp/ci-cd-permissions/service_account.tf grants +# project-scope roles/secretmanager.admin through `locals` + `for_each`, which +# this guard structurally cannot see (issue #1686). That grant is believed +# legitimate, but the distinction matters: this case asserts what the guard +# verifies, not a broader property it never inspects. An earlier version of this +# suite claimed the tree was clean, which was false as written. +run_case "default scan roots hold no directly expressed scope-wide grant" 0 + +echo "" +echo "Results: ${pass} passed, ${fail} failed." +[[ "$fail" -eq 0 ]] 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/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 new file mode 100644 index 000000000..034e45cb3 --- /dev/null +++ b/scripts/testdata/gcp-secret-scope/org-scope.tf.fixture @@ -0,0 +1,12 @@ +# Fixture: organization scope, `_binding` variant. Must exit 1. +# +# Organization scope is the broadest of the three, and `_binding` replaces the +# whole principal list rather than adding one principal. Split from the former +# bundled folder+organization fixture so each is asserted on its own: a guard +# catching only one of them would still have passed the bundled case. + +resource "google_organization_iam_binding" "org_secrets" { + org_id = var.org_id + role = "roles/secretmanager.admin" + members = ["serviceAccount:${google_service_account.workload.email}"] +} 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}"] +} 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}" +} 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)