NC | NSFS | Fix Issue DFBUGS-1307 | Bucket Policy With Principal as ID

src/endpoint/s3/s3_rest.js

@@ -277,23 +277,27 @@ async function authorize_request_policy(req) {
         if (is_owner || is_iam_account_and_same_root_account_owner) return;
         throw new S3Error(S3Error.AccessDenied);
     }
-    let permission;
+    // in case we have bucket policy
+    let permission_by_id;
+    let permission_by_name;
     // 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 = await s3_bucket_policy_utils.has_bucket_policy_permission(
+        permission_by_id = await s3_bucket_policy_utils.has_bucket_policy_permission(
             s3_policy, account_identifier_id, method, arn_path, req);
+        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 === "IMPLICIT_DENY") && account.owner === undefined) {
-        permission = await s3_bucket_policy_utils.has_bucket_policy_permission(
+    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);
+        dbg.log3('authorize_request_policy: permission_by_name', permission_by_name);
     }
-
-    if (permission === "DENY") throw new S3Error(S3Error.AccessDenied);
-    if (permission === "ALLOW" || is_owner) return;
+    if (permission_by_name === "DENY") throw new S3Error(S3Error.AccessDenied);
+    if ((permission_by_id === "ALLOW" || permission_by_name === "ALLOW") || is_owner) return;
 
     throw new S3Error(S3Error.AccessDenied);
 }

src/sdk/bucketspace_fs.js

@@ -805,28 +805,28 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
             throw new Error('has_bucket_action_permission: action is required');
         }
 
