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 | Fix Issue DFBUGS-1307 | Bucket Policy With Principal as ID #8680

Merged
merged 1 commit into from
Jan 16, 2025

Conversation

shirady
Copy link
Contributor

@shirady shirady commented Jan 13, 2025

Explain the changes

  1. Change the condition in rest_s3 and bucketspace_fs to check the permission by the principal on account name when the previous check was only not DENY, so in case someone puts several statements using both account ID and account name, we won't skip it after the checks on the account ID.
  2. Add logs in level 3 to help investigate bucket policy issues.

Issues: Fixed Jira-1307

Currently, we had an issue when someone put a bucket policy with two statements:

  1. Allow by name to all principals ("*").
  2. Deny by account ID to a specific account.
    We used the first one only as it was not "IMPLICIT_DENY" and didn't check the second option.

GAPs:

  1. I think we need to create constants for the bucket policy and reuse them instead of a raw string (from example Deny. Allow) - opened Improve Bucket Policy Code Ideas (Use Enum) #8697.
  2. I think we need to add error logs in case of failure to help us with an investigation - opened NC | NSFS | Add Error Logs Printings In All bucketspace_fs Methods #8699, for example:
    } catch (err) {
    throw translate_error_codes(err, entity_enum.BUCKET);
    }

Testing Instructions:

Automatic test:

  1. In NC Please run: sudo NC_CORETEST=true node ./node_modules/mocha/bin/mocha ./src/test/unit_tests/test_s3_bucket_policy.js
  2. In containerized Please run: make run-single-test testname=test_s3_bucket_policy.js CONTAINER_PLATFORM=linux/arm64 (I have MacOS so I use the flag CONTAINER_PLATFORM=linux/arm64).

Manual Test:

  1. Create an account with the CLI: sudo node src/cmd/manage_nsfs account add --name <account-name> --new_buckets_path /Users/buckets/ --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 /Users/buckets/.
  2. Start the NSFS server with: sudo node src/cmd/nsfs --debug 5
  3. 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’.
  4. Check the connection to the endpoint and try to list the buckets (should be empty): nc-user-1-s3 s3 ls; echo $?
  5. Add bucket to the account using AWS CLI: nc-user-1-s3 s3 mb s3://bucket-1307 (bucket-1307 is the bucket name in this example)
  6. Put an object in the bucket: echo 'test_data' | nc-user-1-s3 s3 cp - s3://bucket-1307/test_object.txt
  7. Create two additional account with the CLI: sudo node src/cmd/manage_nsfs account add --name <account-name> --new_buckets_path /Users/buckets/ --access_key <access-key> --secret_key <secret-key> --uid <uid> --gid <gid> (use the same uid and gid as the first created account).
  8. Create the alias for the S3 service: alias nc-user-2-s3=‘AWS_ACCESS_KEY_ID= AWS_SECRET_ACCESS_KEY= aws --no-verify-ssl --endpoint-url https://localhost:6443’`.
  9. Check the connection to the endpoint and try to list the buckets (should be empty): nc-user-2-s3 s3 ls; echo $?

Statement with * allow, statement with account name deny
13. Put bucket policy (statement with account name): nc-user-1-s3 s3api put-bucket-policy --bucket bucket-1307 --policy file://policy1.json
policy1.json (replace the principal with the account name in the Deny statement):

{
    "Statement": [
       {
          "Effect": "Allow",
          "Principal": {
            "AWS": [
                "*"
            ]
          },
          "Action": "s3:*",
          "Resource": [
          "arn:aws:s3:::*",
          "arn:aws:s3:::bucket-1307/*"
        ]
       },
       {
          "Effect": "Deny",
          "Action": "s3:GetObject",
          "Principal": {
            "AWS": [
                "shira-1003-1"
            ]
          },          
          "Resource": [
            "arn:aws:s3:::bucket-1307/*"
          ]
        }
    ]
 }
  1. Account 2 tries to get the object: nc-user-2-s3 s3api get-object --bucket bucket-1307 --key test_object.txt /dev/stdout (should get AccessDenied error)

Statement with * allow, statement with account ID deny
15. Put bucket policy (statement with account ID): nc-user-1-s3 s3api put-bucket-policy --bucket bucket-1307 --policy file://policy2.json
policy2.json (replace the principal with the account ID in the Deny statement)::

{
    "Statement": [
       {
          "Effect": "Allow",
          "Principal": {
            "AWS": [
                "*"
            ]
          },
          "Action": "s3:*",
          "Resource": [
          "arn:aws:s3:::*",
          "arn:aws:s3:::bucket-1307/*"
        ]
       },
       {
          "Effect": "Deny",
          "Action": "s3:GetObject",
          "Principal": {
            "AWS": [
                "6784c11903ab3d2123b2a1c2"
            ]
          },          
          "Resource": [
            "arn:aws:s3:::bucket-1307/*"
          ]
        }
    ]
 }
  1. Account 2 tries to get the object: nc-user-2-s3 s3api get-object --bucket bucket-1307 --key test_object.txt /dev/stdout (should get AccessDenied error)
  • Doc added/updated
  • Tests added

@shirady shirady changed the title NC | NSFS | Fix Issue Jira-1307 | Bucket Policy With Principal as ID NC | NSFS | Fix Issue DFBUGS-1307 | Bucket Policy With Principal as ID Jan 13, 2025
@shirady shirady self-assigned this Jan 13, 2025
src/test/unit_tests/test_s3_bucket_policy.js Outdated Show resolved Hide resolved
src/test/unit_tests/test_s3_bucket_policy.js Outdated Show resolved Hide resolved
src/test/unit_tests/test_s3_bucket_policy.js Outdated Show resolved Hide resolved
@shirady shirady force-pushed the nsfs-nc-bucket-policy-issue branch 2 times, most recently from 2358091 to 0cef265 Compare January 14, 2025 13:35
@shirady shirady requested a review from romayalon January 14, 2025 14:21
@shirady shirady force-pushed the nsfs-nc-bucket-policy-issue branch from d207166 to f11e531 Compare January 15, 2025 09:00
@alphaprinz
Copy link
Contributor

LGTM :)

@shirady shirady force-pushed the nsfs-nc-bucket-policy-issue branch from f11e531 to d9af68c Compare January 16, 2025 07:40
1. Change the condition in rest_s3 and bucketspace_fs to check the permission by the principal on account name when the previous check was only not DENY,
    so in case someone puts several statements using both account ID and account name, we won't skip it after the checks on the account ID.
2. Add logs in level 3 to help investigate bucket policy issues.

Signed-off-by: shirady <[email protected]>
@shirady shirady force-pushed the nsfs-nc-bucket-policy-issue branch from cedf1f2 to c096f08 Compare January 16, 2025 09:05
@shirady shirady merged commit 873a00c into noobaa:master Jan 16, 2025
11 checks passed
@shirady shirady deleted the nsfs-nc-bucket-policy-issue branch January 16, 2025 09:46
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