fix(aws-ec2): SecurityGroups ensure outbound rules

Make "allowAllOutbound" traffic a property of a SecurityGroup. By making
the SecurityGroup aware of this configuration property, we can make sure
that future egress rules don't get added to the SecurityGroup. There's
no need to, because all traffic is allowed by default anyway.

If we don't do this, our behavior of adding reciprocal rules between
SecurityGroups conflicts with CloudFormation's behavior to strip the
"default outbound" rule when another rule gets added, and it becomes
impossible to configure "all outbound" traffic on a
SecurityGroup.

Fixes #987.

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-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' });