Repository navigation
fix(iac): key the GCP deploy trust on immutable repository ids - #1066
Conversation
|
@coderabbitai review |
|
Warning Review limit reached
This review includes 7 billable files and costs up to $1.75.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 32 minutes for your next included review. View limit details
Comment |
|
cristim
left a comment
There was a problem hiding this comment.
Independent review of 5b1ad3d: no actionable findings
No BLOCKING and no NON-BLOCKING findings survived.
What I attacked
- CEL vs OIDC claims.
github_oidc.tfcondition isassertion.repository_owner_id == '${var.github_owner_id}' && assertion.repository_id == '${var.github_repo_id}' && assertion.ref == '${var.deploy_ref}'. GitHub emitsrepository_id,repository_owner_idandrefas string claims, so comparing to quoted strings is type-correct. The provider mappingsattribute.repository_id = assertion.repository_idandattribute.repository_owner_id = assertion.repository_owner_idare valid custom attributes. The member is.../attribute.repository_id/${var.github_repo_id}, which matches the mapping. - No widening. The condition still pins repo (now by id), owner id and the exact
deploy_ref, so forks, PRs, tags and other branches stay rejected. Owner id plus repo id is strictly narrower than the old name match. Thedeploy_reflogic from CLAUDE.md is untouched (defaultrefs/heads/main, testcondition_follows_deploy_ref). - Injection.
deploy_refis validated^refs/(heads|tags)/[A-Za-z0-9._/-]+$before being interpolated into CEL (quote is excluded). IDs are^[1-9][0-9]*$. Test rejectsx' || true. - Defaults and secrets.
github_owner_idandgithub_repo_idhave no default, so an old tfvars fails loud at plan. The IDs interraform.tfvars.exampleare public GitHub ids, not secrets. No project or account ids added.*.tfvarsis gitignored (.gitignore:108-109). - Migration. The
attribute.repositorymapping is kept, so the old member keeps resolving whilecreate_before_destroyadds the new one.outputs.tf:18only usesgithub_repoas the skip toggle, still consistent. - Deploy-on-merge gate (CRITICAL). Nothing applies
ci-cd-permissionson merge or push. The only references to the path in.githubareci.yml:620(new step,init -backend=false+terraform test, mock provider, no credentials) and comments or an error message (deploy-azure.yml:328,cleanup-staging.yml,rollback.yml:27). Everyterraform plan/applyruns in a different directory:deploy-gcp.yml:188,195,206(terraform/environments/gcp),deploy-aws-lambda.yml:268-325anddeploy-aws-fargate.yml:192-225(.../aws),deploy-azure.yml:209,221,277(.../azure),rollback.yml:278-552(aws/gcp/azure). Terraform does not recurse into subdirectories, and the gcp env root has no module source pointing atci-cd-permissions(module sources are../../modules/...).scripts/*.shapply only in other contexts (tf-deploy.sh, test scripts), none invoked by a workflow on merge for this path. Merging changes no live trust; the owner's manual bootstrap apply is still required. - Local runs (Terraform 1.10.5, no credentials,
GOOGLE_APPLICATION_CREDENTIALS=/dev/null, mock provider only, no plan/apply):terraform fmt -check -recursiveOK,validateOK,terraform test13 passed, 0 failed.actionlintonci.ymlandcleanup-staging.ymlexit 0. - Fail-before. Ran the PR's test file against a copy of origin/main's module:
trust_uses_immutable_idsfails (invalid index on the missing mappings, assertion failures), the remaining runs are skipped. Reproduces the author's claim.
Gate status (origin/main CLAUDE.md "Review gate")
- Independent adversarial review naming the SHA: this comment.
- CI on 5b1ad3d:
gh pr checks 1066all pass, including Validate Terraform (gcp), Lint Workflows, pre-commit, Security Scanning, Snyk, Trivy. CodeRabbit status is "rate limited" (optional; not pinged). - No unresolved actionable review threads; the PR has no human review comments.
- Local verification on macOS: above. Gap: the real IAM replacement ordering (
create_before_destroy) and live token minting cannot be exercised without a real project; the PR's apply runbook (plan must show "create replacement and then destroy", then verify a main deploy) covers that and stays an owner step. - Labels: no closing issue ("Part of #2"). PR labels (priority/p1, severity/high, urgency/this-sprint, impact/internal, effort/m, type/security, triaged) match issue #2 exactly.
- Scope: AWS and Azure are explicitly left to the follow-up PR; the CI step is a no-op for them until tests exist.
Live GCP trust is still name-based (assertion.repository == name), so a renamed or re-created repo under the same name could match. Key the WIF attribute condition and the SA member on the immutable GitHub repository_owner_id and repository_id instead, keeping the ref clause and the attribute.repository mapping. Add github_owner_id and github_repo_id as required, validated variables and validate deploy_ref, which is interpolated into the CEL condition. The member is replaced with create_before_destroy, and the old member keeps resolving through the retained mapping, so there is no access gap. Parity with the already-applied AWS/Azure immutable subjects lands in a separate change. Add a mock-provider terraform test and a CI step running it. Apply is a manual bootstrap step for a privileged human; the runbook is in the PR body (plan 2-oidc-trust-immutable-ids, section 5). Refs #2. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add tftest runs that kill two surviving validation mutants: a loosened repo-id regex (leading zero accepted) and a github_repo validation replaced by a match-all. Add a skip-mode run (github_repo empty creates no WIF resources). Refresh the stale GCP comment in cleanup-staging.yml to the repository_id keying and note in the README that github_repo is still the skip toggle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
5b1ad3d to
ffd7749
Compare
|
The independent review above covers 5b1ad3d. Rebased by claude-cc-pr-merge-3 onto main to resolve a ci.yml conflict with #1023: main's 'Test /health gate modules' step kept, this PR's 'Test CI/CD trust (immutable ids)' step unchanged after it; actionlint clean. The PR's own commits are unchanged, so the verdict carries to ffd7749 (owner #631 rule). |
Summary
Part of #2 (P1 security). This PR covers GCP and adds the CI test step. AWS and Azure follow in a second PR that completes #2.
The live CI/CD deploy trust was re-pointed on 2026-09-29. GitHub's repository setting already emits immutable OIDC subjects (
use_immutable_subject: true,sub_claim_prefix: repo:LeanerCloud@86753534/cloud-commitments-platform@1391259743). Theci-cd-permissionsTerraform in this repo, however, still keys trust on the repository name (var.github_repo). The next bootstrap re-apply would therefore bring back name-based trust, which a rename or a reused repository name can silently break or hijack.GCP (
terraform/environments/gcp/ci-cd-permissions)github_owner_idandgithub_repo_id, validated with^[1-9][0-9]*$. The real values (86753534, 1391259743) are interraform.tfvars.example.attribute.repository_idandattribute.repository_owner_idmappings. The existingattribute.repositorymapping stays, so the old binding keeps resolving during apply.attribute_conditionbecomesassertion.repository_owner_id == '86753534' && assertion.repository_id == '1391259743' && assertion.ref == '<deploy_ref>'.principalSet://…/attribute.repository_id/1391259743, withlifecycle { create_before_destroy = true }. The new binding is added before the old one is removed, so deploys keep access throughout.deploy_refis validated as^refs/(heads|tags)/[A-Za-z0-9._/-]+$, because it is interpolated into CEL.github_repois validated asowner/nameor"". It is still the skip toggle, but it no longer feeds the condition or the member. The CLAUDE.md note about pinningdeploy_refstill applies.CI
One step, "Test CI/CD trust (immutable ids)", in the existing
terraform-validatejob. For eachmatrix.cloudit runsterraform init -backend=falseandterraform testonterraform/environments/<cloud>/ci-cd-permissions, but only when atests/directory exists. It is a no-op for AWS and Azure until the second PR adds their tests, so that PR needs no CI change.The
cleanup-staging.ymlchange is a refreshed comment only.Tests
tests/immutable_trust.tftest.hcl(mock provider, 13 runs):repository_id;deploy_ref;deploy_refvaluesx' || trueand""rejected;github_repovaluesa/b'candnoslashrejected;github_repo="") creates 0 pool, provider and member resources.Fail-before: on origin/main the first run fails (the member was
…/attribute.repository/LeanerCloud/cloud-commitments-platform, and the new mappings were absent). After the change, 13/13 pass.Mutants killed:
attribute.repository;github_repovalidation with.*.create_before_destroycannot be asserted by a test. See the apply runbook.Verification (local, macOS)
terraform fmt -check -recursive terraform/,validateandtestare clean on Terraform 1.10.5, the CI version.Apply runbook (bootstrap: a privileged human, per CLAUDE.md)
gh api repos/LeanerCloud/cloud-commitments-platform/actions/oidc/customization/substill showsuse_immutable_subject: true.terraform/environments/gcp/ci-cd-permissions, set the IDs in the gitignored tfvars, runterraform plan, and confirm:mainauthenticates to GCP.Review
plans/2-oidc-trust-immutable-ids.md, independently reviewed in 2 passes, revisions RV1-RV6.🤖 Generated with Claude Code