Merge pull request #1765 from hashicorp/f-aws-security-group-remove-default-egress

provider/aws: Remove default egress rule from Security Group on creation

builtin/providers/aws/resource_aws_network_interface_test.go

@@ -181,6 +181,13 @@ resource "aws_security_group" "foo" {
   vpc_id = "${aws_vpc.foo.id}"
   description = "foo"
   name = "foo"  
+
+        egress {
+                from_port = 0
+                to_port = 0
+                protocol = "tcp"
+                cidr_blocks = ["10.0.0.0/16"]
+        }
 }
 
 resource "aws_network_interface" "bar" {

builtin/providers/aws/resource_aws_security_group.go

@@ -185,12 +185,44 @@ func resourceAwsSecurityGroupCreate(d *schema.ResourceData, meta interface{}) er
 		Refresh: SGStateRefreshFunc(conn, d.Id()),
 		Timeout: 1 * time.Minute,
 	}
-	if _, err := stateConf.WaitForState(); err != nil {
+
+	resp, err := stateConf.WaitForState()
+	if err != nil {
 		return fmt.Errorf(
 			"Error waiting for Security Group (%s) to become available: %s",
 			d.Id(), err)
 	}
 
+	// AWS defaults all Security Groups to have an ALLOW ALL egress rule. Here we
+	// revoke that rule, so users don't unknowningly have/use it.
+	group := resp.(*ec2.SecurityGroup)
+	if group.VPCID != nil && *group.VPCID != "" {
+		log.Printf("[DEBUG] Revoking default egress rule for Security Group for %s", d.Id())
+
+		req := &ec2.RevokeSecurityGroupEgressInput{
+			GroupID: createResp.GroupID,
+			IPPermissions: []*ec2.IPPermission{
+				&ec2.IPPermission{
+					FromPort: aws.Long(int64(0)),
+					ToPort:   aws.Long(int64(0)),
+					IPRanges: []*ec2.IPRange{
+						&ec2.IPRange{
+							CIDRIP: aws.String("0.0.0.0/0"),
+						},
+					},
+					IPProtocol: aws.String("-1"),
+				},
+			},
+		}
+
+		if _, err = conn.RevokeSecurityGroupEgress(req); err != nil {
+			return fmt.Errorf(
+				"Error revoking default egress rule for Security Group (%s): %s",
+				d.Id(), err)
+		}
+
+	}
+
 	return resourceAwsSecurityGroupUpdate(d, meta)
 }
 

builtin/providers/aws/resource_aws_security_group_test.go

@@ -214,6 +214,40 @@ func TestAccAWSSecurityGroup_generatedName(t *testing.T) {
 	})
 }
 
+func TestAccAWSSecurityGroup_DefaultEgress(t *testing.T) {
+
+        // VPC
+        resource.Test(t, resource.TestCase{
+                PreCheck:     func() { testAccPreCheck(t) },
+                Providers:    testAccProviders,
+                CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
+                Steps: []resource.TestStep{
+                        resource.TestStep{
+                                Config: testAccAWSSecurityGroupConfigDefaultEgress,
+                                Check: resource.ComposeTestCheckFunc(
+                                        testAccCheckAWSSecurityGroupExistsWithoutDefault("aws_security_group.worker"),
+                                ),
+                        },
+                },
+        })
+
+        // Classic
+        var group ec2.SecurityGroup
+        resource.Test(t, resource.TestCase{
+                PreCheck:     func() { testAccPreCheck(t) },
+                Providers:    testAccProviders,
+                CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
+                Steps: []resource.TestStep{
+                        resource.TestStep{
+                                Config: testAccAWSSecurityGroupConfigClassic,
+                                Check: resource.ComposeTestCheckFunc(
+                                        testAccCheckAWSSecurityGroupExists("aws_security_group.web", &group),
+                                ),
+                        },
+                },
+        })
+}
+
 func testAccCheckAWSSecurityGroupDestroy(s *terraform.State) error {
 	conn := testAccProvider.Meta().(*AWSClient).ec2conn
 
@@ -394,6 +428,38 @@ func testAccCheckAWSSecurityGroupAttributesChanged(group *ec2.SecurityGroup) res
 	}
 }
 
+func testAccCheckAWSSecurityGroupExistsWithoutDefault(n string) resource.TestCheckFunc {
+        return func(s *terraform.State) error {
+                rs, ok := s.RootModule().Resources[n]
+                if !ok {
+                        return fmt.Errorf("Not found: %s", n)
+                }
+
+                if rs.Primary.ID == "" {
+                        return fmt.Errorf("No Security Group is set")
+                }
+
+                conn := testAccProvider.Meta().(*AWSClient).ec2conn
+                req := &ec2.DescribeSecurityGroupsInput{
+                        GroupIDs: []*string{aws.String(rs.Primary.ID)},
+                }
+                resp, err := conn.DescribeSecurityGroups(req)
+                if err != nil {
+                        return err
+                }
+
+                if len(resp.SecurityGroups) > 0 && *resp.SecurityGroups[0].GroupID == rs.Primary.ID {
+                        group := *resp.SecurityGroups[0]
+
+                        if len(group.IPPermissionsEgress) != 1 {
+                                return fmt.Errorf("Security Group should have only 1 egress rule, got %d", len(group.IPPermissionsEgress))
+                        }
+                }
+
+                return nil
+        }
+}
+
 const testAccAWSSecurityGroupConfig = `
 resource "aws_security_group" "web" {
   name = "terraform_acceptance_test_example"
@@ -406,6 +472,13 @@ resource "aws_security_group" "web" {
     cidr_blocks = ["10.0.0.0/8"]
   }
 
+  egress {
+    protocol = "tcp"
+    from_port = 80
+    to_port = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
+
 	tags {
 		Name = "tf-acc-test"
 	}
@@ -430,6 +503,13 @@ resource "aws_security_group" "web" {
     to_port = 8000
     cidr_blocks = ["0.0.0.0/0", "10.0.0.0/8"]
   }
+
+  egress {
+    protocol = "tcp"
+    from_port = 80
+    to_port = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
 }
 `
 
@@ -444,6 +524,13 @@ resource "aws_security_group" "web" {
     to_port = 8000
     self = true
   }
+
+  egress {
+    protocol = "tcp"
+    from_port = 80
+    to_port = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
 }
 `
 
@@ -484,6 +571,13 @@ resource "aws_security_group" "worker" {
     to_port = 8000
     cidr_blocks = ["10.0.0.0/8"]
   }
+
+  egress {
+    protocol = "tcp"
+    from_port = 80
+    to_port = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
 }
 
 resource "aws_security_group" "web" {
@@ -510,6 +604,13 @@ resource "aws_security_group" "web" {
     to_port = 8000
     security_groups = ["${aws_security_group.worker.id}"]
   }
+
+  egress {
+    protocol = "tcp"
+    from_port = 80
+    to_port = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
 }
 `
 
@@ -525,6 +626,13 @@ resource "aws_security_group" "foo" {
     cidr_blocks = ["10.0.0.0/8"]
   }
 
+  egress {
+    protocol = "tcp"
+    from_port = 80
+    to_port = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
+
   tags {
     foo = "bar"
   }
@@ -543,6 +651,13 @@ resource "aws_security_group" "foo" {
     cidr_blocks = ["10.0.0.0/8"]
   }
 
+  egress {
+    protocol = "tcp"
+    from_port = 80
+    to_port = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
+
   tags {
     bar = "baz"
   }
@@ -558,8 +673,48 @@ resource "aws_security_group" "web" {
     cidr_blocks = ["10.0.0.0/8"]
   }
 
+  egress {
+    protocol = "tcp"
+    from_port = 80
+    to_port = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
+
 	tags {
 		Name = "tf-acc-test"
 	}
 }
 `
+
+const testAccAWSSecurityGroupConfigDefaultEgress = `
+resource "aws_vpc" "tf_sg_egress_test" {
+        cidr_block = "10.0.0.0/16"
+        tags {
+                Name = "tf_sg_egress_test"
+        }
+}
+
+resource "aws_security_group" "worker" {
+  name = "terraform_acceptance_test_example_1"
+  description = "Used in the terraform acceptance tests"
+        vpc_id = "${aws_vpc.tf_sg_egress_test.id}"
+
+  egress {
+    protocol = "tcp"
+    from_port = 80
+    to_port = 8000
+    cidr_blocks = ["10.0.0.0/8"]
+  }
+}
+`
+
+const testAccAWSSecurityGroupConfigClassic = `
+provider "aws" {
+  region = "us-east-1"
+}
+
+resource "aws_security_group" "web" {
+  name = "terraform_acceptance_test_example_1"
+  description = "Used in the terraform acceptance tests"
+}
+`

website/source/docs/providers/aws/r/security_group.html.markdown

@@ -63,9 +63,8 @@ The following arguments are supported:
 * `description` - (Required) The security group description.
 * `ingress` - (Optional) Can be specified multiple times for each
    ingress rule. Each ingress block supports fields documented below.
-* `egress` - (Optional) Can be specified multiple times for each
+* `egress` - (Optional, VPC only) Can be specified multiple times for each
       egress rule. Each egress block supports fields documented below.
-      VPC only.
 * `vpc_id` - (Optional) The VPC ID.
 * `tags` - (Optional) A mapping of tags to assign to the resource.
 
@@ -93,6 +92,20 @@ The `egress` block supports:
      a source to this egress rule.
 * `to_port` - (Required) The end range port.
 
+~> **NOTE on Egress rules:** By default, AWS creates an `ALLOW ALL` egress rule when creating a
+new Security Group inside of a VPC. When creating a new Security
+Group inside a VPC, **Terraform will remove this default rule**, and require you
+specifically re-create it if you desire that rule. We feel this leads to fewer
+surprises in terms of controlling your egress rules. If you desire this rule to
+be in place, you can use this `egress` block:
+
+    egress {
+      from_port = 0
+      to_port = 0
+      protocol = "-1"
+      cidr_block = "0.0.0.0/0"
+    }
+
 ## Attributes Reference
 
 The following attributes are exported: