Repository: tfsec/tfsec
Pull request #587 (2021-02-10 05:37:52)

Add support for "and" and "or" predicates
## Context
Support for `AND` predicates was added as part of https://github.com/tfsec/tfsec/pull/580. 

## Intent
To re-work the current implementation for composite checks.

## Problem
The major downsides we found with the current implementation are:
- Some custom checks may benefit from an `or` operator being available.
- The implicit `and` behavior of `subMatches` is not apparent from the check definition files. In consequence, non-experienced users will struggle to understand/write checks without having to refer to the official documentation or rely on their teams' tribal knowledge.
- The `subMatch` / `subMatches` operator is a bit overloaded with too many responsibilities:
    -  Provide the ability to “look into” a nested block. 
    -  Provide support for `and`-based composite boolean expressions.

## Proposed Solution
To adopt a new implementation for composite checks that supports both `and` and `or` operators, it is explicitly visible from the check definition files, and do not overload the `subMatch` operator keeping the `chekFunctions` as atomic as possible.

As you can see from the check_all_statements_test.go file, it will still be possible to provide the same functionality that `subMatches` had provided, while allowing for general checkFunctions to be combined. 

(cc: @muplim)

owenrumney: This makes sense, thank @alloveras - Could you update the documentation to reflect this change as well please - https://github.com/tfsec/tfsec/blob/master/docs-website/docs/config/custom_checks.md

@felipesere  - this will be relevant to you as it would be a breaking change to your new addition. I think given its early stages I trust that won't impact you too heavily
felipesere: This looks really good! I'd be happy for this!

alloveras: > All good - just the doc to update - https://github.com/tfsec/tfsec/blob/master/docs-website/docs/config/custom_checks.md

Sure, no problem. I will get to that as soon as I have some spare minutes and send it back for a final review.
alloveras: Hey @owenrumney,
I added the changes to the documentation you requested.

Do you mind having another look?

Thanks!

Patch:
diff --git a/docs-website/docs/config/custom_checks.md b/docs-website/docs/config/custom_checks.md
index 19fda5296..ba163fd12 100644
--- a/docs-website/docs/config/custom_checks.md
+++ b/docs-website/docs/config/custom_checks.md
@@ -98,7 +98,7 @@ The `MatchSpec` is the what will define the check itself - this is fairly basic
 | value | In cases where a value is required, the value to look for |
 | ignoreUndefined | If the attribute is undefined, ignore and pass the check |
 |subMatch | A sub MatchSpec block for nested checking - think looking for `enabled` value in a `logging` block |
-|subMatches | An array of sub MatchSpec block for nested checking. All sub MatchSpecs must match. |
+|predicateMatchSpec | An array of MatchSpec blocks to be logically aggregated by either `and` or `or` actions |
 
 #### Check Actions
 There are a number of `CheckActions` available which should allow you to quickly put together most checks.
@@ -377,8 +377,8 @@ matchSpec:
 ##### isAny
 The `isAny` check action passes when the attribute value can be found in the slice passed as the check value. This check action supports strings and numbers
 
