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

feat(aws): New CloudTrail, DLM, DocumentDB, EC2, Account and Support checks #2675

Merged
merged 62 commits into from
Oct 17, 2023

Conversation

jit-contrib
Copy link
Contributor

@jit-contrib jit-contrib commented Aug 3, 2023

Context

Adding 6 new AWS rules

Description

This PR implement the fixes suggested in #2645.

List of new added rules:

  • account_maintain_different_contact_details_to_security_billing_and_operations
  • cloudtrail_multi_region_enabled_logging_management_events
  • dlm_ebs_snapshot_lifecycle_policy_exists
  • ec2_ebs_volume_snapshots_exists
  • documentdb_instance_storage_encrypted
  • trustedadvisor_premium_support_plan_subscribed

License

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

@jit-contrib jit-contrib requested a review from a team as a code owner August 3, 2023 16:23
@jfagoagas jfagoagas added status/awaiting-reponse Waiting response from Issue owner no-merge Please, DO NOT MERGE this PR. provider/aws Issues/PRs related with the AWS provider labels Aug 4, 2023
@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #2675 (01c55c8) into master (6558aed) will increase coverage by 0.03%.
Report is 44 commits behind head on master.
The diff coverage is 91.01%.

@@            Coverage Diff             @@
##           master    #2675      +/-   ##
==========================================
+ Coverage   86.23%   86.26%   +0.03%     
==========================================
  Files         539      551      +12     
  Lines       17453    17632     +179     
==========================================
+ Hits        15051    15211     +160     
- Misses       2402     2421      +19     
Files Coverage Δ
...tact_details_to_security_billing_and_operations.py 100.00% <100.00%> (ø)
..._region_enabled/cloudtrail_multi_region_enabled.py 100.00% <ø> (ø)
..._multi_region_enabled_logging_management_events.py 100.00% <100.00%> (ø)
..._enabled/cloudtrail_s3_dataevents_write_enabled.py 100.00% <ø> (ø)
...ders/aws/services/cloudtrail/cloudtrail_service.py 90.35% <ø> (ø)
prowler/providers/aws/services/dlm/dlm_client.py 100.00% <100.00%> (ø)
...exists/dlm_ebs_snapshot_lifecycle_policy_exists.py 100.00% <100.00%> (ø)
prowler/providers/aws/services/dlm/dlm_service.py 100.00% <100.00%> (ø)
...iders/aws/services/documentdb/documentdb_client.py 100.00% <100.00%> (ø)
...encrypted/documentdb_instance_storage_encrypted.py 100.00% <100.00%> (ø)
... and 7 more

... and 20 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jfagoagas jfagoagas changed the title Fix PR comments feat(aws): New CloudTrail, DLM, DocumentDB, EC2, Account and Support checks Aug 25, 2023
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.

I've left a lot of comments / changes since some parts of this PR are not coded following our guidelines:

  • Some services are duplicated since documentdb (like neptune) API points to the rds API underneath.
  • The support service is created within the trustedadvisor service so there is no need to have another service.
  • Regarding the support check we have to include a list of accounts using the configuration file because we cannot raise a FAIL in each Prowler scan if the account has not AWS Premium Support configured.

I recommend you to follow the Prowler Developer Guide here https://docs.prowler.cloud/en/latest/developer-guide/introduction/ and we can talk more about this using the Slack.

poetry.lock Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This check is not working as expected, in our demo environments it is raising a FAIL finding while having a multi-region CloudTrail logging all the management events. Please check this out.

prowler/providers/aws/services/support/support_service.py Outdated Show resolved Hide resolved
)
)
else:
raise error
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise error
logger.error(
f"{regional_client.region} --"
f" {error.__class__.__name__}[{error.__traceback__.tb_lineno}]:"
f" {error}"
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I'm catching an error inside a try...catch block already, the raise is to raise other errors that can happen other than the one I want to catch, and be catched and logged by the outer block. I think that the behavior will be the same, the difference is just duplicating the code to log the error instead of reuse this part of the code of the outer try...catch block. Do you prefer me to apply your change? Feel free to do it

jit-contrib and others added 20 commits October 3, 2023 00:14
…ent_contact_details_to_security_billing_and_operations/account_maintain_different_contact_details_to_security_billing_and_operations.metadata.json

Co-authored-by: Pepe Fagoaga <pepe@verica.io>
…ent_contact_details_to_security_billing_and_operations/account_maintain_different_contact_details_to_security_billing_and_operations.metadata.json

Co-authored-by: Pepe Fagoaga <pepe@verica.io>
…ent_contact_details_to_security_billing_and_operations/account_maintain_different_contact_details_to_security_billing_and_operations.metadata.json

Co-authored-by: Pepe Fagoaga <pepe@verica.io>
…ent_contact_details_to_security_billing_and_operations/account_maintain_different_contact_details_to_security_billing_and_operations.py

Co-authored-by: Pepe Fagoaga <pepe@verica.io>
Co-authored-by: Pepe Fagoaga <pepe@verica.io>
Co-authored-by: Pepe Fagoaga <pepe@verica.io>
@jfagoagas jfagoagas removed status/awaiting-reponse Waiting response from Issue owner no-merge Please, DO NOT MERGE this PR. labels Oct 17, 2023
@jfagoagas jfagoagas self-requested a review October 17, 2023 16:24
@jfagoagas jfagoagas self-requested a review October 17, 2023 16:25
jfagoagas
jfagoagas previously approved these changes Oct 17, 2023
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.

May thanks @jit-contrib for this huge contribution to keep improving and growing Prowler 🚀 🚀 🚀 🚀

@jfagoagas jfagoagas merged commit 85e12e9 into prowler-cloud:master Oct 17, 2023
8 checks passed
jfagoagas added a commit to jit-contrib/prowler that referenced this pull request Oct 18, 2023
…checks (prowler-cloud#2675)

Co-authored-by: Pepe Fagoaga <pepe@verica.io>
Co-authored-by: Sergio Garcia <sergargar1@gmail.com>
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.

None yet

3 participants