sec(ci): destroy and rollback workflows have no environment binding, so no reviewer gate applies #1591

Activity

1. cristim

   added

   triagedItem has been triagedItem has been triaged

   priority/p0Drop everything; same-day fixDrop everything; same-day fix

   severity/highSignificant harmSignificant harm

   urgency/nowDrop other thingsDrop other things

   impact/internalTeam-internal onlyTeam-internal only

   effort/mDaysDays

   type/securitySecurity findingSecurity finding

   on Jul 28, 2026
2. cristim commented on Jul 28, 2026

   

   cristim

   on Jul 28, 2026

   MemberAuthor

   More actions

   Partial resolution for the rollback.yml portion: PR #1641 (fixing #1542) drops id-token: write from workflow level to job level, granting it only to the four environment-bound rollback-* jobs. It also deletes the verify-image job, which authenticated to AWS/GCP/Azure with no environment: binding and was the workflow's real gate bypass.

   So validate (:45) and summary (:381) still have no environment: binding, but they now hold only contents: read — with no id-token: write there is no OIDC token for them to mint, so the missing binding on those two is no longer a privilege issue.

   Untouched by #1641 and still open here: cleanup-staging.yml:47,117,187,252, destroy-fargate-dev.yml:38, and the repo:<org/repo>:ref:refs/heads/main subject in role.tf:28-33 (which cannot be removed until those jobs get environment bindings — #1641's PR body documents the dependency).

   Related new finding: the <cloud>-<env>-rollback environment names are not in the trust policy's sub allowlist at all, filed as LeanerCloud/cloud-commitments-platform#139.
3. cristim

   added a commit that references this issue on Jul 29, 2026

   fix(ci): correct the destroy workflows' claims about what the ref che…

   ...

   c39adc3
4. cristim commented on Jul 29, 2026

   

   cristim

   on Jul 29, 2026

   MemberAuthor

   More actions

   PR #1674 adds the environment binding this issue asks for. It does not add a reviewer gate, and this issue closing should not be read as "destroy is now gated."

   Stating that plainly because the title's framing invites the wrong conclusion. environment: buys OIDC/secret scoping and nothing more until protection rules exist. Re-confirmed against the live API today:

   $ gh api repos/LeanerCloud/CUDly/environments
   total_count: 5
   aws-fargate-dev: protection_rules=[] | aws-fargate-staging: [] | azure-: [] | dev: [] | gcp-: []
   

   No environment in this repo has any protection rules, and staging does not exist at all. GitHub auto-creates a referenced environment bare on first use, so the first cleanup-staging dispatch mints staging with no reviewers and runs straight through. A binding alone, on these workflows, is exactly the reassuring-looking control that is not one.

   Which half is code and which is manual

   Code (PR #1674, done): environment: staging on the four cleanup-staging destroy jobs and environment: dev on destroy-fargate-dev; id-token: write dropped from workflow level to only the five jobs that assume a cloud role; permissions: {} on both confirmation guards; per-environment Azure federated credentials; the last ${{ }} removed from a run: block.

   Manual — a privileged human, in repo settings or via the API. None of it is code:

   1. Create the staging environment. dev exists. Not declarable in this repo today: there is no github_repository_environment resource anywhere under terraform/ or iac/, and no GitHub Terraform provider is configured. Adding that provider belongs to sec(ci): deployment environments have no protection rules, so environment-bound credentialed jobs run unapproved cloud-commitments-platform#141, not to sec(ci): bind destroy jobs to an environment and narrow id-token on destroy workflows #1674.
   2. Required reviewers on dev and staging — the actual gate. Without this, nothing in sec(ci): bind destroy jobs to an environment and narrow id-token on destroy workflows #1674 stops anything.
   3. Deployment branch policy — custom_branch_policies with pattern main. Load-bearing, see below.
   4. Wait timer on staging if a cancellation window is wanted.
   5. can_admins_bypass = false, at least on anything production-facing.
   6. Re-apply terraform/environments/azure/ci-cd-permissions (bootstrap-only per CLAUDE.md). Until this runs, the Azure destroy job fails with AADSTS70021 — pre-existing per fix(iac/aws): rollback environments are absent from the OIDC trust allowlist, so AWS/Azure rollback cannot authenticate cloud-commitments-platform#139, and sec(ci): bind destroy jobs to an environment and narrow id-token on destroy workflows #1674 knowingly merges it in that state.

   Item 3 is not optional

   An OIDC sub is either …:ref:refs/heads/<branch> or …:environment:<name>, never both. So the binding replaces the ref-scoped subject with a ref-agnostic one, and since neither environment carries a branch policy, the binding by itself trades main-only for any-branch — a widening, on the workflows this exists to narrow.

   #1674 adds a Restrict to main step, but it is defense-in-depth against accidents and not a security boundary: 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> subject. Only the deployment branch policy survives that, because GitHub evaluates it before the job starts and before the token is minted. It does not require main to be a protected branch — custom_branch_policies takes an explicit pattern and works on an unprotected one.

   Scope correction

   rollback.yml is named in this issue but needs no change: PR #1641 already bound it. Workflow level is contents: read, id-token: write sits only on the four environment-bound rollback-* jobs, and validate/summary are contents: read. Verified against current main. Its remaining gap is the missing protection rules, which is LeanerCloud/cloud-commitments-platform#141.

   So after #1674 merges, the residue of this issue is entirely items 1-6 above. Cross-referencing rather than restating: #1660 owns environments-have-no-protection-rules and main-is-unprotected; #1648 owns the trust-policy sub allowlist gaps; #1659 owns the sibling deploy workflows. Filing nothing new — this family already has three overlapping issues.

   Suggest this issue stays open until at least items 1-3 are done, or is closed with an explicit note that it covered the binding only and the gate lives in LeanerCloud/cloud-commitments-platform#141.
5. cristim

   closed this as completedin #1674on Aug 3, 2026
6. cristim

   added a commit that references this issue on Aug 3, 2026

   sec(ci): bind destroy jobs to an environment and narrow id-token on d…

   ...

   f6bb36a