sec(iac/gcp): pin WIF trust to the exact role ARN and narrow the SA grant

[cristim]

Closes #1667

Recovered from two interrupted sessions (see "Provenance" at the bottom). Both halves of the fix land here.

The defect

iac/federation/gcp-target/terraform/main.tf mapped the attribute raw and gated on a substring:

"attribute.aws_role" = "assertion.arn"
attribute_condition  = "attribute.aws_role.contains('assumed-role/${var.aws_role_name}/')"

AWS's IAM path grammar is (/)|(/[!-~]+/), so an attacker holding only iam:CreateUser in the trusted account creates a user at path /assumed-role/CUDly-Execution/. The resulting ARN arn:aws:iam::<acct>:user/assumed-role/CUDly-Execution/evil contains the substring the condition looked for, and the pool-wide principalSet://.../* grant admitted every identity in the pool. Result: full impersonation of the CUDly service account and its compute.commitments.create/update authority.

IAM role paths do not work here, because STS strips the path from assumed-role ARNs. It is specifically the IAM user ARN, which retains its path, that defeated the substring test.

This is customer-facing: iac/embed.go embeds federation, and internal/api/handler_federation.go serves federation/gcp-target as the default bundle for target=gcp.

Half 1 — exact match, not substring

Normalise the session ARN down to the role ARN in the mapping, then compare with ==:

locals {
  aws_role_arn = "arn:aws:sts::${var.aws_account_id}:assumed-role/${var.aws_role_name}"
}

"attribute.aws_role" = "assertion.arn.contains('assumed-role') ? assertion.arn.extract('{account_arn}assumed-role/') + 'assumed-role/' + assertion.arn.extract('assumed-role/{role_name}/') : assertion.arn"

attribute_condition = "attribute.aws_role == '${local.aws_role_arn}'"

The mapping expression is byte-identical to the one merged into the shell sibling in #1651 (arm/CUDly-CrossSubscription/setup-gcp-wif.sh:209), so the two customer-facing onboarding paths now produce the same provider. Before this change, that script's pool_wide_grants() scan correctly flagged a Terraform-created grant as legacy and refused to report success.

On "genuinely exact, not another prefix construct" — three substring-for-exact-match defects have surfaced in this codebase, so the replacement is tested behaviourally, not just eyeballed. An IAM user ARN normalises to arn:aws:iam::<acct>:user/assumed-role/<role>, which cannot equal an arn:aws:sts:: role ARN; and == (not startsWith) means CUDly-Execution-Admin does not inherit CUDly-Execution's trust. iac/gcp_target_wif_test.go transcribes the CEL extract() semantics into Go and drives a table covering both:

ARN	admitted
arn:aws:sts::<acct>:assumed-role/CUDly-Execution/cudly-session	yes
arn:aws:sts::<acct>:assumed-role/CUDly-Execution/i-0abc123 (different session)	yes
arn:aws:sts::<acct>:assumed-role/CUDly-Execution-Admin/s (longer sibling role)	no
arn:aws:iam::<acct>:user/assumed-role/CUDly-Execution/evil (the reported bypass)	no
arn:aws:iam::<acct>:user/assumed-role/CUDly-Execution/assumed-role/CUDly-Execution/evil (repeated marker)	no
arn:aws:sts::<acct>:assumed-role/Attacker/assumed-role-CUDly-Execution (crafted session name)	no
arn:aws:sts::210987654321:assumed-role/CUDly-Execution/s (wrong account)	no
arn:aws-us-gov:sts::<acct>:assumed-role/CUDly-Execution/s (partition mismatch — fails closed)	no
arn:aws:iam::<acct>:user/alice, ...:root	no

