diff --git a/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts b/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts index 1509b353c7cec..9f82bae8f8098 100644 --- a/packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts +++ b/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') }); diff --git a/packages/@aws-cdk/aws-ec2/lib/security-group.ts b/packages/@aws-cdk/aws-ec2/lib/security-group.ts index e124433616da1..219722a574028 100644 --- a/packages/@aws-cdk/aws-ec2/lib/security-group.ts +++ b/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). diff --git a/packages/@aws-cdk/aws-ec2/test/test.connections.ts b/packages/@aws-cdk/aws-ec2/test/test.connections.ts index 909328f644f19..39767929a71b2 100644 --- a/packages/@aws-cdk/aws-ec2/test/test.connections.ts +++ b/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' });