fix(aws-ec2): fix SecurityGroup's all egress traffic rule #998

rix0rrr · 2018-10-23T11:16:24Z

Make "allowAllOutbound" traffic a property of a SecurityGroup. 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, because all traffic is allowed by default anyway.

If we don't do this, our behavior of adding reciprocal rules between
SecurityGroups conflicts with CloudFormation's behavior to strip the
"default outbound" rule when another rule gets added, and it becomes
impossible to configure "all outbound" traffic on a
SecurityGroup.

Fixes #987.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.

Explain how to add rules to Security Groups, and rewrite the section on the Connections object a bit. Fixes #799.

Make "allowAllOutbound" traffic a property of a SecurityGroup. 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, because all traffic is allowed by default anyway. If we don't do this, our behavior of adding reciprocal rules between SecurityGroups conflicts with CloudFormation's behavior to strip the "default outbound" rule when another rule gets added, and it becomes impossible to configure "all outbound" traffic on a SecurityGroup. Fixes #987.

eladb · 2018-10-23T12:38:21Z

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

+ * in order to lock it down even more we'll restrict to a nonexistent
+ * ICMP traffic type.
+ */
+const BOGUS_RULE = {


Why not call this BLOCK_ALL_TRAFFIC?

Yeah, probably more professional. Actually it should be something like MATCH_NO_TRAFFIC or somesuch.

moofish32

Thanks for taking up this issue so quickly. I just had a couple of some questions.

moofish32 · 2018-10-23T15:20:17Z

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

  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;


Just for my future coding preference do we prefer:

this.allowAllOutbound = props.allowAllOutbound || false; // or this.allowAllOutbound = !!props.allowAllOutbound

My bigger question here is defaulting this to false. I think most users expect this to be true? However, other infra code products take this approach so perhaps that is better?

My thinking here has been torn between two ways.

This is the "secure by default" solution.

However, it takes more work to get to "what you want", if you want to access things outside your VPC (which you almost always want, to use AWS services). All constructs that create SG's now have to set the default to true (which ASGs also do), which is kinda silly.

Yeah, maybe you're right.

To be clear I'm also on the fence just like you, just highlighting the pivot in AWS status quo on this particular feature. I can support either direction.

Going to go with "works by default" over "secure by default" after all I think.

…explicit-outbound-asg

…utbound-asg

__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade your installation the CDK Toolkit to the matching version: ```shell $ npm i -g aws-cdk $ cdk --version 0.14.0 (build ...) ``` Bug Fixes ========= * remove CloudFormation property renames ([#973](#973)) ([3f86603](3f86603)), closes [#852](#852) * **aws-ec2:** fix retention of all egress traffic rule ([#998](#998)) ([b9d5b43](b9d5b43)), closes [#987](#987) * **aws-s3-deployment:** avoid deletion during update using physical ids ([#1006](#1006)) ([bca99c6](bca99c6)), closes [#981](#981) [#981](#981) * **cloudformation-diff:** ignore changes to DependsOn ([#1005](#1005)) ([3605f9c](3605f9c)), closes [#274](#274) * **cloudformation-diff:** track replacements ([#1003](#1003)) ([a83ac5f](a83ac5f)), closes [#1001](#1001) * **docs:** fix EC2 readme for "natgatway" configuration ([#994](#994)) ([0b1e7cc](0b1e7cc)) * **docs:** updates to contribution guide ([#997](#997)) ([b42e742](b42e742)) * **iam:** Merge multiple principals correctly ([#983](#983)) ([3fc5c8c](3fc5c8c)), closes [#924](#924) [#916](#916) [#958](#958) Features ========= * add construct library for Application AutoScaling ([#933](#933)) ([7861c6f](7861c6f)), closes [#856](#856) [#861](#861) [#640](#640) [#644](#644) * add HostedZone context provider ([#823](#823)) ([1626c37](1626c37)) * **assert:** haveResource lists failing properties ([#1016](#1016)) ([7f6f3fd](7f6f3fd)) * **aws-cdk:** add CDK app version negotiation ([#988](#988)) ([db4e718](db4e718)), closes [#891](#891) * **aws-codebuild:** Introduce a CodePipeline test Action. ([#873](#873)) ([770f9aa](770f9aa)) * **aws-sqs:** Add grantXxx() methods ([#1004](#1004)) ([8c90350](8c90350)) * **core:** Pre-concatenate Fn::Join ([#967](#967)) ([33c32a8](33c32a8)), closes [#916](#916) [#958](#958) BREAKING CHANGES ========= * DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling. * CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names. * You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.

__IMPORTANT NOTE__: when upgrading to this version of the CDK framework, you must also upgrade your installation the CDK Toolkit to the matching version: ```shell $ npm i -g aws-cdk $ cdk --version 0.14.0 (build ...) ``` Bug Fixes ========= * remove CloudFormation property renames ([aws#973](aws#973)) ([3f86603](aws@3f86603)), closes [aws#852](aws#852) * **aws-ec2:** fix retention of all egress traffic rule ([aws#998](aws#998)) ([b9d5b43](aws@b9d5b43)), closes [aws#987](aws#987) * **aws-s3-deployment:** avoid deletion during update using physical ids ([aws#1006](aws#1006)) ([bca99c6](aws@bca99c6)), closes [aws#981](aws#981) [aws#981](aws#981) * **cloudformation-diff:** ignore changes to DependsOn ([aws#1005](aws#1005)) ([3605f9c](aws@3605f9c)), closes [aws#274](aws#274) * **cloudformation-diff:** track replacements ([aws#1003](aws#1003)) ([a83ac5f](aws@a83ac5f)), closes [aws#1001](aws#1001) * **docs:** fix EC2 readme for "natgatway" configuration ([aws#994](aws#994)) ([0b1e7cc](aws@0b1e7cc)) * **docs:** updates to contribution guide ([aws#997](aws#997)) ([b42e742](aws@b42e742)) * **iam:** Merge multiple principals correctly ([aws#983](aws#983)) ([3fc5c8c](aws@3fc5c8c)), closes [aws#924](aws#924) [aws#916](aws#916) [aws#958](aws#958) Features ========= * add construct library for Application AutoScaling ([aws#933](aws#933)) ([7861c6f](aws@7861c6f)), closes [aws#856](aws#856) [aws#861](aws#861) [aws#640](aws#640) [aws#644](aws#644) * add HostedZone context provider ([aws#823](aws#823)) ([1626c37](aws@1626c37)) * **assert:** haveResource lists failing properties ([aws#1016](aws#1016)) ([7f6f3fd](aws@7f6f3fd)) * **aws-cdk:** add CDK app version negotiation ([aws#988](aws#988)) ([db4e718](aws@db4e718)), closes [aws#891](aws#891) * **aws-codebuild:** Introduce a CodePipeline test Action. ([aws#873](aws#873)) ([770f9aa](aws@770f9aa)) * **aws-sqs:** Add grantXxx() methods ([aws#1004](aws#1004)) ([8c90350](aws@8c90350)) * **core:** Pre-concatenate Fn::Join ([aws#967](aws#967)) ([33c32a8](aws@33c32a8)), closes [aws#916](aws#916) [aws#958](aws#958) BREAKING CHANGES ========= * DynamoDB AutoScaling: Instead of `addReadAutoScaling()`, call `autoScaleReadCapacity()`, and similar for write scaling. * CloudFormation resource usage: If you use L1s, you may need to change some `XxxName` properties back into `Name`. These will match the CloudFormation property names. * You must use the matching `aws-cdk` toolkit when upgrading to this version, or context providers will cease to work. All existing cached context values in `cdk.json` will be invalidated and refreshed.

rix0rrr and others added 4 commits September 28, 2018 19:27

docs(aws-ec2): Explain Security Groups better

cf5f9c7

Explain how to add rules to Security Groups, and rewrite the section on the Connections object a bit. Fixes #799.

Mention that no egress rules = all traffic

ad7c31c

Update expectation

641d888

rix0rrr requested review from RomainMuller and eladb October 23, 2018 11:16

eladb approved these changes Oct 23, 2018

View reviewed changes

rix0rrr changed the title ~~fix(aws-ec2): SecurityGroups ensure outbound rules~~ fix(aws-ec2): fix SecurityGroup's all egress traffic rule Oct 23, 2018

moofish32 reviewed Oct 23, 2018

View reviewed changes

rix0rrr self-assigned this Oct 23, 2018

moofish32 mentioned this pull request Oct 24, 2018

feat(aws-eks): Add L2 Construct for Amazon EKS clusters and worker nodes #991

Closed

Rico Huijbers added 6 commits October 25, 2018 13:07

WIP

3fc6c75

Merge remote-tracking branch 'origin/huijbers/sg-docs' into huijbers/…

b168d0f

…explicit-outbound-asg

Rename AllConnections() -> AllTraffic()

1e04e0f

Merge remote-tracking branch 'origin/master' into huijbers/explicit-o…

ed013f6

…utbound-asg

Update expectations

e4b6b3e

Make allowAllOutbound configurable for Lambda

95a39df

rix0rrr merged commit b9d5b43 into master Oct 25, 2018

rix0rrr deleted the huijbers/explicit-outbound-asg branch October 25, 2018 13:40

rix0rrr mentioned this pull request Oct 26, 2018

v0.14.0 #1021

Merged

NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(aws-ec2): fix SecurityGroup's all egress traffic rule #998

fix(aws-ec2): fix SecurityGroup's all egress traffic rule #998

rix0rrr commented Oct 23, 2018

eladb Oct 23, 2018

rix0rrr Oct 23, 2018 •

edited

Loading

moofish32 left a comment

moofish32 Oct 23, 2018

rix0rrr Oct 23, 2018

moofish32 Oct 24, 2018

rix0rrr Oct 24, 2018

fix(aws-ec2): fix SecurityGroup's all egress traffic rule #998

fix(aws-ec2): fix SecurityGroup's all egress traffic rule #998

Conversation

rix0rrr commented Oct 23, 2018

eladb Oct 23, 2018

Choose a reason for hiding this comment

rix0rrr Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

moofish32 left a comment

Choose a reason for hiding this comment

moofish32 Oct 23, 2018

Choose a reason for hiding this comment

rix0rrr Oct 23, 2018

Choose a reason for hiding this comment

moofish32 Oct 24, 2018

Choose a reason for hiding this comment

rix0rrr Oct 24, 2018

Choose a reason for hiding this comment

rix0rrr Oct 23, 2018 •

edited

Loading