From 19deb296dee7330061533234cd7b9fa8b9d2869d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Jul 2026 22:12:50 +0200 Subject: [PATCH 1/3] fix(ci): bind destroy jobs to an environment and drop blanket id-token MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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:` rather than a bare branch ref. - Enforce `refs/heads/main` in each `guard` job. An OIDC subject is either `ref:refs/heads/` or `environment:`, 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 LeanerCloud/CUDly#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 --- .github/workflows/cleanup-staging.yml | 74 ++++++++++++++++++- .github/workflows/destroy-fargate-dev.yml | 41 +++++++++- .../azure/ci-cd-permissions/README.md | 19 ++++- .../azure/ci-cd-permissions/sp.tf | 29 +++++++- .../azure/ci-cd-permissions/variables.tf | 30 ++++++++ 5 files changed, 186 insertions(+), 7 deletions(-) diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index 1b43786ad..186c46c10 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -21,8 +21,24 @@ on: description: 'Type "destroy" to confirm destruction of ALL staging resources' required: true +# Least privilege: `id-token: write` is granted per job, only to the four jobs +# that assume a cloud deploy role, and each of those is bound to the `staging` +# deployment environment. `guard` authenticates to nothing and must not hold it. +# +# IMPORTANT — what the `environment:` binding does and does not buy. It scopes +# secrets/vars and sets the OIDC subject to `repo::environment:staging`. +# What each cloud does with that subject differs; see the per-job comments. +# +# It is NOT a reviewer gate. GitHub only blocks a job once required-reviewer +# protection rules are configured on the environment in repo settings, and as of +# this change NO environment in this repo has any. `staging` does not exist at +# all yet, so the first dispatch will AUTO-CREATE it bare — no reviewers, no +# branch policy. Until that is configured out-of-band these destroy jobs still +# run unapproved. See #1660. +# +# Because the environment subject is ref-agnostic, the `guard` job below +# separately enforces that this workflow may only be dispatched from `main`. permissions: - id-token: write contents: read env: @@ -33,7 +49,29 @@ jobs: guard: name: Confirm Destruction runs-on: ubuntu-latest + permissions: {} steps: + # An OIDC `sub` is EITHER `…:ref:refs/heads/` OR + # `…:environment:` -- never both. So binding the destroy jobs to an + # environment (below) REPLACES the ref-scoped subject with a ref-agnostic + # one, and neither `dev` nor `staging` carries a deployment_branch_policy. + # Without this check that would trade a main-only restriction for + # any-branch access -- a widening, on the workflow this change exists to + # narrow. + # + # Enforced here rather than via the environment's branch policy because + # `main` is not a protected branch, so a protected-branches policy would + # match nothing and block every deploy. Keep this check until `main` is + # protected AND the environments carry an explicit branch policy (#1660). + - name: Restrict to main + env: + REF: ${{ github.ref }} + run: | + if [ "$REF" != "refs/heads/main" ]; then + echo "::error::Refusing to destroy from '$REF'; this workflow may only be dispatched from refs/heads/main" + exit 1 + fi + - name: Check confirmation env: CONFIRM: ${{ inputs.confirm }} @@ -48,6 +86,13 @@ jobs: name: Destroy AWS Lambda (staging) runs-on: ubuntu-24.04-arm needs: guard + permissions: + id-token: write + contents: read + # AWS: role.tf's sub allowlist includes `environment:staging`, so this is + # the subject the trust policy matches. Not a reviewer gate until + # protection rules exist -- see the note at the top of this file. + environment: staging env: AWS_REGION: ${{ vars.AWS_REGION || 'us-east-1' }} @@ -118,6 +163,13 @@ jobs: name: Destroy AWS Fargate (staging) runs-on: ubuntu-24.04-arm needs: guard + permissions: + id-token: write + contents: read + # AWS: role.tf's sub allowlist includes `environment:staging`, so this is + # the subject the trust policy matches. Not a reviewer gate until + # protection rules exist -- see the note at the top of this file. + environment: staging env: AWS_REGION: ${{ vars.AWS_REGION || 'us-east-1' }} @@ -188,6 +240,16 @@ jobs: name: Destroy Azure (staging) runs-on: ubuntu-latest needs: guard + permissions: + id-token: write + contents: read + # Azure: REQUIRES the bootstrap module + # terraform/environments/azure/ci-cd-permissions to have been re-applied + # with `github_environments` including "staging". Until that manual apply + # happens this job fails with AADSTS70021, because the only federated + # credentials that exist are ref:refs/heads/main and pull_request (#1648). + # Not a reviewer gate either -- see the note at the top of this file. + environment: staging env: ARM_USE_OIDC: "true" ARM_CLIENT_ID: ${{ secrets.AZURE_CLIENT_ID }} @@ -253,6 +315,16 @@ jobs: name: Destroy GCP (staging) runs-on: ubuntu-latest needs: guard + permissions: + id-token: write + contents: read + # GCP: the WIF provider keys on assertion.repository and assertion.ref + # only (gcp/ci-cd-permissions/github_oidc.tf:37), and the impersonation + # binding is on attribute.repository -- nothing reads the subject. So this + # binding does NOT affect whether the token mints; it is here for parity + # and so protection rules can apply once they exist. GCP therefore stays + # main-only via assertion.ref regardless. + environment: staging env: GCP_REGION: ${{ vars.GCP_REGION || 'us-central1' }} diff --git a/.github/workflows/destroy-fargate-dev.yml b/.github/workflows/destroy-fargate-dev.yml index 58581a40a..85905b20b 100644 --- a/.github/workflows/destroy-fargate-dev.yml +++ b/.github/workflows/destroy-fargate-dev.yml @@ -12,8 +12,23 @@ on: description: 'Type "destroy" to confirm' required: true +# Least privilege: `id-token: write` is granted per job, only to the job that +# assumes the AWS deploy role, and that job is bound to the `dev` deployment +# environment. `guard` authenticates to nothing and must not hold it. +# +# IMPORTANT — what the `environment:` binding does and does not buy. It scopes +# secrets/vars and sets the OIDC subject to `repo::environment:dev`, +# which the AWS trust policy's sub allowlist accepts (role.tf:30). +# +# It is NOT a reviewer gate. GitHub only blocks a job once required-reviewer +# protection rules are configured on the environment in repo settings, and as of +# this change NO environment in this repo has any — `dev` exists but carries +# `protection_rules: []` and no branch policy. Until that is configured +# out-of-band this destroy job still runs unapproved. See #1660. +# +# Because the environment subject is ref-agnostic, the `guard` job below +# separately enforces that this workflow may only be dispatched from `main`. permissions: - id-token: write contents: read env: @@ -25,7 +40,24 @@ jobs: guard: name: Confirm Destruction runs-on: ubuntu-latest + permissions: {} steps: + # See the note in cleanup-staging.yml: an environment binding replaces the + # ref-scoped OIDC subject with a ref-agnostic one, and `dev` carries no + # deployment_branch_policy, so without this check the binding would widen + # the destroy path from main-only to any-branch. Enforced in code because + # `main` is unprotected, so a protected-branches policy would match + # nothing. Keep until `main` is protected and `dev` has a branch policy + # (#1660). + - name: Restrict to main + env: + REF: ${{ github.ref }} + run: | + if [ "$REF" != "refs/heads/main" ]; then + echo "::error::Refusing to destroy from '$REF'; this workflow may only be dispatched from refs/heads/main" + exit 1 + fi + - name: Check confirmation env: CONFIRM: ${{ inputs.confirm }} @@ -39,6 +71,13 @@ jobs: name: Terraform Destroy (Fargate dev) runs-on: ubuntu-24.04-arm needs: guard + permissions: + id-token: write + contents: read + # Binds the OIDC subject to repo::environment:dev, which the AWS + # trust policy matches on. Not a reviewer gate until protection rules exist + # on this environment -- see the note at the top of this file. + environment: dev steps: - name: Checkout code diff --git a/terraform/environments/azure/ci-cd-permissions/README.md b/terraform/environments/azure/ci-cd-permissions/README.md index 1762cd50b..b057b41c4 100644 --- a/terraform/environments/azure/ci-cd-permissions/README.md +++ b/terraform/environments/azure/ci-cd-permissions/README.md @@ -13,6 +13,7 @@ secrets ever need to be stored. | `azuread_service_principal.cudly_deploy` | Service principal (identity) | | `azuread_application_federated_identity_credential.github_main` | Federated credential for main-branch deployments | | `azuread_application_federated_identity_credential.github_pr` | Federated credential for pull-request plan checks | +| `azuread_application_federated_identity_credential.github_environment` | Federated credential per deployment environment (`var.github_environments`) | | `azurerm_role_definition.cudly_deploy` | Custom role with minimum required permissions | | `azurerm_role_assignment.cudly_deploy` | Assigns the custom role to the SP at subscription scope | | `module.cudly_reservation_role` (`azurerm_role_definition`) | Host-side custom "CUDly Reservation Purchaser" role definition consumed by the runtime container-apps deploy | @@ -159,12 +160,24 @@ the `az` CLI context for all subsequent steps. ### Federated credential subjects -Two subjects are registered: +Azure federated credentials allow no wildcards, so one resource is required per subject: | Credential | Subject | Use case | | --- | --- | --- | | `github-actions-main` | `repo:LeanerCloud/CUDly:ref:refs/heads/main` | Deployments from main | | `github-actions-pr` | `repo:LeanerCloud/CUDly:pull_request` | Plan runs on PRs | +| `github-actions-env-` | `repo:LeanerCloud/CUDly:environment:` | Jobs bound to a deployment environment (one per `var.github_environments`) | -If you need to deploy from a different branch or repo, add additional -`azuread_application_federated_identity_credential` resources to `sp.tf`. +A job declaring `environment: ` presents the **environment** subject, not the +main-branch one, so it cannot authenticate unless `` is in +`var.github_environments`. Omitting this is what makes `azure/login` fail with +`AADSTS70021` on an otherwise-correct workflow (see issue #1648). + +Note the environment subject is **ref-agnostic** — it does not encode the branch. Adding +a name here therefore lets any branch that can reach a job bound to that environment +obtain the deploy service principal. Restrict the branch in the workflow itself, or via +the environment's `deployment_branch_policy`. + +To allow a new deployment environment, add its name to `var.github_environments`. To +deploy from a different branch or repo, add a further +`azuread_application_federated_identity_credential` resource to `sp.tf`. diff --git a/terraform/environments/azure/ci-cd-permissions/sp.tf b/terraform/environments/azure/ci-cd-permissions/sp.tf index aabba937e..524954120 100644 --- a/terraform/environments/azure/ci-cd-permissions/sp.tf +++ b/terraform/environments/azure/ci-cd-permissions/sp.tf @@ -23,8 +23,18 @@ resource "azuread_service_principal" "cudly_deploy" { # GitHub Actions Federated Identity Credentials # ============================================================================= # Azure federated credentials require one entry per allowed subject (no -# wildcards). We create two: one for main-branch deployments and one for -# pull-request plan checks. +# wildcards), so each named deployment environment needs its own resource. +# +# The `github_environment` entries below are what let an environment-bound job +# authenticate at all. A job carrying `environment: staging` presents the +# subject `repo::environment:staging`, NOT the main-branch subject, +# so without a matching credential `azure/login` fails with AADSTS70021 even +# though the workflow is running on main. Adding an `environment:` binding to +# an Azure job and forgetting this is the exact breakage recorded in #1648. +# +# NOTE: this module is bootstrap-only (see CLAUDE.md) — it is applied manually +# by a privileged human, not by the deploy workflow. The Azure destroy job in +# cleanup-staging.yml cannot authenticate until that re-apply happens. resource "azuread_application_federated_identity_credential" "github_main" { count = var.github_repo != "" ? 1 : 0 @@ -47,3 +57,18 @@ resource "azuread_application_federated_identity_credential" "github_pr" { issuer = "https://token.actions.githubusercontent.com" subject = "repo:${var.github_repo}:pull_request" } + +# One credential per named deployment environment, so environment-bound jobs +# can authenticate. Mirrors the `environment:{dev,staging,prod}` subjects the +# AWS role's trust policy already allows +# (terraform/environments/aws/ci-cd-permissions/role.tf). +resource "azuread_application_federated_identity_credential" "github_environment" { + for_each = var.github_repo != "" ? toset(var.github_environments) : toset([]) + + application_id = azuread_application.cudly_deploy.id + display_name = "github-actions-env-${each.value}" + description = "GitHub Actions OIDC — ${var.github_repo} ${each.value} environment" + audiences = ["api://AzureADTokenExchange"] + issuer = "https://token.actions.githubusercontent.com" + subject = "repo:${var.github_repo}:environment:${each.value}" +} diff --git a/terraform/environments/azure/ci-cd-permissions/variables.tf b/terraform/environments/azure/ci-cd-permissions/variables.tf index 2cd9d5066..c4622021d 100644 --- a/terraform/environments/azure/ci-cd-permissions/variables.tf +++ b/terraform/environments/azure/ci-cd-permissions/variables.tf @@ -8,3 +8,33 @@ variable "github_repo" { type = string default = "LeanerCloud/CUDly" } + +variable "github_environments" { + description = <<-EOT + GitHub deployment environment names whose jobs may authenticate via federated + identity credentials. A job carrying `environment: ` presents the OIDC + subject repo::environment:, so a name absent from this list + cannot authenticate (AADSTS70021). + + NOTE the subject is ref-agnostic: it does not encode the branch, so a + credential here lets ANY branch that can reach a job bound to that environment + obtain the deploy service principal. Add a name only once a workflow actually + presents it, and prefer restricting the branch in the workflow or via the + environment's deployment_branch_policy. + + `prod` is deliberately absent: no Azure job binds to it today, and adding it + would mint a credential for a path with no consumer. + EOT + type = list(string) + default = ["dev", "staging"] + + validation { + condition = length(var.github_environments) == length(distinct(var.github_environments)) + error_message = "github_environments must not contain duplicates; toset() would silently collapse them, so a duplicate indicates a typo." + } + + validation { + condition = alltrue([for e in var.github_environments : trimspace(e) != ""]) + error_message = "github_environments entries must be non-empty; an empty name yields the unmatchable subject repo::environment:." + } +} From 04031c4f12060eddcd897e1d40e13bd7015f74a2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:19:09 +0200 Subject: [PATCH 2/3] fix(ci): route GCP project id through env in cleanup-staging destroy 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 --- .github/workflows/cleanup-staging.yml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index 186c46c10..df12262bc 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -354,8 +354,15 @@ jobs: - name: Delete Cloud SQL instance directly (avoids user/DB ordering deadlock) env: TF_VAR_project_id: ${{ vars.GCP_PROJECT_ID }} + GCP_PROJECT_ID: ${{ vars.GCP_PROJECT_ID }} run: | - PROJECT="${{ vars.GCP_PROJECT_ID }}" + # Routed via env: rather than interpolated into this script's source, + # per the zero-expression-interpolation-in-run-blocks rule from #1641. + PROJECT="$GCP_PROJECT_ID" + if [ -z "$PROJECT" ]; then + echo "::error::vars.GCP_PROJECT_ID is unset; refusing to run Cloud SQL cleanup against an empty project" + exit 1 + fi # Find and delete the staging Cloud SQL instance to avoid PostgreSQL dependency errors INSTANCE=$(gcloud sql instances list --project="$PROJECT" \ --filter="name:cudly-staging" --format="value(name)" 2>/dev/null | head -1) From c39adc3097a83538c2a3374dd807d395750c17e1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:27:21 +0200 Subject: [PATCH 3/3] fix(ci): correct the destroy workflows' claims about what the ref check 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:` 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--rollback, database-migration.yml's azure-db-) 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 --- .github/workflows/cleanup-staging.yml | 49 ++++++++++++------- .github/workflows/destroy-fargate-dev.yml | 37 ++++++++------ .../azure/ci-cd-permissions/README.md | 6 ++- .../azure/ci-cd-permissions/variables.tf | 20 ++++++-- 4 files changed, 75 insertions(+), 37 deletions(-) diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index df12262bc..c6ef424f5 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -30,14 +30,22 @@ on: # What each cloud does with that subject differs; see the per-job comments. # # It is NOT a reviewer gate. GitHub only blocks a job once required-reviewer -# protection rules are configured on the environment in repo settings, and as of -# this change NO environment in this repo has any. `staging` does not exist at -# all yet, so the first dispatch will AUTO-CREATE it bare — no reviewers, no -# branch policy. Until that is configured out-of-band these destroy jobs still -# run unapproved. See #1660. +# protection rules are configured on the environment, and at the time of writing +# no environment in this repo has any. Note GitHub AUTO-CREATES a referenced +# environment bare on first use — no reviewers, no branch policy — so a binding +# to an environment nobody has configured runs straight through. Until that is +# configured out-of-band these destroy jobs still run unapproved. See #1660 for +# the live state; deliberately not restated here so this comment cannot rot into +# false reassurance. # -# Because the environment subject is ref-agnostic, the `guard` job below -# separately enforces that this workflow may only be dispatched from `main`. +# The environment subject is ref-agnostic, so the binding does not by itself keep +# this workflow on `main`. The `guard` job below checks the ref, but that check is +# defense-in-depth against ACCIDENTS ONLY and is NOT a security boundary: +# `workflow_dispatch` runs the workflow file as it exists on the dispatched ref, so +# anyone able to push a branch can delete the check and still present the +# `environment:staging` subject. The only control that survives that is a +# deployment branch policy, which GitHub evaluates before the job starts and +# before the token is minted. See #1660. permissions: contents: read @@ -52,17 +60,21 @@ jobs: permissions: {} steps: # An OIDC `sub` is EITHER `…:ref:refs/heads/` OR - # `…:environment:` -- never both. So binding the destroy jobs to an + # `…:environment:` -- never both (absent a custom sub-claim + # template, and this repo sets none). So binding the destroy jobs to an # environment (below) REPLACES the ref-scoped subject with a ref-agnostic # one, and neither `dev` nor `staging` carries a deployment_branch_policy. - # Without this check that would trade a main-only restriction for - # any-branch access -- a widening, on the workflow this change exists to - # narrow. + # Without a ref check that trades a main-only restriction for any-branch + # access -- a widening, on the workflow this change exists to narrow. # - # Enforced here rather than via the environment's branch policy because - # `main` is not a protected branch, so a protected-branches policy would - # match nothing and block every deploy. Keep this check until `main` is - # protected AND the environments carry an explicit branch policy (#1660). + # This step catches the accidental case only. It is 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 this step and + # still present the `environment:staging` subject. The control that + # survives that is a deployment branch policy on the environment + # (custom_branch_policies + a `main` pattern), which GitHub evaluates + # before the job starts and before the token is minted, and which does NOT + # require `main` to be a protected branch. Tracked in #1660. - name: Restrict to main env: REF: ${{ github.ref }} @@ -359,8 +371,11 @@ jobs: # Routed via env: rather than interpolated into this script's source, # per the zero-expression-interpolation-in-run-blocks rule from #1641. PROJECT="$GCP_PROJECT_ID" - if [ -z "$PROJECT" ]; then - echo "::error::vars.GCP_PROJECT_ID is unset; refusing to run Cloud SQL cleanup against an empty project" + # Validate the shape, not merely non-emptiness: a whitespace-only or + # malformed value would otherwise reach `gcloud --project=`, match no + # instance, and silently skip the deletion this step exists to perform. + if ! printf '%s' "$PROJECT" | grep -Eq '^[a-z][a-z0-9-]{5,29}$'; then + echo "::error::vars.GCP_PROJECT_ID is unset or malformed ('$PROJECT'); refusing to run Cloud SQL cleanup" exit 1 fi # Find and delete the staging Cloud SQL instance to avoid PostgreSQL dependency errors diff --git a/.github/workflows/destroy-fargate-dev.yml b/.github/workflows/destroy-fargate-dev.yml index 85905b20b..030e5ee26 100644 --- a/.github/workflows/destroy-fargate-dev.yml +++ b/.github/workflows/destroy-fargate-dev.yml @@ -21,18 +21,23 @@ on: # which the AWS trust policy's sub allowlist accepts (role.tf:30). # # It is NOT a reviewer gate. GitHub only blocks a job once required-reviewer -# protection rules are configured on the environment in repo settings, and as of -# this change NO environment in this repo has any — `dev` exists but carries -# `protection_rules: []` and no branch policy. Until that is configured -# out-of-band this destroy job still runs unapproved. See #1660. +# protection rules are configured on the environment, and at the time of writing +# no environment in this repo has any. Until that is configured out-of-band this +# destroy job still runs unapproved. See #1660 for the live state — deliberately +# not restated here, so this comment cannot rot into false reassurance. # -# Because the environment subject is ref-agnostic, the `guard` job below -# separately enforces that this workflow may only be dispatched from `main`. +# The environment subject is ref-agnostic, so the binding does not by itself keep +# this workflow on `main`. The `guard` job below checks the ref, but that check is +# defense-in-depth against ACCIDENTS ONLY and is NOT a security boundary: +# `workflow_dispatch` runs the workflow file as it exists on the dispatched ref, so +# anyone able to push a branch can delete the check and still present the +# `environment:dev` subject. The only control that survives that is a deployment +# branch policy, which GitHub evaluates before the job starts and before the token +# is minted. See #1660. permissions: contents: read env: - AWS_REGION: ${{ vars.AWS_REGION || 'us-east-1' }} TF_VERSION: '1.10.0' FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true @@ -42,13 +47,12 @@ jobs: runs-on: ubuntu-latest permissions: {} steps: - # See the note in cleanup-staging.yml: an environment binding replaces the - # ref-scoped OIDC subject with a ref-agnostic one, and `dev` carries no - # deployment_branch_policy, so without this check the binding would widen - # the destroy path from main-only to any-branch. Enforced in code because - # `main` is unprotected, so a protected-branches policy would match - # nothing. Keep until `main` is protected and `dev` has a branch policy - # (#1660). + # Catches the accidental "dispatched from the wrong branch" case. It does + # NOT stop a deliberate one: this file is attacker-controlled on the ref + # being dispatched, so the step can simply be deleted on that branch. The + # server-side equivalent is a deployment branch policy on `dev` + # (custom_branch_policies + a `main` pattern) — which does NOT require + # `main` to be a protected branch — tracked in #1660. - name: Restrict to main env: REF: ${{ github.ref }} @@ -78,6 +82,11 @@ jobs: # trust policy matches on. Not a reviewer gate until protection rules exist # on this environment -- see the note at the top of this file. environment: dev + # Set at job level, not workflow level: workflow-level `env:` is resolved + # before this job's `dev` environment is in scope, so an environment-scoped + # AWS_REGION variable would be invisible there. Matches cleanup-staging.yml. + env: + AWS_REGION: ${{ vars.AWS_REGION || 'us-east-1' }} steps: - name: Checkout code diff --git a/terraform/environments/azure/ci-cd-permissions/README.md b/terraform/environments/azure/ci-cd-permissions/README.md index b057b41c4..5c3c93a7a 100644 --- a/terraform/environments/azure/ci-cd-permissions/README.md +++ b/terraform/environments/azure/ci-cd-permissions/README.md @@ -175,8 +175,10 @@ main-branch one, so it cannot authenticate unless `` is in Note the environment subject is **ref-agnostic** — it does not encode the branch. Adding a name here therefore lets any branch that can reach a job bound to that environment -obtain the deploy service principal. Restrict the branch in the workflow itself, or via -the environment's `deployment_branch_policy`. +obtain the deploy service principal. Restrict the branch via the environment's +`deployment_branch_policy`; a ref check inside the workflow does **not** bind, because +`workflow_dispatch` runs the workflow file as it exists on the dispatched ref, so the +check can be removed on that branch. To allow a new deployment environment, add its name to `var.github_environments`. To deploy from a different branch or repo, add a further diff --git a/terraform/environments/azure/ci-cd-permissions/variables.tf b/terraform/environments/azure/ci-cd-permissions/variables.tf index c4622021d..897acb3e4 100644 --- a/terraform/environments/azure/ci-cd-permissions/variables.tf +++ b/terraform/environments/azure/ci-cd-permissions/variables.tf @@ -19,11 +19,23 @@ variable "github_environments" { NOTE the subject is ref-agnostic: it does not encode the branch, so a credential here lets ANY branch that can reach a job bound to that environment obtain the deploy service principal. Add a name only once a workflow actually - presents it, and prefer restricting the branch in the workflow or via the - environment's deployment_branch_policy. + presents it, and restrict the branch via the environment's + deployment_branch_policy — an in-workflow ref check does not bind, because + workflow_dispatch runs the file as it exists on the dispatched ref. - `prod` is deliberately absent: no Azure job binds to it today, and adding it - would mint a credential for a path with no consumer. + This list is NOT a complete enumeration of the environment subjects this repo + presents to Azure. It covers exactly the two destroy jobs bound by + cleanup-staging.yml (`staging`) and destroy-fargate-dev.yml (`dev`). + + Knowingly NOT covered, tracked in #1648 — these Azure jobs bind to compound + environment names and therefore still fail with AADSTS70021: + - rollback.yml -> azure-{dev,staging,prod}-rollback + - database-migration.yml -> azure-db-{dev,staging,prod} + + They are excluded here rather than fixed because each needs its environment + created and gated first; minting a credential for an ungated environment that + does not yet exist would widen access without adding a control. Do not read + the absence of a name as "no job uses it". EOT type = list(string) default = ["dev", "staging"]