fix: Allow default_security_group to have no rules #1123
Conversation
britthouser
changed the title
britthouser
force-pushed
the
empty-default-security-group
branch
from
722e54a
to
c4ee6f8
Compare
The current implementation of the aws_default_security_group uses dynamic blocks for both ingress and egress rules. If no rules are passed in, then no dynamic blocks are generated, and no pre-existng rules are changed. In order to implement the CIS benchmark of no rules, the aws_default_security_group resorce needs to be created passing empty lists as ingress and egress rules. This commit updates the default ingress/egress rules to be those AWS uses when it initially created the default SG. It then sets the boolen local.empty_default_security_group if both ingress and egress rules passed in are empty lists. Finally it conditionally creates empty aws_default_security_group resource if local.empty_default_security_group is true. If local.empty_default_security_group is false, the original aws_default_security_group resource is utilized.
britthouser
force-pushed
the
empty-default-security-group
branch
from
c4ee6f8
to
0b0b3c8
Compare
britthouser
force-pushed
the
empty-default-security-group
branch
from
1de84b0
to
6cebe30
Compare
|
@bryantbiggs - would love to get your thoughts on this implementation whenever you get a minute. |
|
@antonbabenko - could I get a review? |
|
I don't think this is a valid change that we should accept here. if you are concerned with CIS and security recommended practices, then I would suggest deleting all default VPCs and security groups created by the AWS account creation process and only use VPCs you've defined and created within Terraform. There are a number of bash/python scripts on the internet that will go through and delete these resources for you, and I believe on Control Tower you now have the option to NOT create these resources when creating/vending new accounts |
|
A point of clarification, this isn't about default VPC(s) AWS creates, but the VPC default security group, which is part of EVERY VPC creation, even via terraform. There is no way to delete this security group. This VPC module has code to "adopt" the default security group already. This MR just updates the behavior when the user specifies empty security group rules. Can we take a second look @bryantbiggs ? |
|
you're starting with a PR and not an issue that describes the situation and provides a reproduction - I tried using one of our samples https://github.com/terraform-aws-modules/terraform-aws-vpc/blob/master/examples/simple/main.tf and I get a single security group with no inbound or outbound rules - so what more is missing here? |
|
Issue #759 describes the problem. I listed it under the motivations in the PR description, but if there a different way I should be linking it, I'm happy to do so. |
|
but again - if you are using Terraform, why should this module worry about things that you are creating outside of Terraform? this isn't a module issue - its a user issue |
|
Modifying it outside of terraform was just an easy way to see the problem. It doesn't matter how the rules are added to the default security group. The problem manifests when you try to remove all the rules of the default security group. With the current implementation, its not possible to go from some rules to no rules. That's the behavior I'm trying to fix here. I do apologize if I've not been clear up to this point. But hopefully you can see the use case now: The default security group has had rules applied at some point in the past, and now we need to remove them. |
The problem though is exactly what you are doing - modifying things outside of Terraform. This is a known thing - so either you can stop doing that (strongly recommended practice when using IaC), or try to get the provider to support something that will ensure things created outside of Terraform are reconciled hashicorp/terraform-provider-aws#4426 |
|
Here is a demonstration of the problem, completely within terraform, using just this module. I start out by by doing a terratform apply of the examples/simple/main.tf, and pull the default security group ID from the outputs: Just to validate I read (not change) the default security group, and see that indeed it has no rules, as that is the module default. Now I modify the terraform to specify ingress and egress rules for the security group, and I run terraform apply again. Checking the default security group rules a second time, we can see that terraform apply updates the rules as requested: Next, I revert the terraform back to the main.tf as it exists in the repo, and re-run apply. Terraform reports not changes to the infra, when in fact I have specified a change! I hope you can see how this is a problem, even when only terraform is used to change the security group rules. With the code currently in the repo, you can NEVER go from some rules to no rules in the default security group. The PR I've proposed allows this change to happen. I hope that clarifies? |
|
hey - look at that! a proper reproduction with steps and output. imagine if we had that from the start |
|
however, your changes still are not valid at this time - that would be a breaking change to adopt the changes proposed (and I don't know if we would adopt those even if we were making a breaking change. If AWS changes the defaults at some point, now what do we do - if we don't have an opinion on a default, we simply do nothing)
default_security_group_ingress = []
default_security_group_egress = [] |
|
Okay - so we have established that there is indeed a problem here. So lets discuss what todo about it. I tried your workaround, and it does not remove the rules. I did a deployment using the examples/sample/main.tf with added some security group rules defined: As expected, terraform created the rules: I updated the terraform with your proposed change: As you can see, the apply still doesn't invoke any change: This is due to the fact that the empty list |
|
Please let me know what is not valid about the PR itself. I was on the fence about what to specify as the default values for |
as I stated above - go get it changed in the provider. If we follow your example
resource "aws_default_security_group" "this" {
vpc_id = module.vpc.vpc_id
ingress {
protocol = -1
self = true
from_port = 0
to_port = 0
}
egress {
from_port = 0
to_port = 0
protocol = "-1"
cidr_blocks = ["0.0.0.0/0"]
ipv6_cidr_blocks = ["::/0"]
}
}
resource "aws_default_security_group" "this" {
vpc_id = module.vpc.vpc_id
}And lastly - your suggested changes are semantically incorrect -
I am going to lock this issue because I don't have any additional time to continue down this path - its simply incorrect |
Description
The current implementation of the aws_default_security_group uses dynamic blocks for both ingress and egress rules. If no rules are passed in, then no dynamic blocks are generated, and no pre-existng rules are changed. In order to implement the CIS benchmark of no rules, the aws_default_security_group resource needs to be created passing empty lists as ingress and egress rules.
This commit updates the default ingress/egress rules to be those AWS uses when it initially created the default SG. It then sets the boolen local.empty_default_security_group if both ingress and egress rules passed in are empty lists. Finally it conditionally creates empty aws_default_security_group resource if local.empty_default_security_group is true. If local.empty_default_security_group is false, the original aws_default_security_group resource is utilized.
Motivation and Context
#759
hashicorp/terraform-provider-aws#20697
Breaking Changes
Yes and No. If the user has specified any custom security group rules, then those are honored and won't break. If the user has not specified any custom security group rules, and has not changed the default rules which AWS created when the security group was created, then those are preserved and won't break. But if the user has not specified any custom security group rules AND outside of terraform changed the rules to something different from the ones AWS supplies, then those will get reverted back to the AWS supplied rules.
How Has This Been Tested?
examples/*to demonstrate and validate my change(s)examples/*projectspre-commit run -aon my pull request