Repository: tfsec/tfsec
Pull request #474 (2020-10-26 19:58:36)

Adding isAny and isNone custom checks
 - update the documentation to reflect changes
 - changed `MatchSpec` value to interface{} for increased flexibility
 - added more checks for custom checks to validate


Patch:
diff --git a/docs-website/_docs/custom_checks.md b/docs-website/_docs/custom_checks.md
index 53b4640ab..54474bc94 100644
--- a/docs-website/_docs/custom_checks.md
+++ b/docs-website/_docs/custom_checks.md
@@ -117,7 +117,7 @@ The `startsWith` check action passes if the checked attribute string starts with
 ```
 
 ##### endsWith
-The `endsWith` check action passes if the checked attribute string ends with the specified value. For example, to check that `acl` begins with `public` you could use the following `MatchSpec`;
+The `endsWith` check action passes if the checked attribute string ends with the specified value. For example, to check that `acl` ends with `read` you could use the following `MatchSpec`;
 
 ```json
 "matchSpec" : {
@@ -176,6 +176,27 @@ When tackling this specific use case of filtering module blocks by source, the `
       }
 ```
 
+##### 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" : {
+  "name": "acl",
+  "action": "isAny",
+  "value": ["private", "log-delivery-write"]
+}
+```
+
+##### 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" : {
+  "name": "acl",
+  "action": "isNone",
+  "value": ["authenticated-read", "public-read"]
+}
+```
 
 ## 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/example/custom/.tfsec/custom_tfchecks.json b/example/custom/.tfsec/custom_tfchecks.json
index aedecd3c0..6194fc7ac 100644
--- a/example/custom/.tfsec/custom_tfchecks.json
+++ b/example/custom/.tfsec/custom_tfchecks.json
@@ -46,6 +46,26 @@
     },
     {
       "code": "CUS003",
+      "description": "Check ACL is not one of bad values",
+      "requiredTypes": [
+        "resource"
+      ],
+      "requiredLabels": [
+        "aws_s3_bucket"
+      ],
+      "severity": "ERROR",
+      "matchSpec": {
+        "name": "acl",
+        "value": ["public-read", "authenticated-users"],
+        "action": "isNone"
+      },
+      "errorMessage": "The ACL must not be one of ['public-read', 'authenticated-users']",
+      "relatedLinks": [
+        "http://internal.acmecorp.com/standards/aws/tagging.html"
+      ]
+    },
+    {
+      "code": "CUS004",
       "description": "Custom check to ensure S3 buckets are only created using the custom_bucket module",
       "requiredTypes": [
         "resource"
@@ -63,7 +83,7 @@
       ]
     },
     {
-      "code": "CUS004",
+      "code": "CUS005",
       "description": "Check that bucket created by custom bucket module must have the acl set to public-read",
       "requiredTypes": [
         "module"
@@ -86,6 +106,26 @@
       "relatedLinks": [
         "http://internal.acmecorp.com/standards/aws/s3_buckets.html"
       ]
+    },
+    {
+      "code": "CUS006",
+      "description": "AMIs must be one of the expected range",
+      "requiredTypes": [
+        "resource"
+      ],
+      "requiredLabels": [
+        "aws_instance"
+      ],
+      "severity": "ERROR",
+      "matchSpec": {
+        "name": "ami",
+        "value": ["ami-1234", "ami-5678"],
+        "action": "isAny"
+      },
+      "errorMessage": "The ec2 instance has an ami that isn't in the permitted range ['ami-1234', 'ami-5678']",
+      "relatedLinks": [
+        "http://internal.acmecorp.com/standards/aws/tagging.html"
+      ]
     }
   ]
 }
\ No newline at end of file
diff --git a/example/custom/custom_check.tf b/example/custom/custom_check.tf
index e4c59048a..32494822b 100644
--- a/example/custom/custom_check.tf
+++ b/example/custom/custom_check.tf
@@ -9,7 +9,7 @@ resource "aws_instance" "non_compliant" {
 }
 
 resource "aws_instance" "compliant" {
-  ami           = "ami-1234"
+  ami           = "ami-12345"
   instance_type = "t2.small"
 
   tags = {
@@ -45,4 +45,13 @@ module "custom_bucket" {
   source      = "./modules/public_custom_bucket"
   bucket_name = "new-public-bucket"
   acl         = "private"
+}
+
+resource "aws_s3_bucket" "bucket_with_public_acl" {
+  bucket = "my-tf-test-bucket"
+  acl    = "public-read"
+
+  versioning {
+    enabled = true
+  }
 }
\ No newline at end of file
diff --git a/internal/app/tfsec/custom/custom_check.go b/internal/app/tfsec/custom/custom_check.go
index 64d426b91..b654b6de5 100644
--- a/internal/app/tfsec/custom/custom_check.go
+++ b/internal/app/tfsec/custom/custom_check.go
@@ -8,7 +8,7 @@ type MatchType string
 type CheckAction string
 
 var ValidCheckActions = []CheckAction{
-	InModule, IsPresent, NotPresent, StartsWith, EndsWith, Contains, Equals, RegexMatches,
+	InModule, IsPresent, NotPresent, StartsWith, EndsWith, Contains, Equals, RegexMatches, IsAny, IsNone,
 }
 
 // InModule checks that the block is part of a module
@@ -32,12 +32,19 @@ const Contains CheckAction = "contains"
 // Contains checks that the named child attribute has a value equal to the check value
 const Equals CheckAction = "equals"
 
+// RegexMatches checks that the named attribute has a value that matches the regex
 const RegexMatches CheckAction = "regexMatches"
 
+// IsAny checks that the named attribute value can be found in the provided slice
+const IsAny CheckAction = "isAny"
+
+// IsNone checks that the named attribute value cannot be found in the provided slice
+const IsNone CheckAction = "isNone"
+
 // MatchSpec specifies the checks that should be performed
 type MatchSpec struct {
 	Name       string      `json:"name,omitempty"`
-	MatchValue string      `json:"value,omitempty"`
+	MatchValue interface{} `json:"value,omitempty"`
 	Action     CheckAction `json:"action,omitempty"`
 	SubMatch   *MatchSpec  `json:"subMatch,omitempty"`
 }
diff --git a/internal/app/tfsec/custom/processing.go b/internal/app/tfsec/custom/processing.go
index d15f59d23..17d0035fb 100644
--- a/internal/app/tfsec/custom/processing.go
+++ b/internal/app/tfsec/custom/processing.go
@@ -30,6 +30,14 @@ var matchFunctions = map[CheckAction]func(*parser.Block, *MatchSpec) bool{
 		attribute := block.GetAttribute(spec.Name)
 		return attribute != nil && attribute.RegexMatches(spec.MatchValue)
 	},
+	IsAny: func(block *parser.Block, spec *MatchSpec) bool {
+		attribute := block.GetAttribute(spec.Name)
+		return attribute != nil && attribute.IsAny(unpackInterfaceToInterfaceSlice(spec.MatchValue)...)
+	},
+	IsNone: func(block *parser.Block, spec *MatchSpec) bool {
+		attribute := block.GetAttribute(spec.Name)
+		return attribute != nil && attribute.IsNone(unpackInterfaceToInterfaceSlice(spec.MatchValue)...)
+	},
 }
 
 func processFoundChecks(checks ChecksFile) {
@@ -84,3 +92,15 @@ func evalMatchSpec(block *parser.Block, spec *MatchSpec) bool {
 	}
 	return evalResult
 }
+
+func unpackInterfaceToInterfaceSlice(t interface{}) []interface{} {
+	switch t := t.(type) {
+	case []interface{}:
+		var result []interface{}
+		for _, i := range t {
+			result = append(result, i)
+		}
+		return result
+	}
+	return nil
+}
diff --git a/internal/app/tfsec/parser/attribute.go b/internal/app/tfsec/parser/attribute.go
index 11e7ded89..fb779295e 100644
--- a/internal/app/tfsec/parser/attribute.go
+++ b/internal/app/tfsec/parser/attribute.go
@@ -1,13 +1,13 @@
 package parser
 
 import (
+	"fmt"
 	"github.com/hashicorp/hcl/v2"
 	"github.com/hashicorp/hcl/v2/hclsyntax"
 	"github.com/tfsec/tfsec/internal/app/tfsec/debug"
 	"github.com/zclconf/go-cty/cty"
 	"github.com/zclconf/go-cty/cty/gocty"
 	"regexp"
-	"strconv"
 	"strings"
 )
 
@@ -54,7 +54,7 @@ func (attr *Attribute) Name() string {
 	return attr.hclAttribute.Name
 }
 
-func (attr *Attribute) Contains(checkValue string) bool {
+func (attr *Attribute) Contains(checkValue interface{}) bool {
 	val := attr.Value()
 	if val.IsNull() {
 		return false
@@ -77,39 +77,34 @@ func (attr *Attribute) Contains(checkValue string) bool {
 		}
 		return false
 	}
-	return strings.Contains(val.AsString(), checkValue)
+	return strings.Contains(val.AsString(), fmt.Sprintf("%v", checkValue))
 }
 
-func (attr *Attribute) StartsWith(prefix string) bool {
+func (attr *Attribute) StartsWith(prefix interface{}) bool {
 	if attr.Value().Type() == cty.String {
-		return strings.HasPrefix(attr.Value().AsString(), prefix)
+		return strings.HasPrefix(attr.Value().AsString(), fmt.Sprintf("%v", prefix))
 	}
 	return false
 }
 
-func (attr *Attribute) EndsWith(prefix string) bool {
+func (attr *Attribute) EndsWith(suffix interface{}) bool {
 	if attr.Value().Type() == cty.String {
-		return strings.HasSuffix(attr.Value().AsString(), prefix)
+		return strings.HasSuffix(attr.Value().AsString(), fmt.Sprintf("%v", suffix))
 	}
 	return false
 }
 
-func (attr *Attribute) Equals(checkValue string) bool {
+func (attr *Attribute) Equals(checkValue interface{}) bool {
 	if attr.Value().Type() == cty.String {
-		return strings.EqualFold(attr.Value().AsString(), checkValue)
+		return strings.EqualFold(attr.Value().AsString(), fmt.Sprintf("%v", checkValue))
 	}
 	if attr.Value().Type() == cty.Bool {
-		checkBool, err := strconv.ParseBool(checkValue)
-		if err != nil {
-			debug.Log("Error converting bool for equality check. %s", err)
-			return false
-		}
-		return attr.Value().True() == checkBool
+		return attr.Value().True() == checkValue
 	}
 	if attr.Value().Type() == cty.Number {
 		checkNumber, err := gocty.ToCtyValue(checkValue, cty.Number)
 		if err != nil {
-			debug.Log("Error converting bool for equality check. %s", err)
+			debug.Log("Error converting number for equality check. %s", err)
 			return false
 		}
 		return attr.Value().RawEquals(checkNumber)
@@ -117,8 +112,9 @@ func (attr *Attribute) Equals(checkValue string) bool {
 	return false
 }
 
-func (attr *Attribute) RegexMatches(pattern string) bool {
-	re, err := regexp.Compile(pattern)
+func (attr *Attribute) RegexMatches(pattern interface{}) bool {
+	patternVal := fmt.Sprintf("%v", pattern)
+	re, err := regexp.Compile(patternVal)
 	if err != nil {
 		debug.Log("an error occurred while compiling the regex: %s", err)
 		return false
@@ -129,3 +125,51 @@ func (attr *Attribute) RegexMatches(pattern string) bool {
 	}
 	return false
 }
+
+func (attr *Attribute) IsAny(options ...interface{}) bool {
+	if attr.Value().Type() == cty.String {
+		for _, option := range options {
+			if option == attr.Value().AsString() {
+				return true
+			}
+		}
+	}
+	if attr.Value().Type() == cty.Number {
+		for _, option := range options {
+			checkValue, err := gocty.ToCtyValue(option, cty.Number)
+			if err != nil {
+				debug.Log("Error converting number for equality check. %s", err)
+				return false
+			}
+			if attr.Value().RawEquals(checkValue) {
+				return true
+			}
+		}
+	}
+	return false
+}
+
+func (attr *Attribute) IsNone(options ...interface{}) bool {
+	if attr.Value().Type() == cty.String {
+		for _, option := range options {
+			if option == attr.Value().AsString() {
+				return false
+			}
+		}
+	}
+	if attr.Value().Type() == cty.Number {
+		for _, option := range options {
+			checkValue, err := gocty.ToCtyValue(option, cty.Number)
+			if err != nil {
+				debug.Log("Error converting number for equality check. %s", err)
+				return false
+			}
+			if attr.Value().RawEquals(checkValue) {
+				return false
+			}
+
+		}
+	}
+
+	return true
+}


Test patch:
diff --git a/internal/app/tfsec/parser/attribute_test.go b/internal/app/tfsec/parser/attribute_test.go
index cb2d27512..83c9d4624 100644
--- a/internal/app/tfsec/parser/attribute_test.go
+++ b/internal/app/tfsec/parser/attribute_test.go
@@ -193,3 +193,121 @@ resource "aws_security_group" "my-security_group" {
 		})
 	}
 }
+
+func Test_AttributeIsAny(t *testing.T) {
+	var tests []struct {
+		name           string
+		source         string
+		checkAttribute string
+		checkValue     []interface{}
+		expectedResult bool
+	} = []struct {
+		name           string
+		source         string
+		checkAttribute string
+		checkValue     []interface{}
+		expectedResult bool
+	}{
+		{
+			name: "bucket acl is not one of the specified acls",
+			source: `
+resource "aws_s3_bucket" "my-bucket" {
+ 	bucket_name = "bucketName"
+	acl = "public-read"
+}`,
+			checkAttribute: "acl",
+			checkValue:     []interface{}{"private", "authenticated-read"},
+			expectedResult: false,
+		},
+		{
+			name: "bucket acl is one of the specified acls",
+			source: `
+resource "aws_s3_bucket" "my-bucket" {
+ 	bucket_name = "bucketName"
+	acl = "private"
+}`,
+			checkAttribute: "acl",
+			checkValue:     []interface{}{"private", "authenticated-read"},
+			expectedResult: true,
+		},
+		{
+			name: "is is one of the provided valued",
+			source: `
+resource "aws_security_group" "my-security_group" {
+	count = 1
+}`,
+			checkAttribute: "count",
+			checkValue:     []interface{}{1, 2},
+			expectedResult: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			blocks := createBlocksFromSource(test.source)
+			for _, block := range blocks {
+				if !block.HasChild(test.checkAttribute) {
+					t.Fail()
+				}
+				attr := block.GetAttribute(test.checkAttribute)
+				assert.Equal(t, attr.IsAny(test.checkValue...), test.expectedResult)
+			}
+		})
+	}
+}
+
+func Test_AttributeIsNone(t *testing.T) {
+	var tests = []struct {
+		name           string
+		source         string
+		checkAttribute string
+		checkValue     []interface{}
+		expectedResult bool
+	}{
+		{
+			name: "bucket acl is not one of the specified acls",
+			source: `
+resource "aws_s3_bucket" "my-bucket" {
+ 	bucket_name = "bucketName"
+	acl = "public-read"
+}`,
+			checkAttribute: "acl",
+			checkValue:     []interface{}{"private", "authenticated-read"},
+			expectedResult: true,
+		},
+		{
+			name: "bucket acl is one of the specified acls",
+			source: `
+resource "aws_s3_bucket" "my-bucket" {
+ 	bucket_name = "bucketName"
+	acl = "private"
+}`,
+			checkAttribute: "acl",
+			checkValue:     []interface{}{"private", "authenticated-read"},
+			expectedResult: false,
+		},
+		{
+			name: "count is non-of the provided values",
+			source: `
+resource "aws_security_group" "my-security_group" {
+	count = 0
+}`,
+			checkAttribute: "count",
+			checkValue:     []interface{}{1, 2},
+			expectedResult: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			blocks := createBlocksFromSource(test.source)
+			for _, block := range blocks {
+				if !block.HasChild(test.checkAttribute) {
+					t.Fail()
+				}
+				attr := block.GetAttribute(test.checkAttribute)
+				assert.Equal(t, attr.IsNone(test.checkValue...), test.expectedResult)
+			}
+		})
+	}
+}