fix(aws-ec2): fix SecurityGroup's all egress traffic rule

packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts

@@ -179,16 +179,15 @@ export class AutoScalingGroup extends cdk.Construct implements cdk.ITaggable, el
   constructor(parent: cdk.Construct, name: string, props: AutoScalingGroupProps) {
     super(parent, name);
 
-    this.securityGroup = new ec2.SecurityGroup(this, 'InstanceSecurityGroup', { vpc: props.vpc });
+    this.securityGroup = new ec2.SecurityGroup(this, 'InstanceSecurityGroup', {
+      vpc: props.vpc,
+      allowAllOutbound: props.allowAllOutbound !== false
+    });
     this.connections = new ec2.Connections({ securityGroup: this.securityGroup });
     this.securityGroups.push(this.securityGroup);
     this.tags = new TagManager(this, {initialTags: props.tags});
     this.tags.setTag(NAME_TAG, this.path, { overwrite: false });
 
-    if (props.allowAllOutbound !== false) {
-      this.connections.allowTo(new ec2.AnyIPv4(), new ec2.AllConnections(), 'Outbound traffic allowed by default');
-    }
-
     this.role = new iam.Role(this, 'InstanceRole', {
       assumedBy: new iam.ServicePrincipal('ec2.amazonaws.com')
     });

packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts

@@ -27,10 +27,8 @@ export = {
             "SecurityGroupEgress": [
               {
                 "CidrIp": "0.0.0.0/0",
-                "Description": "Outbound traffic allowed by default",
-                "FromPort": -1,
+                "Description": "Allow all outbound traffic by default",
                 "IpProtocol": "-1",
-                "ToPort": -1
               }
             ],
             "SecurityGroupIngress": [],

packages/@aws-cdk/aws-ec2/lib/security-group.ts

@@ -115,6 +115,17 @@ export interface SecurityGroupProps {
    * The VPC in which to create the security group.
    */
   vpc: VpcNetworkRef;
+
+  /**
+   * Whether to allow all outbound traffic by default.
+   *
+   * If this is set to true, there will only be a single egress rule which allows all
+   * outbound traffic. If this is set to false, no outbound traffic will be allowed by
+   * default and all egress traffic must be explicitly authorized.
+   *
+   * @default false
+   */
+  allowAllOutbound?: boolean;
 }
 
 /**
@@ -149,11 +160,16 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable {
   private readonly directIngressRules: cloudformation.SecurityGroupResource.IngressProperty[] = [];
   private readonly directEgressRules: cloudformation.SecurityGroupResource.EgressProperty[] = [];
 
+  private readonly allowAllOutbound: boolean;
+
   constructor(parent: Construct, name: string, props: SecurityGroupProps) {
     super(parent, name);
 
     this.tags = new TagManager(this, { initialTags: props.tags});
     const groupDescription = props.description || this.path;
+
+    this.allowAllOutbound = props.allowAllOutbound || false;
+
     this.securityGroup = new cloudformation.SecurityGroupResource(this, 'Resource', {
       groupName: props.groupName,
       groupDescription,
@@ -166,6 +182,8 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable {
     this.securityGroupId = this.securityGroup.securityGroupId;
     this.groupName = this.securityGroup.securityGroupName;
     this.vpcId = this.securityGroup.securityGroupVpcId;
+
+    this.addDefaultEgressRule();
   }
 
   public addIngressRule(peer: ISecurityGroupRule, connection: IPortRange, description?: string) {
@@ -186,6 +204,18 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable {
   }
 
   public addEgressRule(peer: ISecurityGroupRule, connection: IPortRange, description?: string) {
+    if (this.allowAllOutbound) {
+      // In the case of "allowAllOutbound", we don't add any more rules. There
+      // is only one rule which allows all traffic and that subsumes any other
+      // rule.
+      return;
+    } else {
+      // Otherwise, if the bogus rule exists we can now remove it because the
+      // presence of any other rule will get rid of EC2's implicit "all
+      // outbound" rule anyway.
+      this.removeBogusRule();
+    }
+
     if (!peer.canInlineRule || !connection.canInlineRule) {
       super.addEgressRule(peer, connection, description);
       return;
@@ -233,8 +263,66 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable {
   private hasEgressRule(rule: cloudformation.SecurityGroupResource.EgressProperty): boolean {
     return this.directEgressRules.findIndex(r => egressRulesEqual(r, rule)) > -1;
   }
+
+  /**
+   * Add the default egress rule to the securityGroup
+   *
+   * This depends on allowAllOutbound:
+   *
+   * - If allowAllOutbound is true, we *TECHNICALLY* don't need to do anything, because
+   *   EC2 is going to create this default rule anyway. But, for maximum readability
+   *   of the template, we will add one anyway.
+   * - If allowAllOutbound is false, we add a bogus rule that matches no traffic in
+   *   order to get rid of the default "all outbound" rule that EC2 creates by default.
+   *   If other rules happen to get added later, we remove the bogus rule again so
+   *   that it doesn't clutter up the template too much (even though that's not
+   *   strictly necessary).
+   */
+  private addDefaultEgressRule() {
+    if (this.allowAllOutbound) {
+      this.directEgressRules.push(ALLOW_ALL_RULE);
+    } else {
+      this.directEgressRules.push(BOGUS_RULE);
+    }
+  }
+
+  /**
+   * Remove the bogus rule if it exists
+   */
+  private removeBogusRule() {
+    const i = this.directEgressRules.findIndex(r => egressRulesEqual(r, BOGUS_RULE));
+    if (i > -1) {
+      this.directEgressRules.splice(i, 1);
+    }
+  }
 }
 
+/**
+ * Egress rule that purposely matches no traffic
+ *
+ * This is used in order to disable the "all traffic" default of Security Groups.
+ *
+ * No machine can ever actually have the 255.255.255.255 IP address, but
+ * in order to lock it down even more we'll restrict to a nonexistent
+ * ICMP traffic type.
+ */
+const BOGUS_RULE = {
+  cidrIp: '255.255.255.255/32',
+  description: 'Disallow all traffic',
+  ipProtocol: 'icmp',
+  fromPort: 252,
+  toPort: 86
+};
+
+/**
+ * Egress rule that matches all traffic
+ */
+const ALLOW_ALL_RULE = {
+  cidrIp: '0.0.0.0/0',
+  description: 'Allow all outbound traffic by default',
+  ipProtocol: '-1',
+};
+
 export interface ConnectionRule {
   /**
    * The IP protocol name (tcp, udp, icmp) or number (see Protocol Numbers).

packages/@aws-cdk/aws-ec2/test/test.connections.ts

@@ -25,6 +25,100 @@ import {
 } from "../lib";
 
 export = {
+  'security group can allows all outbound traffic by default'(test: Test) {
+    // GIVEN
+    const stack = new Stack();
+    const vpc = new VpcNetwork(stack, 'VPC');
+
+    // WHEN
+    new SecurityGroup(stack, 'SG1', { vpc, allowAllOutbound: true });
+
+    // THEN
+    expect(stack).to(haveResource('AWS::EC2::SecurityGroup', {
+      SecurityGroupEgress: [
+        {
+          CidrIp: "0.0.0.0/0",
+          Description: "Allow all outbound traffic by default",
+          IpProtocol: "-1"
+        }
+      ],
+    }));
+
+    test.done();
+  },
+
+  'no new outbound rule is added if we are allowing all traffic anyway'(test: Test) {
+    // GIVEN
+    const stack = new Stack();
+    const vpc = new VpcNetwork(stack, 'VPC');
+
+    // WHEN
+    const sg = new SecurityGroup(stack, 'SG1', { vpc, allowAllOutbound: true });
+    sg.addEgressRule(new AnyIPv4(), new TcpPort(86), 'This does not show up');
+
+    // THEN
+    expect(stack).to(haveResource('AWS::EC2::SecurityGroup', {
+      SecurityGroupEgress: [
+        {
+          CidrIp: "0.0.0.0/0",
+          Description: "Allow all outbound traffic by default",
+          IpProtocol: "-1"
+        },
+      ],
+    }));
+
+    test.done();
+  },
+
+  'security group disallow outbound traffic by default'(test: Test) {
+    // GIVEN
+    const stack = new Stack();
+    const vpc = new VpcNetwork(stack, 'VPC');
+
+    // WHEN
+    new SecurityGroup(stack, 'SG1', { vpc, allowAllOutbound: false });
+
+    // THEN
+    expect(stack).to(haveResource('AWS::EC2::SecurityGroup', {
+      SecurityGroupEgress: [
+        {
+          CidrIp: "255.255.255.255/32",
+          Description: "Disallow all traffic",
+          FromPort: 252,
+          IpProtocol: "icmp",
+          ToPort: 86
+        }
+      ],
+    }));
+
+    test.done();
+  },
+
+  'bogus outbound rule disappears if another rule is added'(test: Test) {
+    // GIVEN
+    const stack = new Stack();
+    const vpc = new VpcNetwork(stack, 'VPC');
+
+    // WHEN
+    const sg = new SecurityGroup(stack, 'SG1', { vpc });
+    sg.addEgressRule(new AnyIPv4(), new TcpPort(86), 'This replaces the other one');
+
+    // THEN
+    expect(stack).to(haveResource('AWS::EC2::SecurityGroup', {
+      SecurityGroupEgress: [
+        {
+          CidrIp: "0.0.0.0/0",
+          Description: "This replaces the other one",
+          FromPort: 86,
+          IpProtocol: "tcp",
+          ToPort: 86
+        }
+      ],
+    }));
+
+    test.done();
+  },
+
   'peering between two security groups does not recursive infinitely'(test: Test) {
     // GIVEN
     const stack = new Stack(undefined, 'TestStack', { env: { account: '12345678', region: 'dummy' }});
@@ -47,7 +141,7 @@ export = {
     // GIVEN
     const stack = new Stack();
     const vpc = new VpcNetwork(stack, 'VPC');
-    const sg1 = new SecurityGroup(stack, 'SomeSecurityGroup', { vpc });
+    const sg1 = new SecurityGroup(stack, 'SomeSecurityGroup', { vpc, allowAllOutbound: false });
     const somethingConnectable = new SomethingConnectable(new Connections({ securityGroup: sg1 }));
 
     const securityGroup = SecurityGroupRef.import(stack, 'ImportedSG', { securityGroupId: 'sg-12345' });