fix(ec2) Add Ingress rule for VPC Endpoints & CIDR Blocks

[jd-carroll]

• Add ingress rule for VPC Endpoints (VPC Endpoint is not automatically connectable from instances inside VPC #4589)
• Expose CIDR block information on IVpc (Consider providing CIDR in IVpc #2232)

[mergify]

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]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

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

[jogold]

I'm not sure that allowing traffic from all addresses in the VPC CIDR is a good default here (security-wise).

One must use the connections object, as originally documented:

aws-cdk/packages/@aws-cdk/aws-ec2/test/integ.vpc-endpoint.lit.ts

Lines 38 to 41 in 0bce4d6

	// When working with an interface endpoint, use the connections object to
	// allow traffic to flow to the endpoint.
	ecrDockerEndpoint.connections.allowDefaultPortFromAnyIpv4();

[jd-carroll]

@jogold Can you provide an example where that is a security risk? I think the code you just referenced is more of a security risk than using the VPC CIDR, and I can't think of a scenario where I would want some resources in my VPC to use a private endpoint but others not.

The code changes here are more in-line with AWS documentation, such as:
https://aws.amazon.com/premiumsupport/knowledge-center/ec2-systems-manager-vpc-endpoints/

[aws-cdk-automation]

AWS CodeBuild CI Report

• Result: FAILED
• Build Logs (available for 30 days)

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

[jogold]

@jogold Can you provide an example where that is a security risk? I think the code you just referenced is more of a security risk than using the VPC CIDR, and I can't think of a scenario where I would want some resources in my VPC to use a private endpoint but others not.

The code I referenced is indeed not best practice but was just to illustrate the usage of the connections object.

Regarding the security: VPC endpoints allow you to deploy an application in a VPC completely isolated from the internet but still provide access to AWS services. It could be that in that isolated VPC you want to guarantee that your database can never access an AWS service (even if it has been compromised, bad code, injection) or can access one service but not another. Working with the connections object allows you to selectively choose who has access. Maybe adding a allowFromVpc prop, which would allow overriding the default, is the right solution here?

[jd-carroll]

@jogold - That is an awesome point, I did not think of that. I don’t know I have enough context on where all (or how) the Connections object is used, so I’m not going to try and fit these changes on to that (unless otherwise encouraged).

It’s probably fair to close this PR, but I’ll leave it open for now as a means to discuss options for #2232. If there are valid things in here which should be kept, happy to amend the PR as necessary. Otherwise it can just be closed.

[rix0rrr]

Why this change? What is wrong with attrCidrBlock?

[rix0rrr]

I do not feel comfortable with doing this outright. I would feel better if it's behind a flag (suggested name: allowAllVpcTraffic: true|false).

It would be nicer to implement as:

this.connections.allowDefaultPortFrom(Peer.ipv4(props.vpc.vpcCidrBlock));

But even doing this only works for securitygroups that have an outbound rule. If not, you would still need to do the following:

endpoint.connections.allowDefaultPortFrom(myAsg);

So is this even worth it?

[rix0rrr]

I'm all in favor of this, but be aware there is a concurrent PR changing the VPC provider, and these changes will have to be integrated across PRs: #4544

[jd-carroll]

@rix0rrr I generally feel that this PR could be closed, unless there is value related to #2232. I responded to one of your comments above, let me know what your thoughts are on how best to proceed.

[rix0rrr]

I responded to one of your comments above

I'm not seeing it?

[rix0rrr]

Closing this until we see a need to reopen.

[jk21]

@rix0rrr @jd-carroll with PR #4554 merged now, can this one be progressed? because i don't see the possibility to get the CIDRs of an imported VPC at the moment. (or am i missing sth.?)