Skip to content

NC | NSFS | Stop Setting system_owner in Bucket Config #8192

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

Merged
merged 1 commit into from
Aug 8, 2024

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Jul 10, 2024

Explain the changes

  1. Remove the property system_owner as required property in nsfs_bucket_schema.
  2. On the newly created bucket, do not set the system_owner value (on manage_nsfs and bucketspace_fs).
  3. Remove the system_owner (with the value of the account name) from the current docs in NC NSFS.
  4. Update unit test tests and remove the property system_owner.
  5. Add the function modify_bucket_on_read in class ConfigFS, and use it from get_bucket_by_name and inside the bucket list in noobaa-cli.

Issues: Fixed #7793 BZ 2280212

  1. Currently, in the NC NSFS bucket config the system_owner is the account name, and therefore bucket policies can't limit a bucket owner's access to buckets that he created.

Testing Instructions:

Unit Tests:

Please run:

  1. sudo NC_CORETEST=true node --trace-warnings ./node_modules/mocha/bin/mocha ./src/test/unit_tests/test_s3_bucket_policy.js (for NC environment, currently you will 3 failing tests - not related to this PR)
  2. make CONTAINER_PLATFORM=linux/arm64 run-single-test testname=test_s3_bucket_policy.js (for Containerized environment)
    Note: I have MacOS M1 so I need to add the flag CONTAINER_PLATFORM=linux/arm64.
  3. npx jest test_nc_nsfs_bucket_schema_validation.test.js
  4. sudo node ./node_modules/.bin/_mocha src/test/unit_tests/test_nc_nsfs_cli.js
  5. npx jest test_config_fs.test.js

Manual Tests:

(1) On new buckets:

  1. First, create the FS_ROOT and a directory for a new_buckets_path: mkdir -p /tmp/nsfs_root1/ and give permissions chmod 777 /tmp/nsfs_root1/.
  2. Create an account: sudo node src/cmd/manage_nsfs account add --name <account-name> --uid <uid> --gid <gid> --new_buckets_path <new_buckets_path> (use those credentials in the alias step below).
  3. Start the NSFS server with: sudo node src/cmd/nsfs --debug 5
  4. Create the alias for the created account in the S3 service: alias s3-nc-user-1='AWS_ACCESS_KEY_ID=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:6443/'
    Note: I Change the config.NSFS_CHECK_BUCKET_BOUNDARIES = false; //SDSD because I'm using the /tmp/ and not /private/tmp/.
  5. Check the connection to the server (should not have buckets): s3-nc-user-1 s3 ls
  6. Create a bucket: s3-nc-user-1 s3 mb s3://my-bucket-to-check
  7. Create a file: touch denied_test_obj.txt
  8. Upload the file to the bucket: s3-nc-user-1 s3 cp denied_test_obj.txt s3://my-bucket-to-check
  9. See that the user can download the file: s3-nc-user-1 s3 cp s3://my-bucket-to-check/denied_test_obj.txt ~/Downloads/
  10. Put bucket policy: s3-nc-user-1 s3api put-bucket-policy --bucket my-bucket-to-check --policy file://policy.json, the policy is:
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Deny",
            "Action": "s3:*",
            "Principal": {
                "AWS": "*"
            },
            "Resource": "arn:aws:s3:::my-bucket-to-check/denied_test_obj.txt"
        }
    ]
}

(this policy would deny a bucket owner account from accessing an object inside its bucket)
13. See that you cannot download the file: s3-nc-user-1 s3 cp s3://my-bucket-to-check/denied_test_obj.txt ~/Downloads/ (I saw: fatal error: An error occurred (403) when calling the HeadObject operation: Forbidden).
14. (optional) Delete the bucket policy and try again (should download the file):

  • s3-nc-user-1 s3api delete-bucket-policy --bucket my-bucket-to-check
  • s3-nc-user-1 s3 cp s3://my-bucket-to-check/denied_test_obj.txt ~/Downloads/