-```
-"matchSpec" : {
+```json
+"matchSpec": {
   "name": "acl",
   "action": "isAny",
   "value": ["private", "log-delivery-write"]
@@ -397,8 +397,8 @@ matchSpec:
 ##### isNone
 The `isNone` check action passes when the attribute value cannot be found in the slice passed as the check value. This check action supports strings and numbers
 
-```
-"matchSpec" : {
+```json
+"matchSpec": {
   "name": "acl",
   "action": "isNone",
   "value": ["authenticated-read", "public-read"]
@@ -419,7 +419,7 @@ The `requiresPresence` checks that the resouce in `name` is also present in the
 
 If you wanted to ensure that `aws_vpc_flowlogs` is present if there is a `aws_vpc`, you might use the following `matchSpec`:
 
-```
+```json
 "matchSpec" : {
   "action": "requiresPresence",
   "name": "aws_vpc_flowlogs"
@@ -432,6 +432,115 @@ matchSpec:
   action: requiresPresence
 ```
 
+##### and
+The `and` check action passes when all the blocks provided within `predicateMatchSpec` evaluate to `true`. This action
+can be combined with `subMatch` to perform composite checks against the contents of nested blocks.
+
+Note that `or` and `and` actions can be nested as many times as needed.
+
+If you wanted to ensure that `device_name` and `encrypted` are both present in a nested `ebs_block_device` block,
+you might use the following `matchSpec`:
+
+```json
+"matchSpec": {
+  "action": "isPresent",
+  "name": "ebs_block_device",
+  "subMatch": {
+    "action": "and",
+    "predicateMatchSpec": [
+      {
+        "action": "isPresent",
+        "name": "device_name"      
+      },
+      {
+        "action": "isPresent",
+        "name": "encrypted"
+      }
+    ]
+  }
+}
+```
+
+```yaml
+matchSpec:
+  action: isPresent
+  name: ebs_block_device
+  subMatch : 
+    action : and
+    predicateMatchSpec :
+      - action : isPresent
+        name : device_name
+      - action : isPresent
+        name : encrypted 
+```
+
+##### or
+The `or` check action passes when at least one of the blocks provided within `predicateMatchSpec` evaluates to `true`.
+This action can be combined with `subMatch` to perform composite checks against the contents of nested blocks.
+
+Note that `or` and `and` actions can be nested as many times as needed.
+
+If you want to ensure that `virtualization_type` is assigned to either `hvm` or `paravirtual`, while enforcing
+that their implicitly linked required attributes are also present, you might use the following `matchSpec`:
+
+```
+"matchSpec": {
+  "action": "or",
+  "predicateMatchSpec": [
+    {
+      "action": "and",
+      "predicateMatchSpec": [
+        {
+          "name": "virtualization_type",
+          "action": "equals",
+          "value": "hvm"
+        }
+      ]
+    },
+    {
+      "action": "and",
+      "predicateMatchSpec": [
+        {
+          "name": "virtualization_type",
+          "action": "equals",
+          "value": "paravirtual"
+        },
+        {
+          "name": "image_location",
+          "action": "isPresent"
+        },
+        {
+          "name": "kernel_id",
+          "action": "isPresent"
+        }
+      ]
+    }
+  ]
+}
+```
+
+```yaml
+matchSpec:
+  action: or
+  predicateMatchSpec:
+    - action: and
+      predicateMatchSpec:
+        - name: virtualization_type
+          action: equals
+          value: hvm
+        - name: sriov_net_support
+          action: isPresent
+    - action: and
+      predicateMatchSpec:
+        - name: virtualization_type
+          action: equals
+          value: paravirtual
+        - name: image_location
+          action: isPresent
+        - name: kernel_id
+          action: isPresent
+```
+
 ## How do I know my JSON is valid?
 We have provided the `tfsec-checkgen` binary which will validate your check file to ensure that it is valid for use with `tfsec`. 
 
diff --git a/internal/app/tfsec/custom/custom_check.go b/internal/app/tfsec/custom/custom_check.go
index 12c5681ce..4cffff71f 100644
--- a/internal/app/tfsec/custom/custom_check.go
+++ b/internal/app/tfsec/custom/custom_check.go
@@ -25,6 +25,8 @@ var ValidCheckActions = []CheckAction{
 	RequiresPresence,
 	IsAny,
 	IsNone,
+	And,
+	Or,
 }
 
 // InModule checks that the block is part of a module
@@ -78,14 +80,20 @@ const GreaterThanOrEqualTo CheckAction = "greaterThanOrEqualTo"
 // RequiresPresence checks that a second resource is present
 const RequiresPresence CheckAction = "requiresPresence"
 
+// And checks that at both of the given predicateMatchSpec's evaluates to True
+const And CheckAction = "and"
+
+// Or checks that at least one of the given predicateMatchSpec's evaluates to True
+const Or CheckAction = "or"
+
 // MatchSpec specifies the checks that should be performed
 type MatchSpec struct {
-	Name            string      `json:"name,omitempty" yaml:"name,omitempty"`
-	MatchValue      interface{} `json:"value,omitempty" yaml:"value,omitempty"`
-	Action          CheckAction `json:"action,omitempty" yaml:"action,omitempty"`
-	SubMatch        *MatchSpec  `json:"subMatch,omitempty" yaml:"subMatch,omitempty"`
-	SubMatches      []MatchSpec `json:"subMatches,omitempty" yaml:"subMatches,omitempty"`
-	IgnoreUndefined bool        `json:"ignoreUndefined,omitempty" yaml:"ignoreUndefined,omitempty"`
+	Name               string      `json:"name,omitempty" yaml:"name,omitempty"`
+	MatchValue         interface{} `json:"value,omitempty" yaml:"value,omitempty"`
+	Action             CheckAction `json:"action,omitempty" yaml:"action,omitempty"`
+	PredicateMatchSpec []MatchSpec `json:"predicateMatchSpec,omitempty" yaml:"predicateMatchSpec,omitempty"`
+	SubMatch           *MatchSpec  `json:"subMatch,omitempty" yaml:"subMatch,omitempty"`
+	IgnoreUndefined    bool        `json:"ignoreUndefined,omitempty" yaml:"ignoreUndefined,omitempty"`
 }
 
 //Check specifies the check definition represented in json/yaml
diff --git a/internal/app/tfsec/custom/processing.go b/internal/app/tfsec/custom/processing.go
index bc25f95d9..0c65399fa 100644
--- a/internal/app/tfsec/custom/processing.go
+++ b/internal/app/tfsec/custom/processing.go
@@ -151,6 +151,26 @@ func evalMatchSpec(block *parser.Block, spec *MatchSpec, ctx *scanner.Context) b
 		return resourceFound(spec, ctx)
 	}
 
+	// This And MatchSpec is only true if all childSpecs return true
+	if spec.Action == And {
+		for _, childSpec := range spec.PredicateMatchSpec {
+			if !evalMatchSpec(block, &childSpec, ctx) {
+				return false
+			}
+		}
+		return true
+	}
+
+	// If a single childSpec is true then this Or matchSpec is true
+	if spec.Action == Or {
+		for _, childSpec := range spec.PredicateMatchSpec {
+			if evalMatchSpec(block, &childSpec, ctx) {
+				return true
+			}
+		}
+		return false
+	}
+
 	evalResult = matchFunctions[spec.Action](block, spec)
 
 	if spec.SubMatch != nil {
@@ -162,18 +182,6 @@ func evalMatchSpec(block *parser.Block, spec *MatchSpec, ctx *scanner.Context) b
 		}
 	}
 
-	if len(spec.SubMatches) > 0 {
-
-	AllBlocks:
-		for _, block := range block.GetBlocks(spec.Name) {
-			for i := range spec.SubMatches {
-				evalResult = evalMatchSpec(block, &spec.SubMatches[i], nil)
-				if !evalResult {
-					break AllBlocks
-				}
-			}
-		}
-	}
 	return evalResult
 }
 
diff --git a/internal/app/tfsec/custom/validate.go b/internal/app/tfsec/custom/validate.go
index 7ace10c50..05f148992 100644
--- a/internal/app/tfsec/custom/validate.go
+++ b/internal/app/tfsec/custom/validate.go
@@ -74,10 +74,18 @@ func validateMatchSpec(spec *MatchSpec, check *Check, checkErrors []error) []err
 	if !spec.Action.isValid() {
 		checkErrors = append(checkErrors, errors.New(fmt.Sprintf("matchSpec.Action[%s] is not a recognised option. Should be %s", spec.Action, ValidCheckActions)))
 	}
-	// if the check is inModule, no name is required
-	if len(spec.Name) == 0 && spec.Action != "inModule" {
+	// if the check is one of `inModule`,`or`,`and`, no name is required
+	if len(spec.Name) == 0 && spec.Action != "inModule" && spec.Action != "or" && spec.Action != "and" {
 		checkErrors = append(checkErrors, errors.New("matchSpec.Name requires a value"))
 	}
+
+	// if the check is one of `or`, `and`, then all PredicateMatchSpec's must also be valid
+	if spec.Action == "or" || spec.Action == "and" {
+		for _, predicateMatchSpec := range spec.PredicateMatchSpec {
+			checkErrors = append(validateMatchSpec(&predicateMatchSpec, check, checkErrors))
+		}
+	}
+
 	if spec.SubMatch != nil {
 		return validateMatchSpec(spec.SubMatch, check, checkErrors)
 	}


Test patch:
diff --git a/internal/app/tfsec/custom/check_all_statements_test.go b/internal/app/tfsec/custom/check_all_statements_test.go
index c66d01158..a0b632a3a 100644
--- a/internal/app/tfsec/custom/check_all_statements_test.go
+++ b/internal/app/tfsec/custom/check_all_statements_test.go
@@ -42,20 +42,23 @@ func init() {
       "matchSpec": {
         "name": "statement",
         "action": "isPresent",
-        "subMatches": [
-          {
-            "name": "actions",
-            "action": "notContains",
-            "value": "s3:Foo",
-            "ignoreUndefined": true
-          },
-          {
-            "name": "actions",
-            "action": "notContains",
-            "value": "s3:Bar",
-            "ignoreUndefined": true
-          }
-        ]
+        "subMatch": {
+          "action": "and",
+          "predicateMatchSpec": [
+            {
+              "name": "actions",
+              "action": "notContains",
+              "value": "s3:Foo",
+              "ignoreUndefined": true
+            },
+            {
+              "name": "actions",
+              "action": "notContains",
+              "value": "s3:Bar",
+              "ignoreUndefined": true
+            }
+          ]
+        }
       }
     }
   ]
diff --git a/internal/app/tfsec/custom/multiple_submatches_test.go b/internal/app/tfsec/custom/multiple_submatches_test.go
index 53bcf8a10..20f4a68c7 100644
--- a/internal/app/tfsec/custom/multiple_submatches_test.go
+++ b/internal/app/tfsec/custom/multiple_submatches_test.go
@@ -21,28 +21,31 @@ func init() {
       "matchSpec": {
         "name": "metadata_options",
         "action": "isPresent",
-        "subMatches": [
-          {
-            "name": "http_endpoint",
-            "action": "equals",
-            "value": "enabled"
-          },
-          {
-            "name": "http_put_response_hop_limit",
-            "action": "equals",
-            "value": 1
-          },
-          {
-            "name": "http_tokens",
-            "action": "equals",
-            "value": "required"
-          }
-        ]
+        "subMatch": {
+          "action": "and",
+          "predicateMatchSpec": [
+            {
+              "name": "http_endpoint",
+              "action": "equals",
+              "value": "enabled"
+            },
+            {
+              "name": "http_put_response_hop_limit",
+              "action": "equals",
+              "value": 1
+            },
+            {
+              "name": "http_tokens",
+              "action": "equals",
+              "value": "required"
+            }
+          ]
+        }
       }
     }
   ]
 }
-    `)
+`)
 }
 
 func TestInstanceMetadataEndpointPresent(t *testing.T) {
diff --git a/internal/app/tfsec/custom/processing_test.go b/internal/app/tfsec/custom/processing_test.go
index 96d2b292e..d7bb6dbcd 100644
--- a/internal/app/tfsec/custom/processing_test.go
+++ b/internal/app/tfsec/custom/processing_test.go
@@ -8,6 +8,7 @@ import (
 	"github.com/tfsec/tfsec/internal/app/tfsec/scanner"
 	"io/ioutil"
 	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 )
@@ -37,6 +38,58 @@ func init() {
 `)
 }
 
