From 31183e10f6031f10974a7d093abc1ad4f2b1b2fb Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Fri, 9 Nov 2018 19:45:55 -0500 Subject: [PATCH] resource/aws_security_group_rule: Prevent crash when reading rules from groups containing an ALL/-1 protocol rule The rule searching logic was missing `nil` checks. This crash was only triggered under the context of other rules existing in the group having `FromPort` and `ToPort` in comparison to the local rule not having `FromPort` and `ToPort`. This also prevents an errant difference when `to_port` is specified as 65535 in an ALL/-1 protocol rule, which is allowed by the API and previously supported in Terraform without this difference. ``` --- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidTypeError (1.53s) --- PASS: TestAccAWSSecurityGroupRule_ExpectInvalidCIDR (1.70s) --- PASS: TestAccAWSSecurityGroupRule_Ingress_VPC (22.23s) --- PASS: TestAccAWSSecurityGroupRule_EgressDescription (21.89s) --- PASS: TestAccAWSSecurityGroupRule_Egress (25.56s) --- PASS: TestAccAWSSecurityGroupRule_MultipleRuleSearching_AllProtocolCrash (26.06s) --- PASS: TestAccAWSSecurityGroupRule_Issue5310 (26.90s) --- PASS: TestAccAWSSecurityGroupRule_Ingress_Classic (27.31s) --- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts (34.32s) --- PASS: TestAccAWSSecurityGroupRule_Description_AllPorts_ToPort65535 (36.12s) --- PASS: TestAccAWSSecurityGroupRule_IngressDescription_updates (35.41s) --- PASS: TestAccAWSSecurityGroupRule_EgressDescription_updates (37.68s) --- PASS: TestAccAWSSecurityGroupRule_IngressDescription (39.34s) --- PASS: TestAccAWSSecurityGroupRule_Ingress_Ipv6 (41.93s) --- PASS: TestAccAWSSecurityGroupRule_Ingress_Protocol (42.58s) --- PASS: TestAccAWSSecurityGroupRule_SelfReference (43.87s) --- PASS: TestAccAWSSecurityGroupRule_SelfSource (45.94s) --- PASS: TestAccAWSSecurityGroupRule_MultiIngress (25.31s) --- PASS: TestAccAWSSecurityGroupRule_PartialMatching_Source (48.62s) --- PASS: TestAccAWSSecurityGroupRule_PartialMatching_basic (51.84s) --- PASS: TestAccAWSSecurityGroupRule_PrefixListEgress (57.50s) --- PASS: TestAccAWSSecurityGroupRule_MultiDescription (88.64s) --- PASS: TestAccAWSSecurityGroupRule_Race (274.35s) ``` --- aws/resource_aws_security_group_rule.go | 28 ++- aws/resource_aws_security_group_rule_test.go | 175 ++++++++++++++++++- 2 files changed, 197 insertions(+), 6 deletions(-) diff --git a/aws/resource_aws_security_group_rule.go b/aws/resource_aws_security_group_rule.go index 6784ff9940a7..eea278f9f377 100644 --- a/aws/resource_aws_security_group_rule.go +++ b/aws/resource_aws_security_group_rule.go @@ -62,6 +62,13 @@ func resourceAwsSecurityGroupRule() *schema.Resource { Type: schema.TypeInt, Required: true, ForceNew: true, + DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool { + protocol := protocolForValue(d.Get("protocol").(string)) + if protocol == "-1" && old == "0" && new == "65535" { + return true + } + return false + }, }, "protocol": { @@ -430,21 +437,24 @@ func (b ByGroupPair) Less(i, j int) bool { func findRuleMatch(p *ec2.IpPermission, rules []*ec2.IpPermission, isVPC bool) *ec2.IpPermission { var rule *ec2.IpPermission for _, r := range rules { - if r.ToPort != nil && *p.ToPort != *r.ToPort { + if p.ToPort != nil && r.ToPort != nil && *p.ToPort != *r.ToPort { continue } - if r.FromPort != nil && *p.FromPort != *r.FromPort { + if p.FromPort != nil && r.FromPort != nil && *p.FromPort != *r.FromPort { continue } - if r.IpProtocol != nil && *p.IpProtocol != *r.IpProtocol { + if p.IpProtocol != nil && r.IpProtocol != nil && *p.IpProtocol != *r.IpProtocol { continue } remaining := len(p.IpRanges) for _, ip := range p.IpRanges { for _, rip := range r.IpRanges { + if ip.CidrIp == nil || rip.CidrIp == nil { + continue + } if *ip.CidrIp == *rip.CidrIp { remaining-- } @@ -458,6 +468,9 @@ func findRuleMatch(p *ec2.IpPermission, rules []*ec2.IpPermission, isVPC bool) * remaining = len(p.Ipv6Ranges) for _, ipv6 := range p.Ipv6Ranges { for _, ipv6ip := range r.Ipv6Ranges { + if ipv6.CidrIpv6 == nil || ipv6ip.CidrIpv6 == nil { + continue + } if *ipv6.CidrIpv6 == *ipv6ip.CidrIpv6 { remaining-- } @@ -471,6 +484,9 @@ func findRuleMatch(p *ec2.IpPermission, rules []*ec2.IpPermission, isVPC bool) * remaining = len(p.PrefixListIds) for _, pl := range p.PrefixListIds { for _, rpl := range r.PrefixListIds { + if pl.PrefixListId == nil || rpl.PrefixListId == nil { + continue + } if *pl.PrefixListId == *rpl.PrefixListId { remaining-- } @@ -485,10 +501,16 @@ func findRuleMatch(p *ec2.IpPermission, rules []*ec2.IpPermission, isVPC bool) * for _, ip := range p.UserIdGroupPairs { for _, rip := range r.UserIdGroupPairs { if isVPC { + if ip.GroupId == nil || rip.GroupId == nil { + continue + } if *ip.GroupId == *rip.GroupId { remaining-- } } else { + if ip.GroupName == nil || rip.GroupName == nil { + continue + } if *ip.GroupName == *rip.GroupName { remaining-- } diff --git a/aws/resource_aws_security_group_rule_test.go b/aws/resource_aws_security_group_rule_test.go index 6accf4d39682..d470a3c47a57 100644 --- a/aws/resource_aws_security_group_rule_test.go +++ b/aws/resource_aws_security_group_rule_test.go @@ -843,6 +843,111 @@ func TestAccAWSSecurityGroupRule_Description_AllPorts(t *testing.T) { }) } +// Reference: https://github.com/terraform-providers/terraform-provider-aws/issues/6416 +func TestAccAWSSecurityGroupRule_Description_AllPorts_ToPort65535(t *testing.T) { + var group ec2.SecurityGroup + rName := acctest.RandomWithPrefix("tf-acc-test") + securityGroupResourceName := "aws_security_group.test" + resourceName := "aws_security_group_rule.test" + + rule1 := ec2.IpPermission{ + IpProtocol: aws.String("-1"), + IpRanges: []*ec2.IpRange{ + {CidrIp: aws.String("0.0.0.0/0"), Description: aws.String("description1")}, + }, + } + + rule2 := ec2.IpPermission{ + IpProtocol: aws.String("-1"), + IpRanges: []*ec2.IpRange{ + {CidrIp: aws.String("0.0.0.0/0"), Description: aws.String("description2")}, + }, + } + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSSecurityGroupRuleDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSSecurityGroupRuleConfigDescriptionAllPortsToPort65535(rName, "description1"), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSSecurityGroupRuleExists(securityGroupResourceName, &group), + testAccCheckAWSSecurityGroupRuleAttributes(resourceName, &group, &rule1, "ingress"), + resource.TestCheckResourceAttr(resourceName, "description", "description1"), + resource.TestCheckResourceAttr(resourceName, "from_port", "0"), + resource.TestCheckResourceAttr(resourceName, "protocol", "-1"), + resource.TestCheckResourceAttr(resourceName, "to_port", "65535"), + ), + }, + { + ResourceName: resourceName, + ImportState: true, + ImportStateIdFunc: testAccAWSSecurityGroupRuleImportStateIdFunc(resourceName), + ImportStateVerify: true, + }, + { + Config: testAccAWSSecurityGroupRuleConfigDescriptionAllPorts(rName, "description2"), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSSecurityGroupRuleExists(securityGroupResourceName, &group), + testAccCheckAWSSecurityGroupRuleAttributes(resourceName, &group, &rule2, "ingress"), + resource.TestCheckResourceAttr(resourceName, "description", "description2"), + resource.TestCheckResourceAttr(resourceName, "from_port", "0"), + resource.TestCheckResourceAttr(resourceName, "protocol", "-1"), + resource.TestCheckResourceAttr(resourceName, "to_port", "0"), + ), + }, + }, + }) +} + +// Reference: https://github.com/terraform-providers/terraform-provider-aws/issues/6416 +func TestAccAWSSecurityGroupRule_MultipleRuleSearching_AllProtocolCrash(t *testing.T) { + var group ec2.SecurityGroup + rName := acctest.RandomWithPrefix("tf-acc-test") + securityGroupResourceName := "aws_security_group.test" + resourceName1 := "aws_security_group_rule.test1" + resourceName2 := "aws_security_group_rule.test2" + + rule1 := ec2.IpPermission{ + IpProtocol: aws.String("-1"), + IpRanges: []*ec2.IpRange{ + {CidrIp: aws.String("10.0.0.0/8")}, + }, + } + + rule2 := ec2.IpPermission{ + FromPort: aws.Int64(443), + ToPort: aws.Int64(443), + IpProtocol: aws.String("tcp"), + IpRanges: []*ec2.IpRange{ + {CidrIp: aws.String("172.168.0.0/16")}, + }, + } + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSSecurityGroupRuleDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSSecurityGroupRuleConfigMultipleRuleSearchingAllProtocolCrash(rName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSSecurityGroupRuleExists(securityGroupResourceName, &group), + testAccCheckAWSSecurityGroupRuleAttributes(resourceName1, &group, &rule1, "ingress"), + testAccCheckAWSSecurityGroupRuleAttributes(resourceName2, &group, &rule2, "ingress"), + resource.TestCheckResourceAttr(resourceName1, "from_port", "0"), + resource.TestCheckResourceAttr(resourceName1, "protocol", "-1"), + resource.TestCheckResourceAttr(resourceName1, "to_port", "65535"), + resource.TestCheckResourceAttr(resourceName2, "from_port", "443"), + resource.TestCheckResourceAttr(resourceName2, "protocol", "tcp"), + resource.TestCheckResourceAttr(resourceName2, "to_port", "443"), + ), + }, + }, + }) +} + func TestAccAWSSecurityGroupRule_MultiDescription(t *testing.T) { var group ec2.SecurityGroup var nat ec2.SecurityGroup @@ -1114,21 +1219,24 @@ func testAccCheckAWSSecurityGroupRuleAttributes(n string, group *ec2.SecurityGro } for _, r := range rules { - if r.ToPort != nil && *p.ToPort != *r.ToPort { + if p.ToPort != nil && r.ToPort != nil && *p.ToPort != *r.ToPort { continue } - if r.FromPort != nil && *p.FromPort != *r.FromPort { + if p.FromPort != nil && r.FromPort != nil && *p.FromPort != *r.FromPort { continue } - if r.IpProtocol != nil && *p.IpProtocol != *r.IpProtocol { + if p.IpProtocol != nil && r.IpProtocol != nil && *p.IpProtocol != *r.IpProtocol { continue } remaining := len(p.IpRanges) for _, ip := range p.IpRanges { for _, rip := range r.IpRanges { + if ip.CidrIp == nil || rip.CidrIp == nil { + continue + } if *ip.CidrIp == *rip.CidrIp { remaining-- } @@ -1142,6 +1250,9 @@ func testAccCheckAWSSecurityGroupRuleAttributes(n string, group *ec2.SecurityGro remaining = len(p.Ipv6Ranges) for _, ip := range p.Ipv6Ranges { for _, rip := range r.Ipv6Ranges { + if ip.CidrIpv6 == nil || rip.CidrIpv6 == nil { + continue + } if *ip.CidrIpv6 == *rip.CidrIpv6 { remaining-- } @@ -1155,6 +1266,9 @@ func testAccCheckAWSSecurityGroupRuleAttributes(n string, group *ec2.SecurityGro remaining = len(p.UserIdGroupPairs) for _, ip := range p.UserIdGroupPairs { for _, rip := range r.UserIdGroupPairs { + if ip.GroupId == nil || rip.GroupId == nil { + continue + } if *ip.GroupId == *rip.GroupId { remaining-- } @@ -1168,6 +1282,9 @@ func testAccCheckAWSSecurityGroupRuleAttributes(n string, group *ec2.SecurityGro remaining = len(p.PrefixListIds) for _, pip := range p.PrefixListIds { for _, rpip := range r.PrefixListIds { + if pip.PrefixListId == nil || rpip.PrefixListId == nil { + continue + } if *pip.PrefixListId == *rpip.PrefixListId { remaining-- } @@ -1805,6 +1922,58 @@ resource "aws_security_group_rule" "test" { `, rName, description) } +func testAccAWSSecurityGroupRuleConfigDescriptionAllPortsToPort65535(rName, description string) string { + return fmt.Sprintf(` +resource "aws_security_group" "test" { + name = %q + + tags { + Name = "tf-acc-test-ec2-security-group-rule" + } +} + +resource "aws_security_group_rule" "test" { + cidr_blocks = ["0.0.0.0/0"] + description = %q + from_port = 0 + protocol = -1 + security_group_id = "${aws_security_group.test.id}" + to_port = 65535 + type = "ingress" +} +`, rName, description) +} + +func testAccAWSSecurityGroupRuleConfigMultipleRuleSearchingAllProtocolCrash(rName string) string { + return fmt.Sprintf(` +resource "aws_security_group" "test" { + name = %q + + tags { + Name = "tf-acc-test-ec2-security-group-rule" + } +} + +resource "aws_security_group_rule" "test1" { + cidr_blocks = ["10.0.0.0/8"] + from_port = 0 + protocol = -1 + security_group_id = "${aws_security_group.test.id}" + to_port = 65535 + type = "ingress" +} + +resource "aws_security_group_rule" "test2" { + cidr_blocks = ["172.168.0.0/16"] + from_port = 443 + protocol = "tcp" + security_group_id = "${aws_security_group.test.id}" + to_port = 443 + type = "ingress" +} +`, rName) +} + var testAccAWSSecurityGroupRuleRace = func() string { var b bytes.Buffer iterations := 50