aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.71k stars 3.93k forks source link

aws-cdk-lib/aws-ecs-patterns: Default Values for `publicLoadBalancer` and `assignPublicIp` Should Follow Least Privilege Principle in `aws-cdk-lib/aws-ecs-patterns` #32274

Closed axwilliamson-godaddy closed 3 days ago

axwilliamson-godaddy commented 4 days ago

Describe the feature

The default values for publicLoadBalancer and assignPublicIp in the AWS CDK library, specifically within the aws-ecs-patterns module, do not follow the principle of least privilege. The publicLoadBalancer property in the base class defaults to true, which can lead to the creation of public load balancers for internal services. To adhere to the least privilege principle, the default value for publicLoadBalancer should be false.

Setting the default to false would avoid accidentally creating public services and ensure that services are private by default unless explicitly configured otherwise.

Affected Classes

Use Case

Steps to Reproduce

  1. Create an ApplicationLoadBalancedFargateService without specifying publicLoadBalancer or assignPublicIp.
  2. Observe that the load balancer is created as internet-facing (publicLoadBalancer is true), but the Fargate service does not get a public IP (assignPublicIp is false).

Expected Behavior

The default values for publicLoadBalancer and assignPublicIp should follow the least privilege principle. The publicLoadBalancer should default to false to ensure services are private by default.

Proposed Solution

Proposed Solution

Set the default value of publicLoadBalancer to false to ensure services are private by default. I realize this has wide implications, so this could potentially be a candidate for a feature flag that allows the developer to override the default value of publicLoadBalancer.

Example Code

this.publicLoadBalancer = props.publicLoadBalancer ?? false;

or

this.publicLoadBalancer = props.publicLoadBalancer ?? FeatureFlags.of(this).isEnabled(cxapi.ALB_SERVICE_BASE_HAS_PUBLIC_LB_BY_DEFAULT)  // The default value of the feature flag would remain true.

Other Information

Environment

Additional Context

This change would ensure that services are private by default, adhering to the principle of least privilege and avoiding the accidental creation of public services. If this makes sense and I'm not missing anything obvious, I am happy to make the change and submit it as a PR

Acknowledgements

CDK version used

2.151.0

Environment details (OS name and version, etc.)

macOS 14.6.1

pahud commented 4 days ago

Agree!

https://github.com/aws/aws-cdk/blob/c5f895e3cfb3d17b39cad917e8c20560bd5c7824/packages/aws-cdk-lib/aws-ecs-patterns/lib/base/network-load-balanced-service-base.ts#L61-L63

We should get it fixed with a PR and a feature flag. We welcome the PRs to move it forward.

axwilliamson-godaddy commented 4 days ago

Thank you for the quick response, I have created a PR for this: https://github.com/aws/aws-cdk/pull/32276.

hoegertn commented 3 days ago

Can we have a short discussion before jumping to adding new feature flags?

I would assume that the default case of an ALB is to make things available to the public.

In a scenario where you have internal services, you most likely do not have public subnets in your VPC because of a north/south networking concept.

So, I am not sure we should switch the default behavior.

axwilliamson-godaddy commented 3 days ago

@hoegertn Thank you for the thoughtful reply! I’m more than happy to discuss this further to align on the best approach. I completely understand the concern around switching the default behavior.

That said, my primary goal is to ensure there’s at least some mechanism—whether via a feature flag or another solution—that allows for overriding and defaulting to private. Even if we decide to keep the default behavior as true, it would help immensely to have a recommended, configurable way to enforce private load balancers where needed.

In our case, we work with VPCs that include public, private, and direct-connect subnets. We’ve encountered multiple instances where teams accidentally create public load balancers for private services by overlooking the props.publicLoadBalancer = false setting. If a feature flag is introduced, we could leverage it in our internal repository templates to reduce the risk of accidental misconfigurations.

hoegertn commented 3 days ago

I fully get your point as most of my customers are in regulated industries. In this cases I normally also use some automated checks for my CDK apps like cdk-nag that would stop synth until I either set it to private or mark it as an exception.

The benefit I see is education instead of silently overriding the documented value people will read in the docs.

Also I often use customer-specific L3s for the e.g. ApplicationLoadBalancedFargateService that could incorporate all these defaults you want to enforce.

axwilliamson-godaddy commented 3 days ago

Thank you for the comment. I'll close this.

github-actions[bot] commented 3 days ago

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.