From cf5f9c7953f018367f32b2bbfb1e42adf409f7d4 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Fri, 28 Sep 2018 19:27:04 +0200 Subject: [PATCH 1/8] docs(aws-ec2): Explain Security Groups better Explain how to add rules to Security Groups, and rewrite the section on the Connections object a bit. Fixes #799. --- packages/@aws-cdk/aws-ec2/README.md | 59 +++++++++++++++++------------ 1 file changed, 35 insertions(+), 24 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/README.md b/packages/@aws-cdk/aws-ec2/README.md index db5bb8f0f72f9..39928f22d6835 100644 --- a/packages/@aws-cdk/aws-ec2/README.md +++ b/packages/@aws-cdk/aws-ec2/README.md @@ -173,29 +173,40 @@ 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. The Security Groups start out empty (that is, +no network traffic is allowed by default) and rules need to be added to them +to explicitly allow certain types of traffic. + +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', +}); +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) all have security groups +(without you having to create any). 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 @@ -227,10 +238,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. From ad7c31c52ea82819f01f4e24c55e8a4ea7d9d03d Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 11 Oct 2018 11:01:24 +0200 Subject: [PATCH 2/8] Mention that no egress rules = all traffic --- packages/@aws-cdk/aws-ec2/README.md | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/README.md b/packages/@aws-cdk/aws-ec2/README.md index 39928f22d6835..bcc63abd16554 100644 --- a/packages/@aws-cdk/aws-ec2/README.md +++ b/packages/@aws-cdk/aws-ec2/README.md @@ -173,18 +173,21 @@ Application subnets will route to the NAT Gateway. ### Allowing Connections -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. The Security Groups start out empty (that is, -no network traffic is allowed by default) and rules need to be added to them -to explicitly allow certain types of traffic. +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, and the same goes for +egress rules. As soon as you add at least one egress rule to a security +group, only the rules will be honored and the default "all outgoing traffic" +rule no longer applies. You can manipulate Security Groups directly: ```ts const mySecurityGroup = new ec2.SecurityGroup(this, 'SecurityGroup', { - vpc, - description: 'Allow ssh access to ec2 instances', + vpc, + description: 'Allow ssh access to ec2 instances', }); mySecurityGroup.addIngressRule(new ec2.AnyIPv4(), new ec2.TcpPort(22), 'allow ssh access from the world'); ``` From 9c9f912584c6e1ad1561082b4fc1147b4c2b4f33 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 23 Oct 2018 13:07:01 +0200 Subject: [PATCH 3/8] 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. --- .../aws-autoscaling/lib/auto-scaling-group.ts | 9 +- .../@aws-cdk/aws-ec2/lib/security-group.ts | 88 +++++++++++++++++ .../@aws-cdk/aws-ec2/test/test.connections.ts | 96 ++++++++++++++++++- 3 files changed, 187 insertions(+), 6 deletions(-) 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' }); From 641d888f6f8fc8150b1d2d03440d6009f10eab53 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 23 Oct 2018 13:15:27 +0200 Subject: [PATCH 4/8] Update expectation --- .../@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts b/packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts index 453ded434a48e..a2a63050b9658 100644 --- a/packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts +++ b/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": [], From 3fc6c75650477c274f3c5107e469ed795c5bba60 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Wed, 24 Oct 2018 16:41:25 +0200 Subject: [PATCH 5/8] WIP --- packages/@aws-cdk/aws-ec2/lib/security-group.ts | 10 +++++----- packages/@aws-cdk/aws-ec2/test/test.connections.ts | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/lib/security-group.ts b/packages/@aws-cdk/aws-ec2/lib/security-group.ts index 219722a574028..92b945a30573d 100644 --- a/packages/@aws-cdk/aws-ec2/lib/security-group.ts +++ b/packages/@aws-cdk/aws-ec2/lib/security-group.ts @@ -123,7 +123,7 @@ export interface SecurityGroupProps { * 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 + * @default true */ allowAllOutbound?: boolean; } @@ -168,7 +168,7 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable { this.tags = new TagManager(this, { initialTags: props.tags}); const groupDescription = props.description || this.path; - this.allowAllOutbound = props.allowAllOutbound || false; + this.allowAllOutbound = props.allowAllOutbound !== false; this.securityGroup = new cloudformation.SecurityGroupResource(this, 'Resource', { groupName: props.groupName, @@ -282,7 +282,7 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable { if (this.allowAllOutbound) { this.directEgressRules.push(ALLOW_ALL_RULE); } else { - this.directEgressRules.push(BOGUS_RULE); + this.directEgressRules.push(MATCH_NO_TRAFFIC); } } @@ -290,7 +290,7 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable { * Remove the bogus rule if it exists */ private removeBogusRule() { - const i = this.directEgressRules.findIndex(r => egressRulesEqual(r, BOGUS_RULE)); + const i = this.directEgressRules.findIndex(r => egressRulesEqual(r, MATCH_NO_TRAFFIC)); if (i > -1) { this.directEgressRules.splice(i, 1); } @@ -306,7 +306,7 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable { * in order to lock it down even more we'll restrict to a nonexistent * ICMP traffic type. */ -const BOGUS_RULE = { +const MATCH_NO_TRAFFIC = { cidrIp: '255.255.255.255/32', description: 'Disallow all traffic', ipProtocol: 'icmp', diff --git a/packages/@aws-cdk/aws-ec2/test/test.connections.ts b/packages/@aws-cdk/aws-ec2/test/test.connections.ts index 39767929a71b2..9aed57bf7460e 100644 --- a/packages/@aws-cdk/aws-ec2/test/test.connections.ts +++ b/packages/@aws-cdk/aws-ec2/test/test.connections.ts @@ -100,7 +100,7 @@ export = { const vpc = new VpcNetwork(stack, 'VPC'); // WHEN - const sg = new SecurityGroup(stack, 'SG1', { vpc }); + const sg = new SecurityGroup(stack, 'SG1', { vpc, allowAllOutbound: false }); sg.addEgressRule(new AnyIPv4(), new TcpPort(86), 'This replaces the other one'); // THEN From 1e04e0f1fc49ac7881f61da1784accc2f35de1f9 Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 25 Oct 2018 13:50:23 +0200 Subject: [PATCH 6/8] Rename AllConnections() -> AllTraffic() --- packages/@aws-cdk/aws-ec2/README.md | 20 +++++++------- .../aws-ec2/lib/security-group-rule.ts | 24 ++++++++++++++--- .../@aws-cdk/aws-ec2/lib/security-group.ts | 26 ++++++++++++++++--- .../@aws-cdk/aws-ec2/test/test.connections.ts | 21 +++++++++++++-- .../lib/load-balancer.ts | 2 +- .../lib/alb/application-load-balancer.ts | 3 ++- tools/cdk-build-tools/bin/cdk-test.ts | 9 ++++++- 7 files changed, 82 insertions(+), 23 deletions(-) diff --git a/packages/@aws-cdk/aws-ec2/README.md b/packages/@aws-cdk/aws-ec2/README.md index bcc63abd16554..86ac907d29c7e 100644 --- a/packages/@aws-cdk/aws-ec2/README.md +++ b/packages/@aws-cdk/aws-ec2/README.md @@ -173,14 +173,13 @@ Application subnets will route to the NAT Gateway. ### Allowing Connections -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, and the same goes for -egress rules. As soon as you add at least one egress rule to a security -group, only the rules will be honored and the default "all outgoing traffic" -rule no longer applies. +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: @@ -188,13 +187,14 @@ You can manipulate Security Groups directly: 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) all have security groups -(without you having to create any). Those constructs have an attribute called +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, diff --git a/packages/@aws-cdk/aws-ec2/lib/security-group-rule.ts b/packages/@aws-cdk/aws-ec2/lib/security-group-rule.ts index b7ab7925d8dd5..1aae45389a5ee 100644 --- a/packages/@aws-cdk/aws-ec2/lib/security-group-rule.ts +++ b/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'; } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-ec2/lib/security-group.ts b/packages/@aws-cdk/aws-ec2/lib/security-group.ts index 92b945a30573d..49b187bc107bd 100644 --- a/packages/@aws-cdk/aws-ec2/lib/security-group.ts +++ b/packages/@aws-cdk/aws-ec2/lib/security-group.ts @@ -213,7 +213,7 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable { // 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(); + this.removeNoTrafficRule(); } if (!peer.canInlineRule || !connection.canInlineRule) { @@ -225,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); } /** @@ -289,7 +300,7 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable { /** * Remove the bogus rule if it exists */ - private removeBogusRule() { + private removeNoTrafficRule() { const i = this.directEgressRules.findIndex(r => egressRulesEqual(r, MATCH_NO_TRAFFIC)); if (i > -1) { this.directEgressRules.splice(i, 1); @@ -403,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'; +} diff --git a/packages/@aws-cdk/aws-ec2/test/test.connections.ts b/packages/@aws-cdk/aws-ec2/test/test.connections.ts index 9aed57bf7460e..6a56884152f91 100644 --- a/packages/@aws-cdk/aws-ec2/test/test.connections.ts +++ b/packages/@aws-cdk/aws-ec2/test/test.connections.ts @@ -1,13 +1,15 @@ import { expect, haveResource } from '@aws-cdk/assert'; import { Stack } from '@aws-cdk/cdk'; import { Test } from 'nodeunit'; + import { - AllConnections, + AllTraffic, AnyIPv4, AnyIPv6, Connections, IcmpAllTypeCodes, IcmpAllTypesAndCodes, + IcmpPing, IcmpTypeAndCode, IConnectable, PrefixList, @@ -119,6 +121,20 @@ export = { test.done(); }, + 'all outbound rule cannot be added after creation'(test: Test) { + // GIVEN + const stack = new Stack(); + const vpc = new VpcNetwork(stack, 'VPC'); + + // WHEN + const sg = new SecurityGroup(stack, 'SG1', { vpc, allowAllOutbound: false }); + test.throws(() => { + sg.addEgressRule(new AnyIPv4(), new AllTraffic(), 'All traffic'); + }, /Cannot add/); + + 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' }}); @@ -197,7 +213,8 @@ export = { new IcmpTypeAndCode(5, 1), new IcmpAllTypeCodes(8), new IcmpAllTypesAndCodes(), - new AllConnections() + new IcmpPing(), + new AllTraffic() ]; // WHEN diff --git a/packages/@aws-cdk/aws-elasticloadbalancing/lib/load-balancer.ts b/packages/@aws-cdk/aws-elasticloadbalancing/lib/load-balancer.ts index 953a0cacaad61..7df533982bab5 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancing/lib/load-balancer.ts +++ b/packages/@aws-cdk/aws-elasticloadbalancing/lib/load-balancer.ts @@ -207,7 +207,7 @@ export class LoadBalancer extends cdk.Construct implements IConnectable, codedep constructor(parent: cdk.Construct, name: string, props: LoadBalancerProps) { super(parent, name); - this.securityGroup = new SecurityGroup(this, 'SecurityGroup', { vpc: props.vpc }); + this.securityGroup = new SecurityGroup(this, 'SecurityGroup', { vpc: props.vpc, allowAllOutbound: false }); this.connections = new Connections({ securityGroup: this.securityGroup }); // Depending on whether the ELB has public or internal IPs, pick the right backend subnets diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts b/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts index c2a0213cf0003..bb4e15ef8ce22 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts +++ b/packages/@aws-cdk/aws-elasticloadbalancingv2/lib/alb/application-load-balancer.ts @@ -64,7 +64,8 @@ export class ApplicationLoadBalancer extends BaseLoadBalancer implements IApplic this.securityGroup = props.securityGroup || new ec2.SecurityGroup(this, 'SecurityGroup', { vpc: props.vpc, - description: `Automatically created Security Group for ELB ${this.uniqueId}` + description: `Automatically created Security Group for ELB ${this.uniqueId}`, + allowAllOutbound: false }); this.connections = new ec2.Connections({ securityGroup: this.securityGroup }); diff --git a/tools/cdk-build-tools/bin/cdk-test.ts b/tools/cdk-build-tools/bin/cdk-test.ts index f8aa6b9fea07d..2a803249109cc 100644 --- a/tools/cdk-build-tools/bin/cdk-test.ts +++ b/tools/cdk-build-tools/bin/cdk-test.ts @@ -39,6 +39,11 @@ async function main() { default: require.resolve('nodeunit/bin/nodeunit'), defaultDescription: 'nodeunit provided by node dependencies' }) + .option('build', { + type: 'boolean', + desc: 'Rebuild the packages before running the tests', + default: true, + }) .argv as any; const options = cdkBuildOptions(); @@ -50,7 +55,9 @@ async function main() { // Always recompile before running tests, so it's impossible to forget. // During a normal build, this means we'll compile twice, but the // hash calculation makes that cheaper on CPU (if not on disk). - await compileCurrentPackage(timers, { jsii: args.jsii, tsc: args.tsc }); + if (args.build) { + await compileCurrentPackage(timers, { jsii: args.jsii, tsc: args.tsc }); + } const testFiles = await unitTestFiles(); if (testFiles.length > 0) { From e4b6b3ec223bb2737e45c2ea738647b88f7fdead Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 25 Oct 2018 14:06:17 +0200 Subject: [PATCH 7/8] Update expectations --- ...nteg.asg-w-classic-loadbalancer.expected.json | 8 +++----- .../test/integ.asg-w-elbv2.expected.json | 6 ++---- .../test/integ.deployment-group.expected.json | 16 +++++++++++----- .../test/integ.elb.expected.json | 12 ++++++++++-- .../test/integ.alb.expected.json | 10 +++++++++- .../aws-rds/test/integ.cluster.expected.json | 10 ++++++++-- 6 files changed, 43 insertions(+), 19 deletions(-) diff --git a/packages/@aws-cdk/aws-autoscaling/test/integ.asg-w-classic-loadbalancer.expected.json b/packages/@aws-cdk/aws-autoscaling/test/integ.asg-w-classic-loadbalancer.expected.json index 12f63b4c44098..0ca90cc9a329f 100644 --- a/packages/@aws-cdk/aws-autoscaling/test/integ.asg-w-classic-loadbalancer.expected.json +++ b/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 @@ } } } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-autoscaling/test/integ.asg-w-elbv2.expected.json b/packages/@aws-cdk/aws-autoscaling/test/integ.asg-w-elbv2.expected.json index 38158f31daeb9..167f41eed8cac 100644 --- a/packages/@aws-cdk/aws-autoscaling/test/integ.asg-w-elbv2.expected.json +++ b/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": [], diff --git a/packages/@aws-cdk/aws-codedeploy/test/integ.deployment-group.expected.json b/packages/@aws-cdk/aws-codedeploy/test/integ.deployment-group.expected.json index a64931c5829e0..3cb6c923102c3 100644 --- a/packages/@aws-cdk/aws-codedeploy/test/integ.deployment-group.expected.json +++ b/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", diff --git a/packages/@aws-cdk/aws-elasticloadbalancing/test/integ.elb.expected.json b/packages/@aws-cdk/aws-elasticloadbalancing/test/integ.elb.expected.json index 25da8feca4a7f..50513f3f5ba0f 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancing/test/integ.elb.expected.json +++ b/packages/@aws-cdk/aws-elasticloadbalancing/test/integ.elb.expected.json @@ -175,7 +175,15 @@ "Type": "AWS::EC2::SecurityGroup", "Properties": { "GroupDescription": "aws-cdk-elb-integ/LB/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", @@ -225,4 +233,4 @@ } } } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/integ.alb.expected.json b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/integ.alb.expected.json index 47090c4d5e411..6e2cd595b15b8 100644 --- a/packages/@aws-cdk/aws-elasticloadbalancingv2/test/integ.alb.expected.json +++ b/packages/@aws-cdk/aws-elasticloadbalancingv2/test/integ.alb.expected.json @@ -333,7 +333,15 @@ "Type": "AWS::EC2::SecurityGroup", "Properties": { "GroupDescription": "Automatically created Security Group for ELB awscdkelbv2integLB9950B1E4", - "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", diff --git a/packages/@aws-cdk/aws-rds/test/integ.cluster.expected.json b/packages/@aws-cdk/aws-rds/test/integ.cluster.expected.json index c95a923aec045..35e7814dcf7eb 100644 --- a/packages/@aws-cdk/aws-rds/test/integ.cluster.expected.json +++ b/packages/@aws-cdk/aws-rds/test/integ.cluster.expected.json @@ -380,7 +380,13 @@ "Type": "AWS::EC2::SecurityGroup", "Properties": { "GroupDescription": "RDS security group", - "SecurityGroupEgress": [], + "SecurityGroupEgress": [ + { + "CidrIp":"0.0.0.0/0", + "Description":"Allow all outbound traffic by default", + "IpProtocol":"-1" + } + ], "SecurityGroupIngress": [], "VpcId": { "Ref": "VPCB9E5F0B4" @@ -481,4 +487,4 @@ ] } } -} \ No newline at end of file +} From 95a39df043e9c2f647d57a34c6e2124778e5353b Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Thu, 25 Oct 2018 14:56:50 +0200 Subject: [PATCH 8/8] Make allowAllOutbound configurable for Lambda --- packages/@aws-cdk/aws-lambda/lib/lambda.ts | 28 +++++++++++++++---- .../test/integ.vpc-lambda.expected.json | 8 ++---- .../aws-lambda/test/integ.vpc-lambda.ts | 4 +-- .../aws-lambda/test/test.vpc-lambda.ts | 7 +++-- 4 files changed, 30 insertions(+), 17 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/lib/lambda.ts b/packages/@aws-cdk/aws-lambda/lib/lambda.ts index ba409aafd394e..ada7082d963bd 100644 --- a/packages/@aws-cdk/aws-lambda/lib/lambda.ts +++ b/packages/@aws-cdk/aws-lambda/lib/lambda.ts @@ -140,6 +140,16 @@ export interface FunctionProps { */ securityGroup?: ec2.SecurityGroupRef; + /** + * Whether to allow the Lambda to send all network traffic + * + * If set to false, you must individually add traffic rules to allow the + * Lambda to connect to network targets. + * + * @default true + */ + allowAllOutbound?: boolean; + /** * Enabled DLQ. If `deadLetterQueue` is undefined, * an SQS queue with default options will be defined for your Function. @@ -312,16 +322,22 @@ export class Function extends FunctionRef { * Lambda creation properties. */ private configureVpc(props: FunctionProps): cloudformation.FunctionResource.VpcConfigProperty | undefined { + if ((props.securityGroup || props.allowAllOutbound !== undefined) && !props.vpc) { + throw new Error(`Cannot configure 'securityGroup' or 'allowAllOutbound' without configuring a VPC`); + } + if (!props.vpc) { return undefined; } - let securityGroup = props.securityGroup; - if (!securityGroup) { - securityGroup = new ec2.SecurityGroup(this, 'SecurityGroup', { - vpc: props.vpc, - description: 'Automatic security group for Lambda Function ' + this.uniqueId, - }); + if (props.securityGroup && props.allowAllOutbound !== undefined) { + throw new Error(`Configure 'allowAllOutbound' directly on the supplied SecurityGroup.`); } + const securityGroup = props.securityGroup || new ec2.SecurityGroup(this, 'SecurityGroup', { + vpc: props.vpc, + description: 'Automatic security group for Lambda Function ' + this.uniqueId, + allowAllOutbound: props.allowAllOutbound + }); + this._connections = new ec2.Connections({ securityGroup }); // Pick subnets, make sure they're not Public. Routing through an IGW diff --git a/packages/@aws-cdk/aws-lambda/test/integ.vpc-lambda.expected.json b/packages/@aws-cdk/aws-lambda/test/integ.vpc-lambda.expected.json index 3862d4993d222..386f53a95073c 100644 --- a/packages/@aws-cdk/aws-lambda/test/integ.vpc-lambda.expected.json +++ b/packages/@aws-cdk/aws-lambda/test/integ.vpc-lambda.expected.json @@ -355,10 +355,8 @@ "SecurityGroupEgress": [ { "CidrIp": "0.0.0.0/0", - "Description": "Talk to everyone", - "FromPort": 0, - "IpProtocol": "tcp", - "ToPort": 65535 + "Description": "Allow all outbound traffic by default", + "IpProtocol": "-1" } ], "SecurityGroupIngress": [], @@ -405,4 +403,4 @@ ] } } -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-lambda/test/integ.vpc-lambda.ts b/packages/@aws-cdk/aws-lambda/test/integ.vpc-lambda.ts index e2f622f8bdafb..8f59243ce5db5 100644 --- a/packages/@aws-cdk/aws-lambda/test/integ.vpc-lambda.ts +++ b/packages/@aws-cdk/aws-lambda/test/integ.vpc-lambda.ts @@ -7,13 +7,11 @@ const app = new cdk.App(); const stack = new cdk.Stack(app, 'aws-cdk-vpc-lambda'); const vpc = new ec2.VpcNetwork(stack, 'VPC', { maxAZs: 2 }); -const fn = new lambda.Function(stack, 'MyLambda', { +new lambda.Function(stack, 'MyLambda', { code: new lambda.InlineCode('def main(event, context): pass'), handler: 'index.main', runtime: lambda.Runtime.Python36, vpc }); -fn.connections.allowToAnyIPv4(new ec2.TcpAllPorts(), 'Talk to everyone'); - app.run(); diff --git a/packages/@aws-cdk/aws-lambda/test/test.vpc-lambda.ts b/packages/@aws-cdk/aws-lambda/test/test.vpc-lambda.ts index 903fa1c738a57..2af214ae84f0d 100644 --- a/packages/@aws-cdk/aws-lambda/test/test.vpc-lambda.ts +++ b/packages/@aws-cdk/aws-lambda/test/test.vpc-lambda.ts @@ -20,7 +20,8 @@ export = { code: new lambda.InlineCode('foo'), handler: 'index.handler', runtime: lambda.Runtime.NodeJS610, - vpc: this.vpc + vpc: this.vpc, + allowAllOutbound: false }); } @@ -50,7 +51,7 @@ export = { // WHEN this.lambda.connections.allowTo(somethingConnectable, new ec2.TcpAllPorts(), 'Lambda can call connectable'); - // THEN: SomeSecurityGroup accepts connections from Lambda + // THEN: Lambda can connect to SomeSecurityGroup expect(this.stack).to(haveResource("AWS::EC2::SecurityGroupEgress", { GroupId: {"Fn::GetAtt": ["LambdaSecurityGroupE74659A1", "GroupId"]}, IpProtocol: "tcp", @@ -60,7 +61,7 @@ export = { ToPort: 65535 })); - // THEN: Lambda can connect to SomeSecurityGroup + // THEN: SomeSecurityGroup accepts connections from Lambda expect(this.stack).to(haveResource("AWS::EC2::SecurityGroupIngress", { IpProtocol: "tcp", Description: "Lambda can call connectable",