chore(s3): reduce false positive in s3 public check #4281
Conversation
github-actions
bot
added
the
provider/aws
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4281 +/- ##
==========================================
+ Coverage 86.66% 87.03% +0.36%
==========================================
Files 818 843 +25
Lines 25709 26334 +625
==========================================
+ Hits 22281 22919 +638
+ Misses 3428 3415 -13 ☔ View full report in Codecov by Sentry. |
sergargar
changed the title
|
Just tried it out, it works for some bucket policies but not others - this looks like it is due to the case of your |
Great catch, we need to do the match that way, please @puchy22 change it and add some tests to cover that. |
There was a problem hiding this comment.
See above comment and check the function is_condition_block_restrictive at https://github.com/prowler-cloud/prowler/blob/9fbd627f9aec3edee4f7ddc25849faa11db6187e/prowler/providers/aws/lib/policy_condition_parser/policy_condition_parser.py
We should unify both functions since they are similar.
|
@puchy22 where did you get the |
| @@ -68,3 +73,96 @@ def is_policy_public(policy: dict) -> bool: | |||
| ) and "Condition" not in statement: | |||
| return True | |||
| return False | |||
|
|
|||
|
|
|||
| def check_full_service_access(service: str, policy: dict) -> bool: | |||
| if condition_statement["IpAddress"].get("aws:sourceip", ""): | ||
| if not isinstance(condition_statement["IpAddress"]["aws:sourceip"], list): | ||
| condition_statement["IpAddress"]["aws:sourceip"] = [ | ||
| condition_statement["IpAddress"]["aws:sourceip"] |
There was a problem hiding this comment.
Could you define the following as constants?
aws:sourceipIpAddress
| } | ||
| """ | ||
|
|
||
| is_from_private_ip = False |
There was a problem hiding this comment.
Could you add a try/except block for the whole function?
| except ValueError: | ||
| logger.error(f"Invalid IP: {ip}") |
There was a problem hiding this comment.
Cover this line with a test please.
prowler/providers/aws/services/s3/s3_bucket_public_access/s3_bucket_public_access.py
Show resolved
Hide resolved
|
|
||
| is_from_private_ip = False | ||
|
|
||
| if condition_statement.get("IpAddress", {}): |
There was a problem hiding this comment.
I will move this line into the function caller, makes little sense to call the function if the condition is not IpAddress.
| assert check_full_service_access("s3", policy1) | ||
| assert not check_full_service_access("s3", policy2) | ||
| assert not check_full_service_access("s3", policy3) | ||
| assert not check_full_service_access("s3", policy4) |
There was a problem hiding this comment.
Could you please create one test case for each policy? It's simpler to work with the tests are failing.
Context
Fixes #4257
Not checking conditions in policy statements
Description
Add new policy function to check policy conditions and improve checks using this functions. Everything added have been tested properly.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.