Configure network ACLs for public/private/intra subnets

[kinghuang]

Network ACLs provide a layer of security to VPCs with stateless rules for inbound and outbound network connections. This PR adds support for configuring network ACLs for public, private, and intra subnets using a series of variables.

Here is an example that implements some of the rules from Recommended Rules for Scenario 2.

module "vpc" {
  source = "terraform-aws-modules/vpc/aws"
  version = "1.x.x"

  name = "acl-example"
  cidr = "10.64.0.0/16"

  azs              = ["us-east-2a", "us-east-2b", "us-east-2c"]
  private_subnets  = ["10.64.0.0/20", "10.64.16.0/20", "10.64.32.0/20"]
  public_subnets   = ["10.64.192.0/22", "10.64.196.0/22", "10.64.200.0/22"]

  public_inbound_acls = [
    "100", "allow", 80,    80,    "tcp", "0.0.0.0/0",       "Allows inbound HTTP traffic from any IPv4 address",
    "110", "allow", 443,   443,   "tcp", "0.0.0.0/0",       "Allows inbound HTTPS traffic from any IPv4 address",
    "120", "allow", 22,    22,    "tcp", "198.51.100.0/24", "Allows inbound SSH traffic from your home network",
    "130", "allow", 3389,  3389,  "tcp", "198.51.100.0/24", "Allows inbound RDP traffic from your home network",
    "140", "allow", 1024,  65535, "tcp", "0.0.0.0/0",       "Allows inbound return traffic from hosts"
  ]

  public_outbound_acls = [
    "100", "allow", 80,    80,    "tcp", "0.0.0.0/0",       "Allows outbound HTTP traffic from the subnet to the Internet",
    "110", "allow", 443,   443,   "tcp", "0.0.0.0/0",       "Allows outbound HTTPS traffic from the subnet to the Internet",
    "120", "allow", 1433,  1433,  "tcp", "10.64.0.0/17",    "Allows outbound MS SQL access to database servers in the private subnet",
    "140", "allow", 32768, 65535, "tcp", "0.0.0.0/0",       "Allows outbound responses to clients on the Internet",
    "150", "allow", 22,    22,    "tcp", "10.64.0.0/17",    "Allows outbound SSH access to instances in your private subnet"
  ]
}

aws_network_acl and aws_network_acl_rule resources are created for the ACL rules. An aws_default_network_acl resource is also created to adopt the default network ACL in the VPC.

Closes #173.

[kinghuang]

On further thought, maybe this is better done as a separate module. Otherwise, there's a lot of repetition involved, especially if the database, redshift, and elasticache subnets are covered, too.

[antonbabenko]

Hi King, I like the code you have pushed so far very much.

I think it is important to provide all NACL rules in this module even if there will be some repetitions. The users of this module will benefit from having more features using one module.

Thank you for your contribution!

[kinghuang]

I've added ACL resources to cover database, redshift, and elasticache subnets. And, refined the variable names to better reflect that they configure ACL rules, not ACLs.

[iveskins]

When I try to use this branch I get an error:
Error: module "vpc": "public_outbound_acls" is not a valid argument

Maybe this is because the variables are defined like "public_inbound_acl_rules", with '_rules' but the examples are written as "public_inbound_acls = ["
?

[iveskins]

I also got some strange errors, and couldn't refresh my state, after I inserted a new rule into the public_inbound_acls list
from:

 public_inbound_acls = [
    "900", "allow", 3389,  3389,  "tcp", "198.51.100.0/24", "Allows inbound RDP traffic from your home network",
    "901", "allow", 1024,  65535, "tcp", "0.0.0.0/0",       "Allows inbound return traffic from hosts"
  ]

to:

 public_inbound_acls = [
    "200", "allow", 22,    22,    "tcp", "198.51.100.0/24", "Allows inbound SSH traffic from your home network",
    "900", "allow", 3389,  3389,  "tcp", "198.51.100.0/24", "Allows inbound RDP traffic from your home network",
    "901", "allow", 1024,  65535, "tcp", "0.0.0.0/0",       "Allows inbound return traffic from hosts"
  ]

it seems like terraform was keeping track of the rules by their position the the list not the priority number.. so adding an item at the beginning caused problems

ws_route.private_nat_gateway[1]: Refreshing state... (ID: r-rtb-0c9743f91b1aa428c1080289494)

Error: Error refreshing state: 1 error(s) occurred:

* module.vpc.aws_network_acl_rule.public_inbound: 1 error(s) occurred:

