From cee3bcd71e07da516e30c9c96087b904dc86e86e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 13:46:01 +0200 Subject: [PATCH 1/3] sec(iac/aws): require OIDC subject claim in the federation bundle generator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #1602 made OIDCSubjectClaim a required CloudFormation parameter with no default, closing the subject-less AWS trust policy hole for the checked-in template. The internal/iacfiles generator that produces the customer-facing bundle was left untouched at the time (internal/ was owned by concurrent in-flight branches), so three bundle formats still shipped a subject-less policy or a broken deploy: - aws-wif-cli.sh.tmpl documented OIDC_SUBJECT_CLAIM as "Optional" and built a trust policy with only the :aud condition when it was unset. This path never goes through CloudFormation and format=cli is a first-class user-selectable download, so it was the one exploitable gap: any identity the documented issuer (accounts.google.com) can mint could assume the role and place irreversible multi-year commitment purchases. - aws-cfn-deploy.sh.tmpl and buildCFParamsJSON never forwarded OIDCSubjectClaim, so the CFN deploy failed at change-set creation with "Parameters: [OIDCSubjectClaim] must have values" once the parameter became required. - aws-wif.tfvars.tmpl emitted oidc_subject_claim commented out and labelled "Optional", contradicting the Terraform module's required, no-default variable. Fix: aws-wif-cli.sh.tmpl drops the subject-less else branch entirely and validates OIDC_SUBJECT_CLAIM (non-empty, no whitespace/$/*) before making any AWS call, mirroring the guard PR #1602 added to the CloudFormation parameter. federationIaCData gains an OIDCSubjectClaim field threaded through shellEscapeData, buildCFParamsJSON, aws-wif-cf-params.json.tmpl, aws-cfn-deploy.sh.tmpl's --parameter-overrides, and aws-wif.tfvars.tmpl (uncommented, "Optional" label removed). The standalone scripts/generate-federation-iac.go mirror gains the same field plus an --oidc-subject-claim flag. CUDly's server has no generic way to know the calling workload's real subject claim, unlike OIDCIssuerURL/OIDCAudience which are derivable from target/source alone, so the value stays operator-supplied by design — every artifact now requires it explicitly instead of defaulting to a working-but-insecure empty value. Closes #1640. --- internal/api/handler_federation.go | 11 ++ internal/api/handler_federation_test.go | 105 ++++++++++++++++++ .../iacfiles/templates/aws-cfn-deploy.sh.tmpl | 1 + .../templates/aws-wif-cf-params.json.tmpl | 7 +- .../iacfiles/templates/aws-wif-cli.sh.tmpl | 50 +++++---- .../iacfiles/templates/aws-wif.tfvars.tmpl | 10 +- internal/iacfiles/templates_test.go | 46 ++++++++ known_issues/13_iac_aws_target.md | 44 +++++++- scripts/generate-federation-iac.go | 10 +- 9 files changed, 252 insertions(+), 32 deletions(-) diff --git a/internal/api/handler_federation.go b/internal/api/handler_federation.go index c07cc7e51..8425ea5eb 100644 --- a/internal/api/handler_federation.go +++ b/internal/api/handler_federation.go @@ -30,6 +30,15 @@ type federationIaCData struct { OIDCIssuerURL string OIDCIssuerHost string // issuer URL without https:// prefix (used as IAM condition key) OIDCAudience string + // OIDCSubjectClaim restricts the AWS trust policy to a single workload + // subject. Deliberately left empty by every generic-bundle builder below — + // CUDly's server has no generic way to know the calling workload's real + // subject claim (it is not derivable from target/source alone the way + // OIDCIssuerURL/OIDCAudience are). Every AWS-WIF template still emits this + // field as a required, uncommented value so the operator must fill it in + // before the bundle deploys/applies/runs, rather than the bundle silently + // working with no :sub condition (see #1543, #1602, #1640). + OIDCSubjectClaim string // Azure-specific SubscriptionID string TenantID string @@ -282,6 +291,7 @@ func shellEscapeData(data federationIaCData) federationIaCData { d.OIDCIssuerURL = shellEscape(data.OIDCIssuerURL) d.OIDCIssuerHost = shellEscape(data.OIDCIssuerHost) d.OIDCAudience = shellEscape(data.OIDCAudience) + d.OIDCSubjectClaim = shellEscape(data.OIDCSubjectClaim) d.SubscriptionID = shellEscape(data.SubscriptionID) d.TenantID = shellEscape(data.TenantID) d.ProjectID = shellEscape(data.ProjectID) @@ -769,6 +779,7 @@ func buildCFParamsJSON(data federationIaCData, source string) (string, error) { {ParameterKey: "OIDCIssuerURL", ParameterValue: data.OIDCIssuerURL}, {ParameterKey: "OIDCIssuerHost", ParameterValue: strings.TrimPrefix(data.OIDCIssuerURL, "https://")}, {ParameterKey: "OIDCAudience", ParameterValue: data.OIDCAudience}, + {ParameterKey: "OIDCSubjectClaim", ParameterValue: data.OIDCSubjectClaim}, {ParameterKey: "RoleName", ParameterValue: "CUDly-" + data.AccountSlug}, } } diff --git a/internal/api/handler_federation_test.go b/internal/api/handler_federation_test.go index 80bd90566..04aa802c5 100644 --- a/internal/api/handler_federation_test.go +++ b/internal/api/handler_federation_test.go @@ -387,6 +387,111 @@ func TestGetFederationIaC_CFNZip_ParamsValidJSON(t *testing.T) { assert.GreaterOrEqual(t, len(params), 3, "should have at least 3 parameters") } +// TestGetFederationIaC_AWSWIF_SubjectClaimThreaded is the regression test for +// #1640: the CFN params JSON and deploy script, and the Terraform tfvars, +// used to omit OIDCSubjectClaim / oidc_subject_claim entirely (CFN) or emit it +// commented-out and mislabeled "Optional" (tfvars), even though the +// CloudFormation template (#1602) and Terraform module both require it with +// no default. Every AWS-WIF artifact must now emit the parameter/variable +// explicitly so the deploy fails with a specific, actionable error instead of +// either "must have values" (CFN) or a misleading "Optional" comment (tfvars). +func TestGetFederationIaC_AWSWIF_SubjectClaimThreaded(t *testing.T) { + h := federationHandler() + ctx := context.Background() + + t.Run("cfn params JSON", func(t *testing.T) { + res, err := h.getFederationIaC(ctx, federationReq(map[string]string{ + "target": "aws", "source": "gcp", "format": "cfn", + })) + require.NoError(t, err) + zipBytes, err := base64.StdEncoding.DecodeString(res.Content) + require.NoError(t, err) + zr, err := zip.NewReader(bytes.NewReader(zipBytes), int64(len(zipBytes))) + require.NoError(t, err) + + var paramsFile *zip.File + for _, f := range zr.File { + if strings.HasSuffix(f.Name, "-cf-params.json") { + paramsFile = f + break + } + } + require.NotNil(t, paramsFile) + rc, err := paramsFile.Open() + require.NoError(t, err) + defer rc.Close() + var buf bytes.Buffer + _, err = buf.ReadFrom(rc) + require.NoError(t, err) + + var params []map[string]string + require.NoError(t, json.Unmarshal(buf.Bytes(), ¶ms)) + var found bool + for _, p := range params { + if p["ParameterKey"] == "OIDCSubjectClaim" { + found = true + } + } + assert.True(t, found, "cf-params.json must include an OIDCSubjectClaim entry") + }) + + t.Run("cfn deploy script", func(t *testing.T) { + res, err := h.getFederationIaC(ctx, federationReq(map[string]string{ + "target": "aws", "source": "gcp", "format": "cfn", + })) + require.NoError(t, err) + zipBytes, err := base64.StdEncoding.DecodeString(res.Content) + require.NoError(t, err) + zr, err := zip.NewReader(bytes.NewReader(zipBytes), int64(len(zipBytes))) + require.NoError(t, err) + + var deployScript string + for _, f := range zr.File { + if f.Name == "cloudformation/deploy-cfn.sh" { + rc, err := f.Open() + require.NoError(t, err) + var buf bytes.Buffer + _, _ = buf.ReadFrom(rc) + rc.Close() + deployScript = buf.String() + } + } + require.NotEmpty(t, deployScript) + assert.Contains(t, deployScript, `"OIDCSubjectClaim=`, + "deploy script must pass OIDCSubjectClaim in --parameter-overrides") + }) + + t.Run("tfvars", func(t *testing.T) { + res, err := h.getFederationIaC(ctx, federationReq(map[string]string{ + "target": "aws", "source": "gcp", "format": "bundle", + })) + require.NoError(t, err) + zipBytes, err := base64.StdEncoding.DecodeString(res.Content) + require.NoError(t, err) + zr, err := zip.NewReader(bytes.NewReader(zipBytes), int64(len(zipBytes))) + require.NoError(t, err) + + var tfvars string + for _, f := range zr.File { + if strings.HasSuffix(f.Name, ".auto.tfvars") { + rc, err := f.Open() + require.NoError(t, err) + var buf bytes.Buffer + _, _ = buf.ReadFrom(rc) + rc.Close() + tfvars = buf.String() + } + } + require.NotEmpty(t, tfvars) + assert.Contains(t, tfvars, "\noidc_subject_claim = \"", + "tfvars must emit oidc_subject_claim uncommented, at the start of a line") + assert.NotContains(t, tfvars, `# oidc_subject_claim = `, + "oidc_subject_claim must not be commented out as a variable assignment") + assert.NotContains(t, tfvars, "Optional: restrict trust to a specific workload subject claim", + "oidc_subject_claim is required, not optional — the module has no default and rejects empty") + }) +} + func TestSingleFileSpec_CLI_AllScenarios(t *testing.T) { cases := []struct{ target, source, wantContains string }{ {"aws", "aws", "aws-cross-account-cli.sh"}, diff --git a/internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl b/internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl index 3c1f49799..2d594bfa4 100644 --- a/internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl +++ b/internal/iacfiles/templates/aws-cfn-deploy.sh.tmpl @@ -35,6 +35,7 @@ aws cloudformation deploy \ "OIDCIssuerURL={{.OIDCIssuerURL}}" \ "OIDCIssuerHost={{.OIDCIssuerHost}}" \ "OIDCAudience={{.OIDCAudience}}" \ + "OIDCSubjectClaim={{.OIDCSubjectClaim}}" \ "RoleName=CUDly-{{.AccountSlug}}" \ --capabilities CAPABILITY_NAMED_IAM \ --no-fail-on-empty-changeset diff --git a/internal/iacfiles/templates/aws-wif-cf-params.json.tmpl b/internal/iacfiles/templates/aws-wif-cf-params.json.tmpl index 91677f436..947ae3b0d 100644 --- a/internal/iacfiles/templates/aws-wif-cf-params.json.tmpl +++ b/internal/iacfiles/templates/aws-wif-cf-params.json.tmpl @@ -1,5 +1,6 @@ [ - { "ParameterKey": "OIDCIssuerURL", "ParameterValue": "{{.OIDCIssuerURL}}" }, - { "ParameterKey": "OIDCAudience", "ParameterValue": "{{.OIDCAudience}}" }, - { "ParameterKey": "RoleName", "ParameterValue": "CUDly-{{.AccountSlug}}" } + { "ParameterKey": "OIDCIssuerURL", "ParameterValue": "{{.OIDCIssuerURL}}" }, + { "ParameterKey": "OIDCAudience", "ParameterValue": "{{.OIDCAudience}}" }, + { "ParameterKey": "OIDCSubjectClaim", "ParameterValue": "{{.OIDCSubjectClaim}}" }, + { "ParameterKey": "RoleName", "ParameterValue": "CUDly-{{.AccountSlug}}" } ] diff --git a/internal/iacfiles/templates/aws-wif-cli.sh.tmpl b/internal/iacfiles/templates/aws-wif-cli.sh.tmpl index 53357cc26..ded3c0f07 100644 --- a/internal/iacfiles/templates/aws-wif-cli.sh.tmpl +++ b/internal/iacfiles/templates/aws-wif-cli.sh.tmpl @@ -12,9 +12,33 @@ set -euo pipefail ROLE_NAME="${ROLE_NAME:-CUDly-{{.AccountSlug}}}" OIDC_ISSUER_URL="${OIDC_ISSUER_URL:-{{.OIDCIssuerURL}}}" OIDC_AUDIENCE="${OIDC_AUDIENCE:-{{.OIDCAudience}}}" -# Optional: restrict which OIDC subject can assume the role. -# Useful for multi-tenant issuers. Matches TF variable oidc_subject_claim. -OIDC_SUBJECT_CLAIM="${OIDC_SUBJECT_CLAIM:-}" +# REQUIRED: restricts which OIDC subject can assume the role. Without a :sub +# condition the trust policy accepts every identity the issuer can mint — the +# same hole #1543/#1602 closed in the CloudFormation template — so this is +# validated below rather than left to build a subject-less policy silently. +# Matches the required TF variable oidc_subject_claim and the required +# CloudFormation OIDCSubjectClaim parameter. +OIDC_SUBJECT_CLAIM="${OIDC_SUBJECT_CLAIM:-{{.OIDCSubjectClaim}}}" +if [[ -z "${OIDC_SUBJECT_CLAIM}" ]]; then + echo "Error: OIDC_SUBJECT_CLAIM is required. Without it the trust policy has no" >&2 + echo " :sub condition and every identity ${OIDC_ISSUER_URL} can mint is able" >&2 + echo " to assume this role. Set it to the calling workload's subject claim:" >&2 + echo " GCP service account : its numeric unique ID (not the email)" >&2 + echo " Azure managed identity: the object ID of the managed identity" >&2 + echo " e.g. OIDC_SUBJECT_CLAIM=123456789012345678901 bash $0" >&2 + exit 1 +fi +case "${OIDC_SUBJECT_CLAIM}" in + *[[:space:]]*|*'$'*|*'*'*) + echo "Error: OIDC_SUBJECT_CLAIM must not contain whitespace, '\$' or '*'." >&2 + echo " IAM expands \${...} policy variables inside Condition values, so a" >&2 + echo " value such as \${accounts.google.com:sub} would expand to the token's" >&2 + echo " own sub claim and match every identity the issuer can mint. '*' is" >&2 + echo " compared literally by StringEquals and would silently produce a role" >&2 + echo " nobody can assume." >&2 + exit 1 + ;; +esac PROFILE_ARG="" if [[ -n "${AWS_PROFILE:-}" ]]; then PROFILE_ARG="--profile ${AWS_PROFILE}"; fi @@ -37,9 +61,9 @@ PROVIDER_ARN=$(aws iam list-open-id-connect-providers $PROFILE_ARG \ --query "OpenIDConnectProviderList[?contains(Arn, '${OIDC_HOST}')].Arn | [0]" \ --output text) -# Build trust policy — add subject claim restriction when OIDC_SUBJECT_CLAIM is set. -if [[ -n "${OIDC_SUBJECT_CLAIM}" ]]; then - TRUST_POLICY=$(cat < firstAWSCall { + t.Errorf("the empty-OIDC_SUBJECT_CLAIM guard (offset %d) must run before the first aws iam call (offset %d)", guard, firstAWSCall) + } + + // $ and * must be rejected — same characters PR #1602 rejects in the + // CloudFormation OIDCSubjectClaim parameter, for the same reason. + if !strings.Contains(rendered, `must not contain whitespace, '\$' or '*'`) { + t.Error("rendered script must reject whitespace, '$', and '*' in OIDC_SUBJECT_CLAIM") + } +} + // TestCLITemplatesShellMetacharsPassThrough confirms that text/template does // not shell-escape interpolated values. This is intentional: the templates are // designed to be downloaded and inspected by an operator, not auto-executed. diff --git a/known_issues/13_iac_aws_target.md b/known_issues/13_iac_aws_target.md index 20d747e2c..02f8a80b4 100644 --- a/known_issues/13_iac_aws_target.md +++ b/known_issues/13_iac_aws_target.md @@ -1,8 +1,8 @@ # Known Issues: IaC AWS Target Federation -> **Audit status (2026-07-29):** `1 still valid · 9 resolved · 0 partially fixed · 0 moved · 0 needs triage` +> **Audit status (2026-08-03):** `1 still valid · 10 resolved · 0 partially fixed · 0 moved · 0 needs triage` -## CRITICAL: federation bundle generator never emits `OIDCSubjectClaim` +## ~~CRITICAL: federation bundle generator never emits `OIDCSubjectClaim`~~ — RESOLVED **File**: @@ -41,9 +41,43 @@ 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. +**Status:** ✔️ Resolved + +**Resolved by:** #TODO_PR_NUMBER (closes #1640) — `aws-wif-cli.sh.tmpl` drops the subject-less else +branch entirely and validates `OIDC_SUBJECT_CLAIM` (non-empty, no whitespace/ +`$`/`*`) before making any AWS call; `federationIaCData` gains an +`OIDCSubjectClaim` field threaded through `shellEscapeData`, `buildCFParamsJSON`, +`aws-wif-cf-params.json.tmpl`, `aws-cfn-deploy.sh.tmpl`'s `--parameter-overrides`, +and `aws-wif.tfvars.tmpl` (uncommented, "Optional" label removed); the standalone +`scripts/generate-federation-iac.go` mirror gains the same field plus an +`--oidc-subject-claim` flag. CUDly's server still has no generic way to know +the calling workload's real subject (unlike `OIDCIssuerURL`/`OIDCAudience`, +which are derivable from target/source alone), so the value remains +operator-supplied — every artifact now requires it explicitly instead of +defaulting to a working-but-insecure empty value. + +## LOW: `aws-wif-cli.sh.tmpl` hardcodes an unoverridable OIDC-provider thumbprint placeholder + +**File**: `internal/iacfiles/templates/aws-wif-cli.sh.tmpl` — the +`--thumbprint-list` argument to `aws iam create-open-id-connect-provider` +(line number shifts as the file changes; grep for `thumbprint-list`), and the +provider-exists branch a few lines above it that skips re-checking it. +**Description**: The script hardcodes the all-zeros placeholder thumbprint +with no way for the operator to override it, and the `if` that creates the +OIDC provider only runs when one does not already exist — an existing +provider created with the placeholder is never corrected by re-running the +script. This is the CLI-bundle sibling of #1615, which #1678 fixed for the +CloudFormation and Terraform bundles but deliberately left this file alone +(noted on #1640 by the #1678 author, since `internal/iacfiles/` was this +issue's scope). +**Impact**: Not an authentication bypass — AWS only consults the configured +thumbprint on a fallback path (its JWKS certificate does not chain to a +trusted root, AWS cannot retrieve it, or the endpoint requires TLS 1.3), and +all-zeros is not the SHA-1 of any certificate, so it matches nothing on that +path and role assumption fails outright. The failure mode is availability and +misleading configuration, not takeover. `aws iam update-open-id-connect-provider-thumbprint` +is the out-of-band remedy for an already-created provider. +**Status:** ⚠️ Still valid — filed as #1689. ## ~~HIGH: `OIDCThumbprint` defaults to the all-zeros placeholder for any issuer~~ — RESOLVED diff --git a/scripts/generate-federation-iac.go b/scripts/generate-federation-iac.go index 270d8902d..7a0fcf052 100644 --- a/scripts/generate-federation-iac.go +++ b/scripts/generate-federation-iac.go @@ -84,6 +84,10 @@ type iacData struct { // AWS WIF / cross-account OIDCIssuerURL string OIDCAudience string + // OIDCSubjectClaim restricts the AWS trust policy to a single workload + // subject. Empty unless --oidc-subject-claim is passed; every AWS-WIF + // template requires it (no working subject-less default, see #1640). + OIDCSubjectClaim string // Azure-specific SubscriptionID string TenantID string @@ -280,11 +284,12 @@ func singleFileTmpl(target, source, format, slug string) (tmplFile, outName stri } // populateData fills target-specific fields on data from CLI flags. -func populateData(data *iacData, target, source, tenantID, projectID, saEmail string) bool { +func populateData(data *iacData, target, source, tenantID, projectID, saEmail, oidcSubjectClaim string) bool { switch target { case "aws": data.OIDCIssuerURL = awsOIDCIssuer(source, tenantID) data.OIDCAudience = awsOIDCAudience(source) + data.OIDCSubjectClaim = oidcSubjectClaim case "azure": data.SubscriptionID = data.AccountExternalID data.TenantID = tenantID @@ -314,6 +319,7 @@ func main() { tenantID := flag.String("tenant-id", "", "Azure tenant ID (required when source or target is azure)") projectID := flag.String("project-id", "", "GCP project ID (defaults to --account-id when target is gcp)") saEmail := flag.String("service-account-email", "", "GCP service account email (defaults to cudly@.iam.gserviceaccount.com)") + oidcSubjectClaim := flag.String("oidc-subject-claim", "", "Subject (sub) claim restricting the AWS trust policy to one workload (required when target is aws; no working default, see #1640)") outFile := flag.String("output", "", "Output file path; use '-' to print to stdout (default: derived filename in current directory)") templDir := flag.String("templates-dir", "internal/iacfiles/templates", "Path to templates directory (run from repo root)") modulesDir := flag.String("modules-dir", "iac/federation", "Path to Terraform modules directory (used by --format bundle)") @@ -334,7 +340,7 @@ func main() { } data := iacData{AccountName: *accountName, AccountExternalID: *accountID, AccountSlug: slug, Source: *source} - if !populateData(&data, *target, *source, *tenantID, *projectID, *saEmail) { + if !populateData(&data, *target, *source, *tenantID, *projectID, *saEmail, *oidcSubjectClaim) { fmt.Fprintf(os.Stderr, "Error: --target must be aws, azure, or gcp (got %q)\n", *target) os.Exit(1) } From 1f62c7010466ad45bf485e772cd057926c82a3e4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 13:47:54 +0200 Subject: [PATCH 2/3] docs(known-issues): fill in the actual PR number for the #1640 fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to d4776cc67 — the known_issues entry was written before the PR existed and used a placeholder. --- known_issues/13_iac_aws_target.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/known_issues/13_iac_aws_target.md b/known_issues/13_iac_aws_target.md index 02f8a80b4..14624ac97 100644 --- a/known_issues/13_iac_aws_target.md +++ b/known_issues/13_iac_aws_target.md @@ -43,7 +43,7 @@ Terraform prompt for the value interactively or hard-error under property holds; the operator is still misled by the "Optional" label first. **Status:** ✔️ Resolved -**Resolved by:** #TODO_PR_NUMBER (closes #1640) — `aws-wif-cli.sh.tmpl` drops the subject-less else +**Resolved by:** #1691 (closes #1640) — `aws-wif-cli.sh.tmpl` drops the subject-less else branch entirely and validates `OIDC_SUBJECT_CLAIM` (non-empty, no whitespace/ `$`/`*`) before making any AWS call; `federationIaCData` gains an `OIDCSubjectClaim` field threaded through `shellEscapeData`, `buildCFParamsJSON`, From 68a47c00d6565b4dd835a8b6043ea4f0ee92d946 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 18:08:18 +0200 Subject: [PATCH 3/3] sec(scripts): validate --oidc-subject-claim before any rendering The standalone federation IaC generator copied --oidc-subject-claim straight onto iacData with no validation and no escaping. The value is then interpolated verbatim into three different grammars the script renders: - Bash: aws-cfn-deploy.sh.tmpl builds a "OIDCSubjectClaim=" double-quoted word passed to `aws cloudformation deploy`, so a command substitution or a quote executes when the operator runs the generated deploy-cfn.sh. - JSON: aws-wif-cf-params.json.tmpl, where a quote injects extra parameter keys into the CloudFormation parameters file (a second RoleName entry, for one). - HCL: aws-wif.tfvars.tmpl, where a quote breaks out of the attribute value. Escaping is the wrong primary control here: no single escaping helper is correct for all three grammars, and for the ${VAR:-} interpolation contexts the in-script guard runs after the assignment that embeds the value, so a command substitution has already executed by the time it is inspected. Validate at the Go boundary instead, before anything is rendered. The check is a positive allowlist, ^[A-Za-z0-9][A-Za-z0-9._:/@=+-]*$ with a 255-byte cap. It is deliberately stricter than the ^[^\s*$]+$ AllowedPattern the CloudFormation template and Terraform module enforce on the same value: those validate a value that is already a typed parameter and only need to reject what IAM mishandles, while this one validates a value about to become shell, JSON and HCL source. The narrowing costs nothing here because awsOIDCIssuer emits only accounts.google.com, login.microsoftonline.com, or "" for an unrecognised source, so the subjects reachable through this script are a numeric string and a UUID. Also close the inverse fail-quiet case found while wiring this: the flag was only read on the AWS arm, so passing it with --target gcp, or with the aws->aws cross-account combination whose role is trusted by source account plus external ID rather than by an OIDC :sub condition, discarded it silently. An operator who typos --target would get an artifact pinning nothing while believing otherwise. A subjectClaimMode enum now distinguishes required from not-applicable, with the strict value as the zero value so an unset mode fails closed, and populateData returns error rather than bool. --target is checked against an allowlist before the claim is, so a typo there is still reported as a bad --target rather than as an inapplicable flag. internal/iacfiles/templates/README.md is updated to match: its four AWS-WIF examples were copy-pasteable invocations that are now hard errors. It also spells out that the flag not applying to a combination does not mean that bundle needs no pinning, with a per-source table of the required variable each GCP target emits (aws_role_name, oidc_subject, or source_service_account). Tests, both confirmed to fail against the pre-fix code: - scripts/generate_federation_iac_test.go compiles the //go:build ignore script once and exercises it as a subprocess. 16 hostile values, the missing-value case, bundle mode, the four not-applicable combinations, and 6 positive controls. Ordering is proved directly by pointing --templates-dir at an empty directory: a render attempt would fail with "no such file", so the validation error instead is proof nothing was read. - internal/iacfiles/templates_test.go now executes the rendered aws-wif-cli.sh against a recording stub `aws` placed on the child's PATH, asserting both a non-zero exit and zero AWS invocations for every invalid OIDC_SUBJECT_CLAIM, plus a positive control that checks the accepted value actually reaches the trust policy's :sub condition. Against the pre-fix template this shows a role being created with a :aud-only condition, which is #1640 end to end. Adjacent pre-existing gaps found and filed on #1690 rather than fixed here: iacData is missing four fields the templates reference, shellEscape does not escape '}', and two server-side render paths emit shell/HCL from unescaped data. Refs #1640 --- internal/iacfiles/templates/README.md | 33 +- internal/iacfiles/templates_test.go | 207 +++++++++++ scripts/generate-federation-iac.go | 210 ++++++++++- scripts/generate_federation_iac_test.go | 444 ++++++++++++++++++++++++ 4 files changed, 877 insertions(+), 17 deletions(-) create mode 100644 scripts/generate_federation_iac_test.go diff --git a/internal/iacfiles/templates/README.md b/internal/iacfiles/templates/README.md index be78772a9..42b09adf5 100644 --- a/internal/iacfiles/templates/README.md +++ b/internal/iacfiles/templates/README.md @@ -49,8 +49,25 @@ from this directory via the `//go:embed` directive in `internal/iacfiles/embed.g ### Locally from the cloned repo -Use `scripts/generate-federation-iac.go` — a self-contained Go script with no -external dependencies: +Use `scripts/generate-federation-iac.go`, a self-contained Go script with no +external dependencies. + +Every AWS target with a non-AWS source requires `--oidc-subject-claim`: it is +the workload subject the generated AWS trust policy pins to, and there is no +working default (see #1640). Pass the calling workload's subject claim, which is +a GCP service account's numeric unique ID or an Azure managed identity's object +ID. On the other combinations the flag feeds nothing, so passing it is an error +rather than a silent no-op. + +That is not the same as saying those bundles need no pinning. Each GCP target +combination emits its own **required** pin for you to fill in before +`terraform apply`, none of which `--oidc-subject-claim` populates: + +| `--source` | file | variable to fill in | +|---|---|---| +| `aws` | `-gcp-wif.tfvars` | `aws_role_name` (blank) | +| `azure` | `-gcp-wif.tfvars` | `oidc_subject` (blank) | +| `gcp` | `-gcp-sa-impersonation.tfvars` | `source_service_account` (placeholder) | ```bash # Run from the repository root @@ -59,14 +76,16 @@ external dependencies: go run scripts/generate-federation-iac.go \ --target aws --source azure \ --account-name "prod-aws" --account-id "123456789012" \ - --tenant-id "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + --tenant-id "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" \ + --oidc-subject-claim "11111111-2222-3333-4444-555555555555" # AWS target, GCP source go run scripts/generate-federation-iac.go \ --target aws --source gcp \ - --account-name "prod-aws" --account-id "123456789012" + --account-name "prod-aws" --account-id "123456789012" \ + --oidc-subject-claim "123456789012345678901" -# AWS target, AWS source (cross-account role, no WIF) +# AWS target, AWS source (cross-account role, no WIF, no subject claim) go run scripts/generate-federation-iac.go \ --target aws --source aws \ --account-name "target-aws" --account-id "999888777666" @@ -75,7 +94,8 @@ go run scripts/generate-federation-iac.go \ go run scripts/generate-federation-iac.go \ --target aws --source azure --format cf-params \ --account-name "prod-aws" --account-id "123456789012" \ - --tenant-id "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + --tenant-id "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" \ + --oidc-subject-claim "11111111-2222-3333-4444-555555555555" # Azure target go run scripts/generate-federation-iac.go \ @@ -98,6 +118,7 @@ go run scripts/generate-federation-iac.go \ --target aws --source azure \ --account-name "prod" --account-id "123456789012" \ --tenant-id "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" \ + --oidc-subject-claim "11111111-2222-3333-4444-555555555555" \ --output - ``` diff --git a/internal/iacfiles/templates_test.go b/internal/iacfiles/templates_test.go index 23aa17459..03d7086ec 100644 --- a/internal/iacfiles/templates_test.go +++ b/internal/iacfiles/templates_test.go @@ -2,9 +2,15 @@ package iacfiles import ( "bytes" + "context" + "errors" + "os" + "os/exec" + "path/filepath" "strings" "testing" "text/template" + "time" ) // testTemplateData mirrors the fields that CLI templates read. It's a local @@ -246,6 +252,207 @@ func TestAWSWIFCLI_SubjectClaimRequired(t *testing.T) { } } +// awsStubScript returns a stand-in `aws` executable that appends every +// invocation to logPath and prints a value the caller's `$(...)` captures can +// consume. It never contacts AWS, so a test that reaches it has proved the +// guard let the run through. +func awsStubScript(logPath string) string { + return "#!/usr/bin/env bash\n" + + // One log line per invocation: the trust-policy argument is multi-line + // JSON, so newlines inside the arguments are folded to spaces first. + "args=\"$*\"\n" + + "printf '%s\\n' \"${args//$'\\n'/ }\" >> '" + logPath + "'\n" + + // "None" is what the script's provider-lookup branches expect when no + // OIDC provider exists yet, so the rest of the script proceeds. + "echo None\n" +} + +// runRenderedWIFScript writes the rendered aws-wif-cli.sh to a temp file, puts a +// recording `aws` stub first on PATH, runs the script under bash, and returns +// its exit code, stderr and the list of `aws` invocations the stub saw. +// +// PATH is set on the child process only (exec.Cmd.Env). The parent test +// process's environment is never mutated, so a run here cannot race with, or +// leak a stub `aws` into, any other test in this package. +func runRenderedWIFScript(t *testing.T, rendered string, env map[string]string) (exitCode int, stderr string, awsCalls []string) { + t.Helper() + + // Fatal, not Skip: this is a security regression test, and a green run that + // silently never executed the guard is worse than a red one. bash is already + // a hard dependency of this repo (every generated bundle is a bash script, + // and pre-commit runs shellcheck over them). + bashPath, lookErr := exec.LookPath("bash") + if lookErr != nil { + t.Fatalf("bash is required to execute the rendered script: %v", lookErr) + } + + dir := t.TempDir() + scriptPath := filepath.Join(dir, "aws-wif-cli.sh") + if err := os.WriteFile(scriptPath, []byte(rendered), 0o600); err != nil { + t.Fatalf("write rendered script: %v", err) + } + + stubDir := filepath.Join(dir, "bin") + if err := os.Mkdir(stubDir, 0o700); err != nil { + t.Fatalf("create stub dir: %v", err) + } + logPath := filepath.Join(dir, "aws-invocations.log") + if err := os.WriteFile(filepath.Join(stubDir, "aws"), []byte(awsStubScript(logPath)), 0o755); err != nil { + t.Fatalf("write aws stub: %v", err) + } + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + + cmd := exec.CommandContext(ctx, bashPath, scriptPath) + // The real PATH is kept after stubDir so the script's `sed`/`echo` still + // resolve; stubDir comes first so `aws` resolves to the stub. + cmd.Env = []string{"PATH=" + stubDir + string(os.PathListSeparator) + os.Getenv("PATH")} + for k, v := range env { + cmd.Env = append(cmd.Env, k+"="+v) + } + // stdout is captured but not returned: the script's progress chatter is not + // under test, and capturing it keeps it out of the test log. + var errBuf, outBuf strings.Builder + cmd.Stderr = &errBuf + cmd.Stdout = &outBuf + + var exitErr *exec.ExitError + if err := cmd.Run(); err != nil && !errors.As(err, &exitErr) { + t.Fatalf("run rendered script: %v (stderr: %s)", err, errBuf.String()) + } + + logBytes, err := os.ReadFile(logPath) + switch { + case os.IsNotExist(err): + // The stub was never invoked, which is what the reject cases want. + case err != nil: + t.Fatalf("read aws invocation log: %v", err) + default: + for _, line := range strings.Split(strings.TrimSpace(string(logBytes)), "\n") { + if line != "" { + awsCalls = append(awsCalls, line) + } + } + } + return cmd.ProcessState.ExitCode(), errBuf.String(), awsCalls +} + +// TestAWSWIFCLI_SubjectClaimGuardBlocksAWSCalls executes the rendered script +// instead of only reading it. TestAWSWIFCLI_SubjectClaimRequired asserts on +// rendered text, which cannot distinguish a guard that works from a guard whose +// condition is inverted or whose `exit 1` was dropped; this runs Bash against a +// recording `aws` stub and asserts that an invalid OIDC_SUBJECT_CLAIM produces +// both a non-zero exit and zero AWS calls, so a broken guard cannot leave a +// half-created OIDC provider or a subject-less role behind. +func TestAWSWIFCLI_SubjectClaimGuardBlocksAWSCalls(t *testing.T) { + // CUDlyAPIURL is cleared so the auto-registration block (which shells out to + // curl and jq) is not rendered: this test is about the subject-claim guard, + // and the positive control below runs the script to completion. + data := baseData() + data.CUDlyAPIURL = "" + // A real issuer so OIDC_HOST resolves and the positive control can assert on + // the fully-formed ":sub" condition key the trust policy carries. + data.OIDCIssuerURL = "https://accounts.google.com" + rendered := renderCLITemplate(t, "templates/aws-wif-cli.sh.tmpl", data) + + // The two messages the rendered script's guards emit. Whitespace, $ and * + // share one `case` arm, so they share one message. + const ( + wantRequired = "OIDC_SUBJECT_CLAIM is required" + wantForbidden = `OIDC_SUBJECT_CLAIM must not contain whitespace, '$' or '*'.` + ) + + reject := []struct { + name string + // env holds the variables set on the child process. Omitting the + // OIDC_SUBJECT_CLAIM key leaves the variable unset for that run. + env map[string]string + wantText string + }{ + {"unset", map[string]string{}, wantRequired}, + {"empty", map[string]string{"OIDC_SUBJECT_CLAIM": ""}, wantRequired}, + {"only whitespace", map[string]string{"OIDC_SUBJECT_CLAIM": " "}, wantForbidden}, + {"embedded space", map[string]string{"OIDC_SUBJECT_CLAIM": "abc def"}, wantForbidden}, + {"tab", map[string]string{"OIDC_SUBJECT_CLAIM": "abc\tdef"}, wantForbidden}, + {"iam policy variable", map[string]string{"OIDC_SUBJECT_CLAIM": "${accounts.google.com:sub}"}, wantForbidden}, + {"bare dollar", map[string]string{"OIDC_SUBJECT_CLAIM": "abc$def"}, wantForbidden}, + {"bare wildcard", map[string]string{"OIDC_SUBJECT_CLAIM": "*"}, wantForbidden}, + {"embedded wildcard", map[string]string{"OIDC_SUBJECT_CLAIM": "abc*"}, wantForbidden}, + } + + for _, tc := range reject { + t.Run("reject/"+tc.name, func(t *testing.T) { + exitCode, stderr, awsCalls := runRenderedWIFScript(t, rendered, tc.env) + + if exitCode == 0 { + t.Errorf("OIDC_SUBJECT_CLAIM %q was accepted (exit 0)", tc.env["OIDC_SUBJECT_CLAIM"]) + } + if len(awsCalls) != 0 { + t.Errorf("the guard must run before any AWS call, but the aws stub was invoked %d time(s): %v", + len(awsCalls), awsCalls) + } + if !strings.Contains(stderr, tc.wantText) { + t.Errorf("stderr must explain the rejection with %q, got:\n%s", tc.wantText, stderr) + } + }) + } + + // A hostile value baked into the template's default (the "{{.OIDCSubjectClaim}}" + // fallback) is caught by the same guard when it merely contains a forbidden + // character, so an operator who edits the generated script by hand is + // covered too. This is not a substitute for validating the value at + // generation time: the guard runs after the assignment on the + // OIDC_SUBJECT_CLAIM="${OIDC_SUBJECT_CLAIM:-}" line, so a command + // substitution in the default would already have run. That is why + // scripts/generate-federation-iac.go validates the flag before rendering. + t.Run("reject/hostile template default", func(t *testing.T) { + hostile := baseData() + hostile.CUDlyAPIURL = "" + hostile.OIDCSubjectClaim = "abc def" + exitCode, stderr, awsCalls := runRenderedWIFScript(t, + renderCLITemplate(t, "templates/aws-wif-cli.sh.tmpl", hostile), map[string]string{}) + + if exitCode == 0 { + t.Error("a whitespace-bearing template default was accepted (exit 0)") + } + if len(awsCalls) != 0 { + t.Errorf("the aws stub must not be invoked, got %d call(s): %v", len(awsCalls), awsCalls) + } + if !strings.Contains(stderr, wantForbidden) { + t.Errorf("stderr must explain the rejection with %q, got:\n%s", wantForbidden, stderr) + } + }) + + // Positive control: without it, a guard that rejected every value would + // satisfy every assertion above. + t.Run("accept/valid subject claim", func(t *testing.T) { + exitCode, stderr, awsCalls := runRenderedWIFScript(t, rendered, + map[string]string{"OIDC_SUBJECT_CLAIM": "123456789012345678901"}) + + if exitCode != 0 { + t.Fatalf("a valid OIDC_SUBJECT_CLAIM was rejected (exit %d); stderr:\n%s", exitCode, stderr) + } + if len(awsCalls) == 0 { + t.Fatal("a valid OIDC_SUBJECT_CLAIM must get past the guard and reach the aws calls, but the stub was never invoked") + } + // The role must actually be created with a trust policy carrying the + // :sub condition, resolved to the supplied value. + var createRole string + for _, call := range awsCalls { + if strings.HasPrefix(call, "iam create-role") { + createRole = call + } + } + if createRole == "" { + t.Fatalf("expected an `aws iam create-role` invocation, got: %v", awsCalls) + } + if !strings.Contains(createRole, `"accounts.google.com:sub": "123456789012345678901"`) { + t.Errorf("the trust policy passed to create-role must pin :sub to the supplied claim, got:\n%s", createRole) + } + }) +} + // TestCLITemplatesShellMetacharsPassThrough confirms that text/template does // not shell-escape interpolated values. This is intentional: the templates are // designed to be downloaded and inspected by an operator, not auto-executed. diff --git a/scripts/generate-federation-iac.go b/scripts/generate-federation-iac.go index 7a0fcf052..26d5e2537 100644 --- a/scripts/generate-federation-iac.go +++ b/scripts/generate-federation-iac.go @@ -14,19 +14,42 @@ // // Go 1.21+ in PATH. No other dependencies. Run from the repository root. // +// # The --oidc-subject-claim flag +// +// Every AWS-target combination other than aws->aws federates via OIDC and +// requires --oidc-subject-claim: it is the workload subject the generated trust +// policy pins to, and without it the policy would accept every identity the +// issuer can mint (#1543, #1602, #1640). Pass the calling workload's subject: +// a GCP service account's numeric unique ID, or an Azure managed identity's +// object ID. The value is validated against an allowlist before anything is +// rendered; see validateOIDCSubjectClaim. +// +// The remaining combinations render nothing that reads this flag, so passing it +// there is an error rather than a no-op: a silently discarded subject claim +// would look like the trust was pinned when it was not. That is not the same as +// saying they need no pinning. Each gcp-target combination emits its own +// REQUIRED pin, none of which --oidc-subject-claim populates: +// +// --source aws -> -gcp-wif.tfvars, aws_role_name (blank) +// --source azure -> -gcp-wif.tfvars, oidc_subject (blank) +// --source gcp -> -gcp-sa-impersonation.tfvars, source_service_account +// (a placeholder) +// // # Quick examples // // # AWS target, Azure source — Terraform tfvars // go run scripts/generate-federation-iac.go \ // --target aws --source azure \ // --account-name "prod-aws" --account-id "123456789012" \ -// --tenant-id "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" +// --tenant-id "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" \ +// --oidc-subject-claim "11111111-2222-3333-4444-555555555555" // // # AWS target, Azure source — CloudFormation parameters JSON // go run scripts/generate-federation-iac.go \ // --target aws --source azure --format cf-params \ // --account-name "prod-aws" --account-id "123456789012" \ -// --tenant-id "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" +// --tenant-id "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" \ +// --oidc-subject-claim "11111111-2222-3333-4444-555555555555" // // # AWS target, AWS source — cross-account IAM role tfvars // go run scripts/generate-federation-iac.go \ @@ -53,18 +76,21 @@ // go run scripts/generate-federation-iac.go \ // --target aws --source azure --format bundle \ // --account-name "prod-aws" --account-id "123456789012" \ -// --tenant-id "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" +// --tenant-id "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" \ +// --oidc-subject-claim "11111111-2222-3333-4444-555555555555" // // # Print tfvars to stdout // go run scripts/generate-federation-iac.go \ // --target aws --source gcp \ -// --account-name "prod" --account-id "123456789012" --output - +// --account-name "prod" --account-id "123456789012" \ +// --oidc-subject-claim "123456789012345678901" --output - package main import ( "archive/zip" "bytes" + "errors" "flag" "fmt" "os" @@ -283,8 +309,167 @@ func singleFileTmpl(target, source, format, slug string) (tmplFile, outName stri } } -// populateData fills target-specific fields on data from CLI flags. -func populateData(data *iacData, target, source, tenantID, projectID, saEmail, oidcSubjectClaim string) bool { +// oidcSubjectClaimMaxLen bounds --oidc-subject-claim. The two subject formats +// this flag can carry are a GCP service account's numeric unique ID (typically +// 21 digits; Google documents it as a numeric string without guaranteeing a +// length) and an Azure managed identity's object ID (a 36-character UUID), so +// 255 sits far above any real value and exists only to keep an absurd argument +// out of the generated artifacts. +const oidcSubjectClaimMaxLen = 255 + +// displayClaim renders a rejected claim for an error message. Long values are +// truncated so that a multi-kilobyte argument cannot flood the operator's +// terminal; the true length is reported instead. Doing this here rather than +// relying on the length check to run first lets each rejection below report the +// most useful diagnosis without any of them having to worry about size. +func displayClaim(claim string) string { + const maxShown = 64 + if len(claim) <= maxShown { + return fmt.Sprintf("%q", claim) + } + // Cutting at a byte offset can split a multi-byte rune. %q escapes the + // orphaned bytes rather than emitting them raw, which is also what keeps a + // claim carrying terminal control sequences from reaching the terminal. + return fmt.Sprintf("%q... (%d bytes total)", claim[:maxShown], len(claim)) +} + +// oidcSubjectClaimRE is a positive allowlist, not a denylist, because this +// script interpolates the value verbatim into three different grammars: +// - Bash: aws-cfn-deploy.sh.tmpl renders it into the "OIDCSubjectClaim=..." +// double-quoted word passed to `aws cloudformation deploy`. +// - JSON: aws-wif-cf-params.json.tmpl renders it as a string value. +// - HCL: aws-wif.tfvars.tmpl renders it as a quoted attribute value. +// +// No single escaping helper is correct for all three, so the value is +// constrained to characters that are inert in every one of them: letters, +// digits and . _ : / @ = + - with a leading letter or digit so a value can +// never begin with '-' and be re-read as a flag by a downstream command. +// Excluded by construction are whitespace and $ * " ' ` \ ( ) { } ; & | < > , +// % and newlines. +// +// This is deliberately stricter than the AllowedPattern ^[^\s*$]+$ that the +// CloudFormation template and the Terraform module enforce on the same value. +// Those two run on a value that is already a typed parameter, so they only need +// to reject what IAM itself mis-handles: '$' (IAM expands ${...} policy +// variables inside Condition values) and '*' (compared literally by +// StringEquals). This check runs earlier, on a value about to become shell, +// JSON and HCL *source*, so it has to reject the metacharacters of those +// grammars as well. +// +// The practical cost of the extra strictness is nil here: awsOIDCIssuer emits +// only login.microsoftonline.com, accounts.google.com, or "" for a source it +// does not recognise, so the only subjects a bundle from this script can +// legitimately pin are an Azure object-ID UUID and a GCP numeric unique ID. +// Subject formats from issuers that need the wider pattern, such as Auth0's +// "|" and Bitbucket's "{repo-uuid}:{step-uuid}", cannot be +// produced by this script and remain deployable by editing the tfvars or the +// CFN parameters directly. +// +// For the same reason this disagrees with iac/federation/gcp-target/terraform/ +// variables.tf, whose oidc_subject allowlist deliberately permits '|' for +// Auth0/Okta subjects. That value reaches a CEL attribute condition and an IAM +// principal path, where '|' is inert once quotes are excluded. This one reaches +// a Bash word, where '|' is a pipe. Same field name, different sinks, so the +// two allowlists are correctly different rather than accidentally divergent. +var oidcSubjectClaimRE = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._:/@=+-]*$`) + +// subjectClaimMode says what --oidc-subject-claim means for a given +// target/source pair, so an inapplicable value is rejected instead of silently +// dropped. +type subjectClaimMode int + +const ( + // subjectClaimRequired: the pair renders an AWS-WIF trust policy, whose + // :sub condition has no safe default. It is first so that the zero value of + // subjectClaimMode is the strict one: a mode left unset fails closed. + subjectClaimRequired subjectClaimMode = iota + // subjectClaimNotApplicable: the pair renders no artifact that reads + // .OIDCSubjectClaim, so a supplied claim would be silently discarded. + // + // This is narrower than "renders no trust to pin". Every gcp-target + // combination emits a REQUIRED pin of its own: aws_role_name or + // oidc_subject in gcp-wif.tfvars.tmpl, or source_service_account in + // gcp-sa-impersonation.tfvars.tmpl when the source is gcp. Those are + // different fields which this flag has never populated, and pinning them is + // out of scope for --oidc-subject-claim. See the doc comment at the top of + // this file for the full per-source table. + subjectClaimNotApplicable +) + +// subjectClaimModeFor reports whether --oidc-subject-claim feeds anything for +// this target/source pair. Only an AWS target with a non-AWS source renders the +// AWS-WIF artifacts: aws->aws is the cross-account path and selects +// aws-cross-account.tfvars.tmpl, whose role is trusted by source account plus +// external ID rather than by an OIDC :sub condition, and no azure or gcp +// template references .OIDCSubjectClaim at all. Those targets are not +// necessarily subject-less, they just pin their subject through a different +// variable (see the subjectClaimNotApplicable comment above). +func subjectClaimModeFor(target, source string) subjectClaimMode { + if target == "aws" && source != "aws" { + return subjectClaimRequired + } + return subjectClaimNotApplicable +} + +// validateOIDCSubjectClaim checks --oidc-subject-claim before any template is +// rendered. Generation time is the only place this can be enforced for the +// shell artifact: the operator runs the generated deploy-cfn.sh, by which point +// the value is already Bash source. The same ordering problem exists in the +// server-rendered aws-wif-cli.sh, whose OIDC_SUBJECT_CLAIM guard runs *after* +// the OIDC_SUBJECT_CLAIM="${OIDC_SUBJECT_CLAIM:-}" line that embeds the +// value, so a command substitution baked into the default has already executed +// by the time that guard inspects it. A validated value is the only control +// that runs before either. +func validateOIDCSubjectClaim(claim string, mode subjectClaimMode) error { + if claim == "" { + if mode == subjectClaimNotApplicable { + return nil + } + return errors.New("--oidc-subject-claim is required when --target=aws and --source is not aws. " + + "Without it the generated trust policy has no :sub condition and every identity the issuer " + + "can mint is able to assume the role. Set it to the calling workload's subject claim: " + + "a GCP service account's numeric unique ID (not its email), or an Azure managed identity's object ID") + } + // Applicability first: if the flag does not belong on this combination at + // all, saying so is more useful than complaining about its contents. + if mode == subjectClaimNotApplicable { + return fmt.Errorf("--oidc-subject-claim %s is not applicable to this target/source combination. "+ + "It pins the AWS workload identity federation trust policy, which is only generated for "+ + "--target=aws with a non-aws --source. Drop the flag, or correct --target/--source", + displayClaim(claim)) + } + if len(claim) > oidcSubjectClaimMaxLen { + return fmt.Errorf("--oidc-subject-claim is %d bytes, over the %d-byte limit", + len(claim), oidcSubjectClaimMaxLen) + } + if !oidcSubjectClaimRE.MatchString(claim) { + return fmt.Errorf("--oidc-subject-claim %s is not an accepted subject claim: it must start with a letter "+ + "or digit and contain only letters, digits and the characters . _ : / @ = + - . The value is "+ + "interpolated verbatim into the generated Bash, JSON and HCL artifacts, so shell metacharacters, "+ + "quotes and whitespace are rejected rather than escaped", displayClaim(claim)) + } + return nil +} + +// validTargets is the allowlist of target clouds, mirroring +// validFederationTargets in internal/api/handler_federation.go. +var validTargets = map[string]bool{"aws": true, "azure": true, "gcp": true} + +// populateData fills target-specific fields on data from CLI flags. It reports +// an error rather than writing an invalid value into data, so a rejected flag +// stops the run before any template is rendered. +func populateData(data *iacData, target, source, tenantID, projectID, saEmail, oidcSubjectClaim string) error { + // --target is checked first so that a typo there is reported as a bad + // --target rather than as an inapplicable --oidc-subject-claim, which would + // send the operator off to drop a flag that was never the problem. + if !validTargets[target] { + return fmt.Errorf("--target must be aws, azure, or gcp (got %q)", target) + } + // The claim is then validated outside the switch, so the check covers the + // targets that must NOT carry a subject claim as well as the one that must. + if err := validateOIDCSubjectClaim(oidcSubjectClaim, subjectClaimModeFor(target, source)); err != nil { + return err + } switch target { case "aws": data.OIDCIssuerURL = awsOIDCIssuer(source, tenantID) @@ -304,9 +489,12 @@ func populateData(data *iacData, target, source, tenantID, projectID, saEmail, o } data.OIDCIssuerURI = gcpOIDCIssuerURI(source, tenantID) default: - return false + // Unreachable while validTargets and these arms agree; kept so that + // adding a target to the map without an arm here fails loudly instead of + // emitting an artifact with no target-specific fields filled in. + return fmt.Errorf("--target %q is in validTargets but has no populateData branch", target) } - return true + return nil } func main() { @@ -319,7 +507,7 @@ func main() { tenantID := flag.String("tenant-id", "", "Azure tenant ID (required when source or target is azure)") projectID := flag.String("project-id", "", "GCP project ID (defaults to --account-id when target is gcp)") saEmail := flag.String("service-account-email", "", "GCP service account email (defaults to cudly@.iam.gserviceaccount.com)") - oidcSubjectClaim := flag.String("oidc-subject-claim", "", "Subject (sub) claim restricting the AWS trust policy to one workload (required when target is aws; no working default, see #1640)") + oidcSubjectClaim := flag.String("oidc-subject-claim", "", "Subject (sub) claim restricting the AWS trust policy to one workload: a GCP service account's numeric unique ID or an Azure managed identity's object ID. Required when --target=aws and --source is not aws; there is no working default (see #1640). Letters, digits and . _ : / @ = + - only") outFile := flag.String("output", "", "Output file path; use '-' to print to stdout (default: derived filename in current directory)") templDir := flag.String("templates-dir", "internal/iacfiles/templates", "Path to templates directory (run from repo root)") modulesDir := flag.String("modules-dir", "iac/federation", "Path to Terraform modules directory (used by --format bundle)") @@ -340,8 +528,8 @@ func main() { } data := iacData{AccountName: *accountName, AccountExternalID: *accountID, AccountSlug: slug, Source: *source} - if !populateData(&data, *target, *source, *tenantID, *projectID, *saEmail, *oidcSubjectClaim) { - fmt.Fprintf(os.Stderr, "Error: --target must be aws, azure, or gcp (got %q)\n", *target) + if err := populateData(&data, *target, *source, *tenantID, *projectID, *saEmail, *oidcSubjectClaim); err != nil { + fmt.Fprintf(os.Stderr, "Error: %v\n", err) os.Exit(1) } diff --git a/scripts/generate_federation_iac_test.go b/scripts/generate_federation_iac_test.go new file mode 100644 index 000000000..9fcf50c87 --- /dev/null +++ b/scripts/generate_federation_iac_test.go @@ -0,0 +1,444 @@ +// Package main's only non-test file, generate-federation-iac.go, carries a +// //go:build ignore tag so `go build ./...` skips it, which also means these +// tests cannot import its identifiers. They exercise the compiled script as a +// subprocess instead, which is how an operator actually runs it and is the only +// way to assert on its exit status and stderr. +package main + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "strings" + "sync" + "testing" + "time" +) + +const ( + // scriptFile is relative to this package directory. + scriptFile = "generate-federation-iac.go" + // repoRoot is the directory the script must run from: its --templates-dir + // and --modules-dir defaults are repository-root-relative. + repoRoot = ".." + + buildTimeout = 2 * time.Minute + runTimeout = 30 * time.Second +) + +var ( + buildOnce sync.Once + buildDir string + builtBinary string + buildErr error + buildOutput string +) + +// TestMain removes the directory holding the compiled script. t.TempDir cannot +// own it: the binary is built once and shared by every test in this package, so +// its lifetime is the test binary's, not any single test's. +func TestMain(m *testing.M) { + code := m.Run() + if buildDir != "" { + _ = os.RemoveAll(buildDir) + } + os.Exit(code) +} + +// generatorBinary compiles the script once per test binary and returns the path +// to the executable. Naming the file explicitly on the command line is what +// makes the //go:build ignore tag a no-op, exactly as the documented +// `go run scripts/generate-federation-iac.go` invocation does. +func generatorBinary(t *testing.T) string { + t.Helper() + buildOnce.Do(func() { + if _, err := exec.LookPath("go"); err != nil { + buildErr = err + return + } + dir, err := os.MkdirTemp("", "genfediac") + if err != nil { + buildErr = err + return + } + buildDir = dir + bin := filepath.Join(dir, "generate-federation-iac") + ctx, cancel := context.WithTimeout(context.Background(), buildTimeout) + defer cancel() + cmd := exec.CommandContext(ctx, "go", "build", "-o", bin, scriptFile) + out, err := cmd.CombinedOutput() + buildOutput = string(out) + if err != nil { + buildErr = err + return + } + builtBinary = bin + }) + if buildErr != nil { + t.Fatalf("build %s: %v\n%s", scriptFile, buildErr, buildOutput) + } + return builtBinary +} + +type runResult struct { + stdout string + stderr string + exitCode int +} + +// runGenerator executes the compiled script from the repository root. +func runGenerator(t *testing.T, args ...string) runResult { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), runTimeout) + defer cancel() + + cmd := exec.CommandContext(ctx, generatorBinary(t), args...) + cmd.Dir = repoRoot + var stdout, stderr strings.Builder + cmd.Stdout = &stdout + cmd.Stderr = &stderr + + // A non-zero exit is the expected outcome for most cases here, so only a + // failure to start (or the context deadline) is fatal. + var exitErr *exec.ExitError + if err := cmd.Run(); err != nil && !errors.As(err, &exitErr) { + t.Fatalf("run %v: %v (stderr: %s)", args, err, stderr.String()) + } + return runResult{ + stdout: stdout.String(), + stderr: stderr.String(), + exitCode: cmd.ProcessState.ExitCode(), + } +} + +// awsWIFArgs is a complete, otherwise-valid AWS-WIF invocation; each test +// varies only the subject claim and the output destination. +func awsWIFArgs(subjectClaim, output string) []string { + args := []string{ + "--target", "aws", + "--source", "gcp", + "--account-name", "prod", + "--account-id", "123456789012", + "--output", output, + } + if subjectClaim != "" { + args = append(args, "--oidc-subject-claim", subjectClaim) + } + return args +} + +// TestGenerator_RejectsHostileOIDCSubjectClaim is the regression test for the +// #1691 review finding: the generator used to assign --oidc-subject-claim +// straight onto the template data with no validation and no escaping, and the +// value is interpolated verbatim into three grammars this script renders: +// Bash (aws-cfn-deploy.sh.tmpl builds a "OIDCSubjectClaim=" word for +// `aws cloudformation deploy`), JSON (aws-wif-cf-params.json.tmpl) and HCL +// (aws-wif.tfvars.tmpl). None of those artifacts validates the value when the +// operator runs them, and the analogous guard in the server-rendered +// aws-wif-cli.sh could not help even if it were here: it runs after the +// OIDC_SUBJECT_CLAIM="${OIDC_SUBJECT_CLAIM:-}" assignment that embeds +// the value, so a command substitution in the default has already executed by +// the time it is inspected. Generation time is the only point that runs first. +// +// Each case asserts a non-zero exit, that stderr names the flag, and that no +// output file was produced. +func TestGenerator_RejectsHostileOIDCSubjectClaim(t *testing.T) { + cases := []struct { + name string + claim string + }{ + {"command substitution", "$(id)"}, + {"backtick substitution", "`id`"}, + {"shell variable", "${accounts.google.com:sub}"}, + {"closing brace ends the parameter expansion", "x}; touch /tmp/pwned"}, + {"double quote breaks out of the shell word", `a" injected="1`}, + {"single quote", "a' injected='1"}, + {"backslash", `a\b`}, + {"semicolon", "abc;id"}, + {"ampersand", "abc&id"}, + {"pipe", "abc|id"}, + {"redirect", "abc>out"}, + {"whitespace", "abc def"}, + {"newline", "abc\nid"}, + {"wildcard", "*"}, + {"leading dash reads as a flag", "--output"}, + {"over the length cap", strings.Repeat("1", 256)}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + out := filepath.Join(t.TempDir(), "generated.tfvars") + res := runGenerator(t, awsWIFArgs(tc.claim, out)...) + + if res.exitCode == 0 { + t.Errorf("claim %q was accepted (exit 0); stdout:\n%s", tc.claim, res.stdout) + } + if !strings.Contains(res.stderr, "--oidc-subject-claim") { + t.Errorf("claim %q: stderr must name the rejected flag, got:\n%s", tc.claim, res.stderr) + } + // Rendering happens strictly after validation, so the run must not + // have reached renderTmpl's error prefixes. + for _, renderMarker := range []string{"render ", "parse ", "read internal/iacfiles"} { + if strings.Contains(res.stderr, renderMarker) { + t.Errorf("claim %q: run reached template rendering (stderr contains %q):\n%s", + tc.claim, renderMarker, res.stderr) + } + } + if _, err := os.Stat(out); !os.IsNotExist(err) { + t.Errorf("claim %q: output file %s must not be created (stat err: %v)", tc.claim, out, err) + } + if res.stdout != "" { + t.Errorf("claim %q: nothing must be written to stdout, got:\n%s", tc.claim, res.stdout) + } + }) + } +} + +// TestGenerator_RejectsMissingOIDCSubjectClaim covers the case this PR exists +// for: an AWS-WIF bundle with no subject claim at all would pin the trust +// policy to nothing and accept every identity the issuer can mint. +func TestGenerator_RejectsMissingOIDCSubjectClaim(t *testing.T) { + out := filepath.Join(t.TempDir(), "generated.tfvars") + res := runGenerator(t, awsWIFArgs("", out)...) + + if res.exitCode == 0 { + t.Fatalf("an absent --oidc-subject-claim was accepted (exit 0); stdout:\n%s", res.stdout) + } + if !strings.Contains(res.stderr, "--oidc-subject-claim is required") { + t.Errorf("stderr must say the flag is required, got:\n%s", res.stderr) + } + if !strings.Contains(res.stderr, ":sub condition") { + t.Errorf("stderr must explain the missing :sub condition, got:\n%s", res.stderr) + } + if _, err := os.Stat(out); !os.IsNotExist(err) { + t.Errorf("output file %s must not be created (stat err: %v)", out, err) + } +} + +// TestGenerator_RejectsHostileClaimInBundleMode covers --format bundle +// separately from the single-file cases above. Bundle mode is the path that +// renders aws-cfn-deploy.sh.tmpl, i.e. the one artifact that turns the claim +// into Bash source, and it reaches the templates through runBundle rather than +// runSingleFile. Without this case, moving the validator into runSingleFile +// would leave the shell sink unguarded and every other test would still pass. +func TestGenerator_RejectsHostileClaimInBundleMode(t *testing.T) { + for _, claim := range []string{"", "$(id)", `a" injected="1`} { + t.Run("claim="+claim, func(t *testing.T) { + out := filepath.Join(t.TempDir(), "bundle.zip") + res := runGenerator(t, + "--target", "aws", + "--source", "gcp", + "--format", "bundle", + "--account-name", "prod", + "--account-id", "123456789012", + "--oidc-subject-claim", claim, + "--output", out, + ) + if res.exitCode == 0 { + t.Errorf("claim %q accepted in bundle mode (exit 0)", claim) + } + if !strings.Contains(res.stderr, "--oidc-subject-claim") { + t.Errorf("claim %q: stderr must name the rejected flag, got:\n%s", claim, res.stderr) + } + if _, err := os.Stat(out); !os.IsNotExist(err) { + t.Errorf("claim %q: bundle %s must not be written (stat err: %v)", claim, out, err) + } + }) + } +} + +// TestGenerator_RejectsInapplicableSubjectClaim covers the combinations that +// render no AWS trust policy. Accepting the flag there and dropping it would be +// the same fail-quiet shape this PR removes: the operator asks for the trust to +// be pinned and gets an artifact that pins nothing, with no diagnostic. +func TestGenerator_RejectsInapplicableSubjectClaim(t *testing.T) { + cases := []struct{ name, target, source string }{ + {"aws cross-account", "aws", "aws"}, + {"azure target", "azure", "aws"}, + {"gcp target", "gcp", "aws"}, + {"gcp same-cloud", "gcp", "gcp"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + res := runGenerator(t, + "--target", tc.target, + "--source", tc.source, + "--account-name", "prod", + "--account-id", "123456789012", + "--oidc-subject-claim", "123456789012345678901", + "--output", filepath.Join(t.TempDir(), "generated.tfvars"), + ) + if res.exitCode == 0 { + t.Errorf("--oidc-subject-claim was silently accepted for target=%s source=%s", + tc.target, tc.source) + } + if !strings.Contains(res.stderr, "is not applicable") { + t.Errorf("stderr must say the flag is not applicable, got:\n%s", res.stderr) + } + }) + } +} + +// TestGenerator_InvalidTargetReportedAsTarget guards the diagnostic ordering. +// The claim check runs outside the target switch, so without an explicit +// --target check first, a typo'd target combined with a perfectly good +// --oidc-subject-claim reported the claim as "not applicable" and sent the +// operator off to drop a flag that was never the problem. +func TestGenerator_InvalidTargetReportedAsTarget(t *testing.T) { + for _, withClaim := range []bool{false, true} { + name := "without claim" + args := []string{ + "--target", "Aws", // capitalised: a plausible typo + "--source", "gcp", + "--account-name", "prod", + "--account-id", "123456789012", + "--output", "-", + } + if withClaim { + name = "with claim" + args = append(args, "--oidc-subject-claim", "123456789012345678901") + } + t.Run(name, func(t *testing.T) { + res := runGenerator(t, args...) + if res.exitCode == 0 { + t.Fatalf("--target Aws was accepted (exit 0)") + } + if !strings.Contains(res.stderr, "--target must be aws, azure, or gcp") { + t.Errorf("a bad --target must be reported as a --target problem, got:\n%s", res.stderr) + } + }) + } +} + +// TestGenerator_OverlongClaimIsNotEchoed checks that no rejection path echoes a +// multi-kilobyte argument back at the operator, whichever diagnosis it reports. +// Every branch that formats the claim goes through displayClaim, so this holds +// independently of the order the checks run in. +func TestGenerator_OverlongClaimIsNotEchoed(t *testing.T) { + const claimLen = 50_000 + claim := strings.Repeat("A", claimLen) + + for _, tc := range []struct{ name, target, source, wantText string }{ + // Required path: the length cap is what rejects it. + {"required path", "aws", "gcp", "over the 255-byte limit"}, + // Not-applicable path: the flag does not belong here at all, and saying + // so is more useful than complaining about the length. + {"not applicable path", "gcp", "gcp", "is not applicable"}, + } { + t.Run(tc.name, func(t *testing.T) { + res := runGenerator(t, + "--target", tc.target, + "--source", tc.source, + "--account-name", "prod", + "--account-id", "123456789012", + "--oidc-subject-claim", claim, + "--output", filepath.Join(t.TempDir(), "generated.tfvars"), + ) + if res.exitCode == 0 { + t.Fatalf("a %d-character claim was accepted (exit 0)", claimLen) + } + if !strings.Contains(res.stderr, tc.wantText) { + t.Errorf("expected %q in the rejection, got:\n%s", tc.wantText, res.stderr) + } + // Generous bound: any branch that echoed the claim in full would + // blow past this by three orders of magnitude. + if len(res.stderr) > 2000 { + t.Errorf("the rejected claim must not be echoed back, but stderr is %d bytes", len(res.stderr)) + } + }) + } +} + +// TestGenerator_ValidationPrecedesTemplateRead proves the ordering directly +// rather than by inference: pointing --templates-dir at an empty directory +// makes any rendering attempt fail with a "read ...: no such file" error, so +// getting the validation error instead is proof the flag was rejected before +// the generator ever touched a template. +func TestGenerator_ValidationPrecedesTemplateRead(t *testing.T) { + emptyTemplates := t.TempDir() + args := append(awsWIFArgs("$(id)", filepath.Join(t.TempDir(), "generated.tfvars")), + "--templates-dir", emptyTemplates) + res := runGenerator(t, args...) + + if res.exitCode == 0 { + t.Fatalf("hostile claim accepted (exit 0); stdout:\n%s", res.stdout) + } + if !strings.Contains(res.stderr, "--oidc-subject-claim") { + t.Errorf("expected the validation error, got:\n%s", res.stderr) + } + if strings.Contains(res.stderr, "no such file") { + t.Errorf("the generator read a template before validating the flag:\n%s", res.stderr) + } +} + +// TestGenerator_AcceptsValidOIDCSubjectClaim is the positive control: without +// it, a validator that rejected everything would pass every test above. +// +// It uses --format cf-params rather than the default tfvars output because the +// script's iacData is missing the CUDlyAPIURL / SourceAccountID / ContactEmail +// fields the tfvars templates reference, so every tfvars path fails to render +// on main today. That drift predates this change and is tracked in #1690; the +// cf-params path renders end to end. +func TestGenerator_AcceptsValidOIDCSubjectClaim(t *testing.T) { + cases := []struct { + name string + claim string + }{ + {"gcp service account numeric unique id", "123456789012345678901"}, + {"azure managed identity object id", "11111111-2222-3333-4444-555555555555"}, + {"kubernetes style subject", "system:serviceaccount:cudly:reader"}, + {"github actions style subject", "repo:LeanerCloud/CUDly:ref:refs/heads/main"}, + {"email style subject", "cudly@example-project.iam.gserviceaccount.com"}, + {"at the length cap", strings.Repeat("1", 255)}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + res := runGenerator(t, + "--target", "aws", + "--source", "azure", + "--tenant-id", "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee", + "--format", "cf-params", + "--account-name", "prod", + "--account-id", "123456789012", + "--oidc-subject-claim", tc.claim, + "--output", "-", + ) + if res.exitCode != 0 { + t.Fatalf("valid claim %q rejected (exit %d); stderr:\n%s", tc.claim, res.exitCode, res.stderr) + } + want := `"ParameterKey": "OIDCSubjectClaim", "ParameterValue": "` + tc.claim + `"` + if !strings.Contains(res.stdout, want) { + t.Errorf("rendered cf-params must carry the claim verbatim.\nwant substring: %s\ngot:\n%s", + want, res.stdout) + } + }) + } +} + +// TestGenerator_CrossAccountDoesNotRequireSubjectClaim guards the scoping of +// the new requirement. target=aws with source=aws is the cross-account +// (external ID) path: it renders aws-cross-account.tfvars.tmpl, which has no +// OIDC trust policy at all, so demanding a subject claim there would break a +// working combination. +// +// It asserts only on stderr, not on the exit code, because that invocation +// currently fails for an unrelated pre-existing reason: iacData is missing the +// SourceAccountID / CUDlyAPIURL / ContactEmail / OIDCIssuerHost fields the +// tfvars and deploy-script templates reference, so no tfvars path of this +// script renders on main today. That drift is tracked in #1690; once it is +// fixed this can also assert exitCode == 0. +func TestGenerator_CrossAccountDoesNotRequireSubjectClaim(t *testing.T) { + res := runGenerator(t, + "--target", "aws", + "--source", "aws", + "--account-name", "tgt", + "--account-id", "999888777666", + "--output", "-", + ) + if strings.Contains(res.stderr, "--oidc-subject-claim") { + t.Errorf("the aws->aws cross-account path must not require --oidc-subject-claim, got:\n%s", res.stderr) + } +}