[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

@@ -137,18 +137,31 @@ async function _is_object_tag_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_name = undefined) {
     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_name
+    );
     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_name
+    );
     if (res_arr_allow.every(item => item)) return 'ALLOW';
 
     // implicit deny
@@ -168,14 +181,13 @@ 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_name = undefined) {
     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])) {
         dbg.log1('bucket_policy: ', statement.Principal ? 'Principal' : 'NotPrincipal', ' fit?', principal, account);
-        if ((principal.unwrap() === '*') || (principal.unwrap() === account)) {
+        if ((principal.unwrap() === '*') || (principal.unwrap() === account) || (account_identifier_name && (principal.unwrap() === account_identifier_name))) {
             principal_fit = true;
             break;
         }
@@ -198,15 +210,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_name = undefined) {
     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_name)));
 }
 
-async function _is_statements_fit(statements, account, method, arn_path, req) {
+async function _is_statements_fit(statements, account, method, arn_path, req, account_identifier_name = undefined) {
     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_name);
         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

@@ -287,14 +287,14 @@ async function authorize_request_policy(req) {
     // 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);
+            s3_policy, account_identifier_id, method, arn_path, req, account_identifier_name);
         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);
+            s3_policy, {account_identifier_name, account_identifier_id}, method, arn_path, req, account_identifier_name);
         dbg.log3('authorize_request_policy: permission_by_name', permission_by_name);
     }
     if (permission_by_name === "DENY") throw new S3Error(S3Error.AccessDenied);