diff --git a/internal/iacfiles/templates_aws_tagging_test.go b/internal/iacfiles/templates_aws_tagging_test.go index 9a363238c..f165ccdbf 100644 --- a/internal/iacfiles/templates_aws_tagging_test.go +++ b/internal/iacfiles/templates_aws_tagging_test.go @@ -5,6 +5,7 @@ import ( "os" "path/filepath" "regexp" + "strconv" "strings" "testing" @@ -76,59 +77,6 @@ func cfnStatements(t *testing.T, raw []byte) []awsPolicyStatement { return out } -var ( - tfActionList = regexp.MustCompile(`(?s)Action\s*=\s*\[(.*?)\]`) - tfResource = regexp.MustCompile(`Resource\s*=\s*"([^"]*)"`) - quoted = regexp.MustCompile(`"([^"]+)"`) -) - -// tfStatements returns the statement objects of a Terraform jsonencode policy: -// the brace-balanced object around every `Sid =`. -func tfStatements(t *testing.T, raw []byte) []awsPolicyStatement { - t.Helper() - text := string(raw) - sids := regexp.MustCompile(`Sid\s*=`).FindAllStringIndex(text, -1) - out := make([]awsPolicyStatement, 0, len(sids)) - for _, loc := range sids { - start, depth := loc[0], 0 - for ; start > 0; start-- { - if text[start] == '}' { - depth++ - } - if text[start] == '{' { - if depth == 0 { - break - } - depth-- - } - } - end, depth := loc[1], 0 - for ; end < len(text); end++ { - if text[end] == '{' { - depth++ - } - if text[end] == '}' { - if depth == 0 { - break - } - depth-- - } - } - chunk := text[start:end] - var stmt awsPolicyStatement - if m := tfActionList.FindStringSubmatch(chunk); m != nil { - for _, q := range quoted.FindAllStringSubmatch(m[1], -1) { - stmt.Action = append(stmt.Action, q[1]) - } - } - if m := tfResource.FindStringSubmatch(chunk); m != nil { - stmt.Resource = m[1] - } - out = append(out, stmt) - } - return out -} - // memberAccountPolicies lists the seven member-account purchase policies: // the two CLI onboarding templates, the four federation templates and the // legacy CUDly-CrossAccount stack. @@ -163,20 +111,191 @@ func TestMemberAccountPoliciesTagReservedInstancesOnly(t *testing.T) { for path, statements := range memberAccountPolicies(t) { t.Run(path, func(t *testing.T) { require.NotEmpty(t, statements, "no policy statements parsed") - var createTags int - for _, stmt := range statements { - for _, action := range stmt.Action { - switch action { - case "ec2:CreateTags": - assert.Equal(t, reservedInstanceARN, stmt.Resource, - "ec2:CreateTags must be scoped to reserved instances") - createTags++ - case "redshift:CreateTags": - t.Errorf("redshift:CreateTags must not be granted until it is verified and condition-scoped (#702)") - } + assert.Empty(t, createTagsViolations(statements)) + }) + } +} + +// createTagsViolations lists every way statements break the #702 tagging +// rule: ec2:CreateTags granted anywhere but reserved instances, granted other +// than exactly once, or redshift:CreateTags granted at all. +func createTagsViolations(statements []awsPolicyStatement) []string { + var problems []string + var createTags int + for _, stmt := range statements { + for _, action := range stmt.Action { + switch action { + case "ec2:CreateTags": + createTags++ + if stmt.Resource != reservedInstanceARN { + problems = append(problems, "ec2:CreateTags must be scoped to "+reservedInstanceARN+", got Resource "+strconv.Quote(stmt.Resource)) } + case "redshift:CreateTags": + problems = append(problems, "redshift:CreateTags must not be granted until it is verified and condition-scoped (#702)") } - assert.Equal(t, 1, createTags, "ec2:CreateTags must appear exactly once") + } + } + if createTags != 1 { + problems = append(problems, "ec2:CreateTags must appear exactly once, got "+strconv.Itoa(createTags)) + } + return problems +} + +// memberAccountTerraform lists the Terraform member-account policies, the +// only ones read through tfStatements. +var memberAccountTerraform = []string{ + "iac/federation/aws-cross-account/terraform/main.tf", + "iac/federation/aws-target/terraform/main.tf", +} + +// tfStatementMutants are edits to a member-account Terraform policy that widen +// ec2:CreateTags to every EC2 resource. Each is inserted as a new statement in +// front of the CostExplorer statement. None has a Sid, which is what the old +// Sid-anchored scan could not see (#782); the last is a Sid-carrying control. +var tfStatementMutants = map[string]string{ + "Sid-less statement": `{ + Effect = "Allow" + Action = ["ec2:CreateTags"] + Resource = "*" + }, +`, + "Sid-less statement with a scalar Action": `{ + Effect = "Allow" + Action = "ec2:CreateTags" + Resource = "*" + }, +`, + "Sid-less one-line statement": `{ Effect = "Allow", Action = ["ec2:CreateTags"], Resource = "*" }, +`, + "Sid-less statement with quoted keys and colons": `{ + "Effect": "Allow", + "Action": ["ec2:CreateTags"], + "Resource": ["*"], + }, +`, + "Sid-less statement behind a comment holding braces": `# } closing brace in a comment { + { + Effect = "Allow" # trailing comment } + Action = ["ec2:CreateTags"] + Resource = "*" + }, +`, + "control: statement with a Sid": `{ + Sid = "Widened" + Effect = "Allow" + Action = ["ec2:CreateTags"] + Resource = "*" + }, +`, +} + +// withStatementBeforeCostExplorer returns src with stmt inserted as a new +// statement just before the one whose Sid is CostExplorer. +func withStatementBeforeCostExplorer(t *testing.T, src, stmt string) string { + t.Helper() + sid := strings.Index(src, `Sid = "CostExplorer"`) + require.Positive(t, sid, "CostExplorer statement not found") + open := strings.LastIndex(src[:sid], "{") + require.Positive(t, open, "CostExplorer statement has no opening brace") + return src[:open] + stmt + " " + src[open:] +} + +// TestTFStatementsCatchWidenedCreateTags pins #782: a statement that grants +// ec2:CreateTags on every EC2 resource fails the #702 check whether or not it +// has a Sid. Every mutant is plain literal HCL, so the scan must read it (not +// refuse it) and the statements it returns must break the tagging rule. +func TestTFStatementsCatchWidenedCreateTags(t *testing.T) { + for _, path := range memberAccountTerraform { + raw, err := os.ReadFile(filepath.Join("..", "..", path)) + require.NoError(t, err, path) + for name, stmt := range tfStatementMutants { + t.Run(path+"/"+name, func(t *testing.T) { + mutant := withStatementBeforeCostExplorer(t, string(raw), stmt) + statements, err := parseTFStatements([]byte(mutant)) + require.NoError(t, err, "every mutant is literal HCL the scan must read, not refuse") + assert.NotEmpty(t, createTagsViolations(statements), "widened ec2:CreateTags went undetected") + }) + } + // aws_iam_policy_document statement blocks spell the keys actions and + // resources and rarely carry a sid. + t.Run(path+"/Sid-less aws_iam_policy_document statement block", func(t *testing.T) { + mutant := string(raw) + ` +data "aws_iam_policy_document" "widened" { + statement { + effect = "Allow" + actions = ["ec2:CreateTags"] + resources = ["*"] + } +} +` + statements, err := parseTFStatements([]byte(mutant)) + require.NoError(t, err) + assert.NotEmpty(t, createTagsViolations(statements), "widened ec2:CreateTags went undetected") + }) + // The real tagging statement loses its Sid and is widened in place, so + // ec2:CreateTags still appears exactly once: only the Resource check can + // catch it. + t.Run(path+"/Sid-less tagging statement widened in place", func(t *testing.T) { + mutant := regexp.MustCompile(`Sid\s*=\s*"EC2ReservedInstanceTagging"\n`).ReplaceAllString(string(raw), "") + mutant = strings.Replace(mutant, `Resource = "`+reservedInstanceARN+`"`, `Resource = "*"`, 1) + require.NotContains(t, mutant, reservedInstanceARN, "mutant did not widen the tagging statement") + statements, err := parseTFStatements([]byte(mutant)) + require.NoError(t, err) + assert.Contains(t, createTagsViolations(statements), + `ec2:CreateTags must be scoped to `+reservedInstanceARN+`, got Resource "*"`) + }) + } +} + +// TestParseTFStatementsRefusesWhatItCannotRead pins the fail-closed half of +// #782: a statement the scan cannot read is an error, never a silent skip. +func TestParseTFStatementsRefusesWhatItCannotRead(t *testing.T) { + cases := map[string]struct{ stmt, wantErr string }{ + "Action from a reference": {wantErr: "not a literal string", stmt: `{ + Effect = "Allow" + Action = local.tag_actions + Resource = "*" + }, +`}, + "interpolated Action": {wantErr: "not a literal string", stmt: `{ + Effect = "Allow" + Action = ["${var.service}:CreateTags"] + Resource = "*" + }, +`}, + "Resource from a reference": {wantErr: "not a literal string", stmt: `{ + Effect = "Allow" + Action = ["ec2:CreateTags"] + Resource = var.tag_resource + }, +`}, + "duplicate Action key": {wantErr: "repeats its Action or Resource key", stmt: `{ + Effect = "Allow" + Action = ["ec2:CreateTags"] + Action = ["ec2:DescribeTags"] + Resource = "*" + }, +`}, + } + for _, path := range memberAccountTerraform { + raw, err := os.ReadFile(filepath.Join("..", "..", path)) + require.NoError(t, err, path) + for name, tc := range cases { + t.Run(path+"/"+name, func(t *testing.T) { + _, err := parseTFStatements([]byte(withStatementBeforeCostExplorer(t, string(raw), tc.stmt))) + assert.ErrorContains(t, err, tc.wantErr) + }) + } + t.Run(path+"/policy in a heredoc", func(t *testing.T) { + heredoc := string(raw) + ` +resource "aws_iam_policy" "widened" { + policy = <<-EOT + {"Statement": [{"Effect": "Allow", "Action": "ec2:CreateTags", "Resource": "*"}]} + EOT +} +` + _, err := parseTFStatements([]byte(heredoc)) + assert.ErrorContains(t, err, "Action keys") }) } } diff --git a/internal/iacfiles/tfpolicy_scan_test.go b/internal/iacfiles/tfpolicy_scan_test.go new file mode 100644 index 000000000..02aeaa4c1 --- /dev/null +++ b/internal/iacfiles/tfpolicy_scan_test.go @@ -0,0 +1,323 @@ +package iacfiles + +import ( + "fmt" + "regexp" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// This file reads IAM policy statements out of Terraform source without a +// Terraform or HCL dependency (the module has neither). It tokenizes the HCL +// native syntax far enough to know where strings, comments, heredocs and +// brackets begin and end, then treats every object constructor and block body +// that has an Action (jsonencode) or actions (aws_iam_policy_document) attribute +// as one statement. Nothing keys off a Sid: a Sid is optional free text, so a +// scan anchored on it never sees a statement written without one (#782). + +type tfTokenKind int + +const ( + tfPunct tfTokenKind = iota // one of { } [ ] ( ) , = : + tfString // quoted string or heredoc; text is the content between the delimiters + tfWord // identifier, number, traversal or operator + tfNewline // attribute separator in HCL object and block bodies +) + +type tfToken struct { + kind tfTokenKind + text string +} + +type tfAttribute struct { + key string + value []tfToken +} + +const tfPunctChars = "{}[](),=:" + +// tfActionKey counts the Action keys in the raw text, independently of the +// tokenizer, so a statement the tokenizer cannot see (inside a heredoc, say) +// shows up as a count mismatch instead of passing silently. It is +// case-insensitive and also matches the plural `actions` of an +// aws_iam_policy_document statement block; `NotAction` and `not_actions` are +// excluded by the leading non-word character. +var tfActionKey = regexp.MustCompile(`(?i)(?:^|[^A-Za-z0-9_])"?actions?"?\s*[=:]`) + +// tfStatements returns every IAM policy statement in a Terraform file. +func tfStatements(t *testing.T, raw []byte) []awsPolicyStatement { + t.Helper() + statements, err := parseTFStatements(raw) + require.NoError(t, err) + return statements +} + +// parseTFStatements returns every IAM policy statement in a Terraform file. It +// fails rather than guess when a statement's Action or Resource is not a list +// of literal strings, and when the number of statements it found differs from +// the number of Action keys in the text. +func parseTFStatements(raw []byte) ([]awsPolicyStatement, error) { + toks, err := tfTokenize(string(raw)) + if err != nil { + return nil, err + } + var out []awsPolicyStatement + for i, tok := range toks { + if tok.kind != tfPunct || tok.text != "{" { + continue + } + stmt, ok, err := tfStatementFrom(tfAttributes(toks, i+1)) + if err != nil { + return nil, err + } + if ok { + out = append(out, stmt) + } + } + if keys := len(tfActionKey.FindAllString(string(raw), -1)); keys != len(out) { + return nil, fmt.Errorf("found %d Action keys in the text but parsed %d statements; a statement this scan cannot see would pass unchecked", keys, len(out)) + } + return out, nil +} + +// tfStatementFrom builds a statement from an object's attributes. ok is false +// when the object has no Action attribute and so is not a statement. +func tfStatementFrom(attrs []tfAttribute) (stmt awsPolicyStatement, ok bool, err error) { + var actions, resources int + for _, attr := range attrs { + key := strings.ToLower(attr.key) + switch key { + case "action", "actions": + actions++ + stmt.Action, err = tfLiteralStrings(attr) + case "resource", "resources": + resources++ + var list []string + list, err = tfLiteralStrings(attr) + // A multi-resource list is joined so it never equals a single ARN. + stmt.Resource = strings.Join(list, ",") + } + if err != nil { + return stmt, false, err + } + } + if actions > 1 || resources > 1 { + return stmt, false, fmt.Errorf("statement %v repeats its Action or Resource key", stmt.Action) + } + return stmt, actions == 1, nil +} + +// tfLiteralStrings returns the strings of a value that is one literal string or +// a list of them. Anything else (a reference, a function call, an +// interpolation) is an error: the test cannot know what it resolves to. +func tfLiteralStrings(attr tfAttribute) ([]string, error) { + var out []string + for _, tok := range attr.value { + switch { + case tok.kind == tfString && !strings.Contains(tok.text, "${") && !strings.Contains(tok.text, "%{"): + out = append(out, tok.text) + case tok.kind == tfNewline, tok.kind == tfPunct && strings.Contains("[],", tok.text): + // List brackets and separators carry no value. + default: + return nil, fmt.Errorf("%s holds %q, which is not a literal string; write it as literal strings so this test can check it", attr.key, tok.text) + } + } + return out, nil +} + +// tfAttributes returns the `key = value` and `key: value` attributes of the +// object or block body that starts at toks[i], up to its closing brace. Nested +// blocks and anything else that is not an attribute are skipped. +func tfAttributes(toks []tfToken, i int) []tfAttribute { + var attrs []tfAttribute + for i < len(toks) { + tok := toks[i] + switch { + case tfIsClose(tok): + return attrs + case tok.kind == tfNewline, tok.kind == tfPunct && tok.text == ",": + i++ + case (tok.kind == tfWord || tok.kind == tfString) && i+1 < len(toks) && + toks[i+1].kind == tfPunct && (toks[i+1].text == "=" || toks[i+1].text == ":"): + end := tfValueEnd(toks, i+2) + attrs = append(attrs, tfAttribute{key: tok.text, value: toks[i+2 : end]}) + i = end + default: + i = tfValueEnd(toks, i+1) + } + } + return attrs +} + +// tfValueEnd returns the index of the newline, comma or closing bracket that +// ends the value starting at toks[i], ignoring those inside nested brackets. +func tfValueEnd(toks []tfToken, i int) int { + depth := 0 + for ; i < len(toks); i++ { + tok := toks[i] + switch { + case tfIsOpen(tok): + depth++ + case tfIsClose(tok): + if depth == 0 { + return i + } + depth-- + case depth == 0 && (tok.kind == tfNewline || tok.kind == tfPunct && tok.text == ","): + return i + } + } + return i +} + +func tfIsOpen(tok tfToken) bool { + return tok.kind == tfPunct && (tok.text == "{" || tok.text == "[" || tok.text == "(") +} + +func tfIsClose(tok tfToken) bool { + return tok.kind == tfPunct && (tok.text == "}" || tok.text == "]" || tok.text == ")") +} + +// tfTokenize splits Terraform source into tokens, dropping comments and +// whitespace other than newlines. +func tfTokenize(src string) ([]tfToken, error) { + var toks []tfToken + for i := 0; i < len(src); { + tok, next, err := tfNextToken(src, i) + if err != nil { + return nil, err + } + if tok != nil { + toks = append(toks, *tok) + } + i = next + } + return toks, nil +} + +// tfNextToken reads the token at src[i]. It returns a nil token for +// whitespace and comments. +func tfNextToken(src string, i int) (tok *tfToken, next int, err error) { + c := src[i] + switch { + case c == ' ' || c == '\t' || c == '\r': + return nil, i + 1, nil + case c == '\n': + return &tfToken{kind: tfNewline, text: "\n"}, i + 1, nil + case c == '#' || strings.HasPrefix(src[i:], "//") || strings.HasPrefix(src[i:], "/*"): + next, err := tfSkipComment(src, i) + return nil, next, err + case c == '"': + end, err := tfSkipQuoted(src, i) + if err != nil { + return nil, 0, err + } + return &tfToken{kind: tfString, text: src[i+1 : end-1]}, end, nil + case strings.HasPrefix(src[i:], "<<"): + return tfHeredoc(src, i) + case strings.IndexByte(tfPunctChars, c) >= 0: + return &tfToken{kind: tfPunct, text: string(c)}, i + 1, nil + } + end := tfWordEnd(src, i) + return &tfToken{kind: tfWord, text: src[i:end]}, end, nil +} + +// tfWordEnd returns the index just past the word that starts at src[i]. +func tfWordEnd(src string, i int) int { + end := i + 1 + for end < len(src) && !strings.ContainsRune(" \t\r\n\"#"+tfPunctChars, rune(src[end])) && + !strings.HasPrefix(src[end:], "//") && !strings.HasPrefix(src[end:], "/*") { + end++ + } + return end +} + +// tfSkipComment returns the index just past the comment that starts at +// src[i]; a line comment ends before its newline, which is still a token. +func tfSkipComment(src string, i int) (int, error) { + if !strings.HasPrefix(src[i:], "/*") { + return tfLineEnd(src, i), nil + } + end := strings.Index(src[i+2:], "*/") + if end < 0 { + return 0, fmt.Errorf("unterminated comment at offset %d", i) + } + return i + 2 + end + 2, nil +} + +func tfLineEnd(src string, i int) int { + if n := strings.IndexByte(src[i:], '\n'); n >= 0 { + return i + n + } + return len(src) +} + +// tfSkipQuoted returns the index just past the closing quote of the quoted +// string that opens at src[i], stepping over escapes and over `${...}` and +// `%{...}` template sequences, which may themselves contain quoted strings. +func tfSkipQuoted(src string, i int) (int, error) { + for j := i + 1; j < len(src); j++ { + switch { + case src[j] == '\\': + j++ + case src[j] == '"': + return j + 1, nil + case src[j] == '\n': + return 0, fmt.Errorf("unterminated string at offset %d", i) + case strings.HasPrefix(src[j:], "$${"), strings.HasPrefix(src[j:], "%%{"): + j += 2 // escaped template sequence: literal text + case strings.HasPrefix(src[j:], "${"), strings.HasPrefix(src[j:], "%{"): + end, err := tfSkipTemplate(src, j+2) + if err != nil { + return 0, err + } + j = end - 1 + } + } + return 0, fmt.Errorf("unterminated string at offset %d", i) +} + +// tfSkipTemplate returns the index just past the brace that closes a template +// sequence whose body starts at src[i]. +func tfSkipTemplate(src string, i int) (int, error) { + depth := 1 + for j := i; j < len(src); j++ { + switch src[j] { + case '"': + end, err := tfSkipQuoted(src, j) + if err != nil { + return 0, err + } + j = end - 1 + case '{': + depth++ + case '}': + depth-- + if depth == 0 { + return j + 1, nil + } + } + } + return 0, fmt.Errorf("unterminated template sequence at offset %d", i) +} + +// tfHeredoc reads a `<