Fix (NC | NSFS) - list_buckets allowing unauthorized bucket access

Signed-off-by: Aayush Chouhan <[email protected]>

Author: aayushchouhan09

src/sdk/bucketspace_fs.js

@@ -241,7 +241,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
      * permissions
      * a. First iteration - map_with_concurrency with concurrency rate of 10 entries at the same time.
      * a.1. if entry is dir file/entry is non json - we will return undefined
-     * a.2. if bucket is unaccessible by bucket policy - we will return undefined
+     * a.2. if bucket is not owned by the account (or their root account for IAM users) - we will return undefined
      * a.3. if underlying storage of the bucket is unaccessible by the account's uid/gid - we will return undefined
      * a.4. else - return the bucket config info.
      * b. Second iteration - filter empty entries - filter will remove undefined values produced by the map_with_concurrency().
@@ -271,9 +271,9 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
                 if (err.rpc_code !== 'NO_SUCH_BUCKET') throw err;
             }
             if (!bucket) return;
-            const bucket_policy_accessible = await this.has_bucket_action_permission(bucket, account, 's3:ListBucket');
-            dbg.log2(`list_buckets: bucket_name ${bucket_name} bucket_policy_accessible`, bucket_policy_accessible);
-            if (!bucket_policy_accessible) return;
+
+            if (!await this.has_bucket_ownership_permission(bucket, account)) return;
+
             const fs_accessible = await this.validate_fs_bucket_access(bucket, object_sdk);
             if (!fs_accessible) return;
             return bucket;
@@ -921,24 +921,66 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
     ///// UTILS /////
     /////////////////
 
-    // TODO: move the following 3 functions - has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir()
+    // TODO: move the following 6 functions - is_bucket_owner(), is_iam_and_same_root_account_owner(), has_bucket_ownership_permission(), has_bucket_action_permission(), validate_fs_bucket_access(), _has_access_to_nsfs_dir()
     // so they can be re-used
+
+    /**
+     * is_bucket_owner checks if the account is the direct owner of the bucket
+     * @param {Record<string, any>} bucket
+     * @param {Record<string, any>} account
+     * @returns {boolean}
+     */
+    is_bucket_owner(bucket, account) {
+        if (!bucket?.owner_account?.id || !account?._id) return false;
+        return bucket.owner_account.id === account._id;
+    }
+
+    /**
+     * is_iam_and_same_root_account_owner checks if the account is an IAM user and the bucket is owned by their root account
+     * @param {Record<string, any>} account
+     * @param {Record<string, any>} bucket
+     * @returns {boolean}
+     */
+    is_iam_and_same_root_account_owner(account, bucket) {
+        if (!account?.owner || !bucket?.owner_account?.id) return false;
+        return account.owner === bucket.owner_account.id;
+    }
+
+    /**
+     * has_bucket_ownership_permission returns true if the account can list the bucket in ListBuckets operation
+     *
+     * aws-compliant behavior:
+     * - Root accounts can list buckets they own
+     * - IAM users can list their owner buckets
+     *
+     * @param {Record<string, any>} bucket
+     * @param {Record<string, any>} account
+     * @returns {Promise<boolean>}
+     */
+    async has_bucket_ownership_permission(bucket, account) {
+        // check direct ownership
+        if (this.is_bucket_owner(bucket, account)) return true;
+
+        // special case: iam user can list the buckets of their owner
+        if (this.is_iam_and_same_root_account_owner(account, bucket)) return true;
+
+        return false;
+    }
+
     async has_bucket_action_permission(bucket, account, action, bucket_path = "") {
         dbg.log1('has_bucket_action_permission:', bucket.name.unwrap(), account.name.unwrap(), account._id, bucket.bucket_owner.unwrap());
 
-        const is_system_owner = Boolean(bucket.system_owner) && bucket.system_owner.unwrap() === account.name.unwrap();
+        // system_owner is deprecated since version 5.18, so we don't need to check it
+
+        // check ownership: direct owner or IAM user inheritance
+        const is_owner = this.is_bucket_owner(bucket, account);
 
-        // If the system owner account wants to access the bucket, allow it
-        if (is_system_owner) return true;
-        const is_owner = (bucket.owner_account && bucket.owner_account.id === account._id);
         const bucket_policy = bucket.s3_policy;
 
         if (!bucket_policy) {
             // in case we do not have bucket policy
             // we allow IAM account to access a bucket that that is owned by their root account
-            const is_iam_and_same_root_account_owner = account.owner !== undefined &&
-                account.owner === bucket.owner_account.id;
-            return is_owner || is_iam_and_same_root_account_owner;
+            return is_owner || this.is_iam_and_same_root_account_owner(account, bucket);
         }
         if (!action) {
             throw new Error('has_bucket_action_permission: action is required');

src/test/unit_tests/nsfs/test_bucketspace_fs.js

@@ -519,28 +519,6 @@ mocha.describe('bucketspace_fs', function() {
             const res = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_iam_account);
             assert.equal(res.buckets.length, 0);
         });
-        mocha.it('list buckets - different account with bucket policy (principal by name)', async function() {
-            // another user created a bucket
-            // with bucket policy account_user3 can list it
-            const policy = generate_s3_policy(account_user4.name, test_bucket, ['s3:*']).policy;
-            const param = { name: test_bucket, policy: policy };
-            await bucketspace_fs.put_bucket_policy(param);
-            const dummy_object_sdk_for_iam_account = make_dummy_object_sdk_for_account(dummy_object_sdk, account_user4);
-            const res = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_iam_account);
-            assert.equal(res.buckets.length, 1);
-            assert.equal(res.buckets[0].name.unwrap(), test_bucket);
-        });
-        mocha.it('list buckets - different account with bucket policy (principal by id)', async function() {
-            // another user created a bucket
-            // with bucket policy account_user3 can list it
-            const policy = generate_s3_policy(account_user4._id, test_bucket, ['s3:*']).policy;
-            const param = { name: test_bucket, policy: policy };
-            await bucketspace_fs.put_bucket_policy(param);
-            const dummy_object_sdk_for_iam_account = make_dummy_object_sdk_for_account(dummy_object_sdk, account_user4);
-            const res = await bucketspace_fs.list_buckets({}, dummy_object_sdk_for_iam_account);
-            assert.equal(res.buckets.length, 1);
-            assert.equal(res.buckets[0].name.unwrap(), test_bucket);
-        });
         mocha.afterEach(async function() {
             await fs_utils.folder_delete(`${new_buckets_path}/${test_bucket}`);
             const file_path = get_config_file_path(CONFIG_SUBDIRS.BUCKETS, test_bucket);
@@ -928,19 +906,54 @@ mocha.describe('bucketspace_fs', function() {
     });
 
     mocha.describe('bucket tagging operations', function() {
+        const test_bucket_tagging = 'test-bucket-tagging';
+        mocha.before(async function() {
+            await create_bucket(test_bucket_tagging);
+        });
+
         mocha.it('put_bucket_tagging', async function() {
-            const param = { name: test_bucket, tagging: [{ key: 'k1', value: 'v1' }] };
+            const param = { name: test_bucket_tagging, tagging: [{ key: 'k1', value: 'v1' }] };
             await bucketspace_fs.put_bucket_tagging(param, dummy_object_sdk);
             const tag = await bucketspace_fs.get_bucket_tagging(param);
             assert.deepEqual(tag, { tagging: param.tagging });
         });
 
         mocha.it('delete_bucket_tagging', async function() {
-            const param = { name: test_bucket };
+            const param = { name: test_bucket_tagging };
             await bucketspace_fs.delete_bucket_tagging(param);
             const tag = await bucketspace_fs.get_bucket_tagging(param);
             assert.deepEqual(tag, { tagging: [] });
         });
+
+        mocha.it('put_bucket_tagging - different account with bucket policy (principal by name)', async function() {
+            const policy = generate_s3_policy(account_user4.name, test_bucket_tagging, ['s3:PutBucketTagging']).policy;
+            const param = { name: test_bucket_tagging, policy: policy };
+            await bucketspace_fs.put_bucket_policy(param);
+            const dummy_object_sdk_for_account = make_dummy_object_sdk_for_account(dummy_object_sdk, account_user4);
+            const tagging_param = { name: test_bucket_tagging, tagging: [{ key: 'test-key', value: 'test-value' }] };
+            await bucketspace_fs.put_bucket_tagging(tagging_param, dummy_object_sdk_for_account);
+            const tagging = await bucketspace_fs.get_bucket_tagging({ name: test_bucket_tagging });
+            assert.equal(tagging.tagging.length, 1);
+            assert.equal(tagging.tagging[0].key, 'test-key');
+        });
+
+        mocha.it('put_bucket_tagging - different account with bucket policy (principal by id)', async function() {
+            const policy = generate_s3_policy(account_user4._id, test_bucket_tagging, ['s3:PutBucketTagging']).policy;
+            const param = { name: test_bucket_tagging, policy: policy };
+            await bucketspace_fs.put_bucket_policy(param);
+            const dummy_object_sdk_for_account = make_dummy_object_sdk_for_account(dummy_object_sdk, account_user4);
+            const tagging_param = { name: test_bucket_tagging, tagging: [{ key: 'test-key', value: 'test-value' }] };
+            await bucketspace_fs.put_bucket_tagging(tagging_param, dummy_object_sdk_for_account);
+            const tagging = await bucketspace_fs.get_bucket_tagging({ name: test_bucket_tagging });
+            assert.equal(tagging.tagging.length, 1);
+            assert.equal(tagging.tagging[0].key, 'test-key');
+        });
+
+        mocha.after(async function() {
+            await fs_utils.folder_delete(`${new_buckets_path}/${test_bucket_tagging}`);
+            const file_path = get_config_file_path(CONFIG_SUBDIRS.BUCKETS, test_bucket_tagging);
+            await fs_utils.file_delete(file_path);
+        });
     });
 
     mocha.describe('bucket lifecycle operations', function() {