From 902eb83c84fc0fed7da4d956d6a9aa74a0641937 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Jul 2026 18:08:00 +0200 Subject: [PATCH 1/3] sec(iac/aws): require OIDC subject claim in aws-target CloudFormation The OIDCSubjectClaim parameter 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. Customers deploy this template into their own AWS accounts. With the documented accounts.google.com issuer and the default parameters, any Google service account anywhere could mint an ID token with audience=sts.amazonaws.com, present it to sts:AssumeRoleWithWebIdentity, and land in the customer account holding ec2:PurchaseReservedInstancesOffering, savingsplans:CreateSavingsPlan and rds:PurchaseReservedDBInstancesOffering. Those are irreversible multi-year spend commitments, reachable by anyone who could read the role ARN. The Terraform sibling already refuses this: oidc_subject_claim has no default and carries a validation rejecting empty values. Bring the CloudFormation path to parity. - Drop the HasSubject condition and the subject-less trust statement, so the :sub condition is always present. - Make OIDCSubjectClaim required: no Default, MinLength 1, and an AllowedPattern rejecting whitespace-only values and any '*'. The trust policy matches with StringEquals, so a '*' is compared literally and would silently produce a role nobody can assume; reject it loudly rather than deploy a dead role. - Record the finding and the still-open bundle-generator gap in known_issues/13_iac_aws_target.md, and correct the stale "RESOLVED" note that described the conditional :sub as the fix. BREAKING: stacks deployed with the blank default now fail to update until OIDCSubjectClaim is supplied. That is deliberate; the alternative is leaving an open trust policy in place. The rendered trust policy for stacks that already set a subject is byte-identical. Closes #1543 --- .../aws-target/cloudformation/template.yaml | 53 ++++++++++--------- known_issues/13_iac_aws_target.md | 49 ++++++++++++++++- 2 files changed, 77 insertions(+), 25 deletions(-) diff --git a/iac/federation/aws-target/cloudformation/template.yaml b/iac/federation/aws-target/cloudformation/template.yaml index 1a67b1b4e..5432fdf60 100644 --- a/iac/federation/aws-target/cloudformation/template.yaml +++ b/iac/federation/aws-target/cloudformation/template.yaml @@ -52,9 +52,22 @@ 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 unique ID, or the form + system:serviceaccount::. + Mirrors the required oidc_subject_claim variable in the Terraform module. + MinLength: 1 + AllowedPattern: "^[^\\s*]$|^[^\\s*][^*]*[^\\s*]$" + ConstraintDescription: > + OIDCSubjectClaim is required and must be a non-empty string with no + leading/trailing whitespace and no '*'. The trust policy matches the + subject with StringEquals, so a '*' is compared literally rather than + as a wildcard and would silently produce a role nobody can assume. RoleName: Type: String @@ -63,7 +76,6 @@ Parameters: Conditions: HasAudience: !Not [!Equals [!Ref OIDCAudience, ""]] - HasSubject: !Not [!Equals [!Ref OIDCSubjectClaim, ""]] Resources: OIDCProvider: @@ -156,26 +168,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/known_issues/13_iac_aws_target.md b/known_issues/13_iac_aws_target.md index e8c22f2e8..ca217a371 100644 --- a/known_issues/13_iac_aws_target.md +++ b/known_issues/13_iac_aws_target.md @@ -1,6 +1,48 @@ # 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`), `internal/iacfiles/templates/aws-wif-cf-params.json.tmpl`, `internal/iacfiles/templates/aws-wif-cli.sh.tmpl` +**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. +**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 is taught to emit the subject. +The `aws-wif-cli.sh.tmpl` path does not go through CloudFormation and still +creates a subject-less role. +**Status:** ⚠️ Still valid — tracked as a follow-up to #1543, 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 +whitespace-only and `*` values), mirroring the Terraform module's +`oidc_subject_claim` validation. ## ~~CRITICAL: Terraform trust policy silently drops `:aud` condition when `oidc_subject_claim` is set~~ — RESOLVED @@ -20,6 +62,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` From c945995d03a81c57d9d6af64074509bf4b81dd59 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Jul 2026 21:00:11 +0200 Subject: [PATCH 2/3] sec(iac/aws): reject IAM policy variables in OIDC subject and audience The AllowedPattern added for OIDCSubjectClaim rejected whitespace and '*' but still admitted '$'. That left the guard fail-open. AssumeRolePolicyDocument is an IAM policy document (Version 2012-10-17), and IAM expands ${...} policy variables inside Condition VALUES, not just in Resource ARNs. AWS documents ${accounts.google.com:sub} and ${accounts.google.com:aud} as valid web-identity policy variables for exactly the accounts.google.com issuer this template documents. So an operator who sets: OIDCSubjectClaim=${accounts.google.com:sub} renders the condition: "accounts.google.com:sub": "${accounts.google.com:sub}" which IAM expands to the presented token's own sub claim. The condition compares every subject to itself, is satisfied by every token the issuer can mint, and reconstructs the #1543 hole through the very parameter added to close it. The value is reachable by copy-pasting a placeholder, by a templating layer that failed to interpolate, or by a generator emitting an unresolved variable. Unlike '*', which StringEquals compares literally and which therefore only yields a dead role (fail-closed), this one fails OPEN. Reject '$' in both parameters: OIDCSubjectClaim ^[^\s*$]$|^[^\s*$][^*$]*[^\s*$]$ OIDCAudience ^$|^[^\s*$]$|^[^\s*$][^*$]*[^\s*$]$ No legitimate subject or audience format contains '$': GCP 21-digit numeric unique IDs, Azure object-ID GUIDs, system:serviceaccount::, repo:org/repo:ref:refs/heads/main, api://, and the GCP workloadIdentityPools audience URL all pass. Audience is hardened in the same pass because it is now the only other control on this trust policy. The Terraform sibling had the identical hole: oidc_subject_claim only validated non-emptiness, and oidc_audience had no validation at all. Both now reject '$' and '*' too, so the two bundles stay at parity. Also correct two documentation errors this PR is responsible for: - The GCP subject format was given as system:serviceaccount:: . That is the Kubernetes subject format, belongs to a cluster OIDC issuer rather than accounts.google.com, and its field names are wrong for it. For accounts.google.com the sub claim is the service account's 21-digit numeric unique ID. - OIDCAudience claimed "leave empty to skip audience matching". Empty does not skip anything: the condition and the OIDC provider ClientIdList both pin to the literal sts.amazonaws.com. Scope: this closes the CloudFormation and Terraform bundles. The CLI bundle remains open. internal/iacfiles/templates/aws-wif-cli.sh.tmpl still treats OIDC_SUBJECT_CLAIM as optional and its else branch creates the role with only the :aud condition, and format=cli is a user-selectable download in internal/api/handler_federation.go. internal/ is owned by concurrent branches, so that path plus the aws-cfn-deploy.sh.tmpl and aws-wif.tfvars.tmpl gaps are filed as #1640 and recorded in known_issues/13_iac_aws_target.md. Verification: the two AllowedPattern values were parsed back out of the committed YAML and checked against 24 inputs covering every legitimate subject and audience format plus every '$' and '*' form; terraform validate, fmt, tflint and markdownlint all clean; the new Terraform validations were exercised with terraform plan and reject the tautology while accepting a real GCP subject. cfn-lint cannot lint this template (exit 2, E0000 unhashable key on the Fn::Sub condition keys, pre-existing since bf19197ea and the reason check-yaml excludes this path). Refs #1543 Refs #1640 --- .../aws-target/cloudformation/template.yaml | 39 +++++++++++---- .../aws-target/terraform/variables.tf | 31 ++++++++++-- known_issues/13_iac_aws_target.md | 49 +++++++++++++++---- 3 files changed, 98 insertions(+), 21 deletions(-) diff --git a/iac/federation/aws-target/cloudformation/template.yaml b/iac/federation/aws-target/cloudformation/template.yaml index 5432fdf60..8c53d3988 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*$]$|^[^\\s*$][^*$]*[^\\s*$]$" + ConstraintDescription: > + OIDCAudience must be empty, or a non-whitespace string with no + leading/trailing whitespace and containing no '$' or '*'. + '$' 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 @@ -58,16 +69,26 @@ Parameters: 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 unique ID, or the form - system:serviceaccount::. + GCP service account: the service account's 21-digit numeric unique ID. + 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*]$|^[^\\s*][^*]*[^\\s*]$" + AllowedPattern: "^[^\\s*$]$|^[^\\s*$][^*$]*[^\\s*$]$" ConstraintDescription: > OIDCSubjectClaim is required and must be a non-empty string with no - leading/trailing whitespace and no '*'. The trust policy matches the - subject with StringEquals, so a '*' is compared literally rather than - as a wildcard and would silently produce a role nobody can assume. + leading/trailing whitespace and containing no '$' or '*'. + '$' 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 diff --git a/iac/federation/aws-target/terraform/variables.tf b/iac/federation/aws-target/terraform/variables.tf index d35bd8865..fe4bd2092 100644 --- a/iac/federation/aws-target/terraform/variables.tf +++ b/iac/federation/aws-target/terraform/variables.tf @@ -19,18 +19,33 @@ 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. + validation { + condition = !can(regex("[*$]", var.oidc_audience)) + error_message = "oidc_audience must not contain '$' 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 21-digit numeric unique + ID. 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 +55,16 @@ 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. + validation { + condition = !can(regex("[*$]", var.oidc_subject_claim)) + error_message = "oidc_subject_claim must not contain '$' 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 ca217a371..40598d783 100644 --- a/known_issues/13_iac_aws_target.md +++ b/known_issues/13_iac_aws_target.md @@ -4,22 +4,41 @@ ## CRITICAL: federation bundle generator never emits `OIDCSubjectClaim` -**File**: `internal/api/handler_federation.go` (`buildCFParamsJSON`), `internal/iacfiles/templates/aws-wif-cf-params.json.tmpl`, `internal/iacfiles/templates/aws-wif-cli.sh.tmpl` +**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 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 is taught to emit the subject. -The `aws-wif-cli.sh.tmpl` path does not go through CloudFormation and still -creates a subject-less role. -**Status:** ⚠️ Still valid — tracked as a follow-up to #1543, and not fixed in -that PR because `internal/` was owned by concurrent in-flight branches. +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 (Terraform's variable +validation rejects the omission) but misleads the operator 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 @@ -41,9 +60,21 @@ purchases are irreversible multi-year spend. **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 -whitespace-only and `*` values), mirroring the Terraform module's +whitespace-only values and any `*` 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 **File**: `iac/federation/aws-target/terraform/main.tf:120-131` @@ -135,7 +166,7 @@ whitespace-only and `*` values), mirroring the Terraform module's **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*$]$|^[^\\s*$][^*$]*[^\\s*$]$`, additionally rejecting `*` and `$` for the IAM policy-variable-expansion reason described in the #1543 entry above. ### Original implementation plan From b5655644770b23d336239da591bc79f34b2073a0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 28 Jul 2026 21:22:12 +0200 Subject: [PATCH 3/3] sec(iac/aws): reject whitespace in OIDC subject and audience Follow-up polish on the policy-variable guard. Two independent reviews converged on the same residual: the middle character class [^*$] still permitted interior whitespace and newlines, so "a b" and "ab\ncd" passed. That was never a vulnerability. Such a value contains no '$' or '*', so no IAM policy-variable expansion occurs, and CloudFormation JSON-escapes it into the policy document, so no injection occurs. No real token carries a sub or aud claim with embedded whitespace, which means the result is simply a subject nothing equals: a dead role, fail-closed. Both reviews correctly declined to call it a finding. It is tightened here only because a push was happening anyway, and it removes the last open question either review raised at zero cost. Both patterns collapse to a single anchored character class: OIDCSubjectClaim ^[^\s*$]+$ OIDCAudience ^$|^[^\s*$]+$ The three-branch form they replace is exactly equivalent once the middle class matches the edge classes, so this is a simplification rather than a second behaviour change; the verification asserts that equivalence over the full input corpus rather than assuming it. Checked against 18 real subject and audience formats, none of which contains whitespace: GCP numeric unique IDs, Azure object-ID GUIDs and api:// audiences, Kubernetes system:serviceaccount:, GitHub Actions repo: and environment forms, GitLab project_path:, SPIFFE IDs, Auth0 pipe and federated forms, Bitbucket braced UUIDs, the GCP workload-identity-pool provider URL, LDAP-style DNs, URNs and sts.amazonaws.com. The Terraform validations are widened in step, from [*$] to [\s*$], so the module stays equivalent to the AllowedPattern that the template's ConstraintDescription claims to mirror. An empty audience still passes without an explicit empty-string clause, because it contains none of the rejected characters; that is stated in the comment rather than encoded as a redundant condition. Also corrected, all three found by review: - The GCP subject was documented as a "21-digit" numeric unique ID. Google documents uniqueId as a numeric string with no guaranteed length, so an operator holding a 20-digit ID could conclude they had the wrong value. Now "numeric unique ID (typically 21 digits)". Same class of defect as the Kubernetes-format error corrected in the previous commit: a precise-sounding but slightly wrong instruction on a required security parameter. - known_issues/13_iac_aws_target.md stated the tfvars path is fail-closed because "Terraform's variable validation rejects the omission". 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, which makes Terraform prompt interactively or hard-error under -input=false. The security property holds; the stated mechanism did not. - Refreshed the two stale AllowedPattern citations in the same file. Verification: both patterns were re-parsed out of the committed YAML and checked over 43 inputs (18 legitimate formats, 22 rejections, plus the empty-string asymmetry), 0 mismatches; the simplified and three-branch forms proved identical across that corpus; terraform validate, fmt, tflint and markdownlint all exit 0; the widened Terraform validations were exercised with terraform plan and now reject the tautology, '*' and interior whitespace for BOTH variables while accepting real subjects and an empty audience. The audience validation was caught still un-widened by that plan run rather than by inspection. Known engine divergence, fail-closed and recorded rather than fixed: Java's \s is ASCII-only while Python's is Unicode-aware, so a value containing a non-breaking or em space is rejected by the local verification but would be accepted by CloudFormation. It carries no '$' or '*', so it still cannot expand a policy variable, and it still matches no real claim. Refs #1543 Refs #1640 --- .../aws-target/cloudformation/template.yaml | 24 ++++++++++-------- .../aws-target/terraform/variables.tf | 25 +++++++++++++------ known_issues/13_iac_aws_target.md | 13 +++++++--- 3 files changed, 40 insertions(+), 22 deletions(-) diff --git a/iac/federation/aws-target/cloudformation/template.yaml b/iac/federation/aws-target/cloudformation/template.yaml index 8c53d3988..f1d093de6 100644 --- a/iac/federation/aws-target/cloudformation/template.yaml +++ b/iac/federation/aws-target/cloudformation/template.yaml @@ -39,10 +39,10 @@ Parameters: literal string sts.amazonaws.com, so only tokens carrying exactly that audience are accepted. Default: "" - AllowedPattern: "^$|^[^\\s*$]$|^[^\\s*$][^*$]*[^\\s*$]$" + AllowedPattern: "^$|^[^\\s*$]+$" ConstraintDescription: > - OIDCAudience must be empty, or a non-whitespace string with no - leading/trailing whitespace and containing no '$' or '*'. + 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 @@ -69,17 +69,19 @@ Parameters: 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 21-digit numeric unique ID. - 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. + 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*$]$|^[^\\s*$][^*$]*[^\\s*$]$" + AllowedPattern: "^[^\\s*$]+$" ConstraintDescription: > - OIDCSubjectClaim is required and must be a non-empty string with no - leading/trailing whitespace and containing no '$' or '*'. + 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 diff --git a/iac/federation/aws-target/terraform/variables.tf b/iac/federation/aws-target/terraform/variables.tf index fe4bd2092..47ad4ec84 100644 --- a/iac/federation/aws-target/terraform/variables.tf +++ b/iac/federation/aws-target/terraform/variables.tf @@ -29,10 +29,14 @@ variable "oidc_audience" { # 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. + # 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("[*$]", var.oidc_audience)) - error_message = "oidc_audience must not contain '$' 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." + 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." } } @@ -40,8 +44,10 @@ 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's 21-digit numeric unique - ID. That is the sub claim accounts.google.com + 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 @@ -61,9 +67,14 @@ variable "oidc_subject_claim" { # 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("[*$]", var.oidc_subject_claim)) - error_message = "oidc_subject_claim must not contain '$' 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." + 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." } } diff --git a/known_issues/13_iac_aws_target.md b/known_issues/13_iac_aws_target.md index 40598d783..be03f93e0 100644 --- a/known_issues/13_iac_aws_target.md +++ b/known_issues/13_iac_aws_target.md @@ -34,8 +34,13 @@ 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 (Terraform's variable -validation rejects the omission) but misleads the operator first. +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. @@ -60,7 +65,7 @@ purchases are irreversible multi-year spend. **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 -whitespace-only values and any `*` or `$`), mirroring the Terraform module's +any whitespace, `*` or `$`), mirroring the Terraform module's `oidc_subject_claim` validation. `$` is rejected because `AssumeRolePolicyDocument` is an IAM policy document @@ -166,7 +171,7 @@ other control on this trust policy. **Description**: A whitespace-only audience value previously created a trust policy that no token matches. **Status:** ✔️ Resolved -**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*$]$|^[^\\s*$][^*$]*[^\\s*$]$`, additionally rejecting `*` and `$` for the IAM policy-variable-expansion reason described in the #1543 entry above. +**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