Skip to content

Commit

Permalink
fix(aws-ec2): fix retention of all egress traffic rule (#998)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
rix0rrr authored Oct 25, 2018
1 parent 9281f05 commit b9d5b43
Show file tree
Hide file tree
Showing 19 changed files with 369 additions and 80 deletions.
9 changes: 4 additions & 5 deletions packages/@aws-cdk/aws-autoscaling/lib/auto-scaling-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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')
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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": [],
Expand Down Expand Up @@ -656,4 +654,4 @@
}
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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": [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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": [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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": [],
Expand Down Expand Up @@ -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",
Expand Down
62 changes: 38 additions & 24 deletions packages/@aws-cdk/aws-ec2/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
24 changes: 20 additions & 4 deletions packages/@aws-cdk/aws-ec2/lib/security-group-rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Expand Down Expand Up @@ -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';
}
}
}
110 changes: 108 additions & 2 deletions packages/@aws-cdk/aws-ec2/lib/security-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

/**
Expand Down Expand Up @@ -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,
Expand All @@ -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) {
Expand All @@ -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;
Expand All @@ -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);
}

/**
Expand Down Expand Up @@ -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).
Expand Down Expand Up @@ -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';
}
Loading

0 comments on commit b9d5b43

Please sign in to comment.