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

NC | NSFS | Bucket Policy With Principal as account ID #8280

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Aug 13, 2024

Explain the changes

This PR is based on @alphaprinz #8167 PR.

  1. In s3_rest under the function authorize_request_policy rename account_identifier to account_identifier_name and add account_identifier_id.
  2. In bucketspace_fs where we use put_bucket_policy and in manage_nsfs_validations where we validate the bucket policy use is_account_exists_by_principal that accepts both account name and id.
  3. Both in authorize_request_policy (s3_rest) and has_bucket_action_permission (bucketspace_fs) check the principal by ID first and then the principal by name.

Issues: Fixed #xxx / Gap #xxx

List of GAPs

  1. We have code duplication that we need to handle - see this comment in bucketspace_fs:
    // TODO: move the following 3 functions - has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir()
    // so they can be re-used
  2. Currently, we don't support the principal as ARN.
  3. We need to fix issue Noobaa mistakenly accepts NotPrincipal with Effect: Allow #8176, in this PR the related test (that begun failing) was skipped.

Testing Instructions:

Unit Tests:

Please run:

  1. sudo NC_CORETEST=true node ./node_modules/mocha/bin/mocha ./src/test/unit_tests/test_bucketspace_fs.js
  2. sudo npx jest test_nc_nsfs_bucket_cli.test.js
  3. sudo NC_CORETEST=true node ./node_modules/mocha/bin/mocha ./src/test/unit_tests/test_s3_bucket_policy.js

Manual Tests:

High level - more details in the comment below:

  1. Create 2 root accounts (account-1, account-2).
  2. Create a bucket for one of the users, for example, account-1.
  3. Add bucket policy on this bucket where the Principal is the account id of the other account, account-2 ("Principal": { "AWS": [ "<account-id>" ] }).
  4. List the buckets from account-2 (see the bucket in the list).
  • Doc added/updated
  • Tests added

@shirady
Copy link
Contributor Author

shirady commented Aug 13, 2024

