diff --git a/src/endpoint/s3/s3_rest.js b/src/endpoint/s3/s3_rest.js index e7b2691542..a4455486bf 100755 --- a/src/endpoint/s3/s3_rest.js +++ b/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); } diff --git a/src/sdk/bucketspace_fs.js b/src/sdk/bucketspace_fs.js index b41cf681e7..9d9047a746 100644 --- a/src/sdk/bucketspace_fs.js +++ b/src/sdk/bucketspace_fs.js @@ -805,18 +805,19 @@ 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, @@ -824,9 +825,8 @@ class BucketSpaceFS extends BucketSpaceSimpleFS { 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) { diff --git a/src/test/unit_tests/test_s3_bucket_policy.js b/src/test/unit_tests/test_s3_bucket_policy.js index 65123d97f4..151b4f29fd 100644 --- a/src/test/unit_tests/test_s3_bucket_policy.js +++ b/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);