(2) On buckets that already exist and had the system_owner property - they would still have the mentioned issue.
So want to test that we can operate on buckets that has the property system_owner:

  1. First, create the FS_ROOT and a directory for a new_buckets_path: mkdir -p /tmp/nsfs_root1/ and give permissions chmod 777 /tmp/nsfs_root1/.
  2. Create an account: sudo node src/cmd/manage_nsfs account add --name shira-1001 --uid 1001 --gid 1001 --new_buckets_path /tmp/nsfs_root1/ (use those credentials in the alias step below).
  3. Start the NSFS server with: sudo node src/cmd/nsfs --debug 5
  4. Create the alias for the created account in the S3 service: alias s3-nc-user-1='AWS_ACCESS_KEY_ID=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:6443/'
    Note: I Change the config.NSFS_CHECK_BUCKET_BOUNDARIES = false; //SDSD because I'm using the /tmp/ and not /private/tmp/.
  5. Check the connection to the server (should not have buckets): s3-nc-user-1 s3 ls
  6. Create a bucket using the CLI (it doesn't matter if CLI or S3 flow for this purpose): sudo node src/cmd/manage_nsfs bucket add --name my-bucket --path /tmp/nsfs_root1/my-bucket --owner shira-1001
  7. List the bucket: s3-nc-user-1 s3 ls
  8. Edit the bucket config file using sudo vi /etc/noobaa.conf.d/buckets/my-bucket.json and add to it: "system_owner":"shira-1001"
  9. List the bucket: s3-nc-user-1 s3 ls (to test that we can list the bucket even if we have this property).
  10. Create a file: touch denied_test_obj.txt
  11. Upload the file to the bucket: s3-nc-user-1 s3 cp denied_test_obj.txt s3://my-bucket
  12. See that the user can download the file: s3-nc-user-1 s3 cp s3://my-bucket/denied_test_obj.txt ~/Downloads/
  • As mentioned on buckets that were already created in the system, could not use bucket policy to limit a bucket owner's access to buckets that he created.
  • Doc added/updated
  • Tests added

@shirady shirady self-assigned this Jul 10, 2024
@shirady shirady requested a review from alphaprinz July 18, 2024 05:46
@shirady shirady force-pushed the nsfs-nc-system-owner branch 2 times, most recently from d380add to 5b2872a Compare July 18, 2024 12:44
@shirady shirady requested a review from romayalon July 21, 2024 06:30
@shirady shirady force-pushed the nsfs-nc-system-owner branch from 5b2872a to 55ec87c Compare July 23, 2024 08:20
@shirady shirady mentioned this pull request Jul 31, 2024
2 tasks
@shirady shirady force-pushed the nsfs-nc-system-owner branch 3 times, most recently from 9d12dae to 12b0020 Compare August 7, 2024 12:46
@pull-request-size pull-request-size bot added size/L and removed size/M labels Aug 7, 2024
@shirady shirady force-pushed the nsfs-nc-system-owner branch 3 times, most recently from 3540d13 to a60c619 Compare August 8, 2024 05:07
@shirady shirady requested review from romayalon and guymguym August 8, 2024 06:30
@@ -230,7 +230,7 @@ async function authorize_request_policy(req) {

const account = req.object_sdk.requesting_account;
const account_identifier = req.object_sdk.nsfs_config_root ? account.name.unwrap() : account.email.unwrap();
const is_system_owner = account_identifier === system_owner.unwrap();
const is_system_owner = Boolean(system_owner) && system_owner.unwrap() === account_identifier;

// @TODO: System owner as a construct should be removed - Temporary
Copy link
Member

Choose a reason for hiding this comment

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

this todo is still needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to check, if needed will be handled in a different PR.

Copy link
Member

Choose a reason for hiding this comment

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

I added this comment when I was working on removing allowed_buckets. This was added as a way to fix multiple authorization issue that started popping up primarily because some flows expected system owner to have "superuser" privileges so this particular fix addressed that.

If this PR is addressing the same concerns then maybe we can remove this comment. Moreover, I no longer remember why we thought that we can totally get rid of the system_owner construct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tangledbytes In this PR we remove the system_owner as a property in the bucket config - so it addresses it only in the NC NSFS environment. If I understand you, your change was in the Containerized environment, so I cannot remove this comment, right?

BTW, what does it mean "System owner as a construct"?

…cket config

1. Remove the property system_owner as required property in nsfs_bucket_schema.
2. On the newly created bucket, do not set the system_owner value (on manage_nsfs and bucketspace_fs).
3. Remove the system_owner (with the value of the account name) from the current docs in NC NSFS.
4. Update unit test tests and remove the property system_owner.
5. Add the function modify_bucket_on_read in class ConfigFS, and use it from get_bucket_by_name and inside the bucket list in noobaa-cli.

Signed-off-by: shirady <[email protected]>
@shirady shirady force-pushed the nsfs-nc-system-owner branch from a60c619 to 8d110bd Compare August 8, 2024 10:19
@shirady shirady merged commit ad73e9c into noobaa:master Aug 8, 2024
10 checks passed
@shirady shirady deleted the nsfs-nc-system-owner branch August 8, 2024 10:53
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.

NSFS | NC | CLI | Remove system owner from bucket config files and bucket schema
4 participants