diff --git a/iac/federation/aws-target/cloudformation/template.yaml b/iac/federation/aws-target/cloudformation/template.yaml index f1d093de6..7b2546163 100644 --- a/iac/federation/aws-target/cloudformation/template.yaml +++ b/iac/federation/aws-target/cloudformation/template.yaml @@ -53,12 +53,46 @@ Parameters: OIDCThumbprint: Type: String Description: > - SHA-1 thumbprint of the OIDC provider root CA certificate. - For GCP (accounts.google.com) use: 08745487e891c19e3078c1f2a07e452950ef36f6 - For Azure AD use the thumbprint of the DigiCert Global Root CA. - Leave as zeros only for well-known providers where AWS auto-validates the chain. - Default: "0000000000000000000000000000000000000000" - AllowedPattern: "^[0-9a-fA-F]{40}$" + OPTIONAL. SHA-1 thumbprint (40 hex characters) of the top intermediate CA + that signed the TLS certificate of the issuer's JWKS endpoint. + LEAVE THIS EMPTY unless that certificate does not chain to a publicly + trusted CA. Empty omits the ThumbprintList property entirely, and IAM then + retrieves the correct thumbprint itself, which is what the IAM console + does by default. + AWS verifies the JWKS endpoint's TLS certificate against its own library of + trusted root CAs, and falls back to this thumbprint only when that + certificate does not chain to one of them, when AWS cannot retrieve the + certificate, or when the endpoint requires TLS 1.3. Both documented + issuers (login.microsoftonline.com and accounts.google.com) present + publicly trusted certificates, so for them the value is read only if one + of those fallback conditions applies, whatever it is set to. + The all-zeros placeholder that used to be the default is now rejected. It + does not disable chain verification. It is simply not the SHA-1 of any + certificate, so on the fallback path it matches nothing and every + AssumeRoleWithWebIdentity call fails. It nulls out the only check that + path has while reading like a deliberate configuration. + Note: when an issuer's discovery endpoint and jwks_uri are on different + hosts, AWS requires the thumbprints of BOTH. This parameter carries a + single value, so such an issuer must be onboarded with the AWS CLI or the + IAM console instead of this template. + Default: "" + MaxLength: 40 + # Empty, or up to 40 hex characters of which at least one is non-zero. + # The middle character class is what rejects the all-zeros placeholder: do + # not "simplify" this back to ^$|^[0-9a-fA-F]{40}$, which readmits it. + # MaxLength caps the upper end (the pattern alone would admit up to 79 + # characters). A well-formed-but-short value (1-39 hex) passes here and is + # rejected by the IAM API, which fixes the length at 40: fail-closed, with + # the stack rolling back rather than deploying a broken provider. + AllowedPattern: "^$|^[0-9a-fA-F]{0,39}[1-9a-fA-F][0-9a-fA-F]{0,39}$" + ConstraintDescription: > + OIDCThumbprint must be empty (recommended, since IAM then retrieves the + thumbprint itself), or a 40-character hex SHA-1 thumbprint containing at + least one non-zero digit. + The all-zeros placeholder is rejected because it is not the fingerprint of + any certificate: whenever AWS actually falls back to thumbprint + verification it matches nothing, so role assumption fails outright. Supply + the issuer's real thumbprint, or leave this empty. OIDCSubjectClaim: Type: String @@ -99,6 +133,26 @@ Parameters: Conditions: HasAudience: !Not [!Equals [!Ref OIDCAudience, ""]] + HasThumbprint: !Not [!Equals [!Ref OIDCThumbprint, ""]] + +# Rules are evaluated before CloudFormation creates or updates any resource; a +# failed assertion aborts the stack operation. This duplicates the all-zeros +# rejection already encoded in the OIDCThumbprint AllowedPattern on purpose: +# the two are independent, so neither a regex "simplification" nor a submission +# path that skips one of the layers can readmit the placeholder on its own. +Rules: + RejectPlaceholderThumbprint: + Assertions: + - Assert: !Not + - !Equals + - !Ref OIDCThumbprint + - "0000000000000000000000000000000000000000" + AssertDescription: > + OIDCThumbprint must not be the all-zeros placeholder. It is not the + SHA-1 fingerprint of any certificate, so on the one path where AWS + still consults a thumbprint it matches nothing and every + AssumeRoleWithWebIdentity call fails. Leave OIDCThumbprint empty to + have IAM retrieve the real thumbprint, or supply the issuer's own. Resources: OIDCProvider: @@ -107,8 +161,15 @@ Resources: Url: !Ref OIDCIssuerURL ClientIdList: - !If [HasAudience, !Ref OIDCAudience, sts.amazonaws.com] - ThumbprintList: - - !Ref OIDCThumbprint + # Omitted entirely when OIDCThumbprint is empty, so IAM retrieves and uses + # the issuer's real top intermediate CA thumbprint instead of whatever + # placeholder the operator was handed. ThumbprintList is optional on + # AWS::IAM::OIDCProvider and updates with no interruption, so switching + # between the two forms never replaces the provider or changes its ARN. + ThumbprintList: !If + - HasThumbprint + - [!Ref OIDCThumbprint] + - !Ref "AWS::NoValue" CUDlyPolicy: Type: AWS::IAM::ManagedPolicy diff --git a/iac/federation/aws-target/terraform/main.tf b/iac/federation/aws-target/terraform/main.tf index dff000d14..01f39e81e 100644 --- a/iac/federation/aws-target/terraform/main.tf +++ b/iac/federation/aws-target/terraform/main.tf @@ -23,9 +23,20 @@ locals { } resource "aws_iam_openid_connect_provider" "cudly" { - url = local.oidc_issuer_url_normalized - client_id_list = [local.audience] - thumbprint_list = var.thumbprint_list + url = local.oidc_issuer_url_normalized + client_id_list = [local.audience] + + # null, not [], when no thumbprint is configured: thumbprint_list is optional + # on this resource, and leaving it unset is what makes IAM retrieve and use + # the issuer's real top intermediate CA thumbprint. An empty list would + # instead send a provider that has no thumbprints at all. + # + # Note for existing deployments: the argument is Optional+Computed, so + # clearing it does not clear the value already stored on an existing + # provider -- Terraform keeps reading back what is there. A provider created + # with the old all-zeros default must be corrected out of band with + # `aws iam update-open-id-connect-provider-thumbprint`, or replaced. + thumbprint_list = length(var.thumbprint_list) > 0 ? var.thumbprint_list : null } resource "aws_iam_policy" "cudly" { diff --git a/iac/federation/aws-target/terraform/variables.tf b/iac/federation/aws-target/terraform/variables.tf index 47ad4ec84..c978ee7b1 100644 --- a/iac/federation/aws-target/terraform/variables.tf +++ b/iac/federation/aws-target/terraform/variables.tf @@ -86,20 +86,26 @@ variable "role_name" { variable "thumbprint_list" { description = <<-EOT - TLS root CA thumbprints for the OIDC provider (40-character hex SHA-1). - AWS auto-validates well-known providers (Azure AD, Google); for those the - all-zeros placeholder is intentional and accepted. - For any other issuer you MUST supply the real root CA SHA-1 thumbprint. - Supplying the all-zeros placeholder for a custom issuer is rejected by this - module to prevent operators from silently bypassing CA-chain validation. + OPTIONAL. TLS thumbprints (40-character hex SHA-1) of the top intermediate + CA that signed the certificate of the issuer's JWKS endpoint. + + LEAVE THIS EMPTY unless that certificate does not chain to a publicly + trusted CA. An empty list omits the argument entirely, and IAM then + retrieves the correct thumbprint itself. + + AWS verifies the JWKS endpoint's TLS certificate against its own library of + trusted root CAs, and falls back to these thumbprints only when that + certificate does not chain to one of them, when AWS cannot retrieve the + certificate, or when the endpoint requires TLS 1.3. Both documented + issuers (login.microsoftonline.com and accounts.google.com) present + publicly trusted certificates, so for them the value is read only if one of + those fallback conditions applies, whatever it is set to. + + When the issuer's discovery endpoint and jwks_uri are on different hosts, + AWS requires the thumbprints of BOTH; supply both entries in that case. EOT type = list(string) - default = ["0000000000000000000000000000000000000000"] - - validation { - condition = length(var.thumbprint_list) > 0 - error_message = "thumbprint_list must contain at least one thumbprint." - } + default = [] validation { condition = alltrue([ @@ -108,18 +114,22 @@ variable "thumbprint_list" { error_message = "Each thumbprint in thumbprint_list must be a 40-character SHA-1 hex string." } - # Guard against copy-paste of the all-zeros placeholder for custom OIDC - # issuers. AWS natively validates Azure AD and Google endpoints, so - # all-zeros is safe for those. Any other issuer URL requires a real CA - # thumbprint; the all-zeros value bypasses the CA-chain check entirely. + # The all-zeros placeholder used to be this variable's default, guarded by an + # issuer allowlist justified as "the all-zeros value bypasses the CA-chain + # check entirely". That justification was wrong in both directions, so the + # guard is now unconditional and the default is empty. + # + # All-zeros bypasses nothing: it is simply not the SHA-1 of any certificate. + # On the primary path AWS does not read it at all, and on the fallback path it + # matches nothing, so role assumption fails outright. Restricting it to Azure AD and + # Google was equally beside the point -- the thumbprint is unread for every + # publicly-trusted issuer, not only those two, and setting it for a + # private-CA issuer breaks that issuer no matter which one it is. validation { - condition = !( - length(var.thumbprint_list) == 1 && - var.thumbprint_list[0] == "0000000000000000000000000000000000000000" && - !startswith(var.oidc_issuer_url, "https://login.microsoftonline.com/") && - !startswith(var.oidc_issuer_url, "https://accounts.google.com") - ) - error_message = "thumbprint_list is the all-zeros placeholder, which is only safe for Azure AD (login.microsoftonline.com) and Google (accounts.google.com) issuers that AWS validates natively. For any other OIDC issuer you must supply the real root CA SHA-1 thumbprint." + condition = alltrue([ + for t in var.thumbprint_list : t != "0000000000000000000000000000000000000000" + ]) + error_message = "thumbprint_list must not contain the all-zeros placeholder. It is not the fingerprint of any certificate, so whenever AWS actually falls back to thumbprint verification it matches nothing and role assumption fails. Leave thumbprint_list empty to have IAM retrieve the issuer's real thumbprint, or supply that thumbprint." } } diff --git a/internal/api/handler_federation_test.go b/internal/api/handler_federation_test.go index 674f4289b..80bd90566 100644 --- a/internal/api/handler_federation_test.go +++ b/internal/api/handler_federation_test.go @@ -8,6 +8,8 @@ import ( "encoding/json" "io/fs" "os" + "regexp" + "strconv" "strings" "testing" @@ -15,6 +17,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" ) // federationHandler returns a Handler wired for federation IaC tests. @@ -1499,3 +1502,154 @@ func TestValidateFederationTargetSource(t *testing.T) { }) } } + +// --------------------------------------------------------------------------- +// aws-target OIDC provider thumbprint guard (issue #1615) +// --------------------------------------------------------------------------- + +const awsTargetCFNTemplate = "../../iac/federation/aws-target/cloudformation/template.yaml" + +// allZeroThumbprint is the placeholder that used to be the OIDCThumbprint +// default. It is not the SHA-1 fingerprint of any certificate, so on the one +// path where AWS still consults a thumbprint (the JWKS endpoint's TLS +// certificate does not chain to a CA in AWS's trusted-root library, AWS cannot +// retrieve that certificate, or the endpoint requires TLS 1.3) it matches +// nothing and every AssumeRoleWithWebIdentity call fails. +const allZeroThumbprint = "0000000000000000000000000000000000000000" + +// cfnParamBlock returns the lines of a top-level Parameters entry, from +// " :" up to the next sibling key at the same indent. +func cfnParamBlock(t *testing.T, src, name string) string { + t.Helper() + start := strings.Index(src, "\n "+name+":\n") + require.GreaterOrEqual(t, start, 0, "parameter %s not found in %s", name, awsTargetCFNTemplate) + rest := src[start+1:] + lines := strings.Split(rest, "\n") + var out []string + for i, line := range lines { + if i > 0 && len(line) > 2 && line[0] == ' ' && line[1] == ' ' && line[2] != ' ' { + break + } + out = append(out, line) + } + return strings.Join(out, "\n") +} + +// cfnScalar pulls a scalar property out of a parameter block and decodes it +// with the YAML decoder, so the value under test is exactly what CloudFormation +// receives rather than a copy retyped into this test. +func cfnScalar(t *testing.T, block, key string) string { + t.Helper() + for _, line := range strings.Split(block, "\n") { + trimmed := strings.TrimSpace(line) + if !strings.HasPrefix(trimmed, key+":") { + continue + } + var holder struct { + V string `yaml:"v"` + } + require.NoError(t, yaml.Unmarshal([]byte("v:"+strings.TrimPrefix(trimmed, key+":")), &holder), + "could not YAML-decode %s from %q", key, trimmed) + return holder.V + } + t.Fatalf("key %s not found in parameter block:\n%s", key, block) + return "" +} + +// TestAWSTargetTemplate_RejectsPlaceholderThumbprint is the regression test for +// issue #1615. The template used to default OIDCThumbprint to the all-zeros +// placeholder and validate it only as 40 hex characters, which that placeholder +// satisfies. +// +// The AllowedPattern and MaxLength are read back out of the committed template +// and evaluated the way CloudFormation evaluates them (anchored full match plus +// the length cap), so a future edit that relaxes either one fails here. +func TestAWSTargetTemplate_RejectsPlaceholderThumbprint(t *testing.T) { + raw, err := os.ReadFile(awsTargetCFNTemplate) + require.NoError(t, err) + src := string(raw) + + block := cfnParamBlock(t, src, "OIDCThumbprint") + + // The placeholder must not be reachable by leaving the parameter alone. + require.Equal(t, "", cfnScalar(t, block, "Default"), + "OIDCThumbprint must default to empty so ThumbprintList is omitted and IAM "+ + "retrieves the issuer's real thumbprint") + + pattern := cfnScalar(t, block, "AllowedPattern") + maxLen, err := strconv.Atoi(cfnScalar(t, block, "MaxLength")) + require.NoError(t, err, "MaxLength must be an integer") + + re, err := regexp.Compile(pattern) + require.NoError(t, err, "AllowedPattern must be a valid regular expression") + + // CloudFormation applies AllowedPattern as a full match and MaxLength + // independently; a value is accepted only when it clears both. + accepted := func(v string) bool { + return len(v) <= maxLen && re.FindString(v) == v && re.MatchString(v) + } + + mustAccept := []struct{ value, why string }{ + {"", "empty: ThumbprintList is omitted and IAM retrieves the real thumbprint"}, + {"08745487e891c19e3078c1f2a07e452950ef36f6", "a real root CA thumbprint"}, + {"990F4193972F2BECF12DDEDA5237F9C952F20D9E", "uppercase hex"}, + {"1" + strings.Repeat("0", 39), "non-zero digit in the first position only"}, + {strings.Repeat("0", 39) + "1", "non-zero digit in the last position only"}, + {strings.Repeat("0", 20) + "9" + strings.Repeat("0", 19), "non-zero digit in the middle only"}, + {strings.Repeat("0", 39) + "F", "non-zero position holding an uppercase hex letter"}, + {strings.Repeat("0", 39) + "a", "non-zero position holding a lowercase hex letter"}, + } + for _, tc := range mustAccept { + assert.True(t, accepted(tc.value), "must accept %q (%s)", tc.value, tc.why) + } + + mustReject := []struct{ value, why string }{ + {allZeroThumbprint, "the #1615 placeholder itself"}, + {strings.Repeat("0", 39), "all zeros, one character short"}, + {strings.Repeat("0", 41), "all zeros, one character long"}, + {"0", "a single zero"}, + {strings.Repeat("a", 41), "41 hex characters: caught by MaxLength, not the pattern"}, + {strings.Repeat("a", 40) + "\n", "trailing newline: MaxLength rejects it even if $ matched before it"}, + {strings.Repeat("g", 40), "not hexadecimal"}, + {"0x" + strings.Repeat("0", 38), "hex-literal prefix"}, + {" " + strings.Repeat("a", 39), "leading whitespace"}, + {strings.Repeat("a", 39) + " ", "trailing whitespace"}, + {"*", "wildcard"}, + {"${accounts.google.com:sub}", "an IAM policy variable"}, + } + for _, tc := range mustReject { + assert.False(t, accepted(tc.value), "must reject %q (%s)", tc.value, tc.why) + } +} + +// TestAWSTargetTemplate_ThumbprintWiring asserts the second, independent layer +// of the #1615 guard and the resource wiring it protects. The Rules assertion +// duplicates the AllowedPattern's rejection of the placeholder on purpose, so +// neither a regex "simplification" nor a submission path that skips one layer +// can readmit it alone. +func TestAWSTargetTemplate_ThumbprintWiring(t *testing.T) { + raw, err := os.ReadFile(awsTargetCFNTemplate) + require.NoError(t, err) + src := string(raw) + + assert.Contains(t, src, "Rules:", + "template must declare a Rules section; rules are evaluated before any resource is created") + assert.Equal(t, 1, strings.Count(src, allZeroThumbprint), + "the all-zeros placeholder must appear exactly once in the template: in the "+ + "Rules assertion that rejects it, and nowhere else") + assert.Contains(t, src, `HasThumbprint: !Not [!Equals [!Ref OIDCThumbprint, ""]]`, + "HasThumbprint must be defined as a non-empty OIDCThumbprint") + + // The empty branch must omit ThumbprintList entirely rather than send an + // empty or placeholder list, which is what makes IAM retrieve the real + // thumbprint for itself. + assert.Contains(t, src, `ThumbprintList: !If`, + "ThumbprintList must be conditional on HasThumbprint") + assert.Contains(t, src, `- !Ref "AWS::NoValue"`, + "the empty branch must resolve to AWS::NoValue so the property is omitted") + + // The placeholder must appear only inside the Rules assertion that rejects + // it, never as a parameter default or a resource value. + assert.NotContains(t, src, `Default: "`+allZeroThumbprint+`"`, + "the all-zeros placeholder must not be reintroduced as a default") +} diff --git a/known_issues/13_iac_aws_target.md b/known_issues/13_iac_aws_target.md index be03f93e0..20d747e2c 100644 --- a/known_issues/13_iac_aws_target.md +++ b/known_issues/13_iac_aws_target.md @@ -1,6 +1,6 @@ # Known Issues: IaC AWS Target Federation -> **Audit status (2026-07-28):** `1 still valid · 8 resolved · 0 partially fixed · 0 moved · 0 needs triage` +> **Audit status (2026-07-29):** `1 still valid · 9 resolved · 0 partially fixed · 0 moved · 0 needs triage` ## CRITICAL: federation bundle generator never emits `OIDCSubjectClaim` @@ -45,6 +45,76 @@ property holds; the operator is still misled by the "Optional" label first. fixed in that PR because `internal/` was owned by concurrent in-flight branches. +## ~~HIGH: `OIDCThumbprint` defaults to the all-zeros placeholder for any issuer~~ — RESOLVED + +**File**: `iac/federation/aws-target/cloudformation/template.yaml` (`OIDCThumbprint` +parameter and the `ThumbprintList` property), `iac/federation/aws-target/terraform/variables.tf` +(`thumbprint_list`) +**Description**: `OIDCThumbprint` defaulted to +`0000000000000000000000000000000000000000` and was validated only by +`AllowedPattern: "^[0-9a-fA-F]{40}$"`, which that placeholder satisfies. The +Terraform sibling defaulted to the same value and permitted it for +`login.microsoftonline.com` and `accounts.google.com` issuers. + +**Impact**: Not the authentication bypass it looks like (see below), but wrong +in both directions, and documented as if the value carried security meaning it +does not have. + +AWS verifies the JWKS endpoint's TLS certificate against its own library of +trusted root CAs, and consults the configured thumbprint only when that +certificate does not chain to one of them, when AWS cannot retrieve the +certificate, or when the endpoint requires TLS 1.3 +([IAM User Guide](https://docs.aws.amazon.com/IAM/latest/UserGuide/id_roles_providers_create_oidc_verify-thumbprint.html), +[CreateOpenIDConnectProvider](https://docs.aws.amazon.com/IAM/latest/APIReference/API_CreateOpenIDConnectProvider.html)). +Consequences: + +- For a publicly-trusted issuer, including both this template documents, the + thumbprint is read only under the fallback conditions listed above, which + normal operation does not hit. All-zeros is inert there, and so is a correct + thumbprint. An attacker still needs a JWKS-host certificate signed by a CA in + AWS's trusted-root library, and that bar does not move with this parameter. +- On the fallback path, all-zeros is not the SHA-1 of any certificate, so it + matches nothing and every `AssumeRoleWithWebIdentity` call fails. It does not + bypass chain verification; it nulls out the only verification that path has. + The failure mode is availability, not takeover. + +The parameter therefore had no value that fails open, and the guard the +Terraform module carried ("the all-zeros value bypasses the CA-chain check +entirely") was defending against a mechanism that does not exist, while its +issuer allowlist was beside the point in both directions. + +**Status:** ✔️ Resolved + +**Resolved by:** #1615. `ThumbprintList` is optional on +`AWS::IAM::OIDCProvider`; when omitted, IAM retrieves and uses the issuer's +real top intermediate CA thumbprint, which is what the IAM console does by +default. That is now the default on both paths. + +- CloudFormation: `OIDCThumbprint` defaults to empty, a `HasThumbprint` + condition omits `ThumbprintList` via `AWS::NoValue` when it is, and the + placeholder is rejected by two independent layers: an `AllowedPattern` + requiring at least one non-zero hex digit, and an unconditional `Rules` + assertion naming the literal. +- Terraform: `thumbprint_list` defaults to `[]` and is passed as `null` when + empty; the all-zeros entry is rejected unconditionally rather than for + non-allowlisted issuers. +- Regression coverage in `internal/api/handler_federation_test.go` reads the + `AllowedPattern`, `MaxLength` and `Default` back out of the committed + template and evaluates them the way CloudFormation does. + +**Upgrade note:** `ThumbprintList` updates with no interruption, so switching +forms never replaces the OIDC provider or changes its ARN. A CloudFormation +stack still holding the placeholder fails its next update until the parameter +is cleared or set to a real thumbprint. That is deliberate, and such a stack cannot be +authenticating on the fallback path today anyway. Terraform's `thumbprint_list` +is Optional+Computed, so clearing it does not clear a value already stored on +an existing provider; correct those with +`aws iam update-open-id-connect-provider-thumbprint` or replace the resource. + +**Still open:** `internal/iacfiles/templates/aws-wif-cli.sh.tmpl:33` hardcodes +the same placeholder into `aws iam create-open-id-connect-provider`. That file +is owned by #1640 and was left alone here; noted on that issue. + ## ~~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` @@ -216,6 +286,15 @@ other control on this trust policy. **Resolved by:** Added two `validation` blocks to `thumbprint_list`: one rejects empty lists, the other requires every entry to match `^[0-9a-fA-F]{40}$`. The all-zeros default is preserved (AWS auto-validates well-known providers like Azure AD/Google and accepts the placeholder for them); the validation prevents the typo'd / wrong-length cases that otherwise surface only at runtime. Custom issuers that need a real thumbprint are documented in the variable description. +> **Superseded by #1615.** The reasoning above is wrong: AWS does not +> "auto-validate" a special set of well-known providers, it validates the JWKS +> endpoint's TLS certificate against its trusted-root CA library for *every* +> issuer and reads the thumbprint only as a fallback. Keeping the all-zeros +> default was therefore not safe-for-those-two, it was inert everywhere it was +> read and fail-closed everywhere it mattered. The empty-list rejection has +> also been removed, since an empty list is now the correct default: it makes +> IAM retrieve the real thumbprint. See the #1615 entry above. + ### Original implementation plan **Goal:** Prevent the all-zeros default from silently producing a non-functional OIDC provider by validating each thumbprint at `terraform plan` time.