sec(ci): allowlist the roles a per-secret Secret Manager grant may carry

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,16 +392,19 @@ 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
   exit 1
 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."

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

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

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"]
+      },
+    ]
+  })
+}

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"]
+      },
+    ]
+  })
+}

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