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.

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(aws): review checks with wrong attributes #5503

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

sergargar
Copy link
Member

Context

Some checks did not have ARN, Resource Type or Recommendation.Text, which was making SecurityHub failing when sending findings.

Fix #5498.

Description

  • Review checks to complete wrong or empty values.
  • Use the Severity Enum values when changing it within the checks.
  • Improve WAF and Organizations service.

Checklist

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@sergargar sergargar requested review from a team as code owners October 22, 2024 19:32
@github-actions github-actions bot added the provider/aws Issues/PRs related with the AWS provider label Oct 22, 2024
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

Attention: Patch coverage is 95.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.67%. Comparing base (f36d23c) to head (86b30e4).
Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
...ws/services/organizations/organizations_service.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5503      +/-   ##
==========================================
+ Coverage   89.66%   89.67%   +0.01%     
==========================================
  Files        1076     1085       +9     
  Lines       33305    33525     +220     
==========================================
+ Hits        29862    30063     +201     
- Misses       3443     3462      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@jfagoagas jfagoagas left a comment

Choose a reason for hiding this comment

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

That's a great one, so many improvements in our checks 🚀 Please review my comments when you get a chance, thanks!

@@ -11,8 +11,7 @@ def execute(self):
report = Check_Report_AWS(self.metadata())
report.region = registry.region
report.resource_id = registry.id
# A registry cannot have tags
report.resource_tags = []
report.resource_arn = ecr_client.audited_account_arn
Copy link
Member

Choose a reason for hiding this comment

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

I think here we should create our own ARN, what do you think?

@@ -67,11 +67,11 @@ def execute(self):
utc
):
report.status = "FAIL"
report.check_metadata.Severity = "high"
report.check_metadata.Severity = Severity.high
Copy link
Member

Choose a reason for hiding this comment

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

Please, add a test to cover this scenarion.

@@ -11,6 +11,7 @@ def execute(self) -> Check_Report_AWS:
for domain in route53domains_client.domains.values():
report = Check_Report_AWS(self.metadata())
report.resource_id = domain.name
report.resource_arn = route53domains_client.audited_account_arn
Copy link
Member

Choose a reason for hiding this comment

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

The same again to create an ARN for this one.

@@ -11,6 +11,7 @@ def execute(self) -> Check_Report_AWS:
for domain in route53domains_client.domains.values():
report = Check_Report_AWS(self.metadata())
report.resource_id = domain.name
report.resource_arn = route53domains_client.audited_account_arn
Copy link
Member

Choose a reason for hiding this comment

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

The same again to create an ARN for this one.

self._list_web_acls()
self.__threading_call__(self._get_web_acl, self.web_acls.values())
self.__threading_call__(self._get_logging_configuration, self.web_acls.values())
if self.audited_partition == "aws":
Copy link
Member

Choose a reason for hiding this comment

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

Why only aws? China has also CloudFront, at least by our regions file.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can see in https://docs.aws.amazon.com/general/latest/gr/waf-classic.html that only this partition is supported.

@sergargar
Copy link
Member Author

@jfagoagas comments were solved!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider/aws Issues/PRs related with the AWS provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error when sending findings to SecurityHub: data.Resources[0].Id should NOT be shorter than 1 characters
2 participants