feat(ec2): imported SecurityGroups don't create egress rules (#3386)

Security Groups are created with `allowAllOutbound: true` by default, and so imported security groups default to that as well. This means that no egress rules will be created for them, because that will undo the default of `allowAllOutbound`.

This can be configured by setting `allowAllOutbound: false` upon importing.

Fixes #3355.

Author: ayazhussein

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

@@ -276,4 +276,4 @@ class ReactiveList<T> {
   public get length(): number {
     return this.elements.length;
   }
-}
+}

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

@@ -215,6 +215,23 @@ export interface SecurityGroupProps {
   readonly allowAllOutbound?: boolean;
 }
 
+/**
+ * Additional options for imported security groups
+ */
+export interface SecurityGroupImportOptions {
+  /**
+   * Mark the SecurityGroup as having been created allowing all outbound traffico
+   *
+   * Only if this is set to false will egress rules be added to this security
+   * group. Be aware, this would undo any potential "all outbound traffic"
+   * default.
+   *
+   * @experimental
+   * @default true
+   */
+  readonly allowAllOutbound?: boolean;
+}
+
 /**
  * Creates an Amazon EC2 security group within a VPC.
  *
@@ -227,9 +244,16 @@ export class SecurityGroup extends SecurityGroupBase {
   /**
    * Import an existing security group into this app.
    */
-  public static fromSecurityGroupId(scope: Construct, id: string, securityGroupId: string): ISecurityGroup {
+  public static fromSecurityGroupId(scope: Construct, id: string, securityGroupId: string, options: SecurityGroupImportOptions = {}): ISecurityGroup {
     class Import extends SecurityGroupBase {
       public securityGroupId = securityGroupId;
+
+      public addEgressRule(peer: IPeer, connection: Port, description?: string, remoteRule?: boolean) {
+        // Only if allowAllOutbound has been disabled
+        if (options.allowAllOutbound === false) {
+          super.addEgressRule(peer, connection, description, remoteRule);
+        }
+      }
     }
 
     return new Import(scope, id);

packages/@aws-cdk/aws-ec2/package.json

@@ -101,7 +101,8 @@
       "props-physical-name:@aws-cdk/aws-ec2.VpcProps",
       "props-physical-name:@aws-cdk/aws-ec2.InterfaceVpcEndpointProps",
       "from-method:@aws-cdk/aws-ec2.Instance",
-      "attribute-tag:@aws-cdk/aws-ec2.Instance.instance"
+      "attribute-tag:@aws-cdk/aws-ec2.Instance.instance",
+      "from-signature:@aws-cdk/aws-ec2.SecurityGroup.fromSecurityGroupId"
     ]
   },
   "stability": "stable"

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

@@ -261,6 +261,69 @@ export = {
       DestinationSecurityGroupId: { "Fn::GetAtt": [ "SecurityGroupDD263621", "GroupId" ] },
     }));
 
+    test.done();
+  },
+  'Imported SecurityGroup does not create egress rule'(test: Test) {
+    // GIVEN
+    const stack = new Stack();
+    const vpc = new Vpc(stack, 'VPC');
+    const sg1 = new SecurityGroup(stack, 'SomeSecurityGroup', { vpc, allowAllOutbound: false });
+    const somethingConnectable = new SomethingConnectable(new Connections({ securityGroups: [sg1] }));
+
+    const securityGroup = SecurityGroup.fromSecurityGroupId(stack, 'ImportedSG', 'sg-12345');
+
+    // WHEN
+    somethingConnectable.connections.allowFrom(securityGroup, Port.allTcp(), 'Connect there');
+
+    // THEN: rule to generated security group to connect to imported
+    expect(stack).to(haveResource("AWS::EC2::SecurityGroupIngress", {
+      GroupId: { "Fn::GetAtt": [ "SomeSecurityGroupEF219AD6", "GroupId" ] },
+      IpProtocol: "tcp",
+      Description: "Connect there",
+      SourceSecurityGroupId: "sg-12345",
+      FromPort: 0,
+      ToPort: 65535
+    }));
+
+    // THEN: rule to imported security group to allow connections from generated
+    expect(stack).notTo(haveResource("AWS::EC2::SecurityGroupEgress"));
+
+    test.done();
+  },
+  'Imported SecurityGroup with allowAllOutbound: false DOES create egress rule'(test: Test) {
+    // GIVEN
+    const stack = new Stack();
+    const vpc = new Vpc(stack, 'VPC');
+    const sg1 = new SecurityGroup(stack, 'SomeSecurityGroup', { vpc, allowAllOutbound: false });
+    const somethingConnectable = new SomethingConnectable(new Connections({ securityGroups: [sg1] }));
+
+    const securityGroup = SecurityGroup.fromSecurityGroupId(stack, 'ImportedSG', 'sg-12345', {
+      allowAllOutbound: false
+    });
+
+    // WHEN
+    somethingConnectable.connections.allowFrom(securityGroup, Port.allTcp(), 'Connect there');
+
+    // THEN: rule to generated security group to connect to imported
+    expect(stack).to(haveResource("AWS::EC2::SecurityGroupIngress", {
+      GroupId: { "Fn::GetAtt": [ "SomeSecurityGroupEF219AD6", "GroupId" ] },
+      IpProtocol: "tcp",
+      Description: "Connect there",
+      SourceSecurityGroupId: "sg-12345",
+      FromPort: 0,
+      ToPort: 65535
+    }));
+
+    // THEN: rule to imported security group to allow connections from generated
+    expect(stack).to(haveResource("AWS::EC2::SecurityGroupEgress", {
+      IpProtocol: "tcp",
+      Description: "Connect there",
+      FromPort: 0,
+      GroupId: "sg-12345",
+      DestinationSecurityGroupId: { "Fn::GetAtt": [ "SomeSecurityGroupEF219AD6", "GroupId" ] },
+      ToPort: 65535
+    }));
+
     test.done();
   }
 };

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

