Skip to content

Commit

Permalink
fix(ec2): cannot allow all ipv6 traffic (aws#22279)
Browse files Browse the repository at this point in the history 
This PR fixes an issue where it was impossible to add ipv6 egress rules without an escape hatch. The SecurityGroup construct will now track ipv6 and ipv4 separately. This matches the default behavior of CloudFormation and the underlying EC2 API.

By default when you create a SecurityGroup (either via CFN or console) it will create an allow all ipv4 rule. If you later add a more specific rule CloudFormation will remove the ipv4 allow all rule. Since the default behavior is to _not_ add an allow all for ipv6, the SecurityGroup construct will also not add it by default.

There is an edge case that this does break, but I'm not sure if it is a valid case. Previously it would have been possible to do the following (only allow all ipv6 outbound). But we don't want to allow that for the same reason we don't allow it for ipv4 (it will be overwritten by more restrictive rules).

```ts
const sg = new SecurityGroup(this, 'Sg', {
  allowAllOutbound: false,
});
sg.addEgressRule(Peer.anyIpv6(), Port.allTraffic());
```

fixes aws#7094


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@corymhall @madeline-k
corymhall authored and madeline-k committed Oct 10, 2022
1 parent ed9b75d commit 425ee08
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 7 deletions.
81 changes: 77 additions & 4 deletions packages/@aws-cdk/aws-ec2/lib/security-group.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ abstract class SecurityGroupBase extends Resource implements ISecurityGroup {

public abstract readonly securityGroupId: string;
public abstract readonly allowAllOutbound: boolean;
public abstract readonly allowAllIpv6Outbound: boolean;

public readonly canInlineRule = false;
public readonly connections: Connections = new Connections({ securityGroups: [this] });
Expand Down Expand Up @@ -237,10 +238,25 @@ 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.
*
* To allow all ipv6 traffic use allowAllIpv6Outbound
*
* @default true
*/
readonly allowAllOutbound?: boolean;

/**
* Whether to allow all outbound ipv6 traffic by default.
*
* If this is set to true, there will only be a single egress rule which allows all
* outbound ipv6 traffic. If this is set to false, no outbound traffic will be allowed by
* default and all egress ipv6 traffic must be explicitly authorized.
*
* To allow all ipv4 traffic use allowAllOutbound
*
* @default false
*/
readonly allowAllIpv6Outbound?: boolean;

/**
* Whether to disable inline ingress and egress rule optimization.
*
Expand Down Expand Up @@ -274,6 +290,17 @@ export interface SecurityGroupImportOptions {
*/
readonly allowAllOutbound?: boolean;

/**
* Mark the SecurityGroup as having been created allowing all outbound ipv6 traffic
*
* Only if this is set to false will egress rules for ipv6 be added to this security
* group. Be aware, this would undo any potential "all outbound traffic"
* default.
*
* @default false
*/
readonly allowAllIpv6Outbound?: boolean;

/**
* If a SecurityGroup is mutable CDK can add rules to existing groups
*
Expand Down Expand Up @@ -360,6 +387,7 @@ export class SecurityGroup extends SecurityGroupBase {
class MutableImport extends SecurityGroupBase {
public securityGroupId = securityGroupId;
public allowAllOutbound = options.allowAllOutbound ?? true;
public allowAllIpv6Outbound = options.allowAllIpv6Outbound ?? false;

public addEgressRule(peer: IPeer, connection: Port, description?: string, remoteRule?: boolean) {
// Only if allowAllOutbound has been disabled
Expand All @@ -372,6 +400,7 @@ export class SecurityGroup extends SecurityGroupBase {
class ImmutableImport extends SecurityGroupBase {
public securityGroupId = securityGroupId;
public allowAllOutbound = options.allowAllOutbound ?? true;
public allowAllIpv6Outbound = options.allowAllIpv6Outbound ?? false;

public addEgressRule(_peer: IPeer, _connection: Port, _description?: string, _remoteRule?: boolean) {
// do nothing
Expand Down Expand Up @@ -441,6 +470,11 @@ export class SecurityGroup extends SecurityGroupBase {
*/
public readonly allowAllOutbound: boolean;

/**
* Whether the SecurityGroup has been configured to allow all outbound ipv6 traffic
*/
public readonly allowAllIpv6Outbound: boolean;

private readonly securityGroup: CfnSecurityGroup;
private readonly directIngressRules: CfnSecurityGroup.IngressProperty[] = [];
private readonly directEgressRules: CfnSecurityGroup.EgressProperty[] = [];
Expand All @@ -458,6 +492,7 @@ export class SecurityGroup extends SecurityGroupBase {
const groupDescription = props.description || this.node.path;

this.allowAllOutbound = props.allowAllOutbound !== false;
this.allowAllIpv6Outbound = props.allowAllIpv6Outbound ?? false;

this.disableInlineRules = props.disableInlineRules !== undefined ?
!!props.disableInlineRules :
Expand All @@ -476,6 +511,7 @@ export class SecurityGroup extends SecurityGroupBase {
this.securityGroupName = this.securityGroup.ref;

this.addDefaultEgressRule();
this.addDefaultIpv6EgressRule();
}

public addIngressRule(peer: IPeer, connection: Port, description?: string, remoteRule?: boolean) {
Expand All @@ -496,21 +532,33 @@ export class SecurityGroup extends SecurityGroupBase {
}

public addEgressRule(peer: IPeer, connection: Port, description?: string, remoteRule?: boolean) {
if (this.allowAllOutbound) {
const isIpv6 = peer.toEgressRuleConfig().hasOwnProperty('cidrIpv6');

if (!isIpv6 && 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.
if (!remoteRule) { // Warn only if addEgressRule() was explicitely called
Annotations.of(this).addWarning('Ignoring Egress rule since \'allowAllOutbound\' is set to true; To add customized rules, set allowAllOutbound=false on the SecurityGroup');
}
return;
} else {
} else if (!isIpv6 && !this.allowAllOutbound) {
// 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 (isIpv6 && this.allowAllIpv6Outbound) {
// In the case of "allowAllIpv6Outbound", we don't add any more rules. There
// is only one rule which allows all traffic and that subsumes any other
// rule.
if (!remoteRule) { // Warn only if addEgressRule() was explicitely called
Annotations.of(this).addWarning('Ignoring Egress rule since \'allowAllIpv6Outbound\' is set to true; To add customized rules, set allowAllIpv6Outbound=false on the SecurityGroup');
}
return;
}

if (!peer.canInlineRule || !connection.canInlineRule || this.disableInlineRules) {
super.addEgressRule(peer, connection, description, remoteRule);
return;
Expand All @@ -532,7 +580,7 @@ export class SecurityGroup extends SecurityGroupBase {
// 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.');
throw new Error('Cannot add an "all traffic" egress rule in this way; set allowAllOutbound=true (for ipv6) or allowAllIpv6Outbound=true (for ipv6) on the SecurityGroup instead.');
}

this.addDirectEgressRule(rule);
Expand Down Expand Up @@ -596,6 +644,31 @@ export class SecurityGroup extends SecurityGroupBase {
}
}

/**
* Add a allow all ipv6 egress rule to the securityGroup
*
* This depends on allowAllIpv6Outbound:
*
* - If allowAllIpv6Outbound is true, we will add an allow all rule.
* - If allowAllOutbound is false, we don't do anything since EC2 does not add
* a default allow all ipv6 rule.
*/
private addDefaultIpv6EgressRule() {
const description = 'Allow all outbound ipv6 traffic by default';
const peer = Peer.anyIpv6();
if (this.allowAllIpv6Outbound) {
if (this.disableInlineRules) {
super.addEgressRule(peer, Port.allTraffic(), description, false);
} else {
this.directEgressRules.push({
ipProtocol: '-1',
cidrIp: peer.uniqueId,
description,
});
}
}
}

/**
* Remove the bogus rule if it exists
*/
Expand Down Expand Up @@ -721,7 +794,7 @@ function egressRulesEqual(a: CfnSecurityGroup.EgressProperty, b: CfnSecurityGrou
* Whether this rule refers to all traffic
*/
function isAllTrafficRule(rule: any) {
return rule.cidrIp === '0.0.0.0/0' && rule.ipProtocol === '-1';
return (rule.cidrIp === '0.0.0.0/0' || rule.cidrIpv6 === '::/0') && rule.ipProtocol === '-1';
}

/**
Expand Down
66 changes: 63 additions & 3 deletions packages/@aws-cdk/aws-ec2/test/security-group.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,59 @@ describe('security group', () => {
},
],
});
});

test('security group can allows all ipv6 outbound traffic by default', () => {
// GIVEN
const stack = new Stack();
const vpc = new Vpc(stack, 'VPC');

// WHEN
new SecurityGroup(stack, 'SG1', { vpc, allowAllIpv6Outbound: true });

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::EC2::SecurityGroup', {
SecurityGroupEgress: [
{
CidrIp: '0.0.0.0/0',
Description: 'Allow all outbound traffic by default',
IpProtocol: '-1',
},
{
CidrIp: '::/0',
Description: 'Allow all outbound ipv6 traffic by default',
IpProtocol: '-1',
},
],
});
});

test('can add ipv6 rules even if allowAllOutbound=true', () => {
// GIVEN
const stack = new Stack();
const vpc = new Vpc(stack, 'VPC');

// WHEN
const sg = new SecurityGroup(stack, 'SG1', { vpc });
sg.addEgressRule(Peer.ipv6('2001:db8::/128'), Port.tcp(80));

// THEN
Template.fromStack(stack).hasResourceProperties('AWS::EC2::SecurityGroup', {
SecurityGroupEgress: [
{
CidrIp: '0.0.0.0/0',
Description: 'Allow all outbound traffic by default',
IpProtocol: '-1',
},
{
CidrIpv6: '2001:db8::/128',
Description: 'from 2001:db8::/128:80',
FromPort: 80,
ToPort: 80,
IpProtocol: 'tcp',
},
],
});

});

Expand Down Expand Up @@ -96,8 +148,6 @@ describe('security group', () => {
},
],
});


});

test('all outbound rule cannot be added after creation', () => {
Expand All @@ -110,8 +160,18 @@ describe('security group', () => {
expect(() => {
sg.addEgressRule(Peer.anyIpv4(), Port.allTraffic(), 'All traffic');
}).toThrow(/Cannot add/);
});

test('all ipv6 outbound rule cannot be added after creation', () => {
// GIVEN
const stack = new Stack();
const vpc = new Vpc(stack, 'VPC');

// WHEN
const sg = new SecurityGroup(stack, 'SG1', { vpc, allowAllOutbound: false });
expect(() => {
sg.addEgressRule(Peer.anyIpv6(), Port.allTraffic(), 'All traffic');
}).toThrow(/Cannot add/);
});

test('immutable imports do not add rules', () => {
Expand Down Expand Up @@ -171,7 +231,7 @@ describe('security group', () => {
// GIVEN
const stack = new Stack(undefined, 'TestStack', { env: { account: '12345678', region: 'dummy' } });
const vpc = new Vpc(stack, 'VPC');
const sg = new SecurityGroup(stack, 'SG', { vpc });
const sg = new SecurityGroup(stack, 'SG', { vpc, allowAllIpv6Outbound: true });

const peers = [
new SecurityGroup(stack, 'PeerGroup', { vpc }),
Expand Down

0 comments on commit 425ee08

Please sign in to comment.