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

Change default behavior of ALB security group to not open up all ingress & egress #1286

Open
flostadler opened this issue May 8, 2024 · 5 comments
Labels

Comments

@flostadler
Copy link
Contributor

flostadler commented May 8, 2024

Hello!

  • Vote on this issue by adding a 👍 reaction
  • If you want to implement this feature, comment to let us know (we'll work with you on design, scheduling, etc.)

Issue details

Components should be secure by default, but right now a fully open security group (ingress&egress) is created when users create an ALB without specifying a security group. What makes this issue worse is that the load balancer is placed into a public subnet by default, ergo automatically exposing what's behind the ALB to the whole world.

Given that this default behavior is not documented there's a high chance that some users are unintentionally exposing their services to the internet.

Affected area/feature

This affects the ApplicationLoadBalancer component resource. This is where the default security group is created:

const securityGroup = new aws.ec2.SecurityGroup(
name,
defaultSecurityGroup?.args ?? {
// TO-DO: we default to open SG rules at this point - we should review this!
vpcId: this.vpcId,
ingress: [
{
fromPort: 0,
toPort: 0,
protocol: "-1",
cidrBlocks: ["0.0.0.0/0"],
ipv6CidrBlocks: ["::/0"],
},
],
egress: [
{
fromPort: 0,
toPort: 65535,
protocol: "tcp",
cidrBlocks: ["0.0.0.0/0"],
ipv6CidrBlocks: ["::/0"],
},
],
},
{ parent: this },
);

@flostadler flostadler added kind/enhancement Improvements or new features service/elb labels May 8, 2024
@flostadler
Copy link
Contributor Author

@t0yv0 @corymhall @mjeffryes Do you have thoughts about this? This would definitely be a breaking change, but I'd argue that this is actually good if a user has a fully opened up security group that they might not even know about.

@corymhall
Copy link
Contributor

@flostadler yeah we should probably change this. We shouldn't need any egress rules because security group rules are stateful. I think the ideal setup would be:

  1. Create a private LB by default
  2. If they create a public LB then default to not adding any security group rules.
  3. Extra flag to open the default security group on the Listener ports. If I am intentionally creating a public load balancer and I specify to open the default security group then I think its a good default to allow ingress on the ports for each of the listeners that are attached.

This would all be breaking changes though

@flostadler
Copy link
Contributor Author

We do need egress rules, otherwise the ALB is not allowed to talk to the destination (e.g. EC2 instances). But it shouldn't be completely open, rather scoped to the specific target. But that's information (target SG, port range and proto) we do not have access to within the component. We could scope it to the VPCs CIDR range by default, but that will inevitably get out of state if the VPC gets modified (e.g. adding a secondary range).

Generally I like the idea of the flag you mentioned in 3), but I fear that this could cause confusion for users as well. I've recently seen a couple of users attach listeners out of band (by adding a aws.lb.Listener resource) and that would break the listener port range detection.
The more I think about it, the more I think that adding any default rules will end up causing problems in one way or another.

What do you think about making the LB internal by default and then making the users be explicit about what firewall rules they add?

@corymhall
Copy link
Contributor

We do need egress rules, otherwise the ALB is not allowed to talk to the destination (e.g. EC2 instances). But it shouldn't be completely open, rather scoped to the specific target.

Ah yep you are right.

I've recently seen a couple of users attach listeners out of band (by adding a aws.lb.Listener resource) and that would break the listener port range detection.

I think we definitely have to allow for adding things out of band, but I think the default/best experience should be assuming the user is not, otherwise the abstraction becomes less and less useful.

What do you think about making the LB internal by default and then making the users be explicit about what firewall rules they add?

I still think we should find a way to add some default rules, even if it is for a more narrow case. If we don't have access to the information to do so then there is nothing we can do, but I wonder if we could refactor the components to allow for it?

@ZacHigi
Copy link

ZacHigi commented Jun 18, 2024

Just commenting as a customer of this module I would love it if Crosswalk not only handled this but also other default VPC related settings that AWS creates by default. Security scanners flag the default VPC, it's permissive NACLs, open gateway, etc.

Currently my project has a custom VPC created, then I add the ported special TF module for clearing the rules from the default security group with a load of comments to make it clear what the heck is going on:

//This is written this way to bring the default security group under management, and by doing so clear all the rules from it.
    const defaultGroup = new aws.ec2.DefaultSecurityGroup("defaultSecurityGroupResource", {
        vpcId: customVpc.vpcId
    });

Then, I've added some custom code using the AWS SDK to detect and detach the default internet gateway, then delete it and delete the default VPC.

I hate that AWS creates these things, and it would be amazing if this module had a 'destroyDefaultVPC' parameter that I could set that would do these kinds of things for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants