Account with NotPrincipal should explicitly provide permission

The is the bucket policy with below combination

Effect : DENY,
Action: $OPERATION,
NotPrincipal: $ACCOUNT

The permission on the "Action" for the $ACCOUNT mentioned in the
"NotPrincipal" should be explicitly given.

Example: For "Action: get_object", if we want to DENY permission
for * (all accounts) and we want to give explicit permission to
any one or few accounts, then we can include that account as
part of part of "NotPrincipal"

Check DFBUGS-1519 for steps to reproduce the issue

Fixes: https://issues.redhat.com/browse/DFBUGS-1519

Signed-off-by: Vinayakswami Hariharmath <[email protected]>

Author: vh05

src/endpoint/s3/s3_bucket_policy_utils.js

@@ -189,7 +189,7 @@ function _is_principal_fit(account, statement, ignore_public_principal = false)
     statement_principal = statement_principal.AWS ? statement_principal.AWS : statement_principal;
     for (const principal of _.flatten([statement_principal])) {
         dbg.log1('bucket_policy: ', statement.Principal ? 'Principal' : 'NotPrincipal', ' fit?', principal, account);
-        if ((principal.unwrap() === '*') || (principal.unwrap() === account)) {
+        if ((principal.unwrap() === '*') || (principal.unwrap() === account.id) || (principal.unwrap() === account.name)) {
             if (ignore_public_principal && principal.unwrap() === '*' && statement.Principal) {
                 // Ignore the "fit" if ignore_public_principal is requested
                 continue;

src/endpoint/s3/s3_rest.js

@@ -229,6 +229,24 @@ async function authorize_request_policy(req) {
     if (!req.params.bucket) return;
     if (req.op_name === 'put_bucket') return;
 
+    // Allow bucket owner or system owner to put a bucket policy
+    if (req.op_name === 'put_bucket_policy') {
+        const { system_owner, bucket_owner } = await req.object_sdk.read_bucket_sdk_policy_info(req.params.bucket);
+        const account = req.object_sdk.requesting_account;
+        const account_identifier_name = req.object_sdk.nsfs_config_root ? account.name.unwrap() : account.email.unwrap();
+        const account_identifier_id = req.object_sdk.nsfs_config_root ? account._id : undefined;
+
+        const is_system_owner = (Boolean(system_owner) && system_owner.unwrap() === account_identifier_name) ||
+                                (Boolean(system_owner) && system_owner.unwrap() === account_identifier_id);
+        const is_bucket_owner = (Boolean(bucket_owner) && bucket_owner.unwrap() === account_identifier_name) ||
+                                (Boolean(bucket_owner) && bucket_owner.unwrap() === account_identifier_id);
+
+        if (is_system_owner || is_bucket_owner) return;
+
+        // If the account is neither the system owner nor the bucket owner, deny access
+        throw new S3Error(S3Error.AccessDenied);
+    }
+
     // owner_account is { id: bucket.owner_account, email: bucket.bucket_owner };
     const { s3_policy, system_owner, bucket_owner, owner_account } = await req.object_sdk.read_bucket_sdk_policy_info(req.params.bucket);
     const auth_token = req.object_sdk.get_auth_token();
@@ -279,30 +297,24 @@ async function authorize_request_policy(req) {
         if (is_owner || is_iam_account_and_same_root_account_owner) return;
         throw new S3Error(S3Error.AccessDenied);
     }
-    // in case we have bucket policy
-    let permission_by_id;
-    let permission_by_name;
 
     const public_access_block_cfg = await req.object_sdk.get_public_access_block({ name: req.params.bucket });
     // In NC, we allow principal to be:
     // 1. account name (for backwards compatibility)
     // 2. account id
     // we start the permission check on account identifier intentionally
-    if (account_identifier_id) {
-        permission_by_id = await s3_bucket_policy_utils.has_bucket_policy_permission(
-            s3_policy, account_identifier_id, method, arn_path, req, public_access_block_cfg?.public_access_block?.restrict_public_buckets);
-        dbg.log3('authorize_request_policy: permission_by_id', permission_by_id);
-    }
-    if (permission_by_id === "DENY") throw new S3Error(S3Error.AccessDenied);
 
-    if ((!account_identifier_id || permission_by_id !== "DENY") && account.owner === undefined) {
-        permission_by_name = await s3_bucket_policy_utils.has_bucket_policy_permission(
-            s3_policy, account_identifier_name, method, arn_path, req, public_access_block_cfg?.public_access_block?.restrict_public_buckets
-        );
-        dbg.log3('authorize_request_policy: permission_by_name', permission_by_name);
-    }
-    if (permission_by_name === "DENY") throw new S3Error(S3Error.AccessDenied);
-    if ((permission_by_id === "ALLOW" || permission_by_name === "ALLOW") || is_owner) return;
+    const account_identifier = {
+        id: account_identifier_id,
+        name: account_identifier_name,
+    };
+
+    const bucket_permission = await s3_bucket_policy_utils.has_bucket_policy_permission(
+        s3_policy, account_identifier, method, arn_path, req, public_access_block_cfg?.public_access_block?.restrict_public_buckets);
+    dbg.log3('authorize_request_policy: bucket_permission', bucket_permission);
+
+    if (bucket_permission === "DENY") throw new S3Error(S3Error.AccessDenied);
+    if (bucket_permission === "ALLOW") return;
 
     throw new S3Error(S3Error.AccessDenied);
 }

src/test/unit_tests/test_s3_bucket_policy.js

@@ -1645,6 +1645,51 @@ mocha.describe('s3_bucket_policy', function() {
             Bucket: BKT,
             Key: KEY,
         }));
+
+    });
+
+    mocha.it('should be able to use notPrincipal with Effect Deny', async function() {
+        const self = this; // eslint-disable-line no-invalid-this
+        self.timeout(15000);
+        const auth_put_policy = {
+        Version: '2012-10-17',
+        Statement: [
+            {
+                Effect: 'Allow',
+                Principal: { AWS: [user_a, user_b] },
+                Action: ['s3:*'],
+                Resource: [`arn:aws:s3:::${BKT}/*`]
+            },
+            {
+                Effect: 'Deny',
+                NotPrincipal: { AWS: user_a },
+                Action: ['s3:GetObject'],
+                Resource: [`arn:aws:s3:::${BKT}/*`]
+            }
+        ]};
+        let res = await s3_owner.putBucketPolicy({
+            Bucket: BKT,
+            Policy: JSON.stringify(auth_put_policy)
+        });
+        assert.equal(res.$metadata.httpStatusCode, 200);
+
+        res = await s3_a.putObject({
+            Body: BODY,
+            Bucket: BKT,
+            Key: KEY,
+        });
+        assert.equal(res.$metadata.httpStatusCode, 200);
+
+        res = await s3_a.getObject({
+            Bucket: BKT,
+            Key: KEY,
+        });
+        assert.equal(res.$metadata.httpStatusCode, 200);
+
+        await assert_throws_async(s3_b.getObject({
+            Bucket: BKT,
+            Key: KEY,
+        }));
     });
 
     mocha.it('should be able to use notResource', async function() {