From 924446f406f84e7502f664b769176d3aaca9ca90 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 19:40:05 +0200 Subject: [PATCH 1/2] fix(iac): key the GCP deploy trust on immutable repository ids Live GCP trust is still name-based (assertion.repository == name), so a renamed or re-created repo under the same name could match. Key the WIF attribute condition and the SA member on the immutable GitHub repository_owner_id and repository_id instead, keeping the ref clause and the attribute.repository mapping. Add github_owner_id and github_repo_id as required, validated variables and validate deploy_ref, which is interpolated into the CEL condition. The member is replaced with create_before_destroy, and the old member keeps resolving through the retained mapping, so there is no access gap. Parity with the already-applied AWS/Azure immutable subjects lands in a separate change. Add a mock-provider terraform test and a CI step running it. Apply is a manual bootstrap step for a privileged human; the runbook is in the PR body (plan 2-oidc-trust-immutable-ids, section 5). Refs #2. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/ci.yml | 10 ++ .../gcp/ci-cd-permissions/README.md | 16 ++- .../gcp/ci-cd-permissions/github_oidc.tf | 17 ++- .../terraform.tfvars.example | 9 ++ .../tests/immutable_trust.tftest.hcl | 124 ++++++++++++++++++ .../gcp/ci-cd-permissions/variables.tf | 30 +++++ 6 files changed, 199 insertions(+), 7 deletions(-) create mode 100644 terraform/environments/gcp/ci-cd-permissions/tests/immutable_trust.tftest.hcl 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/terraform/environments/gcp/ci-cd-permissions/README.md b/terraform/environments/gcp/ci-cd-permissions/README.md index 2a07afaf0..a4c00ce5c 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,11 @@ 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. + +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..c05088ee8 --- /dev/null +++ b/terraform/environments/gcp/ci-cd-permissions/tests/immutable_trust.tftest.hcl @@ -0,0 +1,124 @@ +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] +} 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)." + } } From ffd7749e590e248e516d04f78282841526cc8e3c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 19:45:45 +0200 Subject: [PATCH 2/2] test(iac): pin the GCP trust input validations Add tftest runs that kill two surviving validation mutants: a loosened repo-id regex (leading zero accepted) and a github_repo validation replaced by a match-all. Add a skip-mode run (github_repo empty creates no WIF resources). Refresh the stale GCP comment in cleanup-staging.yml to the repository_id keying and note in the README that github_repo is still the skip toggle. Co-Authored-By: Claude Opus 5.5 --- .github/workflows/cleanup-staging.yml | 7 +-- .../gcp/ci-cd-permissions/README.md | 3 +- .../tests/immutable_trust.tftest.hcl | 43 +++++++++++++++++++ 3 files changed, 49 insertions(+), 4 deletions(-) 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 a4c00ce5c..81e16c36d 100644 --- a/terraform/environments/gcp/ci-cd-permissions/README.md +++ b/terraform/environments/gcp/ci-cd-permissions/README.md @@ -152,7 +152,8 @@ gcloud iam workload-identity-pools providers describe github-actions \ 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. +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 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 index c05088ee8..d80072a90 100644 --- a/terraform/environments/gcp/ci-cd-permissions/tests/immutable_trust.tftest.hcl +++ b/terraform/environments/gcp/ci-cd-permissions/tests/immutable_trust.tftest.hcl @@ -122,3 +122,46 @@ run "rejects_empty_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" + } +}