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

[s3] Support ACL #4519

Open
kmlebedev opened this issue May 31, 2023 · 8 comments
Open

[s3] Support ACL #4519

kmlebedev opened this issue May 31, 2023 · 8 comments

Comments

@kmlebedev
Copy link
Contributor

kmlebedev commented May 31, 2023

Need to add ACL support so that s3 tests pass
and #4259

#4090
#3849
#3848
#3846
#3845
#3844
#3843
#3842

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

@kmlebedev
Copy link
Contributor Author

Similar to Iam Identity file, we store acl for buckets in the original AWS json with ACL

{
  "Grants": [
    {
      "Grantee": {
        "DisplayName": "string",
        "EmailAddress": "string",
        "ID": "string",
        "Type": "CanonicalUser"|"AmazonCustomerByEmail"|"Group",
        "URI": "string"
      },
      "Permission": "FULL_CONTROL"|"WRITE"|"WRITE_ACP"|"READ"|"READ_ACP"
    }
    ...
  ],
  "Owner": {
    "DisplayName": "string",
    "ID": "string"
  }
}

https://awscli.amazonaws.com/v2/documentation/api/latest/reference/s3api/put-bucket-acl.html#options

First of all, I want to get Canned ACL
https://docs.aws.amazon.com/AmazonS3/latest/userguide/acl-overview.html

@kmlebedev
Copy link
Contributor Author

@chrislusf @shichanglin5

Hi, I tried to refactor the PR #4090
But there are too many changes, so I will split it into several preparatory PRs for passing s3 integration tests

Step 0: Passing test s3tests_boto3/functional/test_s3.py::test_bucket_acl_default
Step 1: Move s3account.AccountManager struct into iam.S3ApiConfiguration
Step 2: Create global s3 ACL setting (BlockPublicAcls, IgnorePublicAcls, BlockPublicPolicy, RestrictPublicBuckets)
https://docs.aws.amazon.com/AmazonS3/latest/userguide/access-control-block-public-access.html
Step 3: Move BucketMetaData struct into iam.S3ApiConfiguration
Step 4: Passing tests:

s3tests_boto3/functional/test_s3.py::test_bucket_acl_canned_during_create 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_canned 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_canned_publicreadwrite 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_canned_authenticatedread 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_canned_private_to_private

Step 5: Passing tests:

s3tests_boto3/functional/test_s3.py::test_bucket_acl_grant_userid_fullcontrol 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_grant_userid_read 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_grant_userid_readacp 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_grant_userid_write 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_grant_userid_writeacp 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_grant_nonexist_user 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_no_grants 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_grant_email 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_grant_email_not_exist 
s3tests_boto3/functional/test_s3.py::test_bucket_acl_revoke_all 

@chrislusf
Copy link
Collaborator

Thanks! Maybe also write up some design doc for easier understanding.

kmlebedev pushed a commit to kmlebedev/seaweedfs that referenced this issue Sep 20, 2023
test_bucket_acl_default
test_bucket_acl_canned_private_to_private

seaweedfs#4519
@kmlebedev
Copy link
Contributor Author

Thanks! Maybe also write up some design doc for easier understanding.

  • Put and GET Bucket ACL handlers interact with the S3ApiServer.IdentityAccessManagement.acl() interface.
    If the data changes, then the entire structure is saved to a file in the filer. Other s3-api are subscribed to changes in this file and re-read it into S3ApiServer.IdentityAccessManagement structure

  • Verification of access by Credential and ACL is carried out centrally in one place S3ApiServer.IdentityAccessManagement.Auth(), where all the necessary data for this is contained within the structure (without external calls, for example to a filer)

  • A redundant concept of an account is introduced, which may be the owner of resources with access granted according to this criterion. At the first stage, we will divide into customizable admin and public (anonymous) accounts.

  • Put and Get Object ACL handlers interact with the entry metadata in the filer

@shichanglin5
Copy link
Contributor

Very happy to see this!

By the way, I think there is a better way than splite ACL into several PR:
Create a new feature branch, add a commit instead of a pr for each functional change. finally, run the integration test after the development is completed, and merge to the main branch after no problem.