+var testOrMatchSpec = MatchSpec{
+	Action: "or",
+	PredicateMatchSpec: []MatchSpec{
+		{
+			Name:   "name",
+			Action: "isPresent",
+		},
+		{
+			Name:   "description",
+			Action: "isPresent",
+		},
+	},
+}
+
+var testAndMatchSpec = MatchSpec{
+	Action: "and",
+	PredicateMatchSpec: []MatchSpec{
+		{
+			Name:   "name",
+			Action: "isPresent",
+		},
+		{
+			Name:   "description",
+			Action: "isPresent",
+		},
+	},
+}
+
+var testNestedMatchSpec = MatchSpec{
+	Action: "and",
+	PredicateMatchSpec: []MatchSpec{
+		{
+			Name:       "virtualization_type",
+			Action:     "equals",
+			MatchValue: "paravirtual",
+		},
+		{
+			Action: "or",
+			PredicateMatchSpec: []MatchSpec{
+				{
+					Name:   "image_location",
+					Action: "isPresent",
+				},
+				{
+					Name:   "kernel_id",
+					Action: "isPresent",
+				},
+			},
+		},
+	},
+}
+
 func TestRequiresPresenceWithResourcePresent(t *testing.T) {
 	scanResults := scanTerraform(t, `
 resource "aws_vpc" "main" {
@@ -71,6 +124,150 @@ resource "aws_vpc" "main" {
 	assert.Len(t, scanResults, 1)
 }
 
+func TestOrMatchFunction(t *testing.T) {
+
+	var tests = []struct {
+		name               string
+		source             string
+		predicateMatchSpec MatchSpec
+		expected           bool
+	}{
+		{
+			name: "check `or` match function with no true evaluation",
+			source: `
+resource "aws_ami" "example" {
+}
+`,
+			predicateMatchSpec: testOrMatchSpec,
+			expected:           false,
+		},
+		{
+			name: "check `or` match function with a single true evaluation",
+			source: `
+resource "aws_ami" "example" {
+	name = "placeholder-name"
+}
+`,
+			predicateMatchSpec: testOrMatchSpec,
+			expected:           true,
+		},
+		{
+			name: "check `or` match function with all true evaluation",
+			source: `
+resource "aws_ami" "example" {
+	name = "placeholder-name"
+	description = "this is a description."
+}
+`,
+			predicateMatchSpec: testOrMatchSpec,
+			expected:           true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			blocks := createBlocksFromSource(test.source)[0]
+			result := evalMatchSpec(blocks, &test.predicateMatchSpec, nil)
+			assert.Equal(t, result, test.expected, "`Or` match function evaluating incorrectly.")
+		})
+	}
+}
+
+func TestAndMatchFunction(t *testing.T) {
+	var tests = []struct {
+		name               string
+		source             string
+		predicateMatchSpec MatchSpec
+		expected           bool
+	}{
+		{
+			name: "check `and` match function with no true evaluation",
+			source: `
+resource "aws_ami" "example" {
+}
+`,
+			predicateMatchSpec: testAndMatchSpec,
+			expected:           false,
+		},
+		{
+			name: "check `and` match function with a single true evaluation",
+			source: `
+resource "aws_ami" "example" {
+	name = "placeholder-name"
+}
+`,
+			predicateMatchSpec: testAndMatchSpec,
+			expected:           false,
+		},
+		{
+			name: "check `and` match function with all true evaluation",
+			source: `
+resource "aws_ami" "example" {
+	name = "placeholder-name"
+	description = "this is a description."
+}
+`,
+			predicateMatchSpec: testAndMatchSpec,
+			expected:           true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			blocks := createBlocksFromSource(test.source)[0]
+			result := evalMatchSpec(blocks, &test.predicateMatchSpec, nil)
+			assert.Equal(t, result, test.expected, "`And` match function evaluating incorrectly.")
+		})
+	}
+}
+func TestNestedMatchFunction(t *testing.T) {
+	var tests = []struct {
+		name               string
+		source             string
+		predicateMatchSpec MatchSpec
+		expected           bool
+	}{
+		{
+			name: "check nested match function with only inner true evaluation",
+			source: `
+resource "aws_ami" "example" {
+	virtualization_type = "hvm"
+	image_location = "image-XXXX"
+	kernel_id = "XXXXXXXXXX"
+}
+`,
+			predicateMatchSpec: testNestedMatchSpec,
+			expected:           false,
+		},
+		{
+			name: "check nested match function with no true evaluation",
+			source: `
+resource "aws_ami" "example" {
+	virtualization_type = "hvm"
+}
+`,
+			predicateMatchSpec: testNestedMatchSpec,
+			expected:           false,
+		},
+		{
+			name: "check nested match function with all true evaluation",
+			source: `
+resource "aws_ami" "example" {
+	virtualization_type = "paravirtual"
+	image_location = "image-XXXX"
+	kernel_id = "XXXXXXXXXX"
+}
+`,
+			predicateMatchSpec: testNestedMatchSpec,
+			expected:           true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			blocks := createBlocksFromSource(test.source)[0]
+			result := evalMatchSpec(blocks, &test.predicateMatchSpec, nil)
+			assert.Equal(t, result, test.expected, "Nested match functions evaluating incorrectly.")
+		})
+	}
+}
 func givenCheck(jsonContent string) {
 	var checksfile ChecksFile
 	err := json.NewDecoder(strings.NewReader(jsonContent)).Decode(&checksfile)
@@ -92,3 +289,28 @@ func scanTerraform(t *testing.T, mainTf string) []scanner.Result {
 
 	return scanner.New().Scan(blocks, []string{})
 }
+
+// This function is copied from setup_test.go as it is not possible to import function from test files.
+// TODO: Extract into a testing utility package once the amount of duplication justifies introducing an extra package.
+func createBlocksFromSource(source string) []*parser.Block {
+	path := createTestFile("test.tf", source)
+	blocks, err := parser.New(filepath.Dir(path), "").ParseDirectory()
+	if err != nil {
+		panic(err)
+	}
+	return blocks
+}
+
+// This function is copied from setup_test.go as it is not possible to import function from test files.
+// TODO: Extract into a testing utility package once the amount of duplication justifies introducing an extra package.
+func createTestFile(filename, contents string) string {
+	dir, err := ioutil.TempDir(os.TempDir(), "tfsec")
+	if err != nil {
+		panic(err)
+	}
+	path := filepath.Join(dir, filename)
+	if err := ioutil.WriteFile(path, []byte(contents), 0755); err != nil {
+		panic(err)
+	}
+	return path
+}