-        let permission;
-        permission = await bucket_policy_utils.has_bucket_policy_permission(
+        let permission_by_name;
+        const permission_by_id = await bucket_policy_utils.has_bucket_policy_permission(
             bucket_policy,
             account._id,
             action,
             `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`,
             undefined
         );
+        if (permission_by_id === "DENY") return false;
         // we (currently) allow account identified to be both id and name,
         // so if by-id failed, try also name
-        if (account.owner === undefined && permission === 'IMPLICIT_DENY') {
-            permission = await bucket_policy_utils.has_bucket_policy_permission(
+        if (account.owner === undefined) {
+            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
             );
         }
-
-        if (permission === 'DENY') return false;
-        return is_owner || permission === 'ALLOW';
+        if (permission_by_name === 'DENY') return false;
+        return is_owner || (permission_by_id === 'ALLOW' || permission_by_name === 'ALLOW');
     }
 
     async validate_fs_bucket_access(bucket, object_sdk) {

src/test/unit_tests/test_s3_bucket_policy.js

@@ -10,6 +10,7 @@ const { rpc_client, EMAIL, POOL_LIST, anon_rpc_client } = coretest;
 const MDStore = require('../../server/object_services/md_store').MDStore;
 coretest.setup({ pools_to_create: process.env.NC_CORETEST ? undefined : [POOL_LIST[1]] });
 const path = require('path');
+const _ = require('lodash');
 const fs_utils = require('../../util/fs_utils');
 
 const { S3 } = require('@aws-sdk/client-s3');
@@ -33,6 +34,7 @@ async function assert_throws_async(promise, expected_message = 'Access Denied')
 const BKT = 'test2-bucket-policy-ops';
 const BKT_B = 'test2-bucket-policy-ops-1';
 const BKT_C = 'test2-bucket-policy-ops-2';
+const BKT_D = 'test2-bucket-policy-ops-3';
 const KEY = 'file1.txt';
 const user_a = 'alice';
 const user_b = 'bob';
@@ -134,6 +136,7 @@ async function setup() {
     s3_owner = new S3(s3_creds);
     await s3_owner.createBucket({ Bucket: BKT });
     await s3_owner.createBucket({ Bucket: BKT_C });
+    await s3_owner.createBucket({ Bucket: BKT_D });
     s3_anon = new S3({
         ...s3_creds,
         credentials: {
@@ -147,7 +150,7 @@ async function setup() {
     });
 }
 
-/*eslint max-lines-per-function: ["error", 1600]*/
+/*eslint max-lines-per-function: ["error", 2000]*/
 mocha.describe('s3_bucket_policy', function() {
     mocha.before(setup);
     mocha.it('should fail setting bucket policy when user doesn\'t exist', async function() {
@@ -335,6 +338,289 @@ mocha.describe('s3_bucket_policy', function() {
         }));
     });
 
+    mocha.describe('s3_bucket_policy with more complex policies (conflict statements)', function() {
+        mocha.after(async function() {
+            await s3_owner.deleteBucketPolicy({
+                Bucket: BKT_D,
+            });
+        });
+
+        const allow_all_principals_all_s3_actions_statement = {
+            Sid: `Allow all s3 actions on bucket ${BKT_D} to all principals`,
+            Effect: 'Allow',
+            Principal: { AWS: "*" },
+            Action: ['s3:*'],
+            Resource: [`arn:aws:s3:::${BKT_D}`, `arn:aws:s3:::${BKT_D}/*`]
+        };
+
+        const deny_all_principals_get_object_action_statement = {
+            Sid: `Deny all GetObject on bucket ${BKT_D} to all principals`,
+            Effect: 'Deny',
+            Principal: { AWS: "*" },
+            Action: 's3:GetObject',
+            Resource: [`arn:aws:s3:::${BKT_D}/*`]
+        };
+
+        function get_deny_account_by_id_all_s3_actions_statement(_id) {
+            return {
+                Sid: `Do not allow user ${_id} any s3 action`,
+                Effect: 'Deny',
+                Principal: { AWS: [_id] },
+                Action: ['s3:*'],
+                Resource: [`arn:aws:s3:::${BKT_D}/*`]
+            };
+        }
+
+        const deny_account_by_name_all_s3_actions_statement = {
+            Sid: `Do not allow user ${user_a} any s3 action`,
+            Effect: 'Deny',
+            Principal: { AWS: [user_a] },
+            Action: ['s3:*'],
+            Resource: [`arn:aws:s3:::${BKT_D}/*`]
+        };
+
+        mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
+            '(1) DENY principal by account ID (2) ALLOW all principals as *', async function() {
+                // in NC we allow principal to be also IDs
+                if (!is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this
+                const deny_account_by_id_all_s3_actions_statement =
+                    get_deny_account_by_id_all_s3_actions_statement(user_a_account_details._id);
+                const policy = {
+                    Statement: [
+                        allow_all_principals_all_s3_actions_statement,
+                        deny_account_by_id_all_s3_actions_statement
+                    ]
+                };
+                await s3_owner.putBucketPolicy({
+                    Bucket: BKT_D,
+                    Policy: JSON.stringify(policy)
+                });
+                // prepare - put the object to get
+                const key2 = 'file2.txt';
+                const res_put_object = await s3_owner.putObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key2
+                });
+                assert.equal(res_put_object.$metadata.httpStatusCode, 200);
+                // should fail - user a has a DENY statement
+                await assert_throws_async(s3_a.getObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key2
+                }));
+                // should fail - user b does not have a DENY statement (uses the general ALLOW statement)
+                const res_get_object = await s3_b.getObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key2
+                });
+                assert.equal(res_get_object.$metadata.httpStatusCode, 200);
+            });
+
+        mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
+            '(1) DENY principal by account name (2) ALLOW all principals as *', async function() {
+                const policy = {
+                    Statement: [
+                        allow_all_principals_all_s3_actions_statement,
+                        deny_account_by_name_all_s3_actions_statement
+                    ]
+                };
+                await s3_owner.putBucketPolicy({
+                    Bucket: BKT_D,
+                    Policy: JSON.stringify(policy)
+                });
+                // prepare - put the object to get
+                const key3 = 'file3.txt';
+                const res_put_object = await s3_owner.putObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key3
+                });
+                assert.equal(res_put_object.$metadata.httpStatusCode, 200);
+                // should fail - user a has a DENY statement
+                await assert_throws_async(s3_a.getObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key3
+                }));
+                // should fail - user b does not have a DENY statement (uses the general ALLOW statement)
+                const res_get_object = await s3_b.getObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key3
+                });
+                assert.equal(res_get_object.$metadata.httpStatusCode, 200);
+            });
+
+        mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
+            '(1) DENY principal by account ID (2) ALLOW by account name', async function() {
+                // in NC we allow principal to be also IDs
+                if (!is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this
+                const deny_account_by_id_all_s3_actions_statement =
+                    get_deny_account_by_id_all_s3_actions_statement(user_a_account_details._id);
+                const allow_account_by_name_all_s3_actions_statement = _.cloneDeep(deny_account_by_name_all_s3_actions_statement);
+                allow_account_by_name_all_s3_actions_statement.Effect = 'Allow';
+                allow_account_by_name_all_s3_actions_statement.Sid = `Allow user ${user_a} any s3 action`;
+                const policy = {
+                    Statement: [
+                        deny_account_by_id_all_s3_actions_statement,
+                        allow_account_by_name_all_s3_actions_statement
+                    ]
+                };
+                await s3_owner.putBucketPolicy({
+                    Bucket: BKT_D,
+                    Policy: JSON.stringify(policy)
+                });
+                // prepare - put the object to get
+                const key4 = 'file4.txt';
+                const res_put_object = await s3_owner.putObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key4
+                });
+                assert.equal(res_put_object.$metadata.httpStatusCode, 200);
+                // should fail - user a has a DENY statement
+                await assert_throws_async(s3_a.getObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key4
+                }));
+            });
+
+        mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
+            '(1) DENY principal by account name (2) ALLOW by account ID', async function() {
+                // in NC we allow principal to be also IDs
+                if (!is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this
+                const deny_account_by_id_all_s3_actions_statement =
+                    get_deny_account_by_id_all_s3_actions_statement(user_a_account_details._id);
+                const allow_account_by_id_all_s3_actions_statement = _.cloneDeep(deny_account_by_id_all_s3_actions_statement);
+                allow_account_by_id_all_s3_actions_statement.Effect = 'Allow';
+                allow_account_by_id_all_s3_actions_statement.Sid = `Allow user ${user_a_account_details._id} any s3 action`;
+                const policy = {
+                    Statement: [
+                        deny_account_by_name_all_s3_actions_statement,
+                        allow_account_by_id_all_s3_actions_statement
+                    ]
+                };
+                await s3_owner.putBucketPolicy({
+                    Bucket: BKT_D,
+                    Policy: JSON.stringify(policy)
+                });
+                // prepare - put the object to get
+                const key5 = 'file5.txt';
+                const res_put_object = await s3_owner.putObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key5
+                });
+                assert.equal(res_put_object.$metadata.httpStatusCode, 200);
+                // should fail - user a has a DENY statement
+                await assert_throws_async(s3_a.getObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key5
+                }));
+            });
+
+        mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
+            '(1) ALLOW principal by account name (2) DENY all principals as * (specific action only)', async function() {
+                const allow_account_by_name_all_s3_actions_statement = _.cloneDeep(deny_account_by_name_all_s3_actions_statement);
+                allow_account_by_name_all_s3_actions_statement.Effect = 'Allow';
+                allow_account_by_name_all_s3_actions_statement.Sid = `Allow user ${user_a} any s3 action`;
+                const policy = {
+                    Statement: [
+                        allow_account_by_name_all_s3_actions_statement,
+                        deny_all_principals_get_object_action_statement
+                    ]
+                };
+                await s3_owner.putBucketPolicy({
+                    Bucket: BKT_D,
+                    Policy: JSON.stringify(policy)
+                });
+                // prepare - put the object to get
+                const key6 = 'file6.txt';
+                const res_put_object = await s3_owner.putObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key6
+                });
+                assert.equal(res_put_object.$metadata.httpStatusCode, 200);
+                // should fail - user a has a DENY statement
+                await assert_throws_async(s3_a.getObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key6
+                }));
+            });
+
+        mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
+            '(1) ALLOW principal by account ID (2) DENY all principals as * (specific action only)', async function() {
+                // in NC we allow principal to be also IDs
+                if (!is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this
+                const deny_account_by_id_all_s3_actions_statement =
+                    get_deny_account_by_id_all_s3_actions_statement(user_a_account_details._id);
+                const allow_account_by_id_all_s3_actions_statement = _.cloneDeep(deny_account_by_id_all_s3_actions_statement);
+                allow_account_by_id_all_s3_actions_statement.Effect = 'Allow';
+                allow_account_by_id_all_s3_actions_statement.Sid = `Allow user ${user_a_account_details._id} any s3 action`;
+                const policy = {
+                    Statement: [
+                        allow_account_by_id_all_s3_actions_statement,
+                        deny_all_principals_get_object_action_statement
+                    ]
+                };
+                await s3_owner.putBucketPolicy({
+                    Bucket: BKT_D,
+                    Policy: JSON.stringify(policy)
+                });
+                // prepare - put the object to get
+                const key7 = 'file7.txt';
+                const res_put_object = await s3_owner.putObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key7
+                });
+                assert.equal(res_put_object.$metadata.httpStatusCode, 200);
+                // should fail - user a has a DENY statement
+                await assert_throws_async(s3_a.getObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key7
+                }));
+            });
+
+        mocha.it('should not allow principal get object bucket policy with 2 statements: ' +
+            '(1) ALLOW principal by account name (2) DENY all principals as * (specific action only)', async function() {
+                const allow_account_by_name_all_s3_actions_statement = _.cloneDeep(deny_account_by_name_all_s3_actions_statement);
+                allow_account_by_name_all_s3_actions_statement.Effect = 'Allow';
+                allow_account_by_name_all_s3_actions_statement.Sid = `Allow user ${user_a} any s3 action`;
+                const policy = {
+                    Statement: [
+                        allow_account_by_name_all_s3_actions_statement,
+                        deny_all_principals_get_object_action_statement
+                    ]
+                };
+                await s3_owner.putBucketPolicy({
+                    Bucket: BKT_D,
+                    Policy: JSON.stringify(policy)
+                });
+                // prepare - put the object to get
+                const key6 = 'file6.txt';
+                const res_put_object = await s3_owner.putObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key6
+                });
+                assert.equal(res_put_object.$metadata.httpStatusCode, 200);
+                // should fail - user a has a DENY statement
+                await assert_throws_async(s3_a.getObject({
+                    Body: BODY,
+                    Bucket: BKT_D,
+                    Key: key6
+                }));
+            });
+    });
+
     mocha.it('should be able to set bucket policy when none set', async function() {
         const self = this; // eslint-disable-line no-invalid-this
         self.timeout(15000);