-
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
chore(arn): improve resource ARNs in checks #3388
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3388 +/- ##
==========================================
+ Coverage 85.85% 85.89% +0.04%
==========================================
Files 586 670 +84
Lines 18893 20827 +1934
==========================================
+ Hits 16220 17890 +1670
- Misses 2673 2937 +264 ☔ View full report in Codecov by Sentry. |
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.
As a general recommendation, save that generic ARNs in the services and then get that in the checks, we should define that in just one place. Check that please.
@@ -17,7 +17,7 @@ def execute(self): | |||
report = Check_Report_AWS(self.metadata()) | |||
report.status = "FAIL" | |||
report.status_extended = "No Backup Plan exist." | |||
report.resource_arn = backup_client.audited_account_arn | |||
report.resource_arn = f"arn:{backup_client.audited_partition}:backup:{backup_client.region}:{backup_client.audited_account}:backup-plan" |
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.
This applies to all the backup checks.
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.
Maybe you can define those ARNs in a single place to use that also for the tests. Maybe it's better to have something in the lib/arn
with the service generic ARNs.
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.
Good point, it would be a nice-to-have. But I see it complicated, since each check can have a different resource type and we need to use the information of each service.
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.
Done! I have added them in each service class.
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.
Please, modify Prowler Developer Guide to reflect this change here https://docs.prowler.com/projects/prowler-open-source/en/latest/developer-guide/checks/#resource-id-name-and-arn
You can check the documentation for this PR here -> SaaS Documentation |
docs/developer-guide/checks.md
Outdated
@@ -125,7 +125,7 @@ All the checks MUST fill the `report.resource_id` and `report.resource_arn` with | |||
- Resource ARN -- `report.resource_arn` | |||
- AWS Account --> Root ARN `arn:aws:iam::123456789012:root` | |||
- AWS Resource --> Resource ARN | |||
- Root resource --> Root ARN `arn:aws:iam::123456789012:root` | |||
- Root resource --> Root ARN `f"arn:{service_client.audited_partition}:<service_name>:{service_client.region}:{service_client.audited_account}:<resource_type>"` |
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.
Name the result differently than root
, maybe Service ARN or Resource Type ARN
You can check the documentation for this PR here -> SaaS Documentation |
You can check the documentation for this PR here -> SaaS Documentation |
You can check the documentation for this PR here -> SaaS Documentation |
You can check the documentation for this PR here -> SaaS Documentation |
@@ -21,8 +21,8 @@ def execute(self): | |||
"No CloudWatch log groups found with metric filters or alarms associated." | |||
) | |||
report.region = cloudwatch_client.region |
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.
report.region = cloudwatch_client.region | |
report.region = logs_client.region |
@@ -21,8 +21,8 @@ def execute(self): | |||
"No CloudWatch log groups found with metric filters or alarms associated." | |||
) | |||
report.region = cloudwatch_client.region |
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.
report.region = cloudwatch_client.region | |
report.region = logs_client.region |
@@ -21,8 +21,8 @@ def execute(self): | |||
"No CloudWatch log groups found with metric filters or alarms associated." | |||
) | |||
report.region = cloudwatch_client.region |
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.
report.region = cloudwatch_client.region | |
report.region = logs_client.region |
@@ -21,8 +21,8 @@ def execute(self): | |||
"No CloudWatch log groups found with metric filters or alarms associated." | |||
) | |||
report.region = cloudwatch_client.region |
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.
report.region = cloudwatch_client.region | |
report.region = logs_client.region |
@@ -23,8 +23,8 @@ def execute(self): | |||
"No CloudWatch log groups found with metric filters or alarms associated." | |||
) | |||
report.region = cloudwatch_client.region |
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.
report.region = cloudwatch_client.region | |
report.region = logs_client.region |
@@ -23,8 +23,8 @@ def execute(self): | |||
"No CloudWatch log groups found with metric filters or alarms associated." | |||
) | |||
report.region = cloudwatch_client.region |
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.
report.region = cloudwatch_client.region | |
report.region = logs_client.region |
@@ -8,7 +8,7 @@ def execute(self): | |||
for ebs_encryption in ec2_client.ebs_encryption_by_default: | |||
report = Check_Report_AWS(self.metadata()) | |||
report.region = ebs_encryption.region |
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.
Check this region please.
@@ -9,7 +9,7 @@ def execute(self): | |||
report = Check_Report_AWS(self.metadata()) | |||
report.region = region |
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.
Check this region please.
@@ -10,7 +10,7 @@ def execute(self): | |||
if encryption.tables or not glue_client.audit_info.ignore_unused_services: | |||
report = Check_Report_AWS(self.metadata()) | |||
report.resource_id = glue_client.audited_account | |||
report.resource_arn = glue_client.audited_account_arn | |||
report.resource_arn = glue_client.data_catalog_arn_template | |||
report.region = encryption.region |
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.
Check this region please.
@@ -10,7 +10,7 @@ def execute(self): | |||
if encryption.tables or not glue_client.audit_info.ignore_unused_services: | |||
report = Check_Report_AWS(self.metadata()) | |||
report.resource_id = glue_client.audited_account | |||
report.resource_arn = glue_client.audited_account_arn | |||
report.resource_arn = glue_client.data_catalog_arn_template | |||
report.region = encryption.region |
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.
Check this region please.
@@ -9,7 +9,7 @@ def execute(self): | |||
for session in macie_client.sessions: | |||
report = Check_Report_AWS(self.metadata()) | |||
report.region = session.region |
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.
Check this region please.
You can check the documentation for this PR here -> SaaS 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 improvement @sergargar 🚀
You can check the documentation for this PR here -> SaaS Documentation |
You can check the documentation for this PR here -> SaaS Documentation |
You can check the documentation for this PR here -> SaaS Documentation |
You can check the documentation for this PR here -> SaaS 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.
👍
Description
Improve resource ARNs in checks where we had the Account ARN, now Prowler will have the correct ARN for each resource.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.