[MCG NC] Bucket policy with deny statement using NotPrincipal also denies the principal

This is the put-bucket-policy case for combination of

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

The $ACCOUNT mentioned in the "NotPrincipal" should be
excluded from DENy operation and should be allowed on the
$OPERATION

Example: For operation get_object, if we have DENY effect
for * (all accounts) and we want to give access to any one
or few accounts, then that account can be part of "NotPrincipal"

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

@@ -146,18 +146,31 @@ async function _is_object_version_fit(req, predicate, value) {
     return res;
 }
 
-async function has_bucket_policy_permission(policy, account, method, arn_path, req) {
+async function has_bucket_policy_permission(policy, account, method, arn_path, req, account_identifier_type = 'id') {
     const [allow_statements, deny_statements] = _.partition(policy.Statement, statement => statement.Effect === 'Allow');
 
     // the case where the permission is an array started in op get_object_attributes
     const method_arr = Array.isArray(method) ? method : [method];
 
     // look for explicit denies
-    const res_arr_deny = await is_statement_fit_of_method_array(deny_statements, account, method_arr, arn_path, req);
+    const res_arr_deny = await is_statement_fit_of_method_array(deny_statements,
+        account,
+        method_arr,
+        arn_path,
+        req,
+        account_identifier_type
+    );
     if (res_arr_deny.every(item => item)) return 'DENY';
 
     // look for explicit allows
-    const res_arr_allow = await is_statement_fit_of_method_array(allow_statements, account, method_arr, arn_path, req);
+    const res_arr_allow = await is_statement_fit_of_method_array(
+        allow_statements,
+        account,
+        method_arr,
+        arn_path,
+        req,
+        account_identifier_type
+    );
     if (res_arr_allow.every(item => item)) return 'ALLOW';
 
     // implicit deny
@@ -177,9 +190,8 @@ function _is_action_fit(method, statement) {
     return statement.Action ? action_fit : !action_fit;
 }
 
-function _is_principal_fit(account, statement) {
+function _is_principal_fit(account, statement, account_identifier_type) {
     let statement_principal = statement.Principal || statement.NotPrincipal;
-
     let principal_fit = false;
     statement_principal = statement_principal.AWS ? statement_principal.AWS : statement_principal;
     for (const principal of _.flatten([statement_principal])) {
@@ -189,7 +201,23 @@ function _is_principal_fit(account, statement) {
             break;
         }
     }
-    return statement.Principal ? principal_fit : !principal_fit;
+
+    if (statement.NotPrincipal) {
+        // If the account is categorized as "Not Principal," we need to ALLOW operations
+        // for that account under the associated principal.
+        //
+        // To maintain backward compatibility, we also support name-based comparison for principal matching.
+        //
+        // - If `account_identifier_type` is "id" but the `account` parameter contains a name,
+        //   we return `false` to proceed/bypass to the next step, where we check for a name match.
+        //
+        // - In the next step, if `account_identifier_type` is "name" and the `account` also contains a name,
+        //   we return the actual value.
+
+        return account_identifier_type === 'id' ? principal_fit : !principal_fit;
+    }
+
+    return principal_fit;
 }
 
 function _is_resource_fit(arn_path, statement) {
@@ -207,15 +235,15 @@ function _is_resource_fit(arn_path, statement) {
     return statement.Resource ? resource_fit : !resource_fit;
 }
 
-async function is_statement_fit_of_method_array(statements, account, method_arr, arn_path, req) {
+async function is_statement_fit_of_method_array(statements, account, method_arr, arn_path, req, account_identifier_type) {
     return Promise.all(method_arr.map(method_permission =>
-        _is_statements_fit(statements, account, method_permission, arn_path, req)));
+        _is_statements_fit(statements, account, method_permission, arn_path, req, account_identifier_type)));
 }
 
-async function _is_statements_fit(statements, account, method, arn_path, req) {
+async function _is_statements_fit(statements, account, method, arn_path, req, account_identifier_type) {
     for (const statement of statements) {
         const action_fit = _is_action_fit(method, statement);
-        const principal_fit = _is_principal_fit(account, statement);
+        const principal_fit = _is_principal_fit(account, statement, account_identifier_type);
         const resource_fit = _is_resource_fit(arn_path, statement);
         const condition_fit = await _is_condition_fit(statement, req, method);
 

src/endpoint/s3/s3_rest.js

@@ -293,8 +293,9 @@ async function authorize_request_policy(req) {
     if (permission_by_id === "DENY") throw new S3Error(S3Error.AccessDenied);
 
     if ((!account_identifier_id || permission_by_id !== "DENY") && account.owner === undefined) {
+        const account_identifier_type = 'name';
         permission_by_name = await s3_bucket_policy_utils.has_bucket_policy_permission(
-            s3_policy, account_identifier_name, method, arn_path, req);
+            s3_policy, account_identifier_name, method, arn_path, req, account_identifier_type);
         dbg.log3('authorize_request_policy: permission_by_name', permission_by_name);
     }
     if (permission_by_name === "DENY") throw new S3Error(S3Error.AccessDenied);

src/sdk/bucketspace_fs.js

@@ -819,12 +819,14 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
         // we (currently) allow account identified to be both id and name,
         // so if by-id failed, try also name
         if (account.owner === undefined) {
+            const account_identifier_type = 'name';
             permission_by_name = await bucket_policy_utils.has_bucket_policy_permission(
                 bucket_policy,
                 account.name.unwrap(),
                 action,
                 `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`,
-                undefined
+                undefined,
+                account_identifier_type
             );
         }
         if (permission_by_name === 'DENY') return false;

src/test/unit_tests/test_s3_bucket_policy.js

@@ -1647,6 +1647,47 @@ mocha.describe('s3_bucket_policy', function() {
         }));
     });
 
+    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}/*`]
+            }
+        ]};
+        await s3_owner.putBucketPolicy({
+            Bucket: BKT,
+            Policy: JSON.stringify(auth_put_policy)
+        });
+
+        await s3_a.putObject({
+            Body: BODY,
+            Bucket: BKT,
+            Key: KEY,
+        });
+
+        await s3_a.getObject({
+            Bucket: BKT,
+            Key: KEY,
+        });
+
+        await assert_throws_async(s3_b.getObject({
+            Bucket: BKT,
+            Key: KEY,
+        }));
+    });
+
     mocha.it('should be able to use notResource', async function() {
         const self = this; // eslint-disable-line no-invalid-this
         self.timeout(15000);