Skip to content

Commit b9d5b43

Browse files
rix0rrrrix0rrr
authored
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.
1 parent 9281f05 commit b9d5b43

File tree

19 files changed

+369
-80
lines changed

19 files changed

+369
-80
lines changed

‎packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -179,16 +179,15 @@ export class AutoScalingGroup extends cdk.Construct implements cdk.ITaggable, el
179179
constructor(parent: cdk.Construct, name: string, props: AutoScalingGroupProps) {
180180
super(parent, name);
181181

182-
this.securityGroup = new ec2.SecurityGroup(this, 'InstanceSecurityGroup', { vpc: props.vpc });
182+
this.securityGroup = new ec2.SecurityGroup(this, 'InstanceSecurityGroup', {
183+
vpc: props.vpc,
184+
allowAllOutbound: props.allowAllOutbound !== false
185+
});
183186
this.connections = new ec2.Connections({ securityGroup: this.securityGroup });
184187
this.securityGroups.push(this.securityGroup);
185188
this.tags = new TagManager(this, {initialTags: props.tags});
186189
this.tags.setTag(NAME_TAG, this.path, { overwrite: false });
187190

188-
if (props.allowAllOutbound !== false) {
189-
this.connections.allowTo(new ec2.AnyIPv4(), new ec2.AllConnections(), 'Outbound traffic allowed by default');
190-
}
191-
192191
this.role = new iam.Role(this, 'InstanceRole', {
193192
assumedBy: new iam.ServicePrincipal('ec2.amazonaws.com')
194193
});

‎packages/@aws-cdk/aws-autoscaling/test/integ.asg-w-classic-loadbalancer.expected.json‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -446,10 +446,8 @@
446446
"SecurityGroupEgress": [
447447
{
448448
"CidrIp": "0.0.0.0/0",
449-
"Description": "Outbound traffic allowed by default",
450-
"FromPort": -1,
451-
"IpProtocol": "-1",
452-
"ToPort": -1
449+
"Description": "Allow all outbound traffic by default",
450+
"IpProtocol": "-1"
453451
}
454452
],
455453
"SecurityGroupIngress": [],
@@ -656,4 +654,4 @@
656654
}
657655
}
658656
}
659-
}
657+
}

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -312,10 +312,8 @@
312312
"SecurityGroupEgress": [
313313
{
314314
"CidrIp": "0.0.0.0/0",
315-
"Description": "Outbound traffic allowed by default",
316-
"FromPort": -1,
317-
"IpProtocol": "-1",
318-
"ToPort": -1
315+
"Description": "Allow all outbound traffic by default",
316+
"IpProtocol": "-1"
319317
}
320318
],
321319
"SecurityGroupIngress": [],

‎packages/@aws-cdk/aws-autoscaling/test/test.auto-scaling-group.ts‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,10 +27,8 @@ export = {
2727
"SecurityGroupEgress": [
2828
{
2929
"CidrIp": "0.0.0.0/0",
30-
"Description": "Outbound traffic allowed by default",
31-
"FromPort": -1,
30+
"Description": "Allow all outbound traffic by default",
3231
"IpProtocol": "-1",
33-
"ToPort": -1
3432
}
3533
],
3634
"SecurityGroupIngress": [],

‎packages/@aws-cdk/aws-codedeploy/test/integ.deployment-group.expected.json‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -446,10 +446,8 @@
446446
"SecurityGroupEgress": [
447447
{
448448
"CidrIp": "0.0.0.0/0",
449-
"Description": "Outbound traffic allowed by default",
450-
"FromPort": -1,
451-
"IpProtocol": "-1",
452-
"ToPort": -1
449+
"Description": "Allow all outbound traffic by default",
450+
"IpProtocol": "-1"
453451
}
454452
],
455453
"SecurityGroupIngress": [],
@@ -626,7 +624,15 @@
626624
"Type": "AWS::EC2::SecurityGroup",
627625
"Properties": {
628626
"GroupDescription": "aws-cdk-codedeploy-server-dg/ELB/SecurityGroup",
629-
"SecurityGroupEgress": [],
627+
"SecurityGroupEgress": [
628+
{
629+
"CidrIp":"255.255.255.255/32",
630+
"Description":"Disallow all traffic",
631+
"FromPort":252,
632+
"IpProtocol":"icmp",
633+
"ToPort":86
634+
}
635+
],
630636
"SecurityGroupIngress": [
631637
{
632638
"CidrIp": "0.0.0.0/0",

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

Lines changed: 38 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -174,29 +174,43 @@ Application subnets will route to the NAT Gateway.
174174

175175
### Allowing Connections
176176

177-
In AWS, all connections to and from EC2 instances are governed by *Security
178-
Groups*. You can think of these as a firewall with rules. All Constructs that
179-
create instances on your behalf implicitly have such a security group.
180-
Unless otherwise indicated using properites, the security groups start out
181-
empty; that is, no connections are allowed by default.
182-
183-
In general, whenever you link two Constructs together (such as the load balancer and the
184-
fleet in the previous example), the security groups will be automatically updated to allow
185-
network connections between the indicated instances. In other cases, you will need to
186-
configure these allows connections yourself, for example if the connections you want to
187-
allow do not originate from instances in a CDK construct, or if you want to allow
188-
connections among instances inside a single security group.
189-
190-
All Constructs with security groups have a member called `connections`, which
191-
can be used to configure permissible connections. In the most general case, a
192-
call to allow connections needs both a connection peer and the type of
193-
connection to allow:
177+
In AWS, all network traffic in and out of **Elastic Network Interfaces** (ENIs)
178+
is controlled by **Security Groups**. You can think of Security Groups as a
179+
firewall with a set of rules. By default, Security Groups allow no incoming
180+
(ingress) traffic and all outgoing (egress) traffic. You can add ingress rules
181+
to them to allow incoming traffic streams. To exert fine-grained control over
182+
egress traffic, set `allowAllOutbound: false` on the `SecurityGroup`, after
183+
which you can add egress traffic rules.
184+
185+
You can manipulate Security Groups directly:
194186

195187
```ts
196-
lb.connections.allowFrom(new ec2.AnyIPv4(), new ec2.TcpPort(443), 'Allow inbound');
188+
const mySecurityGroup = new ec2.SecurityGroup(this, 'SecurityGroup', {
189+
vpc,
190+
description: 'Allow ssh access to ec2 instances',
191+
allowAllOutbound: true // Can be set to false
192+
});
193+
mySecurityGroup.addIngressRule(new ec2.AnyIPv4(), new ec2.TcpPort(22), 'allow ssh access from the world');
194+
```
195+
196+
All constructs that create ENIs on your behalf (typically constructs that create
197+
EC2 instances or other VPC-connected resources) will all have security groups
198+
automatically assigned. Those constructs have an attribute called
199+
**connections**, which is an object that makes it convenient to update the
200+
security groups. If you want to allow connections between two constructs that
201+
have security groups, you have to add an **Egress* rule to one Security Group,
202+
and an **Ingress** rule to the other. The connections object will automatically
203+
take care of this for you:
204+
205+
```ts
206+
// Allow connections from anywhere
207+
loadBalancer.connections.allowFromAnyIpv4(new ec2.TcpPort(443), 'Allow inbound HTTPS');
208+
209+
// The same, but an explicit IP address
210+
loadBalancer.connections.allowFrom(new ec2.CidrIpv4('1.2.3.4/32'), new ec2.TcpPort(443), 'Allow inbound HTTPS');
197211

198-
// Or using a convenience function
199-
lb.connections.allowFromAnyIpv4(new ec2.TcpPort(443), 'Allow inbound');
212+
// Allow connection between AutoScalingGroups
213+
appFleet.connections.allowTo(dbFleet, new ec2.TcpPort(443), 'App can call database');
200214
```
201215

202216
### Connection Peers
@@ -228,10 +242,10 @@ The connections that are allowed are specified by port ranges. A number of class
228242
the connection specifier:
229243

230244
```ts
231-
new ec2.TcpPort(80);
232-
new ec2.TcpPortRange(60000, 65535);
233-
new ec2.TcpAllPorts();
234-
new ec2.AllConnections();
245+
new ec2.TcpPort(80)
246+
new ec2.TcpPortRange(60000, 65535)
247+
new ec2.TcpAllPorts()
248+
new ec2.AllConnections()
235249
```
236250

237251
> 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‎

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -340,6 +340,24 @@ export class IcmpTypeAndCode implements IPortRange {
340340
}
341341
}
342342

343+
/**
344+
* ICMP Ping traffic
345+
*/
346+
export class IcmpPing implements IPortRange {
347+
public readonly canInlineRule = true;
348+
349+
public toRuleJSON(): any {
350+
return {
351+
ipProtocol: Protocol.Icmp,
352+
fromPort: 8,
353+
};
354+
}
355+
356+
public toString() {
357+
return `ICMP PING`;
358+
}
359+
}
360+
343361
/**
344362
* All ICMP Codes for a given ICMP Type
345363
*/
@@ -384,18 +402,16 @@ export class IcmpAllTypesAndCodes implements IPortRange {
384402
/**
385403
* All Traffic
386404
*/
387-
export class AllConnections implements IPortRange {
405+
export class AllTraffic implements IPortRange {
388406
public readonly canInlineRule = true;
389407

390408
public toRuleJSON(): any {
391409
return {
392410
ipProtocol: '-1',
393-
fromPort: -1,
394-
toPort: -1,
395411
};
396412
}
397413

398414
public toString() {
399415
return 'ALL TRAFFIC';
400416
}
401-
}
417+
}

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

Lines changed: 108 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,17 @@ export interface SecurityGroupProps {
115115
* The VPC in which to create the security group.
116116
*/
117117
vpc: VpcNetworkRef;
118+
119+
/**
120+
* Whether to allow all outbound traffic by default.
121+
*
122+
* If this is set to true, there will only be a single egress rule which allows all
123+
* outbound traffic. If this is set to false, no outbound traffic will be allowed by
124+
* default and all egress traffic must be explicitly authorized.
125+
*
126+
* @default true
127+
*/
128+
allowAllOutbound?: boolean;
118129
}
119130

120131
/**
@@ -149,11 +160,16 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable {
149160
private readonly directIngressRules: cloudformation.SecurityGroupResource.IngressProperty[] = [];
150161
private readonly directEgressRules: cloudformation.SecurityGroupResource.EgressProperty[] = [];
151162

163+
private readonly allowAllOutbound: boolean;
164+
152165
constructor(parent: Construct, name: string, props: SecurityGroupProps) {
153166
super(parent, name);
154167

155168
this.tags = new TagManager(this, { initialTags: props.tags});
156169
const groupDescription = props.description || this.path;
170+
171+
this.allowAllOutbound = props.allowAllOutbound !== false;
172+
157173
this.securityGroup = new cloudformation.SecurityGroupResource(this, 'Resource', {
158174
groupName: props.groupName,
159175
groupDescription,
@@ -166,6 +182,8 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable {
166182
this.securityGroupId = this.securityGroup.securityGroupId;
167183
this.groupName = this.securityGroup.securityGroupName;
168184
this.vpcId = this.securityGroup.securityGroupVpcId;
185+
186+
this.addDefaultEgressRule();
169187
}
170188

171189
public addIngressRule(peer: ISecurityGroupRule, connection: IPortRange, description?: string) {
@@ -186,6 +204,18 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable {
186204
}
187205

188206
public addEgressRule(peer: ISecurityGroupRule, connection: IPortRange, description?: string) {
207+
if (this.allowAllOutbound) {
208+
// In the case of "allowAllOutbound", we don't add any more rules. There
209+
// is only one rule which allows all traffic and that subsumes any other
210+
// rule.
211+
return;
212+
} else {
213+
// Otherwise, if the bogus rule exists we can now remove it because the
214+
// presence of any other rule will get rid of EC2's implicit "all
215+
// outbound" rule anyway.
216+
this.removeNoTrafficRule();
217+
}
218+
189219
if (!peer.canInlineRule || !connection.canInlineRule) {
190220
super.addEgressRule(peer, connection, description);
191221
return;
@@ -195,11 +225,22 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable {
195225
description = `from ${peer.uniqueId}:${connection}`;
196226
}
197227

198-
this.addDirectEgressRule({
228+
const rule = {
199229
...peer.toEgressRuleJSON(),
200230
...connection.toRuleJSON(),
201231
description
202-
});
232+
};
233+
234+
if (isAllTrafficRule(rule)) {
235+
// We cannot allow this; if someone adds the rule in this way, it will be
236+
// removed again if they add other rules. We also can't automatically switch
237+
// to "allOutbound=true" mode, because we might have already emitted
238+
// EgressRule objects (which count as rules added later) and there's no way
239+
// to recall those. Better to prevent this for now.
240+
throw new Error('Cannot add an "all traffic" egress rule in this way; set allowAllOutbound=true on the SecurityGroup instead.');
241+
}
242+
243+
this.addDirectEgressRule(rule);
203244
}
204245

205246
/**
@@ -233,8 +274,66 @@ export class SecurityGroup extends SecurityGroupRef implements ITaggable {
233274
private hasEgressRule(rule: cloudformation.SecurityGroupResource.EgressProperty): boolean {
234275
return this.directEgressRules.findIndex(r => egressRulesEqual(r, rule)) > -1;
235276
}
277+
278+
/**
279+
* Add the default egress rule to the securityGroup
280+
*
281+
* This depends on allowAllOutbound:
282+
*
283+
* - If allowAllOutbound is true, we *TECHNICALLY* don't need to do anything, because
284+
* EC2 is going to create this default rule anyway. But, for maximum readability
285+
* of the template, we will add one anyway.
286+
* - If allowAllOutbound is false, we add a bogus rule that matches no traffic in
287+
* order to get rid of the default "all outbound" rule that EC2 creates by default.
288+
* If other rules happen to get added later, we remove the bogus rule again so
289+
* that it doesn't clutter up the template too much (even though that's not
290+
* strictly necessary).
291+
*/
292+
private addDefaultEgressRule() {
293+
if (this.allowAllOutbound) {
294+
this.directEgressRules.push(ALLOW_ALL_RULE);
295+
} else {
296+
this.directEgressRules.push(MATCH_NO_TRAFFIC);
297+
}
298+
}
299+
300+
/**
301+
* Remove the bogus rule if it exists
302+
*/
303+
private removeNoTrafficRule() {
304+
const i = this.directEgressRules.findIndex(r => egressRulesEqual(r, MATCH_NO_TRAFFIC));
305+
if (i > -1) {
306+
this.directEgressRules.splice(i, 1);
307+
}
308+
}
236309
}
237310

311+
/**
312+
* Egress rule that purposely matches no traffic
313+
*
314+
* This is used in order to disable the "all traffic" default of Security Groups.
315+
*
316+
* No machine can ever actually have the 255.255.255.255 IP address, but
317+
* in order to lock it down even more we'll restrict to a nonexistent
318+
* ICMP traffic type.
319+
*/
320+
const MATCH_NO_TRAFFIC = {
321+
cidrIp: '255.255.255.255/32',
322+
description: 'Disallow all traffic',
323+
ipProtocol: 'icmp',
324+
fromPort: 252,
325+
toPort: 86
326+
};
327+
328+
/**
329+
* Egress rule that matches all traffic
330+
*/
331+
const ALLOW_ALL_RULE = {
332+
cidrIp: '0.0.0.0/0',
333+
description: 'Allow all outbound traffic by default',
334+
ipProtocol: '-1',
335+
};
336+
238337
export interface ConnectionRule {
239338
/**
240339
* The IP protocol name (tcp, udp, icmp) or number (see Protocol Numbers).
@@ -315,3 +414,10 @@ function egressRulesEqual(a: cloudformation.SecurityGroupResource.EgressProperty
315414
&& a.destinationPrefixListId === b.destinationPrefixListId
316415
&& a.destinationSecurityGroupId === b.destinationSecurityGroupId;
317416
}
417+
418+
/**
419+
* Whether this rule refers to all traffic
420+
*/
421+
function isAllTrafficRule(rule: any) {
422+
return rule.cidrIp === '0.0.0.0/0' && rule.ipProtocol === '-1';
423+
}

0 commit comments

Comments
 (0)