Repository: aquasecurity/tfsec
Pull request #1036 (2021-08-17 23:59:04)

Add check to avoid mixing usage of security_group_rule and security_group with inline rules
## Description

Hello, this adds an AWS check to ensure that usage of security groups with inline rules and security group rule resources are not mixed.

Mixing Terraform standalone `security_group_rule` resource and `security_group` resource with inline ingress/egress rules results in rules being overwritten during Terraform apply. 

This is intended behavior as per [terraform](https://github.com/hashicorp/terraform/issues/11011#issuecomment-283076580) but can result in security group rules being overwritten if one is not careful. Big banner warning from terraform [security group docs](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group) :

<img width="500" alt="Screen Shot 2021-08-18 at 7 58 15 AM" src="https://user-images.githubusercontent.com/87681618/129815604-d6e08f60-3466-45c2-af3e-8ad9a099f297.png">

## Solution

This adds the check in go where we have  access to the `GetReferencedBlock` function. 

I'm not able to express how I can refer to "co-related" resources with the custom check language. Please let me know  if this is achievable, may have just missed it. Thank you 🙏 

## References

* https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group
* https://github.com/hashicorp/terraform/issues/11011#issuecomment-283076580



Patch:
diff --git a/internal/app/tfsec/rules/aws/vpc/disallow_mixed_sgr_rule.go b/internal/app/tfsec/rules/aws/vpc/disallow_mixed_sgr_rule.go
new file mode 100644
index 000000000..ef9e0ffc3
--- /dev/null
+++ b/internal/app/tfsec/rules/aws/vpc/disallow_mixed_sgr_rule.go
@@ -0,0 +1,74 @@
+package vpc
+
+import (
+	"fmt"
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/block"
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/debug"
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/scanner"
+	"github.com/aquasecurity/tfsec/pkg/provider"
+	"github.com/aquasecurity/tfsec/pkg/result"
+	"github.com/aquasecurity/tfsec/pkg/rule"
+	"github.com/aquasecurity/tfsec/pkg/severity"
+)
+
+func init() {
+	scanner.RegisterCheckRule(rule.Rule{
+		Provider:  provider.AWSProvider,
+		Service:   "vpc",
+		ShortCode: "disallow-mixed-sgr",
+		Documentation: rule.RuleDocumentation{
+			Summary:     "Ensures that usage of security groups with inline rules and security group rule resources are not mixed.",
+			Explanation: `
+Mixing Terraform standalone security_group_rule resource and security_group resource with inline ingress/egress rules results in rules being overwritten during Terraform apply.
+`,
+			Impact:      "Security group rules will be overwritten and will result in unintended blocking of network traffic",
+			Resolution:  "Either define all of a security group's rules inline, or none of the security group's rules inline",
+			BadExample: []string{`
+resource "aws_security_group_rule" "bad_example" {
+  	security_group_id = aws_security_group.bad_example_sg.id
+	type = "ingress"
+	cidr_blocks = ["172.31.0.0/16"]
+}
+
+resource "aws_security_group" "bad_example_sg" {
+	ingress {
+		cidr_blocks = ["10.0.0.0/16"]
+	}
+}
+`},
+			GoodExample: []string{`
+resource "aws_security_group_rule" "good_example" {
+  	security_group_id = aws_security_group.good_example_sg.id
+	type = "ingress"
+	cidr_blocks = ["10.0.0.0/16", "172.31.0.0/16"]
+}
+
+resource "aws_security_group" "good_example_sg" {
+}
+`},
+			Links: []string{
+				"https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group#resource-aws_security_group",
+				"https://github.com/hashicorp/terraform/issues/11011#issuecomment-283076580",
+			},
+		},
+		RequiredTypes:  []string{"resource"},
+		RequiredLabels: []string{
+			"aws_security_group_rule",
+		},
+		DefaultSeverity: severity.Low,
+		CheckFunc: func(set result.Set, resourceBlock block.Block, module block.Module) {
+			if resourceBlock.HasChild("security_group_id") {
+				sgAttr := resourceBlock.GetAttribute("security_group_id")
+				referencedBlock, err := module.GetReferencedBlock(sgAttr)
+				if err == nil {
+					if referencedBlock.HasChild("egress") || referencedBlock.HasChild("ingress") {
+						set.AddResult().
+							WithDescription(fmt.Sprintf("Mixed usage between '%s' and '%s'", resourceBlock.FullName(), referencedBlock.FullName()))
+					}
+				} else {
+					debug.Log(err.Error())
+				}
+			}
+		},
+	})
+}


Test patch:
diff --git a/internal/app/tfsec/rules/aws/vpc/disallow_mixed_sgr_rule_test.go b/internal/app/tfsec/rules/aws/vpc/disallow_mixed_sgr_rule_test.go
new file mode 100644
index 000000000..ea74af2e6
--- /dev/null
+++ b/internal/app/tfsec/rules/aws/vpc/disallow_mixed_sgr_rule_test.go
@@ -0,0 +1,93 @@
+package vpc
+
+import (
+	"github.com/aquasecurity/tfsec/internal/app/tfsec/testutil"
+	"testing"
+)
+
+func Test_AWSDisallowMixedSecurityGroupRule(t *testing.T) {
+	expectedCode := "aws-vpc-disallow-mixed-sgr"
+
+	var tests = []struct {
+		name                  string
+		source                string
+		mustIncludeResultCode string
+		mustExcludeResultCode string
+	}{
+		{
+			name: "check mixed aws_security_group_rule and inline egress & ingress on security_group",
+			source: `
+resource "aws_security_group_rule" "my-security-group-rule" {
+  	security_group_id = aws_security_group.my-security-group.id
+	type = "ingress"
+	cidr_blocks = ["172.31.0.0/16"]
+}
+
+resource "aws_security_group" "my-security-group" {
+	ingress {
+		cidr_blocks = ["10.0.0.0/16"]
+	}
+	egress {
+		cidr_blocks = ["0.0.0.0/0"]
+	}
+}`,
+			mustIncludeResultCode: expectedCode,
+		},
+		{
+			name: "check defined in aws_security_group_rule only",
+			source: `
+resource "aws_security_group_rule" "my-security-group-rule" {
+  	security_group_id = aws_security_group.bad_example_sg.id
+	type = "ingress"
+	cidr_blocks = ["172.31.0.0/16", "10.0.0.0/16"]
+}
+
+resource "aws_security_group" "my-security-group" {
+}`,
+			mustExcludeResultCode: expectedCode,
+		},
+		{
+			name: "check defined in aws_security_group only",
+			source: `
+resource "aws_security_group" "my-security-group" {
+	ingress {
+		cidr_blocks = ["10.0.0.0/16"]
+	}
+	egress {
+		cidr_blocks = ["0.0.0.0/0"]
+	}
+}`,
+			mustExcludeResultCode: expectedCode,
+		},
+		{
+			name: "check applicable only on co-related aws_security_group and aws_security_group_rule",
+			source: `
+resource "aws_security_group_rule" "my-security-group-rule" {
+  	security_group_id = aws_security_group.my-security-group-a.id
+	type = "ingress"
+	cidr_blocks = ["172.31.0.0/16"]
+}
+
+resource "aws_security_group" "my-security-group-a" {
+}
+
+resource "aws_security_group" "my-security-group-b" {
+	ingress {
+		cidr_blocks = ["10.0.0.0/16"]
+	}
+	egress {
+		cidr_blocks = ["0.0.0.0/0"]
+	}
+}`,
+			mustExcludeResultCode: expectedCode,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+
+			results := testutil.ScanHCL(test.source, t)
+			testutil.AssertCheckCode(t, test.mustIncludeResultCode, test.mustExcludeResultCode, results)
+		})
+	}
+}