* module.vpc.aws_network_acl_rule.public_inbound[4]: aws_network_acl_rule.public_inbound.4: Expected the Network ACL to have Entries, got: {
  Associations: [{
      NetworkAclAssociationId: "aclassoc-",
      NetworkAclId: "acl-",
      SubnetId: "subnet-"
    }],
  Entries: [
...

[dng-dev]

When you dig into the code here is the line which creates the internal hash. So as you see they are using the rule number for this.

d.SetId(networkAclIdRuleNumberEgressHash(d.Get("network_acl_id").(string), d.Get("rule_number").(int), d.Get("egress").(bool), d.Get("protocol").(string)))

[tbugfinder]

@kinghuang Will you finished on that pull request, looks like there are conflicts?

[ppieprzycki]

Any plans for merging this PR into master?

[antonbabenko]

@ppieprzycki Yes, there are plans but I have not enough time in a day to cover all projects I work for free.

Regarding this PR, it requires some improvements related to conditional creation. I will fix and test it manually.

[kinghuang]

Sorry, I forgot about the changes. I can rebase and take a stab at that today.

[jczerniak]

hi, why not use lookup function instead of element function?

With lookup function, our nacl definition is more readable and formatable.

1. replace count
   from
   count = "${var.create_vpc && length(var.public_subnets) > 0 ? length(var.public_outbound_acl_rules) / 7 : 0}"
   to
   count = "${var.create_vpc && length(var.public_subnets) > 0 ? length(var.public_outbound_acl_rules) : 0}"

2. replace element with lookup function
   from
   rule_number = "${element(var.public_inbound_acl_rules, count.index * 7 + 0)}"
   to
   rule_number = "${lookup(var.public_outbound_acl_rules[count.index], "rule_number")}"

3. define nacl definitions with local variables

locals {
  network_acls_public = {
    public_inbound = [
      {
        rule_number = 100
        egress      = false
        protocol    = "tcp"
        rule_action = "allow"
        cidr_block  = "0.0.0.0/0"
        from_port   = 2222
        to_port     = 2222
      },
      {
        rule_number = 200
        egress      = true
        protocol    = "tcp"
        rule_action = "allow"
        cidr_block  = "0.0.0.0/0"
        from_port   = 1024
        to_port     = 65535
      },
    ]
   public_outbound = [
      {
        rule_number = 100
        egress      = true
        protocol    = "tcp"
        rule_action = "allow"
        cidr_block  = "0.0.0.0/0"
        from_port   = 23
        to_port     = 23
      },
      {
        rule_number = 200
        egress      = true
        protocol    = "tcp"
        rule_action = "allow"
        cidr_block  = "0.0.0.0/0"
        from_port   = 1024
        to_port     = 65535
      },
    ]
  }
network_acls_default = {
    default_inbound = [
      {
        rule_number = 10
        egress      = false
        protocol    = "tcp"
        rule_action = "allow"
        cidr_block  = "0.0.0.0/0"
        from_port   = 2222
        to_port     = 2222
      },
      {
        rule_number = 20
        egress      = true
        protocol    = "tcp"
        rule_action = "allow"
        cidr_block  = "0.0.0.0/0"
        from_port   = 1024
        to_port     = 65535
      },
    ]
    default_outbound = [
      {
        rule_number = 10
        egress      = true
        protocol    = "tcp"
        rule_action = "allow"
        cidr_block  = "0.0.0.0/0"
        from_port   = 2222
        to_port     = 2222
      },
      {
        rule_number = 20
        egress      = true
        protocol    = "tcp"
        rule_action = "allow"
        cidr_block  = "0.0.0.0/0"
        from_port   = 1024
        to_port     = 65535
      },
    ]
  }
}

4. invoke module with local variable

• with defaults
  public_inbound_acl_rules = "${local.network_acls_default["default_inbound"]}"

• custom with defaults
  public_inbound_acl_rules = "${concat(local.network_acls_default["default_inbound"],local.network_acls_public["public_inbound"]}"

I think that this will give more flexibility in a more complex environment

Regards
Jacek

[antonbabenko]

I like the structure and ideas described by @jczerniak very much.

@kinghuang will you be able to update this PR?

[kinghuang]

Following @jczerniak's suggestions, I've change the rules from lists of lists to lists of maps. And, updated the network-acls example to use locals and a concatenation of "default" and "custom" rules, instead of inline rules.

[antonbabenko]

v1.60.0 has been released!

Huge thanks to @kinghuang for the great contribution!

[github-actions]

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.