Additional Testing instructions:

  1. Create 2 root user account with the CLI:
  • sudo node src/cmd/manage_nsfs account add --name shira-1001 --new_buckets_path /tmp/nsfs_root1 --access_key <access-key> --secret_key <secret-key> --uid <uid> --gid <gid>
  • sudo node src/cmd/manage_nsfs account add --name shira-1002 --new_buckets_path /tmp/nsfs_root2 --access_key <access-key> --secret_key <secret-key> --uid <uid> --gid <gid>
    Note: before creating the account need to give permission to the new_buckets_path: chmod 777 /tmp/nsfs_root1, chmod 777 /tmp/nsfs_root2.
  1. Start the NSFS server with: sudo node src/cmd/nsfs --debug 5
    Notes:
  • Before starting the server please add this line: process.env.NOOBAA_LOG_LEVEL = ‘nsfs’; in the endpoint.js (before the condition if (process.env.NOOBAA_LOG_LEVEL) {)
  • I Change the config.NSFS_CHECK_BUCKET_BOUNDARIES = false; //SDSD because I’m using the /tmp/ and not /private/tmp/.
  1. Create the alias for S3 service:
  • alias nc-user-1-s3=‘AWS_ACCESS_KEY_ID=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:6443’.
  • alias nc-user-2-s3=‘AWS_ACCESS_KEY_ID=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:6443’.
  1. Check the connection to the endpoint and try to list the buckets (should be empty):
  • nc-user-1-s3 s3 ls; echo $?
  • nc-user-2-s3 s3 ls; echo $?
  1. Add bucket to shira-1001: sudo node src/cmd/manage_nsfs bucket add --name my-bucket --path /tmp/nsfs_root1/my-bucket --owner shira-1001
  2. List the buckets again:
  • nc-user-1-s3 s3 ls; echo $? (should see bucket my-bucket)
  • nc-user-2-s3 s3 ls; echo $? (should be empty)
  1. Add bucket policy using s3 api: nc-user-1-s3 s3api put-bucket-policy --bucket my-bucket --policy file://policy_by_id.json
    policy_by_id.json
{
  "Version": "2012-10-17",
  "Statement": [ 
    { 
     "Effect": "Allow", 
     "Principal": { "AWS": [ "<account-id>" ] }, 
     "Action": [ "s3:*" ], 
     "Resource": [ "arn:aws:s3:::my-bucket/*", "arn:aws:s3:::my-bucket" ] 
    }
  ]
}

note: replace the <account-id> with account ID of shira-1002
8. List the buckets again:

  • nc-user-1-s3 s3 ls; echo $? (should see bucket my-bucket)
  • nc-user-2-s3 s3 ls; echo $? (should also see bucket my-bucket)
  1. Put object by shira-1002 (should succeed): nc-user-2-s3 s3api put-object --bucket my-bucket --key hello
  2. Remove the bucket policy using the s3api: nc-user-1-s3 s3api delete-bucket-policy --bucket my-bucket
  3. Add bucket policy using the CLI: sudo node src/cmd/manage_nsfs bucket update --name my-bucket --bucket_policy '{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"AWS":["<account-id>"]},"Action":["s3:*"],"Resource":["arn:aws:s3:::my-bucket/*","arn:aws:s3:::my-bucket"]}]}'
    note: replace the <account-id> with account ID of shira-1002
  4. List the buckets again:
  • nc-user-1-s3 s3 ls; echo $? (should see bucket my-bucket)
  • nc-user-2-s3 s3 ls; echo $? (should also see bucket my-bucket)
  1. Remove the bucket policy using the CLI: sudo node src/cmd/manage_nsfs bucket update --name my-bucket --bucket_policy ''

Note: in order to see the changes immediately, change the Object SDK bucket cache expiration time config.OBJECT_SDK_BUCKET_CACHE_EXPIRY_MS = 1; //SDSD (before step2 starting the server).

@shirady shirady self-assigned this Aug 13, 2024
@shirady shirady force-pushed the nsfs-nc-bucket-policy-by-id branch from 3f6944e to b2b325a Compare August 20, 2024 06:47
Copy link
Contributor

@romayalon romayalon left a comment

Choose a reason for hiding this comment

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

I suggest to add full S3 flow test (test_s3_bucket_policy.js) for testing put_bucket_policy using S3 and the authorization of some functionalities

src/endpoint/s3/s3_bucket_policy_utils.js Outdated Show resolved Hide resolved
src/sdk/bucketspace_fs.js Outdated Show resolved Hide resolved
@shirady shirady force-pushed the nsfs-nc-bucket-policy-by-id branch from b2b325a to f4f5ba9 Compare August 26, 2024 07:38
@shirady shirady requested a review from romayalon August 26, 2024 09:37
src/sdk/bucketspace_fs.js Outdated Show resolved Hide resolved
src/sdk/config_fs.js Show resolved Hide resolved
src/test/unit_tests/test_bucketspace_fs.js Show resolved Hide resolved
@romayalon
Copy link
Contributor

@shirady looks good, added a few minor comments

@shirady shirady force-pushed the nsfs-nc-bucket-policy-by-id branch from a3537f7 to 8783f0b Compare August 28, 2024 10:30
@shirady shirady requested a review from romayalon August 28, 2024 11:01
This PR is based on @alphaprinz noobaa#8167 PR.

1. In s3_rest under the function authorize_request_policy  rename account_identifier to account_identifier_name and add account_identifier_id.
2. In bucketspace_fs where we use put_bucket_policy and in manage_nsfs_validations where we validate the bucket policy use is_account_exists_by_principal that accepts both account name and id.
3. Both in authorize_request_policy (s3_rest) and has_bucket_action_permission (bucketspace_fs) check the principal by ID first and then the principal by name.

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
@shirady shirady force-pushed the nsfs-nc-bucket-policy-by-id branch from 8783f0b to 4c60720 Compare August 28, 2024 12:35
@shirady shirady merged commit 29d705f into noobaa:master Aug 28, 2024
10 checks passed
@shirady shirady deleted the nsfs-nc-bucket-policy-by-id branch August 28, 2024 13:05
@nimrod-becker nimrod-becker mentioned this pull request Sep 4, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants