sec(iac/gcp): scope cleanup-function SA to the one secret it reads - #1682
sec(iac/gcp): scope cleanup-function SA to the one secret it reads#1682cristim wants to merge 2 commits into
Conversation
`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
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
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
Comment |
1 participant
Closes #1614
What changed
terraform/modules/compute/gcp/cleanup-function/main.tf: replaces the project-scopegoogle_project_iam_member.cleanup_secretswith agoogle_secret_manager_secret_iam_memberbound tovar.db_password_secret_id, mirroring the migration already applied to the sibling Cloud Run module (compute/gcp/cloud-run/main.tf:262-267).scripts/check-gcp-secret-scope.sh+ self-tests + a gatinggcp-secret-scopeCI job: fails when any Terraform file binds aroles/secretmanager.*role through a scope-wide IAM resource.Reachability is narrower than the issue states
The issue describes this as live tenant-credential compromise. It is a real defect and worth fixing, but the exposure is latent rather than active, and the PR should not claim otherwise:
The GCP
cleanup-functionmodule is not instantiated anywhere in this repo. Nomoduleblock sources it.terraform/environments/gcp/wires onlycompute/gcp/cloud-runandcompute/gcp/gke; the same is true of the.bak. So no service account currently holds project-widesecretAccessoras a result of this resource, and the issue's failure scenario ("anyone who compromises the cleanup function...") presumes a deployed function that this repo does not deploy.What is real: the module is committed, its AWS and Azure counterparts exist, and the moment anyone wires it into an environment it ships a project-wide grant. That is a landmine, not an active breach. I would suggest re-triaging off
priority/p0/severity/criticalon that basis; I have mirrored the issue's existing labels rather than unilaterally downgrading someone else's triage.If the module was ever applied out of band, this change is still the right one and is safe: Terraform destroys the project-level binding and creates the per-secret binding, and the function keeps the only access it actually uses.
How the one-secret list was established
Narrowing IAM breaks things silently when the enumeration is wrong, so this is the evidence rather than an assertion:
cmd/cleanup-lambda/main.go(thecleanupExpiredRecordsentry point named atmain.tf:23) touches secrets only throughdatabase.OpenFromEnv. Its whole body is two SQL statements againstsessionsandpurchase_executions.internal/database/open_from_env.gobuilds a secret resolver only whendbConfig.PasswordSecret != "", and resolves that single value throughNewConnection. No other secret is fetched on that path.internal/secrets/gcp_resolver.goGetSecretissuesAccessSecretVersionagainst the one named secret.ListSecretsexists but is not on this path, andsecretAccessordoes not grant list anyway.environment_variablesblock wires exactly one secret:DB_PASSWORD_SECRET = var.db_password_secret_id.So the function reads one secret, and that is the one now bound.
Nothing referenced the removed resource address (
outputs.tfexposes only the function URI/name/schedule and the SA email), so removing it is self-contained.The guard
The bug survived because nothing looked for it: Cloud Run was migrated, its sibling was not, and there was no mechanism to notice. The guard closes that.
google_{project,folder,organization}_iam_{member,binding}carrying aroles/secretmanager.*role. Folder and org scope are included because they are strictly broader than project scope.roles/cloudsql.clientproject grants do not trip it.ci-success.needs, so it gates rather than merely reporting.terraform/tree is clean today (the cleanup-function grant was the only project-scope Secret Manager binding in the repo), so no allowlist or suppression was needed. Nothing pre-existing is being masked.Verification
cleanup-function/main.tf, naming the exact line, and exits 0 on the fixed file.scripts/test-gcp-secret-scope.sh: 7/7 pass, covering both directions plus usage errors (exit 2) so a broken invocation is distinguishable from a real finding.terraform fmt -checkclean;terraform init -backend=false+terraform validateon the module: "The configuration is valid."pre-commiton all changed files: Terraform format/validate/lint, trivy config, AWS secret scan and the rest all pass. No--no-verify.Not in scope
The AWS sibling
compute/aws/cleanup-lambdawas checked and is already correctly scoped (Resource = var.db_password_secret_arn), so there is no sibling issue to file.