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

Don't Specify an ACL by Default (Fixes #110) #124

Merged
merged 3 commits into from
Sep 1, 2023

Conversation

nhandler
Copy link
Contributor

ACLs in S3 predate IAM. They are also no longer recommended. Instead, users are encouraged to rely on IAM and Bucket Policies to manage access. Amazon is even going to start disabling ACLs on new buckets (see https://docs.aws.amazon.com/AmazonS3/latest/userguide/ensure-object-ownership.html).Users are also generally encouraged to set BucketOwnerEnforced on existing buckets to disable ACLs.

When ACLs are disabled on a bucket, attempts to call s3:PutObject while specifying an acl parameter will cause an
AccessControlListNotSupported error from AWS specifying that The bucket does not allow ACLs.

This change updates the plugin so that there is no longer a default value for the ACL. The plugin will now only pass an ACL to s3:PutObject if one is explicitly specified by the user.

ACLs in S3 predate IAM. They are also no longer recommended. Instead,
users are encouraged to rely on IAM and Bucket Policies to manage
access. Amazon is even going to start disabling ACLs on new buckets (see
https://docs.aws.amazon.com/AmazonS3/latest/userguide/ensure-object-ownership.html).Users
are also generally encouraged to set `BucketOwnerEnforced` on existing
buckets to disable ACLs.

When ACLs are disabled on a bucket, attempts to call `s3:PutObject`
while specifying an `acl` parameter will cause an
`AccessControlListNotSupported` error from AWS specifying that `The
bucket does not allow ACLs`.

This change updates the plugin so that there is no longer a default
value for the ACL. The plugin will now only pass an ACL to
`s3:PutObject` if one is explicitly specified by the user.
@nhandler
Copy link
Contributor Author

@bradrydzewski Any chance you might be able to help get some eyes on this change?

@devkimittal
Copy link
Collaborator

@nhandler approved. Can you please rebase your code changes?

@nhandler
Copy link
Contributor Author

@nhandler approved. Can you please rebase your code changes?

@devkimittal The branch has been rebased.

@rajatharanganath rajatharanganath merged commit ab94fa2 into drone-plugins:master Sep 1, 2023
@nhandler nhandler deleted the 110-optional-acl branch September 1, 2023 15:13
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.

3 participants