umotif-public / terraform-aws-waf-webaclv2

Terraform module to configure WAF V2 Web ACL with managed rules for Application Load Balancer
https://registry.terraform.io/modules/umotif-public/waf-webaclv2/aws
Other
137 stars 124 forks source link

*.field_to_match.headers.match_pattern "all" option is not working #94

Closed felipe88alves closed 1 year ago

felipe88alves commented 1 year ago

What is the current behavior? Applying a rule with the field_to_match set to headers will fail when the match_pattern is set to all

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. The following rule will not fail a plan, but will produce an empty statement, which fails when running an apply

    {
      name     = "custom_block_rule_1"
      priority = 1
      action   = "block"
      statement = {
        byte_match_statement = {
          field_to_match = {
            headers = {
              match_pattern     = {
                "all" = {}
              }
              match_scope       = "ALL"
              oversize_handling = "CONTINUE"
            }
          }
          positional_constraint = "CONTAINS"
          search_string         = "string"
          priority              = 0
          type                  = "NONE"
        }
      }
      visibility_config = {
        sampled_requests_enabled   = true
        cloudwatch_metrics_enabled = true
        metric_name                = "custom_block_rule_1"
      }
    }

What is the expected behavior? It should not produce an empty statement and should not fail an apply

Software versions? umotif/terraform-aws-waf-webaclv2: v4.2.0 hashicorp/aws: v4.59.0

MatanHeledPort commented 1 year ago

Hey, I created a PR which I think is supposed to fix this issue? https://github.com/umotif-public/terraform-aws-waf-webaclv2/pull/95

felipe88alves commented 1 year ago

Thanks @MatanHeledPort! Did you test the solution?

I was thinking of something similar to what's done with the cookies: for_each = contains(keys(match_pattern.value), "all") ? [lookup(match_pattern.value, "all")] : []

MatanHeledPort commented 1 year ago

Maybe keeping convention is better - I will change it to match cookies

MatanHeledPort commented 1 year ago

For some reason it doesnt behave the same as cookies image

Seems like this also happens with cookies

felipe88alves commented 1 year ago

seems like the problem is with your use of the statement block. Want to share you config here?

felipe88alves commented 1 year ago

Would probably be good to add an example as well in https://github.com/umotif-public/terraform-aws-waf-webaclv2/blob/main/examples/wafv2-bytematch-rules/main.tf

MatanHeledPort commented 1 year ago

Sure! This is the scope_down_statement:

        scope_down_statement = {
          and_statement = {
            statements = [
              {
                not_statement = {
                  byte_match_statement = {
                    field_to_match = {
                      headers = {
                        match_pattern = {
                          all = {}
                        }
                        match_scope = "ALL"
                        oversize_handling = "CONTINUE"
                      }
                    }
                  positional_constraint = "CONTAINS"
                  search_string         = "authorization"
                  priority              = 0
                  type                  = "NONE"
                  }
                }
              },
              {
                not_statement = {
                  byte_match_statement = {
                    field_to_match = {
                      uri_path = "{}"
                    }
                  positional_constraint = "EXACTLY"
                  search_string         = "/"
                  priority              = 0
                  type                  = "NONE"
                  }
                }
              }
            ]
          }
        }

Working on an example aswell

felipe88alves commented 1 year ago

What initial statement is the scope_down_statement under? A rate_based_statement? Something like this?

  rules = [
    {
      name     = "test_rule"
      priority = 1
      action   = "block"
      rate_based_statement = {
        limit              = 500000
        aggregate_key_type = "IP"
        scope_down_statement = {
          and_statement = {
            statements = [
              {
                not_statement = {
                  byte_match_statement = {
                    field_to_match = {
                      headers = {
                        match_pattern = {
                          all = {}
                        }
                        match_scope = "ALL"
                        oversize_handling = "CONTINUE"
                      }
                    }
                  positional_constraint = "CONTAINS"
                  search_string         = "authorization"
                  priority              = 0
                  type                  = "NONE"
                  }
                }
              },
              {
                not_statement = {
                  byte_match_statement = {
                    field_to_match = {
                      uri_path = "{}"
                    }
                  positional_constraint = "EXACTLY"
                  search_string         = "/"
                  priority              = 0
                  type                  = "NONE"
                  }
                }
              }
            ]
          }
        }
      }
    }
  ]
MatanHeledPort commented 1 year ago

Yes, exactly Edit: This is the full rules array

    waf_rules = [
      {
        name = "UnauthorizedRateLimit"
        priority = "1"

        override_action = "count"

        visibility_config = {
          metric_name = "UnauthorizedRateLimit-metric"
        }
        rate_based_statement = {
        limit              = 5000
        aggregate_key_type = "IP"
        # Optional scope_down_statement to refine what gets rate limited
        scope_down_statement = {
          and_statement = {
            statements = [
              {
                not_statement = {
                  byte_match_statement = {
                    field_to_match = {
                      headers = {
                        match_pattern = {
                          "all" = {}
                        }
                        match_scope = "ALL"
                        oversize_handling = "CONTINUE"
                      }
                    }
                  positional_constraint = "CONTAINS"
                  search_string         = "authorization"
                  priority              = 0
                  type                  = "NONE"
                  }
                }
              },
              {
                not_statement = {
                  byte_match_statement = {
                    field_to_match = {
                      uri_path = "{}"
                    }
                  positional_constraint = "EXACTLY"
                  search_string         = "/"
                  priority              = 0
                  type                  = "NONE"
                  }
                }
              }
            ]
          }
        }
      }
      }
    ]
felipe88alves commented 1 year ago

Took a quick look here, seems like the problem is with the use of override_action. It is described as a dynamic field in the main.tf schema, whereas it is used as a normal var in the example.

Replace override_action = "count" to action = "count" and it should work

MatanHeledPort commented 1 year ago

Ok that seemed to do the trick! The change worked

image
MatanHeledPort commented 1 year ago

When you get to merging LMK @felipe88alves

felipe88alves commented 1 year ago

I'm don't have write access, but maybe @Ohid25 can help 👍

felipe88alves commented 1 year ago

Can we please get some feedback on this PR? Really needing it to reduce some manual toil. Cheers

MatanHeledPort commented 1 year ago

Feel free to use my forked PR branch. Its what Im doing untill this merges @felipe88alves

Ohid25 commented 1 year ago

This should now be fixed as a part of 4.4.0.