Skip to content

NSFS | NC | IAM Service - Accounts Permission When No Bucket Policy#8175

Merged
shirady merged 1 commit into
noobaa:masterfrom
shirady:nsfs-iam-account-implicit-policy
Jul 28, 2024
Merged

NSFS | NC | IAM Service - Accounts Permission When No Bucket Policy#8175
shirady merged 1 commit into
noobaa:masterfrom
shirady:nsfs-iam-account-implicit-policy

Conversation

@shirady

@shirady shirady commented Jul 1, 2024

Copy link
Copy Markdown
Contributor

Explain the changes

  1. Add more properties to nsfs_bucket_schema (not required) and update the new_bucket_defaults in BucketSpaceFS:
  • creator = creator is the account ID that created this bucket (internal information).
    Notes: Currently we do not allow IAM accounts to create a bucket (temporary), it will be changed after we have the config structure, therefore in the future, we could see the IAM account ID in the creator property.
  1. Change the condition in authorize_request_policy (in s3_rest) and has_bucket_action_permission (in bucketspace_fs) for the same root account (alternative for ownership when there is no bucket policy).

Those next changes are not related to IAM, but were raised as a part of the code review:
3. In authorize_request_policy (in s3_rest) remove the condition req.object_sdk.nsfs_config_root from the owner condition.
4. In has_bucket_action_permission (in bucketspace_fs) change the condition of is_owner from account name to id.

Issues:

List of GAPs:

  1. Move the mutual code of AccountSpaceFS, BucketSpaceFS, and Manage NSFS to a specific file.
  2. Allow IAM users to create buckets (see comment below).

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. npx jest test_nc_nsfs_bucket_schema_validation.test.js.

Manual Tests:

  1. Create the 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>
    Note: before creating the account need to give permission to the new_buckets_path: chmod 777 /tmp/nsfs_root1.
  2. Start the NSFS server with: sudo node src/cmd/nsfs --debug 5 --https_port_iam 7005
    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 IAM service: alias s3-nc-user-1-iam='AWS_ACCESS_KEY_ID=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:7005'.
  2. Use AWS CLI to create 2 IAM accounts:
    user1: s3-nc-user-1-iam iam create-user --user-name Bob + s3-nc-user-1-iam iam create-access-key --user-name Bob
    user2: s3-nc-user-1-iam iam create-user --user-name Robert + s3-nc-user-1-iam iam create-access-key --user-name Robert
  3. Create an alias for each one of the accounts for using S3 endpoint, for example: alias s3-nc-user-1-s3-regular='AWS_ACCESS_KEY_ID=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:6443' (using different port). (the second one will be s3-nc-user-2-s3-regular).
  4. List the buckets from user1 and user2 (expect no buckets): s3-nc-user-1-s3-regular s3 ls, s3-nc-user-2-s3-regular s3 ls
  5. Create a bucket from the root account: s3-nc-user-1-s3 s3 mb s3://bucket-01
  6. List the buckets from user1 and user2 (expect to see the bucket from both users, same root account): s3-nc-user-1-s3-regular s3 ls, s3-nc-user-2-s3-regular s3 ls.
  7. Delete the bucket by user2: s3-nc-user-2-s3-regular s3 rb s3://bucket-01/.
  • Doc added/updated
  • Tests added

@shirady shirady self-assigned this Jul 1, 2024
@shirady shirady force-pushed the nsfs-iam-account-implicit-policy branch 2 times, most recently from adbef9f to cd8909b Compare July 7, 2024 06:20
@shirady shirady force-pushed the nsfs-iam-account-implicit-policy branch 2 times, most recently from 764d0ca to 86b2e88 Compare July 15, 2024 07:55
Comment thread src/sdk/bucketspace_fs.js Outdated
Comment thread docs/design/iam.md Outdated
@shirady shirady force-pushed the nsfs-iam-account-implicit-policy branch from 86b2e88 to 07efca5 Compare July 15, 2024 15:11
Comment thread src/endpoint/s3/s3_rest.js Outdated
Comment thread src/manage_nsfs/manage_nsfs_cli_utils.js Outdated
Comment thread src/manage_nsfs/nc_utils.js Outdated
Comment thread src/sdk/bucketspace_fs.js Outdated
Comment thread src/sdk/bucketspace_fs.js Outdated
Comment thread src/sdk/bucketspace_fs.js Outdated
Comment thread src/sdk/bucketspace_fs.js Outdated
Comment thread src/sdk/bucketspace_fs.js Outdated
@shirady shirady force-pushed the nsfs-iam-account-implicit-policy branch from 07efca5 to 868a624 Compare July 17, 2024 11:01
@shirady shirady requested review from guymguym and romayalon July 17, 2024 11:41
@shirady shirady mentioned this pull request Jul 17, 2024
2 tasks
@shirady shirady requested a review from alphaprinz July 18, 2024 05:47
@shirady shirady force-pushed the nsfs-iam-account-implicit-policy branch from 868a624 to 748167e Compare July 23, 2024 07:11
Comment thread src/sdk/bucketspace_fs.js Outdated
@shirady shirady force-pushed the nsfs-iam-account-implicit-policy branch 2 times, most recently from 1d24f6f to 31deb5c Compare July 24, 2024 09:18
@shirady shirady requested a review from guymguym July 24, 2024 09:57
@shirady shirady force-pushed the nsfs-iam-account-implicit-policy branch from 31deb5c to 5e4e47a Compare July 25, 2024 12:25
1. Add more properties to nsfs_bucket_schema (not required) and update the new_bucket_defaults in BucketSpaceFS:
   - creator = creator is the account ID that created this bucket (internal information).
     Notes: Currently we do not allow IAM accounts to create a bucket (temporary), it will be changed after we have the config structure, therefore in the future, we could see the IAM account ID in the creator property.
2. Change the condition in authorize_request_policy (in s3_rest) and has_bucket_action_permission (in bucketspace_fs) for the same root account (alternative for ownership when there is no bucket policy).

Those next changes are not related to IAM, but were raised as a part of the code review:
3. In authorize_request_policy (in s3_rest) remove the condition req.object_sdk.nsfs_config_root from the owner condition.
4. In has_bucket_action_permission (in bucketspace_fs) change the condition of is_owner from account name to id.

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
@shirady shirady force-pushed the nsfs-iam-account-implicit-policy branch from 5e4e47a to c65e20d Compare July 28, 2024 06:35
@shirady shirady merged commit c5026ab into noobaa:master Jul 28, 2024
@shirady shirady deleted the nsfs-iam-account-implicit-policy branch July 28, 2024 07:07
@shirady

shirady commented Jul 28, 2024

Copy link
Copy Markdown
Contributor Author

Internal Documentation of the PR:

Before adding the limitation of bucket creation on IAM accounts - the options were discussed in this PR related to the function new_bucket_defaults (in the file bucketspace_fs.js):

  1. Copy the system_owner and the bucket_owner from the IAM account - the worst solution, even if we want to remove those properties in the future, attaching them with the IAM account details is wrong.
  2. Copy the system_owner and the bucket_owner from the root account - those fields are the names, and when we have the IAM account config we have the property of owner which is ID so to get it we thought of:
    (1) Iterate all the accounts and search for the account that has this ID - this would damage the performance of this operation.
    (2) Add temporary property owner_name in the account schema and config - we would still have issues with accounts that change their name.

Therefore, we add the limitation for creating a bucket by the IAM account, that we will remove once we could get the root account config by ID.

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.

3 participants