Otherwise, the function of the master branch is incomplete and cannot be used, and there is no integration test. I wonder if there will be any other problems.

@kmlebedev
Copy link
Contributor Author

Very happy to see this!

By the way, I think there is a better way than splite ACL into several PR: Create a new feature branch, add a commit instead of a pr for each functional change. finally, run the integration test after the development is completed, and merge to the main branch after no problem.

Otherwise, the function of the master branch is incomplete and cannot be used, and there is no integration test. I wonder if there will be any other problems.

This feature is a branch with 4k lines of changes and that’s not all that I would like to fix. It’s impossible to review it, not even for me.
new feature branch: https://github.com/seaweedfs/seaweedfs/pull/4520/files

@shichanglin5 I have an important question about backward compatibility with your current installation where ACL in some form already works. Do I need to think about this?

@shichanglin5
Copy link
Contributor

Very happy to see this!
By the way, I think there is a better way than splite ACL into several PR: Create a new feature branch, add a commit instead of a pr for each functional change. finally, run the integration test after the development is completed, and merge to the main branch after no problem.
Otherwise, the function of the master branch is incomplete and cannot be used, and there is no integration test. I wonder if there will be any other problems.

This feature is a branch with 4k lines of changes and that’s not all that I would like to fix. It’s impossible to review it, not even for me. new feature branch: https://github.com/seaweedfs/seaweedfs/pull/4520/files

@shichanglin5 I have an important question about backward compatibility with your current installation where ACL in some form already works. Do I need to think about this?

It would be great if it were compatible, thank you.

kmlebedev pushed a commit to kmlebedev/seaweedfs that referenced this issue Sep 21, 2023
chrislusf pushed a commit that referenced this issue Sep 21, 2023
optimization iam lookup for reducing algorithm complexity
#4519

Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>
chrislusf added a commit that referenced this issue Sep 21, 2023
…ivate. (#4856)

* Passing test:
test_bucket_acl_default
test_bucket_acl_canned_private_to_private

#4519

* Update weed/s3api/s3api_bucket_handlers.go

---------

Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>
Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
kmlebedev pushed a commit to kmlebedev/seaweedfs that referenced this issue Sep 21, 2023
chrislusf pushed a commit that referenced this issue Sep 21, 2023
… acl (#4858)

Replace action read/write to readAcp/writeAcp for handlers with acl query
 #4519

Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>
kmlebedev pushed a commit to kmlebedev/seaweedfs that referenced this issue Sep 22, 2023
chrislusf added a commit that referenced this issue Sep 25, 2023
…ration (#4859)

* move s3account.AccountManager into to iam.S3ApiConfiguration and switch to Interface

#4519

* fix: test bucket acl default and
adjust the variable names

* fix: s3 api config test

---------

Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>
Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
kmlebedev added a commit to kmlebedev/seaweedfs that referenced this issue Dec 22, 2023
…edfs#4857)

optimization iam lookup for reducing algorithm complexity
seaweedfs#4519

Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>
kmlebedev added a commit to kmlebedev/seaweedfs that referenced this issue Dec 22, 2023
…ivate. (seaweedfs#4856)

* Passing test:
test_bucket_acl_default
test_bucket_acl_canned_private_to_private

seaweedfs#4519

* Update weed/s3api/s3api_bucket_handlers.go

---------

Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>
Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
kmlebedev added a commit to kmlebedev/seaweedfs that referenced this issue Dec 22, 2023
… acl (seaweedfs#4858)

Replace action read/write to readAcp/writeAcp for handlers with acl query
 seaweedfs#4519

Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>
kmlebedev added a commit to kmlebedev/seaweedfs that referenced this issue Dec 22, 2023
…ration (seaweedfs#4859)

* move s3account.AccountManager into to iam.S3ApiConfiguration and switch to Interface

seaweedfs#4519

* fix: test bucket acl default and
adjust the variable names

* fix: s3 api config test

---------

Co-authored-by: Konstantin Lebedev <9497591+kmlebedev@users.noreply.github.co>
Co-authored-by: Chris Lu <chrislusf@users.noreply.github.com>
@nawbc
Copy link

nawbc commented May 6, 2024

Hey, folks. any updates ?

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

No branches or pull requests

4 participants