Repository: aquasecurity/tfsec
Pull request #1015 (2021-08-11 23:28:19)

Add requiredSources functionality to filter modules
# Intent
Add a `requiredSources` filter block to custom rules to make checking modules simple and targeted.

# Problem
Currently there is no way to easily target specific modules for custom rules, especially so if there is no common label pattern across instances of the module.  For modules that are stored on a local path, matching filtering with a complex `matchSpec` does not work as the `source` value is relative to the root module's position and is not a constant value.

# Proposed Solution
Addition of a top level property called `requiredSources` which operates similarly to the `requiredLabels` property and targets a modules `source` attribute.  For modules that reference a local path in their `source` attribute, the path is resolved to a path relative to the current working directory.  This enables users to write a custom rule that targets a module regardless of where in the directory tree it is referenced.

As reference, imagine the following directory structure:
```
tf
├── apps
│   ├── applicationA
│   │   ├── common
│   │   └── nested
│   └── applicationB
└── common
```

Root modules defined in `tf/apps/applicationA/nested` and `tf/apps/applicationB` would reference the top level `common` directory using a `source` value of `../../../common` and `../../common` respectively.  With this path resolution implementation, it would be resolved to a consistent value of `common`.  This also ensures the `common` directory under `apps/applicationA/` does not incorrectly match.

# Notes
- I had to break up the `IsRuleRequiredForBlock` function due to a `gocyclo` complexity threshold.  That refactor is in a separate commit.


