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(check): New check7172 for S3 Bucket ACLs #1023

Merged
merged 3 commits into from
Feb 7, 2022

Conversation

jeffmaley
Copy link
Contributor

Context

#1022

Description

This pr adds a check (7172) to look for bucket ACLs

License

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

checks/check_extra7172 Outdated Show resolved Hide resolved
checks/check_extra7172 Outdated Show resolved Hide resolved
@lazize
Copy link
Contributor

lazize commented Feb 4, 2022

Why execute two CLI commands if one is enough?

Variable LIST_OF_BUCKETS can be used for both cases. Access check and loop iteration.
You just need to remove | xargs -n1.

| xargs -n1 will give a better view of the output, but as it will be used by checking error and to perform for loop iteration, it doesn't need a better view. Both cases work fine with a "not so beautiful" output.
Variable LIST_OF_BUCKETS is not used to output details to user.

@jeffmaley
Copy link
Contributor Author

My thought was to keep the two bits separate. If we need to change the access check or bucket controls check, we don't have to refactor the other piece. If you'd rather they be combined, I will make the change.

@lazize
Copy link
Contributor

lazize commented Feb 4, 2022

There is a throttling that impacts Prowler when it is running inside AWS, so I believe fewer request is better.
https://docs.aws.amazon.com/AWSEC2/latest/APIReference/throttling.html

But let's wait maintainer people to comment.

@toniblyx
Copy link
Member

toniblyx commented Feb 4, 2022

I agree with @lazize the less the better regardless of cli and boto3 underneath.

@toniblyx
Copy link
Member

toniblyx commented Feb 4, 2022

and thank you @jeffmaley for the check!

@jeffmaley jeffmaley requested review from a team, toniblyx and n4ch04 February 4, 2022 18:16
@jeffmaley
Copy link
Contributor Author

I've condensed the access check into the s3 check itself and removed the extra api call.

@jfagoagas jfagoagas changed the title added check7172 for s3 bucket acls feat(check): New check7172 for S3 Bucket ACLs Feb 7, 2022
Copy link
Member

@toniblyx toniblyx left a comment

Choose a reason for hiding this comment

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

LGTM

@toniblyx toniblyx merged commit c6f0351 into prowler-cloud:master Feb 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants