diff --git a/iac/federation/gcp-target/terraform/main.tf b/iac/federation/gcp-target/terraform/main.tf index 048596888..86c1e95f1 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,46 @@ 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. + # + # 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" - "attribute.account" = "assertion.account" + "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" } : 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 +188,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..8dbd99fc8 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..e2bc85239 --- /dev/null +++ b/iac/gcp_target_wif_test.go @@ -0,0 +1,360 @@ +package iac + +import ( + "os" + "regexp" + "slices" + "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 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` + +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(`principal(Set)?://[^"]*\*`) + 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) + } +} + +// 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 +// 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..b5fa1c92e 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..dc111beb9 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 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. + 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) {