From 33e4b7e18acb061c47d3c8c9456f80f82182442f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:23:40 +0200 Subject: [PATCH 1/4] sec(iac/gcp): pin WIF trust to the exact role ARN and narrow the SA grant The GCP WIF Terraform module mapped attribute.aws_role to the raw assertion.arn and gated on a substring test: "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 can create a user at path /assumed-role//. The resulting ARN arn:aws:iam:::user/assumed-role// carries the substring the condition looked for, and the pool-wide principalSet://.../* impersonation grant admitted every identity in the pool, yielding full impersonation of the CUDly service account and its compute.commitments.create/update authority. Normalise the session ARN down to the role ARN in the attribute mapping, then compare with == against arn:aws:sts:::assumed-role/. An IAM user ARN normalises to an arn:aws:iam::...:user/... value that cannot equal an arn:aws:sts:: role ARN, and equality (not a prefix or substring test) means a longer sibling role such as CUDly-Execution-Admin does not inherit CUDly-Execution's trust. Narrow the impersonation grant from the pool-wide wildcard to principalSet://.../attribute.aws_role/, restoring a second independent gate so a future mapping bug cannot widen access pool-wide on its own. The normalised attribute strips the per-session suffix, which is what makes an exact-value principalSet possible; the variable session name was the stated reason the wildcard was believed necessary. Mapping expression and member form are byte-identical to the shell sibling fixed in #1651, so the two customer-facing onboarding paths produce the same provider. That script's pool_wide_grants() scan flags a Terraform-created wildcard grant as legacy, so before this change a customer who onboarded with Terraform and later ran the script was correctly told their setup was unsafe. Upgrade ordering for already-applied deployments: attribute_mapping and attribute_condition update in place (neither is ForceNew), while member is ForceNew. depends_on reconfigures the provider before the member swap, and create_before_destroy adds the narrow member before the old pool-wide one is removed, so no window exists in which the service account has no matching grant. Also add validation blocks on aws_role_name and oidc_subject, which are interpolated into the CEL condition and into an IAM principal identifier: aws_role_name = "x') || true || ('" rendered an always-true condition. And stop shipping the identity pin commented out and labelled "Recommended" in the served tfvars template; main.tf requires it, so the downloaded bundle could not apply. Closes #1667 --- iac/federation/gcp-target/terraform/main.tf | 76 ++++- .../gcp-target/terraform/variables.tf | 50 ++++ iac/gcp_target_wif_test.go | 269 ++++++++++++++++++ .../iacfiles/templates/gcp-wif.tfvars.tmpl | 14 +- internal/iacfiles/templates_test.go | 46 +++ 5 files changed, 445 insertions(+), 10 deletions(-) create mode 100644 iac/gcp_target_wif_test.go diff --git a/iac/federation/gcp-target/terraform/main.tf b/iac/federation/gcp-target/terraform/main.tf index 048596888..d99b5e3f3 100644 --- a/iac/federation/gcp-target/terraform/main.tf +++ b/iac/federation/gcp-target/terraform/main.tf @@ -22,6 +22,16 @@ locals { project = var.project != "" ? var.project : data.google_project.current.project_id create_service_account = var.service_account_email == "" service_account_email = local.create_service_account ? google_service_account.cudly[0].email : var.service_account_email + + # The single AWS identity this deployment trusts. Both the provider's + # attribute_condition and the impersonation grant compare against this exact + # string, so they cannot drift apart. + # + # Standard AWS partition only: a GovCloud or China-partition caller presents + # arn:aws-us-gov:... / arn:aws-cn:..., which simply will not equal this value, + # so the token exchange is refused rather than admitted on a partition + # mismatch. + aws_role_arn = "arn:aws:sts::${var.aws_account_id}:assumed-role/${var.aws_role_name}" } resource "google_project_service" "iam" { @@ -97,16 +107,34 @@ resource "google_iam_workload_identity_pool_provider" "cudly" { workload_identity_pool_id = google_iam_workload_identity_pool.cudly.workload_identity_pool_id workload_identity_pool_provider_id = var.provider_id + # attribute.aws_role normalises the session ARN + # (arn:aws:sts:::assumed-role//) down to the role ARN, so + # the condition below and the IAM grant on the service account can both match + # it with == instead of testing for a substring. + # + # A substring test on the raw assertion.arn is bypassable. AWS IAM user paths + # accept any printable ASCII (documented grammar: (/)|(/[!-~]+/)), so an + # attacker holding only iam:CreateUser in the trusted account can create a + # user at path /assumed-role// whose ARN, + # arn:aws:iam:::user/assumed-role//, contains the + # substring the old condition looked for. Normalising first maps that ARN to + # arn:aws:iam:::user/assumed-role/, which is not equal to + # the arn:aws:sts:::assumed-role/ pinned below, so it is + # refused. STS drops the path from assumed-role ARNs, so the same trick does + # not work through an IAM role. + # + # Kept byte-identical to the mapping in arm/CUDly-CrossSubscription/setup-gcp-wif.sh + # so the two customer-facing onboarding paths produce the same provider. attribute_mapping = var.provider_type == "aws" ? { "google.subject" = "assertion.arn" - "attribute.aws_role" = "assertion.arn" + "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.account" = "assertion.account" } : var.oidc_attribute_mapping # Both branches are non-null: aws_role_name and oidc_subject are validated # as non-empty by lifecycle preconditions below, so neither falls back to null. attribute_condition = var.provider_type == "aws" ? ( - "attribute.aws_role.contains('assumed-role/${var.aws_role_name}/')" + "attribute.aws_role == '${local.aws_role_arn}'" ) : ( "google.subject == '${var.oidc_subject}'" ) @@ -148,17 +176,49 @@ resource "google_iam_workload_identity_pool_provider" "cudly" { } # Use _member (not _binding) to add one member without replacing existing bindings. +# +# Both branches name a single identity, so a future mapping or condition bug +# cannot widen impersonation beyond the pinned principal: +# OIDC — principal://.../subject/. +# AWS — principalSet://.../attribute.aws_role/. This is the +# exact-value form of a principalSet, not a prefix or wildcard: it +# admits only identities whose attribute.aws_role equals the value. +# Session ARNs carry a per-session suffix, which is why the grant names +# the normalised role attribute rather than google.subject; it is not a +# reason to fall back to the pool-wide wildcard principalSet this +# replaced, which granted impersonation to everything in the pool. +# +# Principal identifiers are pool-scoped, not provider-scoped: any provider added +# to this pool that can mint the same attribute value satisfies this grant. Keep +# var.pool_id dedicated to CUDly unless you intend to share it. resource "google_service_account_iam_member" "cudly_wif" { service_account_id = "projects/${local.project}/serviceAccounts/${local.service_account_email}" role = "roles/iam.workloadIdentityUser" - # For OIDC: scope binding to the specific subject (oidc_subject is required; see - # lifecycle precondition above), so only that subject can impersonate this SA. - # For AWS: wildcard principalSet is intentional; session ARNs include variable - # session names so exact-match principalSet cannot work. Trust is scoped by the - # mandatory attribute_condition on the provider (aws_role_name is required). member = var.provider_type == "oidc" ? ( "principal://iam.googleapis.com/${google_iam_workload_identity_pool.cudly.name}/subject/${var.oidc_subject}" ) : ( - "principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.cudly.name}/*" + "principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.cudly.name}/attribute.aws_role/${local.aws_role_arn}" ) + + # Upgrade ordering for deployments applied before the pool-wide grant was + # narrowed. `member` is ForceNew, so changing it replaces this resource, and + # the wrong order breaks federation for live customers: + # + # depends_on — attribute_mapping and attribute_condition are updated + # in place by the provider (neither is ForceNew; both + # go into the PATCH updateMask), so the provider is + # reconfigured first. Until that lands, attribute.aws_role + # still holds the raw session ARN and the new member + # below would match nothing. + # create_before_destroy — adds the narrow member while the old pool-wide one + # is still present, so there is no window in which the + # service account has neither. + # + # Net apply order: reconfigure provider -> add narrow member -> remove the old + # pool-wide member. Every step keeps at least one matching grant in place. + depends_on = [google_iam_workload_identity_pool_provider.cudly] + + lifecycle { + create_before_destroy = true + } } diff --git a/iac/federation/gcp-target/terraform/variables.tf b/iac/federation/gcp-target/terraform/variables.tf index 3f827b270..f87d48f51 100644 --- a/iac/federation/gcp-target/terraform/variables.tf +++ b/iac/federation/gcp-target/terraform/variables.tf @@ -54,16 +54,66 @@ variable "oidc_attribute_mapping" { } } +# Both values below are interpolated into a single-quoted string literal inside +# the provider's CEL attribute_condition and into the IAM principal identifier of +# the impersonation grant. The character checks reject the forms that fail OPEN +# in those two destinations rather than merely looking wrong: +# +# ' " \ ` terminate the CEL string literal, so the rest of the value rewrites +# the condition — aws_role_name = "x') || true || ('" renders an +# always-true condition that admits every identity; +# * widens an IAM principal identifier to match every identity; +# $ catches a pasted ${...} placeholder, which is not expanded here and +# yields a binding matching nothing (or, where expanded, everything). +# +# The format checks that follow already exclude those characters. They are kept +# as separate validations because Terraform reports every failing validation, and +# "this would rewrite the attribute condition" is far more actionable than a bare +# charset regex when someone pastes a crafted value. + variable "aws_role_name" { description = "AWS IAM role name to restrict trust to (e.g. 'CUDly-Execution'). Required when provider_type is 'aws'. Without this the attribute_condition is null and any IAM role in the account can federate." type = string default = "" + + validation { + condition = !can(regex("['\"\\\\`*$]", var.aws_role_name)) + error_message = "aws_role_name must not contain quotes, backslashes, backticks, '*' or '$'. It is interpolated into the provider's CEL attribute condition and into the IAM principal identifier of the impersonation grant, where those characters can escape the string literal or widen the grant to every identity." + } + + # Matches AWS's own role-name charset [\w+=,.@-]{1,64}. The comma is safe here: + # the role name reaches attribute_condition (a plain string), never the + # comma-delimited attribute_mapping keys. + validation { + condition = var.aws_role_name == "" || can(regex("^[A-Za-z0-9_+=,.@-]{1,64}$", var.aws_role_name)) + error_message = "aws_role_name must be a bare IAM role name of 1-64 characters from [A-Za-z0-9_+=,.@-] (e.g. \"CUDly-Execution\"), not an ARN or a path." + } } variable "oidc_subject" { description = "OIDC subject claim to restrict trust to. Required when provider_type is 'oidc'. Without this the attribute_condition is null and any subject from the issuer can federate." type = string default = "" + + validation { + condition = !can(regex("['\"\\\\`*$]", var.oidc_subject)) + error_message = "oidc_subject must not contain quotes, backslashes, backticks, '*' or '$'. It is interpolated into the provider's CEL attribute condition and into the IAM principal identifier of the impersonation grant, where those characters can escape the string literal or widen the grant to every identity." + } + + # '|' is permitted because Auth0/Okta-style subjects use it (google-oauth2|123). + # It is inert in both destinations: quotes are rejected above, so it cannot + # escape the CEL string literal, and it carries no meaning in a principal path. + validation { + condition = var.oidc_subject == "" || can(regex("^[A-Za-z0-9][-A-Za-z0-9._:/@=+~|]*$", var.oidc_subject)) + error_message = "oidc_subject must start with an alphanumeric character and contain only [-A-Za-z0-9._:/@=+~|] (e.g. \"repo:my-org/my-repo:ref:refs/heads/main\")." + } + + # oidc_subject becomes google.subject, which GCP caps at 127 characters. A + # longer value is rejected at token-exchange time, long after apply succeeds. + validation { + condition = length(var.oidc_subject) <= 127 + error_message = "oidc_subject must be at most 127 characters: it becomes google.subject, which GCP rejects above that length at token-exchange time." + } } # ------------------------------------------------------------------------ diff --git a/iac/gcp_target_wif_test.go b/iac/gcp_target_wif_test.go new file mode 100644 index 000000000..99eb3bc7a --- /dev/null +++ b/iac/gcp_target_wif_test.go @@ -0,0 +1,269 @@ +package iac + +import ( + "regexp" + "strings" + "testing" +) + +// These tests assert on the exact bytes of the Terraform module shipped to +// customers (internal/api/handler_federation.go serves federation/gcp-target as +// the default bundle for target=gcp), so a regression is caught in the artifact +// the customer applies rather than in a copy of it. + +const gcpTargetMainTF = "federation/gcp-target/terraform/main.tf" + +// awsRoleAttributeMapping is the CEL expression main.tf maps to +// attribute.aws_role. It normalises a session ARN +// (arn:aws:sts:::assumed-role//) down to the role ARN. +// +// It is asserted byte-for-byte below so that normalizeAWSRoleAttr — the Go +// transcription the behavioural tests run — cannot silently drift from the +// expression Terraform actually applies. Editing the mapping fails the string +// assertion and forces the transcription to be revisited. +const awsRoleAttributeMapping = `assertion.arn.contains('assumed-role') ? assertion.arn.extract('{account_arn}assumed-role/') + 'assumed-role/' + assertion.arn.extract('assumed-role/{role_name}/') : assertion.arn` + +const ( + testAccountID = "123456789012" + testRoleName = "CUDly-Execution" +) + +// testPinnedRoleARN is what local.aws_role_arn renders to for the values above, +// and therefore both the right-hand side of the attribute condition and the +// attribute value named by the impersonation grant. +const testPinnedRoleARN = "arn:aws:sts::" + testAccountID + ":assumed-role/" + testRoleName + +func readMainTF(t *testing.T) string { + t.Helper() + raw, err := Modules.ReadFile(gcpTargetMainTF) + if err != nil { + t.Fatalf("read %s: %v", gcpTargetMainTF, err) + } + return string(raw) +} + +// TestGCPTargetPinsRoleARNByEquality asserts the provider normalises the +// assertion ARN and then compares it with ==. A substring test on the raw ARN is +// bypassable (see TestGCPTargetRejectsIAMUserPathBypass). +func TestGCPTargetPinsRoleARNByEquality(t *testing.T) { + tf := readMainTF(t) + + wantMapping := `"attribute.aws_role" = "` + awsRoleAttributeMapping + `"` + if !strings.Contains(tf, wantMapping) { + t.Errorf("%s: attribute.aws_role must be mapped to the normalising expression, want line:\n%s", gcpTargetMainTF, wantMapping) + } + + wantLocal := `aws_role_arn = "arn:aws:sts::${var.aws_account_id}:assumed-role/${var.aws_role_name}"` + if !strings.Contains(tf, wantLocal) { + t.Errorf("%s: expected local pinning the trusted role ARN:\n%s", gcpTargetMainTF, wantLocal) + } + + wantCondition := `"attribute.aws_role == '${local.aws_role_arn}'"` + if !strings.Contains(tf, wantCondition) { + t.Errorf("%s: attribute_condition must compare attribute.aws_role for equality, want:\n%s", gcpTargetMainTF, wantCondition) + } + + // Guard the two shapes this fix replaced: a raw mapping, and a substring + // test. Either one on its own re-opens the bypass. + if strings.Contains(tf, `"attribute.aws_role" = "assertion.arn"`) { + t.Errorf("%s: attribute.aws_role is mapped raw again; the ARN must be normalised before it is compared", gcpTargetMainTF) + } + if strings.Contains(tf, "attribute.aws_role.contains(") || strings.Contains(tf, "attribute.aws_role.startsWith(") { + t.Errorf("%s: attribute_condition uses a substring/prefix test on attribute.aws_role; only == pins a single role", gcpTargetMainTF) + } +} + +// TestGCPTargetGrantsImpersonationToOneIdentity asserts the +// roles/iam.workloadIdentityUser grant names an exact identity rather than the +// whole pool. Tightening the attribute condition alone leaves the blast radius +// of the next mapping bug unchanged. +func TestGCPTargetGrantsImpersonationToOneIdentity(t *testing.T) { + tf := readMainTF(t) + + wantAWSMember := `"principalSet://iam.googleapis.com/${google_iam_workload_identity_pool.cudly.name}/attribute.aws_role/${local.aws_role_arn}"` + if !strings.Contains(tf, wantAWSMember) { + t.Errorf("%s: AWS impersonation grant must name the exact normalised role ARN, want:\n%s", gcpTargetMainTF, wantAWSMember) + } + + wantOIDCMember := `"principal://iam.googleapis.com/${google_iam_workload_identity_pool.cudly.name}/subject/${var.oidc_subject}"` + if !strings.Contains(tf, wantOIDCMember) { + t.Errorf("%s: OIDC impersonation grant must name the exact subject, want:\n%s", gcpTargetMainTF, wantOIDCMember) + } + + // Any wildcard in a principal identifier matches every identity in the pool, + // whichever role or attribute it is attached to. + wildcard := regexp.MustCompile(`principalSet?://[^"]*\*`) + if m := wildcard.FindString(tf); m != "" { + t.Errorf("%s: wildcard principal identifier %q grants impersonation pool-wide", gcpTargetMainTF, m) + } +} + +// TestGCPTargetUpgradeOrderingIsPinned asserts the two lifecycle directives that +// make the member swap safe on deployments applied before this fix. Losing +// either one turns a security fix into an outage: without depends_on the narrow +// member can be created while attribute.aws_role still holds the raw session ARN +// (matching nothing), and without create_before_destroy the old pool-wide member +// is removed before the narrow one exists. +func TestGCPTargetUpgradeOrderingIsPinned(t *testing.T) { + tf := readMainTF(t) + + _, grant, found := strings.Cut(tf, `resource "google_service_account_iam_member" "cudly_wif"`) + if !found { + t.Fatalf("%s: google_service_account_iam_member.cudly_wif not found", gcpTargetMainTF) + } + if !strings.Contains(grant, "depends_on = [google_iam_workload_identity_pool_provider.cudly]") { + t.Errorf("%s: cudly_wif must depend on the provider so the attribute mapping is updated before the narrow member is created", gcpTargetMainTF) + } + if !strings.Contains(grant, "create_before_destroy = true") { + t.Errorf("%s: cudly_wif must set create_before_destroy so the narrow member exists before the pool-wide one is removed", gcpTargetMainTF) + } +} + +// extractTemplate models the extract() function available in GCP's workload +// identity attribute-mapping CEL. The template is a literal containing exactly +// one {placeholder}; the result is the text between the literal prefix and +// suffix, or "" when the template does not match. +// +// Google's own documented example is the expression under test: +// "arn:aws:sts::123456789012:assumed-role/Admin/85" extracts +// "arn:aws:sts::123456789012:" via '{account_arn}assumed-role/' and "Admin" via +// 'assumed-role/{role_name}/'. +func extractTemplate(s, tmpl string) string { + open := strings.Index(tmpl, "{") + closeIdx := strings.Index(tmpl, "}") + if open < 0 || closeIdx < open { + return "" + } + prefix, suffix := tmpl[:open], tmpl[closeIdx+1:] + + rest := s + if prefix != "" { + i := strings.Index(rest, prefix) + if i < 0 { + return "" + } + rest = rest[i+len(prefix):] + } + if suffix == "" { + return rest + } + j := strings.Index(rest, suffix) + if j < 0 { + return "" + } + return rest[:j] +} + +// normalizeAWSRoleAttr is the Go transcription of awsRoleAttributeMapping. It is +// the value attribute.aws_role carries at token-exchange time, i.e. the value +// the attribute condition and the impersonation grant both compare against. +func normalizeAWSRoleAttr(arn string) string { + if !strings.Contains(arn, "assumed-role") { + return arn + } + return extractTemplate(arn, "{account_arn}assumed-role/") + + "assumed-role/" + + extractTemplate(arn, "assumed-role/{role_name}/") +} + +// oldConditionAdmits reproduces the condition this fix replaced: +// attribute.aws_role.contains('assumed-role//') applied to the raw ARN. +// Used to prove the exploit case below genuinely passed the previous gate. +func oldConditionAdmits(arn, roleName string) bool { + return strings.Contains(arn, "assumed-role/"+roleName+"/") +} + +func TestGCPTargetRejectsIAMUserPathBypass(t *testing.T) { + // arn:aws:iam:::user/assumed-role// is reachable by + // anyone holding iam:CreateUser in the trusted account: AWS's IAM path + // grammar is (/)|(/[!-~]+/), so /assumed-role/CUDly-Execution/ is a legal + // path and the resulting user ARN carries it. STS strips the path from + // assumed-role ARNs, so the same trick does not work through an IAM role. + exploit := "arn:aws:iam::" + testAccountID + ":user/assumed-role/" + testRoleName + "/evil" + + if !oldConditionAdmits(exploit, testRoleName) { + t.Fatalf("test fixture is wrong: %q must satisfy the substring condition this fix replaced, or it does not reproduce the reported bypass", exploit) + } + if got := normalizeAWSRoleAttr(exploit); got == testPinnedRoleARN { + t.Errorf("IAM user path bypass still admitted: normalised to %q, which equals the pinned %q", got, testPinnedRoleARN) + } +} + +func TestNormalizedAWSRoleAttrAdmitsOnlyThePinnedRole(t *testing.T) { + tests := []struct { + name string + arn string + admit bool + }{ + { + name: "pinned role session", + arn: "arn:aws:sts::" + testAccountID + ":assumed-role/" + testRoleName + "/cudly-session", + admit: true, + }, + { + // The session name varies per exchange; normalisation exists so the + // same grant matches every session of the pinned role. + name: "pinned role, different session name", + arn: "arn:aws:sts::" + testAccountID + ":assumed-role/" + testRoleName + "/i-0abc123", + admit: true, + }, + { + // Equality, not a prefix test: a longer sibling role must not inherit + // the pinned role's trust. + name: "longer sibling role name", + arn: "arn:aws:sts::" + testAccountID + ":assumed-role/" + testRoleName + "-Admin/s", + admit: false, + }, + { + name: "IAM user crafted at path /assumed-role//", + arn: "arn:aws:iam::" + testAccountID + ":user/assumed-role/" + testRoleName + "/evil", + admit: false, + }, + { + // Two occurrences: whichever occurrence extract() binds to, the value + // keeps the arn:aws:iam:::user/ prefix and cannot equal an + // arn:aws:sts:: role ARN. + name: "IAM user path repeating the marker", + arn: "arn:aws:iam::" + testAccountID + ":user/assumed-role/" + testRoleName + "/assumed-role/" + testRoleName + "/evil", + admit: false, + }, + { + name: "plain IAM user", + arn: "arn:aws:iam::" + testAccountID + ":user/alice", + admit: false, + }, + { + // Session name carrying the pinned role name. + name: "unrelated role with a crafted session name", + arn: "arn:aws:sts::" + testAccountID + ":assumed-role/Attacker/assumed-role-" + testRoleName, + admit: false, + }, + { + name: "pinned role in a different AWS account", + arn: "arn:aws:sts::210987654321:assumed-role/" + testRoleName + "/s", + admit: false, + }, + { + // The pinned ARN is standard-partition; a GovCloud caller is refused + // rather than admitted on a partition mismatch. + name: "pinned role in the GovCloud partition", + arn: "arn:aws-us-gov:sts::" + testAccountID + ":assumed-role/" + testRoleName + "/s", + admit: false, + }, + { + name: "root account principal", + arn: "arn:aws:iam::" + testAccountID + ":root", + admit: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := normalizeAWSRoleAttr(tc.arn) + if admitted := got == testPinnedRoleARN; admitted != tc.admit { + t.Errorf("attribute.aws_role for %q normalised to %q; admitted=%v, want %v (pinned: %q)", + tc.arn, got, admitted, tc.admit, testPinnedRoleARN) + } + }) + } +} diff --git a/internal/iacfiles/templates/gcp-wif.tfvars.tmpl b/internal/iacfiles/templates/gcp-wif.tfvars.tmpl index 872598c71..0bbab1a83 100644 --- a/internal/iacfiles/templates/gcp-wif.tfvars.tmpl +++ b/internal/iacfiles/templates/gcp-wif.tfvars.tmpl @@ -25,9 +25,19 @@ provider_type = "oidc" oidc_issuer_uri = "{{.OIDCIssuerURI}}"{{if eq .Source "azure"}} # Azure: https://login.microsoftonline.com//v2.0{{end}} {{- end}} {{- if eq .Source "aws"}} -# aws_role_name = "" # Recommended: restrict to specific IAM role (e.g. "CUDly-WIF") +# REQUIRED. The bare name of the IAM role CUDly assumes in account +# {{.SourceAccountID}} — ask your CUDly contact if you are unsure. It is the only +# identity that will be able to impersonate the service account above: the +# Terraform module pins the provider's attribute condition and the impersonation +# grant to arn:aws:sts::{{.SourceAccountID}}:assumed-role/. +# `terraform apply` fails while this is empty; it is not a hardening option. +aws_role_name = "" # e.g. "CUDly-Execution" {{- else}} -# oidc_subject = "" # Recommended: restrict to specific OIDC subject +# REQUIRED. The exact `sub` claim CUDly's token carries. It is the only subject +# that will be able to impersonate the service account above: the Terraform +# module pins the provider's attribute condition and the impersonation grant to +# it. `terraform apply` fails while this is empty; it is not a hardening option. +oidc_subject = "" # e.g. "repo:my-org/my-repo:ref:refs/heads/main" {{- end}} # --- CUDly auto-registration (optional) --- diff --git a/internal/iacfiles/templates_test.go b/internal/iacfiles/templates_test.go index 1f280536c..083eb5df2 100644 --- a/internal/iacfiles/templates_test.go +++ b/internal/iacfiles/templates_test.go @@ -222,6 +222,52 @@ func TestCLITemplatesShellMetacharsPassThrough(t *testing.T) { } } +// TestGCPWIFTfvarsMarksPinnedIdentityRequired proves the generated tfvars file +// presents aws_role_name / oidc_subject as a blank the operator must fill, not +// as an optional hardening step. iac/federation/gcp-target/terraform pins both +// the provider's attribute condition and the impersonation grant to that value +// and refuses to apply without it, so "Recommended" understated it: a bundle +// following the old wording could not apply at all. +func TestGCPWIFTfvarsMarksPinnedIdentityRequired(t *testing.T) { + const path = "templates/gcp-wif.tfvars.tmpl" + + awsData := baseData() + awsData.Source = "aws" + aws := renderCLITemplate(t, path, awsData) + + azureData := baseData() + azureData.Source = "azure" + azure := renderCLITemplate(t, path, azureData) + + cases := []struct { + name string + rendered string + assign string + }{ + {"aws", aws, "aws_role_name = \"\""}, + {"azure", azure, "oidc_subject = \"\""}, + } + for _, tc := range cases { + if strings.Contains(tc.rendered, "Recommended") { + t.Errorf("%s (%s): pinning the trusted identity is required, not recommended; terraform apply fails without it", path, tc.name) + } + if !strings.Contains(tc.rendered, "REQUIRED") { + t.Errorf("%s (%s): expected the pinned-identity variable to be labelled REQUIRED", path, tc.name) + } + // Emitted uncommented so the blank is visible in the file the customer + // edits; a commented-out line reads as an option that was left off. + if !strings.Contains(tc.rendered, "\n"+tc.assign) { + t.Errorf("%s (%s): expected an uncommented %q line for the operator to fill", path, tc.name, tc.assign) + } + } + + // The AWS branch names the account the role must live in, so the operator + // knows which account's role to pin. + if !strings.Contains(aws, awsData.SourceAccountID) { + t.Errorf("%s (aws): expected the source AWS account ID %q in the aws_role_name guidance", path, awsData.SourceAccountID) + } +} + // TestCLITemplatesOmitRegisterBlock proves the auto-register section is gated // on CUDlyAPIURL -- when it's empty, the block disappears entirely. func TestCLITemplatesOmitRegisterBlock(t *testing.T) { From 2c8b62ee721b698ea76399938658c2c3e23f1f38 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:31:06 +0200 Subject: [PATCH 2/4] sec(iac/gcp): catch a wildcard on the OIDC branch in the WIF grant test The wildcard guard added alongside the narrowed impersonation grant read `principalSet?://[^"]*\*`, where the `?` applies to the preceding `t` rather than to `Set`. It therefore matched only `principalSe`/ `principalSet`, and a wildcard reintroduced on the OIDC branch (`principal://.../*`) would have slipped past the very check that exists to reject it. Group the optional segment: `principal(Set)?://[^"]*\*`. Confirmed by running the test against the pre-fix main.tf: the guard now reports the wildcard member specifically, not just the missing exact-value member. Also drop the em-dashes from the lines this change added, and correct the comment claiming the mapping is byte-identical to the shell sibling: the attribute.aws_role expression, the condition and the member form are identical, but the module additionally maps attribute.account, which the script does not. The script compares conditions rather than mappings, so the extra attribute stays inert there. --- iac/federation/gcp-target/terraform/main.tf | 43 +++++++++++-------- .../gcp-target/terraform/variables.tf | 2 +- iac/gcp_target_wif_test.go | 6 +-- .../iacfiles/templates/gcp-wif.tfvars.tmpl | 2 +- 4 files changed, 29 insertions(+), 24 deletions(-) diff --git a/iac/federation/gcp-target/terraform/main.tf b/iac/federation/gcp-target/terraform/main.tf index d99b5e3f3..7c4d932cf 100644 --- a/iac/federation/gcp-target/terraform/main.tf +++ b/iac/federation/gcp-target/terraform/main.tf @@ -123,8 +123,13 @@ resource "google_iam_workload_identity_pool_provider" "cudly" { # refused. STS drops the path from assumed-role ARNs, so the same trick does # not work through an IAM role. # - # Kept byte-identical to the mapping in arm/CUDly-CrossSubscription/setup-gcp-wif.sh - # so the two customer-facing onboarding paths produce the same provider. + # This expression, the attribute_condition below, and the impersonation grant's + # member are byte-identical to their counterparts in + # arm/CUDly-CrossSubscription/setup-gcp-wif.sh, so the two customer-facing + # onboarding paths pin the same identity and the script's condition check + # accepts a provider this module created. (The script does not map + # attribute.account, which is mapped here and unused by the condition; it + # compares conditions, not mappings, so the extra attribute is inert there.) attribute_mapping = var.provider_type == "aws" ? { "google.subject" = "assertion.arn" "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" @@ -179,14 +184,14 @@ resource "google_iam_workload_identity_pool_provider" "cudly" { # # Both branches name a single identity, so a future mapping or condition bug # cannot widen impersonation beyond the pinned principal: -# OIDC — principal://.../subject/. -# AWS — principalSet://.../attribute.aws_role/. This is the -# exact-value form of a principalSet, not a prefix or wildcard: it -# admits only identities whose attribute.aws_role equals the value. -# Session ARNs carry a per-session suffix, which is why the grant names -# the normalised role attribute rather than google.subject; it is not a -# reason to fall back to the pool-wide wildcard principalSet this -# replaced, which granted impersonation to everything in the pool. +# OIDC: principal://.../subject/. +# AWS: principalSet://.../attribute.aws_role/. This is the +# exact-value form of a principalSet, not a prefix or wildcard: it +# admits only identities whose attribute.aws_role equals the value. +# Session ARNs carry a per-session suffix, which is why the grant names +# the normalised role attribute rather than google.subject; it is not a +# reason to fall back to the pool-wide wildcard principalSet this +# replaced, which granted impersonation to everything in the pool. # # Principal identifiers are pool-scoped, not provider-scoped: any provider added # to this pool that can mint the same attribute value satisfies this grant. Keep @@ -204,15 +209,15 @@ resource "google_service_account_iam_member" "cudly_wif" { # narrowed. `member` is ForceNew, so changing it replaces this resource, and # the wrong order breaks federation for live customers: # - # depends_on — attribute_mapping and attribute_condition are updated - # in place by the provider (neither is ForceNew; both - # go into the PATCH updateMask), so the provider is - # reconfigured first. Until that lands, attribute.aws_role - # still holds the raw session ARN and the new member - # below would match nothing. - # create_before_destroy — adds the narrow member while the old pool-wide one - # is still present, so there is no window in which the - # service account has neither. + # depends_on: attribute_mapping and attribute_condition are + # updated in place by the provider (neither is + # ForceNew; both go into the PATCH updateMask), so the + # provider is reconfigured first. Until that lands, + # attribute.aws_role still holds the raw session ARN + # and the new member below would match nothing. + # create_before_destroy: adds the narrow member while the old pool-wide one + # is still present, so there is no window in which the + # service account has neither. # # Net apply order: reconfigure provider -> add narrow member -> remove the old # pool-wide member. Every step keeps at least one matching grant in place. diff --git a/iac/federation/gcp-target/terraform/variables.tf b/iac/federation/gcp-target/terraform/variables.tf index f87d48f51..8dbd99fc8 100644 --- a/iac/federation/gcp-target/terraform/variables.tf +++ b/iac/federation/gcp-target/terraform/variables.tf @@ -60,7 +60,7 @@ variable "oidc_attribute_mapping" { # in those two destinations rather than merely looking wrong: # # ' " \ ` terminate the CEL string literal, so the rest of the value rewrites -# the condition — aws_role_name = "x') || true || ('" renders an +# the condition; aws_role_name = "x') || true || ('" renders an # always-true condition that admits every identity; # * widens an IAM principal identifier to match every identity; # $ catches a pasted ${...} placeholder, which is not expanded here and diff --git a/iac/gcp_target_wif_test.go b/iac/gcp_target_wif_test.go index 99eb3bc7a..2a750f630 100644 --- a/iac/gcp_target_wif_test.go +++ b/iac/gcp_target_wif_test.go @@ -17,8 +17,8 @@ const gcpTargetMainTF = "federation/gcp-target/terraform/main.tf" // attribute.aws_role. It normalises a session ARN // (arn:aws:sts:::assumed-role//) down to the role ARN. // -// It is asserted byte-for-byte below so that normalizeAWSRoleAttr — the Go -// transcription the behavioural tests run — cannot silently drift from the +// It is asserted byte-for-byte below so that normalizeAWSRoleAttr, the Go +// transcription the behavioural tests run, cannot silently drift from the // expression Terraform actually applies. Editing the mapping fails the string // assertion and forces the transcription to be revisited. const awsRoleAttributeMapping = `assertion.arn.contains('assumed-role') ? assertion.arn.extract('{account_arn}assumed-role/') + 'assumed-role/' + assertion.arn.extract('assumed-role/{role_name}/') : assertion.arn` @@ -92,7 +92,7 @@ func TestGCPTargetGrantsImpersonationToOneIdentity(t *testing.T) { // Any wildcard in a principal identifier matches every identity in the pool, // whichever role or attribute it is attached to. - wildcard := regexp.MustCompile(`principalSet?://[^"]*\*`) + wildcard := regexp.MustCompile(`principal(Set)?://[^"]*\*`) if m := wildcard.FindString(tf); m != "" { t.Errorf("%s: wildcard principal identifier %q grants impersonation pool-wide", gcpTargetMainTF, m) } diff --git a/internal/iacfiles/templates/gcp-wif.tfvars.tmpl b/internal/iacfiles/templates/gcp-wif.tfvars.tmpl index 0bbab1a83..b5fa1c92e 100644 --- a/internal/iacfiles/templates/gcp-wif.tfvars.tmpl +++ b/internal/iacfiles/templates/gcp-wif.tfvars.tmpl @@ -26,7 +26,7 @@ oidc_issuer_uri = "{{.OIDCIssuerURI}}"{{if eq .Source "azure"}} # Azure: https: {{- end}} {{- if eq .Source "aws"}} # REQUIRED. The bare name of the IAM role CUDly assumes in account -# {{.SourceAccountID}} — ask your CUDly contact if you are unsure. It is the only +# {{.SourceAccountID}}. Ask your CUDly contact if you are unsure. It is the only # identity that will be able to impersonate the service account above: the # Terraform module pins the provider's attribute condition and the impersonation # grant to arn:aws:sts::{{.SourceAccountID}}:assumed-role/. From 4b39d46b23fd2b29ab44eafbc3e65dd7212fedd1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 29 Jul 2026 11:34:19 +0200 Subject: [PATCH 3/4] style(iac/gcp): use the US spellings misspell enforces in the WIF tests golangci-lint at the version CI pins (v2.10.1) flags "behavioural" and "labelled" via the misspell linter. Both are in a comment and an assertion message, so the rewrite touches no identifier and no asserted string. --- iac/gcp_target_wif_test.go | 2 +- internal/iacfiles/templates_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/iac/gcp_target_wif_test.go b/iac/gcp_target_wif_test.go index 2a750f630..89972a62c 100644 --- a/iac/gcp_target_wif_test.go +++ b/iac/gcp_target_wif_test.go @@ -18,7 +18,7 @@ const gcpTargetMainTF = "federation/gcp-target/terraform/main.tf" // (arn:aws:sts:::assumed-role//) down to the role ARN. // // It is asserted byte-for-byte below so that normalizeAWSRoleAttr, the Go -// transcription the behavioural tests run, cannot silently drift from the +// transcription the behavioral tests run, cannot silently drift from the // expression Terraform actually applies. Editing the mapping fails the string // assertion and forces the transcription to be revisited. const awsRoleAttributeMapping = `assertion.arn.contains('assumed-role') ? assertion.arn.extract('{account_arn}assumed-role/') + 'assumed-role/' + assertion.arn.extract('assumed-role/{role_name}/') : assertion.arn` diff --git a/internal/iacfiles/templates_test.go b/internal/iacfiles/templates_test.go index 083eb5df2..dc111beb9 100644 --- a/internal/iacfiles/templates_test.go +++ b/internal/iacfiles/templates_test.go @@ -252,7 +252,7 @@ func TestGCPWIFTfvarsMarksPinnedIdentityRequired(t *testing.T) { t.Errorf("%s (%s): pinning the trusted identity is required, not recommended; terraform apply fails without it", path, tc.name) } if !strings.Contains(tc.rendered, "REQUIRED") { - t.Errorf("%s (%s): expected the pinned-identity variable to be labelled REQUIRED", path, tc.name) + t.Errorf("%s (%s): expected the pinned-identity variable to be labeled REQUIRED", path, tc.name) } // Emitted uncommented so the blank is visible in the file the customer // edits; a commented-out line reads as an option that was left off. From 40d5207b78fc8692b40602506fee10878de6fc93 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 13:35:24 +0200 Subject: [PATCH 4/4] fix(iac/gcp): drop attribute.account so both WIF onboarding paths agree The Terraform module maps three attributes on the AWS branch (google.subject, attribute.aws_role, attribute.account) while arm/CUDly-CrossSubscription/setup-gcp-wif.sh maps only the first two. Nothing reads attribute.account: its only two references in the tree were the mapping line itself and the comment above it. It is not named by the attribute_condition, by the impersonation grant's principalSet, or by any test. Today that divergence is inert, because the script compares only the provider's attributeCondition. Once the script also compares attributeMapping as an exact set, it stops being inert: both paths default to pool 'cudly-pool' and provider 'cudly-provider', so a customer onboarded via the Terraform bundle who then runs the script against the same project lands on this module's provider, fails the set comparison on the one extra key, and is told to delete the provider and re-run. That detaches every live federated session, and the next terraform apply then fights the recreated provider. The verdict would also be wrong: the mapping is not unsafe, it just carries one extra unused key. Removing the key rather than teaching the script to expect it keeps the comparison an exact set, which is what stops an unexpected attribute from being introduced later and keyed on by a pool-wide principalSet://.../attribute.X/... grant. Account pinning is unaffected: the provider's aws.account_id block and the account number inside the pinned role ARN both still constrain it. Also corrects the comment above the mapping. It specifically anticipated this attribute and concluded it was inert because the script "compares conditions, not mappings" - true when written, false as soon as the script compares mappings, and a future reader would have trusted it. Migration: attribute_mapping is not ForceNew in the google provider (the field is Optional with no ForceNew, and Update appends "attributeMapping" to the PATCH updateMask), so dropping a key is an in-place update. It rides along with the change this branch already makes to the same field and adds no replacement risk. TestGCPTargetMappingMatchesSetupScript locks the two paths to the same mapping set, parsing both files rather than restating either, and fails if a key is ever added on one side alone. Refs #1667 --- iac/federation/gcp-target/terraform/main.tf | 23 ++++-- iac/gcp_target_wif_test.go | 91 +++++++++++++++++++++ 2 files changed, 106 insertions(+), 8 deletions(-) diff --git a/iac/federation/gcp-target/terraform/main.tf b/iac/federation/gcp-target/terraform/main.tf index 7c4d932cf..86c1e95f1 100644 --- a/iac/federation/gcp-target/terraform/main.tf +++ b/iac/federation/gcp-target/terraform/main.tf @@ -123,17 +123,24 @@ resource "google_iam_workload_identity_pool_provider" "cudly" { # refused. STS drops the path from assumed-role ARNs, so the same trick does # not work through an IAM role. # - # This expression, the attribute_condition below, and the impersonation grant's - # member are byte-identical to their counterparts in - # arm/CUDly-CrossSubscription/setup-gcp-wif.sh, so the two customer-facing - # onboarding paths pin the same identity and the script's condition check - # accepts a provider this module created. (The script does not map - # attribute.account, which is mapped here and unused by the condition; it - # compares conditions, not mappings, so the extra attribute is inert there.) + # This expression, the attribute_condition below, the impersonation grant's + # member, and the set of keys mapped here are byte-identical to their + # counterparts in arm/CUDly-CrossSubscription/setup-gcp-wif.sh, so the two + # customer-facing onboarding paths pin the same identity and configure the + # same provider. The two paths do converge on one provider rather than staying + # independent: both default to pool 'cudly-pool' and provider + # 'cudly-provider', so a customer who applies this module and then runs the + # script lands on the provider this resource created. + # + # Do not map an attribute here without mapping it in the script too. The + # script refuses to reuse a provider whose configuration differs from the one + # it would have written, and the only remedy it offers is deleting the + # provider, which detaches every live federated session. An extra key on one + # side alone is enough to trigger that, even when nothing reads its value. + # TestGCPTargetMappingMatchesSetupScript guards the parity. attribute_mapping = var.provider_type == "aws" ? { "google.subject" = "assertion.arn" "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.account" = "assertion.account" } : var.oidc_attribute_mapping # Both branches are non-null: aws_role_name and oidc_subject are validated diff --git a/iac/gcp_target_wif_test.go b/iac/gcp_target_wif_test.go index 89972a62c..e2bc85239 100644 --- a/iac/gcp_target_wif_test.go +++ b/iac/gcp_target_wif_test.go @@ -1,7 +1,9 @@ package iac import ( + "os" "regexp" + "slices" "strings" "testing" ) @@ -119,6 +121,95 @@ func TestGCPTargetUpgradeOrderingIsPinned(t *testing.T) { } } +// setupScript is the shell onboarding path. It is the second way a customer can +// configure the same pool and provider; both paths default to pool 'cudly-pool' +// and provider 'cudly-provider', so a customer who uses one and then the other +// lands on the same provider rather than on two independent ones. +const setupScript = "../arm/CUDly-CrossSubscription/setup-gcp-wif.sh" + +// awsMappingBlock captures the body of the AWS branch of attribute_mapping in +// main.tf, i.e. everything between the ternary's opening brace and the OIDC +// alternative. +var awsMappingBlock = regexp.MustCompile(`(?s)attribute_mapping = var\.provider_type == "aws" \? \{(.*?)\n\s*\} : var\.oidc_attribute_mapping`) + +// hclMappingPair matches one `"key" = "value"` entry inside that block. +var hclMappingPair = regexp.MustCompile(`(?m)^\s*"([^"]+)"\s*=\s*"(.*)"\s*$`) + +// scriptMappingLiteral captures the quoted, comma-delimited mapping dict the +// script hands to gcloud's --attribute-mapping. It is anchored on the +// attribute.aws_role key so it selects the AWS dict and not the OIDC one. +var scriptMappingLiteral = regexp.MustCompile(`"([^"]*attribute\.aws_role=[^"]*)"`) + +// tfAWSMapping returns main.tf's AWS attribute_mapping as sorted "key=value" +// entries, the form both sides are compared in. +func tfAWSMapping(t *testing.T) []string { + t.Helper() + + block := awsMappingBlock.FindStringSubmatch(readMainTF(t)) + if block == nil { + t.Fatalf("%s: AWS branch of attribute_mapping not found", gcpTargetMainTF) + } + pairs := hclMappingPair.FindAllStringSubmatch(block[1], -1) + if len(pairs) == 0 { + t.Fatalf("%s: AWS attribute_mapping has no entries", gcpTargetMainTF) + } + + got := make([]string, 0, len(pairs)) + for _, p := range pairs { + got = append(got, p[1]+"="+p[2]) + } + slices.Sort(got) + return got +} + +// scriptAWSMapping returns the script's AWS attribute mapping as sorted +// "key=value" entries. Every literal carrying the mapping must agree, so the +// script cannot create a provider with one mapping and reconcile against +// another. +func scriptAWSMapping(t *testing.T) []string { + t.Helper() + + raw, err := os.ReadFile(setupScript) + if err != nil { + t.Fatalf("read %s: %v", setupScript, err) + } + literals := scriptMappingLiteral.FindAllStringSubmatch(string(raw), -1) + if len(literals) == 0 { + t.Fatalf("%s: no --attribute-mapping dict containing attribute.aws_role found", setupScript) + } + for _, l := range literals[1:] { + if l[1] != literals[0][1] { + t.Fatalf("%s: AWS attribute mapping is spelled two different ways:\n%s\n%s", setupScript, literals[0][1], l[1]) + } + } + + got := strings.Split(literals[0][1], ",") + slices.Sort(got) + return got +} + +// TestGCPTargetMappingMatchesSetupScript pins the two onboarding paths to the +// same attribute mapping. +// +// The script refuses to reuse a provider whose configuration differs from the +// one it would have written, so a mapping this module writes but the script +// does not expect is not a cosmetic divergence: a customer onboarded via the +// Terraform bundle who later runs the script against the same project is told +// to delete the provider and start over, which detaches every live federated +// session and which the next terraform apply then fights. An extra key does +// that even when nothing reads its value, so the whole set is compared here, +// not just the keys the attribute_condition and the impersonation grant consume. +func TestGCPTargetMappingMatchesSetupScript(t *testing.T) { + tfMapping := tfAWSMapping(t) + scriptMapping := scriptAWSMapping(t) + + if !slices.Equal(tfMapping, scriptMapping) { + t.Errorf("AWS attribute mapping differs between the two onboarding paths; a provider created by one is unusable by the other.\n%s:\n %s\n%s:\n %s", + gcpTargetMainTF, strings.Join(tfMapping, "\n "), + setupScript, strings.Join(scriptMapping, "\n ")) + } +} + // extractTemplate models the extract() function available in GCP's workload // identity attribute-mapping CEL. The template is a literal containing exactly // one {placeholder}; the result is the text between the literal prefix and