fix(arm): validate attribute mapping on GCP WIF provider reuse

[cristim]

Closes #1672 (refs #1544)

PR #1651 (merged as 3fc9b5700) hardened arm/CUDly-CrossSubscription/setup-gcp-wif.sh's provider-reuse path to reject an existing provider whose attributeCondition doesn't match what this script would have written. A follow-up adversarial review found that check validates only half of the property it exists to enforce: the condition is only meaningful relative to the attributeMapping that produces the value it is evaluated against, and the mapping was never compared on reuse.

The bypass this closes

A pre-existing provider whose condition is byte-identical to the one this script would write (so the existing check passes) but whose mapping is:

attribute.aws_role = 'arn:aws:sts::' + assertion.account + ':assumed-role/' + assertion.arn.extract('assumed-role/{role_name}/')

An attacker holding only iam:CreateUser in the trusted AWS account creates an IAM user with path /assumed-role/CUDly-Execution/. extract('assumed-role/{role_name}/') then yields CUDly-Execution, so attribute.aws_role becomes exactly the pinned role ARN, satisfying both the condition and the narrow principalSet grant this script installs. Full service-account impersonation, and the script reports "Reusing existing provider (attribute condition matches: ...)" as success.

This is the same substring-vs-normalisation bypass class as the Terraform sibling's bug (#1667), reachable here only through a hand-crafted mapping on a provider this script did not itself create (this script's own create path is immune, since it writes the mapping and condition together).

The fix

EXPECTED_MAPPING is now built once per provider type (previously duplicated inline at both the create-aws and create-oidc call sites — a drift risk this change also removes) and compared against the existing provider's mapping on reuse, failing the same way the condition check does, naming what differed.

gcloud's value(...) printer renders attributeMapping as semicolon-joined, key-sorted pairs (confirmed by reading googlecloudsdk/core/resource/csv_printer.py's CsvPrinter._AddRecord / ValuePrinter), not the comma-joined form --attribute-mapping takes as input — a raw string comparison would report every correctly-configured provider as mismatched. normalize_mapping() splits each side on its own pair separator, re-sorts, and compares parsed key/value pairs, so neither that shape difference nor a hypothetical future change to gcloud's key ordering produces a false positive, while an actually-different mapping (including the bypass above) still compares unequal.

Also recorded in a comment: the AWS session-ARN 127-byte budget computed a few lines above is only correct because the role-name and OIDC-subject charsets are both ASCII-only (chars == bytes); it would silently mis-budget if that charset were ever widened.

Verification

• bash -n — exit 0. shellcheck 0.11.0 — exit 0, no findings.
• Unit-tested normalize_mapping() in isolation: a matching AWS mapping (comma-joined input form vs. semicolon-joined, sorted describe-output form) compares equal; a matching single-pair OIDC mapping compares equal; the exact bypass mapping above compares unequal to expected; gcloud output with keys in the opposite order still compares equal (order independence).
• End-to-end against a stubbed gcloud exercising the real script: a provider with a matching condition and matching mapping reaches "Reusing existing provider (attribute condition and mapping match expected values)" and proceeds to grant impersonation. The same setup with the bypass mapping substituted exits 1 with provider 'cudly-aws' already exists with a different attribute mapping..., printing both the found and expected values.
• Diff reviewed against origin/main (cherry-picked cleanly onto current main; scope is exactly this file, this change).

Note on branch history

This fix was originally developed against #1651 while that PR was still open (worktree wt-1544, branch sec/1544-gcp-wif-attribute-condition). By the time it was ready, #1651 had merged (squashed into 3fc9b5700), so that branch's push had no PR to land on. This PR is the same commit cherry-picked onto current main via a fresh branch.

Update: account/issuer reuse check was also a substring test

A second look at the same provider-reuse path (65de3d24b) found the check just above the ones this PR added — the one confirming an existing provider is actually the right type (AWS vs. OIDC) before the condition/mapping checks even run — had the identical bypass shape, and it is pre-existing (introduced by #1651, not by this PR).

It ran a single describe --format='value(aws.accountId,oidc.issuerUri)' and tab-joined the two columns, then tested the OIDC side with a suffix glob (!= *"${ISSUER_URI}") and the AWS side with a prefix glob (!= "${AWS_ACCOUNT_ID}"*). Both are substring tests standing in for equality:

• An existing OIDC provider whose issuer merely ends with the expected one — e.g. https://evil.example.com/https://token.actions.githubusercontent.com, an ordinary HTTPS URL from which GCP fetches /.well-known/openid-configuration — passed the suffix test, letting the attacker mint tokens with any sub.
• An existing AWS provider whose account ID merely starts with the expected one (e.g. 123456789012999999 against expected 123456789012) passed the prefix test.

Either way, the type check this PR's condition/mapping checks depend on was vacuous, so a provider under attacker control could pass everything downstream too. This is the fifth substring-standing-in-for-equality defect found in this codebase (after #1667's Terraform sibling, and others in the same review sweep).

Fixed by splitting into two separate describe calls — value(aws.accountId) and value(oidc.issuerUri) — each compared with exact !=, mirroring the pattern the condition/mapping checks already use.

Verified against three hostile OIDC issuer shapes (path-embedded, query-string, and concatenated-host, all ending in the legitimate issuer string) plus a digit-appended AWS account ID: all four are accepted by the pre-fix code and rejected post-fix, while a byte-identical legitimate provider is accepted both before and after in both the AWS and OIDC branches. bash -n and shellcheck 0.11.0 both exit 0 on the updated file.

[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: f0efc6c9-cb73-4999-983a-963d4ffcf8a5

📥 Commits

Reviewing files that changed from the base of the PR and between b117634 and 235eb7f.

📒 Files selected for processing (1)

• arm/CUDly-CrossSubscription/setup-gcp-wif.sh

---

_{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 48 minutes.

[cristim]

Review and fix record (merged)

Two rounds of independent adversarial review; recording both because CodeRabbit's status on the final head read success with description "Review rate limited".

Round 1 — mapping validation

The reuse path validated attributeCondition but never attributeMapping. That is load-bearing: the condition may use == instead of a substring test only because the mapping normalises the session ARN down to the role ARN, so validating one without the other checks half a property. A provider whose condition matched but whose mapping was hand-written to rebuild attribute.aws_role from an attacker-controllable path segment would be vouched for and reported safe.

The comparison is the part that had to be right in both directions: gcloud's value() printer emits semicolon-joined, key-sorted pairs while --attribute-mapping takes comma-joined input, so a raw string compare would have rejected every correctly-configured provider. Verified by reading googlecloudsdk's csv_printer.py rather than guessing. normalize_mapping() parses both sides into sorted pairs. Tested with the exact malicious mapping from the bug report (correctly rejected) and key-order independence, plus an end-to-end run against a stubbed gcloud.

EXPECTED_MAPPING was also hoisted to a single definition rather than left duplicated at both call sites — two copies of an expected security value is how the create and reuse paths drift apart in the first place.

Round 2 — the issuer check was a substring test

Found by adversarial review of this PR, pre-existing rather than introduced here, and it negated the PR's own goal:

if [[ "$PROVIDER_TYPE" == "oidc" && "$EXISTING_TYPE" != *"${ISSUER_URI}" ]]; then die

*"${ISSUER_URI}" accepts any issuer merely ending with the expected one. Reproduced: https://evil.example.com/https://token.actions.githubusercontent.com — an ordinary HTTPS URL — was accepted, along with query-string and concatenated-host variants. GCP would fetch the discovery document from the attacker, who then mints tokens with any sub, making both the condition check and the new mapping check vacuous for the OIDC path.

The AWS side had the mirror shape: "${AWS_ACCOUNT_ID}"*, a prefix match, safe only by luck because account IDs are fixed-length.

Fix: split the single describe --format='value(aws.accountId,oidc.issuerUri)' into two separate calls and compare each exactly. Verified with a stub gcloud driving both vulnerable and fixed code: three hostile issuers plus a prefix-matching account ID went from accepted to rejected, while a legitimate provider still passes in both branches. Every consumer of the removed EXISTING_TYPE was traced to confirm nothing was left unset, and PROVIDER_TYPE is validated to exactly aws/oidc at entry so the else branch cannot catch a third value.

This was the sixth substring-standing-in-for-equality defect found in this codebase in one session.

Cross-PR interaction caught before it shipped

This PR's exact-set attributeMapping comparison would have hard-died on every Terraform-onboarded customer once #1675 merged, because the Terraform module mapped a third attribute (attribute.account) the script does not. The verdict would have been wrong (an inert extra key, not an unsafe mapping) and the suggested remedy destructive — delete a live provider, detaching every federated session. Resolved by dropping the unreferenced attribute in #1675; the two mapping sets are now byte-identical, verified by cmp.