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

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
2 tasks done
axwilliamson-godaddy opened this issue Nov 25, 2024 · 7 comments
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2

Comments

@axwilliamson-godaddy
Copy link

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

  • ApplicationLoadBalancedServiceBase (file: packages/aws-cdk-lib/aws-ecs-patterns/lib/base/application-load-balanced-service-base.ts)
  • ApplicationLoadBalancedFargateService (file: packages/aws-cdk-lib/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts)

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

  • CDK Version: 2.151.0
  • Language: TypeScript

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

  • I may be able to implement this feature request
  • This feature might incur a breaking change

CDK version used

2.151.0

Environment details (OS name and version, etc.)

macOS 14.6.1

@axwilliamson-godaddy axwilliamson-godaddy added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Nov 25, 2024
@github-actions github-actions bot added the @aws-cdk/aws-ecs-patterns Related to ecs-patterns library label Nov 25, 2024
@pahud pahud self-assigned this Nov 25, 2024
@pahud
Copy link
Contributor

pahud commented Nov 25, 2024

Agree!

* @default true
*/
readonly publicLoadBalancer?: boolean;

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

@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Nov 25, 2024
@pahud pahud removed their assignment Nov 25, 2024
axwilliamson-godaddy added a commit to axwilliamson-godaddy/aws-cdk that referenced this issue Nov 25, 2024
…or of creating public load balancers

If this flag is not set, the default behavior for `ApplicationLoadBalancedFargateService` and `NetworkLoadBalancedFargateService` is to create a public load balancer (not changed).

If this flag is set to false, the behavior is that the load balancer will be private by default.

This is a feature flag as to keep compatibility with the old behavior.

Relevant issue: aws#32274
@axwilliamson-godaddy
Copy link
Author

Thank you for the quick response, I have created a PR for this: #32276.

axwilliamson-godaddy added a commit to axwilliamson-godaddy/aws-cdk that referenced this issue Nov 25, 2024
…or of creating public load balancers

If this flag is not set, the default behavior for `ApplicationLoadBalancedFargateService` and `NetworkLoadBalancedFargateService` is to create a public load balancer (not changed).

If this flag is set to false, the behavior is that the load balancer will be private by default.

This is a feature flag as to keep compatibility with the old behavior.

Relevant issue: aws#32274
axwilliamson-godaddy added a commit to axwilliamson-godaddy/aws-cdk that referenced this issue Nov 25, 2024
…or of creating public load balancers

If this flag is not set, the default behavior for `ApplicationLoadBalancedFargateService` and `NetworkLoadBalancedFargateService` is to create a public load balancer (not changed).

If this flag is set to false, the behavior is that the load balancer will be private by default.

This is a feature flag as to keep compatibility with the old behavior.

Relevant issue: aws#32274
axwilliamson-godaddy added a commit to axwilliamson-godaddy/aws-cdk that referenced this issue Nov 25, 2024
…or of creating public load balancers

If this flag is not set, the default behavior for `ApplicationLoadBalancedFargateService` and `NetworkLoadBalancedFargateService` is to create a public load balancer (not changed).

If this flag is set to false, the behavior is that the load balancer will be private by default.

This is a feature flag as to keep compatibility with the old behavior.

Relevant issue: aws#32274
@hoegertn
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Author

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

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 26, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@aws-cdk/aws-ecs-patterns Related to ecs-patterns library effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2
Projects
None yet
3 participants