fix(arm): pin GCP WIF attribute condition and narrow SA grant - #1651
Conversation
setup-gcp-wif.sh created Workload Identity Federation providers that
admitted every identity the issuer would vouch for, then granted
roles/iam.workloadIdentityUser to the whole pool. Customers run this
against their own GCP projects, so both defects exposed their service
account, which carries commitment-purchase authority.
AWS mode called providers create-aws with only --account-id and no
--attribute-condition, so any IAM principal in the referenced account
could exchange credentials at sts.googleapis.com and impersonate the
service account. OIDC mode printed a stderr warning when
--subject-condition was empty and then created the provider anyway; the
documented copy-paste example used token.actions.githubusercontent.com
and omitted the flag, so any GitHub Actions workflow on the platform
could federate in. The impersonation grant was bound to
principalSet://.../workloadIdentityPools/<pool>/*, so anything admitted
to the pool could impersonate regardless.
The Terraform sibling at iac/federation/gcp-target already enforces this
via lifecycle preconditions; the shell path for the same onboarding job
did not.
Both modes now require an identity to pin and exit nonzero without one:
--aws-role-name for aws, --oidc-subject for oidc. The provider is always
created with an attribute condition, and the grant names a single
principal (attribute.aws_role for AWS, principal://.../subject/ for
OIDC). The AWS attribute mapping normalises the session ARN to the role
ARN so both the condition and the grant match it exactly rather than by
substring.
--subject-condition is removed rather than made mandatory: it took a raw
CEL expression, so a value such as "true" looked like a restriction while
admitting every subject. Values interpolated into the condition or the
principal identifier are now validated, rejecting the forms that fail
open: '*' widens an IAM principal to match everything, a quote or
backslash terminates the CEL string literal and lets the rest of the
value append "|| true", and a '$' catches pasted ${...} placeholders that
are never expanded here. Issuer URIs must be https, and account IDs, role
names, pool and provider IDs are format-checked.
Re-running over an earlier setup no longer reports success while the
pool-wide grant survives: it is detected and the script exits nonzero
with the removal command, or deletes it under
--remove-legacy-pool-binding. A pre-existing provider carrying no
attribute condition is likewise refused instead of reused.
BREAKING CHANGE: --aws-role-name (aws) and --oidc-subject (oidc) are now
required, and --subject-condition is rejected. Anyone who ran an earlier
version still has the pool-wide grant and remains exposed until they
re-run with --remove-legacy-pool-binding.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 30 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)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
An adversarial review of the previous commit found four ways the hardening could still be bypassed on a re-run against an already provisioned project. The legacy pool-wide grant was only looked for under the --pool-id of the current run, and only under roles/iam.workloadIdentityUser. Both scopings were escapable. The script's own pool-reuse note recommends moving to a dedicated pool, and doing so rebuilt the search string for the new pool, so the original wildcard grant went unseen and the script exited 0. A wildcard under roles/iam.serviceAccountTokenCreator was matched by the policy grep but removal was hardcoded to workloadIdentityUser, so it survived a run that reported success; generateAccessToken confers the same impersonation power. The scan is now pool-agnostic and role-agnostic, removal uses the role each member is actually bound to, and the policy is re-read afterwards so a partial removal cannot pass. The existing-provider gate only asserted that the attribute condition was non-empty, which grandfathered exactly the values --subject-condition was removed to prevent: "true" and "assertion.sub != ''" are both non-empty and both admit every identity. The condition is now compared to the one this script would have written, and the provider type is checked so an OIDC provider is not reused under --provider-type aws. This also rejects a provider created by the Terraform sibling, whose contains() condition pairs with a different attribute mapping that the grant here would never match. The get-iam-policy read is kept out of the grep pipeline: piping it into `grep ... || true` would let a failed policy read produce empty output and pass as "no wildcard grants found". Also: surface narrow grants for identities other than this run's, since re-running with a corrected role or subject otherwise leaves the previous one able to impersonate; warn when the AWS role name is long enough that the session ARN risks exceeding GCP's 127-character google.subject limit; and accept two characters real identities use, "," in AWS role names (AWS permits it and it never reaches the comma-delimited attribute mapping) and "|" in OIDC subjects (Auth0 and Okta style), both inert because quotes remain rejected.
|
@coderabbitai full review |
✅ Action performedFull 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 36 minutes. |
Three follow-ups from review, all in the advisory paths of the GCP WIF setup script. The sibling-grant notice excluded this run's member with `grep -vF`, which drops every line containing it rather than every line equal to it. A grant to CUDly-Execution-Admin was therefore suppressed while pinning CUDly-Execution, hiding a stale grant during exactly the "re-ran with a corrected role" case the notice exists for. The member is now compared as a whole field. The google.subject budget used 38 characters for "arn:aws:sts::<12 digits>:assumed-role/", which is 39. A 24-character role name skipped the warning while still producing a 128-character subject that GCP rejects at token-exchange time. The pool-scope notice printed only when reusing an existing pool. A pool this script creates fresh is equally exposed once a second provider is added to it later, and that operator never saw the warning. It now prints unconditionally.
Closes #1544
arm/CUDly-CrossSubscription/setup-gcp-wif.shis run by customers against their own GCP projects. Both defects filed in #1544 were still live onmainat 887d51f (re-verified, not just at the filed commit be11bdc).What was wrong
A. No attribute condition on the provider. AWS mode called
providers create-awswith only--account-id. OIDC mode printed a stderr warning when--subject-conditionwas empty and then created the provider anyway, and the header's copy-paste example usedhttps://token.actions.githubusercontent.comwithout the flag.B. Pool-wide impersonation.
roles/iam.workloadIdentityUserwas bound toprincipalSet://.../workloadIdentityPools/<pool>/*, so anything admitted to the pool could impersonate the service account regardless of how the provider was configured.The Terraform sibling
iac/federation/gcp-target/terraform/main.tfalready forbids the condition-less state withlifecyclepreconditions. The shell path for the same onboarding job did not.Before / after
Provider creation, AWS mode:
Provider creation, OIDC mode (the
--attribute-conditionline was previously appended only when--subject-conditionhappened to be non-empty):Impersonation grant:
AWS keeps a
principalSeton the normalised role attribute because session ARNs carry a per-session suffix; the attribute mapping normalisesarn:aws:sts::<acct>:assumed-role/<role>/<session>down to the role ARN so the grant matches it exactly rather than by substring. That mapping is verbatim Google's documented AWS default.Attacker's view
Before: in OIDC mode with the documented command, the identity that could federate was any GitHub Actions job in any repository on GitHub -
token.actions.githubusercontent.commints a token for every workflow on the platform, the pool had no condition to reject them, and the pool-wide grant let anything admitted impersonate the SA. In AWS mode it was any IAM principal in the referenced AWS account that can callsts:GetCallerIdentity: a sandbox role, a CI runner, an EC2 instance profile on a compromised host. Either way the attacker lands on a service account holdingcompute.commitments.create/update, i.e. spend authority on the customer's project, with no CUDly involvement.After: the provider's attribute condition rejects the token at STS exchange time unless
google.subjectequals the one pinned subject (OIDC) or the normalised role ARN equals the one pinned role (AWS), so a workflow inattacker/repoor a different role in the same AWS account never enters the pool. The grant is a second, independent gate: it names one principal, so even an identity that did enter the pool is not a member of the binding. Neither gate can be skipped, because the script exits nonzero without a value to pin.Fail-closed, and the fail-open validation forms
--aws-role-name(aws) and--oidc-subject(oidc) are now required; missing either exits 1 before anygcloudcall.--subject-conditionis removed, not made mandatory. It took a raw CEL expression, so--subject-condition trueor--subject-condition "assertion.sub != ''"satisfied a non-empty check while admitting every subject from the issuer - a mandatory-but-unvalidated flag would have reproduced the bug it was meant to close.--oidc-subjecttakes the subject itself and the script builds the equality condition.Validation targets the forms that fail open, not the merely ugly ones:
'"\`are rejected because they terminate the CEL string literal in--attribute-condition.--oidc-subject "x' || true || '"would otherwise rendergoogle.subject == 'x' || true || '', a condition that is always true. This is the direct analogue of the${...}expansion trap on sec(iac/aws): require OIDC subject claim in aws-target CloudFormation #1602: the guard being added is what gets rewritten.*is rejected because it widens an IAM principal identifier rather than being compared literally.$is rejected to catch a pasted${...}placeholder. Checked explicitly: GCP IAM principal identifiers and CEL attribute conditions have no policy-variable expansion (unlike AWS IAM's${aws:...}), so a${...}here is literal and fails closed rather than open - but it still silently produces a binding that matches nothing, so it is refused rather than accepted.evals and quotes every interpolation, so the shell itself performs no second expansion.Also enforced:
--issuer-urimust behttps://,--aws-account-idmust be 12 digits, and pool/provider/project/SA values are format-checked. Two characters real identities legitimately use are permitted:,in AWS role names (AWS's own charset allows it, and role names never reach the comma-delimited attribute mapping) and|in OIDC subjects (Auth0/Okta style). Both are inert because quotes remain rejected.Second commit: fail-open gaps on the re-run path
An adversarial review of the first commit found four ways the hardening could still be bypassed against an already-provisioned project. All four are fixed in 21af9cc:
--pool-id. The script's own pool-reuse note recommends moving to a dedicated pool, and doing so rebuilt the search string for the new pool, so the original wildcard grant went unseen and the script exited 0 - the p0 surviving via a path the script recommends. The scan is now pool-agnostic.roles/iam.workloadIdentityUserwhile the detection scanned the whole policy. A wildcard underroles/iam.serviceAccountTokenCreator(same impersonation power viagenerateAccessToken) matched the check but survived the removal, and nothing re-verified. Removal now uses the role each member is actually bound to, and the policy is re-read afterwards so a partial removal cannot pass.--subject-conditionwas removed to prevent. It now compares against the condition this script would have written, and checks the provider type so an OIDC provider is not reused under--provider-type aws.get-iam-policyread is kept out of the grep pipeline. Piping it intogrep ... || truewould let a failed policy read produce empty output and pass as "no wildcard grants found".Also added: narrow grants for identities other than this run's are surfaced (a re-run with a corrected role otherwise leaves the previous one impersonating), and a warning when the AWS role name is long enough that the session ARN risks exceeding GCP's 127-character
google.subjectlimit.Upgrade path (breaking)
Anyone who ran an earlier version of this script still has the pool-wide grant and remains exposed until they re-run it. Re-running is not enough on its own: the script now detects wildcard pool grants on the service account - across every pool and every role, not just this run's - and exits nonzero without printing the credential config while any survive, either printing the
remove-iam-policy-bindingcommands or deleting them under--remove-legacy-pool-binding. A pre-existing provider whose attribute condition is missing, different, or merely non-empty is refused rather than silently reused.The Terraform sibling is worse, and is now its own p0
#1544's body frames both defects as "the shell path lacks the guard its Terraform sibling enforces". That premise is disproved by this PR, and I have posted a correction on the issue so no later reader treats
iac/federation/gcp-target/terraformas the safe reference. It is the worse of the two paths, and it is equally customer-facing (iac/embed.go:9embedsfederation;handler_federation.go:641-655servesfederation/gcp-targetas the default bundle fortarget=gcp).Filed as #1667 (p0/critical): the Terraform module carries the same pool-wide grant at
main.tf:162, plus a live bypass this script never had. It mapsattribute.aws_role = "assertion.arn"raw and substring-tests it withcontains('assumed-role/<role>/'), so an attacker with onlyiam:CreateUserin the trusted account creates an IAM user with path/assumed-role/<role>/(legal under AWS's(/)|(/[!-~]+/)path regex), andGetCallerIdentityreturns an ARN containing the exact substring the condition tests for. IAM role paths do not work, because STS strips the path fromassumed-roleARNs; it is specifically the user ARN, which retains its path, that defeats a substring test.This is the argument for the fix shape used here. The script is immune because it normalises the assertion down to the role ARN and compares with
==, which no user ARN can satisfy at any path. The Terraform module's inline comment claiming exact-match "cannot work" for AWS is what this PR disproves.Divergence worth noting:
pool_wide_grants()in this PR flags a Terraform-created grant as legacy and refuses to report success, so the two onboarding paths actively contradict each other until #1667 lands.Closes #1544is nonetheless correct - that issue is file-scoped by both its title and its "two defects in one script" body, and both are fully fixed here.Verification
bash -n- exit 0.shellcheck0.11.0 - exit 0, no findings.--subject-condition,*in subject / role / pool-id, the' || true || 'CEL break-out, a${...}placeholder,http://issuer, non-numeric account ID): all exit 1, and a stubgcloudthat exits 99 on any invocation was never reached, confirming nothing is created before validation.serviceAccountTokenCreatorwith and without the flag; wildcards under both roles at once (both removed); existing provider with conditiontrue, withassertion.sub != '', and with the Terraform sibling'scontains()form (all rejected); existing provider with the exact expected condition (still reused, no false mismatch); wrong provider type; stale narrow grant surfaced;|and,now accepted.create-aws/create-oidc/add-iam-policy-bindinginvocations captured against a stubbedgcloudand checked argument by argument.CUDly-Execution-Adminis now surfaced while pinningCUDly-Execution(grep -vFhad suppressed it as a substring) and the run's own member is still correctly excluded; thegoogle.subjectbudget lands exactly on the boundary after the 38 -> 39 correction (23-char role = 127 chars, no warning; 24-char = 128, warns); and the pool-scope notice now prints once on a freshly created pool, not only on reuse.create-cred-configproblem in item 3 of sec(iac): served GCP WIF script swallows provider-create failure + unvalidated CEL subject; OIDC cred-config bug #1661 is reported as unconfirmed for that reason.Reported, not changed here
Filed as #1667 (p0): the Terraform bypass and pool-wide grant, above.
Filed as #1661 (p1): the served onboarding template
internal/iacfiles/templates/gcp-wif-cli.sh.tmplswallowing provider-create failures with|| echo "(provider may already exist)", its unvalidatedCUDLY_FEDERATED_SUBJECTreaching a CEL literal, andcreate-cred-configbeing called with no credential source in this script's OIDC branch (pre-existing, unverified against a live gcloud).Checked and clean or out of scope:
--attribute-conditionand bindsprincipal://.../subject/..., and its AWS-STS provider path was deliberately removed (asserted bytemplates_test.go'smustNot: "create-aws"). This is the sec(iac/aws): require OIDC subject claim in aws-target CloudFormation #1602-style "second reachable copy" check, and it came back clean.internal/iacfiles/templates/gcp-wif.tfvars.tmplshipsaws_role_name/oidc_subjectcommented out and labelled "Recommended" while the module's preconditions make them required. Fails closed (the apply errors), so this is a wording bug, not a hole.--allowed-audiencesis pinned; GCP's default allowed audience is the provider's own resource URL, already provider-scoped.create-cred-configoutput contains none, as the script states.iac/federation/aws-target/**(sec(iac/aws): require OIDC subject claim in aws-target CloudFormation #1602) and the ARM template (sec(iac): ARM cross-subscription template assigns the purchase role at tenant Microsoft.Capacity scope #1545).--projectis now format-checked as a GCP project ID, so a numeric project number is rejected wheregcloud projects describepreviously accepted it. Fails closed with a clear message; flagged rather than fixed because project IDs are what every other argument and the printed registration values assume.docs/DEPLOYMENT.mdmentions the auth mode only;frontend/src/index.htmlnames the script without flags), so the usage examples in the script header are the only documentation updated.