-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(mutelist): change logic for tags in aws mutelist #4786
Conversation
You can check the documentation for this PR here -> Prowler Documentation |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4786 +/- ##
==========================================
- Coverage 89.10% 89.03% -0.07%
==========================================
Files 921 923 +2
Lines 28157 28280 +123
==========================================
+ Hits 25089 25180 +91
- Misses 3068 3100 +32 ☔ View full report in Codecov by Sentry. |
docs/tutorials/mutelist.md
Outdated
@@ -9,7 +9,7 @@ Mutelist option works along with other options and will modify the output in the | |||
|
|||
## How the Mutelist Works | |||
|
|||
The Mutelist uses an "ANDed" and "ORed" logic to determine which resources, checks, regions, and tags should be muted. For each check, the Mutelist checks if the account, region, and resource match the specified criteria, using an "ANDed" logic. If tags are specified, the mutelist uses and "ORed" logic to see if at least one tag is present in the resource. | |||
The Mutelist uses an "ANDed" and "ORed" logic to determine which resources, checks, regions, and tags should be muted. For each check, the Mutelist checks if the account, region, and resource match the specified criteria, using an "ANDed" logic. If tags are specified, the mutelist uses an "ANDed" logic to see if all of the tags are present in the resource. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say that this is still a bit confusing, as it mentions first to be using both AND or OR, but then later only mentions using AND. You guys could probably add some of the explanation of #4782 (comment) in here to make clear how to use both use cases 🙇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dlouzan I've just updated the docs, tell me your thoughts!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedrooot Now it's much clearer, but this has led to some more questions 😃
Without having delved too much into the codebase, I see that this part is doing a re.match
on the tags:
- What is the syntax for multiple tags that is obtained in the input
finding_items
when the function is called? Is that a comma-separated list of tags in a string? - Is the code doing any kind of cleanup/filtering of the passed string from the mutelist rules for tags? or it goes as-is from the yaml user input?
- If it is not, and it is passed directly to
re.match
, can't users do here a lot of magic (intendedly or unintendedly 😅). E.g. would your OR sample also match withproject=(test|stage)
? - If that's the case, it might be worth noting in the docs the fact that this is actually an interpreted regex (with characters that might need escaping, interpreted quantifiers, etc).
- Some other cases where I'm wondering what happens is substrings, do we need anchors here? E.g. I have a rule for
Name=myhost
but the actual resource hasName=myhost.mydomain
, I think this will match the rule in your example? But that would also depend on the syntax that is passed onfinding_items
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's using re.search for this part, findings_items are a string whose format can be seen on unroll_dict
docstring from prowler/prowler/lib/outputs/utils.py
.
For your second and third point, we check that the mutelist follow a defined schema (prowler/lib/mutelist/models.py
) and yes, project=(test|stage)
will mute project=test
and project=stage
. I'll add this to docs and tests to cover this cases.
Regarding to the last point: yes, it will match the rule because the mutelist contains the substring from the resource. If you don't want this to happen remember that mutelist is using regular expressions, just modify your rule to Name=myhost(?!\.)
to ensure that Name=myhost.mydomain
won't be muted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pedrooot Thanks for the clarifications, appreciated. AFAIU the substring case might need some more thinking depending on the case, e.g. a mute rule with Name=my_host
would also match a resource with Name=my_host_is_not_really_this
. We probably would need something like Name=my_host\\b
, but honestly I have not really thought this through and I'm starting my holidays today 😁
The fact that the regex depends on matching the pipechar-separated string from unroll_dict
might make this a bit weird. Perhaps it'd be best if the match was done iterating over a list for each tag of the resource individually, that would make possible to anchor searches or provide options to specify whether to do regex or substring searches. But here I'm just speculating as I do not know the codebase and there might be good reasons for the current behaviour.
You can check the documentation for this PR here -> Prowler Documentation |
You can check the documentation for this PR here -> Prowler Documentation |
You can check the documentation for this PR here -> Prowler Documentation |
You can check the documentation for this PR here -> Prowler Documentation |
You can check the documentation for this PR here -> Prowler Documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great fix 👏🏼
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation and see the Github Action logs for details |
(cherry picked from commit e11bb47)
Context
Mutelist allow the user to mute certain findings.
Fix #4782
Description
To mute a finding, all the tags should be in the mutelist. In this PR this logic is applied and docs are updated.
Thanks @dlouzan for the report!
Checklist
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.