From f0cbb79de971cc3fe9ca43109b3e4c62241ed549 Mon Sep 17 00:00:00 2001 From: Pankaj Patil Date: Sun, 10 Jan 2021 22:56:59 +0530 Subject: [PATCH 1/6] 1. support skip resource with comment. 2. skipped resource violations in output. --- pkg/iac-providers/kubernetes/v1/normalize.go | 47 +++++++++++++++++--- pkg/iac-providers/output/types.go | 8 +++- pkg/policy/opa/engine.go | 39 +++++++++------- pkg/policy/types.go | 5 ++- pkg/results/interface.go | 4 +- pkg/results/store.go | 18 ++++++-- pkg/results/types.go | 7 ++- pkg/termcolor/writer_test.go | 2 +- 8 files changed, 95 insertions(+), 35 deletions(-) diff --git a/pkg/iac-providers/kubernetes/v1/normalize.go b/pkg/iac-providers/kubernetes/v1/normalize.go index bfc518062..ffc693894 100644 --- a/pkg/iac-providers/kubernetes/v1/normalize.go +++ b/pkg/iac-providers/kubernetes/v1/normalize.go @@ -28,7 +28,11 @@ import ( "gopkg.in/yaml.v3" ) -const terrascanSkip = "terrascanSkip" +const ( + terrascanSkip = "terrascanSkip" + terrascanSkipRule = "rule" + terrascanSkipComment = "comment" +) var ( errUnsupportedDoc = fmt.Errorf("unsupported document type") @@ -146,7 +150,7 @@ func (k *K8sV1) Normalize(doc *utils.IacDocument) (*output.ResourceConfig, error return &resourceConfig, nil } -func readSkipRulesFromAnnotations(annotations map[string]interface{}, resourceID string) []string { +func readSkipRulesFromAnnotations(annotations map[string]interface{}, resourceID string) []output.SkipRule { var skipRulesFromAnnotations interface{} var ok bool @@ -155,18 +159,47 @@ func readSkipRulesFromAnnotations(annotations map[string]interface{}, resourceID return nil } - skipRules := make([]string, 0) + skipRules := make([]output.SkipRule, 0) if rules, ok := skipRulesFromAnnotations.([]interface{}); ok { for _, rule := range rules { - if value, ok := rule.(string); ok { - skipRules = append(skipRules, value) + if value, ok := rule.(map[string]interface{}); ok { + skipRule := getSkipRuleObject(value) + if skipRule != nil { + skipRules = append(skipRules, *skipRule) + } } else { - zap.S().Debugf("each rule in %s must be of string type", terrascanSkip) + zap.S().Debugf("each rule in %s must be of map type", terrascanSkip) } } } else { - zap.S().Debugf("%s must be an array of rules to skip", terrascanSkip) + zap.S().Debugf("%s must be an array of {rule: ruleID, comment: reason for skipping}", terrascanSkip) } return skipRules } + +func getSkipRuleObject(m map[string]interface{}) *output.SkipRule { + var skipRule output.SkipRule + var rule, comment interface{} + var ok bool + + // get rule, if rule not found return nil + if rule, ok = m[terrascanSkipRule]; ok { + if _, ok = rule.(string); ok { + skipRule.Rule = rule.(string) + } else { + return nil + } + } else { + return nil + } + + // get comment + if comment, ok = m[terrascanSkipComment]; ok { + if _, ok = comment.(string); ok { + skipRule.Comment = comment.(string) + } + } + + return &skipRule +} diff --git a/pkg/iac-providers/output/types.go b/pkg/iac-providers/output/types.go index cfa19c2db..425a12711 100644 --- a/pkg/iac-providers/output/types.go +++ b/pkg/iac-providers/output/types.go @@ -27,7 +27,13 @@ type ResourceConfig struct { // SkipRules will hold the rules to be skipped for the resource. // Each iac provider should append the rules to be skipped for a resource, // while extracting resource from the iac files - SkipRules []string `json:"skip_rules"` + SkipRules []SkipRule `json:"skip_rules" yaml:"skip_rules"` +} + +// SkipRule struct will hold the skipped rule and any comment for the skipped rule +type SkipRule struct { + Rule string `json:"rule"` + Comment string `json:"comment"` } // AllResourceConfigs is a list/slice of resource configs present in IaC diff --git a/pkg/policy/opa/engine.go b/pkg/policy/opa/engine.go index 09777e0b0..e5c92a394 100644 --- a/pkg/policy/opa/engine.go +++ b/pkg/policy/opa/engine.go @@ -285,7 +285,7 @@ func (e *Engine) Release() error { } // reportViolation Add a violation for a given resource -func (e *Engine) reportViolation(regoData *RegoData, resource *output.ResourceConfig) { +func (e *Engine) reportViolation(regoData *RegoData, resource *output.ResourceConfig, isSkipped bool, skipComment string) { violation := results.Violation{ RuleName: regoData.Metadata.Name, Description: regoData.Metadata.Description, @@ -301,20 +301,24 @@ func (e *Engine) reportViolation(regoData *RegoData, resource *output.ResourceCo LineNumber: resource.Line, } - severity := regoData.Metadata.Severity - if strings.ToLower(severity) == "high" { - e.results.ViolationStore.Summary.HighCount++ - } else if strings.ToLower(severity) == "medium" { - e.results.ViolationStore.Summary.MediumCount++ - } else if strings.ToLower(severity) == "low" { - e.results.ViolationStore.Summary.LowCount++ + if !isSkipped { + severity := regoData.Metadata.Severity + if strings.ToLower(severity) == "high" { + e.results.ViolationStore.Summary.HighCount++ + } else if strings.ToLower(severity) == "medium" { + e.results.ViolationStore.Summary.MediumCount++ + } else if strings.ToLower(severity) == "low" { + e.results.ViolationStore.Summary.LowCount++ + } else { + zap.S().Warn("invalid severity found in rule definition", + zap.String("rule id", violation.RuleID), zap.String("severity", severity)) + } + e.results.ViolationStore.AddResult(&violation, false) + e.results.ViolationStore.Summary.ViolatedPolicies++ } else { - zap.S().Warn("invalid severity found in rule definition", - zap.String("rule id", violation.RuleID), zap.String("severity", severity)) + violation.Comment = skipComment + e.results.ViolationStore.AddResult(&violation, true) } - e.results.ViolationStore.Summary.ViolatedPolicies++ - - e.results.ViolationStore.AddResult(&violation) } // Evaluate Executes compiled OPA queries against the input JSON data @@ -378,16 +382,19 @@ func (e *Engine) Evaluate(engineInput policy.EngineInput) (policy.EngineOutput, continue } - // do no report violations if rule is skipped for resource + // add to skipped violations if rule is skipped for resource if len(resource.SkipRules) > 0 { found := false + var skipComment string for _, rule := range resource.SkipRules { - if strings.EqualFold(k, rule) { + if strings.EqualFold(k, rule.Rule) { found = true + skipComment = rule.Comment break } } if found { + e.reportViolation(e.regoDataMap[k], resource, true, skipComment) zap.S().Debugf("rule: %s skipped for resource: %s", k, resource.Name) continue } @@ -401,7 +408,7 @@ func (e *Engine) Evaluate(engineInput policy.EngineInput) (policy.EngineOutput, zap.S().Debug("violation found for rule with rego", zap.String("rego", string("\n")+string(e.regoDataMap[k].RawRego)+string("\n"))) // Report the violation - e.reportViolation(e.regoDataMap[k], resource) + e.reportViolation(e.regoDataMap[k], resource, false, "") } } diff --git a/pkg/policy/types.go b/pkg/policy/types.go index 3250ea60e..33b192344 100644 --- a/pkg/policy/types.go +++ b/pkg/policy/types.go @@ -32,7 +32,8 @@ func (me EngineOutput) AsViolationStore() results.ViolationStore { return results.ViolationStore{} } return results.ViolationStore{ - Violations: me.Violations, - Summary: me.Summary, + Violations: me.Violations, + SkippedViolations: me.SkippedViolations, + Summary: me.Summary, } } diff --git a/pkg/results/interface.go b/pkg/results/interface.go index 66718591d..59b6a5f52 100644 --- a/pkg/results/interface.go +++ b/pkg/results/interface.go @@ -2,6 +2,6 @@ package results // Store manages the storage and export of results information type Store interface { - AddResult(violation *Violation) - GetResults() []*Violation + AddResult(violation *Violation, isSkipped bool) + GetResults(isSkipped bool) []*Violation } diff --git a/pkg/results/store.go b/pkg/results/store.go index 8e3252951..8b629ad1b 100644 --- a/pkg/results/store.go +++ b/pkg/results/store.go @@ -19,16 +19,26 @@ package results // NewViolationStore returns a new violation store func NewViolationStore() *ViolationStore { return &ViolationStore{ - Violations: []*Violation{}, + Violations: []*Violation{}, + SkippedViolations: []*Violation{}, } } // AddResult Adds individual violations into the violation store -func (s *ViolationStore) AddResult(violation *Violation) { - s.Violations = append(s.Violations, violation) +// when skip is true, violations are added to skipped violations +func (s *ViolationStore) AddResult(violation *Violation, isSkipped bool) { + if isSkipped { + s.SkippedViolations = append(s.SkippedViolations, violation) + } else { + s.Violations = append(s.Violations, violation) + } } // GetResults Retrieves all violations from the violation store -func (s *ViolationStore) GetResults() []*Violation { +// when skip is true, it returns only the skipped violations +func (s *ViolationStore) GetResults(isSkipped bool) []*Violation { + if isSkipped { + return s.SkippedViolations + } return s.Violations } diff --git a/pkg/results/types.go b/pkg/results/types.go index f7a0ac16e..fcf5456e3 100644 --- a/pkg/results/types.go +++ b/pkg/results/types.go @@ -29,6 +29,7 @@ type Violation struct { Category string `json:"category" yaml:"category" xml:"category,attr"` RuleFile string `json:"-" yaml:"-" xml:"-"` RuleData interface{} `json:"-" yaml:"-" xml:"-"` + Comment string `json:"skip_comment,omitempty" yaml:"skip_comment,omitempty" xml:"skip_comment,omitempty"` ResourceName string `json:"resource_name" yaml:"resource_name" xml:"resource_name,attr"` ResourceType string `json:"resource_type" yaml:"resource_type" xml:"resource_type,attr"` ResourceData interface{} `json:"-" yaml:"-" xml:"-"` @@ -38,8 +39,9 @@ type Violation struct { // ViolationStore Storage area for violation data type ViolationStore struct { - Violations []*Violation `json:"violations" yaml:"violations" xml:"violations>violation"` - Summary ScanSummary `json:"scan_summary" yaml:"scan_summary" xml:"scan_summary"` + Violations []*Violation `json:"violations" yaml:"violations" xml:"violations>violation"` + SkippedViolations []*Violation `json:"skipped_violations" yaml:"skipped_violations" xml:"skipped_violations>violation"` + Summary ScanSummary `json:"scan_summary" yaml:"scan_summary" xml:"scan_summary"` } // ScanSummary will hold the default scan summary data @@ -59,6 +61,7 @@ type ScanSummary struct { func (vs ViolationStore) Add(extra ViolationStore) ViolationStore { // Just concatenate the slices, since order shouldn't be important vs.Violations = append(vs.Violations, extra.Violations...) + vs.SkippedViolations = append(vs.SkippedViolations, extra.SkippedViolations...) // Add the scan summary vs.Summary.LowCount += extra.Summary.LowCount diff --git a/pkg/termcolor/writer_test.go b/pkg/termcolor/writer_test.go index 42159b272..4e4fe4ecd 100644 --- a/pkg/termcolor/writer_test.go +++ b/pkg/termcolor/writer_test.go @@ -26,7 +26,7 @@ func buildStore() *results.ViolationStore { ResourceType: "resource type", File: "file", LineNumber: 1, - }) + }, false) return res } From 580847e3e191d5b5742b73034acaf33e95aea546 Mon Sep 17 00:00:00 2001 From: Pankaj Patil Date: Sun, 10 Jan 2021 23:11:20 +0530 Subject: [PATCH 2/6] modify tests for k8s iac-provider --- pkg/cli/testdata/run-test/test_pod.yaml | 10 +- .../kubernetes/v1/normalize_test.go | 110 ++++++++++++++++-- .../file-test-data/test_pod_skip_rules.yaml | 10 +- 3 files changed, 114 insertions(+), 16 deletions(-) diff --git a/pkg/cli/testdata/run-test/test_pod.yaml b/pkg/cli/testdata/run-test/test_pod.yaml index 925a9fd34..1830d34bc 100644 --- a/pkg/cli/testdata/run-test/test_pod.yaml +++ b/pkg/cli/testdata/run-test/test_pod.yaml @@ -7,7 +7,9 @@ metadata: test: someupdate test2: someupdate3 annotations: - terrascanSkip: [accurics.kubernetes.IAM.109] + terrascanSkip: + - rule: accurics.kubernetes.IAM.109 + comment: reason to skip the rule spec: containers: - name: myapp-container @@ -25,7 +27,11 @@ metadata: test: someupdate test2: someupdate3 annotations: - terrascanSkip: [accurics.kubernetes.IAM.3, accurics.kubernetes.OPS.461] + terrascanSkip: + - rule: accurics.kubernetes.IAM.3 + comment: reason to skip the rule + - rule: accurics.kubernetes.OPS.461 + comment: reason to skip the rule spec: template: spec: diff --git a/pkg/iac-providers/kubernetes/v1/normalize_test.go b/pkg/iac-providers/kubernetes/v1/normalize_test.go index 3af534e97..07699fab3 100644 --- a/pkg/iac-providers/kubernetes/v1/normalize_test.go +++ b/pkg/iac-providers/kubernetes/v1/normalize_test.go @@ -55,7 +55,9 @@ kind: Pod metadata: name: myapp-pod annotations: - terrascanSkip: [accurics.kubernetes.IAM.109] + terrascanSkip: + - rule: accurics.kubernetes.IAM.109 + comment: reason to skip the rule spec: containers: - name: myapp-container @@ -121,7 +123,10 @@ func TestK8sV1ExtractResource(t *testing.T) { Metadata: k8sMetadata{ Name: "myapp-pod", Annotations: map[string]interface{}{ - terrascanSkip: []interface{}{"accurics.kubernetes.IAM.109"}, + terrascanSkip: []interface{}{map[string]interface{}{ + "rule": "accurics.kubernetes.IAM.109", + "comment": "reason to skip the rule", + }}, }, }, }, @@ -179,6 +184,12 @@ func TestK8sV1GetNormalizedName(t *testing.T) { func TestK8sV1Normalize(t *testing.T) { testRule := "accurics.kubernetes.IAM.109" + testComment := "reason to skip the rule" + + testSkipRule := output.SkipRule{ + Rule: testRule, + Comment: testComment, + } type args struct { doc *utils.IacDocument @@ -215,7 +226,10 @@ func TestK8sV1Normalize(t *testing.T) { "kind": "Pod", "metadata": map[string]interface{}{ "annotations": map[string]interface{}{ - terrascanSkip: []interface{}{testRule}, + terrascanSkip: []interface{}{map[string]interface{}{ + "rule": testRule, + "comment": testComment, + }}, }, "name": "myapp-pod", }, @@ -262,7 +276,7 @@ func TestK8sV1Normalize(t *testing.T) { }, }, }, - SkipRules: []string{testRule}, + SkipRules: []output.SkipRule{testSkipRule}, }, }, } @@ -284,8 +298,13 @@ func TestK8sV1Normalize(t *testing.T) { func TestReadSkipRulesFromAnnotations(t *testing.T) { // test data testRuleA := "RuleA" + testCommentA := "RuleA can be skipped" testRuleB := "RuleB" + testCommentB := "RuleB must be skipped" testRuleC := "RuleC" + testCommentC := "RuleC skipped" + + testSkipRule := output.SkipRule{Rule: testRuleA} type args struct { annotations map[string]interface{} @@ -294,7 +313,7 @@ func TestReadSkipRulesFromAnnotations(t *testing.T) { tests := []struct { name string args args - want []string + want []output.SkipRule }{ { name: "nil annotations", @@ -317,34 +336,101 @@ func TestReadSkipRulesFromAnnotations(t *testing.T) { terrascanSkip: "test", }, }, - want: []string{}, + want: []output.SkipRule{}, }, { - name: "annotations with invalid terrascanSkipRules rule value", + name: "annotations with invalid SkipRule object", args: args{ annotations: map[string]interface{}{ terrascanSkip: []interface{}{1}, }, }, - want: []string{}, + want: []output.SkipRule{}, + }, + { + name: "annotations with invalid terrascanSkipRules rule value", + args: args{ + annotations: map[string]interface{}{ + terrascanSkip: []interface{}{map[string]interface{}{ + terrascanSkipRule: 1, + }}, + }, + }, + want: []output.SkipRule{}, }, { name: "annotations with one terrascanSkipRules", args: args{ annotations: map[string]interface{}{ - terrascanSkip: []interface{}{testRuleA}, + terrascanSkip: []interface{}{map[string]interface{}{ + terrascanSkipRule: testRuleA, + }}, + }, + }, + want: []output.SkipRule{ + { + Rule: testRuleA, }, }, - want: []string{testRuleA}, }, { name: "annotations with multiple terrascanSkipRules", args: args{ annotations: map[string]interface{}{ - terrascanSkip: []interface{}{testRuleA, testRuleB, testRuleC}, + terrascanSkip: []interface{}{ + map[string]interface{}{ + terrascanSkipRule: testRuleA, + terrascanSkipComment: testCommentA, + }, + map[string]interface{}{ + terrascanSkipRule: testRuleB, + terrascanSkipComment: testCommentB, + }, + map[string]interface{}{ + terrascanSkipRule: testRuleC, + terrascanSkipComment: testCommentC, + }}, + }, + }, + want: []output.SkipRule{ + { + Rule: testRuleA, + Comment: testCommentA, + }, + { + Rule: testRuleB, + Comment: testCommentB, + }, + { + Rule: testRuleC, + Comment: testCommentC, + }, + }, + }, + { + name: "annotations with invalid rule key in terrascanSkipRules", + args: args{ + annotations: map[string]interface{}{ + terrascanSkip: []interface{}{ + map[string]interface{}{ + "skip-rule": testRuleA, + terrascanSkipComment: testCommentA, + }}, + }, + }, + want: []output.SkipRule{}, + }, + { + name: "annotations with no comment key in terrascanSkipRules", + args: args{ + annotations: map[string]interface{}{ + terrascanSkip: []interface{}{ + map[string]interface{}{ + terrascanSkipRule: testRuleA, + }}, }, }, - want: []string{testRuleA, testRuleB, testRuleC}, + want: []output.SkipRule{testSkipRule}, }, } for _, tt := range tests { diff --git a/pkg/iac-providers/kubernetes/v1/testdata/file-test-data/test_pod_skip_rules.yaml b/pkg/iac-providers/kubernetes/v1/testdata/file-test-data/test_pod_skip_rules.yaml index 04ab9a462..05dc75778 100644 --- a/pkg/iac-providers/kubernetes/v1/testdata/file-test-data/test_pod_skip_rules.yaml +++ b/pkg/iac-providers/kubernetes/v1/testdata/file-test-data/test_pod_skip_rules.yaml @@ -7,7 +7,9 @@ metadata: test: someupdate test2: someupdate3 annotations: - terrascanSkip: [accurics.kubernetes.IAM.109] + terrascanSkip: + - rule: accurics.kubernetes.IAM.109 + comment: reason to skip the rule spec: containers: - name: myapp-container @@ -25,7 +27,11 @@ metadata: test: someupdate test2: someupdate3 annotations: - terrascanSkip: [accurics.kubernetes.IAM.3, accurics.kubernetes.OPS.461] + terrascanSkip: + - rule: accurics.kubernetes.IAM.3 + comment: reason to skip the rule + - rule: accurics.kubernetes.OPS.461 + comment: reason to skip the rule spec: template: spec: From d583cd39a332f76b87637acf5a5a7dca2a6868db Mon Sep 17 00:00:00 2001 From: Pankaj Patil Date: Sun, 10 Jan 2021 23:20:08 +0530 Subject: [PATCH 3/6] fix writer tests --- pkg/writer/json_test.go | 13 +++++++++++++ pkg/writer/xml_test.go | 3 +++ pkg/writer/yaml_test.go | 26 +++++++++++++++++++++++++- 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/pkg/writer/json_test.go b/pkg/writer/json_test.go index 824f69227..34d869518 100644 --- a/pkg/writer/json_test.go +++ b/pkg/writer/json_test.go @@ -39,6 +39,19 @@ const ( "line": 20 } ], + "skipped_violations": [ + { + "rule_name": "s3EnforceUserACL", + "description": "S3 bucket Access is allowed to all AWS Account Users.", + "rule_id": "AWS.S3Bucket.DS.High.1043", + "severity": "HIGH", + "category": "S3", + "resource_name": "bucket", + "resource_type": "aws_s3_bucket", + "file": "modules/m1/main.tf", + "line": 20 + } + ], "scan_summary": { "file/folder": "test", "iac_type": "terraform", diff --git a/pkg/writer/xml_test.go b/pkg/writer/xml_test.go index f7cba8baf..2b99d95ec 100644 --- a/pkg/writer/xml_test.go +++ b/pkg/writer/xml_test.go @@ -14,6 +14,9 @@ const ( + + + ` diff --git a/pkg/writer/yaml_test.go b/pkg/writer/yaml_test.go index e7c34bd02..7d92d4aee 100644 --- a/pkg/writer/yaml_test.go +++ b/pkg/writer/yaml_test.go @@ -43,6 +43,20 @@ var ( LineNumber: 20, }, }, + SkippedViolations: []*results.Violation{ + { + RuleName: "s3EnforceUserACL", + Description: "S3 bucket Access is allowed to all AWS Account Users.", + RuleID: "AWS.S3Bucket.DS.High.1043", + Severity: "HIGH", + Category: "S3", + Comment: "", + ResourceName: "bucket", + ResourceType: "aws_s3_bucket", + File: "modules/m1/main.tf", + LineNumber: 20, + }, + }, Summary: results.ScanSummary{ ResourcePath: "test", IacType: "terraform", @@ -67,7 +81,7 @@ const ( config: bucket: ${module.m3.fullbucketname} policy: ${module.m2.fullbucketpolicy} - skiprules: []` + skip_rules: []` scanTestOutputYAML = `results: violations: @@ -80,6 +94,16 @@ const ( resource_type: aws_s3_bucket file: modules/m1/main.tf line: 20 + skipped_violations: + - rule_name: s3EnforceUserACL + description: S3 bucket Access is allowed to all AWS Account Users. + rule_id: AWS.S3Bucket.DS.High.1043 + severity: HIGH + category: S3 + resource_name: bucket + resource_type: aws_s3_bucket + file: modules/m1/main.tf + line: 20 scan_summary: file/folder: test iac_type: terraform From 63d9867556f53569a74342111a684b1601fec30c Mon Sep 17 00:00:00 2001 From: Pankaj Patil Date: Mon, 11 Jan 2021 06:32:00 +0530 Subject: [PATCH 4/6] 1. support of rule skip comment for tf 2. human readable output modifications --- pkg/utils/skip_rules.go | 34 ++++++++++++--- pkg/utils/skip_rules_test.go | 79 +++++++++++++++++++++++++++------ pkg/writer/human_readable.go | 84 +++++++++++++++++++++++++++--------- pkg/writer/yaml_test.go | 17 ++++---- 4 files changed, 167 insertions(+), 47 deletions(-) diff --git a/pkg/utils/skip_rules.go b/pkg/utils/skip_rules.go index 08e464d6b..81565606b 100644 --- a/pkg/utils/skip_rules.go +++ b/pkg/utils/skip_rules.go @@ -19,19 +19,22 @@ package utils import ( "regexp" "strings" + + "github.com/accurics/terrascan/pkg/iac-providers/output" ) var ( - skipRulesPattern = regexp.MustCompile(`#ts:skip=\s*(([A-Za-z0-9]+\.?){5})(\s*,\s*([A-Za-z0-9]+\.?){5})*`) - skipRulesPrefix = "#ts:skip=" + skipRulesPattern = regexp.MustCompile(`#ts:skip=\s*(([A-Za-z0-9]+\.?){5}(\s*[|][\w\s]*\.){0,1})(\s*,\s*([A-Za-z0-9]+\.?){5}(\s*[|][\w\s]*\.){0,1})*`) + skipRulesPrefix = "#ts:skip=" + ruleCommentSeparator = "|" ) // GetSkipRules returns a list of rules to be skipped. The rules to be skipped // can be set in terraform resource config with the following comma separated pattern: // #ts:skip=AWS.S3Bucket.DS.High.1043, AWS.S3Bucket.DS.High.1044 -func GetSkipRules(body string) []string { +func GetSkipRules(body string) []output.SkipRule { - var skipRules []string + var skipRules []output.SkipRule // check if any rules comments are present in body if !skipRulesPattern.MatchString(body) { @@ -44,8 +47,27 @@ func GetSkipRules(body string) []string { // extract rule ids from comments for _, c := range comments { c = strings.TrimPrefix(c, skipRulesPrefix) - c = strings.ReplaceAll(c, ",", " ") - skipRules = append(skipRules, strings.Fields(c)...) + cmts := strings.Split(c, ",") + for _, v := range cmts { + skipRule := getSkipRuleObject(v) + if skipRule != nil { + skipRules = append(skipRules, *skipRule) + } + } } return skipRules } + +func getSkipRuleObject(s string) *output.SkipRule { + if s == "" { + return nil + } + var skipRule output.SkipRule + ruleComment := strings.Split(s, ruleCommentSeparator) + + skipRule.Rule = strings.TrimSpace(ruleComment[0]) + if len(ruleComment) > 1 { + skipRule.Comment = strings.TrimSpace(ruleComment[1]) + } + return &skipRule +} diff --git a/pkg/utils/skip_rules_test.go b/pkg/utils/skip_rules_test.go index 182c98aa1..fe4e55add 100644 --- a/pkg/utils/skip_rules_test.go +++ b/pkg/utils/skip_rules_test.go @@ -19,40 +19,93 @@ package utils import ( "reflect" "testing" + + "github.com/accurics/terrascan/pkg/iac-providers/output" ) // ---------------------- unit tests -------------------------------- // func TestGetSkipRules(t *testing.T) { + testRule1 := "AWS.S3Bucket.DS.High.1041" + testRule2 := "AWS.S3Bucket.DS.High.1042" table := []struct { name string input string - expected []string + expected []output.SkipRule }{ { name: "no rules", input: "no rules here", - // expected would be an empty slice of strings + // expected would be an empty slice of output.SkipRule + }, + { + name: "single rule id with no comment", + input: "#ts:skip=AWS.S3Bucket.DS.High.1041", + expected: []output.SkipRule{ + {Rule: testRule1}, + }, }, { - name: "single rule id", - input: "#ts:skip=AWS.S3Bucket.DS.High.1041", - expected: []string{"AWS.S3Bucket.DS.High.1041"}, + name: "single rule id with comment", + input: "#ts:skip=AWS.S3Bucket.DS.High.1041 | this rule should be skipped.", + expected: []output.SkipRule{ + { + Rule: testRule1, + Comment: "this rule should be skipped.", + }, + }, }, { - name: "multiple comma separated no space", - input: "#ts:skip=AWS.S3Bucket.DS.High.1041,AWS.S3Bucket.DS.High.1042", - expected: []string{"AWS.S3Bucket.DS.High.1041", "AWS.S3Bucket.DS.High.1042"}, + name: "multiple comma separated no space, with comments", + input: "#ts:skip=AWS.S3Bucket.DS.High.1041|some reason to skip. , AWS.S3Bucket.DS.High.1042| should_skip_the_rule.", + expected: []output.SkipRule{ + { + Rule: testRule1, + Comment: "some reason to skip.", + }, + { + Rule: testRule2, + Comment: "should_skip_the_rule.", + }, + }, }, { - name: "multiple comma separated random space", + name: "multiple comma separated random space, without comments", input: "#ts:skip= AWS.S3Bucket.DS.High.1041 , AWS.S3Bucket.DS.High.1042", - expected: []string{"AWS.S3Bucket.DS.High.1041", "AWS.S3Bucket.DS.High.1042"}, + expected: []output.SkipRule{ + { + Rule: testRule1, + }, + { + Rule: testRule2, + }, + }, }, { - name: "sample resource config", - input: "{\n #ts:skip=AWS.S3Bucket.DS.High.1041\n region = var.region\n #ts:skip=AWS.S3Bucket.DS.High.1042 AWS.S3Bucket.DS.High.1043\n bucket = local.bucket_name\n #ts:skip=AWS.S3Bucket.DS.High.1044,AWS.S3Bucket.DS.High.1045\n force_destroy = true\n #ts:skip= AWS.S3Bucket.DS.High.1046 , AWS.S3Bucket.DS.High.1047\n acl = \"public-read\"\n }", - expected: []string{"AWS.S3Bucket.DS.High.1041", "AWS.S3Bucket.DS.High.1042", "AWS.S3Bucket.DS.High.1044", "AWS.S3Bucket.DS.High.1045", "AWS.S3Bucket.DS.High.1046", "AWS.S3Bucket.DS.High.1047"}, + name: "sample resource config", + input: "{\n #ts:skip=AWS.S3Bucket.DS.High.1041 | skip the rule. \n region = var.region\n #ts:skip=AWS.S3Bucket.DS.High.1042 AWS.S3Bucket.DS.High.1043\n bucket = local.bucket_name\n #ts:skip=AWS.S3Bucket.DS.High.1044 | resource skipped for this rule. ,AWS.S3Bucket.DS.High.1045\n force_destroy = true\n #ts:skip= AWS.S3Bucket.DS.High.1046 , AWS.S3Bucket.DS.High.1047\n acl = \"public-read\"\n }", + expected: []output.SkipRule{ + { + Rule: testRule1, + Comment: "skip the rule.", + }, + { + Rule: testRule2, + }, + { + Rule: "AWS.S3Bucket.DS.High.1044", + Comment: "resource skipped for this rule.", + }, + { + Rule: "AWS.S3Bucket.DS.High.1045", + }, + { + Rule: "AWS.S3Bucket.DS.High.1046", + }, + { + Rule: "AWS.S3Bucket.DS.High.1047", + }, + }, }, } diff --git a/pkg/writer/human_readable.go b/pkg/writer/human_readable.go index 27fe4ca36..898c49546 100644 --- a/pkg/writer/human_readable.go +++ b/pkg/writer/human_readable.go @@ -18,9 +18,11 @@ package writer import ( "bytes" + "fmt" "io" "text/template" + "github.com/accurics/terrascan/pkg/results" "go.uber.org/zap" ) @@ -32,30 +34,27 @@ const ( Violation Details - {{- $showDetails := .ViolationStore.Summary.ShowViolationDetails}} {{range $index, $element := .ViolationStore.Violations}} - {{printf "%-15v" "Description"}}:{{"\t"}}{{$element.Description}} - {{printf "%-15v" "File"}}:{{"\t"}}{{$element.File}} - {{printf "%-15v" "Line"}}:{{"\t"}}{{$element.LineNumber}} - {{printf "%-15v" "Severity"}}:{{"\t"}}{{$element.Severity}} - {{if $showDetails -}} - {{printf "%-15v" "Rule Name"}}:{{"\t"}}{{$element.RuleName}} - {{printf "%-15v" "Rule ID"}}:{{"\t"}}{{$element.RuleID}} - {{printf "%-15v" "Resource Name"}}:{{"\t"}}{{if $element.ResourceName}}{{$element.ResourceName}}{{else}}""{{end}} - {{printf "%-15v" "Resource Type"}}:{{"\t"}}{{$element.ResourceType}} - {{printf "%-15v" "Category"}}:{{"\t"}}{{$element.Category}} + {{defaultViolations $element false | printf "%s"}} + {{- if $showDetails -}} + {{detailedViolations $element | printf "%s"}} + {{- end}} + ----------------------------------------------------------------------- {{end}} +{{end}} +{{- if (gt (len .ViolationStore.SkippedViolations) 0) }} +Skipped Violations - + {{- $showDetails := .ViolationStore.Summary.ShowViolationDetails}} + {{range $index, $element := .ViolationStore.SkippedViolations}} + {{defaultViolations $element true | printf "%s"}} + {{- if $showDetails -}} + {{detailedViolations $element | printf "%s"}} + {{- end}} ----------------------------------------------------------------------- {{end}} -{{end}} +{{end}} Scan Summary - - {{printf "%-20v" "File/Folder"}}:{{"\t"}}{{.ViolationStore.Summary.ResourcePath}} - {{printf "%-20v" "IaC Type"}}:{{"\t"}}{{.ViolationStore.Summary.IacType}} - {{printf "%-20v" "Scanned At"}}:{{"\t"}}{{.ViolationStore.Summary.Timestamp}} - {{printf "%-20v" "Policies Validated"}}:{{"\t"}}{{.ViolationStore.Summary.TotalPolicies}} - {{printf "%-20v" "Violated Policies"}}:{{"\t"}}{{.ViolationStore.Summary.ViolatedPolicies}} - {{printf "%-20v" "Low"}}:{{"\t"}}{{.ViolationStore.Summary.LowCount}} - {{printf "%-20v" "Medium"}}:{{"\t"}}{{.ViolationStore.Summary.MediumCount}} - {{printf "%-20v" "High"}}:{{"\t"}}{{.ViolationStore.Summary.HighCount}} + {{scanSummary .ViolationStore.Summary | printf "%s"}} ` ) @@ -65,7 +64,11 @@ func init() { // HumanReadbleWriter display scan summary in human readable format func HumanReadbleWriter(data interface{}, writer io.Writer) error { - tmpl, err := template.New("Report").Parse(defaultTemplate) + tmpl, err := template.New("Report").Funcs(template.FuncMap{ + "defaultViolations": defaultViolations, + "detailedViolations": detailedViolations, + "scanSummary": scanSummary, + }).Parse(defaultTemplate) if err != nil { zap.S().Errorf("failed to write human readable output. error: '%v'", err) return err @@ -81,3 +84,44 @@ func HumanReadbleWriter(data interface{}, writer io.Writer) error { writer.Write([]byte{'\n'}) return nil } + +func defaultViolations(v results.Violation, isSkipped bool) string { + out := fmt.Sprintf("%-15v:\t%s\n\t%-15v:\t%s\n\t%-15v:\t%d\n\t%-15v:\t%s\n\t", + "Description", v.Description, + "File", v.File, + "Line", v.LineNumber, + "Severity", v.Severity) + if isSkipped { + skipComment := fmt.Sprintf("%-15v:\t%s\n\t", "Skip Comment", v.Comment) + out = out + skipComment + } + return out +} + +func detailedViolations(v results.Violation) string { + resourceName := v.ResourceName + // print "" when as resource name value when it is empty + if resourceName == "" { + resourceName = `""` + } + out := fmt.Sprintf("%-15v:\t%s\n\t%-15v:\t%s\n\t%-15v:\t%s\n\t%-15v:\t%s\n\t%-15v:\t%s\n\t", + "Rule Name", v.RuleName, + "Rule ID", v.RuleID, + "Resource Name", resourceName, + "Resource Type", v.ResourceType, + "Category", v.Category) + return out +} + +func scanSummary(s results.ScanSummary) string { + out := fmt.Sprintf("%-20v:\t%s\n\t%-20v:\t%s\n\t%-20v:\t%s\n\t%-20v:\t%d\n\t%-20v:\t%d\n\t%-20v:\t%d\n\t%-20v:\t%d\n\t%-20v:\t%d\n\t", + "File/Folder", s.ResourcePath, + "IaC Type", s.IacType, + "Scanned At", s.Timestamp, + "Policies Validated", s.TotalPolicies, + "Violated Policies", s.ViolatedPolicies, + "Low", s.LowCount, + "Medium", s.MediumCount, + "High", s.HighCount) + return out +} diff --git a/pkg/writer/yaml_test.go b/pkg/writer/yaml_test.go index 7d92d4aee..27aeece60 100644 --- a/pkg/writer/yaml_test.go +++ b/pkg/writer/yaml_test.go @@ -58,14 +58,15 @@ var ( }, }, Summary: results.ScanSummary{ - ResourcePath: "test", - IacType: "terraform", - Timestamp: "2020-12-12 11:21:29.902796 +0000 UTC", - TotalPolicies: 566, - LowCount: 0, - MediumCount: 0, - HighCount: 1, - ViolatedPolicies: 1, + ResourcePath: "test", + IacType: "terraform", + Timestamp: "2020-12-12 11:21:29.902796 +0000 UTC", + TotalPolicies: 566, + LowCount: 0, + MediumCount: 0, + HighCount: 1, + ViolatedPolicies: 1, + ShowViolationDetails: true, }, }, } From 73a11690a2bd5647ee6e3686265baf422332e385 Mon Sep 17 00:00:00 2001 From: Pankaj Patil Date: Thu, 14 Jan 2021 14:52:09 +0530 Subject: [PATCH 5/6] update regex and unit tests --- pkg/utils/skip_rules.go | 26 +++++------ pkg/utils/skip_rules_test.go | 89 +++++++++++++++++++++++------------- 2 files changed, 69 insertions(+), 46 deletions(-) diff --git a/pkg/utils/skip_rules.go b/pkg/utils/skip_rules.go index 81565606b..cfec2a371 100644 --- a/pkg/utils/skip_rules.go +++ b/pkg/utils/skip_rules.go @@ -24,16 +24,16 @@ import ( ) var ( - skipRulesPattern = regexp.MustCompile(`#ts:skip=\s*(([A-Za-z0-9]+\.?){5}(\s*[|][\w\s]*\.){0,1})(\s*,\s*([A-Za-z0-9]+\.?){5}(\s*[|][\w\s]*\.){0,1})*`) - skipRulesPrefix = "#ts:skip=" - ruleCommentSeparator = "|" + skipRulesPattern = regexp.MustCompile(`(#ts:skip=[ \t]*(([A-Za-z0-9]+[.-]{1}){3,5}([\d]+)){1}([ \t]+.*){0,1})`) + skipRulesPrefix = "#ts:skip=" ) // GetSkipRules returns a list of rules to be skipped. The rules to be skipped -// can be set in terraform resource config with the following comma separated pattern: -// #ts:skip=AWS.S3Bucket.DS.High.1043, AWS.S3Bucket.DS.High.1044 +// can be set in terraform resource config with the following pattern: +// #ts:skip=AWS.S3Bucket.DS.High.1043 +// $ts:skip=AWS.S3Bucket.DS.High.1044 reason to skip the rule +// each rule and its optional comment must be in a new line func GetSkipRules(body string) []output.SkipRule { - var skipRules []output.SkipRule // check if any rules comments are present in body @@ -47,12 +47,9 @@ func GetSkipRules(body string) []output.SkipRule { // extract rule ids from comments for _, c := range comments { c = strings.TrimPrefix(c, skipRulesPrefix) - cmts := strings.Split(c, ",") - for _, v := range cmts { - skipRule := getSkipRuleObject(v) - if skipRule != nil { - skipRules = append(skipRules, *skipRule) - } + skipRule := getSkipRuleObject(c) + if skipRule != nil { + skipRules = append(skipRules, *skipRule) } } return skipRules @@ -63,11 +60,12 @@ func getSkipRuleObject(s string) *output.SkipRule { return nil } var skipRule output.SkipRule - ruleComment := strings.Split(s, ruleCommentSeparator) + ruleComment := strings.Fields(s) skipRule.Rule = strings.TrimSpace(ruleComment[0]) if len(ruleComment) > 1 { - skipRule.Comment = strings.TrimSpace(ruleComment[1]) + comment := strings.Join(ruleComment[1:], " ") + skipRule.Comment = strings.TrimSpace(comment) } return &skipRule } diff --git a/pkg/utils/skip_rules_test.go b/pkg/utils/skip_rules_test.go index fe4e55add..7965238b1 100644 --- a/pkg/utils/skip_rules_test.go +++ b/pkg/utils/skip_rules_test.go @@ -25,8 +25,11 @@ import ( // ---------------------- unit tests -------------------------------- // func TestGetSkipRules(t *testing.T) { - testRule1 := "AWS.S3Bucket.DS.High.1041" - testRule2 := "AWS.S3Bucket.DS.High.1042" + testRuleAWS1 := "AWS.S3Bucket.DS.High.1041" + testRuleAWS2 := "AWS.S3Bucket.DS.High.1042" + testRuleAWSwithHyphen := "AC-AWS-NS-IN-M-1172" + testRuleAzure := "accurics.azure.NS.147" + testRuleKubernetesWithHyphen := "AC-K8-DS-PO-M-0143" table := []struct { name string @@ -39,72 +42,94 @@ func TestGetSkipRules(t *testing.T) { // expected would be an empty slice of output.SkipRule }, { - name: "single rule id with no comment", - input: "#ts:skip=AWS.S3Bucket.DS.High.1041", + name: "rule id with no comment, aws", + input: "#ts:skip=AWS.S3Bucket.DS.High.1041\n", expected: []output.SkipRule{ - {Rule: testRule1}, + {Rule: testRuleAWS1}, }, }, { - name: "single rule id with comment", - input: "#ts:skip=AWS.S3Bucket.DS.High.1041 | this rule should be skipped.", + name: "rule id with no comment, aws, with '-'", + input: "#ts:skip=AC-AWS-NS-IN-M-1172\n", + expected: []output.SkipRule{ + {Rule: testRuleAWSwithHyphen}, + }, + }, + { + // gcp, kubernetes, github rules are of same format + name: "rule id with no comment, azure", + input: "#ts:skip=accurics.azure.NS.147\n", + expected: []output.SkipRule{ + {Rule: testRuleAzure}, + }, + }, + { + name: "rule id with no comment, kubernetes with '-'", + input: "#ts:skip=AC-K8-DS-PO-M-0143\n", + expected: []output.SkipRule{ + {Rule: testRuleKubernetesWithHyphen}, + }, + }, + { + name: "rule id with comment", + input: "#ts:skip=AWS.S3Bucket.DS.High.1041 This rule should be skipped.\n", expected: []output.SkipRule{ { - Rule: testRule1, - Comment: "this rule should be skipped.", + Rule: testRuleAWS1, + Comment: "This rule should be skipped.", }, }, }, { + // should match only one rule, we support single rule and comment in one line + // everything after the first group match will be considered a comment name: "multiple comma separated no space, with comments", - input: "#ts:skip=AWS.S3Bucket.DS.High.1041|some reason to skip. , AWS.S3Bucket.DS.High.1042| should_skip_the_rule.", + input: "#ts:skip=AWS.S3Bucket.DS.High.1041 some reason to skip. , AWS.S3Bucket.DS.High.1042 should_skip_the_rule.\n", expected: []output.SkipRule{ { - Rule: testRule1, - Comment: "some reason to skip.", - }, - { - Rule: testRule2, - Comment: "should_skip_the_rule.", + Rule: testRuleAWS1, + Comment: "some reason to skip. , AWS.S3Bucket.DS.High.1042 should_skip_the_rule.", }, }, }, { - name: "multiple comma separated random space, without comments", - input: "#ts:skip= AWS.S3Bucket.DS.High.1041 , AWS.S3Bucket.DS.High.1042", + name: "rule and comment with random space characters", + input: "#ts:skip= AWS.S3Bucket.DS.High.1041 reason_to skip. the rule\n", expected: []output.SkipRule{ { - Rule: testRule1, - }, - { - Rule: testRule2, + Rule: testRuleAWS1, + Comment: "reason_to skip. the rule", }, }, }, { - name: "sample resource config", - input: "{\n #ts:skip=AWS.S3Bucket.DS.High.1041 | skip the rule. \n region = var.region\n #ts:skip=AWS.S3Bucket.DS.High.1042 AWS.S3Bucket.DS.High.1043\n bucket = local.bucket_name\n #ts:skip=AWS.S3Bucket.DS.High.1044 | resource skipped for this rule. ,AWS.S3Bucket.DS.High.1045\n force_destroy = true\n #ts:skip= AWS.S3Bucket.DS.High.1046 , AWS.S3Bucket.DS.High.1047\n acl = \"public-read\"\n }", + name: "sample resource config", + input: `{ + #ts:skip=AWS.S3Bucket.DS.High.1041 skip the rule. + region = var.region + #ts:skip=AWS.S3Bucket.DS.High.1042 AWS.S3Bucket.DS.High.1043 + bucket = local.bucket_name + #ts:skip=AWS.S3Bucket.DS.High.1044 resource skipped for this rule. + force_destroy = true + #ts:skip= AWS.S3Bucket.DS.High.1046 + acl = "public-read" + }`, expected: []output.SkipRule{ { - Rule: testRule1, + Rule: testRuleAWS1, Comment: "skip the rule.", }, { - Rule: testRule2, + Rule: testRuleAWS2, + Comment: "AWS.S3Bucket.DS.High.1043", }, { Rule: "AWS.S3Bucket.DS.High.1044", Comment: "resource skipped for this rule.", }, - { - Rule: "AWS.S3Bucket.DS.High.1045", - }, { Rule: "AWS.S3Bucket.DS.High.1046", }, - { - Rule: "AWS.S3Bucket.DS.High.1047", - }, }, }, } From 6530dc80c7c39206c534d11b9a2b194d8cd7f1a3 Mon Sep 17 00:00:00 2001 From: Pankaj Patil Date: Thu, 14 Jan 2021 15:13:30 +0530 Subject: [PATCH 6/6] rebase and fix failing test --- pkg/iac-providers/kubernetes/v1/normalize_test.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/pkg/iac-providers/kubernetes/v1/normalize_test.go b/pkg/iac-providers/kubernetes/v1/normalize_test.go index 07699fab3..71b0c5ea5 100644 --- a/pkg/iac-providers/kubernetes/v1/normalize_test.go +++ b/pkg/iac-providers/kubernetes/v1/normalize_test.go @@ -68,7 +68,9 @@ kind: CRD metadata: generateName: myapp-pod-prefix- annotations: - terrascanSkip: [accurics.kubernetes.IAM.109] + terrascanSkip: + - rule: accurics.kubernetes.IAM.109 + comment: reason to skip the rule spec: containers: - name: myapp-container @@ -242,7 +244,7 @@ func TestK8sV1Normalize(t *testing.T) { }, }, }, - SkipRules: []string{testRule}, + SkipRules: []output.SkipRule{testSkipRule}, }, }, { @@ -263,7 +265,10 @@ func TestK8sV1Normalize(t *testing.T) { "kind": "CRD", "metadata": map[string]interface{}{ "annotations": map[string]interface{}{ - terrascanSkip: []interface{}{testRule}, + terrascanSkip: []interface{}{map[string]interface{}{ + "rule": testRule, + "comment": testComment, + }}, }, "generateName": "myapp-pod-prefix-", },