From a36582f5c200b15eda34fb25aa5e3e1cc86f44dc Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 26 Oct 2026 19:30:23 +0000 Subject: [PATCH] fix(iac/federation): make the AWS-target CloudFormation template loadable The trust policy in iac/federation/aws-target/cloudformation/template.yaml used !Sub expressions as the keys of its StringEquals map. CloudFormation does not accept an intrinsic function as a map key, so the template could not be loaded at all (cfn-lint E0000 "Unhashable type") and deploy-cfn.sh from a downloaded AWS-target bundle created no stack. AssumeRolePolicyDocument is now a single Fn::Sub JSON string producing the same policy: one Allow statement for the OIDC provider, sts:AssumeRoleWithWebIdentity, StringEquals on :aud and :sub, the host still derived from OIDCIssuerURL. Fn::Sub inserts raw text, so OIDCIssuerURL, OIDCAudience and OIDCSubjectClaim now also reject '"', '\' and control characters, and no admitted value can leave its JSON string. Tests render the document with the in-repo evaluator (now with !If and the template's Conditions), decode it with a duplicate-key-rejecting decoder, and compare it to the exact intended policy, including for every ASCII character in each parameter. A bundle test checks the download ships the committed template byte for byte and runs cfn-lint on it when available, and a pinned cfn-lint pre-commit hook now lints every CloudFormation template. Closes #1489 Co-Authored-By: Claude Opus 5.5 --- .pre-commit-config.yaml | 22 ++ iac/aws_target_cfn_test.go | 167 ++++++--- iac/aws_target_cfn_trust_policy_test.go | 326 ++++++++++++++++++ .../aws-target/cloudformation/template.yaml | 101 ++++-- .../aws-target/terraform/variables.tf | 23 +- .../api/handler_federation_cfn_params_test.go | 39 +++ scripts/generate-federation-iac.go | 17 +- 7 files changed, 591 insertions(+), 104 deletions(-) create mode 100644 iac/aws_target_cfn_trust_policy_test.go diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index fa1272951..19c3a3725 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -145,6 +145,28 @@ repos: types: [yaml] files: ^\.github/workflows/ + # CloudFormation linting + # + # The aws-target federation template shipped to customers once used + # intrinsic functions as map keys, which CloudFormation cannot load at all + # (cfn-lint E0000), and the repo's own Go evaluator accepted it (#1489). + # check-yaml above excludes these directories because PyYAML cannot read the + # !Ref/!Sub short forms, so this hook is what checks they load. Local hook + # with the version pinned in additional_dependencies, like zizmor above. + # + # --non-zero-exit-code error: errors (templates CloudFormation rejects) + # fail the hook. Warnings do not fail it, because the + # cloudformation/stacks/CUDly and aws-cross-account templates carry a W3005 + # (redundant DependsOn) that is not a load or deploy failure. + - repo: local + hooks: + - id: cfn-lint + name: Lint CloudFormation templates + language: python + additional_dependencies: ['cfn-lint==1.58.0'] + entry: cfn-lint --non-zero-exit-code error + files: ^(cloudformation/stacks/[^/]+|iac/federation/[^/]+/cloudformation)/template\.ya?ml$ + # Markdown linting - repo: https://github.com/igorshubovych/markdownlint-cli rev: 76b3d32d3f4b965e1d6425253c59407420ae2c43 # frozen: v0.47.0 diff --git a/iac/aws_target_cfn_test.go b/iac/aws_target_cfn_test.go index 0bc9127e7..ccd5c818e 100644 --- a/iac/aws_target_cfn_test.go +++ b/iac/aws_target_cfn_test.go @@ -3,7 +3,6 @@ package iac import ( "fmt" "regexp" - "sort" "strconv" "strings" "testing" @@ -57,22 +56,32 @@ func loadAWSTargetCFN(t *testing.T) *yaml.Node { return doc.Content[0] } -// cfnEval evaluates the subset of CloudFormation intrinsics the trust -// policy's condition keys use (!Ref, !Select, !Split, !Sub) against the given -// parameter values. Anything else is an error, so a rewrite into a form this -// cannot model fails the test instead of passing unevaluated. -func cfnEval(n *yaml.Node, params map[string]string) (any, error) { +// cfnEnv is what the evaluator below resolves references against: refs holds +// parameter values and resource attributes (keyed "Resource.Attribute", the +// way ${OIDCProvider.Arn} names them), and conds holds the template's +// evaluated Conditions for !If. +type cfnEnv struct { + refs map[string]string + conds map[string]bool +} + +// cfnEval evaluates the subset of CloudFormation intrinsics the trust policy +// uses (!Ref, !Select, !Split, !Sub, !If, and the !Equals and !Not its +// condition is built from) against env. Anything else is an error, so a +// rewrite into a form this cannot model fails the test instead of passing +// unevaluated. +func cfnEval(n *yaml.Node, env cfnEnv) (any, error) { switch n.Tag { case "!!str", "!!int": return n.Value, nil case "!Ref": - v, ok := params[n.Value] + v, ok := env.refs[n.Value] if !ok { return nil, fmt.Errorf("!Ref to %q, which is not a parameter value under test", n.Value) } return v, nil case "!Split": - args, err := cfnArgs(n, 2, params) + args, err := cfnArgs(n, 2, env) if err != nil { return nil, err } @@ -82,7 +91,7 @@ func cfnEval(n *yaml.Node, params map[string]string) (any, error) { } return strings.Split(s, delim), nil case "!Select": - args, err := cfnArgs(n, 2, params) + args, err := cfnArgs(n, 2, env) if err != nil { return nil, err } @@ -93,19 +102,41 @@ func cfnEval(n *yaml.Node, params map[string]string) (any, error) { } return list[idx], nil case "!Sub": - return cfnSub(n, params) + return cfnSub(n, env) + case "!If": + return cfnIf(n, env) + case "!Equals": + args, err := cfnArgs(n, 2, env) + if err != nil { + return nil, err + } + a, b, ok := twoStrings(args) + if !ok { + return nil, fmt.Errorf("!Equals arguments are not strings: %v", args) + } + return a == b, nil + case "!Not": + args, err := cfnArgs(n, 1, env) + if err != nil { + return nil, err + } + b, ok := args[0].(bool) + if !ok { + return nil, fmt.Errorf("!Not argument is not a condition: %v", args[0]) + } + return !b, nil } return nil, fmt.Errorf("unsupported node tag %q (value %q)", n.Tag, n.Value) } // cfnArgs evaluates the elements of a sequence-form intrinsic. -func cfnArgs(n *yaml.Node, want int, params map[string]string) ([]any, error) { +func cfnArgs(n *yaml.Node, want int, env cfnEnv) ([]any, error) { if n.Kind != yaml.SequenceNode || len(n.Content) != want { return nil, fmt.Errorf("%s wants a %d-element list", n.Tag, want) } out := make([]any, 0, want) for _, c := range n.Content { - v, err := cfnEval(c, params) + v, err := cfnEval(c, env) if err != nil { return nil, err } @@ -120,10 +151,43 @@ func twoStrings(args []any) (string, string, bool) { return a, b, ok1 && ok2 } +// cfnIf evaluates only the branch its named condition selects, as +// CloudFormation does. +func cfnIf(n *yaml.Node, env cfnEnv) (any, error) { + if n.Kind != yaml.SequenceNode || len(n.Content) != 3 || n.Content[0].Tag != "!!str" { + return nil, fmt.Errorf("!If wants [condition name, value if true, value if false]") + } + cond, ok := env.conds[n.Content[0].Value] + if !ok { + return nil, fmt.Errorf("!If names condition %q, which the template does not define", n.Content[0].Value) + } + if cond { + return cfnEval(n.Content[1], env) + } + return cfnEval(n.Content[2], env) +} + +// cfnConditions evaluates the template's Conditions section for refs. +func cfnConditions(t *testing.T, root *yaml.Node, refs map[string]string) map[string]bool { + t.Helper() + section := mapPath(t, root, "Conditions") + conds := map[string]bool{} + for i := 0; i+1 < len(section.Content); i += 2 { + v, err := cfnEval(section.Content[i+1], cfnEnv{refs: refs}) + require.NoError(t, err, "condition %s", section.Content[i].Value) + b, ok := v.(bool) + require.True(t, ok, "condition %s does not evaluate to a boolean", section.Content[i].Value) + conds[section.Content[i].Value] = b + } + return conds +} + var subVarRe = regexp.MustCompile(`\$\{([^}!]+)\}`) -// cfnSub renders both the string and the [string, {vars}] forms of !Sub. -func cfnSub(n *yaml.Node, params map[string]string) (any, error) { +// cfnSub renders both the string and the [string, {vars}] forms of !Sub. Like +// CloudFormation it substitutes in a single pass: text a variable expands to +// is not scanned for further ${...} references. +func cfnSub(n *yaml.Node, env cfnEnv) (any, error) { format, vars := n, map[string]string{} if n.Kind == yaml.SequenceNode { if len(n.Content) != 2 || n.Content[1].Kind != yaml.MappingNode { @@ -132,7 +196,7 @@ func cfnSub(n *yaml.Node, params map[string]string) (any, error) { format = n.Content[0] m := n.Content[1] for i := 0; i+1 < len(m.Content); i += 2 { - v, err := cfnEval(m.Content[i+1], params) + v, err := cfnEval(m.Content[i+1], env) if err != nil { return nil, err } @@ -149,7 +213,7 @@ func cfnSub(n *yaml.Node, params map[string]string) (any, error) { if v, ok := vars[name]; ok { return v } - if v, ok := params[name]; ok { + if v, ok := env.refs[name]; ok { return v } missing = fmt.Errorf("!Sub references %q, which is neither a variable nor a parameter value under test", name) @@ -174,47 +238,44 @@ func TestAWSTargetCFN_HasNoIssuerHostParameter(t *testing.T) { require.NotContains(t, string(raw), "${OIDCIssuerHost}", "nothing may still reference the removed parameter") } -// TestAWSTargetCFN_ConditionKeysDeriveFromIssuerURL evaluates every key of the -// trust policy's StringEquals block for real issuer URLs and checks it is the -// host Terraform derives, followed by :aud or :sub. -func TestAWSTargetCFN_ConditionKeysDeriveFromIssuerURL(t *testing.T) { - root := loadAWSTargetCFN(t) - urlPattern := regexp.MustCompile(mapPath(t, root, "Parameters", "OIDCIssuerURL", "AllowedPattern").Value) - statements := mapPath(t, root, "Resources", "CUDlyRole", "Properties", "AssumeRolePolicyDocument", "Statement") - require.Equal(t, yaml.SequenceNode, statements.Kind) - require.Len(t, statements.Content, 1, "the trust policy has exactly one statement") - stringEquals := mapPath(t, statements.Content[0], "Condition", "StringEquals") - - for _, url := range []string{ - "https://accounts.google.com", - "https://login.microsoftonline.com/11111111-1111-1111-1111-111111111111/v2.0", - } { - t.Run(url, func(t *testing.T) { - require.True(t, urlPattern.MatchString(url), "fixture must satisfy OIDCIssuerURL's AllowedPattern") - host := terraformConditionHost(url) - var keys []string - for i := 0; i+1 < len(stringEquals.Content); i += 2 { - v, err := cfnEval(stringEquals.Content[i], map[string]string{"OIDCIssuerURL": url}) - require.NoError(t, err, "condition key %d must be computed from OIDCIssuerURL alone", i/2) - keys = append(keys, v.(string)) - } - sort.Strings(keys) - require.Equal(t, []string{host + ":aud", host + ":sub"}, keys) - }) - } -} - // TestCFNEval_Positive is the control for the evaluator above: it must -// resolve the derivation the template uses, and reject an unknown parameter, -// so the condition-key test cannot pass by evaluating nothing. +// resolve the derivation and the condition the template uses, and reject an +// unknown parameter or condition, so the trust-policy tests cannot pass by +// evaluating nothing. func TestCFNEval_Positive(t *testing.T) { - var n yaml.Node - require.NoError(t, yaml.Unmarshal([]byte(`!Sub ["${Host}:aud", {Host: !Select [1, !Split ["https://", !Ref OIDCIssuerURL]]}]`), &n)) - got, err := cfnEval(n.Content[0], map[string]string{"OIDCIssuerURL": "https://accounts.google.com"}) + env := cfnEnv{refs: map[string]string{"OIDCIssuerURL": "https://accounts.google.com"}} + eval := func(src string, env cfnEnv) (any, error) { + var n yaml.Node + require.NoError(t, yaml.Unmarshal([]byte(src), &n)) + return cfnEval(n.Content[0], env) + } + + got, err := eval(`!Sub ["${Host}:aud", {Host: !Select [1, !Split ["https://", !Ref OIDCIssuerURL]]}]`, env) require.NoError(t, err) require.Equal(t, "accounts.google.com:aud", got) - require.NoError(t, yaml.Unmarshal([]byte(`!Sub "${OIDCIssuerHost}:aud"`), &n)) - _, err = cfnEval(n.Content[0], map[string]string{"OIDCIssuerURL": "https://accounts.google.com"}) + _, err = eval(`!Sub "${OIDCIssuerHost}:aud"`, env) require.Error(t, err) + + for aud, want := range map[string]bool{"": false, "api://x": true} { + got, err = eval(`!Not [!Equals [!Ref OIDCAudience, ""]]`, cfnEnv{refs: map[string]string{"OIDCAudience": aud}}) + require.NoError(t, err) + require.Equal(t, want, got, "audience %q", aud) + } + + ifSrc := `!If [HasAudience, !Ref OIDCAudience, sts.amazonaws.com]` + refs := map[string]string{"OIDCAudience": "api://x"} + got, err = eval(ifSrc, cfnEnv{refs: refs, conds: map[string]bool{"HasAudience": true}}) + require.NoError(t, err) + require.Equal(t, "api://x", got) + got, err = eval(ifSrc, cfnEnv{refs: refs, conds: map[string]bool{"HasAudience": false}}) + require.NoError(t, err) + require.Equal(t, "sts.amazonaws.com", got) + _, err = eval(ifSrc, cfnEnv{refs: refs}) + require.Error(t, err, "an !If on an undefined condition must not evaluate") + + // Single-pass substitution: a value containing ${...} is not re-expanded. + got, err = eval(`!Sub ["${A}", {A: "${B}"}]`, cfnEnv{refs: map[string]string{"B": "expanded"}}) + require.NoError(t, err) + require.Equal(t, "${B}", got) } diff --git a/iac/aws_target_cfn_trust_policy_test.go b/iac/aws_target_cfn_trust_policy_test.go new file mode 100644 index 000000000..00671c03f --- /dev/null +++ b/iac/aws_target_cfn_trust_policy_test.go @@ -0,0 +1,326 @@ +package iac + +import ( + "encoding/json" + "errors" + "fmt" + "io" + "regexp" + "strconv" + "strings" + "testing" + + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" +) + +// Regression tests for #1489. The aws-target template built its trust policy +// as a YAML mapping whose two StringEquals keys were !Sub expressions (the +// host is derived from OIDCIssuerURL, #324). CloudFormation does not accept an +// intrinsic function as a map key, so the template could not be loaded at all +// (cfn-lint E0000 "Unhashable type") and deploy-cfn.sh created no stack. The +// document is now a single !Sub JSON string. Fn::Sub inserts raw text, so +// these tests also prove that no value the parameters' AllowedPatterns admit +// can escape the JSON string it lands in and change the policy. + +// trustPolicyProviderARN stands in for ${OIDCProvider.Arn}. It is not a real +// ARN; the tests only need it to come back out of the rendered JSON unchanged. +const trustPolicyProviderARN = "OIDC-PROVIDER-ARN-PLACEHOLDER" + +// trustPolicyDefaults are valid values for every parameter the trust policy +// reads, so each test can vary one at a time. +func trustPolicyDefaults() map[string]string { + return map[string]string{ + "OIDCIssuerURL": "https://accounts.google.com", + "OIDCAudience": "api://11111111-1111-1111-1111-111111111111", + "OIDCSubjectClaim": "123456789012345678901", + "OIDCThumbprint": "", + "RoleName": "CUDly-WIF", + } +} + +// renderTrustPolicy evaluates CUDlyRole's AssumeRolePolicyDocument for params +// and returns the JSON text CloudFormation would hand to IAM. +func renderTrustPolicy(t *testing.T, root *yaml.Node, params map[string]string) (string, error) { + t.Helper() + doc := mapPath(t, root, "Resources", "CUDlyRole", "Properties", "AssumeRolePolicyDocument") + require.Equal(t, "!Sub", doc.Tag, "the trust policy must be a single !Sub JSON string (#1489)") + refs := map[string]string{"OIDCProvider.Arn": trustPolicyProviderARN} + for k, v := range params { + refs[k] = v + } + v, err := cfnEval(doc, cfnEnv{refs: refs, conds: cfnConditions(t, root, refs)}) + if err != nil { + return "", err + } + s, ok := v.(string) + require.True(t, ok, "the trust policy must render to a string") + return s, nil +} + +// wantTrustPolicy is the exact policy the template must produce: one Allow +// statement for the OIDC provider, sts:AssumeRoleWithWebIdentity only, and a +// StringEquals block holding exactly the :aud and :sub keys. Comparing the +// whole document means an added statement, operator, key or principal fails +// the test, not only a changed value. +func wantTrustPolicy(host, aud, sub string) map[string]any { + return map[string]any{ + "Version": "2012-10-17", + "Statement": []any{map[string]any{ + "Effect": "Allow", + "Principal": map[string]any{"Federated": trustPolicyProviderARN}, + "Action": "sts:AssumeRoleWithWebIdentity", + "Condition": map[string]any{"StringEquals": map[string]any{ + host + ":aud": aud, + host + ":sub": sub, + }}, + }}, + } +} + +// decodeStrictJSON decodes a single JSON document and, unlike json.Unmarshal, +// rejects duplicate object keys and trailing data. A duplicate key is how an +// injected value would override a condition (IAM's handling of duplicates is +// not something to rely on), so it must not collapse silently. +func decodeStrictJSON(s string) (any, error) { + dec := json.NewDecoder(strings.NewReader(s)) + v, err := decodeJSONValue(dec) + if err != nil { + return nil, err + } + switch extra, err := dec.Token(); { + case errors.Is(err, io.EOF): + return v, nil + case err != nil: + return nil, fmt.Errorf("after the JSON document: %w", err) + default: + return nil, fmt.Errorf("data after the JSON document: %v", extra) + } +} + +func decodeJSONValue(dec *json.Decoder) (any, error) { + tok, err := dec.Token() + if err != nil { + return nil, err + } + switch tok { + case json.Delim('{'): + obj := map[string]any{} + for dec.More() { + kt, kerr := dec.Token() + if kerr != nil { + return nil, kerr + } + k := kt.(string) + if _, dup := obj[k]; dup { + return nil, fmt.Errorf("duplicate key %q", k) + } + if obj[k], err = decodeJSONValue(dec); err != nil { + return nil, err + } + } + _, err = dec.Token() + return obj, err + case json.Delim('['): + arr := []any{} + for dec.More() { + v, verr := decodeJSONValue(dec) + if verr != nil { + return nil, verr + } + arr = append(arr, v) + } + _, err = dec.Token() + return arr, err + } + return tok, nil +} + +// TestDecodeStrictJSON_Control proves the decoder the trust-policy tests rely +// on rejects what json.Unmarshal would let through silently. +func TestDecodeStrictJSON_Control(t *testing.T) { + got, err := decodeStrictJSON(`{"a": ["b", {"c": "d"}]}`) + require.NoError(t, err) + require.Equal(t, map[string]any{"a": []any{"b", map[string]any{"c": "d"}}}, got) + for _, bad := range []string{ + `{"a": "b", "a": "c"}`, + `{"a": {"b": "c", "b": "d"}}`, + `{"a": "b"} {"a": "c"}`, + `{"a": "b"}]`, + "{\"a\": \"b\x01\"}", + } { + _, err := decodeStrictJSON(bad) + require.Error(t, err, "%q", bad) + } +} + +// paramAdmits reports whether CloudFormation would accept value for the named +// parameter: its AllowedPattern as a whole-string match (Java's +// Matcher.matches(), which is what CloudFormation applies) and its MinLength, +// where the parameter declares them. +func paramAdmits(t *testing.T, root *yaml.Node, name, value string) bool { + t.Helper() + param := mapPath(t, root, "Parameters", name) + if minLen := mapValue(param, "MinLength"); minLen != nil { + n, err := strconv.Atoi(minLen.Value) + require.NoError(t, err) + if len(value) < n { + return false + } + } + allowed := mapValue(param, "AllowedPattern") + if allowed == nil { + return true + } + return regexp.MustCompile(`^(?:` + allowed.Value + `)$`).MatchString(value) +} + +// TestAWSTargetCFN_MappingKeysArePlainStrings is the load-failure regression +// itself: CloudFormation rejects the whole template when any mapping key is an +// intrinsic function or other non-string node, as the trust policy's condition +// keys were. It walks every mapping in the template. +func TestAWSTargetCFN_MappingKeysArePlainStrings(t *testing.T) { + var walk func(n *yaml.Node, path string) + walk = func(n *yaml.Node, path string) { + if n.Kind == yaml.MappingNode { + for i := 0; i+1 < len(n.Content); i += 2 { + k := n.Content[i] + require.True(t, k.Kind == yaml.ScalarNode && k.Tag == "!!str", + "mapping key under %s at line %d is a %s node tagged %q; CloudFormation cannot load a template whose map keys are not plain strings (#1489)", + path, k.Line, kindName(k.Kind), k.Tag) + walk(n.Content[i+1], path+"/"+k.Value) + } + return + } + for _, c := range n.Content { + walk(c, path) + } + } + walk(loadAWSTargetCFN(t), "") +} + +func kindName(k yaml.Kind) string { + switch k { + case yaml.ScalarNode: + return "scalar" + case yaml.SequenceNode: + return "sequence" + case yaml.MappingNode: + return "mapping" + } + return fmt.Sprintf("kind-%d", k) +} + +// TestAWSTargetCFN_ConditionKeysDeriveFromIssuerURL renders the trust policy +// for real issuer URLs, with and without an audience, and checks it parses to +// exactly the intended policy: condition keys built from the host Terraform +// derives, the audience falling back to sts.amazonaws.com when empty, and the +// subject pinned. +func TestAWSTargetCFN_ConditionKeysDeriveFromIssuerURL(t *testing.T) { + root := loadAWSTargetCFN(t) + for _, url := range []string{ + "https://accounts.google.com", + "https://login.microsoftonline.com/11111111-1111-1111-1111-111111111111/v2.0", + } { + for _, aud := range []string{"", "api://11111111-1111-1111-1111-111111111111"} { + t.Run(url+"/aud="+aud, func(t *testing.T) { + params := trustPolicyDefaults() + params["OIDCIssuerURL"], params["OIDCAudience"] = url, aud + for name, v := range params { + require.True(t, paramAdmits(t, root, name, v), "fixture %s=%q must satisfy the parameter's constraints", name, v) + } + rendered, err := renderTrustPolicy(t, root, params) + require.NoError(t, err) + got, err := decodeStrictJSON(rendered) + require.NoError(t, err, "rendered trust policy is not valid JSON:\n%s", rendered) + + wantAud := aud + if wantAud == "" { + wantAud = "sts.amazonaws.com" + } + require.Equal(t, wantTrustPolicy(terraformConditionHost(url), wantAud, params["OIDCSubjectClaim"]), got) + }) + } + } +} + +// probeValues returns, for each parameter the trust policy reads, values that +// carry every ASCII character and a few non-ASCII ones at each place the +// parameter's pattern treats differently (the URL's host and path segments). +func probeValues() map[string][]string { + runes := make([]rune, 0, 132) + for r := rune(0); r < 0x80; r++ { + runes = append(runes, r) + } + runes = append(runes, ' ', 'é', '
', '
') + out := map[string][]string{} + for _, r := range runes { + c := string(r) + out["OIDCIssuerURL"] = append(out["OIDCIssuerURL"], + "https://acc"+c+"ounts.google.com", "https://login.microsoftonline.com/te"+c+"nant/v2.0") + out["OIDCAudience"] = append(out["OIDCAudience"], "api://a"+c+"b") + out["OIDCSubjectClaim"] = append(out["OIDCSubjectClaim"], "12"+c+"34") + } + return out +} + +// TestAWSTargetCFN_TrustPolicyValuesCannotEscapeJSON is the injection +// property: for every probe value the parameter's constraints admit, the +// rendered document must still decode to exactly the intended policy with that +// value in its place. A '"' or '\' or control character that got through would +// make the JSON invalid, re-escape the value, or add a key, and fail here. +func TestAWSTargetCFN_TrustPolicyValuesCannotEscapeJSON(t *testing.T) { + root := loadAWSTargetCFN(t) + for name, values := range probeValues() { + t.Run(name, func(t *testing.T) { + admitted := 0 + for _, v := range values { + if !paramAdmits(t, root, name, v) { + continue + } + admitted++ + params := trustPolicyDefaults() + params[name] = v + rendered, err := renderTrustPolicy(t, root, params) + require.NoError(t, err) + got, err := decodeStrictJSON(rendered) + require.NoError(t, err, "%s=%q is admitted but breaks the trust policy JSON:\n%s", name, v, rendered) + want := wantTrustPolicy(terraformConditionHost(params["OIDCIssuerURL"]), params["OIDCAudience"], params["OIDCSubjectClaim"]) + require.Equal(t, want, got, "%s=%q is admitted but changes the trust policy", name, v) + } + require.Positive(t, admitted, "the probe must exercise admitted values, not only rejected ones") + }) + } +} + +// TestAWSTargetCFN_RejectsJSONBreakout pins the characters that would let a +// value leave its JSON string, and whole breakout payloads, as rejected at +// submission time for every parameter the trust policy reads. +func TestAWSTargetCFN_RejectsJSONBreakout(t *testing.T) { + root := loadAWSTargetCFN(t) + cases := map[string][]string{ + "OIDCIssuerURL": { + `https://accounts.google.com"`, + `https://accounts.google.com/a\b`, + "https://accounts.google.com/a\nb", + `https://x:aud":"y","x:sub":"z"},"StringLike":{"accounts.google.com`, + }, + "OIDCAudience": { + `api://x"`, + `api://x"`, + "api://x\tb", + `x","accounts.google.com:sub":"other`, + }, + "OIDCSubjectClaim": { + `123"`, + `123"`, + "123\x00", + `1"}},{"Effect":"Allow","Principal":{"AWS":"arn:aws:iam::other:root"},"Action":"sts:AssumeRole`, + }, + } + for name, values := range cases { + for _, v := range values { + require.False(t, paramAdmits(t, root, name, v), "%s must reject %q", name, v) + } + } +} diff --git a/iac/federation/aws-target/cloudformation/template.yaml b/iac/federation/aws-target/cloudformation/template.yaml index 4e2e1ce0c..e1c39a311 100644 --- a/iac/federation/aws-target/cloudformation/template.yaml +++ b/iac/federation/aws-target/cloudformation/template.yaml @@ -15,8 +15,14 @@ Parameters: string-equal. Azure AD: https://login.microsoftonline.com//v2.0 GCP: https://accounts.google.com - AllowedPattern: "^https://[^/]+(/[^/]+)*$" - ConstraintDescription: "OIDCIssuerURL must be https:// and must not contain a trailing slash." + # Single-quoted so the regex needs no YAML escaping. Besides '/', each + # segment excludes whitespace, '"', '\' and control characters: the host + # is substituted into the trust policy's JSON text (see CUDlyRole), and + # any of those could end or corrupt the JSON string it lands in. + AllowedPattern: '^https://[^/\s"\\\x00-\x1F]+(/[^/\s"\\\x00-\x1F]+)*$' + ConstraintDescription: > + OIDCIssuerURL must be https://, must not contain a trailing slash, and + must not contain whitespace, '"', '\' or control characters. OIDCAudience: Type: String @@ -29,16 +35,21 @@ Parameters: literal string sts.amazonaws.com, so only tokens carrying exactly that audience are accepted. Default: "" - AllowedPattern: "^$|^[^\\s*$]+$" + # '"', '\' and control characters are excluded because the value is + # substituted into the trust policy's JSON text (see CUDlyRole). + AllowedPattern: '^$|^[^\s*$"\\\x00-\x1F]+$' ConstraintDescription: > - OIDCAudience must be empty, or a string containing no whitespace, '$' - or '*' anywhere. + OIDCAudience must be empty, or a string containing no whitespace, '$', + '*', '"', '\' or control characters anywhere. '$' is rejected because the trust policy is an IAM policy document and IAM expands ${...} policy variables inside Condition values: an audience of ${accounts.google.com:aud} would expand to the presented token's own aud claim and therefore match every token. '*' is rejected because StringEquals compares it literally rather than as a wildcard, silently producing a role nobody can assume. + '"', '\' and control characters are rejected because the trust policy + is built as JSON text with this value substituted in, and they could + end or corrupt the JSON string it lands in. OIDCThumbprint: Type: String @@ -102,10 +113,12 @@ Parameters: accounts.google.com. Mirrors the required oidc_subject_claim variable in the Terraform module. MinLength: 1 - AllowedPattern: "^[^\\s*$]+$" + # '"', '\' and control characters are excluded because the value is + # substituted into the trust policy's JSON text (see CUDlyRole). + AllowedPattern: '^[^\s*$"\\\x00-\x1F]+$' ConstraintDescription: > OIDCSubjectClaim is required and must be a non-empty string containing - no whitespace, '$' or '*' anywhere. + no whitespace, '$', '*', '"', '\' or control characters anywhere. '$' is rejected because the trust policy below is an IAM policy document (Version 2012-10-17) and IAM expands ${...} policy variables inside Condition values. A subject of ${accounts.google.com:sub} would expand @@ -115,6 +128,9 @@ Parameters: time rather than deployed. '*' is rejected because StringEquals compares it literally rather than as a wildcard, silently producing a role nobody can assume. + '"', '\' and control characters are rejected because the trust policy + is built as JSON text with this value substituted in, and they could + end or corrupt the JSON string it lands in. RoleName: Type: String @@ -246,33 +262,50 @@ Resources: Properties: RoleName: !Ref RoleName Description: Role assumed by CUDly via workload identity federation - AssumeRolePolicyDocument: - Version: "2012-10-17" - Statement: - # Both :aud and :sub are always enforced. There is intentionally no - # subject-less variant of this statement: omitting :sub would trust - # every identity the issuer can mint. OIDCSubjectClaim is a required - # parameter, so this condition can never collapse to an empty match. - - Effect: Allow - Principal: - Federated: !GetAtt OIDCProvider.Arn - Action: sts:AssumeRoleWithWebIdentity - # The condition-key host is OIDCIssuerURL minus its https:// - # prefix, derived here rather than taken as a second parameter so - # the two cannot disagree (#324). OIDCIssuerURL's AllowedPattern - # guarantees exactly one leading https:// and no trailing slash, so - # element 1 of the split is the whole host and path. This is the - # same value as local.oidc_condition_host in the Terraform module. - Condition: - StringEquals: - ? !Sub - - "${Host}:aud" - - Host: !Select [1, !Split ["https://", !Ref OIDCIssuerURL]] - : !If [HasAudience, !Ref OIDCAudience, sts.amazonaws.com] - ? !Sub - - "${Host}:sub" - - Host: !Select [1, !Split ["https://", !Ref OIDCIssuerURL]] - : !Ref OIDCSubjectClaim + # Both :aud and :sub are always enforced. There is intentionally no + # subject-less variant of this statement: omitting :sub would trust + # every identity the issuer can mint. OIDCSubjectClaim is a required + # parameter, so this condition can never collapse to an empty match. + # + # The condition-key host is OIDCIssuerURL minus its https:// prefix, + # derived here rather than taken as a second parameter so the two cannot + # disagree (#324). OIDCIssuerURL's AllowedPattern guarantees exactly one + # leading https:// and no trailing slash, so element 1 of the split is + # the whole host and path. This is the same value as + # local.oidc_condition_host in the Terraform module. + # + # The document is a single Fn::Sub JSON string, not a YAML mapping, + # because its two condition keys are computed and CloudFormation does + # not accept an intrinsic function as a map key: a mapping with a !Sub + # key fails to load at all (cfn-lint E0000 "Unhashable type", #1489). + # Fn::Sub inserts raw text with no JSON escaping, so every value that + # reaches this string is confined to it by its AllowedPattern: + # OIDCIssuerURL, OIDCAudience and OIDCSubjectClaim all reject '"', '\' + # and control characters. The provider ARN is the only other value, and + # its caller-supplied part is the same OIDCIssuerURL. No parameter value + # can therefore end its JSON string early and add, replace or widen a + # key. iac/aws_target_cfn_trust_policy_test.go renders this document and + # checks it parses to exactly the intended policy. + AssumeRolePolicyDocument: !Sub + - | + { + "Version": "2012-10-17", + "Statement": [ + { + "Effect": "Allow", + "Principal": {"Federated": "${OIDCProvider.Arn}"}, + "Action": "sts:AssumeRoleWithWebIdentity", + "Condition": { + "StringEquals": { + "${Host}:aud": "${Audience}", + "${Host}:sub": "${OIDCSubjectClaim}" + } + } + } + ] + } + - Host: !Select [1, !Split ["https://", !Ref OIDCIssuerURL]] + Audience: !If [HasAudience, !Ref OIDCAudience, sts.amazonaws.com] ManagedPolicyArns: - !Ref CUDlyPolicy diff --git a/iac/federation/aws-target/terraform/variables.tf b/iac/federation/aws-target/terraform/variables.tf index c978ee7b1..47248d827 100644 --- a/iac/federation/aws-target/terraform/variables.tf +++ b/iac/federation/aws-target/terraform/variables.tf @@ -30,10 +30,13 @@ variable "oidc_audience" { # audience of ${accounts.google.com:aud} expands to the token's own aud # claim and matches every token. Audience is the only other control on this # trust policy, so it gets the same guard, including the whitespace - # rejection that keeps it equivalent to the CloudFormation AllowedPattern - # ^$|^[^\s*$]+$. An empty audience still passes (it contains none of the - # rejected characters) and falls back to sts.amazonaws.com, matching that - # pattern's ^$ branch without needing an explicit empty-string clause. + # rejection the CloudFormation template's OIDCAudience AllowedPattern also + # applies. An empty audience still passes (it contains none of the rejected + # characters) and falls back to sts.amazonaws.com, matching that pattern's + # ^$ branch without needing an explicit empty-string clause. The template + # additionally rejects '"', '\' and control characters, because it builds + # its trust policy by substituting the value into JSON text (#1489). + # jsonencode escapes them here, so they cannot change this policy. validation { condition = !can(regex("[\\s*$]", var.oidc_audience)) error_message = "oidc_audience must not contain whitespace, '$' or '*'. IAM expands $${...} policy variables inside Condition values, so a value such as $${accounts.google.com:aud} would expand to the token's own aud claim and match every token. '*' is compared literally by StringEquals." @@ -67,11 +70,13 @@ variable "oidc_subject_claim" { # expands to the presented token's own sub claim, making the condition a # tautology that matches every identity the issuer can mint. Reject '$' # outright rather than emit a trust policy that only looks restricted. - # Whitespace is rejected in the same expression so this stays byte-for-byte - # equivalent to the CloudFormation AllowedPattern ^[^\s*$]+$ that the - # template's ConstraintDescription claims to mirror. No real OIDC subject - # (GCP numeric IDs, Azure GUIDs, system:serviceaccount:, GitHub, GitLab, - # SPIFFE, Auth0, Bitbucket) contains whitespace. + # Whitespace is rejected in the same expression, as the CloudFormation + # template's OIDCSubjectClaim AllowedPattern also does. That pattern + # additionally rejects '"', '\' and control characters, because the + # template builds its trust policy by substituting the value into JSON text + # (#1489); jsonencode escapes them here, so they cannot change this policy. + # No real OIDC subject (GCP numeric IDs, Azure GUIDs, system:serviceaccount:, + # GitHub, GitLab, SPIFFE, Auth0, Bitbucket) contains whitespace. validation { condition = !can(regex("[\\s*$]", var.oidc_subject_claim)) error_message = "oidc_subject_claim must not contain whitespace, '$' or '*'. IAM expands $${...} policy variables inside Condition values, so a value such as $${accounts.google.com:sub} would expand to the token's own sub claim and match every identity the issuer can mint. '*' is compared literally by StringEquals and would silently produce a role nobody can assume." diff --git a/internal/api/handler_federation_cfn_params_test.go b/internal/api/handler_federation_cfn_params_test.go index b91074976..b47a9afd7 100644 --- a/internal/api/handler_federation_cfn_params_test.go +++ b/internal/api/handler_federation_cfn_params_test.go @@ -7,10 +7,14 @@ import ( "encoding/base64" "encoding/json" "io" + "os" + "os/exec" + "path/filepath" "regexp" "sort" "strings" "testing" + "time" "github.com/stretchr/testify/require" "gopkg.in/yaml.v3" @@ -121,3 +125,38 @@ func TestCFNBundle_OverridesMatchTemplateParameters(t *testing.T) { }) } } + +// TestCFNBundle_TemplatePassesCFNLint covers #1489's acceptance check on the +// bundle a customer actually downloads, not only the in-repo evaluator in +// iac/aws_target_cfn_trust_policy_test.go: the aws-target template once +// rendered fine there yet could not be loaded by CloudFormation at all, +// because its trust policy used intrinsic functions as map keys. +// +// The byte comparison always runs. It is what lets the cfn-lint pre-commit +// hook, which lints the committed file, vouch for every downloaded bundle. The +// cfn-lint run itself needs cfn-lint on PATH (pip install cfn-lint) and skips +// without it, the same way the Terraform bundle test skips without terraform. +// The aws-target template must lint with no findings at all, warnings +// included. +func TestCFNBundle_TemplatePassesCFNLint(t *testing.T) { + committed, err := os.ReadFile(awsTargetCFNTemplate) + require.NoError(t, err) + for _, source := range []string{"gcp", "azure"} { + t.Run(source, func(t *testing.T) { + template, _, _ := cfnBundleFiles(t, source) + require.Equal(t, string(committed), string(template), + "the bundle must ship the committed aws-target template byte for byte, so linting that file covers the download") + + cfnLint, err := exec.LookPath("cfn-lint") + if err != nil { + t.Skip("cfn-lint not on PATH") + } + path := filepath.Join(t.TempDir(), "template.yaml") + require.NoError(t, os.WriteFile(path, template, 0o600)) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + out, err := exec.CommandContext(ctx, cfnLint, path).CombinedOutput() + require.NoError(t, err, "cfn-lint rejects the template in the %s-source AWS-target bundle:\n%s", source, out) + }) + } +} diff --git a/scripts/generate-federation-iac.go b/scripts/generate-federation-iac.go index 0e9ef53ed..9741f2ef4 100644 --- a/scripts/generate-federation-iac.go +++ b/scripts/generate-federation-iac.go @@ -446,14 +446,15 @@ func displayClaim(claim string) string { // 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. +// This is deliberately stricter than the checks the CloudFormation template and +// the Terraform module apply to the same value. Those run on a value that is +// already a typed parameter, so they only need to reject whitespace and what +// IAM itself mis-handles: '$' (IAM expands ${...} policy variables inside +// Condition values) and '*' (compared literally by StringEquals). The template +// also rejects '"', '\' and control characters, because it substitutes the +// value into its trust policy's JSON text (#1489). 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