Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ec2): imported SecurityGroups don't create egress rules #3386

Merged
merged 8 commits into from
Aug 26, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/@aws-cdk/aws-ec2/lib/connections.ts
Original file line number Diff line number Diff line change
Expand Up @@ -276,4 +276,4 @@ class ReactiveList<T> {
public get length(): number {
return this.elements.length;
}
}
}
26 changes: 25 additions & 1 deletion packages/@aws-cdk/aws-ec2/lib/security-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,23 @@ export interface SecurityGroupProps {
readonly allowAllOutbound?: boolean;
}

/**
* Additional options for imported security groups
*/
export interface SecurityGroupImportOptions {
/**
* Mark the SecurityGroup as having been created allowing all outbound traffico
*
* Only if this is set to false will egress rules be added to this security
* group. Be aware, this would undo any potential "all outbound traffic"
* default.
*
* @experimental
* @default true
*/
readonly allowAllOutbound?: boolean;
}

/**
* Creates an Amazon EC2 security group within a VPC.
*
Expand All @@ -227,9 +244,16 @@ export class SecurityGroup extends SecurityGroupBase {
/**
* Import an existing security group into this app.
*/
public static fromSecurityGroupId(scope: Construct, id: string, securityGroupId: string): ISecurityGroup {
public static fromSecurityGroupId(scope: Construct, id: string, securityGroupId: string, options: SecurityGroupImportOptions = {}): ISecurityGroup {
class Import extends SecurityGroupBase {
public securityGroupId = securityGroupId;

public addEgressRule(peer: IPeer, connection: Port, description?: string, remoteRule?: boolean) {
// Only if allowAllOutbound has been disabled
if (options.allowAllOutbound === false) {
super.addEgressRule(peer, connection, description, remoteRule);
}
}
}

return new Import(scope, id);
Expand Down
3 changes: 2 additions & 1 deletion packages/@aws-cdk/aws-ec2/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,8 @@
"props-physical-name:@aws-cdk/aws-ec2.VpcProps",
"props-physical-name:@aws-cdk/aws-ec2.InterfaceVpcEndpointProps",
"from-method:@aws-cdk/aws-ec2.Instance",
"attribute-tag:@aws-cdk/aws-ec2.Instance.instance"
"attribute-tag:@aws-cdk/aws-ec2.Instance.instance",
"from-signature:@aws-cdk/aws-ec2.SecurityGroup.fromSecurityGroupId"
]
},
"stability": "stable"
Expand Down
63 changes: 63 additions & 0 deletions packages/@aws-cdk/aws-ec2/test/test.connections.ts
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,69 @@ export = {
DestinationSecurityGroupId: { "Fn::GetAtt": [ "SecurityGroupDD263621", "GroupId" ] },
}));

test.done();
},
'Imported SecurityGroup does not create egress rule'(test: Test) {
// GIVEN
const stack = new Stack();
const vpc = new Vpc(stack, 'VPC');
const sg1 = new SecurityGroup(stack, 'SomeSecurityGroup', { vpc, allowAllOutbound: false });
const somethingConnectable = new SomethingConnectable(new Connections({ securityGroups: [sg1] }));

const securityGroup = SecurityGroup.fromSecurityGroupId(stack, 'ImportedSG', 'sg-12345');

// WHEN
somethingConnectable.connections.allowFrom(securityGroup, Port.allTcp(), 'Connect there');

// THEN: rule to generated security group to connect to imported
expect(stack).to(haveResource("AWS::EC2::SecurityGroupIngress", {
GroupId: { "Fn::GetAtt": [ "SomeSecurityGroupEF219AD6", "GroupId" ] },
IpProtocol: "tcp",
Description: "Connect there",
SourceSecurityGroupId: "sg-12345",
FromPort: 0,
ToPort: 65535
}));

// THEN: rule to imported security group to allow connections from generated
expect(stack).notTo(haveResource("AWS::EC2::SecurityGroupEgress"));

test.done();
},
'Imported SecurityGroup with allowAllOutbound: false DOES create egress rule'(test: Test) {
// GIVEN
const stack = new Stack();
const vpc = new Vpc(stack, 'VPC');
const sg1 = new SecurityGroup(stack, 'SomeSecurityGroup', { vpc, allowAllOutbound: false });
const somethingConnectable = new SomethingConnectable(new Connections({ securityGroups: [sg1] }));

const securityGroup = SecurityGroup.fromSecurityGroupId(stack, 'ImportedSG', 'sg-12345', {
allowAllOutbound: false
});

// WHEN
somethingConnectable.connections.allowFrom(securityGroup, Port.allTcp(), 'Connect there');

// THEN: rule to generated security group to connect to imported
expect(stack).to(haveResource("AWS::EC2::SecurityGroupIngress", {
GroupId: { "Fn::GetAtt": [ "SomeSecurityGroupEF219AD6", "GroupId" ] },
IpProtocol: "tcp",
Description: "Connect there",
SourceSecurityGroupId: "sg-12345",
FromPort: 0,
ToPort: 65535
}));

// THEN: rule to imported security group to allow connections from generated
expect(stack).to(haveResource("AWS::EC2::SecurityGroupEgress", {
IpProtocol: "tcp",
Description: "Connect there",
FromPort: 0,
GroupId: "sg-12345",
DestinationSecurityGroupId: { "Fn::GetAtt": [ "SomeSecurityGroupEF219AD6", "GroupId" ] },
ToPort: 65535
}));

test.done();
}
};
Expand Down
6 changes: 2 additions & 4 deletions packages/@aws-cdk/aws-ec2/test/test.security-group.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,9 @@ export = {

'passes with unresolved IP CIDR token'(test: Test) {
// GIVEN
const cidrIp = Token.asString(new Intrinsic('ip'));
Token.asString(new Intrinsic('ip'));

// THEN
test.equal(Peer.ipv4(cidrIp).uniqueId, '${Token[TOKEN.1385]}');
test.equal(Peer.ipv6(cidrIp).uniqueId, '${Token[TOKEN.1385]}');
// THEN: don't throw

test.done();
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,15 @@ export interface ApplicationListenerAttributes {
* The default port on which this listener is listening
*/
readonly defaultPort?: number;

/**
* Whether the security group allows all outbound traffic or not
*
* Unless set to `false`, no egress rules will be added to the security group.
*
* @default true
*/
readonly securityGroupAllowsAllOutbound?: boolean;
}

class ImportedApplicationListener extends Resource implements IApplicationListener {
Expand All @@ -349,7 +358,9 @@ class ImportedApplicationListener extends Resource implements IApplicationListen
const defaultPort = props.defaultPort !== undefined ? ec2.Port.tcp(props.defaultPort) : undefined;

this.connections = new ec2.Connections({
securityGroups: [ec2.SecurityGroup.fromSecurityGroupId(this, 'SecurityGroup', props.securityGroupId)],
securityGroups: [ec2.SecurityGroup.fromSecurityGroupId(this, 'SecurityGroup', props.securityGroupId, {
allowAllOutbound: props.securityGroupAllowsAllOutbound
})],
defaultPort,
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -516,6 +516,15 @@ export interface ApplicationLoadBalancerAttributes {
* @default - When not provided, LB cannot be used as Route53 Alias target.
*/
readonly loadBalancerDnsName?: string;

/**
* Whether the security group allows all outbound traffic or not
*
* Unless set to `false`, no egress rules will be added to the security group.
*
* @default true
*/
readonly securityGroupAllowsAllOutbound?: boolean;
}

// https://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-access-logs.html#access-logging-bucket-permissions
Expand Down Expand Up @@ -567,7 +576,9 @@ class ImportedApplicationLoadBalancer extends Resource implements IApplicationLo

this.loadBalancerArn = props.loadBalancerArn;
this.connections = new ec2.Connections({
securityGroups: [ec2.SecurityGroup.fromSecurityGroupId(this, 'SecurityGroup', props.securityGroupId)]
securityGroups: [ec2.SecurityGroup.fromSecurityGroupId(this, 'SecurityGroup', props.securityGroupId, {
allowAllOutbound: props.securityGroupAllowsAllOutbound
})]
});
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,8 @@ export = {
// WHEN
const lb2 = elbv2.ApplicationLoadBalancer.fromApplicationLoadBalancerAttributes(stack2, 'LB', {
loadBalancerArn: fixture.lb.loadBalancerArn,
securityGroupId: fixture.lb.connections.securityGroups[0].securityGroupId
securityGroupId: fixture.lb.connections.securityGroups[0].securityGroupId,
securityGroupAllowsAllOutbound: false,
});
const listener2 = lb2.addListener('YetAnotherListener', { port: 80 });
listener2.addTargetGroups('Default', { targetGroups: [group] });
Expand Down Expand Up @@ -142,7 +143,8 @@ export = {
const listener2 = elbv2.ApplicationListener.fromApplicationListenerAttributes(stack2, 'YetAnotherListener', {
defaultPort: 8008,
securityGroupId: fixture.listener.connections.securityGroups[0].securityGroupId,
listenerArn: fixture.listener.listenerArn
listenerArn: fixture.listener.listenerArn,
securityGroupAllowsAllOutbound: false,
});
listener2.addTargetGroups('Default', {
// Must be a non-default target
Expand Down