WAFv2 Web ACL interface improvement, simplification, automation

[dvishniakov]

Community Note

• Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
• Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
• If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

I've stumbled upon significant interface change and much less dry configuration for dynamic ACLs and rules during upgrading a WAF module from using WAF Classic resources to WAFv2 resources. Below are a few examples. I understand that the way resources are represented in the provider are mostly mimicking upstream APIs, types and objects of GO SDK because in general it makes sense. But sometimes this means more work on customers, less dry and less readable configuration, less ability of automation.

WAFv2 resources use blocks instead of attributes a lot more. While it is possible to automate and interpolate blocks in Terraform >= 0.12 (I'm not going to mention 0.11 with its pros and cons), that is: 1) Even simple 1-level nested blocks look much less dry because of current Terraform/HCL limitations:

• it's not possible to interpolate block name. You have to specify each possible option for each block
• it's not possible to supply a predefined structure as a value for sub-block 2) Dealing with complex multiple nested levels is currently frustrating, affects performance even when you don't have any rules or statements defined (#14062), people will come up with even more complex and deep rules with more than 3 nested levels (#14377).

Some of the options which I can think of, but I'm sure folks more knowledgeable in the AWS SDK, provider and Terraform codebases can suggest more and even better:

• Use JSON input such as in ECS Container Definitions or IAM policies
• Wrap WAFv2 interface with a interface similar to WAF Classic for easier automation

New or Affected Resource(s)

• aws_wafv2_web_acl

Potential Terraform Configuration

Note: all code samples are simplified, most of insignificant attributes are removed for easier reading, so copy-paste of the code will not work without modifications.

WAF Classic

Example of one rule definition which could be nicely defined in a separate file:

# Manual blacklist
locals {
  manual_blacklist_name  = "manual_blacklist"
  manual_blacklist_count = var.enabled ? 1 : 0

  manual_blacklist_acl_rule = {
    action_type = var.blocking_rules_action != "" ? var.blocking_rules_action : var.manual_blacklist_rule_action
    priority    = var.manual_blacklist_priority
    rule_id     = element(concat(aws_waf_rule.manual_blacklist.*.id, list("")), 0)
    type        = "REGULAR"
  }
}

resource "aws_waf_rule" "manual_blacklist" {
  count = local.manual_blacklist_count

  predicates {
    data_id = element(concat(aws_waf_ipset.manual_blacklist.*.id, list("")), 0)
    negated = false
    type    = "IPMatch"
  }
}

resource "aws_waf_ipset" "manual_blacklist" {
  count = local.manual_blacklist_count

  dynamic "ip_set_descriptors" {
    for_each = var.manual_blacklist_ip_set_descriptors
    content {
      type  = ip_set_descriptors.value.type
      value = ip_set_descriptors.value.value
    }
  }
}

WAF Classic ACL:

locals {
  # Filtering enabled rules. All enabled and created rules will have RuleId as non-empty string.
  all_rules_global = [
    for rule in [
      local.manual_blacklist_acl_rule,
      local.manual_whitelist_acl_rule,
      local.rate_acl_rule,
      local.reputation_list_acl_rule,
      local.sql_injection_acl_rule,
      local.xss_acl_rule
    ] :
    rule
    if rule.rule_id != ""
  ]
}

resource "aws_waf_web_acl" "waf_global_acl" {
...
  default_action {
    type = var.acl_default_action_block ? "BLOCK" : "ALLOW"
  }

  dynamic "rules" {
    # THAT'S ALL! Wonderful, no need to have a complex dynamic blocks here
    for_each = local.all_rules_global
    content {
      action {
        type = rules.value.action_type
      }
      priority = rules.value.priority
      rule_id  = rules.value.rule_id
      type     = rules.value.type
    }
  }
...
}

WAFv2

Now, compare the simple WAF Classic interface with what WAFv2 requires for using externally managed rule group (no affiliation or advertisement, just a publicly available confirmation of what I've experienced): https://github.com/umotif-public/terraform-aws-waf-webaclv2/blob/master/main.tf#L10-L55 Which is only top of the iceberg. Actual rule definitions and nested statements should be defined elsewhere. Either with plain nested block statements or with a complex unreadable dynamic blocks if you want some automation.

Example of just one, top-level (not nested) dynamically-defined SQL injection rule. Dynamic XSS rule definition will look pretty similar, while other top-level rules are simpler. Is there a name for this similarly to callback hell? dynamic block hell"?

# SQL Injection https://github.com/awslabs/aws-waf-security-automations/blob/master/deployment/aws-waf-security-automations-webacl.template#L508-L559
  dynamic "rule" {
    for_each = local.sql_injection_rule_enabled ? [true] : []
    content {
      name     = local.sql_injection_rule_name
      priority = var.sql_injection_rule_priority

      action {
        dynamic "allow" {
          for_each = lower(var.sql_injection_rule_action) == "allow" ? [true] : []
          content {}
        }
        dynamic "block" {
          for_each = lower(var.sql_injection_rule_action) == "block" ? [true] : []
          content {}
        }
        dynamic "count" {
          for_each = lower(var.sql_injection_rule_action) == "count" ? [true] : []
          content {}
        }
      }

      # This is a nested mess.
      statement {
        or_statement {
          dynamic "statement" {
            for_each = {
              all_query_arguments         = ["URL_DECODE", "HTML_ENTITY_DECODE"],
              body                        = ["URL_DECODE", "HTML_ENTITY_DECODE"],
              uri_path                    = ["URL_DECODE", "HTML_ENTITY_DECODE"],
              single_header_Cookie        = ["URL_DECODE", "HTML_ENTITY_DECODE"],
              single_header_Authorization = ["URL_DECODE", "HTML_ENTITY_DECODE"],
            }
            content {
              sqli_match_statement {
                field_to_match {
                  dynamic "all_query_arguments" {
                    for_each = statement.key == "all_query_arguments" ? [true] : []
                    content {}
                  }
                  dynamic "body" {
                    for_each = statement.key == "body" ? [true] : []
                    content {}
                  }
                  dynamic "method" {
                    for_each = statement.key == "method" ? [true] : []
                    content {}
                  }
                  dynamic "single_header" {
                    for_each = substr(statement.key, 0, length("single_header_")) == "single_header_" ? [true] : []
                    content {
                      name = lower(replace(statement.key, "single_header_", ""))
                    }
                  }
                  dynamic "single_query_argument" {
                    for_each = substr(statement.key, 0, length("single_query_argument_")) == "single_query_argument_" ? [true] : []
                    content {
                      name = lower(replace(statement.key, "single_query_argument_", ""))
                    }
                  }
                  dynamic "query_string" {
                    for_each = statement.key == "query_string" ? [true] : []
                    content {}
                  }
                  dynamic "uri_path" {
                    for_each = statement.key == "uri_path" ? [true] : []
                    content {}
                  }
                }
                dynamic "text_transformation" {
                  for_each = statement.value
                  content {
                    priority = text_transformation.key +1
                    type     = text_transformation.value
                  }
                }
              }
            }
          }
        }
      }

      visibility_config {
        cloudwatch_metrics_enabled = var.cloudwatch_metrics_enabled
        metric_name                = local.sql_injection_rule_name
        sampled_requests_enabled   = var.sampled_requests_enabled
      }
    }
  }

That was a top-level statement. My brain melts when I try to think about nested statements with URL match, exclusions, mixing special IP sets there to protect admin pages or internal resources.

WAFv2 made improvement in terms of consistent resource and attribute names between global and regional WAF resources, but it would be great to keep the resource definition and automation simple as well.

References

• https://github.com/hashicorp/terraform/issues/25889#issuecomment-675789876

[gdavison]

Hi @dvishniakov, thanks for the issue. The WAFv2 rules are definitely more complex than v1, as a cost of more flexibility. As you noted, we follow the AWS Go SDK closely, as deviating from the API can lead to future maintenance problems.

In typical use of this resource, dynamic blocks would not be used everywhere, as the implementation would specify only the fields needed. This would simplify the use of the resource.

You mentioned, however, that you wanted something more automation-friendly. One approach would be to use a combination of the Terraform JSON syntax and override files. The ACL rules could then be generated by an external tool.

It may be possible to request a simplified rule API using a support ticket with AWS.

I will, however, leave this issue open as a tracking issue. While we will not be investigating further at this time, we are especially open to proposals for potential solutions.

[anGie44]

Hi @dvishniakov 👋 -- dropping a note here that while we are still open to discussing proposals, the performance issue referenced in #2 has been addressed when using v0.13.5+ of Terraform, with significant runtime improvements (runtime snippet in: https://github.com/terraform-providers/terraform-provider-aws/issues/14062#issuecomment-717659882 and benchmarks https://github.com/hashicorp/terraform/pull/26577#issue-502887591) 😃

[bushong1]

What was it about the AWS API that necessitated:

action {
  dynamic "allow" {
    for_each = lower(var.sql_injection_rule_action) == "allow" ? [true] : []
    content {}
  }
  dynamic "block" {
    for_each = lower(var.sql_injection_rule_action) == "block" ? [true] : []
    content {}
  }
  dynamic "count" {
    for_each = lower(var.sql_injection_rule_action) == "count" ? [true] : []
    content {}
  }
}

Instead of...

action = var.sql_injection_rule_action

It feels like the latter could be considered a QOL feature of the terraform aws provider, even if the API behind the scenes is overly complex. I get not being able to have variables in the dynamic block property name, but like... don't make it a dynamic block?

[github-actions[bot]]

Marking this issue as stale due to inactivity. This helps our maintainers find and focus on the active issues. If this issue receives no comments in the next 30 days it will automatically be closed. Maintainers can also remove the stale label.

If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you!

[github-actions[bot]]

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.