-
Notifications
You must be signed in to change notification settings - Fork 9.6k
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
Remove default allow-all egress rule for VPC secgrp if egress rules are specified #1169
Comments
Closing this... user error. It appears that Terraform does to the right thing with the default rule and my script has been lying to me. |
Re-opening as now Im definitely seeing this happening in my deployed configurations. Specifying egress rules for a security group does not remove the default egress allow-all rule Terraform should remove the security group default egress rule if any egress rules are specified. Working on putting together a minimal test configuration now to demonstrate. |
Thanks for the report. This makes sense. Marking as a bug. That test configuration you mentioned will be very helpful. |
Here is the minimal reproduction case that demonstrates the issue:
If you then go to the AWS console and look at the security group egress rules you see this:
That second rule is the one should be automatically removed. |
Also a word of caution here for people using egress rules as well... I had assumed all my security group egress rules in my configuration were working as expected until I discovered that they were all being overridden by the default allow-all egress rule. When I manually deleted all these allow-all default rules I discovered a number of things that were incorrect with my original egress rules that blocked traffic but were hidden by the default rules. Easy to get these wrong when you are doing a bunch of NAT stuff with tiered configurations. Others may discover such issues with their deployments once egress rules are actually enforced :-) |
I believe this is an issue with Cloudformation in general (https://forums.aws.amazon.com/message.jspa?messageID=413748). It's not clear to me that it is possible to fix this within the cloudformation framework (aside from making a dummy egress rule to an illegal address like 192.0.2.0/32 |
CloudFormation does not have any way to create a security group with no egress rules at all where providing a dummy rule is the only way to avoid getting the default rule. However, for the more common case where you do specify explicit egress rules it does correctly remove the default rule and replaces it with the specified rules. This was not always the case but was fixed some time ago in CloudFormation. I think Terraform could just apply the same logic as CloudFormation that will cover the general case and follow the principle of least surprise by leaving the default rule in place if no egress rules are specified. |
Proposing #1765 as a fix to this |
I removed the default egress rule by using the following command: aws ec2 revoke-security-group-egress --group-id "sg-xxxxxxxx" --protocol all --port all --cidr 0.0.0.0/0 |
For boto it's: |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
VPC security groups are by default created with an allow-all egress rule. This rule has to be explicitly removed or it will defeat any other explicit egress rules specified for the group. There does not seem to be any way (or maybe I have just not found it yet) to remove this rule automatically with Terraform. I end up having to run scripts to explicitly remove it from VPC security groups after deploying with Terraform, which should not be necessary.
CloudFormation solves this problem by automatically removing the default allow-all egress rule from a VPC security group if any explicit egress rules are specified for the security group. I suggest this as an intuitive solution for the problem that could also be adopted by Terraform.
The text was updated successfully, but these errors were encountered: