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 false negatives in the S3 invalid ACL rule #341

Merged
merged 1 commit into from
Jun 27, 2019

Conversation

wata727
Copy link
Member

@wata727 wata727 commented Jun 26, 2019

This PR replaces aws_s3_bucket_invalid_acl rule with the auto-generated rule introduced in #274.

As a result, it also fixes false negatives in the rule. The previous list included object ACLs such as bucket-owner-read. These are invalid values for bucket ACL. See https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl

@wata727 wata727 merged commit 7c303fa into master Jun 27, 2019
@wata727 wata727 deleted the migrate_s3_invalid_acl_rule branch June 27, 2019 13:50
"authenticated-read": true,
"bucket-owner-read": true,
"bucket-owner-full-control": true,
"log-delivery-write": true,
Copy link

Choose a reason for hiding this comment

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

why was this one removed though? It seems perfectly valid to me?

Copy link

Choose a reason for hiding this comment

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

In fact, all of the above are valid according to the same link you've referenced in the PR description (https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#canned-acl), so I don't quite understand the change?

Copy link
Member Author

@wata727 wata727 Jul 4, 2019

Choose a reason for hiding this comment

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

Umm, what you are saying seems right. It's weird...
The list is generated from aws-sdk API definition, so It may be something wrong with how to use. I will investigate it.

Copy link

Choose a reason for hiding this comment

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

Looks like that block is way out of date - hasn't been updated in 5 years...

Copy link

Choose a reason for hiding this comment

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

There were commits to add new Canned ACLs since then (aws/aws-sdk-go@0922c23 for example), but that particular block was never updated :(

Copy link

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants