fix(ec2): Allow ingress to VPC interface endpoints #4938

joehillen · 2019-11-09T01:31:39Z

The default rule created for VPC endpoints did not have any ingress
rules meaning that the VPC endpoints were totally inaccessible.
This made them completely useless.

This commit add a default ingress rule that allows all IPv4 and IPv6
traffic to the port of the service.

fixes #4937

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

mergify · 2019-11-09T01:32:16Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-11-09T01:39:08Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

joehillen · 2019-11-09T01:42:46Z

The build failed because changing the security group is a security related change. I don't know what I need to do to fix this.

jogold · 2019-11-09T08:26:39Z

How about a allowAllInbound prop (implemented in the code with the connections object)?

See my comment here on why I think that allowing all inbound traffic is not a good default, this is especially true in the context of the CDK where security groups allow all outbound traffic by default.

rix0rrr · 2019-11-11T13:11:30Z

This seems to be a common enough issue that trips people up.

Maybe addInterfaceEndpoint() should take an option (open?: boolean like load balancers do) to allow all addresses to connect to it?

Given that most security groups will be defaulted to "allow all outbound", it will Just Work (which we prize highly), and given that the endpoint will only be routable from the VPC, not much of a security risk.

People who care enough about locking down to the highest degree will still have the opportunity of doing so by setting open: false and doing everything by hand. I think it is a good compromise. Thoughts?

jogold · 2019-11-11T14:33:05Z

Yes adding a prop and changing the default is fine. I would call this allowAllInbound as suggested.

rix0rrr · 2019-11-11T16:26:54Z

allowAllInbound would mirror allowAllOutbound on a security group, but I would add this property not to the SG but to the interface endpoint.

In that sense I like open better as it mimics the property Load Balancers have, also allowing traffic from all sources.

joehillen · 2019-11-11T16:31:37Z

@jogold allowAllInbound does not exist

@rix0rrr The open prop should not be necessary as the default security group is only used if no security group is passed to the VPC endpoint. If a user wants to restrict access, they can just specify their own security group. In the open = false if there are no ingress rules, these endpoints are completely inaccessible and there is no point in creating them in the first place. It definitely will not just work unless in the open = true state.

There isn't a security risk by allowing inbound traffic as most use cases are to set up an VPC Endpoint for AWS Services such as those listed here. These services are accessible to the open internet and already have an access control mechanism. Inside the VPC, the endpoints work the same and are only accessible inside the VPC.

jogold · 2019-11-11T16:35:13Z

allowAllInbound would mirror allowAllOutbound on a security group, but I would add this property not to the SG but to the interface endpoint.

Yes, I'm also talking about adding this prop to the interface endpoint.

In that sense I like open better as it mimics the property Load Balancers have, also allowing traffic from all sources.

OK

@joehillen something like:

if (open) {
  this.connections.allowDefaultPortFromAnyIpv4()
}

@rix0rrr or .allowFromAnyIpv4(ec2.Port.allTraffic())? Some endpoints work with multiple ports...

joehillen · 2019-11-11T16:39:12Z

Some endpoints work with multiple ports...

@jogold If that's true, then that is not reflected in the type. https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ec2/lib/vpc-endpoint.ts#L202

jogold · 2019-11-11T16:41:39Z

Some endpoints work with multiple ports...

@jogold If that's true, then that is not reflected in the type. https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ec2/lib/vpc-endpoint.ts#L202

This is only to define the defaultPort for connections, which is unique.

The default rule created for VPC endpoints did not have any ingress rules meaning that the VPC endpoints were totally inaccessible. This made them completely useless. This commit add a default ingress rule that allows all IPv4 and IPv6 traffic to the port of the service. fixes aws#4937

aws-cdk-automation · 2019-11-13T17:04:58Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-11-25T15:06:07Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

The default rule created for VPC endpoints did not have any ingress rules meaning that the VPC endpoints were totally inaccessible. This made them completely useless. Add `open` property to control security group ingress. Default is true which means allow all IPv4 and IPv6 traffic to the service port. fixes aws#4937

joehillen · 2019-11-25T17:23:05Z

Thank you @rix0rrr! I'm happy with this. I've squashed and rebased off of master.

aws-cdk-automation · 2019-11-25T17:50:54Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

…nto pr/joehillen/4938

aws-cdk-automation · 2019-11-26T13:14:15Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr · 2019-11-26T14:28:33Z

After discussing with AWS security, they requested the default be false.

joehillen · 2019-11-26T16:29:50Z

Can't argue with that logic. It's the most secure if nothing can talk to it.

rix0rrr · 2019-11-27T09:12:19Z

I'm going back to them to really understand the possible attacks. I'd still prefer to have the default the "it works out of the box" experience.

joehillen · 2019-12-03T18:29:24Z

This is exactly why I argued against the open flag. It only complicates the interface and makes it error prone when the user can and should supply their own security groups if they don't agree with the default. AWS VPC Interface Endpoints have their own authorization mechanism as well as VPC Endpoint Policies.

To give an example, I had lambda functions that were running outside of a VPC. While outside the VPC they were able to talk to services like S3, SSM, SQS, SNS, and APIGW without extra configuration or network access except for the IAM permissions for the specific resources. After configuring to run them inside a VPC, I had to add to create the VPC Interface Endpoints so that these lambda functions could talk to those services. The extra restriction of the security group adds nothing.

https://docs.aws.amazon.com/vpc/latest/userguide/vpc-endpoints-access.html#vpc-endpoints-security-groups

By default, Amazon VPC security groups allow all outbound traffic, unless you've specifically restricted outbound access.

When you create an interface endpoint, you can associate security groups with the endpoint network interface that is created in your VPC. If you do not specify a security group, the default security group for your VPC is automatically associated with the endpoint network interface. You must ensure that the rules for the security group allow communication between the endpoint network interface and the resources in your VPC that communicate with the service.

According to this, a new security group shouldn't even be created, it should use the default security group for the VPC by default.

I'll open a new PR for that instead.

jogold · 2019-12-03T20:58:29Z

According to this, a new security group shouldn't even be created, it should use the default security group for the VPC by default.

I wouldn't rely on the default security group because you don't know what ingress rules are currently configured in it (it could be that it has been modified since VPC creation).

joehillen · 2019-12-03T21:08:07Z

@jogold That's the behavior of AWS. You should take that up with whoever designed VPC Endpoints

joehillen · 2019-12-03T21:10:06Z

securityGroupIds isn't even a required argument on CfnVpcEndpoint, so there should have never been a default security group in the first place.

rix0rrr · 2019-12-13T13:12:39Z

@joehillen The defaults of CDK do not need to mirror the defaults for AWS. The defaults for CDK are more like sensible defaults that most people should want. So, while the default for AWS is "if you don't specify anything you all share the default security group", the default for CDK is "if you don't specify anything, each resource is going to get its own individual security group".

@slipdexic I guess we have a difference of mental model here that is worth exploring further. I guess your mental model is "I will prepare a SG with all the rules I want to see, then I pass it into a construct and it should stay as is". My mental model was "I let the SG be created (maybe I will I pass an SG in to share the same SG between constructs) and afterwards I will tell the construct where to allow traffic from (which incidentally changes the SG, but that is not my primary focus).

Where is your restrictive (10.0.2.0/23) Security Group coming from? Is it created in the same app, or does it already exist, preconfigured, and you're importing it?

I guess we satisfy both use cases at the same time if security groups had a mutable property like Roles do, so you can make your restricted, immutable security group and pass it in (so the default rules would not be added), or you don't and then they will.

I don't find the linked OWASP vulnerability very enlightening. "Missing appropriate hardening" seems to apply, but it's none too specific about what it thinks is "appropriate". The 99% use case of interface endpoints will be exposing AWS services inside your VPC. Those are already hardened because they can just as easily be hit from the Internet (and even if they weren't, it's not even your problem). And even for other kinds of endpoints, the attack surface is still limited as it will be scoped to "your VPC".

The AWS security department has required that the security group rule be restricted to the VPC CIDR if it is to be added by default, so that's a change I intend to make to this PR.

mergify · 2019-12-16T11:06:08Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-12-16T11:11:40Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2019-12-16T12:34:06Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-12-16T12:49:05Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2019-12-16T13:01:19Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T13:53:26Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:39:10Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T14:52:54Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

mergify · 2019-12-16T15:18:10Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-12-18T10:33:05Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

Pull request has been modified.

aws-cdk-automation · 2019-12-18T12:30:27Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

joehillen · 2019-12-19T17:53:57Z

@rix0rrr Thank you so much for your hard work to make this happen and for taking the problem seriously! <3

joehillen requested a review from rix0rrr as a code owner November 9, 2019 01:31

rix0rrr added the SECURITY label Nov 11, 2019

SomayaB added @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud and removed @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud labels Nov 11, 2019

SomayaB assigned rix0rrr Nov 11, 2019

joehillen mentioned this pull request Nov 12, 2019

Security Group for VPC Endpoint does not have any Ingress rules. #4937

Closed

joehillen force-pushed the vpc-endpoint-ingress branch from fc75423 to d110e20 Compare November 13, 2019 16:57

Add open property to control security group defaults

1561352

joehillen force-pushed the vpc-endpoint-ingress branch from 1561352 to 95481c9 Compare November 25, 2019 17:22

rix0rrr added 2 commits November 26, 2019 13:50

Make the default for 'open' false.

538d351

Merge branch 'vpc-endpoint-ingress' of github.com:joehillen/aws-cdk i…

f133be9

…nto pr/joehillen/4938

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Nov 26, 2019

Restrict allowed traffic to traffic originating from VPC

a665fb6

joehillen requested review from eladb and RomainMuller as code owners December 16, 2019 11:05

Update expectation

703ce40

rix0rrr previously approved these changes Dec 17, 2019

View reviewed changes

Update docs and tests to reflect new default

f5ddf9c

Merge branch 'master' into vpc-endpoint-ingress

a3540cf

rix0rrr approved these changes Dec 19, 2019

View reviewed changes

rix0rrr merged commit d5ed97a into aws:master Dec 19, 2019

fix(ec2): Allow ingress to VPC interface endpoints #4938

fix(ec2): Allow ingress to VPC interface endpoints #4938

Conversation

joehillen commented Nov 9, 2019

mergify bot commented Nov 9, 2019

aws-cdk-automation commented Nov 9, 2019

AWS CodeBuild CI Report

joehillen commented Nov 9, 2019

jogold commented Nov 9, 2019 • edited Loading

rix0rrr commented Nov 11, 2019

jogold commented Nov 11, 2019

rix0rrr commented Nov 11, 2019

joehillen commented Nov 11, 2019

jogold commented Nov 11, 2019 • edited Loading

joehillen commented Nov 11, 2019

jogold commented Nov 11, 2019

aws-cdk-automation commented Nov 13, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 25, 2019

AWS CodeBuild CI Report

joehillen commented Nov 25, 2019

aws-cdk-automation commented Nov 25, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Nov 26, 2019

AWS CodeBuild CI Report

rix0rrr commented Nov 26, 2019

joehillen commented Nov 26, 2019

rix0rrr commented Nov 27, 2019

joehillen commented Dec 3, 2019 • edited Loading

jogold commented Dec 3, 2019

joehillen commented Dec 3, 2019

joehillen commented Dec 3, 2019

rix0rrr commented Dec 13, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 16, 2019

AWS CodeBuild CI Report

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 16, 2019

AWS CodeBuild CI Report

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

mergify bot commented Dec 16, 2019

aws-cdk-automation commented Dec 18, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Dec 18, 2019

AWS CodeBuild CI Report

joehillen commented Dec 19, 2019

jogold commented Nov 9, 2019 •

edited

Loading

jogold commented Nov 11, 2019 •

edited

Loading

joehillen commented Dec 3, 2019 •

edited

Loading