diff --git a/scripts/generate-federation-iac.go b/scripts/generate-federation-iac.go index 26d5e2537..10de1b57b 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,44 @@ 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. 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 + } + 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 +} + // 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 +528,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 +546,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 +571,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 +593,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..9e468be7c --- /dev/null +++ b/scripts/generate-federation-iac_test.go @@ -0,0 +1,265 @@ +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) + } + }) + } +} + +// 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) + } + }) + } +}