Patch:
diff --git a/internal/app/tfsec/custom/custom_check.go b/internal/app/tfsec/custom/custom_check.go
index f38423200..5d2f09af8 100644
--- a/internal/app/tfsec/custom/custom_check.go
+++ b/internal/app/tfsec/custom/custom_check.go
@@ -116,16 +116,17 @@ type MatchSpec struct {
 
 //Check specifies the check definition represented in json/yaml
 type Check struct {
-	Code           string            `json:"code" yaml:"code"`
-	Description    string            `json:"description" yaml:"description"`
-	RequiredTypes  []string          `json:"requiredTypes" yaml:"requiredTypes"`
-	RequiredLabels []string          `json:"requiredLabels" yaml:"requiredLabels"`
-	Severity       severity.Severity `json:"severity" yaml:"severity"`
-	ErrorMessage   string            `json:"errorMessage,omitempty" yaml:"errorMessage,omitempty"`
-	MatchSpec      *MatchSpec        `json:"matchSpec" yaml:"matchSpec"`
-	RelatedLinks   []string          `json:"relatedLinks,omitempty" yaml:"relatedLinks,omitempty"`
-	Impact         string            `json:"impact,omitempty" yaml:"impact,omitempty"`
-	Resolution     string            `json:"resolution,omitempty" yaml:"resolution,omitempty"`
+	Code            string            `json:"code" yaml:"code"`
+	Description     string            `json:"description" yaml:"description"`
+	RequiredTypes   []string          `json:"requiredTypes" yaml:"requiredTypes"`
+	RequiredLabels  []string          `json:"requiredLabels" yaml:"requiredLabels"`
+	RequiredSources []string          `json:"requiredSources" yaml:"requiredSources,omitempty"`
+	Severity        severity.Severity `json:"severity" yaml:"severity"`
+	ErrorMessage    string            `json:"errorMessage,omitempty" yaml:"errorMessage,omitempty"`
+	MatchSpec       *MatchSpec        `json:"matchSpec" yaml:"matchSpec"`
+	RelatedLinks    []string          `json:"relatedLinks,omitempty" yaml:"relatedLinks,omitempty"`
+	Impact          string            `json:"impact,omitempty" yaml:"impact,omitempty"`
+	Resolution      string            `json:"resolution,omitempty" yaml:"resolution,omitempty"`
 }
 
 func (action *CheckAction) isValid() bool {
diff --git a/internal/app/tfsec/custom/processing.go b/internal/app/tfsec/custom/processing.go
index e334de628..78b0e96d8 100644
--- a/internal/app/tfsec/custom/processing.go
+++ b/internal/app/tfsec/custom/processing.go
@@ -142,6 +142,7 @@ func processFoundChecks(checks ChecksFile) {
 				Provider:        provider.CustomProvider,
 				RequiredTypes:   customCheck.RequiredTypes,
 				RequiredLabels:  customCheck.RequiredLabels,
+				RequiredSources: customCheck.RequiredSources,
 				DefaultSeverity: severity.Medium,
 				CheckFunc: func(set result.Set, rootBlock block.Block, ctx *hclcontext.Context) {
 					matchSpec := customCheck.MatchSpec
diff --git a/pkg/rule/check.go b/pkg/rule/check.go
index f823a1a9d..4ccafc777 100644
--- a/pkg/rule/check.go
+++ b/pkg/rule/check.go
@@ -3,6 +3,7 @@ package rule
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	runtimeDebug "runtime/debug"
 	"strings"
 
@@ -49,34 +50,27 @@ func CheckRule(r *Rule, resourceBlock block.Block, ctx *hclcontext.Context, igno
 }
 
 // IsRuleRequiredForBlock returns true if the Rule should be applied to the given HCL block
-func IsRuleRequiredForBlock(rule *Rule, block block.Block) bool {
+func IsRuleRequiredForBlock(rule *Rule, b block.Block) bool {
 
 	if rule.CheckFunc == nil {
 		return false
 	}
 
 	if len(rule.RequiredTypes) > 0 {
-		var found bool
-		for _, requiredType := range rule.RequiredTypes {
-			if block.Type() == requiredType {
-				found = true
-				break
-			}
-		}
-		if !found {
+		if !checkRequiredTypesMatch(rule, b) {
 			return false
 		}
 	}
 
 	if len(rule.RequiredLabels) > 0 {
-		var found bool
-		for _, requiredLabel := range rule.RequiredLabels {
-			if requiredLabel == "*" || (len(block.Labels()) > 0 && wildcardMatch(requiredLabel, block.TypeLabel())) {
-				found = true
-				break
-			}
+		if !checkRequiredLabelsMatch(rule, b) {
+			return false
 		}
-		if !found {
+
+	}
+
+	if len(rule.RequiredSources) > 0 && b.Type() == block.TypeModule.Name() {
+		if !checkRequiredSourcesMatch(rule, b) {
 			return false
 		}
 	}
@@ -84,6 +78,75 @@ func IsRuleRequiredForBlock(rule *Rule, block block.Block) bool {
 	return true
 }
 
+func checkRequiredTypesMatch(rule *Rule, b block.Block) bool {
+	var found bool
+	for _, requiredType := range rule.RequiredTypes {
+		if b.Type() == requiredType {
+			found = true
+			break
+		}
+	}
+
+	return found
+}
+
+func checkRequiredLabelsMatch(rule *Rule, b block.Block) bool {
+	var found bool
+	for _, requiredLabel := range rule.RequiredLabels {
+		if requiredLabel == "*" || (len(b.Labels()) > 0 && wildcardMatch(requiredLabel, b.TypeLabel())) {
+			found = true
+			break
+		}
+	}
+
+	return found
+}
+
+func checkRequiredSourcesMatch(rule *Rule, b block.Block) bool {
+	var found bool
+	if sourceAttr := b.GetAttribute("source"); sourceAttr.IsNotNil() {
+		sourcePath := sourceAttr.ValueAsStrings()[0]
+
+		// resolve module source path to path relative to cwd
+		if strings.HasPrefix(sourcePath, ".") {
+			var err error
+			sourcePath, err = cleanPathRelativeToWorkingDir(filepath.Dir(b.Range().Filename), sourcePath)
+			if err != nil {
+				_, _ = fmt.Fprintf(os.Stderr, "WARNING: did not path for module %s due to error(s): %s\n", fmt.Sprintf("%s:%s", b.FullName(), b.Range().Filename), err)
+			}
+		}
+
+		for _, requiredSource := range rule.RequiredSources {
+			if requiredSource == "*" || wildcardMatch(requiredSource, sourcePath) {
+				found = true
+				break
+			}
+		}
+	}
+
+	return found
+}
+
+func cleanPathRelativeToWorkingDir(dir, path string) (string, error) {
+	absPath := filepath.Clean(filepath.Join(dir, path))
+
+	wDir, err := os.Getwd()
+	if err != nil {
+		return "", err
+	}
+
+	if !strings.HasSuffix(wDir, "/") {
+		wDir = filepath.Join(wDir, "/")
+	}
+
+	relPath, err := filepath.Rel(wDir, absPath)
+	if err != nil {
+		return "", err
+	}
+
+	return relPath, nil
+}
+
 func wildcardMatch(pattern string, subject string) bool {
 	if pattern == "" {
 		return false
diff --git a/pkg/rule/rule.go b/pkg/rule/rule.go
index f89398b45..74228440a 100644
--- a/pkg/rule/rule.go
+++ b/pkg/rule/rule.go
@@ -24,6 +24,7 @@ type Rule struct {
 	Provider        provider.Provider
 	RequiredTypes   []string
 	RequiredLabels  []string
+	RequiredSources []string
 	DefaultSeverity severity.Severity
 	CheckFunc       func(result.Set, block.Block, *hclcontext.Context)
 }


Test patch:
diff --git a/pkg/rule/check_test.go b/pkg/rule/check_test.go
new file mode 100644
index 000000000..c45e265ac
--- /dev/null
+++ b/pkg/rule/check_test.go
@@ -0,0 +1,195 @@
+package rule
+
+import (
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/block"
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/hclcontext"
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/parser"
+	"github.com/aquasecurity/tfsec/pkg/result"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRequiredSourcesMatch(t *testing.T) {
+
+	var moduleSource string = `resource "simple" "very" {
+		something = "1"
+	}`
+
+	var tests = []struct {
+		name       string
+		rule       Rule
+		source     string
+		modulePath string
+		expected   bool
+	}{
+		{
+			name: "check false evaluation when module not in required type",
+			rule: Rule{
+				RequiredTypes:  []string{"data"},
+				RequiredLabels: []string{"custom_module"},
+				CheckFunc:      func(result.Set, block.Block, *hclcontext.Context) {},
+			},
+			modulePath: "module",
+			source: `
+module "custom_module" {
+	source = "../module"
+}
+`,
+			expected: false,
+		},
+		{
+			name: "check false evaluation when requiredLabels doesn't match",
+			rule: Rule{
+				RequiredTypes:  []string{"module"},
+				RequiredLabels: []string{"dont_match"},
+				CheckFunc:      func(result.Set, block.Block, *hclcontext.Context) {},
+			},
+			modulePath: "module",
+			source: `
+module "custom_module" {
+	source = "../module"
+}
+`,
+			expected: false,
+		},
+		{
+			name: "check true evaluation when requiredTypes and requiredLabels match",
+			rule: Rule{
+				RequiredTypes:  []string{"module"},
+				RequiredLabels: []string{"*"},
+				CheckFunc:      func(result.Set, block.Block, *hclcontext.Context) {},
+			},
+			modulePath: "module",
+			source: `
+module "custom_module" {
+	source = "../module"
+}
+`,
+			expected: true,
+		},
+		{
+			name: "check false evaluation when requiredSources does not match",
+			rule: Rule{
+				RequiredTypes:   []string{"module"},
+				RequiredLabels:  []string{"*"},
+				RequiredSources: []string{"path_doesnt_match"},
+				CheckFunc:       func(result.Set, block.Block, *hclcontext.Context) {},
+			},
+			modulePath: "module",
+			source: `
+module "custom_module" {
+	source = "../module"
+}
+`,
+			expected: false,
+		},
+		{
+			name: "check true evaluation when requiredSources does match",
+			rule: Rule{
+				RequiredTypes:   []string{"module"},
+				RequiredLabels:  []string{"*"},
+				RequiredSources: []string{"github.com/hashicorp/example"},
+				CheckFunc:       func(result.Set, block.Block, *hclcontext.Context) {},
+			},
+			modulePath: "module",
+			source: `
+module "custom_module" {
+	source = "github.com/hashicorp/example"
+}
+`,
+			expected: true,
+		},
+		{
+			name: "check true evaluation when requiredSources does match with wildcard prefix",
+			rule: Rule{
+				RequiredTypes:   []string{"module"},
+				RequiredLabels:  []string{"*"},
+				RequiredSources: []string{"*two/three"},
+				CheckFunc:       func(result.Set, block.Block, *hclcontext.Context) {},
+			},
+			modulePath: "one/two/three",
+			source: `
+module "custom_module" {
+	source = "../one/two/three"
+}
+`,
+			expected: true,
+		},
+		{
+			name: "check true evaluation when requiredSources does match relative path match",
+			rule: Rule{
+				RequiredTypes:   []string{"module"},
+				RequiredLabels:  []string{"*"},
+				RequiredSources: []string{"one/two/three"},
+				CheckFunc:       func(result.Set, block.Block, *hclcontext.Context) {},
+			},
+			modulePath: "one/two/three",
+			source: `
+module "custom_module" {
+	source = "../one/two/three"
+}
+`,
+			expected: true,
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			block, testDir := createBlocksFromSourceWithModule(test.source, test.modulePath, moduleSource)
+			os.Chdir(testDir) // change directory for relative path tests to work
+			result := IsRuleRequiredForBlock(&test.rule, block[0])
+			assert.Equal(t, test.expected, result, "`IsRuleRequiredForBlock` match function evaluating incorrectly for requiredSources test.")
+		})
+	}
+}
+
+func createBlocksFromSourceWithModule(contents string, moduleSubDir string, moduleContents string) ([]block.Block, string) {
+	dir := createTestFileWithModuleSubDir(contents, moduleSubDir, moduleContents)
+	blocks, err := parser.New(dir, parser.OptionStopOnHCLError()).ParseDirectory()
+	if err != nil {
+		panic(err)
+	}
+	return blocks, dir
+}
+
+func createTestFileWithModuleSubDir(contents string, moduleSubDir string, moduleContents string) string {
+	var tempDir string
+	if runtime.GOOS == "darwin" {
+		// osx tmpdir path is a symlink to /private/var/... which messes with tests
+		osxTmpDir := os.TempDir()
+		if strings.HasPrefix(osxTmpDir, "/var") {
+			tempDir = filepath.Join("/private/", osxTmpDir)
+		}
+	}
+
+	dir, err := ioutil.TempDir(tempDir, "tfsec-testing-")
+	if err != nil {
+		panic(err)
+	}
+
+	rootPath := filepath.Join(dir, "main")
+	modulePath := filepath.Join(dir, moduleSubDir)
+
+	if err := os.Mkdir(rootPath, 0755); err != nil {
+		panic(err)
+	}
+
+	if err := os.MkdirAll(modulePath, 0755); err != nil {
+		panic(err)
+	}
+
+	if err := ioutil.WriteFile(filepath.Join(rootPath, "main.tf"), []byte(contents), 0755); err != nil {
+		panic(err)
+	}
+
+	if err := ioutil.WriteFile(filepath.Join(modulePath, "main.tf"), []byte(moduleContents), 0755); err != nil {
+		panic(err)
+	}
+
+	return dir
+}