diff --git a/scripts/check-gcp-secret-scope.sh b/scripts/check-gcp-secret-scope.sh index ece200f7d..ac5313411 100755 --- a/scripts/check-gcp-secret-scope.sh +++ b/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 - # "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,10 +392,12 @@ 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 - " + echo " # gcp-secret-scope: allow - " echo "" echo "See the header of scripts/check-gcp-secret-scope.sh for what is exempt." } >&2 @@ -378,5 +405,6 @@ if [[ -n "$violations" ]]; then 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." diff --git a/scripts/test-gcp-secret-scope.sh b/scripts/test-gcp-secret-scope.sh index 8790f12b1..4f7895690 100755 --- a/scripts/test-gcp-secret-scope.sh +++ b/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 diff --git a/scripts/testdata/gcp-secret-scope/per-secret-admin.tf.fixture b/scripts/testdata/gcp-secret-scope/per-secret-admin.tf.fixture new file mode 100644 index 000000000..8723620f1 --- /dev/null +++ b/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" +} diff --git a/scripts/testdata/gcp-secret-scope/per-secret-allowed-roles.tf.fixture b/scripts/testdata/gcp-secret-scope/per-secret-allowed-roles.tf.fixture new file mode 100644 index 000000000..17edea2ae --- /dev/null +++ b/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"] + }, + ] + }) +} diff --git a/scripts/testdata/gcp-secret-scope/per-secret-role-lookalikes.tf.fixture b/scripts/testdata/gcp-secret-scope/per-secret-role-lookalikes.tf.fixture new file mode 100644 index 000000000..e413737dc --- /dev/null +++ b/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"] + }, + ] + }) +} diff --git a/scripts/testdata/gcp-secret-scope/per-secret-role-waived.tf.fixture b/scripts/testdata/gcp-secret-scope/per-secret-role-waived.tf.fixture new file mode 100644 index 000000000..3112489bc --- /dev/null +++ b/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" +}