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

[flostadler]

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:

pulumi-awsx/awsx/lb/applicationLoadBalancer.ts

Lines 89 to 114 in adae402

	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]

@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]

@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]

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]

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?