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

fix(mutelist): change logic for tags in aws mutelist #4786

Merged
merged 6 commits into from
Aug 20, 2024
Merged

fix(mutelist): change logic for tags in aws mutelist #4786

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1677105
feat(aws): change logic for tags
pedrooot Aug 19, 2024
63dbbce
docs(mutelist): improve docs about mutelist
pedrooot Aug 19, 2024
902d2aa
fix(mutelist): improve tests
pedrooot Aug 19, 2024
a06ad20
fix(mutelist): add tests and improve docs
pedrooot Aug 20, 2024
684ebdc
fix: typo
pedrooot Aug 20, 2024
14fd589
docs(mutelist): improve docs
pedrooot Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/tutorials/mutelist.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link

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 🙇

Copy link
Member Author

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!

Copy link

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 with project=(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 has Name=myhost.mydomain, I think this will match the rule in your example? But that would also depend on the syntax that is passed on finding_items.

Copy link
Member Author

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.

Copy link

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.


If any of the criteria do not match, the check is not muted.

Expand Down
23 changes: 17 additions & 6 deletions prowler/lib/mutelist/mutelist.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,9 @@ def is_muted_in_check(
muted_in_resource = self.is_item_matched(
muted_resources, finding_resource
)
muted_in_tags = self.is_item_matched(muted_tags, finding_tags)
muted_in_tags = self.is_item_matched(
muted_tags, finding_tags, tag=True
)

# For a finding to be muted requires the following set to True:
# - muted_in_check -> True
Expand Down Expand Up @@ -279,7 +281,9 @@ def is_excepted(
)

excepted_tags = exceptions.get("Tags", [])
is_tag_excepted = self.is_item_matched(excepted_tags, finding_tags)
is_tag_excepted = self.is_item_matched(
excepted_tags, finding_tags, tag=True
)

if (
not is_account_excepted
Expand All @@ -303,7 +307,7 @@ def is_excepted(
return False

@staticmethod
def is_item_matched(matched_items, finding_items):
def is_item_matched(matched_items, finding_items, tag=False) -> bool:
jfagoagas marked this conversation as resolved.
Show resolved Hide resolved
"""
Check if any of the items in matched_items are present in finding_items.

Expand All @@ -317,12 +321,19 @@ def is_item_matched(matched_items, finding_items):
try:
is_item_matched = False
if matched_items and (finding_items or finding_items == ""):
if tag:
is_item_matched = True
jfagoagas marked this conversation as resolved.
Show resolved Hide resolved
for item in matched_items:
if item.startswith("*"):
item = ".*" + item[1:]
if re.search(item, finding_items):
is_item_matched = True
break
if tag:
if not re.search(item, finding_items):
is_item_matched = False
break
jfagoagas marked this conversation as resolved.
Show resolved Hide resolved
else:
if re.search(item, finding_items):
is_item_matched = True
break
return is_item_matched
except Exception as error:
logger.error(
Expand Down
12 changes: 6 additions & 6 deletions tests/providers/aws/lib/mutelist/aws_mutelist_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,7 +1132,7 @@ def test_is_muted_tags(self):
}
mutelist = AWSMutelist(mutelist_content=mutelist_content)

assert mutelist.is_muted(
pedrooot marked this conversation as resolved.
Show resolved Hide resolved
assert not mutelist.is_muted(
AWS_ACCOUNT_NUMBER,
"check_test",
AWS_REGION_US_EAST_1,
Expand Down Expand Up @@ -1321,23 +1321,23 @@ def test_is_excepted(self):
AWS_ACCOUNT_NUMBER,
"eu-central-1",
"test",
"environment=test",
"environment=test | project=prowler",
)

assert mutelist.is_excepted(
exceptions,
AWS_ACCOUNT_NUMBER,
"eu-south-3",
"test",
"environment=test",
"environment=test | project=prowler",
)

assert mutelist.is_excepted(
exceptions,
AWS_ACCOUNT_NUMBER,
"eu-south-3",
"test123",
"environment=test",
"environment=test | project=prowler",
)

def test_is_excepted_only_in_account(self):
Expand Down Expand Up @@ -1413,7 +1413,7 @@ def test_is_excepted_in_account_and_tags(self):
"Accounts": [AWS_ACCOUNT_NUMBER],
"Regions": [],
"Resources": [],
"Tags": ["environment=test"],
"Tags": ["environment=test", "project=example"],
}
mutelist = AWSMutelist(mutelist_content={})

Expand All @@ -1422,7 +1422,7 @@ def test_is_excepted_in_account_and_tags(self):
AWS_ACCOUNT_NUMBER,
AWS_REGION_EU_CENTRAL_1,
"resource_1",
"environment=test",
"environment=test | project=example",
)

assert not mutelist.is_excepted(
Expand Down