The bypass test carries a fixture guard that first asserts the exploit ARN does satisfy the old contains() condition — so if the fixture ever stops reproducing the reported bug, the test fails loudly rather than passing vacuously. A separate test pins the CEL expression in main.tf byte-for-byte, so the Go transcription cannot silently drift from what Terraform actually applies, and rejects a re-introduced contains( / startsWith( on attribute.aws_role.

Half 2 — the pool-wide binding is narrowed (it does ship)

-  "principalSet://iam.googleapis.com/${pool.name}/*"
+  "principalSet://iam.googleapis.com/${pool.name}/attribute.aws_role/${local.aws_role_arn}"

Tightening only the condition would have left the blast radius of the next mapping bug intact, so this was treated as required, not optional. The old inline comment claimed exact-match was impossible because session ARNs carry variable session names — that is true of the raw ARN, but the normalised attribute.aws_role strips the session suffix, which is precisely what makes an exact-value principalSet work. The same member form is already merged and shipping in the shell path (setup-gcp-wif.sh:267).

A regression test rejects any wildcard in a principal identifier (principal(Set)?://[^"]*\*), on either branch. The guard first shipped as principalSet?://..., where the ? bound to the preceding t and so never matched a wildcard reintroduced on the OIDC branch; fixed in the follow-up commit on this branch.

Documented caveat added in code: principal identifiers are pool-scoped, not provider-scoped, so any provider added to this pool that can mint the same attribute value satisfies the grant. var.pool_id should stay dedicated to CUDly.

Migration verdict: safe in-place upgrade, no replacement of the pool or provider

Determined from the provider schema, not memory.

attribute_mapping and attribute_condition update IN PLACE. In hashicorp/terraform-provider-google, google/services/iambeta/resource_iam_workload_identity_pool_provider.go, neither schema entry carries ForceNew:

"attribute_condition": { Type: schema.TypeString, Optional: true, ... },  // no ForceNew
"attribute_mapping":   { Type: schema.TypeMap,    Optional: true, ... },  // no ForceNew

Only workload_identity_pool_id and workload_identity_pool_provider_id are ForceNew: true. The registry docs contain no "Changing this forces a new resource to be created" text for either field. The pool and the provider are not replaced — this is not a breaking change for existing customers.

member on google_service_account_iam_member IS ForceNew. From the shared generator google/tpgiamresource/resource_iam_member.go:

"role":   { Type: schema.TypeString, Required: true, ForceNew: true },
"member": { Type: schema.TypeString, Required: true, ForceNew: true, ... },

So the grant is replaced, and ordering is the whole risk. The dangerous window the issue warns about is real: if the narrow member were created while attribute.aws_role still resolved to the raw session ARN, it would match nothing, and if the old pool-wide member were removed first, coverage would lapse. A safe sequence exists and is now pinned in the config:

depends_on = [google_iam_workload_identity_pool_provider.cudly]

lifecycle {
  create_before_destroy = true
}

Net apply order: reconfigure provider (in place) → add narrow member → remove old pool-wide member. At least one matching grant is in place at every step.

create_before_destroy is meaningful here because google_service_account_iam_member is the non-authoritative form. Registry docs, verbatim: "google_service_account_iam_member: Non-authoritative. Updates the IAM policy to grant a role to a new member. Other members for the role for the service account are preserved." Two members granting the same role to different principals coexist during the window; the authoritative _binding / _policy forms would not permit this.

TestGCPTargetUpgradeOrderingIsPinned asserts both directives are present, because losing either turns a security fix into a customer outage.

One point I could not hard-confirm: I found no Google doc stating explicitly whether an attribute value containing colons (a full ARN) must be URL-encoded inside a principalSet:// identifier. Google's docs do show unencoded slashes in attribute values (attribute.repository/my-org/my-repo), and the provider's own registry example compares attribute.aws_role to a full unencoded ARN. Decisive for this PR: the identical unencoded-ARN member string is already merged and shipping in the shell path (#1651), so the two paths agree either way — which was the goal. Flagging it rather than presenting it as settled.

Interop window with #1651 (already merged as 3fc9b57)

#1651 is on main, this is not. Until this merges and each Terraform-onboarded
customer re-applies, the two customer-facing onboarding paths actively contradict
each other:

• setup-gcp-wif.sh now scans the service account policy for
  principalSet://.../workloadIdentityPools/<pool>/* and refuses to report
  success while one exists, calling it a legacy grant from an older run of
  itself.
• The grant it finds on a Terraform-onboarded account was written by
  iac/federation/gcp-target, not by the script. The verdict is correct (the
  grant really is pool-wide) but the attribution is not, and the script's
  suggested remedy, --remove-legacy-pool-binding, would delete a member that
  Terraform still owns in state, so the next terraform apply recreates it.

That window closes per-customer on terraform apply after this merges: the apply
replaces the wildcard member with the exact-value one, and the provider condition
this writes is byte-identical to the one the script expects, so a subsequent
script run reports success instead of failing its condition comparison.

Operators who want the wildcard gone before re-applying should re-apply rather
than run --remove-legacy-pool-binding, for the state-drift reason above.

Also fixed (issue items c and d)

• (c) CEL break-out — aws_role_name and oidc_subject are interpolated into a single-quoted CEL string literal and into an IAM principal identifier, with no validation. aws_role_name = "x') || true || ('" rendered an always-true condition. Added validation blocks rejecting ' " \ * $, plus format checks (AWS's own role-name charset [A-Za-z0-9_+=,.@-]{1,64}; subject charset and GCP's 127-char google.subject` cap). The character check is kept separate from the format check because Terraform reports every failing validation, and "this would rewrite the attribute condition" is more actionable than a bare charset regex.
• (d) stale "Recommended" wording — internal/iacfiles/templates/gcp-wif.tfvars.tmpl shipped the identity pin commented out and labelled "Recommended" while main.tf requires it, so the downloaded bundle could not terraform apply. Now emitted uncommented and labelled REQUIRED, with the AWS branch naming the account the role must live in. The value is intentionally left empty: an empty string hits the lifecycle precondition and fails the apply with an explanation, whereas a REPLACE_ME placeholder would satisfy the variable validation and silently pin trust to a nonexistent role.

These template changes are required by the fix, not incidental — main.tf's precondition rejects an empty aws_role_name, so a tfvars still shipping it commented out would break the apply. templates_test.go covers both the aws and azure render branches.

Verification

• terraform fmt -check -diff — clean.
• terraform init -backend=false && terraform validate — Success! The configuration is valid.
• go test ./iac/... ./internal/iacfiles/... — 24 pass.
• Regression tests confirmed to fail pre-fix: reverting main.tf + the tfvars template to the parent commit and re-running gives 4 failures (TestGCPTargetPinsRoleARNByEquality, TestGCPTargetGrantsImpersonationToOneIdentity, TestGCPTargetUpgradeOrderingIsPinned, TestGCPWIFTfvarsMarksPinnedIdentityRequired); all pass after. Tests that would have stayed green with the bug present do not count as verification.
• Full pre-commit hooks ran (no --no-verify).

Not verified: a live terraform apply against a real GCP project and a real AWS token exchange. The migration verdict rests on the provider schema and the already-merged shell path, not on an executed upgrade.

Provenance

Recovered from two interrupted sessions that left uncommitted work in two worktrees:

• wt-1667 on branch sec/1667-gcp-wif-tf-exact-match, based on an older main — 4 files staged, never committed.
• wt-1667 on branch sec/1667-gcp-wif-tf-bypass, based on current main — the same 4 files with more complete content, plus an untracked iac/gcp_target_wif_test.go carrying the behavioural test suite.

I read both diffs before choosing. The second was both more complete and already exactly at origin/main (0 ahead / 0 behind), so it needed no rebase; the first attempt's stale base was avoided rather than rebased. Adopted from the second attempt: all five files. Verified independently rather than taken on trust: the ForceNew/migration claims in its code comments (checked against provider source — they hold), that the regression tests fail pre-fix (they do), that the mapping expression matches the merged shell sibling byte-for-byte (it does), that the template changes are required by the fix (they are), and terraform fmt / validate / go test.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 42 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0fc88acb-be95-45bd-92ca-a29c384a0f75

📥 Commits

Reviewing files that changed from the base of the PR and between 6ded401 and bdcc8c8.

📒 Files selected for processing (5)

• iac/federation/gcp-target/terraform/main.tf
• iac/federation/gcp-target/terraform/variables.tf
• iac/gcp_target_wif_test.go
• internal/iacfiles/templates/gcp-wif.tfvars.tmpl
• internal/iacfiles/templates_test.go

---

_{Comment @coderabbitai help to get the list of available commands.}