@@ -199,11 +199,9 @@ export = {
 
     'passes with unresolved IP CIDR token'(test: Test) {
       // GIVEN
-      const cidrIp = Token.asString(new Intrinsic('ip'));
+      Token.asString(new Intrinsic('ip'));
 
-      // THEN
-      test.equal(Peer.ipv4(cidrIp).uniqueId, '${Token[TOKEN.1385]}');
-      test.equal(Peer.ipv6(cidrIp).uniqueId, '${Token[TOKEN.1385]}');
+      // THEN: don't throw
 
       test.done();
     },

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-listener.ts

@@ -331,6 +331,15 @@ export interface ApplicationListenerAttributes {
    * The default port on which this listener is listening
    */
   readonly defaultPort?: number;
+
+  /**
+   * Whether the security group allows all outbound traffic or not
+   *
+   * Unless set to `false`, no egress rules will be added to the security group.
+   *
+   * @default true
+   */
+  readonly securityGroupAllowsAllOutbound?: boolean;
 }
 
 class ImportedApplicationListener extends Resource implements IApplicationListener {
@@ -349,7 +358,9 @@ class ImportedApplicationListener extends Resource implements IApplicationListen
     const defaultPort = props.defaultPort !== undefined ? ec2.Port.tcp(props.defaultPort) : undefined;
 
     this.connections = new ec2.Connections({
-      securityGroups: [ec2.SecurityGroup.fromSecurityGroupId(this, 'SecurityGroup', props.securityGroupId)],
+      securityGroups: [ec2.SecurityGroup.fromSecurityGroupId(this, 'SecurityGroup', props.securityGroupId, {
+        allowAllOutbound: props.securityGroupAllowsAllOutbound
+      })],
       defaultPort,
     });
   }

packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts

@@ -516,6 +516,15 @@ export interface ApplicationLoadBalancerAttributes {
    * @default - When not provided, LB cannot be used as Route53 Alias target.
    */
   readonly loadBalancerDnsName?: string;
+
+  /**
+   * Whether the security group allows all outbound traffic or not
+   *
+   * Unless set to `false`, no egress rules will be added to the security group.
+   *
+   * @default true
+   */
+  readonly securityGroupAllowsAllOutbound?: boolean;
 }
 
 // https://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-access-logs.html#access-logging-bucket-permissions
@@ -567,7 +576,9 @@ class ImportedApplicationLoadBalancer extends Resource implements IApplicationLo
 
     this.loadBalancerArn = props.loadBalancerArn;
     this.connections = new ec2.Connections({
-      securityGroups: [ec2.SecurityGroup.fromSecurityGroupId(this, 'SecurityGroup', props.securityGroupId)]
+      securityGroups: [ec2.SecurityGroup.fromSecurityGroupId(this, 'SecurityGroup', props.securityGroupId, {
+        allowAllOutbound: props.securityGroupAllowsAllOutbound
+      })]
     });
   }
 

packages/@aws-cdk/aws-elasticloadbalancingv2/test/alb/test.security-groups.ts

@@ -114,7 +114,8 @@ export = {
     // WHEN
     const lb2 = elbv2.ApplicationLoadBalancer.fromApplicationLoadBalancerAttributes(stack2, 'LB', {
       loadBalancerArn: fixture.lb.loadBalancerArn,
-      securityGroupId: fixture.lb.connections.securityGroups[0].securityGroupId
+      securityGroupId: fixture.lb.connections.securityGroups[0].securityGroupId,
+      securityGroupAllowsAllOutbound: false,
     });
     const listener2 = lb2.addListener('YetAnotherListener', { port: 80 });
     listener2.addTargetGroups('Default', { targetGroups: [group] });
@@ -142,7 +143,8 @@ export = {
     const listener2 = elbv2.ApplicationListener.fromApplicationListenerAttributes(stack2, 'YetAnotherListener', {
       defaultPort: 8008,
       securityGroupId: fixture.listener.connections.securityGroups[0].securityGroupId,
-      listenerArn: fixture.listener.listenerArn
+      listenerArn: fixture.listener.listenerArn,
+      securityGroupAllowsAllOutbound: false,
     });
     listener2.addTargetGroups('Default', {
       // Must be a non-default target