fix(aws-ec2): fix retention of all egress traffic rule (#998)

Fix the issue where "all outbound traffic allowed" rules would be
overwritten if any other egress rules are added to the Security Group.

To solve this, we make `allowAllOutbound` a property of a SecurityGroup
(defaults to `true`). 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, and adding them 
would only make CloudFormation delete the "all traffic allowed" rule.

Also update documentation on Security Groups in the `README`.

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-autoscaling/test/integ.asg-w-classic-loadbalancer.expected.json

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

packages/@aws-cdk/aws-autoscaling/test/integ.asg-w-elbv2.expected.json

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

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-codedeploy/test/integ.deployment-group.expected.json

@@ -446,10 +446,8 @@
         "SecurityGroupEgress": [
           {
             "CidrIp": "0.0.0.0/0",
-            "Description": "Outbound traffic allowed by default",
-            "FromPort": -1,
-            "IpProtocol": "-1",
-            "ToPort": -1
+            "Description": "Allow all outbound traffic by default",
+            "IpProtocol": "-1"
           }
         ],
         "SecurityGroupIngress": [],
@@ -626,7 +624,15 @@
       "Type": "AWS::EC2::SecurityGroup",
       "Properties": {
         "GroupDescription": "aws-cdk-codedeploy-server-dg/ELB/SecurityGroup",
-        "SecurityGroupEgress": [],
+        "SecurityGroupEgress": [
+          {
+            "CidrIp":"255.255.255.255/32",
+            "Description":"Disallow all traffic",
+            "FromPort":252,
+            "IpProtocol":"icmp",
+            "ToPort":86
+          }
+        ],
         "SecurityGroupIngress": [
           {
             "CidrIp": "0.0.0.0/0",

packages/@aws-cdk/aws-ec2/README.md

@@ -174,29 +174,43 @@ Application subnets will route to the NAT Gateway.
 
 ### Allowing Connections
 
-In AWS, all connections to and from EC2 instances are governed by *Security
-Groups*. You can think of these as a firewall with rules. All Constructs that
-create instances on your behalf implicitly have such a security group.
-Unless otherwise indicated using properites, the security groups start out
-empty; that is, no connections are allowed by default.
-
-In general, whenever you link two Constructs together (such as the load balancer and the
-fleet in the previous example), the security groups will be automatically updated to allow
-network connections between the indicated instances. In other cases, you will need to
-configure these allows connections yourself, for example if the connections you want to
-allow do not originate from instances in a CDK construct, or if you want to allow
-connections among instances inside a single security group.
-
-All Constructs with security groups have a member called `connections`, which
-can be used to configure permissible connections. In the most general case, a
-call to allow connections needs both a connection peer and the type of
-connection to allow:
+In AWS, all network traffic in and out of **Elastic Network Interfaces** (ENIs)
+is controlled by **Security Groups**. You can think of Security Groups as a
+firewall with a set of rules. By default, Security Groups allow no incoming
+(ingress) traffic and all outgoing (egress) traffic. You can add ingress rules
+to them to allow incoming traffic streams. To exert fine-grained control over
+egress traffic, set `allowAllOutbound: false` on the `SecurityGroup`, after
+which you can add egress traffic rules.
+
+You can manipulate Security Groups directly:
 
 ```ts
-lb.connections.allowFrom(new ec2.AnyIPv4(), new ec2.TcpPort(443), 'Allow inbound');
+const mySecurityGroup = new ec2.SecurityGroup(this, 'SecurityGroup', {
+  vpc,
+  description: 'Allow ssh access to ec2 instances',
+  allowAllOutbound: true   // Can be set to false
+});
+mySecurityGroup.addIngressRule(new ec2.AnyIPv4(), new ec2.TcpPort(22), 'allow ssh access from the world');
+```
+
+All constructs that create ENIs on your behalf (typically constructs that create
+EC2 instances or other VPC-connected resources) will all have security groups
+automatically assigned. Those constructs have an attribute called
+**connections**, which is an object that makes it convenient to update the
+security groups. If you want to allow connections between two constructs that
+have security groups, you have to add an **Egress* rule to one Security Group,
+and an **Ingress** rule to the other. The connections object will automatically
+take care of this for you:
+
+```ts
+// Allow connections from anywhere
+loadBalancer.connections.allowFromAnyIpv4(new ec2.TcpPort(443), 'Allow inbound HTTPS');
+
+// The same, but an explicit IP address
+loadBalancer.connections.allowFrom(new ec2.CidrIpv4('1.2.3.4/32'), new ec2.TcpPort(443), 'Allow inbound HTTPS');
 
-// Or using a convenience function
-lb.connections.allowFromAnyIpv4(new ec2.TcpPort(443), 'Allow inbound');
+// Allow connection between AutoScalingGroups
+appFleet.connections.allowTo(dbFleet, new ec2.TcpPort(443), 'App can call database');
 ```
 
 ### Connection Peers
@@ -228,10 +242,10 @@ The connections that are allowed are specified by port ranges. A number of class
 the connection specifier:
 
 ```ts
-new ec2.TcpPort(80);
-new ec2.TcpPortRange(60000, 65535);
-new ec2.TcpAllPorts();
-new ec2.AllConnections();
+new ec2.TcpPort(80)
+new ec2.TcpPortRange(60000, 65535)
+new ec2.TcpAllPorts()
+new ec2.AllConnections()
 ```
 
 > NOTE: This set is not complete yet; for example, there is no library support for ICMP at the moment.

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

@@ -340,6 +340,24 @@ export class IcmpTypeAndCode implements IPortRange {
   }
 }
 
+/**
+ * ICMP Ping traffic
+ */
+export class IcmpPing implements IPortRange {
+  public readonly canInlineRule = true;
+
+  public toRuleJSON(): any {
+    return {
+      ipProtocol: Protocol.Icmp,
+      fromPort: 8,
+    };
+  }
+
+  public toString() {
+    return `ICMP PING`;
+  }
+}
+
 /**
  * All ICMP Codes for a given ICMP Type
  */
@@ -384,18 +402,16 @@ export class IcmpAllTypesAndCodes implements IPortRange {
 /**
  * All Traffic
  */
-export class AllConnections implements IPortRange {
+export class AllTraffic implements IPortRange {
   public readonly canInlineRule = true;
 
   public toRuleJSON(): any {
     return {
       ipProtocol: '-1',
-      fromPort: -1,
-      toPort: -1,
     };
   }
 
   public toString() {
     return 'ALL TRAFFIC';
   }
-}
+}

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 true
+   */
+  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.removeNoTrafficRule();
+    }
+
     if (!peer.canInlineRule || !connection.canInlineRule) {
       super.addEgressRule(peer, connection, description);
       return;
@@ -195,11 +225,22 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable {
       description = `from ${peer.uniqueId}:${connection}`;
     }
 
-    this.addDirectEgressRule({
+    const rule = {
       ...peer.toEgressRuleJSON(),
       ...connection.toRuleJSON(),
       description
-    });
+    };
+
+    if (isAllTrafficRule(rule)) {
+      // We cannot allow this; if someone adds the rule in this way, it will be
+      // removed again if they add other rules. We also can't automatically switch
+      // to "allOutbound=true" mode, because we might have already emitted
+      // EgressRule objects (which count as rules added later) and there's no way
+      // to recall those. Better to prevent this for now.
+      throw new Error('Cannot add an "all traffic" egress rule in this way; set allowAllOutbound=true on the SecurityGroup instead.');
+    }
+
+    this.addDirectEgressRule(rule);
   }
 
   /**
@@ -233,8 +274,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(MATCH_NO_TRAFFIC);
+    }
+  }
+
+  /**
+   * Remove the bogus rule if it exists
+   */
+  private removeNoTrafficRule() {
+    const i = this.directEgressRules.findIndex(r => egressRulesEqual(r, MATCH_NO_TRAFFIC));
+    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 MATCH_NO_TRAFFIC = {
+  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).
@@ -315,3 +414,10 @@ function egressRulesEqual(a: cloudformation.SecurityGroupResource.EgressProperty
     && a.destinationPrefixListId === b.destinationPrefixListId
     && a.destinationSecurityGroupId === b.destinationSecurityGroupId;
 }
+
+/**
+ * Whether this rule refers to all traffic
+ */
+function isAllTrafficRule(rule: any) {
+  return rule.cidrIp === '0.0.0.0/0' && rule.ipProtocol === '-1';
+}