Repository navigation
sec(iac/gcp): scope cleanup-function SA to the one secret it reads - #1682
Conversation
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 33 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 (10)
Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 55 minutes. |
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
`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
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
4157147 to
7c32b01
Compare
Adversarial review record (merged)Independent reviewer, distinct from the author. CodeRabbit's status on this PR read The narrowed grant is provably sufficient — verified by enumerating the whole dependency closure rather than following the call chain. Reachability confirmed, and the issue is latent rather than live. No Six guard bypasses were proven, and one was live in the tree the guard certified clean: a project-scope No false positives — clean on the real What the author changed in response, and why it was better than what was askedI suggested adding an allowlist entry for the live I also suggested globbing Scan root extended to Follow-ups filedLeanerCloud/cloud-commitments-platform#149 (locals/for_each blind spot, includes the live grant), LeanerCloud/cloud-commitments-platform#150 ( |
…1682) * 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 * 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 * 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
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.