diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index 1b43786ad..c6ef424f5 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -21,8 +21,32 @@ 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, 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. +# +# 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: - id-token: write contents: read env: @@ -33,7 +57,33 @@ jobs: guard: name: Confirm Destruction runs-on: ubuntu-latest + permissions: {} steps: + # An OIDC `sub` is EITHER `…:ref:refs/heads/` OR + # `…: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 a ref check that trades a main-only restriction for any-branch + # access -- a widening, on the workflow this change exists to narrow. + # + # 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 }} + 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 +98,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 +175,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 +252,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 +327,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' }} @@ -282,8 +366,18 @@ 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" + # 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 INSTANCE=$(gcloud sql instances list --project="$PROJECT" \ --filter="name:cudly-staging" --format="value(name)" 2>/dev/null | head -1) diff --git a/.github/workflows/destroy-fargate-dev.yml b/.github/workflows/destroy-fargate-dev.yml index 58581a40a..030e5ee26 100644 --- a/.github/workflows/destroy-fargate-dev.yml +++ b/.github/workflows/destroy-fargate-dev.yml @@ -12,12 +12,32 @@ 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, 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. +# +# 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: - id-token: write contents: read env: - AWS_REGION: ${{ vars.AWS_REGION || 'us-east-1' }} TF_VERSION: '1.10.0' FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true @@ -25,7 +45,23 @@ jobs: guard: name: Confirm Destruction runs-on: ubuntu-latest + permissions: {} steps: + # 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 }} + 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 +75,18 @@ 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 + # 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 1762cd50b..5c3c93a7a 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,26 @@ 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 | - -If you need to deploy from a different branch or repo, add additional -`azuread_application_federated_identity_credential` resources to `sp.tf`. +| `github-actions-env-` | `repo:LeanerCloud/CUDly:environment:` | Jobs bound to a deployment environment (one per `var.github_environments`) | + +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 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 +`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..897acb3e4 100644 --- a/terraform/environments/azure/ci-cd-permissions/variables.tf +++ b/terraform/environments/azure/ci-cd-permissions/variables.tf @@ -8,3 +8,45 @@ 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 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. + + 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"] + + 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:." + } +}