From c7b8a058ba3ff4cdc23f64f71d4f69573ccfe334 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 3 Aug 2026 18:53:25 +0200 Subject: [PATCH 1/2] fix(scripts): add missing tfvars fields to generate-federation-iac.go iacData was missing ContactEmail, CUDlyAPIURL, and SourceAccountID, which every tfvars template references. text/template treats a missing struct field as a hard execution error, so the standalone generator failed on every target/source combination in its default --format=tfvars mode. ContactEmail and CUDlyAPIURL feed the optional auto-registration block and now come from new --contact-email/--cudly-api-url flags, defaulting to "" (the Terraform modules already treat empty as "skip registration"). SourceAccountID identifies the AWS account CUDly itself runs in, which the standalone script cannot resolve the way the server does via STS, so it now comes from a required --source-account-id flag for the two combinations that need it (--target aws --source aws, and --target gcp --source aws), failing loud instead of silently rendering an empty or wrong trust boundary. Adds an end-to-end regression test that runs each routed --target/--source combination through the real --format=tfvars path, plus coverage for the new fail-loud validation. Closes #1709 --- scripts/generate-federation-iac.go | 72 +++++++- scripts/generate-federation-iac_test.go | 217 ++++++++++++++++++++++++ 2 files changed, 283 insertions(+), 6 deletions(-) create mode 100644 scripts/generate-federation-iac_test.go diff --git a/scripts/generate-federation-iac.go b/scripts/generate-federation-iac.go index 26d5e2537..c23214945 100644 --- a/scripts/generate-federation-iac.go +++ b/scripts/generate-federation-iac.go @@ -52,9 +52,11 @@ // --oidc-subject-claim "11111111-2222-3333-4444-555555555555" // // # AWS target, AWS source — cross-account IAM role tfvars +// # --source-account-id is CUDly's own AWS account, not --account-id (the target) // go run scripts/generate-federation-iac.go \ // --target aws --source aws \ -// --account-name "target-aws" --account-id "999888777666" +// --account-name "target-aws" --account-id "999888777666" \ +// --source-account-id "111122223333" // // # Azure target — WIF App Registration tfvars // go run scripts/generate-federation-iac.go \ @@ -63,9 +65,11 @@ // --tenant-id "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" // // # GCP target, AWS source — WIF pool tfvars +// # --source-account-id is CUDly's own AWS account (required when --source aws) // go run scripts/generate-federation-iac.go \ // --target gcp --source aws \ -// --account-name "prod-gcp" --account-id "my-gcp-project" +// --account-name "prod-gcp" --account-id "my-gcp-project" \ +// --source-account-id "111122223333" // // # GCP target, GCP source — service account impersonation tfvars // go run scripts/generate-federation-iac.go \ @@ -107,6 +111,12 @@ type iacData struct { AccountExternalID string AccountSlug string Source string + // SourceAccountID is the AWS account ID where CUDly itself runs, used by + // aws-cross-account.tfvars.tmpl and gcp-wif.tfvars.tmpl (source=aws). The + // server resolves this via STS; the standalone script has no such context, + // so it comes from the --source-account-id flag. Not the same as + // AccountExternalID/--account-id, which is the target account. + SourceAccountID string // AWS WIF / cross-account OIDCIssuerURL string OIDCAudience string @@ -121,6 +131,15 @@ type iacData struct { ProjectID string ServiceAccountEmail string OIDCIssuerURI string + // CUDlyAPIURL and ContactEmail feed the optional auto-registration block + // in the tfvars/deploy-script templates. The server pre-fills them from + // the dashboard URL and the authenticated session; the standalone script + // has neither, so they come from --cudly-api-url / --contact-email and + // default to "" (which the Terraform modules treat as "skip + // registration" — see the cudly_api_url/contact_email variable + // descriptions in iac/federation/*/terraform/variables.tf). + CUDlyAPIURL string + ContactEmail string } var slugRE = regexp.MustCompile(`[^a-z0-9]+`) @@ -455,10 +474,31 @@ func validateOIDCSubjectClaim(claim string, mode subjectClaimMode) error { // validFederationTargets in internal/api/handler_federation.go. var validTargets = map[string]bool{"aws": true, "azure": true, "gcp": true} +// requireSourceAccountID fills data.SourceAccountID from sourceAccountID when +// source is aws, or reports errMsg if it was not supplied. It is a no-op for +// any other source. Both the aws-target and gcp-target arms of populateData +// need this same aws-source gate (the aws-cross-account and gcp-wif-from-aws +// paths respectively), so it is factored out rather than duplicated to keep +// populateData's branching within the pre-commit gocyclo budget. +func requireSourceAccountID(data *iacData, source, sourceAccountID, errMsg string) error { + if source != "aws" { + return nil + } + if sourceAccountID == "" { + return errors.New(errMsg) + } + data.SourceAccountID = sourceAccountID + return nil +} + // 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 { +// stops the run before any template is rendered. This covers two independent +// gates: --oidc-subject-claim, required (and only meaningful) for an AWS +// target with a non-AWS source; and --source-account-id, required whenever +// the source is AWS itself (the aws-cross-account and gcp-wif-from-aws paths), +// since CUDly's own account has no other way to reach this standalone script. +func populateData(data *iacData, target, source, tenantID, projectID, saEmail, oidcSubjectClaim, sourceAccountID 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. @@ -475,6 +515,11 @@ func populateData(data *iacData, target, source, tenantID, projectID, saEmail, o data.OIDCIssuerURL = awsOIDCIssuer(source, tenantID) data.OIDCAudience = awsOIDCAudience(source) data.OIDCSubjectClaim = oidcSubjectClaim + if err := requireSourceAccountID(data, source, sourceAccountID, + "--source-account-id is required for --target aws --source aws "+ + "(the AWS account ID where CUDly itself runs; --account-id is the target account)"); err != nil { + return err + } case "azure": data.SubscriptionID = data.AccountExternalID data.TenantID = tenantID @@ -488,6 +533,11 @@ func populateData(data *iacData, target, source, tenantID, projectID, saEmail, o data.ServiceAccountEmail = "cudly@" + data.ProjectID + ".iam.gserviceaccount.com" } data.OIDCIssuerURI = gcpOIDCIssuerURI(source, tenantID) + if err := requireSourceAccountID(data, source, sourceAccountID, + "--source-account-id is required for --target gcp --source aws "+ + "(the AWS account ID where CUDly itself runs; --account-id is the target project)"); err != nil { + return err + } default: // Unreachable while validTargets and these arms agree; kept so that // adding a target to the map without an arm here fails loudly instead of @@ -508,6 +558,9 @@ func main() { 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") + sourceAccountID := flag.String("source-account-id", "", "AWS account ID where CUDly itself runs (required for --target aws --source aws, and --target gcp --source aws; NOT the same as --account-id, which is the target account)") + contactEmail := flag.String("contact-email", "", "Contact email pre-filled for CUDly auto-registration (optional; empty skips auto-registration)") + cudlyAPIURL := flag.String("cudly-api-url", "", "CUDly API base URL pre-filled for auto-registration (optional; empty skips auto-registration)") 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)") @@ -527,8 +580,15 @@ func main() { slug = slugify(*accountID) } - data := iacData{AccountName: *accountName, AccountExternalID: *accountID, AccountSlug: slug, Source: *source} - if err := populateData(&data, *target, *source, *tenantID, *projectID, *saEmail, *oidcSubjectClaim); err != nil { + data := iacData{ + AccountName: *accountName, + AccountExternalID: *accountID, + AccountSlug: slug, + Source: *source, + ContactEmail: *contactEmail, + CUDlyAPIURL: *cudlyAPIURL, + } + if err := populateData(&data, *target, *source, *tenantID, *projectID, *saEmail, *oidcSubjectClaim, *sourceAccountID); 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..4a0e753a8 --- /dev/null +++ b/scripts/generate-federation-iac_test.go @@ -0,0 +1,217 @@ +package main + +// generate-federation-iac_test.go drives scripts/generate-federation-iac.go +// end to end via `go run`, the same way an operator would invoke it. The +// script carries a `//go:build ignore` tag, so it is never compiled as part +// of this package (or any other) — the only way to exercise its actual +// --format=tfvars rendering path, and therefore the only way to catch drift +// between iacData and the tfvars templates it renders, is to run it as a +// subprocess. See #1709: iacData was missing three fields (ContactEmail, +// CUDlyAPIURL, SourceAccountID) referenced by every tfvars template, and +// nothing exercised this path so the break went unnoticed. + +import ( + "os/exec" + "strings" + "testing" +) + +// runViaGoRun invokes `go run generate-federation-iac.go ` from the +// repository root (the test binary's working directory is this package's +// directory, scripts/, so ".." is the repo root — matching how the script's +// own doc comment says to invoke it). Named distinctly from the +// generate_federation_iac_test.go file's own runGenerator (same package, +// different signature: that one runs a pre-built binary) to avoid a symbol +// collision between the two test files. +func runViaGoRun(t *testing.T, args ...string) (stdout string, err error) { + t.Helper() + cmd := exec.Command("go", append([]string{"run", "scripts/generate-federation-iac.go"}, args...)...) + cmd.Dir = ".." + out, err := cmd.CombinedOutput() + return string(out), err +} + +// TestGenerateFederationIaC_TfvarsCombinations runs every --target/--source +// combination routed by singleFileTmpl through the real --format=tfvars +// path (the default format) and asserts a successful render plus the +// presence of the fields that data. Failing to add a field the templates +// reference is exactly the bug in #1709: text/template treats a missing +// struct field as a hard execution error, not an empty string. +func TestGenerateFederationIaC_TfvarsCombinations(t *testing.T) { + tests := []struct { + name string + args []string + wantContain []string + }{ + { + name: "aws target, azure source", + args: []string{ + "--target", "aws", "--source", "azure", + "--account-name", "Acme", "--account-id", "123456789012", + "--tenant-id", "11111111-2222-3333-4444-555555555555", + "--oidc-subject-claim", "11111111-2222-3333-4444-555555555555", + "--contact-email", "ops@example.com", "--cudly-api-url", "https://cudly.example.com", + "--output", "-", + }, + wantContain: []string{ + `cudly_api_url = "https://cudly.example.com"`, + `contact_email = "ops@example.com"`, + }, + }, + { + name: "aws target, gcp source", + args: []string{ + "--target", "aws", "--source", "gcp", + "--account-name", "Acme", "--account-id", "123456789012", + "--oidc-subject-claim", "123456789012345678901", + "--contact-email", "ops@example.com", "--cudly-api-url", "https://cudly.example.com", + "--output", "-", + }, + wantContain: []string{ + `cudly_api_url = "https://cudly.example.com"`, + `contact_email = "ops@example.com"`, + }, + }, + { + name: "aws target, aws source (cross-account)", + args: []string{ + "--target", "aws", "--source", "aws", + "--account-name", "Acme", "--account-id", "999888777666", + "--source-account-id", "111122223333", + "--contact-email", "ops@example.com", "--cudly-api-url", "https://cudly.example.com", + "--output", "-", + }, + wantContain: []string{ + `source_account_id = "111122223333"`, + `cudly_api_url = "https://cudly.example.com"`, + `contact_email = "ops@example.com"`, + }, + }, + { + name: "azure target, aws source", + args: []string{ + "--target", "azure", "--source", "aws", + "--account-name", "Acme", "--account-id", "sub-1234", + "--tenant-id", "11111111-2222-3333-4444-555555555555", + "--contact-email", "ops@example.com", "--cudly-api-url", "https://cudly.example.com", + "--output", "-", + }, + wantContain: []string{ + `cudly_api_url = "https://cudly.example.com"`, + `contact_email = "ops@example.com"`, + }, + }, + { + name: "azure target, gcp source", + args: []string{ + "--target", "azure", "--source", "gcp", + "--account-name", "Acme", "--account-id", "sub-1234", + "--tenant-id", "11111111-2222-3333-4444-555555555555", + "--contact-email", "ops@example.com", "--cudly-api-url", "https://cudly.example.com", + "--output", "-", + }, + wantContain: []string{ + `cudly_api_url = "https://cudly.example.com"`, + `contact_email = "ops@example.com"`, + }, + }, + { + name: "gcp target, gcp source (sa impersonation)", + args: []string{ + "--target", "gcp", "--source", "gcp", + "--account-name", "Acme", "--account-id", "my-project", + "--contact-email", "ops@example.com", "--cudly-api-url", "https://cudly.example.com", + "--output", "-", + }, + wantContain: []string{ + `cudly_api_url = "https://cudly.example.com"`, + `contact_email = "ops@example.com"`, + }, + }, + { + name: "gcp target, aws source (WIF pool)", + args: []string{ + "--target", "gcp", "--source", "aws", + "--account-name", "Acme", "--account-id", "my-project", + "--source-account-id", "111122223333", + "--contact-email", "ops@example.com", "--cudly-api-url", "https://cudly.example.com", + "--output", "-", + }, + wantContain: []string{ + `aws_account_id = "111122223333"`, + `cudly_api_url = "https://cudly.example.com"`, + `contact_email = "ops@example.com"`, + }, + }, + { + name: "gcp target, azure source (WIF pool)", + args: []string{ + "--target", "gcp", "--source", "azure", + "--account-name", "Acme", "--account-id", "my-project", + "--tenant-id", "11111111-2222-3333-4444-555555555555", + "--contact-email", "ops@example.com", "--cudly-api-url", "https://cudly.example.com", + "--output", "-", + }, + wantContain: []string{ + `cudly_api_url = "https://cudly.example.com"`, + `contact_email = "ops@example.com"`, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out, err := runViaGoRun(t, tt.args...) + if err != nil { + t.Fatalf("generator failed: %v\noutput:\n%s", err, out) + } + for _, want := range tt.wantContain { + if !strings.Contains(out, want) { + t.Errorf("output missing %q\noutput:\n%s", want, out) + } + } + }) + } +} + +// TestGenerateFederationIaC_RequiresSourceAccountID guards the fail-loud +// behavior added alongside the #1709 fix: --source-account-id has no +// sensible default in the standalone script (unlike the server, which +// resolves it via STS), so target/source combinations that need it must +// error explicitly instead of silently rendering an empty or wrong account +// ID into the trust policy. +func TestGenerateFederationIaC_RequiresSourceAccountID(t *testing.T) { + tests := []struct { + name string + args []string + }{ + { + name: "aws target, aws source, no source-account-id", + args: []string{ + "--target", "aws", "--source", "aws", + "--account-name", "Acme", "--account-id", "999888777666", + "--output", "-", + }, + }, + { + name: "gcp target, aws source, no source-account-id", + args: []string{ + "--target", "gcp", "--source", "aws", + "--account-name", "Acme", "--account-id", "my-project", + "--output", "-", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + out, err := runViaGoRun(t, tt.args...) + if err == nil { + t.Fatalf("expected failure without --source-account-id, got success:\n%s", out) + } + if !strings.Contains(out, "--source-account-id is required") { + t.Errorf("expected error naming --source-account-id, got:\n%s", out) + } + }) + } +} From 9b0c09a466eaad7fdea1df563acea9e45151347c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 5 Aug 2026 11:22:36 +0200 Subject: [PATCH 2/2] fix(scripts): validate --source-account-id is 12 ASCII digits requireSourceAccountID previously only rejected an empty value. A non-empty but malformed value (wrong length, non-digit characters, leading sign, whitespace padding) reached data.SourceAccountID unchecked and flowed into the rendered tfvars, where the problem would surface far from its cause as a confusing Terraform or AWS error. Reject anything that isn't exactly 12 ASCII digits before it is written to data, covering both consumers: --target aws --source aws and --target gcp --source aws. --- scripts/generate-federation-iac.go | 23 +++++++++--- scripts/generate-federation-iac_test.go | 48 +++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 5 deletions(-) diff --git a/scripts/generate-federation-iac.go b/scripts/generate-federation-iac.go index c23214945..10de1b57b 100644 --- a/scripts/generate-federation-iac.go +++ b/scripts/generate-federation-iac.go @@ -474,12 +474,22 @@ func validateOIDCSubjectClaim(claim string, mode subjectClaimMode) error { // validFederationTargets in internal/api/handler_federation.go. var validTargets = map[string]bool{"aws": true, "azure": true, "gcp": true} +// sourceAccountIDRE matches an AWS account ID: exactly 12 ASCII digits, no +// whitespace padding, leading sign, or Unicode digit look-alikes. AWS account +// IDs are always 12 digits; anything else could not be a real one and would +// otherwise flow untouched into the generated tfvars (#1710 CR). +var sourceAccountIDRE = regexp.MustCompile(`^[0-9]{12}$`) + // requireSourceAccountID fills data.SourceAccountID from sourceAccountID when -// source is aws, or reports errMsg if it was not supplied. It is a no-op for -// any other source. Both the aws-target and gcp-target arms of populateData -// need this same aws-source gate (the aws-cross-account and gcp-wif-from-aws -// paths respectively), so it is factored out rather than duplicated to keep -// populateData's branching within the pre-commit gocyclo budget. +// source is aws, or reports errMsg if it was not supplied. A non-empty value +// is also checked against sourceAccountIDRE before being written to data: a +// malformed ID must fail loud here rather than reach the rendered tfvars, +// where the problem would surface far from its cause as a confusing +// Terraform or AWS error. It is a no-op for any other source. Both the +// aws-target and gcp-target arms of populateData need this same aws-source +// gate (the aws-cross-account and gcp-wif-from-aws paths respectively), so it +// is factored out rather than duplicated to keep populateData's branching +// within the pre-commit gocyclo budget. func requireSourceAccountID(data *iacData, source, sourceAccountID, errMsg string) error { if source != "aws" { return nil @@ -487,6 +497,9 @@ func requireSourceAccountID(data *iacData, source, sourceAccountID, errMsg strin if sourceAccountID == "" { return errors.New(errMsg) } + if !sourceAccountIDRE.MatchString(sourceAccountID) { + return fmt.Errorf("--source-account-id %q is not a valid AWS account ID: it must be exactly 12 digits", sourceAccountID) + } data.SourceAccountID = sourceAccountID return nil } diff --git a/scripts/generate-federation-iac_test.go b/scripts/generate-federation-iac_test.go index 4a0e753a8..9e468be7c 100644 --- a/scripts/generate-federation-iac_test.go +++ b/scripts/generate-federation-iac_test.go @@ -215,3 +215,51 @@ func TestGenerateFederationIaC_RequiresSourceAccountID(t *testing.T) { }) } } + +// TestGenerateFederationIaC_RejectsMalformedSourceAccountID guards the CR +// finding on #1710: a non-empty --source-account-id is not necessarily a +// valid one. Before this check, a malformed value flowed straight into +// data.SourceAccountID and out into the rendered tfvars, where the problem +// would only surface later as a confusing Terraform or AWS error far from +// its actual cause. Covers both consumers of SourceAccountID: --target aws +// --source aws (aws-cross-account) and --target gcp --source aws (WIF pool). +func TestGenerateFederationIaC_RejectsMalformedSourceAccountID(t *testing.T) { + tests := []struct { + name string + target string + sourceAccountID string + }{ + {name: "aws target, too short", target: "aws", sourceAccountID: "12345"}, + {name: "aws target, non-digit characters", target: "aws", sourceAccountID: "1111222233aa"}, + {name: "aws target, leading plus sign", target: "aws", sourceAccountID: "+11122223333"}, + {name: "aws target, whitespace padded", target: "aws", sourceAccountID: " 111122223333"}, + {name: "gcp target, too short", target: "gcp", sourceAccountID: "12345"}, + {name: "gcp target, non-digit characters", target: "gcp", sourceAccountID: "1111222233aa"}, + {name: "gcp target, leading plus sign", target: "gcp", sourceAccountID: "+11122223333"}, + {name: "gcp target, whitespace padded", target: "gcp", sourceAccountID: " 111122223333"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + accountID := "999888777666" + if tt.target == "gcp" { + accountID = "my-project" + } + out, err := runViaGoRun(t, + "--target", tt.target, "--source", "aws", + "--account-name", "Acme", "--account-id", accountID, + "--source-account-id", tt.sourceAccountID, + "--output", "-", + ) + if err == nil { + t.Fatalf("expected failure for malformed --source-account-id %q, got success:\n%s", tt.sourceAccountID, out) + } + if !strings.Contains(out, "is not a valid AWS account ID") { + t.Errorf("expected error naming the invalid --source-account-id, got:\n%s", out) + } + if !strings.Contains(out, tt.sourceAccountID) { + t.Errorf("expected error to name the rejected value %q, got:\n%s", tt.sourceAccountID, out) + } + }) + } +}