Merge pull request #9272 from aayushchouhan09/list-fix

Fix - list_buckets allowing unauthorized bucket access

Author: aayushchouhan09

src/server/common_services/auth_server.js

@@ -496,6 +496,10 @@ function _prepare_auth_request(req) {
     req.has_bucket_action_permission = async function(bucket, action, bucket_path, req_query) {
         return has_bucket_action_permission(bucket, req.account, action, req_query, bucket_path);
     };
+
+    req.has_bucket_ownership_permission = function(bucket) {
+        return has_bucket_ownership_permission(bucket, req.account, req.auth && req.auth.role);
+    };
 }
 
 function _get_auth_info(account, system, authorized_by, role, extra) {
@@ -520,6 +524,73 @@ function _get_auth_info(account, system, authorized_by, role, extra) {
     return response;
 }
 
+/**
+ * is_system_owner checks if the account is the system owner
+ * @param {Record<string, any>} bucket
+ * @param {Record<string, any>} account
+ * @returns {boolean}
+ */
+function is_system_owner(bucket, account) {
+    if (!bucket?.system?.owner?.email || !account?.email) return false;
+    return bucket.system.owner.email.unwrap() === account.email.unwrap();
+}
+
+/**
+ * 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}
+ */
+function is_bucket_owner(bucket, account) {
+    if (!bucket?.owner_account?.email || !account?.email) return false;
+    return bucket.owner_account.email.unwrap() === account.email.unwrap();
+}
+
+/**
+ * is_bucket_claim_owner checks if the account is the OBC (ObjectBucketClaim) owner of the bucket
+ * @param {Record<string, any>} bucket
+ * @param {Record<string, any>} account
+ * @returns {boolean}
+ */
+function is_bucket_claim_owner(bucket, account) {
+    if (!account?.bucket_claim_owner || !bucket?.name) return false;
+    return account.bucket_claim_owner.name.unwrap() === bucket.name.unwrap();
+}
+
+/**
+ * has_bucket_ownership_permission returns true if the account can list the bucket in ListBuckets operation
+ *
+ * aws-compliant behavior:
+ * - System owner can list all the buckets
+ * - Operator account (noobaa cli) can list all the buckets
+ * - Root accounts can list buckets they own
+ * - OBC owner can list their buckets
+ * - IAM users can list their owner buckets
+ *
+ * @param {Record<string, any>} bucket
+ * @param {Record<string, any>} account
+ * @param {string} role
+ * @returns {Promise<boolean>}
+ */
+async function has_bucket_ownership_permission(bucket, account, role) {
+    // system owner can list all the buckets
+    if (is_system_owner(bucket, account)) return true;
+
+    // operator account (noobaa cli) can list all the buckets
+    if (role === 'operator') return true;
+
+    // check direct ownership
+    if (is_bucket_owner(bucket, account)) return true;
+
+    // special case: check bucket claim ownership (OBC)
+    if (is_bucket_claim_owner(bucket, account)) return true;
+
+    // special case: iam user can list the buckets of their owner
+    // TODO: handle iam user
+
+    return false;
+}
+
 /**
  * has_bucket_action_permission returns true if the requesting account has permission to perform
  * the given action on the given bucket.
@@ -538,19 +609,21 @@ function _get_auth_info(account, system, authorized_by, role, extra) {
  */
 async function has_bucket_action_permission(bucket, account, action, req_query, bucket_path = "") {
     dbg.log1('has_bucket_action_permission:', bucket.name, account.email, bucket.owner_account.email);
-    // If the system owner account wants to access the bucket, allow it
-    if (bucket.system.owner.email.unwrap() === account.email.unwrap()) return true;
 
-    const is_owner = (bucket.owner_account.email.unwrap() === account.email.unwrap()) ||
-        (account.bucket_claim_owner && account.bucket_claim_owner.name.unwrap() === bucket.name.unwrap());
+    // system owner can access all buckets
+    if (is_system_owner(bucket, account)) return true;
+
+    // check ownership: direct owner or OBC
+    const has_owner_access = is_bucket_owner(bucket, account) || is_bucket_claim_owner(bucket, account);
+
     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
+        // we allow IAM account to access a bucket that is owned by their root account
         const is_iam_and_same_root_account_owner = account.owner !== undefined &&
             account.owner._id.toString() === bucket.owner_account._id.toString();
-        return is_owner || is_iam_and_same_root_account_owner;
+        return has_owner_access || is_iam_and_same_root_account_owner;
     }
     if (!action) {
         throw new Error('has_bucket_action_permission: action is required');
@@ -566,7 +639,8 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
     );
 
     if (result === 'DENY') return false;
-    return is_owner || result === 'ALLOW';
+
+    return has_owner_access || result === 'ALLOW';
 }
 
 /**

src/server/system_services/bucket_server.js

@@ -1096,9 +1096,13 @@ async function list_buckets(req) {
     let continuation_token = req.rpc_params?.continuation_token;
     const max_buckets = req.rpc_params?.max_buckets;
 
-    const accessible_bucket_list = system_store.data.buckets.filter(
-        async bucket => await req.has_s3_bucket_permission(bucket, "s3:ListBucket", req) && !bucket.deleting
-    );
+    // filter buckets based on ownership
+    const bucket_permissions = await P.map(system_store.data.buckets, async bucket => {
+        if (bucket.deleting) return null;
+        const has_permission = await req.has_bucket_ownership_permission(bucket);
+        return has_permission ? bucket : null;
+    });
+    const accessible_bucket_list = bucket_permissions.filter(bucket => bucket !== null);
 
     accessible_bucket_list.sort((a, b) => a.name.unwrap().localeCompare(b.name.unwrap()));
 

src/test/integration_tests/api/s3/test_s3_list_buckets.js

@@ -127,5 +127,53 @@ mocha.describe('s3_ops', function() {
         });
     });
 
+    mocha.describe('list_buckets permissions', function() {
+        this.timeout(60000);
+        let s3_account_a;
+        let s3_account_b;
+
+        async function create_account_and_client(name) {
+            const account = await rpc_client.account.create_account({
+                name, email: name, has_login: false, s3_access: true,
+                default_resource: coretest.POOL_LIST[0].name
+            });
+            return new S3Client({
+                ...s3_client_params,
+                credentials: {
+                    accessKeyId: account.access_keys[0].access_key.unwrap(),
+                    secretAccessKey: account.access_keys[0].secret_key.unwrap(),
+                }
+            });
+        }
+
+        mocha.before(async function() {
+            s3_account_a = await create_account_and_client('account-a');
+            s3_account_b = await create_account_and_client('account-b');
+            await s3_account_a.send(new CreateBucketCommand({ Bucket: 'bucket-a' }));
+            await s3_account_b.send(new CreateBucketCommand({ Bucket: 'bucket-b' }));
+            await s3.send(new CreateBucketCommand({ Bucket: 'admin-buck' }));
+        });
+
+        mocha.after(async function() {
+            await s3_account_a.send(new DeleteBucketCommand({ Bucket: 'bucket-a' }));
+            await s3_account_b.send(new DeleteBucketCommand({ Bucket: 'bucket-b' }));
+            await s3.send(new DeleteBucketCommand({ Bucket: 'admin-buck' }));
+            await rpc_client.account.delete_account({ email: 'account-a' });
+            await rpc_client.account.delete_account({ email: 'account-b' });
+        });
+
+        mocha.it('accounts should list only owned buckets', async function() {
+            const buckets_a = (await s3_account_a.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
+            const buckets_b = (await s3_account_b.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
+            assert.deepStrictEqual(buckets_a, ['bucket-a']);
+            assert.deepStrictEqual(buckets_b, ['bucket-b']);
+        });
+
+        mocha.it('admin should lists all the buckets', async function() {
+            const buckets = (await s3.send(new ListBucketsCommand())).Buckets.map(b => b.Name);
+            assert(buckets.includes('bucket-a') && buckets.includes('bucket-b') && buckets.includes('admin-buck'));
+        });
+    });
+
 });
 

src/test/integration_tests/api/sts/test_sts.js

@@ -457,6 +457,7 @@ function verify_session_token(session_token, access_key, secret_key, assumed_rol
 mocha.describe('Session token tests', function() {
     const { rpc_client } = coretest;
     const alice2 = 'alice2';
+    const alice2_buck = 'alice2-test-bucket';
     const bob2 = 'bob2';
     const charlie2 = 'charlie2';
     const accounts = [{ email: alice2 }, { email: bob2 }, { email: charlie2 }];
@@ -466,6 +467,8 @@ mocha.describe('Session token tests', function() {
     mocha.after(async function() {
         const self = this; // eslint-disable-line no-invalid-this
         self.timeout(60000);
+
+        await accounts[0].s3.deleteBucket({ Bucket: alice2_buck }).promise();
         for (const account of accounts) {
             await rpc_client.account.delete_account({ email: account.email });
         }
@@ -542,6 +545,10 @@ mocha.describe('Session token tests', function() {
             name: 'first.bucket',
             policy: s3accesspolicy,
         });
+
+        // create a bucket owned by alice2 for ListBuckets to work
+        // Note: bucket policy is not related to ListBuckets operation
+        await accounts[0].s3.createBucket({ Bucket: alice2_buck }).promise();
     });
 
     mocha.it('user b assume role of user a - default expiry - list s3 - should be allowed', async function() {
@@ -564,7 +571,7 @@ mocha.describe('Session token tests', function() {
         });
 
         const buckets1 = await temp_s3_with_session_token.listBuckets().promise();
-        assert.ok(buckets1.Buckets.length > 0);
+        assert.ok(buckets1.Buckets[0].Name === alice2_buck);
     });
 
     mocha.it('user b assume role of user a - valid expiry via durationSeconds - list s3 - should be allowed', async function() {
@@ -589,7 +596,7 @@ mocha.describe('Session token tests', function() {
         });
 
         const buckets1 = await temp_s3_with_session_token.listBuckets().promise();
-        assert.ok(buckets1.Buckets.length > 0);
+        assert.ok(buckets1.Buckets[0].Name === alice2_buck);
     });
 
     mocha.it('user b assume role of user a - invalid expiry via durationSeconds - should be rejected', async function() {