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/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';
+}