apache / incubator-answer-plugins

The Apache Answer plugins repository.
https://answer.apache.org/plugins
Apache License 2.0
62 stars 31 forks source link

Storage S3: Public Read ACL #97

Closed calmdev closed 3 months ago

calmdev commented 3 months ago

I was trying the storage s3 plugin with digital ocean which is working, but noticed that all uploaded files are private by default. So when the files upload they appear broken on the website. Adding an ACL with value public-read fixes the permissions issue allowing the s3 files to load:

diff --git a/storage-s3/s3_client.go b/storage-s3/s3_client.go
index 445e1dd..332625a 100644
--- a/storage-s3/s3_client.go
+++ b/storage-s3/s3_client.go
@@ -55,6 +55,7 @@ func (s *Client) PutObject(key, ext string, file io.ReadSeeker) (err error) {
        }
        contentType := fmt.Sprintf("image/%s", strings.TrimPrefix(ext, "."))
        _, err = s3.New(newSession).PutObject(&s3.PutObjectInput{
+               ACL:         aws.String("public-read"),
                Body:        file,
                Bucket:      aws.String(s.bucket),
                Key:         aws.String(key),

No ACL:

https://github.com/apache/incubator-answer-plugins/assets/461249/10c9191a-87d6-4102-8a9a-885aa432c8e0

With ACL:

https://github.com/apache/incubator-answer-plugins/assets/461249/30290c87-25c0-4fbf-9c28-53ee805b2bc8

LinkinStars commented 3 months ago

@calmdev Thanks for feedback. In the official AWS documentation, there is a mention of setting ACLs (Access Control Lists) for objects, and the official recommendation is not to set ACLs in that way. If I understand correctly, the recommendation is to set ACLs at the bucket level instead of individual objects. Sorry, as I haven't used DigitalOcean, I am not certain about if DigitalOcean allows setting ACLs at the bucket level. 🤔

FYI: https://docs.aws.amazon.com/AmazonS3/latest/userguide/acl-overview.html

A majority of modern use cases in Amazon S3 no longer require the use of ACLs. We recommend that you keep ACLs disabled, except in unusual circumstances where you need to control access for each object individually.

In AWS sdk, there is the comment in code.

image

github.com/aws/aws-sdk-go@v1.44.314/service/s3/api.go

calmdev commented 3 months ago

Hey I'm not familiar with S3 on AWS, but the digital ocean spaces API is supposed to be compatible with AWS S3. I don't see any options around configuring default file permissions for a bucket in the UI. On AWS is this typically done in the UI or using an external app/cli to configure the bucket itself? Sounds like it uses something other than ACL? Trying to wrap my head around it, because if it's not applied to the files I'm assuming there is some default settings that are configured which applies to all files placed into the bucket store. I've just noticed by default all the files are private and show a forbidden response when accessed until the file ACL was added. So not exactly sure what I'm looking for, but I can keep digging.

FWIW, bucket ACLs are mentioned here: https://docs.digitalocean.com/reference/api/spaces-api/

calmdev commented 3 months ago

@LinkinStars – after reading more here: https://docs.digitalocean.com/products/spaces/reference/s3cmd-usage/

I set ACL on bucket itself:

s3cmd --access_key=YOUR_ACCESS_KEY \
--secret_key=YOUR_SECRET_KEY \
--host=YOUR_BUCKET_REGION.digitaloceanspaces.com \
--host-bucket=YOUR_BUCKET_NAME.YOUR_BUCKET_REGION.digitaloceanspaces.com \
--region=YOUR_BUCKET_REGION \
setacl s3://YOUR_BUCKET_NAME \
--acl-public

Which produces the following output:

s3://godev/: ACL set to Public 

Then I noticed the ACL toggled only seems to control listing contents of a bucket.

There is a UI note:

Important: This setting has no effect on whether individual files are visible. It only determines if anonymous users can list the name, size, and other metadata for the files in this Spaces Bucket.

So it doesn't seem to solve the problem. Files are created, but still not accessible by default on digital ocean.

Screenshot:

Screenshot 2024-04-01 at 7 31 29 PM
LinkinStars commented 3 months ago

@calmdev Thanks for the link you gave. I read the documentation in it in detail and found the following.

Firstly, the code provided by the official DigitalOcean documentation include the ACL configuration. Therefore, this is a necessary option for DigitalOcean.

https://docs.digitalocean.com/reference/api/spaces-api/#hello-world-program

image

Secondly, I found the following description in the documentation. It states that even if the bucket is set to public, any objects uploaded through the s3api will default to private if the ACL is not set. This is the reason behind this issue.

https://docs.digitalocean.com/reference/api/spaces-api/#upload-an-object-put

image

In conclusion, while DigitalOcean is compatible with the S3 API, there are still some differences. My suggestion is that you can continue using it with your modifications and see if there are any other variations. As for the solutions, we can discuss two options:

  1. Use an S3 plugin that provides an environment variable to determine if the user is a DigitalOcean user. If so, add the ACL during the upload process.
  2. Develop a separate plugin specifically for DigitalOcean. I can handle this task if needed.
calmdev commented 3 months ago

Ok from what I've seen with initial tests everything else worked fine. I tried uploading various files for avatar, posts, branding, etc. They all worked. It was only the ACL issue that was a problem. I'd lean towards option 1 since it's only a small difference. Wouldn't make a separate plugin unless there are enough differences to make it worth splitting. My thought anyways.

LinkinStars commented 3 months ago

I'd lean towards option 1 since it's only a small difference. Wouldn't make a separate plugin unless there are enough differences to make it worth splitting.

@calmdev I agree. You can try using version 1.2.6 of the storage-s3 plugin. To resolve this issue, set the environment variable ACL_PUBLIC_READ to true. We currently don't have a DigitalOcean testing environment, so if you encounter any other problems during the usage, please continue to raise them. Thanks.

calmdev commented 3 months ago

Awesome. The change looks good.

Closing this ticket. I'll open a new one if I find anything else.