fix(arm): validate attribute mapping on GCP WIF provider reuse - #1673
fix(arm): validate attribute mapping on GCP WIF provider reuse#1673cristim wants to merge 1 commit into
Conversation
The provider-reuse path in setup-gcp-wif.sh compared an existing provider's attributeCondition against the expected value but never checked attributeMapping. The condition is only meaningful relative to the mapping that feeds it: a pre-existing provider whose condition is byte-identical to expected but whose mapping derives attribute.aws_role from a caller-controlled ARN path segment instead of the real assumed-role name lets an attacker who can create an IAM user with a crafted path impersonate the trusted role, while the script reports success. Extract the AWS and OIDC attribute mappings into EXPECTED_MAPPING (previously duplicated inline at both create-provider call sites) and compare it against the existing provider's mapping on reuse, failing the same way the condition check does on mismatch. gcloud's `value(...)` printer renders attributeMapping as semicolon-joined, key-sorted pairs, not the comma-joined form the --attribute-mapping flag takes as input, so a raw string comparison would false-positive on every correctly configured provider. Added normalize_mapping() to parse both sides into sorted key/value pairs before comparing. Also records that the AWS session-ARN 127-byte budget relies on the role/subject charset staying ASCII (chars == bytes).
cristim
added
triaged
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
1 participant
Closes #1672 (refs #1544)
PR #1651 (merged as
3fc9b5700) hardenedarm/CUDly-CrossSubscription/setup-gcp-wif.sh's provider-reuse path to reject an existing provider whoseattributeConditiondoesn'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 theattributeMappingthat 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:
An attacker holding only
iam:CreateUserin the trusted AWS account creates an IAM user with path/assumed-role/CUDly-Execution/.extract('assumed-role/{role_name}/')then yieldsCUDly-Execution, soattribute.aws_rolebecomes exactly the pinned role ARN, satisfying both the condition and the narrowprincipalSetgrant 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_MAPPINGis now built once per provider type (previously duplicated inline at both thecreate-awsandcreate-oidccall 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'svalue(...)printer rendersattributeMappingas semicolon-joined, key-sorted pairs (confirmed by readinggooglecloudsdk/core/resource/csv_printer.py'sCsvPrinter._AddRecord/ValuePrinter), not the comma-joined form--attribute-mappingtakes 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.shellcheck0.11.0 — exit 0, no findings.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).gcloudexercising 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 withprovider 'cudly-aws' already exists with a different attribute mapping..., printing both the found and expected values.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, branchsec/1544-gcp-wif-attribute-condition). By the time it was ready, #1651 had merged (squashed into3fc9b5700), so that branch's push had no PR to land on. This PR is the same commit cherry-picked onto currentmainvia a fresh branch.