Repository navigation
Conversation
From the 2026-09-02 audit (issue LeanerCloud#323): - A13-014: the GCP CI impersonation member pinned only the repo id, so any other provider in the pool that mapped attribute.repository_id could impersonate the deploy SA from any ref. Map repo id and ref into one attribute (attribute.repository_id_ref) and key the member on <github_repo_id>:<deploy_ref>, matching the provider's condition. The member is replaced after the provider gains the mapping and before the old member goes (depends_on + create_before_destroy). The pool rename is deferred: it replaces the pool and provider and changes GCP_WORKLOAD_IDENTITY_PROVIDER for every GCP workflow. - A13c-021: the rotation role's RDS grant was arn:aws:rds:<region>:*:db:*. Build it from the caller account and var.rds_cluster_id, and validate the id so a wildcard cannot re-widen it. - A13c-022: replace the Cloud Run SA's roles/compute.viewer with a custom role cudlyComputeReader holding only the reads the pinned GCP provider makes (regions.list, machineTypes.list, commitments.list/get, regionOperations.get for the purchase op.Wait, resourcemanager.projects.get for credential validation and the ambient project lookup). Drop the stale compute_viewer import from gcp-import-dev-state.sh. A13c-023 (Azure subscription Reader) is deferred: the scheduler's ambient Azure fallback collects from the host subscription by default, so gating Reader off by default could break that path. Refs LeanerCloud#323 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
IAmJSD
added
impact/internal
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
Comment |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Issue #323 lists four least-privilege gaps from the 2026-09-02 audit. I re-checked each one against current
main(a14dc8f). This PR narrows three of them. The fourth (Azure) is deferred, and so is the pool rename half of A13-014, because each could break a path that works today. I didn't find any merged PR that had already fixed these. The one commit whose message cites #323 (#832, a frontend savings-percentage fix) refers to an older, unrelated issue with that number.Refs #323
Per-finding status
workloadIdentityUsernot pinned to a refattribute.repositorytoattribute.repository_id(immutable). It still had no ref, and the pool is still namedgithub-actions.attribute.repository_id_ref = assertion.repository_id + ':' + assertion.ref. The member is nowprincipalSet://…/attribute.repository_id_ref/<github_repo_id>:<deploy_ref>. Pool rename deferred (see below).ci-cd-permissionsre-apply (GCP) by a privileged human.arn:aws:rds:<region>:*:db:*rotation_function.zipdoesn't exist, so the rotation Lambda can't be applied.Resource = "arn:aws:rds:${var.region}:${data.aws_caller_identity.current.account_id}:db:${var.rds_cluster_id}", plus avalidationonrds_cluster_id(RDS identifier shape, null still allowed) so a*input can't widen it again.enable_secret_rotation = true, and that is false by default and in every committed tfvars.roles/compute.viewercompute_vieweris replaced by the custom rolecudlyComputeReader, bound to the Cloud Run SA (permissions below).roles/iam.roleAdminfor the existing custom roles, so no bootstrap re-apply is needed.ReaderunconditionallyA13c-022: why the permission list differs from the issue's suggestion
The issue suggested
compute.commitments.list/getandcompute.machineTypes.list/get. Following only that list would have broken two paths. I traced every Compute and Resource Manager call in the pinnedcloud-commitments-go/providers/gcp(v0.0.0-20261017134002-3def3f898617) and thecloud.google.com/go/computev1.54.0 operation poller, and the role holds exactly what those calls need:compute.regions.listRegions.List: the first call of GCP recommendation collection (recommendations.go,GetRegions)compute.machineTypes.listMachineTypes.List(GetValidResourceTypes, purchase validation)compute.commitments.listRegionCommitments.List(existing CUDs)compute.commitments.getRegionCommitments.GetinadoptExistingCommitment(a re-driven purchase that hits a 409)compute.regionOperations.getop.Wait()afterRegionCommitments.InsertpollsRegionOperations.Get. Without this permission, a CUD that was actually bought is reported as a failed purchase.resourcemanager.projects.getValidateCredentialsandIsConfiguredcallGetProject. The ambient path (no GCP account registered, the default on a hosted GCP deploy) also resolves its project throughprojects.list, which only returns projects the caller canget. I believeroles/compute.viewerwas what supplied this, so it is kept explicitly.compute.machineTypes.getis left out because nothing calls it. The commitment write side (cudlyCommitmentWriter) and the recommender read side (cudlyRecommendationReader) are unchanged.Deferred, with reasons
github-actionsto a CUDly-scoped id).workload_identity_pool_idis ForceNew. Renaming it destroys and recreates the pool and its provider, which changes the provider resource name stored invars.GCP_WORKLOAD_IDENTITY_PROVIDER.deploy-gcp,rollback,database-migrationandcleanup-stagingall use that variable, so they would fail until it was updated. GCP also soft-deletes pool ids for 30 days, which blocks an easy rollback. With the ref pinned in the member, a second provider in the pool would also need to emitrepository_id_reffor this repo and the deploy ref. I added a comment on the pool saying to keep it dedicated to this trust. The rename belongs in a planned bootstrap migration: create the new pool alongside the old one, switch the variable, then remove the old pool.subscription_reader. The audit treats host-subscription ingestion as opt-in, but the scheduler disagrees. Ininternal/scheduler/scheduler.go,collectAzureRecommendationsfalls back to ambient managed-identity collection againstAZURE_SUBSCRIPTION_IDwhenever no Azure account is registered. The container-apps module always sets that variable. The pinned Azure provider'sGetAllRecommendationsthen lists Redis, Cosmos, SQL, Search and Compute SKUs on the host subscription, and Advisor recommendations. Those calls needReader, not justCost Management Reader. So aenable_self_account_ingestiongate that defaults to false would quietly break default recommendation collection on any hosted Azure deployment that has no Azure accounts registered, and also for any registered Self (managed_identity) account. Owner decision: should the hosted Azure deploy ingest its own subscription by default? If not, the Reader gate and the scheduler's ambient fallback should be switched off together by the same flag, for example a flag that also stopsAZURE_SUBSCRIPTION_IDfrom enabling the fallback. That second half is an app change and outside this issue's scope.Notes for operators
ci-cd-permissions): a plain apply updates the provider in place (adds the mapping), creates the new member and then destroys the old one.depends_onpluscreate_before_destroykeeps at least one matching grant at every step. As before, pindeploy_refinterraform.tfvars, because the member now followsdeploy_refjust as the condition does. Every token the existing condition accepts already hasref == deploy_ref, so the new member matches every workflow that can authenticate today.Verification
All of this is unit, mock-provider and fixture evidence. I made no real cloud calls and ran no plan or apply against real backends. I couldn't check that GCP accepts each permission in a custom role, or that the
principalSetattribute value format works, without calling GCP. Both are expected to hold: these are standard custom-role permissions, and:and/already appear in documented GitHubprincipal/principalSetvalues.Fail before, pass after:
terraform testinterraform/environments/gcp/ci-cd-permissions(mockedgoogleprovider), with the newimmutable_trust.tftest.hclrun against theHEADmodule in a temp copy:Failure! 11 passed, 2 failed. Four new assertions fail: the member suffix/attribute.repository_id_ref/1391259743:refs/heads/main, the old/attribute.repository_id/member being absent, the mapping, and the member followingdeploy_ref(refs/heads/feature/x). On this branch:Success! 13 passed, 0 failed. This file already runs in CI's "Test CI/CD trust (immutable ids)" step.go test -count=1 ./terraform/modules/secrets/aws/(newrotation_rds_iam_test.go) fails before the fix (wildcard Resource, missingaws_caller_identity, missingrds_cluster_idvalidation) and passes after. The validation test pulls the regex out ofvariables.tfand checks it in Go's RE2. It rejects*,cudly-*,a:b,a/b, leading digits or hyphens and 64-character ids, and acceptscudly-dev-postgres. I also ran an uncommitted mock-providerterraform testprobe on a temp copy. It confirmed the rendered policy Resource isarn:aws:rds:us-east-1:<mock account>:db:cudly-dev-postgres, thatrds_cluster_id = "*"fails validation, and thatnullstill produces no policy.go test -count=1 -run TestCloudRunComputeGrantIsNarrow ./terraform/modules/compute/gcp/(newruntime_compute_iam_test.go) fails before ("must not grant a predefined roles/compute.* role", "must define … cudlyComputeReader") and passes after. It checks the exact permission set and that the role is bound exactly once, project-scoped, to the Cloud Run SA.Other checks (all passed):
shellcheck scripts/gcp-import-dev-state.shreports only SC2295 (info) at line 162. That warning already exists onmainin a line this PR doesn't touch. No non-test Go files changed, so gocyclo doesn't apply.Combined-tree verification. This branch was tested together with the other queued branches on a local, never-pushed tree:
queue/platform@936996ad= origin/maina14dc8f2plus these heads, merged in this order:fix/issue-72andfix/issue-312conflict on one line ininternal/api/handler_dashboard.go(both change thecalculateCommitmentMetricscall). The tree used #72's side with #312's validatedprovidervariable; whichever PR merges second takes that same resolution.Linux, Go 1.26.9,
AWS_CONFIG_FILE=/dev/null AWS_SHARED_CREDENTIALS_FILE=/dev/null AWS_EC2_METADATA_DISABLED=true; Docker Postgres via testcontainers:936996ad:go build ./...,go vet ./...,go vet -tags integration ./...and the fullgo test -race -short -count=1 ./....936996ad: golangci-lint v2.10.1 found 0 issues on every changed package, with and without--build-tags integration.tests/e2eis a separate module, so it was vetted under its owne2etag.gocyclo -over 10is silent on the changed non-test files.-tags integration -count=1, 60-minute local timeout because the VM was shared):936996ad, these pass:internal/server,internal/credentials,internal/scheduler,internal/database/postgres/migrations,internal/purchaseandinternal/api.internal/configpasses exceptTestPostgresStoreDB_ResolveStaleRIExchange_ValidatesCloudCheck, which fails the same way on unmodified main (fixed by fix(config): compare resolve checked_at at the database's microsecond precision #1252).26795376, same branches at earlier heads):internal/analytics,internal/auth,internal/commitmentopts,internal/databaseandcmd/lambda/clear-rate-limitall pass. The later fixes don't touch those packages.httpToLambdaRequestsignature change;scripts/setup-git-secrets.shpatterns in a fresh repo with an isolated HOME, found nothing in the changed files, and a planted fake key was flagged.scripts/test-git-secrets-allowlist.sh: 137 PASS, 0 FAIL.Out of scope (noted, not fixed)
scripts/gcp-import-dev-state.shwas already stale: it importscompute_commitment_adminwithroles/compute.admin, and that resource no longer exists. I only removed thecompute_viewerimport that this PR makes invalid.modules/compute/gcp/gke) has no Compute read grant at all, so the regions and commitments reads above would 403 there. That's a separate gap, not an over-grant.terraform/modules/compute/gcp/cloud-run/main.tfwas already over the 500-line guideline before this change (576 lines, now 608).rotation_function.zip, tracked in fix(iac/gcp): cleanup-function ships a placeholder.zip source that doesn't exist, breaking apply #124 / chore(terraform/modules): medium/low findings from the 2026-07-28 full review #131). If a future rotation function callsDescribeDBInstanceswithout an identifier, it would need an account-scopeddb:*describe-only statement.ModifyDBInstanceshould stay instance-scoped.Maintainer: please mirror the issue's triage labels on this PR:
impact/internal,triaged,priority/p3,severity/low,urgency/eventually,effort/s,type/security.🤖 Generated with Claude Code