Skip to content
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

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

Closed
felipe88alves opened this issue Apr 19, 2023 · 16 comments
Closed

Comments

@felipe88alves
Copy link
Contributor

felipe88alves commented Apr 19, 2023

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
Copy link
Contributor

Hey, I created a PR which I think is supposed to fix this issue?
#95

@felipe88alves
Copy link
Contributor Author

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
Copy link
Contributor

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

@MatanHeledPort
Copy link
Contributor

MatanHeledPort commented Apr 19, 2023

For some reason it doesnt behave the same as cookies
image

Seems like this also happens with cookies

@felipe88alves
Copy link
Contributor Author

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

@felipe88alves
Copy link
Contributor Author

@MatanHeledPort
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

MatanHeledPort commented Apr 21, 2023

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
Copy link
Contributor Author

felipe88alves commented Apr 24, 2023

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
Copy link
Contributor

MatanHeledPort commented Apr 25, 2023

Ok that seemed to do the trick!
The change worked

image

@MatanHeledPort
Copy link
Contributor

When you get to merging LMK @felipe88alves

@felipe88alves
Copy link
Contributor Author

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

@felipe88alves
Copy link
Contributor Author

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

@MatanHeledPort
Copy link
Contributor

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

Ohid25 added a commit that referenced this issue May 12, 2023
* Small fix for 'field_to_match' to enable 'all' option'

* fixed for all header field matches

* changed to match cookies

* changed array output

* changed array output

* revert array change

* added example

* fix

* chore: changelog + hooks update + lint

---------

Co-authored-by: Abdul Wahid <[email protected]>
@Ohid25
Copy link
Contributor

Ohid25 commented May 12, 2023

This should now be fixed as a part of 4.4.0.

@Ohid25 Ohid25 closed this as completed May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants