Skip to content

Commit

Permalink
fix(ec2): NAT provider's default outbound rules cannot be disabled (a…
Browse files Browse the repository at this point in the history 
…ws#12674)

`allowAllTraffic` only applies to inbound traffic, but it should also apply to outbound traffic. 

Deprecate it and add a new enum-based property, `defaultAllowedTraffic`, which also allows controlling outbound traffic rules. There is no option to allow inbound but not outbound because there is no use case for that.

Fix aws#12673

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
@hedrall @cornerwings
hedrall authored and cornerwings committed Mar 8, 2021
1 parent 54eefcd commit 53d5802
Show file tree
Hide file tree
Showing 2 changed files with 202 additions and 6 deletions.
57 changes: 55 additions & 2 deletions packages/@aws-cdk/aws-ec2/lib/nat.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,26 @@ import { Port } from './port';
import { ISecurityGroup, SecurityGroup } from './security-group';
import { PrivateSubnet, PublicSubnet, RouterType, Vpc } from './vpc';

/**
* Direction of traffic to allow all by default.
*/
export enum NatTrafficDirection {
/**
* Allow all outbound traffic and disallow all inbound traffic.
*/
OUTBOUND_ONLY = 'OUTBOUND_ONLY',

/**
* Allow all outbound and inbound traffic.
*/
INBOUND_AND_OUTBOUND = 'INBOUND_AND_OUTBOUND',

/**
* Disallow all outbound and inbound traffic.
*/
NONE = 'NONE',
}

/**
* Pair represents a gateway created by NAT Provider
*/
Expand Down Expand Up @@ -148,7 +168,7 @@ export interface NatInstanceProps {
readonly securityGroup?: ISecurityGroup;

/**
* Allow all traffic through the NAT instance
* Allow all inbound traffic through the NAT instance
*
* If you set this to false, you must configure the NAT instance's security
* groups in another way, either by passing in a fully configured Security
Expand All @@ -157,8 +177,24 @@ export interface NatInstanceProps {
* Provider to a Vpc.
*
* @default true
* @deprecated - Use `defaultAllowedTraffic`.
*/
readonly allowAllTraffic?: boolean;

/**
* Direction to allow all traffic through the NAT instance by default.
*
* By default, inbound and outbound traffic is allowed.
*
* If you set this to another value than INBOUND_AND_OUTBOUND, you must
* configure the NAT instance's security groups in another way, either by
* passing in a fully configured Security Group using the `securityGroup`
* property, or by configuring it using the `.securityGroup` or
* `.connections` members after passing the NAT Instance Provider to a Vpc.
*
* @default NatTrafficDirection.INBOUND_AND_OUTBOUND
*/
readonly defaultAllowedTraffic?: NatTrafficDirection;
}

/**
Expand Down Expand Up @@ -205,18 +241,26 @@ export class NatInstanceProvider extends NatProvider implements IConnectable {

constructor(private readonly props: NatInstanceProps) {
super();

if (props.defaultAllowedTraffic !== undefined && props.allowAllTraffic !== undefined) {
throw new Error('Can not specify both of \'defaultAllowedTraffic\' and \'defaultAllowedTraffic\'; prefer \'defaultAllowedTraffic\'');
}
}

public configureNat(options: ConfigureNatOptions) {
const defaultDirection = this.props.defaultAllowedTraffic ??
(this.props.allowAllTraffic ?? true ? NatTrafficDirection.INBOUND_AND_OUTBOUND : NatTrafficDirection.OUTBOUND_ONLY);

// Create the NAT instances. They can share a security group and a Role.
const machineImage = this.props.machineImage || new NatInstanceImage();
this._securityGroup = this.props.securityGroup ?? new SecurityGroup(options.vpc, 'NatSecurityGroup', {
vpc: options.vpc,
description: 'Security Group for NAT instances',
allowAllOutbound: isOutboundAllowed(defaultDirection),
});
this._connections = new Connections({ securityGroups: [this._securityGroup] });

if (this.props.allowAllTraffic ?? true) {
if (isInboundAllowed(defaultDirection)) {
this.connections.allowFromAnyIpv4(Port.allTraffic());
}

Expand Down Expand Up @@ -325,3 +369,12 @@ export class NatInstanceImage extends LookupMachineImage {
});
}
}

function isOutboundAllowed(direction: NatTrafficDirection) {
return direction === NatTrafficDirection.INBOUND_AND_OUTBOUND ||
direction === NatTrafficDirection.OUTBOUND_ONLY;
}

function isInboundAllowed(direction: NatTrafficDirection) {
return direction === NatTrafficDirection.INBOUND_AND_OUTBOUND;
}
151 changes: 147 additions & 4 deletions packages/@aws-cdk/aws-ec2/test/vpc.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,30 @@ import { countResources, expect as cdkExpect, haveResource, haveResourceLike, is
import { CfnOutput, CfnResource, Fn, Lazy, Stack, Tags } from '@aws-cdk/core';
import { nodeunitShim, Test } from 'nodeunit-shim';
import {
AclCidr, AclTraffic, BastionHostLinux, CfnSubnet, CfnVPC, SubnetFilter, DefaultInstanceTenancy, GenericLinuxImage,
InstanceType, InterfaceVpcEndpoint, InterfaceVpcEndpointService, NatProvider, NetworkAcl, NetworkAclEntry, Peer, Port, PrivateSubnet,
PublicSubnet, RouterType, Subnet, SubnetType, TrafficDirection, Vpc,
AclCidr,
AclTraffic,
BastionHostLinux,
CfnSubnet,
CfnVPC,
SubnetFilter,
DefaultInstanceTenancy,
GenericLinuxImage,
InstanceType,
InterfaceVpcEndpoint,
InterfaceVpcEndpointService,
NatProvider,
NatTrafficDirection,
NetworkAcl,
NetworkAclEntry,
Peer,
Port,
PrivateSubnet,
PublicSubnet,
RouterType,
Subnet,
SubnetType,
TrafficDirection,
Vpc,
} from '../lib';

nodeunitShim({
Expand Down Expand Up @@ -904,6 +925,22 @@ nodeunitShim({
DestinationCidrBlock: '0.0.0.0/0',
InstanceId: { Ref: 'TheVPCPublicSubnet1NatInstanceCC514192' },
}));
cdkExpect(stack).to(haveResource('AWS::EC2::SecurityGroup', {
SecurityGroupEgress: [
{
CidrIp: '0.0.0.0/0',
Description: 'Allow all outbound traffic by default',
IpProtocol: '-1',
},
],
SecurityGroupIngress: [
{
CidrIp: '0.0.0.0/0',
Description: 'from 0.0.0.0/0:ALL TRAFFIC',
IpProtocol: '-1',
},
],
}));

test.done();
},
Expand All @@ -929,7 +966,7 @@ nodeunitShim({
test.done();
},

'can configure Security Groups of NAT instances'(test: Test) {
'can configure Security Groups of NAT instances with allowAllTraffic false'(test: Test) {
// GIVEN
const stack = getTestStack();

Expand All @@ -948,6 +985,13 @@ nodeunitShim({

// THEN
cdkExpect(stack).to(haveResource('AWS::EC2::SecurityGroup', {
SecurityGroupEgress: [
{
CidrIp: '0.0.0.0/0',
Description: 'Allow all outbound traffic by default',
IpProtocol: '-1',
},
],
SecurityGroupIngress: [
{
CidrIp: '1.2.3.4/32',
Expand All @@ -962,6 +1006,105 @@ nodeunitShim({
test.done();
},

'can configure Security Groups of NAT instances with defaultAllowAll INBOUND_AND_OUTBOUND'(test: Test) {
// GIVEN
const stack = getTestStack();

// WHEN
const provider = NatProvider.instance({
instanceType: new InstanceType('q86.mega'),
machineImage: new GenericLinuxImage({
'us-east-1': 'ami-1',
}),
defaultAllowedTraffic: NatTrafficDirection.INBOUND_AND_OUTBOUND,
});
new Vpc(stack, 'TheVPC', {
natGatewayProvider: provider,
});

// THEN
cdkExpect(stack).to(haveResource('AWS::EC2::SecurityGroup', {
SecurityGroupEgress: [
{
CidrIp: '0.0.0.0/0',
Description: 'Allow all outbound traffic by default',
IpProtocol: '-1',
},
],
SecurityGroupIngress: [
{
CidrIp: '0.0.0.0/0',
Description: 'from 0.0.0.0/0:ALL TRAFFIC',
IpProtocol: '-1',
},
],
}));

test.done();
},

'can configure Security Groups of NAT instances with defaultAllowAll OUTBOUND_ONLY'(test: Test) {
// GIVEN
const stack = getTestStack();

// WHEN
const provider = NatProvider.instance({
instanceType: new InstanceType('q86.mega'),
machineImage: new GenericLinuxImage({
'us-east-1': 'ami-1',
}),
defaultAllowedTraffic: NatTrafficDirection.OUTBOUND_ONLY,
});
new Vpc(stack, 'TheVPC', {
natGatewayProvider: provider,
});

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

test.done();
},

'can configure Security Groups of NAT instances with defaultAllowAll NONE'(test: Test) {
// GIVEN
const stack = getTestStack();

// WHEN
const provider = NatProvider.instance({
instanceType: new InstanceType('q86.mega'),
machineImage: new GenericLinuxImage({
'us-east-1': 'ami-1',
}),
defaultAllowedTraffic: NatTrafficDirection.NONE,
});
new Vpc(stack, 'TheVPC', {
natGatewayProvider: provider,
});

// THEN
cdkExpect(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();
},

},

'Network ACL association': {
Expand Down

0 comments on commit 53d5802

Please sign in to comment.