diff --git a/iac/federation/aws-target/cloudformation/template.yaml b/iac/federation/aws-target/cloudformation/template.yaml index 1a67b1b4e..f1d093de6 100644 --- a/iac/federation/aws-target/cloudformation/template.yaml +++ b/iac/federation/aws-target/cloudformation/template.yaml @@ -34,10 +34,21 @@ Parameters: Expected audience (aud) in the incoming OIDC token. Azure: api:// or GCP: https://iam.googleapis.com/projects//locations/global/workloadIdentityPools//providers/ - Leave empty to skip audience matching, or supply a non-whitespace string. + Leaving this empty does NOT skip audience matching. Both the trust + policy condition and the OIDC provider ClientIdList fall back to the + literal string sts.amazonaws.com, so only tokens carrying exactly that + audience are accepted. Default: "" - AllowedPattern: "^$|^\\S$|^\\S.*\\S$" - ConstraintDescription: "OIDCAudience must be either empty or a non-whitespace string (no leading/trailing whitespace)." + AllowedPattern: "^$|^[^\\s*$]+$" + ConstraintDescription: > + OIDCAudience must be empty, or a string containing no whitespace, '$' + or '*' anywhere. + '$' is rejected because the trust policy is an IAM policy document and + IAM expands ${...} policy variables inside Condition values: an audience + of ${accounts.google.com:aud} would expand to the presented token's own + aud claim and therefore match every token. + '*' is rejected because StringEquals compares it literally rather than + as a wildcard, silently producing a role nobody can assume. OIDCThumbprint: Type: String @@ -52,9 +63,34 @@ Parameters: OIDCSubjectClaim: Type: String Description: > - Optional subject (sub) claim to restrict which identity can assume the role. - Leave empty to allow any authenticated subject from this issuer. - Default: "" + REQUIRED. Subject (sub) claim identifying the single workload allowed to + assume this role. There is deliberately no default: without a :sub + condition the trust policy accepts EVERY identity the issuer can mint, + so any tenant of that issuer could assume this role and place + irreversible multi-year commitment purchases in this account. + Azure AD managed identity: the object ID of the managed identity. + GCP service account: the service account's numeric unique ID (typically + 21 digits; Google documents it as a numeric string without guaranteeing + a length). That is the sub claim accounts.google.com actually issues; it + is not the service account email. + The system:serviceaccount:: form is the Kubernetes + subject format and belongs to a cluster OIDC issuer, not to + accounts.google.com. + Mirrors the required oidc_subject_claim variable in the Terraform module. + MinLength: 1 + AllowedPattern: "^[^\\s*$]+$" + ConstraintDescription: > + OIDCSubjectClaim is required and must be a non-empty string containing + no whitespace, '$' or '*' anywhere. + '$' is rejected because the trust policy below is an IAM policy document + (Version 2012-10-17) and IAM expands ${...} policy variables inside + Condition values. A subject of ${accounts.google.com:sub} would expand + to the presented token's own sub claim, making the condition a tautology + that matches EVERY identity the issuer can mint. That is exactly the + hole this parameter exists to close, so it is rejected at submission + time rather than deployed. + '*' is rejected because StringEquals compares it literally rather than + as a wildcard, silently producing a role nobody can assume. RoleName: Type: String @@ -63,7 +99,6 @@ Parameters: Conditions: HasAudience: !Not [!Equals [!Ref OIDCAudience, ""]] - HasSubject: !Not [!Equals [!Ref OIDCSubjectClaim, ""]] Resources: OIDCProvider: @@ -156,26 +191,19 @@ Resources: AssumeRolePolicyDocument: Version: "2012-10-17" Statement: - # Use Fn::If to conditionally include :sub restriction alongside :aud - - !If - - HasSubject - - Effect: Allow - Principal: - Federated: !GetAtt OIDCProvider.Arn - Action: sts:AssumeRoleWithWebIdentity - Condition: - StringEquals: - !Sub "${OIDCIssuerHost}:aud": - !If [HasAudience, !Ref OIDCAudience, sts.amazonaws.com] - !Sub "${OIDCIssuerHost}:sub": !Ref OIDCSubjectClaim - - Effect: Allow - Principal: - Federated: !GetAtt OIDCProvider.Arn - Action: sts:AssumeRoleWithWebIdentity - Condition: - StringEquals: - !Sub "${OIDCIssuerHost}:aud": - !If [HasAudience, !Ref OIDCAudience, sts.amazonaws.com] + # Both :aud and :sub are always enforced. There is intentionally no + # subject-less variant of this statement: omitting :sub would trust + # every identity the issuer can mint. OIDCSubjectClaim is a required + # parameter, so this condition can never collapse to an empty match. + - Effect: Allow + Principal: + Federated: !GetAtt OIDCProvider.Arn + Action: sts:AssumeRoleWithWebIdentity + Condition: + StringEquals: + !Sub "${OIDCIssuerHost}:aud": + !If [HasAudience, !Ref OIDCAudience, sts.amazonaws.com] + !Sub "${OIDCIssuerHost}:sub": !Ref OIDCSubjectClaim ManagedPolicyArns: - !Ref CUDlyPolicy diff --git a/iac/federation/aws-target/terraform/variables.tf b/iac/federation/aws-target/terraform/variables.tf index d35bd8865..47ad4ec84 100644 --- a/iac/federation/aws-target/terraform/variables.tf +++ b/iac/federation/aws-target/terraform/variables.tf @@ -19,18 +19,39 @@ variable "oidc_audience" { Expected audience (aud) in the OIDC token. Azure: api:// or GCP: https://iam.googleapis.com/projects/.../providers/... - Defaults to sts.amazonaws.com when empty. + Leaving this empty does not skip audience matching: both the trust policy + condition and the OIDC provider client ID list fall back to the literal + string sts.amazonaws.com. EOT type = string default = "" + + # Same IAM policy-variable expansion hazard as oidc_subject_claim: an + # audience of ${accounts.google.com:aud} expands to the token's own aud + # claim and matches every token. Audience is the only other control on this + # trust policy, so it gets the same guard, including the whitespace + # rejection that keeps it equivalent to the CloudFormation AllowedPattern + # ^$|^[^\s*$]+$. An empty audience still passes (it contains none of the + # rejected characters) and falls back to sts.amazonaws.com, matching that + # pattern's ^$ branch without needing an explicit empty-string clause. + validation { + condition = !can(regex("[\\s*$]", var.oidc_audience)) + error_message = "oidc_audience must not contain whitespace, '$' or '*'. IAM expands $${...} policy variables inside Condition values, so a value such as $${accounts.google.com:aud} would expand to the token's own aud claim and match every token. '*' is compared literally by StringEquals." + } } variable "oidc_subject_claim" { description = <<-EOT Subject (sub) claim used to restrict OIDC trust to a specific identity. Azure AD managed identity: the object ID of the managed identity. - GCP service account: the service account email in the form - system:serviceaccount::. + GCP service account: the service account's numeric unique ID + (typically 21 digits; Google documents it as a + numeric string without guaranteeing a length). + That is the sub claim accounts.google.com + actually issues; it is not the SA email. The + system:serviceaccount:: form + is the Kubernetes subject format and belongs to + a cluster OIDC issuer, not accounts.google.com. This variable is required and must not be empty. An empty value would allow any principal in the same OIDC provider tenant to assume this role. EOT @@ -40,6 +61,21 @@ variable "oidc_subject_claim" { condition = var.oidc_subject_claim != null && length(trimspace(var.oidc_subject_claim)) > 0 error_message = "oidc_subject_claim must be set to a non-empty subject claim. Leaving it empty would allow any principal in the OIDC provider tenant to assume this role." } + + # assume_role_policy is an IAM policy document, and IAM expands ${...} policy + # variables inside Condition values. A subject of ${accounts.google.com:sub} + # expands to the presented token's own sub claim, making the condition a + # tautology that matches every identity the issuer can mint. Reject '$' + # outright rather than emit a trust policy that only looks restricted. + # Whitespace is rejected in the same expression so this stays byte-for-byte + # equivalent to the CloudFormation AllowedPattern ^[^\s*$]+$ that the + # template's ConstraintDescription claims to mirror. No real OIDC subject + # (GCP numeric IDs, Azure GUIDs, system:serviceaccount:, GitHub, GitLab, + # SPIFFE, Auth0, Bitbucket) contains whitespace. + validation { + condition = !can(regex("[\\s*$]", var.oidc_subject_claim)) + error_message = "oidc_subject_claim must not contain whitespace, '$' or '*'. IAM expands $${...} policy variables inside Condition values, so a value such as $${accounts.google.com:sub} would expand to the token's own sub claim and match every identity the issuer can mint. '*' is compared literally by StringEquals and would silently produce a role nobody can assume." + } } variable "role_name" { diff --git a/known_issues/13_iac_aws_target.md b/known_issues/13_iac_aws_target.md index e8c22f2e8..be03f93e0 100644 --- a/known_issues/13_iac_aws_target.md +++ b/known_issues/13_iac_aws_target.md @@ -1,6 +1,84 @@ # Known Issues: IaC AWS Target Federation -> **Audit status (2026-04-20):** `0 still valid · 7 resolved · 0 partially fixed · 0 moved · 0 needs triage` +> **Audit status (2026-07-28):** `1 still valid · 8 resolved · 0 partially fixed · 0 moved · 0 needs triage` + +## CRITICAL: federation bundle generator never emits `OIDCSubjectClaim` + +**File**: + +- `internal/api/handler_federation.go` (`buildCFParamsJSON`, and the + `format=cli` bundle selector at `:401`/`:415`) +- `internal/iacfiles/templates/aws-wif-cf-params.json.tmpl` +- `internal/iacfiles/templates/aws-wif-cli.sh.tmpl:17` (documents + `OIDC_SUBJECT_CLAIM` as Optional) and `:57-70` (else branch builds the role + with only the `:aud` condition and no `:sub`) +- `internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl:34-38`, the + `--parameter-overrides` list in the script that deploys the CloudFormation + bundle; it does not pass `OIDCSubjectClaim` +- `internal/iacfiles/templates/aws-wif.tfvars.tmpl:14-15`, which emits + `# oidc_subject_claim = ""` labelled Optional, contradicting the REQUIRED + `oidc_subject_claim` variable in `terraform/variables.tf:28-42` + +**Description**: The customer-facing onboarding bundle writes CloudFormation +parameter overrides for `OIDCIssuerURL`, `OIDCIssuerHost`, `OIDCAudience` and +`RoleName`, but never `OIDCSubjectClaim`. `aws-wif-cli.sh.tmpl` has the same +shape: it documents `OIDC_SUBJECT_CLAIM` as "Optional" and builds a trust +policy with no `:sub` condition when the variable is unset. Both paths +therefore produce the unrestricted trust policy that issue #1543 removed from +the template itself. The tfvars template repeats the claim that the subject is +optional, and the deploy script never forwards the parameter. +**Impact**: Now that `OIDCSubjectClaim` is a required template parameter, the +generated CloudFormation bundle fails at change-set creation with +`Parameters: [OIDCSubjectClaim] must have values` — fail-closed, but the +onboarding flow is broken until the generator and `aws-cfn-deploy.sh.tmpl` are +taught to emit the subject. The `aws-wif-cli.sh.tmpl` path does not go through +CloudFormation at all: `format=cli` is a first-class user-selectable download, +so a customer choosing the CLI bundle still gets exactly the subject-less +role #1543 describes. The tfvars path is fail-closed, but not for the reason +one might assume: with `oidc_subject_claim` commented out there is no value +for a `validation` block to inspect, so the validations never fire. What makes +it fail-closed is the **absent default** on the variable, which makes +Terraform prompt for the value interactively or hard-error under +`-input=false`. The validations only apply once a value exists. The security +property holds; the operator is still misled by the "Optional" label first. +**Status:** ⚠️ Still valid — tracked as a follow-up to #1543 in #1640, and not +fixed in that PR because `internal/` was owned by concurrent in-flight +branches. + +## ~~CRITICAL: CloudFormation `:sub` restriction is optional and defaults to trusting every issuer identity~~ — RESOLVED + +**File**: `iac/federation/aws-target/cloudformation/template.yaml:52-57`, `:64-66`, `:171-178` +**Description**: `OIDCSubjectClaim` defaulted to `""` and a `HasSubject` +condition selected a trust statement that omitted the `:sub` condition +entirely, gating only on `:aud`. With `OIDCAudience` also left at its +default the audience collapsed to the literal `sts.amazonaws.com`, so any +identity the issuer could mint — for the documented `accounts.google.com` +issuer, any Google service account anywhere — could call +`sts:AssumeRoleWithWebIdentity` and obtain `ec2:PurchaseReservedInstancesOffering`, +`savingsplans:CreateSavingsPlan` and `rds:PurchaseReservedDBInstancesOffering` +in the customer's account. +**Impact**: Unauthenticated-in-practice takeover of the commitment-purchasing +role in every customer account deployed with the default parameters. The +purchases are irreversible multi-year spend. +**Status:** ✔️ Resolved + +**Resolved by:** #1543 — removes the `HasSubject` condition and the +subject-less trust statement, and makes `OIDCSubjectClaim` a required +parameter (no `Default`, `MinLength: 1`, `AllowedPattern` rejecting +any whitespace, `*` or `$`), mirroring the Terraform module's +`oidc_subject_claim` validation. + +`$` is rejected because `AssumeRolePolicyDocument` is an IAM policy document +and IAM expands `${...}` policy variables inside `Condition` values. A subject +of `${accounts.google.com:sub}` (a documented web-identity policy variable for +the `accounts.google.com` issuer this template targets) expands to the +presented token's own `sub` claim, making the condition a tautology that +matches every identity the issuer can mint. An operator could reach that value +by copy-pasting a placeholder or by running a templating layer that failed to +interpolate, reconstructing the #1543 hole through the guard added to close +it. The same guard is applied to `OIDCAudience` and to the Terraform +`oidc_subject_claim` / `oidc_audience` variables, since audience is the only +other control on this trust policy. ## ~~CRITICAL: Terraform trust policy silently drops `:aud` condition when `oidc_subject_claim` is set~~ — RESOLVED @@ -20,6 +98,11 @@ **Resolved by:** `a96fc719f` — adds an `OIDCSubjectClaim` parameter and conditional `:sub` `StringEquals` alongside the existing `:aud` check. +> **Follow-up:** the *conditional* form this entry describes was itself the +> CRITICAL hole filed as #1543 — the parameter defaulted to `""` and the +> condition then selected a subject-less trust statement. The `:sub` check is +> now unconditional; see the #1543 entry above. + ## ~~HIGH: `OIDCThumbprint` parameter has no format validation in CloudFormation~~ — RESOLVED **File**: `iac/federation/aws-target/cloudformation/template.yaml:33-40` @@ -88,7 +171,7 @@ **Description**: A whitespace-only audience value previously created a trust policy that no token matches. **Status:** ✔️ Resolved -**Resolved by:** Added `AllowedPattern: "^$|^\\S$|^\\S.*\\S$"` and `ConstraintDescription` to the `OIDCAudience` parameter. Empty strings remain allowed (HasAudience stays false); whitespace-only and leading/trailing-whitespace values are now rejected at change-set creation time. +**Resolved by:** Added `AllowedPattern` and `ConstraintDescription` to the `OIDCAudience` parameter. Empty strings remain allowed (HasAudience stays false); whitespace-only and leading/trailing-whitespace values are now rejected at change-set creation time. #1543 tightened the pattern to `^$|^[^\\s*$]+$`, additionally rejecting `*`, `$` and interior whitespace for the IAM policy-variable-expansion reason described in the #1543 entry above. ### Original implementation plan