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