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/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/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") + } +} + +// 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/known_issues/13_iac_aws_target.md b/known_issues/13_iac_aws_target.md index 20d747e2c..14624ac97 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:** #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`, +`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..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" @@ -84,6 +110,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 @@ -279,12 +309,172 @@ 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 { +// 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) data.OIDCAudience = awsOIDCAudience(source) + data.OIDCSubjectClaim = oidcSubjectClaim case "azure": data.SubscriptionID = data.AccountExternalID data.TenantID = tenantID @@ -299,9 +489,12 @@ func populateData(data *iacData, target, source, tenantID, projectID, saEmail st } 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() { @@ -314,6 +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: 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)") @@ -334,8 +528,8 @@ func main() { } data := iacData{AccountName: *accountName, AccountExternalID: *accountID, AccountSlug: slug, Source: *source} - if !populateData(&data, *target, *source, *tenantID, *projectID, *saEmail) { - 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) + } +}