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.

Cross-PR interop fix: attribute.account dropped (found by adversarial review of #1673 against #1675)

Found only by reviewing this PR against open #1673, not by reviewing either
PR on its own. Neither is broken alone; the incompatibility exists only once both
merge.

main.tf mapped three attributes on the AWS branch while
arm/CUDly-CrossSubscription/setup-gcp-wif.sh maps two:

key	Terraform	script
google.subject = assertion.arn	yes	yes
attribute.aws_role = (the normalising CEL)	yes	yes
attribute.account = assertion.account	yes	no

On main today this is inert, because the script compares only
attributeCondition. #1673 adds an exact-SET comparison of
attributeMapping, not a subset test. Both paths default to pool cudly-pool
and provider cudly-provider (verified at variables.tf:9,15 and
setup-gcp-wif.sh:46-47), so a customer onboarded via the Terraform bundle who
then runs the script lands on this module's provider and hits a hard die:

provider 'cudly-provider' already exists with a different attribute mapping,
so this script cannot vouch for which identities enter pool 'cudly-pool'

whose only offered remedy is deleting the provider, which detaches every live
federated session and which the next terraform apply then fights. The verdict
would also be wrong: the mapping is not unsafe, it carries one extra inert key.

Reproduced by running #1673's normalize_mapping verbatim against a TF-created
provider's mapping: 3 parsed pairs vs 2, so MISMATCH, so die. The OIDC branch
matches on both sides and was never affected.

Fix: drop "attribute.account" = "assertion.account". Nothing reads it. Its
only two references in the tree were the mapping line and the comment above it;
it is not named by the attribute_condition, by the impersonation grant's
principalSet, or by any test. It is pre-existing on main (last touched by
#510), so this PR did not introduce it, but making the two paths agree is this
PR's stated goal and this was dead configuration.

Both alternatives are worse. Relaxing #1673's check to a subset test re-opens the
door to an unexpected extra attribute that a pool-wide
principalSet://.../attribute.X/... grant could key on. Adding attribute.account
to the script's EXPECTED_MAPPING forces #1673 to change and leaves dead
configuration on both sides.

Account pinning is unaffected: the provider's aws { account_id = ... } block
and the account number inside the pinned role ARN both still constrain it.

Migration: safe, in place, no replacement. attribute_mapping is not
ForceNew (same schema check as the section above; Update appends
"attributeMapping" to the PATCH updateMask), so dropping a key is an in-place
update. It rides along with the change this PR already makes to that same field
and costs no additional migration risk.

The comment above the mapping is corrected, not just trimmed. It specifically
anticipated this attribute and reached the wrong conclusion ("it compares
conditions, not mappings, so the extra attribute is inert there"): accurate
against main today, false the moment #1673 merges. A comment a future reader
would trust is worse than no comment, so it now states the invariant that
actually holds and the consequence of breaking it.

TestGCPTargetMappingMatchesSetupScript locks the parity. It parses the AWS
mapping out of both files rather than restating either, and fails if a key is
ever added on one side alone. Confirmed to fail with attribute.account re-added
and pass without it, and to pass against both the current script and #1673's
EXPECTED_MAPPING form.

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/... — 25 pass (24 + TestGCPTargetMappingMatchesSetupScript).
• Mapping parity proven byte-identically: the AWS mapping extracted from main.tf and the one extracted from setup-gcp-wif.sh, each sorted and rendered as key=value lines, cmp equal (sha256 686df559...). Before dropping attribute.account they differed by exactly that line.
• fix(arm): validate attribute mapping on GCP WIF provider reuse #1673's normalize_mapping run verbatim against a simulated gcloud ... describe --format='value(attributeMapping)' of a TF-created provider: MISMATCH (would die) before, MATCH (reuses the provider) after.
• 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: 35 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: b9664367-e303-4543-b8f4-7bf2670a4e4c

📥 Commits

Reviewing files that changed from the base of the PR and between b117634 and 40d5207.

📒 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.}

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 56 minutes.

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 10 minutes.

[cristim]

Adversarial review record (merged)

Independent reviewer, distinct from the author. Recording it here because the merge rested on this — CodeRabbit's status on this PR read success while its description said "Review rate limited", i.e. it never actually reviewed this p0 customer-facing security PR, which presented as "0 unresolved threads".

Exactness verified, no bypass found. The condition is == against a normalised value. Exhaustive argument: for the normalised value to equal the pinned string, the raw ARN must literally begin with arn:aws:sts::<pinned acct>:assumed-role/, and the iam vs sts service segment is not attacker-controllable at any path depth. So the IAM-user-path exploit normalises to arn:aws:iam::<acct>:user/assumed-role/CUDly-Execution and is rejected; CUDly-Execution-Admin is rejected by ==. Edge cases probed beyond the PR's table (ARN ending at assumed-role with no trailing slash, no session name, non-assumed-role ARNs) all fail closed. CEL extract() was not executed locally — labelled unverified; the conclusion holds under both first- and last-occurrence semantics.

Non-obvious correctness point: Google documents two AWS mapping variants. The simpler attribute.aws_role=assertion.arn.extract('assumed-role/{role_name}/') form would still be vulnerable to this exact attack. This PR picked the full-ARN ternary variant, which is the safe one.

Pool-wide binding genuinely narrowed to the documented exact-attribute-value principalSet form. This also resolved an open question the PR itself flagged: Google's docs show the ARN unencoded inside the identifier, so colons need no URL-encoding.

Migration confirmed from provider source, not the PR's word. attribute_mapping and attribute_condition have no ForceNew and both enter the updateMask; only the pool/provider IDs are ForceNew. iam_member's member and role are ForceNew with no Update function, so the grant is genuinely replaced. Not a breaking change.

Tests mutation-tested, not merely run pre-fix: four mutations of main.tf (reintroduce /* on the AWS branch, on the OIDC branch, == → startsWith, mapping back to raw assertion.arn) and every one is caught by a distinct test. The OIDC-branch mutation specifically verifies that the principalSet?:// → principal(Set)?:// regex fix in fd7f8678e works.

Finding recorded, not blocking

depends_on + create_before_destroy order the API calls but cannot order GCP's propagation. The comment's "Every step keeps at least one matching grant in place" is true at the API layer and not guaranteed at the consistency layer, so an existing customer re-applying can see a transient federation gap of minutes. Self-healing, no Terraform-side fix exists.

Two intentional narrowings worth knowing: the pinned ARN hardcodes arn:aws:, so GovCloud/China now fail closed where the partition-agnostic contains() admitted them; and the account is now pinned in the condition, though it was already enforced by the provider's aws { account_id } block.

Hygiene issue found

known_issues/15_iac_gcp_target.md marked the wildcard-principalSet item ✔️ Resolved while the wildcard was still live at merge-base — plausibly why #1667 survived to be found later. This PR finally makes that claim true.