sec(iac/aws): require OIDC subject claim in aws-target CloudFormation - #1602
Conversation
The OIDCSubjectClaim parameter defaulted to "" and a HasSubject condition selected a trust statement that omitted the <issuer>:sub condition entirely, gating only on <issuer>: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
cristim
added
priority/p0
📝 WalkthroughWalkthroughCloudFormation now requires and validates ChangesAWS OIDC trust hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@iac/federation/aws-target/cloudformation/template.yaml`:
- Around line 55-70: Update the CloudFormation bundle generation flow in
handler_federation.go, including its input path, to accept and emit the
validated OIDC subject claim as the OIDCSubjectClaim override. Ensure generated
AWS WIF bundles always provide a non-empty, whitespace-trimmed, wildcard-free
subject claim, and do not add an empty or optional default.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1944ef7-acdf-494e-bb27-74916bcb93c4
📒 Files selected for processing (2)
iac/federation/aws-target/cloudformation/template.yamlknown_issues/13_iac_aws_target.md
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:<ns>:<name>,
repo:org/repo:ref:refs/heads/main, api://<client_id>, 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:<project>:
<sa-email>. 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 bf19197 and the reason check-yaml excludes this path).
Refs #1543
Refs #1640
|
Pushed The Changes:
Both patterns were re-parsed out of the committed YAML and checked against 24 inputs (table in the PR body); every legitimate subject/audience format passes and every Note for review: @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== PR metadata ==='
gh pr view 1602 --json number,title,body,baseRefName,headRefName,commits --jq '{number,title,baseRefName,headRefName,commits:[.commits[]|{oid,messageHeadline}]}'
echo '=== Changed files ==='
git diff --name-status HEAD^ HEAD
echo '=== Commit diff ==='
git diff --find-renames --unified=80 HEAD^ HEAD -- \
iac/federation/aws-target/cloudformation/template.yaml \
known_issues/13_iac_aws_target.md \
':(glob)**/*.tf' \
':(glob)**/*.tmpl' \
.pre-commit-config.yaml
echo '=== Candidate source files ==='
fd -t f -a . | rg '(template\.yaml|variables\.tf|aws-wif-cli\.sh\.tmpl|aws-cfn-deploy\.sh\.tmpl|aws-wif\.tfvars\.tmpl|13_iac_aws_target\.md|\.pre-commit-config\.yaml)$'
echo '=== OIDC parameter/variable and trust-policy references ==='
rg -n -C 8 --glob '*.yaml' --glob '*.yml' --glob '*.tf' --glob '*.tmpl' --glob '*.md' \
'(OIDCSubjectClaim|OIDCAudience|oidc_subject_claim|oidc_audience|AssumeRoleWithWebIdentity|AssumeRolePolicyDocument|accounts\.google\.com:sub|aws-wif-cli)' \
iac known_issues 2>/dev/null || true
echo '=== Related issue status ==='
gh issue view 1543 --json number,state,title,url,body --jq '{number,state,title,url,body}'
gh issue view 1640 --json number,state,title,url,body --jq '{number,state,title,url,body}'Length of output: 88682 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="$(git merge-base origin/main HEAD 2>/dev/null || git merge-base main HEAD)"
echo "=== Merge base: $base ==="
echo '=== Aggregate PR files ==='
git diff --name-status "$base" HEAD
echo '=== Aggregate PR diff ==='
git diff --find-renames --check "$base" HEAD
git diff --find-renames --unified=35 "$base" HEAD -- \
iac/federation/aws-target/cloudformation/template.yaml \
iac/federation/aws-target/terraform/main.tf \
iac/federation/aws-target/terraform/variables.tf \
known_issues/13_iac_aws_target.md
echo '=== CloudFormation trust-policy and parameter context ==='
nl -ba iac/federation/aws-target/cloudformation/template.yaml | sed -n '1,220p'
echo '=== Terraform module outline and trust-policy context ==='
ast-grep outline iac/federation/aws-target/terraform --items all --type variable,resource,locals
nl -ba iac/federation/aws-target/terraform/main.tf | sed -n '1,180p'
nl -ba iac/federation/aws-target/terraform/variables.tf | sed -n '1,100p'
echo '=== Current generated bundle interfaces (out-of-scope gap verification) ==='
nl -ba internal/iacfiles/templates/aws-wif-cli.sh.tmpl | sed -n '1,115p'
nl -ba internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl | sed -n '1,90p'
nl -ba internal/iacfiles/templates/aws-wif.tfvars.tmpl | sed -n '1,70p'
rg -n -C 10 'func buildCFParamsJSON|buildCFParamsJSON\s*\(' internal/api/handler_federation.go
echo '=== Parameter regex test matrix using Python re (structural sanity check) ==='
python3 - <<'PY'
import re
patterns = {
"audience": r"^$|^[^\s*$]$|^[^\s*$][^*$]*[^\s*$]$",
"subject": r"^[^\s*$]$|^[^\s*$][^*$]*[^\s*$]$",
}
values = ["", "x", "abc", " abc", "abc ", " ", "\t", "${accounts.google.com:sub}",
"${accounts.google.com:aud}", "foo$bar", "foo*bar", "*", "$", "a\nb"]
for name, pattern in patterns.items():
print(name, [(repr(v), bool(re.fullmatch(pattern, v))) for v in values])
PYLength of output: 32725 🏁 Script executed: #!/bin/bash
set -euo pipefail
show_range() {
local file="$1" start="$2" end="$3"
echo "=== ${file}:${start}-${end} ==="
awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
}
show_range iac/federation/aws-target/cloudformation/template.yaml 25 210
show_range iac/federation/aws-target/terraform/main.tf 1 165
show_range iac/federation/aws-target/terraform/variables.tf 15 75
show_range internal/iacfiles/templates/aws-wif-cli.sh.tmpl 1 115
show_range internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl 1 90
show_range internal/iacfiles/templates/aws-wif.tfvars.tmpl 1 70
echo '=== buildCFParamsJSON context ==='
rg -n -C 14 'func buildCFParamsJSON|buildCFParamsJSON\s*\(' internal/api/handler_federation.go
echo '=== Regex matrix ==='
python3 - <<'PY'
import re
patterns = {
"audience": r"^$|^[^\s*$]$|^[^\s*$][^*$]*[^\s*$]$",
"subject": r"^[^\s*$]$|^[^\s*$][^*$]*[^\s*$]$",
}
values = ["", "x", "abc", " abc", "abc ", " ", "\t", "${accounts.google.com:sub}",
"${accounts.google.com:aud}", "foo$bar", "foo*bar", "*", "$", "a\nb"]
for name, pattern in patterns.items():
print(name)
for value in values:
print(f" {value!r}: {'accept' if re.fullmatch(pattern, value) else 'reject'}")
PY
echo '=== Existing PR review comments ==='
gh api repos/LeanerCloud/CUDly/pulls/1602/comments --paginate \
--jq '.[] | {path,line,side,body,user:.user.login}' || trueLength of output: 35975 🤖 web_search executed:
Length of output: 21047
No new actionable findings. The required, unconditional ✅ Action performedReview finished.
|
Note for anyone reading this PR's checks: the
|
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
|
Pushed Interior whitespace tightened. Both reviews independently noted the middle class That simplified form is provably identical to the three-branch form it replaces once the middle class matches the edge classes; the verification asserts the equivalence over the full corpus rather than assuming it. Re-parsed out of the committed YAML and checked over 43 inputs: 18 real subject/audience formats (GCP numeric IDs, Azure GUIDs and The Terraform validations widened from Three review findings also addressed:
One divergence recorded rather than fixed: Java's Gates: regex verification exit 0, structural YAML parse exit 0, Reminder for anyone reading the checks on this PR: the @coderabbitai review |
|
✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
iac/federation/aws-target/cloudformation/template.yaml (1)
66-93: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftWire the required subject through the bundle generators.
OIDCSubjectClaimis now mandatory, but the supplied generator and deployment paths still omit it. Generated CloudFormation bundles will fail withParameters: [OIDCSubjectClaim] must have valuesuntil the validated subject is added to the generation input and every CloudFormation override path; do not restore an empty default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@iac/federation/aws-target/cloudformation/template.yaml` around lines 66 - 93, Propagate the validated OIDC subject through all bundle-generation and deployment paths: update the generator input and every CloudFormation parameter-override path to supply OIDCSubjectClaim. Trace the existing subject value from validation through the relevant generator/deployment functions and preserve it unchanged; do not reintroduce an empty default or omit the parameter in any path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@iac/federation/aws-target/cloudformation/template.yaml`:
- Around line 66-93: Propagate the validated OIDC subject through all
bundle-generation and deployment paths: update the generator input and every
CloudFormation parameter-override path to supply OIDCSubjectClaim. Trace the
existing subject value from validation through the relevant generator/deployment
functions and preserve it unchanged; do not reintroduce an empty default or omit
the parameter in any path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5b8ccdb-041b-46d6-a4e9-5a365f2d2bbc
📒 Files selected for processing (3)
iac/federation/aws-target/cloudformation/template.yamliac/federation/aws-target/terraform/variables.tfknown_issues/13_iac_aws_target.md
1 participant
Refs #1543. Follow-up for the remaining attack surface: #1640.
iac/federation/aws-target/cloudformation/template.yamlmade the OIDC subject restriction optional:OIDCSubjectClaimdefaulted to"", and aHasSubjectcondition selected a trust statement that omitted the<issuer>:subcondition entirely, gating only on<issuer>:aud. WithOIDCAudiencealso left at its default, the audience collapsed to the literalsts.amazonaws.com.Customers deploy this template into their own AWS accounts. With the documented
accounts.google.comissuer and the default parameters, any Google service account anywhere could mint an ID token withaudience=sts.amazonaws.com, present it tosts:AssumeRoleWithWebIdentity, and land in the customer account holdingec2:PurchaseReservedInstancesOffering,savingsplans:CreateSavingsPlanandrds: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_claimhas no default and validates against empty values); this brings the CloudFormation path to parity.Changes
HasSubjectcondition and the subject-less trust statement, so the:subcondition is always present.OIDCSubjectClaimrequired: noDefault,MinLength: 1, and anAllowedPatternrejecting whitespace-only values and any*or$.$/*rejection toOIDCAudience, and to the Terraformoidc_subject_claimandoidc_audiencevariables.known_issues/13_iac_aws_target.md.Why
$had to be rejected: the guard was fail-openThe first version of the
AllowedPatternrejected whitespace and*but still admitted$. An adversarial review found that left the guard fail-open.AssumeRolePolicyDocumentis an IAM policy document (Version: 2012-10-17), and IAM expands${...}policy variables inside Condition values, not only in resource ARNs. AWS documents${accounts.google.com:sub}and${accounts.google.com:aud}as valid web-identity policy variables, for exactly theaccounts.google.comissuer this template documents. So an operator who sets:renders the condition:
IAM expands that to the presented token's own
subclaim. The condition compares every subject to itself: it is a tautology satisfied by every token the issuer can mint. That is the #1543 hole reconstructed 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.Contrast with
*, whichStringEqualscompares literally and which therefore only produces a dead role that nobody can assume.*fails closed;$fails open. Both are now rejected at submission time.Audience is hardened in the same pass because, with the subject now pinned,
audis the only other control on this trust policy.Verified pattern behaviour
Final patterns, both a single anchored character class:
Both were parsed back out of the committed YAML (not retyped) and checked as an anchored full match over 43 inputs, 0 mismatches. Every legitimate format passes; every whitespace,
$and*form is rejected.Legitimate formats, all PASS both patterns (18 checked): GCP service-account numeric unique IDs (21- and 20-digit), Azure object-ID GUIDs, Azure
api://<guid>, Kubernetessystem:serviceaccount:ns:name, GitHub Actionsrepo:org/repo:ref:refs/heads/mainandrepo:org/repo:environment:production, GitLabproject_path:group/project:ref_type:branch:ref:main, SPIFFEspiffe://cluster.local/ns/default/sa/my-app, Auth0auth0|5f7c...andgoogle-oauth2|1034..., Bitbucket{f7c8ec7c-...}, the GCP workload-identity-pool provider URL,sts.amazonaws.com, LDAP-style DNs, URNs, and a single character.All REJECTED by both patterns (22 checked):
" "," ", tab, newline, leading and trailing whitespace, interior space, interior newline, interior tab,*,a*b,*abc,abc*,$,$$,abc$def,${accounts.google.com:sub},${accounts.google.com:aud},${aws:userid},${aws:PrincipalTag/x},x${accounts.google.com:sub},${accounts.google.com:sub}x.Empty string: subject FAILS (required), audience PASSES (falls back to
sts.amazonaws.com).The simplified
^[^\s*$]+$is provably identical to the three-branch form it replaces (^[^\s*$]$|^[^\s*$][^\s*$]*[^\s*$]$) once the middle class matches the edge classes. The verification asserts that equivalence across the whole corpus rather than assuming it, so the simplification is not a second behaviour change.Interior whitespace was tightened, not left. Two independent reviews noted that the earlier middle class
[^*$]still admitteda bandab\ncd. Neither called it a finding, correctly: such a value carries no$or*, so no policy-variable expansion; CloudFormation JSON-escapes it, so no injection; and no realsub/audclaim contains whitespace, so the result is a subject nothing equals, i.e. a dead role. Fail-closed. It is tightened here only because a push was happening anyway, and none of the 18 real formats above contains whitespace.One recorded engine divergence, fail-closed. Java's
\sis ASCII-only while Python's is Unicode-aware, so a value containing a non-breaking space (U+00A0) or em space (U+2003) is rejected by the local verification but would be accepted by CloudFormation's Java engine. This is not claimed as blocked. It remains fail-closed for the same reason as above: no$or*means no policy-variable expansion, and no real claim contains those characters. This is a documentation-accuracy point, not a hole.One uncited claim, flagged rather than dressed up. The parameter documentation now says the GCP service-account subject is a "numeric unique ID (typically 21 digits; Google documents it as a numeric string without guaranteeing a length)". The no-guaranteed-length part was not sourced against Google's published documentation by anyone in this review chain. The wording is deliberately weaker than the "21-digit" assertion it replaces, so it holds whether or not a length guarantee exists somewhere, but it should not be read as a verified citation.
Attack surface: closed vs. still open
Closed by this PR:
iac/federation/aws-target/cloudformation/). The:subcondition is unconditional and the parameter cannot hold a tautology.iac/federation/aws-target/terraform/), which gains the same$/*rejection on bothoidc_subject_claimandoidc_audience.Why Terraform is fixed in the same PR rather than deferred. The Terraform sibling had the identical fail-open:
oidc_subject_claimvalidated only non-emptiness, andoidc_audiencehad no validation at all. The CloudFormationConstraintDescriptionexplicitly claims parity with it ("Mirrors the requiredoidc_subject_claimvariable in the Terraform module"). Fixing only the CloudFormation path would leave the same tautology exploitable through Terraform while the docs assert the two are equivalent, which is worse than fixing either both or neither, because a reader is actively misled about coverage. That is the same "sibling instance" shape that already bit this PR once: the CLI bundle still ships the vulnerable policy, which is exactly why the trailer is nowRefsrather thanCloses. Fixing one instance of a defect class while a named-as-equivalent sibling keeps it is the pattern to avoid. Blast radius is contained:iac/federation/aws-target/has no other open PR touching it.Still open, tracked in #1640:
internal/iacfiles/templates/aws-wif-cli.sh.tmpl:17still treatsOIDC_SUBJECT_CLAIMas optional, and its else branch (:57-70) creates the role with only the:audcondition and no:sub.format=cliis a first-class user-selectable download (internal/api/handler_federation.go:401,415), so a customer choosing the CLI bundle still gets exactly the role sec(iac): aws-target CloudFormation makes the OIDC sub restriction optional; the default trusts every identity from the issuer #1543 describes. This path never goes through CloudFormation, so nothing rejects it.internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl:34-38does not forwardOIDCSubjectClaim, so the CloudFormation onboarding flow is now broken (fail-closed) until the script is updated.internal/iacfiles/templates/aws-wif.tfvars.tmpl:14-15labelsoidc_subject_claimOptional, contradicting the required Terraform variable (fail-closed, but misleading).internal/is owned by concurrent in-flight branches, so it is deliberately untouched here. The trailer isRefs #1543, notCloses, so merging this does not auto-close a p0/CRITICAL whose exploit is still reachable through the CLI bundle.Breaking change
Stacks deployed with the blank default now fail to update until
OIDCSubjectClaimis 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.Existing deployments remain vulnerable until they redeploy with a subject claim set.
Second breaking surface: the audience and the Terraform variables. The
OIDCAudienceAllowedPatternwas tightened in the same pass, and the Terraform module gained validations onoidc_subject_claimandoidc_audience. So an existing stack, or an existing.tfvars, carrying whitespace,$or*in either value will now fail at change-set creation or atterraform planrather than applying. Realistically no such deployment exists (none of the 18 real subject/audience formats contains any of those characters, and a$value would have been a live tautology), but it is a second way this PR can break an existing deployment and it should not go unmentioned.Verification
AllowedPatterns re-parsed from the committed YAML and checked against 43 inputs (18 legitimate formats, 22 rejections, empty-string asymmetry):audand:sub;HasSubjectgone; subject has noDefault,MinLength: 1)terraform validate/terraform fmt -check/tflintterraform plan, both variables*and interior whitespace rejected; real GCP subject,system:serviceaccount:,sts.amazonaws.comand empty audience acceptedc945995d0(all 4 runs)CI - Build & Test,pre-commit, AWS Sanity, Azure Sanity: all successmarkdownlint(repo config)Do not read the
CodeRabbitcheck on this commit either way. Forc945995d0it reportsstate=successwithdescription="Review rate limited"(updated19:05:00Z), while a genuine full-range CodeRabbit review did run and posted at19:04:55Z. The status is unreliable in both directions here: green does not mean a review happened, and the "rate limited" text does not mean one did not. The truth lives only in the comment stream. This is the hazard documented in #1530.cfn-lintcannot lint this template: it exits 2 withE0000 Unhashable type "{'Fn::Sub': '${OIDCIssuerHost}:aud'}". That is a known cfn-lint limitation with intrinsics in mapping-key position, it is pre-existing sincebf19197ea(identical exit 2 on the parent commit's template), and it is why.pre-commit-config.yamlexcludesiac/.*/cloudformation/.*fromcheck-yaml. It is a tool limitation, not a template defect, and is not recorded as a passing gate. There is nocfn-lintorvalidate-templatestep anywhere in CI, so no automated check guards this file.Provenance
The original fix was authored and pushed but its PR was never opened, because the authoring session hit an account-level limit immediately afterwards. Recovered from the pushed branch rather than re-implemented. The
$fail-open gap was found by a subsequent independent adversarial review of that commit.Summary by CodeRabbit
New Features
sts.amazonaws.cominstead of disabling audience matching.Bug Fixes
Documentation