diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 68e10999e..b885e287e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -624,6 +624,16 @@ jobs: terraform -chdir="$d" test done + - name: Test CI/CD trust (immutable ids) + # Skips a cloud until it has a tests dir. The lock file is gitignored, + # so no -lockfile=readonly. + run: | + d=terraform/environments/${{ matrix.cloud }}/ci-cd-permissions + if [ -d "$d/tests" ]; then + terraform -chdir="$d" init -backend=false -input=false + terraform -chdir="$d" test + fi + - name: Terraform Init run: | cd terraform/environments/${{ matrix.cloud }} diff --git a/.github/workflows/cleanup-staging.yml b/.github/workflows/cleanup-staging.yml index 8d8a57cfa..cabff1944 100644 --- a/.github/workflows/cleanup-staging.yml +++ b/.github/workflows/cleanup-staging.yml @@ -347,9 +347,10 @@ jobs: 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 + # GCP: the WIF provider keys on assertion.repository_owner_id, + # assertion.repository_id and assertion.ref (gcp/ci-cd-permissions/ + # github_oidc.tf), and the impersonation binding is on + # attribute.repository_id -- 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. diff --git a/terraform/environments/gcp/ci-cd-permissions/README.md b/terraform/environments/gcp/ci-cd-permissions/README.md index 2a07afaf0..81e16c36d 100644 --- a/terraform/environments/gcp/ci-cd-permissions/README.md +++ b/terraform/environments/gcp/ci-cd-permissions/README.md @@ -119,13 +119,14 @@ Workload Identity Federation endpoint for a short-lived SA access token, and set ### Attribute condition -The WIF provider enforces **two** conditions simultaneously: +The WIF provider enforces **three** conditions simultaneously: ```text -assertion.repository == '' && assertion.ref == '' +assertion.repository_owner_id == '' && assertion.repository_id == '' && assertion.ref == '' ``` -Only workflows running from the exact repository **and** the exact ref (branch) configured in +Only workflows running from the exact repository (matched by immutable numeric ids, so a rename +or delete-and-recreate under the same name does not match) **and** the exact ref (branch) configured in `terraform.tfvars` can obtain a GCP token. Any other branch, PR, fork, tag, or `workflow_dispatch` from a different ref is rejected with: @@ -148,4 +149,12 @@ gcloud iam workload-identity-pools providers describe github-actions \ --format="value(attributeCondition)" ``` -To change the allowed repo, update `github_repo` in `terraform.tfvars` and re-apply. +To change the allowed repo, update `github_owner_id` and `github_repo_id` in `terraform.tfvars` +(`gh api repos// --jq '.owner.id, .id'`) and re-apply. The `principalSet` member is +keyed on `attribute.repository_id`; the `attribute.repository` mapping is kept so the old member +keeps resolving while the replacement is created first. `github_repo` is still the skip toggle +(empty disables WIF setup) but no longer feeds the condition or the member. + +AWS and Azure match the GitHub immutable OIDC subject (`repo:@/@:...`), +which depends on the repository setting `use_immutable_subject` (Settings, Actions, OIDC). Check it with +`gh api repos///actions/oidc/customization/sub`. GCP does not use `sub`, so it is unaffected. diff --git a/terraform/environments/gcp/ci-cd-permissions/github_oidc.tf b/terraform/environments/gcp/ci-cd-permissions/github_oidc.tf index 94aecf876..00eef32e6 100644 --- a/terraform/environments/gcp/ci-cd-permissions/github_oidc.tf +++ b/terraform/environments/gcp/ci-cd-permissions/github_oidc.tf @@ -25,16 +25,21 @@ resource "google_iam_workload_identity_pool_provider" "github" { "attribute.actor" = "assertion.actor" "attribute.repository" = "assertion.repository" "attribute.ref" = "assertion.ref" + + # Immutable ids: a renamed or deleted-and-recreated repo gets a new id, so + # a name squatter cannot match. + "attribute.repository_id" = "assertion.repository_id" + "attribute.repository_owner_id" = "assertion.repository_owner_id" } - # Restrict to the specific repo AND only allow the designated deploy branch. + # Restrict to the specific repo (by immutable ids, not name) AND only allow the designated deploy branch. # PRs, tags, forks, and workflow_dispatch from other refs are blocked. # # deploy_ref defaults to refs/heads/main. Override in terraform.tfvars only # when temporarily deploying from a feature branch; flip back to main once # that branch merges. Forgetting to flip locks out main-branch deploys with # "unauthorized_client: The given credential is rejected by the attribute condition." - attribute_condition = "assertion.repository == '${var.github_repo}' && assertion.ref == '${var.deploy_ref}'" + attribute_condition = "assertion.repository_owner_id == '${var.github_owner_id}' && assertion.repository_id == '${var.github_repo_id}' && assertion.ref == '${var.deploy_ref}'" } # Allow workflows from the permitted repo+ref to impersonate the deploy SA. @@ -43,5 +48,11 @@ resource "google_service_account_iam_member" "github_actions" { service_account_id = google_service_account.cudly_deploy.name role = "roles/iam.workloadIdentityUser" - member = "principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.github[0].name}/attribute.repository/${var.github_repo}" + member = "principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.github[0].name}/attribute.repository_id/${var.github_repo_id}" + + # The old member (keyed on attribute.repository) keeps resolving because that + # mapping stays, so the new binding is created first and no access gap opens. + lifecycle { + create_before_destroy = true + } } diff --git a/terraform/environments/gcp/ci-cd-permissions/terraform.tfvars.example b/terraform/environments/gcp/ci-cd-permissions/terraform.tfvars.example index e650fd979..46a923946 100644 --- a/terraform/environments/gcp/ci-cd-permissions/terraform.tfvars.example +++ b/terraform/environments/gcp/ci-cd-permissions/terraform.tfvars.example @@ -31,6 +31,15 @@ project_id = "your-gcp-project-id" # github_repo = "LeanerCloud/cloud-commitments-platform" +# ─── github_owner_id / github_repo_id ─────────────────────────────────────────── +# Immutable numeric ids. The WIF condition and the SA binding match on these, so +# a renamed or re-created repository (same name, new id) cannot impersonate the +# deploy SA. Required; look them up with: +# gh api repos/LeanerCloud/cloud-commitments-platform --jq '.owner.id, .id' +# +github_owner_id = "86753534" +github_repo_id = "1391259743" + # ─── deploy_ref ──────────────────────────────────────────────────────────────── # Git ref whose workflows are permitted to impersonate the deploy SA. # Only exact-match pushes to this branch can obtain a GCP token. diff --git a/terraform/environments/gcp/ci-cd-permissions/tests/immutable_trust.tftest.hcl b/terraform/environments/gcp/ci-cd-permissions/tests/immutable_trust.tftest.hcl new file mode 100644 index 000000000..d80072a90 --- /dev/null +++ b/terraform/environments/gcp/ci-cd-permissions/tests/immutable_trust.tftest.hcl @@ -0,0 +1,167 @@ +mock_provider "google" { + mock_resource "google_service_account" { + defaults = { + name = "projects/p/serviceAccounts/cudly-terraform-deploy@p.iam.gserviceaccount.com" + } + } +} + +variables { + project_id = "p" + github_owner_id = "86753534" + github_repo_id = "1391259743" +} + +run "trust_uses_immutable_ids" { + command = apply + + assert { + condition = google_iam_workload_identity_pool_provider.github[0].attribute_condition == "assertion.repository_owner_id == '86753534' && assertion.repository_id == '1391259743' && assertion.ref == 'refs/heads/main'" + error_message = "attribute_condition must be exactly owner id, repo id and ref" + } + + assert { + condition = !strcontains(google_iam_workload_identity_pool_provider.github[0].attribute_condition, "assertion.repository ==") + error_message = "name-only repository clause must not remain" + } + + assert { + condition = ( + google_iam_workload_identity_pool_provider.github[0].attribute_mapping["attribute.repository_id"] == "assertion.repository_id" + && google_iam_workload_identity_pool_provider.github[0].attribute_mapping["attribute.repository_owner_id"] == "assertion.repository_owner_id" + && google_iam_workload_identity_pool_provider.github[0].attribute_mapping["attribute.repository"] == "assertion.repository" + ) + error_message = "id mappings must exist and the repository mapping must stay" + } + + assert { + condition = endswith(google_service_account_iam_member.github_actions[0].member, "/attribute.repository_id/1391259743") + error_message = "member must be keyed on attribute.repository_id" + } +} + +run "condition_follows_deploy_ref" { + command = apply + + variables { + deploy_ref = "refs/heads/feature/x" + } + + assert { + condition = endswith(google_iam_workload_identity_pool_provider.github[0].attribute_condition, "assertion.ref == 'refs/heads/feature/x'") + error_message = "condition must follow deploy_ref" + } +} + +run "rejects_bad_repo_id" { + command = plan + + variables { + github_repo_id = "abc" + } + + expect_failures = [var.github_repo_id] +} + +run "rejects_empty_repo_id" { + command = plan + + variables { + github_repo_id = "" + } + + expect_failures = [var.github_repo_id] +} + +run "rejects_leading_zero_owner_id" { + command = plan + + variables { + github_owner_id = "0123" + } + + expect_failures = [var.github_owner_id] +} + +run "rejects_non_numeric_owner_id" { + command = plan + + variables { + github_owner_id = "octo" + } + + expect_failures = [var.github_owner_id] +} + +run "rejects_empty_owner_id" { + command = plan + + variables { + github_owner_id = "" + } + + expect_failures = [var.github_owner_id] +} + +run "rejects_cel_injection_in_deploy_ref" { + command = plan + + variables { + deploy_ref = "x' || true" + } + + expect_failures = [var.deploy_ref] +} + +run "rejects_empty_deploy_ref" { + command = plan + + variables { + deploy_ref = "" + } + + expect_failures = [var.deploy_ref] +} + +run "rejects_leading_zero_repo_id" { + command = plan + + variables { + github_repo_id = "0123" + } + + expect_failures = [var.github_repo_id] +} + +run "rejects_github_repo_with_quote" { + command = plan + + variables { + github_repo = "a/b'c" + } + + expect_failures = [var.github_repo] +} + +run "rejects_github_repo_without_slash" { + command = plan + + variables { + github_repo = "noslash" + } + + expect_failures = [var.github_repo] +} + +run "skip_mode_creates_no_wif" { + command = apply + + variables { + github_repo = "" + } + + assert { + condition = length(google_iam_workload_identity_pool.github) == 0 && length(google_iam_workload_identity_pool_provider.github) == 0 && length(google_service_account_iam_member.github_actions) == 0 + error_message = "empty github_repo must disable WIF resources" + } +} diff --git a/terraform/environments/gcp/ci-cd-permissions/variables.tf b/terraform/environments/gcp/ci-cd-permissions/variables.tf index 573965b9e..e570387ea 100644 --- a/terraform/environments/gcp/ci-cd-permissions/variables.tf +++ b/terraform/environments/gcp/ci-cd-permissions/variables.tf @@ -7,10 +7,40 @@ variable "github_repo" { description = "GitHub repository (owner/name) whose Actions workflows may impersonate the deploy SA via Workload Identity Federation (e.g. 'LeanerCloud/cloud-commitments-platform'). Leave empty to skip WIF setup." type = string default = "LeanerCloud/cloud-commitments-platform" + + validation { + condition = var.github_repo == "" || can(regex("^[A-Za-z0-9-]+/[A-Za-z0-9._-]+$", var.github_repo)) + error_message = "github_repo must be empty or owner/name." + } +} + +variable "github_owner_id" { + description = "Immutable numeric id of the GitHub owner (gh api repos/OWNER/REPO --jq .owner.id). Trust is keyed on it, not on the owner name." + type = string + + validation { + condition = can(regex("^[1-9][0-9]*$", var.github_owner_id)) + error_message = "github_owner_id must be a positive integer without leading zeros." + } +} + +variable "github_repo_id" { + description = "Immutable numeric id of the GitHub repository (gh api repos/OWNER/REPO --jq .id). Trust is keyed on it, not on the repo name." + type = string + + validation { + condition = can(regex("^[1-9][0-9]*$", var.github_repo_id)) + error_message = "github_repo_id must be a positive integer without leading zeros." + } } variable "deploy_ref" { description = "Git ref allowed to deploy (e.g. refs/heads/main). Only workflows from this exact ref can impersonate the deploy SA. PRs, tags, and other branches are blocked." type = string default = "refs/heads/main" + + validation { + condition = can(regex("^refs/(heads|tags)/[A-Za-z0-9._/-]+$", var.deploy_ref)) + error_message = "deploy_ref must look like refs/heads/ or refs/tags/ (it is interpolated into the attribute condition)." + } }