Repository navigation
sec(ci): bind destroy jobs to an environment and narrow id-token on destroy workflows - #1674
Conversation
`cleanup-staging.yml` and `destroy-fargate-dev.yml` declared `id-token: write` at workflow level and bound no job to a deployment environment. Five `terraform destroy -auto-approve` jobs across AWS, GCP and Azure therefore ran with production deploy credentials and no environment scoping, guarded only by a typed-string confirmation entered by the same person who dispatched the run. Changes: - Grant `id-token: write` per job, only to the five jobs that assume a cloud role. The two `guard` jobs authenticate to nothing and now hold no permissions at all. - Bind the four staging destroy jobs to the `staging` environment and the Fargate dev destroy job to `dev`, so their OIDC subject is `environment:<name>` rather than a bare branch ref. - Enforce `refs/heads/main` in each `guard` job. An OIDC subject is either `ref:refs/heads/<branch>` or `environment:<name>`, never both, so the binding above REPLACES the ref-scoped subject with a ref-agnostic one. Neither `dev` nor `staging` carries a deployment_branch_policy, so without this check the change would have traded a main-only restriction for any-branch access — a widening, on the workflows it is meant to narrow. The check lives in the workflow rather than in the environment's branch policy because `main` is not a protected branch, so a protected-branches policy would match nothing and block every deploy. - Add per-environment Azure federated identity credentials. Azure allows no wildcards and had only `ref:refs/heads/main` and `pull_request` subjects, so an environment-bound job would have failed AADSTS70021 — the breakage recorded in #1648. Defaults to dev and staging only; `prod` is omitted because no Azure job binds to it. The `environment:` binding is NOT a reviewer gate and the comments say so: no environment in this repo has any protection rules, and `staging` does not exist yet, so the first dispatch auto-creates it bare. That is tracked in #1660 and needs a privileged human, not code. `rollback.yml`, also named in the issue, is deliberately untouched — PR #1641 already removes workflow-level `id-token: write` from it. Verified: actionlint (with shellcheck) exit 0 on both workflows; terraform fmt and validate clean; both new variable validations proven to reject duplicate and empty environment names. Refs #1591
The Cloud SQL cleanup step interpolated `vars.GCP_PROJECT_ID` directly into its shell source, the last such interpolation in either destroy workflow. #1641 established the rule for rollback.yml: no expression interpolation in any `run:` block, route through `env:` and reference a quoted shell variable. `vars.*` is settable only by a repo admin, so this is hardening rather than a live injection, but the rule is worth holding uniformly on a workflow that runs `terraform destroy -auto-approve`. Also fails loud when the variable is unset. Previously an empty project made `gcloud sql instances list` match nothing, so the step silently skipped the deletion it exists to perform and left the destroy to hit the dependency deadlock it was meant to avoid. actionlint: exit 0. Zero run-block interpolations remain in cleanup-staging.yml, destroy-fargate-dev.yml and rollback.yml. Refs #1591
…ck binds Adversarial review found the new comments overstating two controls, which is the same defect class #1591 is about: a control that reads as a restriction and does not restrict. 1. The "Restrict to main" step was described as enforcing main-only dispatch. It does not. `workflow_dispatch` runs the workflow file as it exists on the dispatched ref, so anyone who can push a branch can delete the step and still present the `environment:<name>` OIDC subject. Relabelled as defense-in-depth against accidents, explicitly not a security boundary. 2. The comments justified checking the ref in-workflow on the grounds that a deployment branch policy needs `main` to be a protected branch. That is wrong. Environments support `custom_branch_policies` with an explicit branch pattern, which works on an unprotected branch and is evaluated before the job starts and before the token is minted. Naming the wrong reason steered the reader away from the only control that actually holds. Also: - github_environments no longer reads as a complete enumeration. It now names the Azure jobs it knowingly does not cover (rollback.yml's azure-<env>-rollback, database-migration.yml's azure-db-<env>) and says why, cross-referencing #1648, so absence is not misread as "no job uses it". - Live environment state (which environments exist, which have protection rules) is no longer asserted in code comments, only referenced via #1660, so it cannot rot into false reassurance once that issue is fixed. - GCP_PROJECT_ID validated for shape rather than mere non-emptiness; a whitespace-only value previously reached `gcloud --project=`, matched no instance and silently skipped the deletion. - AWS_REGION moved to job level in destroy-fargate-dev.yml. Workflow-level env is resolved before the job's environment is in scope, so an environment-scoped override would have been invisible. The OIDC subject claim itself was independently verified and holds: `sub` is either ref-scoped or environment-scoped, never both, absent a custom sub-claim template, and this repo sets none. Refs #1591
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 57 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 (5)
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 57 minutes. |
… cudly_deploy (#1683) * fix(iac/aws): extend OIDC trust allowlist to cover every job assuming cudly_deploy The AWS deploy role's trust policy sub allowlist (role.tf) covered only ref:refs/heads/main and the three bare environment:{dev,staging,prod} subjects. Live AWS still runs the pre-b876d7fa0 wildcard (repo:LeanerCloud/CUDly:*), so that allowlist was never applied. Tightening it as committed would immediately break every job whose environment binding isn't in it. Enumerated every workflow job that assumes this role (role-to-assume: ${{ vars.AWS_ROLE_TO_ASSUME }}, 9 job-steps across 7 files) and traced each environment: binding to its concrete value. Twelve subjects were missing: deploy-aws-fargate.yml (aws-fargate-{dev,staging,prod}), database-migration.yml (aws-db-{dev,staging,prod}), and rollback.yml's two AWS jobs (aws-{lambda,fargate}-{dev,staging,prod}-rollback). pull_request is deliberately not added: the only pull_request-triggered workflow that calls configure-aws-credentials (aws_sanity.yml) assumes a separate read-only role, not this one. database-migration.yml's workflow_call path takes an unconstrained string environment input, but nothing in this repo calls it that way today, so it's excluded and flagged rather than covered with a wildcard. Also corrects ci-cd-permissions/README.md's "Trust policy conditions" section, which still described the wildcard rather than the allowlist role.tf has actually specified since b876d7f. Not applied here, this module is bootstrap-only, applied manually by a privileged human. Refs #1648, #1674. * docs(iac/aws): state the environment-subject branch-scoping gap explicitly The owner constraint on this repo is that only main may deploy. ref:refs/heads/main in the allowlist enforces that for unbound jobs, but environment:<name> subjects are ref-agnostic: a workflow_dispatch from any branch against an environment-bound job presents the identical subject a main run would. Reattaching the branch requirement needs a deployment branch policy per environment, which none of the 15 environments this allowlist names currently has (several don't exist yet), and this module has no GitHub provider to configure that declaratively. Makes this explicit in role.tf's comment and adds a README section naming every environment that needs the policy, so the allowlist isn't read as delivering "only main deploys" before that manual step happens. * docs(iac/aws): distinguish allowlist, branch policy, and reviewers explicitly Team feedback: the previous pass stated that this allowlist does not restrict deploys to main, but didn't name the three separate GitHub-side controls that keep getting conflated across this repo's issues (allowlist membership, deployment branch policy, required reviewers), and didn't say which of the 15 named environments exist yet. Checked live via `gh api repos/LeanerCloud/CUDly/environments`: only 3 of 15 exist today (dev, aws-fargate-dev, aws-fargate-staging), none with any protection rules or branch policy. Adds the three-way distinction to role.tf's top comment and a short existence note next to each entry group, and replaces the README section with the same distinction plus a per-environment existence table.
Adversarial review record (merged)Independent reviewer, distinct from the author. Recording it here because the merge rested on this — this PR's Lead concern tested and cleared: does binding jobs to an environment break OIDC? An
Zero Honest gap, stated by the PR: no real destroy was dispatched. Order-independent with #1683: this PR binds those jobs to |
1 participant
Closes #1591
Read this first: this PR does not add a reviewer gate
The issue title says these workflows have "no environment binding, so no reviewer gate". This PR adds the binding. It does not add the reviewer gate, and closing #1591 should not be read as "destroy is now gated".
environment:buys exactly two things: it scopes secrets/vars, and it sets the OIDC subject torepo:<org/repo>:environment:<name>. GitHub only blocks a job once protection rules are configured on the environment, and re-confirmed against the live API on this branch:No environment in this repo has any protection rules, and
stagingdoes not exist at all. GitHub auto-creates a referenced environment bare on first use, so the firstcleanup-stagingdispatch will mintstagingwith no reviewers and run straight through. Until someone configures rules out-of-band, these destroy jobs still run unapproved.The code / manual split, explicitly
id-token: write; per-env Azure federated credentialsstaging; set required reviewers ondevandstaging; set a deployment branch policy; considercan_admins_bypass=falseci-cd-permissionsbootstrap moduleExact manual steps, on environments
devandstaging:staging(devalready exists). Neither can be created declaratively today: there is nogithub_repository_environmentresource anywhere underterraform/oriac/, and no GitHub Terraform provider is configured in this repo. Adding that provider is sec(ci): deployment environments have no protection rules, so environment-bound credentialed jobs run unapproved cloud-commitments-platform#141's scope, not this PR's.custom_branch_policieswith patternmain. See below for why this one is load-bearing.terraform/environments/azure/ci-cd-permissionsso the new per-environment federated credentials exist. Until this runs, the Azure destroy job fails withAADSTS70021— it is knowingly merged broken, matching the pre-existing state recorded in fix(iac/aws): rollback environments are absent from the OIDC trust allowlist, so AWS/Azure rollback cannot authenticate cloud-commitments-platform#139.Cross-referenced, not duplicated: #1660 owns environments-have-no-protection-rules and
main-is-unprotected. #1648 owns the trust-policysuballowlist gaps. #1659 owns the sibling deploy workflows. Nothing new filed; this family already has enough overlapping issues.The binding is a widening unless the branch is pinned
An OIDC
subis either…:ref:refs/heads/<branch>or…:environment:<name>— never both (absent a custom sub-claim template; this repo sets none). So addingenvironment:replaces the ref-scoped subject with a ref-agnostic one. Since neitherdevnorstagingcarries a deployment branch policy, the binding on its own trades a main-only restriction for any-branch access — a widening, on the workflows this change exists to narrow.There is a
Restrict to mainstep in eachguardjob for this. It is defense-in-depth against accidents and is not a security boundary, and the comments say so in those words.workflow_dispatchruns the workflow file as it exists on the dispatched ref, so anyone who can push a branch can delete the step and still present theenvironment:<name>subject. The control that survives that is the deployment branch policy in step 3 above, which GitHub evaluates before the job starts and before the token is minted — and which does not requiremainto be protected.An earlier revision of this branch got that reason wrong, claiming a branch policy was unavailable because
mainis unprotected. It was corrected in c39adc3;custom_branch_policiestakes an explicit pattern and works on an unprotected branch.Also in scope, since I was in these files
permissions:— both workflows declaredpermissions: id-token: writeat workflow level, handing an OIDC-mintable token to every job including the confirmation guard. Nowcontents: readat workflow level,id-token: writegranted per job only to the five jobs that assume a cloud role, andpermissions: {}on bothguardjobs (neither checks out code). Same split #1657 applied todeploy-aws-lambda.yml. This is the destroy-workflow analogue of #1665.Residual
${{ }}inrun:blocks — #1641's rule is zero.destroy-fargate-dev.ymlwas already clean.cleanup-staging.ymlhad one,vars.GCP_PROJECT_IDinterpolated into the Cloud SQL cleanup step; routed throughenv:and referenced as a quoted shell variable. Admin-settable, so hardening rather than a live injection. Both files are now at zero, as isrollback.yml.workflow_dispatchinput validation — the only input on either workflow isconfirm, already compared against the literaldestroyviaenv:. No input reaches anything credentialed. Note the guard is in a preceding job in the same run, typed by the same person who dispatched — it is a typo-catcher, not an authorization control, which is the substance of #1591.rollback.ymlis untouched, deliberately. #1591 names it, but PR #1641 already fixed it: workflow level iscontents: read,id-token: writesits only on the four environment-boundrollback-*jobs, andvalidate/summaryarecontents: read. Verified on currentmain, not assumed. Its remaining gap is the missing protection rules — LeanerCloud/cloud-commitments-platform#141 — not a missing binding.Verification
I did not dispatch a real destroy, so no end-to-end verification is claimed. Neither workflow can be exercised without deleting a live estate. What was actually run, on
c39adc3:actionlint—cleanup-staging.ymlactionlint—destroy-fargate-dev.ymlactionlint—rollback.yml(untouched, regression check)yaml.safe_load)${{ }}inside anyrun:blockid-token: writeand noenvironment:needs: guardpre-commit(incl. terraform fmt/validate/tflint, trivy)Structural checks were done by parsing the YAML rather than grepping, so a job that inherits
id-tokencannot hide from them.Independent adversarial review of the branch confirmed the OIDC subject claim, confirmed the
needs:graph has no bypass, confirmedpermissions: {}does not break either guard (neither checks out code), and confirmedrole.tf:30-31allowlistsenvironment:devandenvironment:staging. Its two high findings were both about my own comments overstating the ref check; both are fixed in c39adc3.Known-broken on merge
The Azure destroy job in
cleanup-staging.ymlcannot authenticate until the bootstrap re-apply in manual step 4. This is pre-existing (#1648) and is flagged in the workflow itself, but it is a checklist item, not just a comment: do not treat Azure staging cleanup as working until that apply has run.