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.

Sign up for GitHub

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

Jump to bottom

aws - cloudfront - wafv2-enabled fix to find resources which are associated with waf-classic acl #7986

Merged
merged 13 commits into from
Nov 29, 2022
Merged

aws - cloudfront - wafv2-enabled fix to find resources which are associated with waf-classic acl #7986

merged 13 commits into from
Nov 29, 2022

Conversation

harishachappa
Copy link
Contributor

This change will fix #7985

for wafv2-enabled with state: False, check if resource has web_acl_id and if associated, check if that web_acl_id is from wafv2 or not.
web-acl for state: False should not be included in filter

@cahn1
Copy link
Contributor

cahn1 commented Nov 11, 2022 •
edited
Loading

@harishachappa ,
I was thinking too fast. Deleting previous comment and will test some policies see if it's working correctly with this PR.

@harishachappa
Copy link
Contributor Author

@harishachappa , I was thinking too fast. Deleting previous comment and will test some policies see if it's working correctly with this PR.

@cahn1 wafv2-enabled should return resources which are either not associated with any waf or associated with waf-classic but not wafv2.

@cahn1
Copy link
Contributor

cahn1 commented Nov 11, 2022

@harishachappa,
I tested all use cases. Since the current code focused on only WAFv2 usecase, it's working but as you said, this won't cover if CF is associated w/ WAFv1 webacl. (verified)
However, this PR won't cover this use case for WAFv2:

CF attaches WAFv2 web-acl named "WAFv2-test2".

Policy:
    - type: wafv2-enabled
      state: false
      web-acl: '.*FMManagedWebACLV2-?FMS-.*’

This policy checks if CF's web-acl is NOT '.FMManagedWebACLV2-?FMS-.’, then it will add to results.
From my test, your PR didn't add that. (Could you test your side please?)

After reviewing the current code, this is my suggestion to support WAFv1 use cases.
https://github.com/cloud-custodian/cloud-custodian/compare/master...cahn1:update_cf_wafv2?expand=1
Please let me know your thought.

CC @StevenGunn @darrendao

@harishachappa
Copy link
Contributor Author

@harishachappa, I tested all use cases. Since the current code focused on only WAFv2 usecase, it's working but as you said, this won't cover if CF is associated w/ WAFv1 webacl. (verified) However, this PR won't cover this use case for WAFv2:

CF attaches WAFv2 web-acl named "WAFv2-test2".

Policy:
    - type: wafv2-enabled
      state: false
      web-acl: '.*FMManagedWebACLV2-?FMS-.*’

This policy checks if CF's web-acl is NOT '.FMManagedWebACLV2-?FMS-.’, then it will add to results. From my test, your PR didn't add that. (Could you test your side please?)

After reviewing the current code, this is my suggestion to support WAFv1 use cases. https://github.com/cloud-custodian/cloud-custodian/compare/master...cahn1:update_cf_wafv2?expand=1 Please let me know your thought.

CC @StevenGunn @darrendao

@cahn1
wafv2-enabled with state: False should return resources which are not associated with wafv2. If you want resources which are not associated with specific wafv2 acl, then you should use below filter

Policy:
   - not:
       - type: wafv2-enabled
         state: true
         web-acl: '.*FMManagedWebACLV2-?FMS-.*’

@cahn1
Copy link
Contributor

cahn1 commented Nov 11, 2022 •
edited
Loading

@harishachappa ,
As my understanding, "state" is used "true | false" for evaluating the following "web-acl".

    - type: wafv2-enabled
      state: false

=> This will return resources which are not associated with wafv2.

    - type: wafv2-enabled
      state: false
      web-acl: '.*FMManagedWebACLV2-?FMS-.*’

=> This will return resources which are not associated with specific wafv2 acl.

@darrendao
Copy link
Collaborator

darrendao commented Nov 15, 2022 •
edited
Loading

I think part of the problem here is with the lack of documentation and the inconsistent behavior of the state and web-acl options.

From what I understand, state allows us to filter resources that are either enabled or not enabled with WAFv2. Example:

  1. match resources that are enabled with WAFv2
    - type: wafv2-enabled
      state: true
  1. match resources that are NOT enabled with WAFv2
    - type: wafv2-enabled
      state: false

Now to further filter things down, we should be able to use web-acl to check if the resource is attached or not attached to a particular web acl. That would look like this:

  1. match resources that are attached to the specified web acl
    - type: wafv2-enabled
      state: true
      web-acl: '.*FMManagedWebACLV2-?FMS-.*’
  1. match resources that are NOT attached to the specified web acl
    - type: wafv2-enabled
      state: false
      web-acl: '.*FMManagedWebACLV2-?FMS-.*’

The problem with this PR is that it doesn't work with scenario 4. What the code will do is that it will just return true if the specified web-acl does not exist. It doesn't even care about the resource we're trying to filter. To handle scenario 4, we would have to invert the filter and use the not operation like what @harishachappa had mentioned. To me, this is inconsistent and confusing behavior that should be fixed. @cahn1 has a fix for it (see his comment). I haven't look at his code but he has tested against all the different scenarios (across wafv1 too).

@ajkerrigan @kapilt @thisisshi thoughts on this?

@harishachappa
Copy link
Contributor Author

I think part of the problem here is with the lack of documentation and the inconsistent behavior of the state and web-acl options.

From what I understand, state allows us to filter resources that are either enabled or not enabled with WAFv2. Example:

  1. match resources that are enabled with WAFv2
    - type: wafv2-enabled
      state: true
  1. match resources that are NOT enabled with WAFv2
    - type: wafv2-enabled
      state: false

Now to further filter things down, we should be able to use web-acl to check if the resource is attached or not attached to a particular web acl. That would look like this:

  1. match resources that are attached to the specified web acl
    - type: wafv2-enabled
      state: true
      web-acl: '.*FMManagedWebACLV2-?FMS-.*’
  1. match resources that are NOT attached to the specified web acl
    - type: wafv2-enabled
      state: false
      web-acl: '.*FMManagedWebACLV2-?FMS-.*’

The problem with this PR is that it doesn't work with scenario 4. What the code will do is that it will just return true if the specified web-acl does not exist. It doesn't even care about the resource we're trying to filter. To handle scenario 4, we would have to invert the filter and use the not operation like what @harishachappa had mentioned. To me, this is inconsistent and confusing behavior that should be fixed. @cahn1 has a fix for it (see his comment). I haven't look at his code but he has tested against all the different scenarios (across wafv1 too).

@ajkerrigan @kapilt @thisisshi thoughts on this?

@darrendao, @cahn1,
To keep the implementation same across wafv1 and wafv2, I have pushed a change which should address scenario 4. Please test and let me know if you have concerns with this PR

@cahn3
Copy link

cahn3 commented Nov 15, 2022

@harishachappa , I tested all combinations with the change and all working good!
Thanks @harishachappa @darrendao

@ajkerrigan
Copy link
Member

I think if that's the intended behavior @darrendao, it would probably be helpful to add those 4 sample cases as example policies to the docstring so it's clear to folks who try to use this.

Case 4 wouldn't have been obvious to me for example - @harishachappa 's example with the not block feels a little more obvious. I definitely see the value of a flatter filter structure though, so it makes sense in retrospect 👍 .

One other thing that is probably worth an explicit mention in the docstring is how web-acl is now a regex pattern. That's a pretty useful addition from waf-enabled to wafv2-enabled, worthy of highlighting!

@harishachappa
Copy link
Contributor Author

@ajkerrigan Updated examples to include more usecases

@harishachappa
Copy link
Contributor Author

@ajkerrigan, @kapilt,
Can I get this reviewed and merged please?

ajkerrigan
ajkerrigan approved these changes Nov 23, 2022
View reviewed changes
Copy link
Member

@ajkerrigan ajkerrigan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Thanks for the doc/example additions, I think they'll help a lot! 👍

@harishachappa
Copy link
Contributor Author

@ajkerrigan @kapilt
Can someone please merge this PR?

@ajkerrigan ajkerrigan merged commit 09efdd7 into cloud-custodian:master Nov 29, 2022
@harishachappa harishachappa deleted the cloufront-wafv2enabled-fix branch December 12, 2022 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajkerrigan ajkerrigan ajkerrigan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

wafv-enabled for CloudFront fails to return resources which are associated with waf-classic acl
5 participants
@harishachappa @cahn1 @darrendao @cahn3 @ajkerrigan