From 8729074243fc9d353583cac12b131e501a317bd0 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 23 Oct 2026 04:32:05 +0000 Subject: [PATCH] fix(scripts): add OIDCIssuerHost to generate-federation-iac and test for drift iacData in scripts/generate-federation-iac.go lacked OIDCIssuerHost, which aws-cfn-deploy.sh.tmpl references, so --format bundle failed for every AWS target with a non-AWS source. Add the field and derive it from OIDCIssuerURL the same way the server does. The doc comment asked people to keep iacData in sync with the server's federationIaCData, and nothing enforced it. New tests read both structs with go/parser and every template with text/template: they fail when the two structs stop having the same fields, or when any template references a field iacData lacks. Doing it from source keeps the script decoupled from internal/api. The doc comment now points at these tests. Also drop the stale "iacData is missing fields" rationale from three tests. TestGenerator_CrossAccountDoesNotRequireSubjectClaim now asserts a successful render instead of only checking stderr (audit A14-028). Closes #153 Co-Authored-By: Claude Opus 5.5 --- scripts/generate-federation-iac.go | 16 +- scripts/generate_federation_iac_drift_test.go | 209 ++++++++++++++++++ .../generate_federation_iac_escape_test.go | 10 +- scripts/generate_federation_iac_test.go | 29 +-- 4 files changed, 239 insertions(+), 25 deletions(-) create mode 100644 scripts/generate_federation_iac_drift_test.go diff --git a/scripts/generate-federation-iac.go b/scripts/generate-federation-iac.go index 7e1852c84..aab02e98e 100644 --- a/scripts/generate-federation-iac.go +++ b/scripts/generate-federation-iac.go @@ -108,8 +108,13 @@ import ( "unicode" ) -// iacData mirrors internal/api/handler_federation.go:federationIaCData. -// Keep in sync with any template variable changes. +// iacData is this script's copy of federationIaCData in +// internal/api/handler_federation.go: both render the same templates in +// internal/iacfiles/templates. The script cannot import that type (it would +// pull internal/api into a //go:build ignore dev tool), so the two are held in +// step by tests in scripts/generate_federation_iac_drift_test.go instead: they +// fail when the two structs stop having the same fields, or when any template +// references a field iacData lacks (#153). type iacData struct { AccountName string AccountExternalID string @@ -123,7 +128,11 @@ type iacData struct { SourceAccountID string // AWS WIF / cross-account OIDCIssuerURL string - OIDCAudience string + // OIDCIssuerHost is OIDCIssuerURL without its https:// prefix, the form + // IAM uses in condition keys; aws-cfn-deploy.sh.tmpl passes it to + // CloudFormation. Derived the same way as the server does. + OIDCIssuerHost 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). @@ -664,6 +673,7 @@ func populateData(data *iacData, target, source, tenantID, projectID, saEmail, o switch target { case "aws": data.OIDCIssuerURL = awsOIDCIssuer(source, tenantID) + data.OIDCIssuerHost = strings.TrimPrefix(data.OIDCIssuerURL, "https://") data.OIDCAudience = awsOIDCAudience(source) data.OIDCSubjectClaim = oidcSubjectClaim if err := requireSourceAccountID(data, source, sourceAccountID, diff --git a/scripts/generate_federation_iac_drift_test.go b/scripts/generate_federation_iac_drift_test.go new file mode 100644 index 000000000..96f456f60 --- /dev/null +++ b/scripts/generate_federation_iac_drift_test.go @@ -0,0 +1,209 @@ +package main + +// Drift guards for #153. generate-federation-iac.go renders the same templates +// as the server's GET /api/federation/iac, from its own copy of the template +// data struct (iacData, a copy of internal/api's federationIaCData). A comment +// asking people to keep the two in sync did not: iacData went without +// OIDCIssuerHost, which aws-cfn-deploy.sh.tmpl references, so +// `--format bundle` failed for every AWS target with a non-AWS source. +// +// The script carries //go:build ignore and internal/api is a large package, so +// neither struct is imported here. Both are read from source with go/parser, +// and every template is parsed with text/template/parse, so a field added to +// one side or used by a new template fails these tests without coupling the +// script to the server package. + +import ( + "archive/zip" + "go/ast" + "go/parser" + "go/token" + "io" + "os" + "path/filepath" + "sort" + "strings" + "testing" + "text/template" + "text/template/parse" + + "github.com/stretchr/testify/require" +) + +// structFieldNames returns the field names of the struct type typeName +// declared in the Go source file at path, sorted. +func structFieldNames(t *testing.T, path, typeName string) []string { + t.Helper() + file, err := parser.ParseFile(token.NewFileSet(), path, nil, parser.SkipObjectResolution) + require.NoError(t, err) + var names []string + found := false + ast.Inspect(file, func(n ast.Node) bool { + spec, ok := n.(*ast.TypeSpec) + if !ok || spec.Name.Name != typeName { + return true + } + st, ok := spec.Type.(*ast.StructType) + require.True(t, ok, "%s in %s is not a struct type", typeName, path) + found = true + for _, field := range st.Fields.List { + require.NotEmpty(t, field.Names, "embedded field in %s is not supported by this check", typeName) + for _, name := range field.Names { + names = append(names, name.Name) + } + } + return false + }) + require.True(t, found, "type %s not found in %s", typeName, path) + sort.Strings(names) + return names +} + +// templateFieldRefs adds to refs every top-level field that node reads from +// the template's data, mapped to the template that reads it. The templates +// never rebind dot, so .X always names a field of the data struct; a with or +// range block would break that assumption, so it fails the test instead of +// being guessed at. +func templateFieldRefs(t *testing.T, tmpl string, node parse.Node, refs map[string]string) { + t.Helper() + switch n := node.(type) { + case nil: + case *parse.ListNode: + if n == nil { + return + } + for _, child := range n.Nodes { + templateFieldRefs(t, tmpl, child, refs) + } + case *parse.ActionNode: + templateFieldRefs(t, tmpl, n.Pipe, refs) + case *parse.IfNode: + templateFieldRefs(t, tmpl, n.Pipe, refs) + templateFieldRefs(t, tmpl, n.List, refs) + templateFieldRefs(t, tmpl, n.ElseList, refs) + case *parse.PipeNode: + if n == nil { + return + } + for _, cmd := range n.Cmds { + for _, arg := range cmd.Args { + templateFieldRefs(t, tmpl, arg, refs) + } + } + case *parse.FieldNode: + refs[n.Ident[0]] = tmpl + case *parse.VariableNode: + if n.Ident[0] == "$" && len(n.Ident) > 1 { + refs[n.Ident[1]] = tmpl + } + case *parse.TextNode, *parse.CommentNode, *parse.StringNode, *parse.NumberNode, + *parse.BoolNode, *parse.NilNode, *parse.DotNode, *parse.IdentifierNode: + default: + t.Fatalf("%s: template node %T (%s) is not handled by this check; extend templateFieldRefs", tmpl, node, node) + } +} + +// iacTemplateFieldRefs parses every template in internal/iacfiles/templates +// and returns the data fields they reference, each mapped to one template +// that uses it. +func iacTemplateFieldRefs(t *testing.T) map[string]string { + t.Helper() + paths, err := filepath.Glob(filepath.Join(repoRoot, "internal", "iacfiles", "templates", "*.tmpl")) + require.NoError(t, err) + require.NotEmpty(t, paths, "no templates found") + refs := map[string]string{} + for _, path := range paths { + raw, err := os.ReadFile(path) + require.NoError(t, err) + // Parsed the way the script and the server parse them, so the + // builtin functions such as eq are defined. + tmpl, err := template.New(filepath.Base(path)).Parse(string(raw)) + require.NoError(t, err, "parse %s", path) + for _, tt := range tmpl.Templates() { + templateFieldRefs(t, filepath.Base(path), tt.Root, refs) + } + } + return refs +} + +// TestIaCData_MatchesServerTemplateData fails when iacData and the server's +// federationIaCData stop having the same fields. Templates are shared, so a +// field the server fills is one a template can start using at any time. +func TestIaCData_MatchesServerTemplateData(t *testing.T) { + script := structFieldNames(t, scriptFile, "iacData") + server := structFieldNames(t, filepath.Join(repoRoot, "internal", "api", "handler_federation.go"), "federationIaCData") + require.Equal(t, server, script, + "iacData in scripts/%s and federationIaCData in internal/api/handler_federation.go render the same "+ + "templates and must have the same fields (#153)", scriptFile) +} + +// TestIaCData_CoversEveryTemplateField fails as soon as any template in +// internal/iacfiles/templates references a field iacData lacks. text/template +// treats a missing struct field as an execution error, so such a template +// cannot be rendered by the script at all. +func TestIaCData_CoversEveryTemplateField(t *testing.T) { + have := map[string]bool{} + for _, name := range structFieldNames(t, scriptFile, "iacData") { + have[name] = true + } + var missing []string + for field, tmpl := range iacTemplateFieldRefs(t) { + if !have[field] { + missing = append(missing, field+" (used by "+tmpl+")") + } + } + sort.Strings(missing) + require.Empty(t, missing, "iacData lacks fields that the templates reference: %s", strings.Join(missing, ", ")) +} + +// TestIaCData_FieldWalkerSeesKnownFields is the positive control for the two +// tests above: a parser that found no fields would make both pass vacuously. +func TestIaCData_FieldWalkerSeesKnownFields(t *testing.T) { + refs := iacTemplateFieldRefs(t) + for _, field := range []string{"AccountName", "CUDlyAPIURL", "OIDCIssuerHost", "SourceAccountID"} { + require.Contains(t, refs, field) + } + require.Contains(t, structFieldNames(t, scriptFile, "iacData"), "AccountName") +} + +// TestGenerator_AWSWIFBundleRendersDeployScript runs the real +// aws-cfn-deploy.sh.tmpl through --format bundle for each non-AWS source. It +// only reads the rendered script; running it would call the AWS CLI. +func TestGenerator_AWSWIFBundleRendersDeployScript(t *testing.T) { + const tenant = "aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee" + cases := []struct { + source, wantHost string + extra []string + }{ + {"gcp", "accounts.google.com", nil}, + {"azure", "login.microsoftonline.com/" + tenant + "/v2.0", []string{"--tenant-id", tenant}}, + } + for _, tc := range cases { + t.Run(tc.source, func(t *testing.T) { + bundle := filepath.Join(t.TempDir(), "bundle.zip") + args := append([]string{ + "--target", "aws", "--source", tc.source, "--format", "bundle", "--output", bundle, + "--account-name", "prod", "--account-id", validAccountIDFor("aws"), + "--oidc-subject-claim", "123456789012345678901", + }, tc.extra...) + res := runGenerator(t, args...) + require.Zero(t, res.exitCode, "stderr: %s", res.stderr) + script := readZipEntry(t, bundle, "cloudformation/deploy-cfn.sh") + require.Contains(t, script, `"OIDCIssuerHost=`+tc.wantHost+`"`) + }) + } +} + +// readZipEntry returns the contents of the named entry in the zip at path. +func readZipEntry(t *testing.T, path, name string) string { + t.Helper() + archive, err := zip.OpenReader(path) + require.NoError(t, err) + defer archive.Close() + entry, err := archive.Open(name) + require.NoError(t, err) + defer entry.Close() + b, err := io.ReadAll(entry) + require.NoError(t, err) + return string(b) +} diff --git a/scripts/generate_federation_iac_escape_test.go b/scripts/generate_federation_iac_escape_test.go index ab69012f5..e4175d4e8 100644 --- a/scripts/generate_federation_iac_escape_test.go +++ b/scripts/generate_federation_iac_escape_test.go @@ -227,11 +227,11 @@ func TestGenerator_CFParamsValuesCannotChangeStructure(t *testing.T) { }, got) } -// probeDeployScript stands in for aws-cfn-deploy.sh.tmpl. The real template -// cannot be rendered by this script today (iacData lacks OIDCIssuerHost, -// #153), so this probe exercises the same .sh.tmpl escaping path with the -// quoting shapes the real Bash templates use, and prints what Bash made of -// each value instead of calling any cloud CLI. +// probeDeployScript stands in for aws-cfn-deploy.sh.tmpl. Running the real +// script would call the AWS CLI, so this probe exercises the same .sh.tmpl +// escaping path with the quoting shapes the real Bash templates use, and +// prints what Bash made of each value instead of calling any cloud CLI. +// TestGenerator_AWSWIFBundleRendersDeployScript renders the real template. const probeDeployScript = `#!/usr/bin/env bash # Target: {{.AccountName}} ({{.AccountExternalID}}) set -euo pipefail diff --git a/scripts/generate_federation_iac_test.go b/scripts/generate_federation_iac_test.go index d3f4c4030..69280350f 100644 --- a/scripts/generate_federation_iac_test.go +++ b/scripts/generate_federation_iac_test.go @@ -434,13 +434,9 @@ func TestGenerator_ValidationPrecedesTemplateRead(t *testing.T) { } // 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. +// it, a validator that rejected everything would pass every test above. It +// checks the cf-params output because that is where the claim is easiest to +// read back exactly; the tfvars and bundle paths are covered elsewhere. func TestGenerator_AcceptsValidOIDCSubjectClaim(t *testing.T) { cases := []struct { name string @@ -482,23 +478,22 @@ func TestGenerator_AcceptsValidOIDCSubjectClaim(t *testing.T) { // 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. +// working combination. It asserts a successful render, not just the absence of +// a complaint, so a regression that breaks the path outright also fails (#153). func TestGenerator_CrossAccountDoesNotRequireSubjectClaim(t *testing.T) { res := runGenerator(t, "--target", "aws", "--source", "aws", "--account-name", "tgt", "--account-id", "999888777666", + "--source-account-id", "111122223333", "--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) + if res.exitCode != 0 { + t.Fatalf("the aws->aws cross-account path must render without --oidc-subject-claim (exit %d); stderr:\n%s", + res.exitCode, res.stderr) + } + if !strings.Contains(res.stdout, "999888777666") { + t.Errorf("rendered cross-account tfvars must carry the target account ID, got:\n%s", res.stdout) } }