diff --git a/src/endpoint/s3/s3_bucket_policy_utils.js b/src/endpoint/s3/s3_bucket_policy_utils.js index 1132a0e33e..d4290f2915 100644 --- a/src/endpoint/s3/s3_bucket_policy_utils.js +++ b/src/endpoint/s3/s3_bucket_policy_utils.js @@ -150,16 +150,26 @@ async function _is_object_version_fit(req, predicate, value) { return res; } +/** + * has_bucket_policy_permission validate the bucket policy principal + * + * @param {object} policy + * @param {string[] | string} account + * @param {string[] | string} method + * @param {string} arn_path + * @param {object} req + */ async function has_bucket_policy_permission(policy, account, method, arn_path, req, { disallow_public_access = false, should_pass_principal = true } = {}) { 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]; + const account_arr = Array.isArray(account) ? account : [account]; // look for explicit denies const res_arr_deny = await is_statement_fit_of_method_array( - deny_statements, account, method_arr, arn_path, req, { + deny_statements, account_arr, method_arr, arn_path, req, { disallow_public_access: false, // No need to disallow in "DENY" should_pass_principal } @@ -168,7 +178,7 @@ async function has_bucket_policy_permission(policy, account, method, arn_path, r // look for explicit allows const res_arr_allow = await is_statement_fit_of_method_array( - allow_statements, account, method_arr, arn_path, req, { + allow_statements, account_arr, method_arr, arn_path, req, { disallow_public_access, should_pass_principal }); @@ -191,14 +201,14 @@ function _is_action_fit(method, statement) { return statement.Action ? action_fit : !action_fit; } -function _is_principal_fit(account, statement, ignore_public_principal = false) { +function _is_principal_fit(account_arr, statement, ignore_public_principal = false) { 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)) { + dbg.log1('bucket_policy: ', statement.Principal ? 'Principal' : 'NotPrincipal', ' fit?', principal, account_arr); + if ((principal.unwrap() === '*') || account_arr.includes(principal.unwrap())) { if (ignore_public_principal && principal.unwrap() === '*' && statement.Principal) { // Ignore the "fit" if ignore_public_principal is requested continue; @@ -226,19 +236,19 @@ 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_arr, method_arr, arn_path, req, { disallow_public_access = false, should_pass_principal = true } = {}) { return Promise.all(method_arr.map(method_permission => - _is_statements_fit(statements, account, method_permission, arn_path, req, { disallow_public_access, should_pass_principal }))); + _is_statements_fit(statements, account_arr, method_permission, arn_path, req, { disallow_public_access, should_pass_principal }))); } -async function _is_statements_fit(statements, account, method, arn_path, req, +async function _is_statements_fit(statements, account_arr, method, arn_path, req, { disallow_public_access = false, should_pass_principal = true} = {}) { for (const statement of statements) { const action_fit = _is_action_fit(method, statement); // When evaluating IAM user inline policies, should_pass_principal is false since these policies // don't have a Principal field (the principal is implicitly the user) - const principal_fit = should_pass_principal ? _is_principal_fit(account, statement, disallow_public_access) : true; + const principal_fit = should_pass_principal ? _is_principal_fit(account_arr, statement, disallow_public_access) : true; const resource_fit = _is_resource_fit(arn_path, statement); const condition_fit = await _is_condition_fit(statement, req, method); diff --git a/src/endpoint/s3/s3_rest.js b/src/endpoint/s3/s3_rest.js index 9781bffb9e..1785e7b0bb 100755 --- a/src/endpoint/s3/s3_rest.js +++ b/src/endpoint/s3/s3_rest.js @@ -14,7 +14,7 @@ const http_utils = require('../../util/http_utils'); const signature_utils = require('../../util/signature_utils'); const config = require('../../../config'); const s3_utils = require('./s3_utils'); -const { _create_detailed_message_for_iam_user_access_in_s3 } = require('../iam/iam_utils'); // for IAM policy +const { _create_detailed_message_for_iam_user_access_in_s3, get_owner_account_id } = require('../iam/iam_utils'); // for IAM policy const S3_MAX_BODY_LEN = 4 * 1024 * 1024; @@ -252,7 +252,8 @@ async function authorize_request_policy(req) { const account = req.object_sdk.requesting_account; const is_nc_deployment = Boolean(req.object_sdk.nsfs_config_root); const account_identifier_name = is_nc_deployment ? account.name.unwrap() : account.email.unwrap(); - const account_identifier_id = is_nc_deployment ? account._id : undefined; + // Both NSFS NC and containerized will validate bucket policy against acccount id. + const account_identifier_id = account._id; const account_identifier_arn = s3_bucket_policy_utils.get_bucket_policy_principal_arn(account); // deny delete_bucket permissions from bucket_claim_owner accounts (accounts that were created by OBC from openshift\k8s) @@ -296,7 +297,7 @@ async function authorize_request_policy(req) { let permission_by_id; let permission_by_name; let permission_by_arn; - let permission_by_arn_owner; + let permission_by_owner; // In NC, we allow principal to be: // 1. account name (for backwards compatibility) @@ -310,7 +311,8 @@ async function authorize_request_policy(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_by_id !== "DENY") && account.owner === undefined) { + // Check bucket policy permission against account name only for NSFS NC + if (is_nc_deployment && 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, { disallow_public_access: public_access_block?.restrict_public_buckets } @@ -318,9 +320,9 @@ async function authorize_request_policy(req) { dbg.log3('authorize_request_policy: permission_by_name', permission_by_name); } if (permission_by_name === "DENY") throw new S3Error(S3Error.AccessDenied); - // Containerized deployment always will have account_identifier_id undefined + // Check bucket policy permission against ARN only for containerized deployments. // Policy permission is validated by account arn - if (!account_identifier_id) { + if (!is_nc_deployment && permission_by_id !== "DENY") { permission_by_arn = await s3_bucket_policy_utils.has_bucket_policy_permission( s3_policy, account_identifier_arn, method, arn_path, req, { disallow_public_access: public_access_block?.restrict_public_buckets } @@ -329,19 +331,20 @@ async function authorize_request_policy(req) { } if (permission_by_arn === "DENY") throw new S3Error(S3Error.AccessDenied); - // ARN check for users under the account + // ARN and ID check for users under the account // ARN check is not implemented in NC yet if (!is_nc_deployment && account.owner !== undefined) { - const owner_account_identifier_arn = s3_bucket_policy_utils.create_arn_for_root(account.owner); - permission_by_arn_owner = await s3_bucket_policy_utils.has_bucket_policy_permission( - s3_policy, owner_account_identifier_arn, method, arn_path, req, + const owner_account_id = get_owner_account_id(account); + const owner_account_identifier_arn = s3_bucket_policy_utils.create_arn_for_root(owner_account_id); + permission_by_owner = await s3_bucket_policy_utils.has_bucket_policy_permission( + s3_policy, [owner_account_identifier_arn, owner_account_id], method, arn_path, req, { disallow_public_access: public_access_block?.restrict_public_buckets } ); - dbg.log3('authorize_request_policy permission_by_arn_owner', permission_by_arn_owner); - if (permission_by_arn_owner === "DENY") throw new S3Error(S3Error.AccessDenied); + dbg.log3('authorize_request_policy permission_by_arn_owner', permission_by_owner); + if (permission_by_owner === "DENY") throw new S3Error(S3Error.AccessDenied); } if ((permission_by_id === "ALLOW" || permission_by_name === "ALLOW" || - permission_by_arn === "ALLOW" || permission_by_arn_owner === "ALLOW") || is_owner) return; + permission_by_arn === "ALLOW" || permission_by_owner === "ALLOW") || is_owner) return; throw new S3Error(S3Error.AccessDenied); } diff --git a/src/server/common_services/auth_server.js b/src/server/common_services/auth_server.js index ca2bc1480c..d36bd91115 100644 --- a/src/server/common_services/auth_server.js +++ b/src/server/common_services/auth_server.js @@ -629,10 +629,10 @@ async function has_bucket_action_permission(bucket, account, action, req_query, throw new Error('has_bucket_action_permission: action is required'); } const arn = account.owner ? iam_utils.create_arn_for_user(account.owner._id.toString(), account.name.unwrap().split(':')[0], account.iam_path) : - iam_utils.create_arn_for_root(account._id); + iam_utils.create_arn_for_root(account._id.toString()); const result = await s3_bucket_policy_utils.has_bucket_policy_permission( bucket_policy, - arn, + [arn, account._id.toString()], action, `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`, req_query @@ -641,11 +641,16 @@ async function has_bucket_action_permission(bucket, account, action, req_query, if (result === 'DENY') return false; let permission_by_arn_owner; + // Added to verify the IAM users's owner account have bucket access + // If yes, IAM user also should get access if (account.owner) { - const owner_account_identifier_arn = s3_bucket_policy_utils.create_arn_for_root(account.owner._id.toString()); + const owner_account_id = iam_utils.get_owner_account_id(account); + const owner_account_identifier_arn = s3_bucket_policy_utils.create_arn_for_root(owner_account_id); + // We support both ARN and account/user ID in bucket policy Principal, So if the bucket policy have only ARN + // sharing array of ARN and ID can fix the issue that misses the access just because of bucket policy have ID as principal permission_by_arn_owner = await s3_bucket_policy_utils.has_bucket_policy_permission( bucket_policy, - owner_account_identifier_arn, + [owner_account_identifier_arn, owner_account_id], action, `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`, req_query, diff --git a/src/server/system_services/bucket_server.js b/src/server/system_services/bucket_server.js index d11e779b56..f5c4a6ff53 100644 --- a/src/server/system_services/bucket_server.js +++ b/src/server/system_services/bucket_server.js @@ -523,10 +523,9 @@ async function get_bucket_policy(req) { eg: arn:aws:iam::${account_id}:user/${iam_path}/${user_name} account email = ${iam_user_name}:${account_id} - @param {SensitiveString | String} principal Bucket policy principal + @param {String} principal_as_string Bucket policy principal string */ -async function get_account_by_principal(principal) { - const principal_as_string = principal instanceof SensitiveString ? principal.unwrap() : principal; +async function account_exists_by_principal_arn(principal_as_string) { const root_sufix = 'root'; const user_sufix = 'user'; const arn_parts = principal_as_string.split(':'); @@ -546,6 +545,27 @@ async function get_account_by_principal(principal) { //} } +/** + Validate and return account by principal ARN and account id. + + @param {SensitiveString | String} principal Bucket policy principal +*/ +async function get_account_by_principal(principal) { + const principal_as_string = principal instanceof SensitiveString ? principal.unwrap() : principal; + const is_principal_arn = principal_as_string.startsWith('arn:aws:iam::'); + if (is_principal_arn) { + const principal_by_arn = await account_exists_by_principal_arn(principal_as_string); + dbg.log3('get_account_by_principal: principal_by_arn', principal_by_arn); + if (principal_by_arn) return true; + } else { + const account = system_store.data.accounts.find(acc => acc._id.toString() === principal_as_string); + const principal_by_id = account !== undefined; + dbg.log3('get_account_by_principal: principal_by_id', principal_by_id); + if (principal_by_id) return true; + } + return false; +} + async function put_bucket_policy(req) { dbg.log0('put_bucket_policy:', req.rpc_params); const bucket = find_bucket(req, req.rpc_params.name); diff --git a/src/test/integration_tests/api/iam/test_iam_basic_integration.js b/src/test/integration_tests/api/iam/test_iam_basic_integration.js index be8507d5dc..2d1262e0ab 100644 --- a/src/test/integration_tests/api/iam/test_iam_basic_integration.js +++ b/src/test/integration_tests/api/iam/test_iam_basic_integration.js @@ -73,7 +73,9 @@ mocha.describe('IAM basic integration tests - happy path', async function() { }); mocha.after(async () => { - fs_utils.folder_delete(`${config_root}`); + if (is_nc_coretest) { + fs_utils.folder_delete(`${config_root}`); + } }); mocha.describe('IAM User API', async function() { diff --git a/src/test/integration_tests/api/s3/test_s3_bucket_policy.js b/src/test/integration_tests/api/s3/test_s3_bucket_policy.js index af2a9b331f..f3fa15ce21 100644 --- a/src/test/integration_tests/api/s3/test_s3_bucket_policy.js +++ b/src/test/integration_tests/api/s3/test_s3_bucket_policy.js @@ -65,9 +65,10 @@ const cross_test_store = {}; let user_a_account_details; let user_b_account_details; +let user_a_account_id; +let user_b_account_id; + let admin_info; -let account_info_a; -let account_info_b; let a_principal; let b_principal; @@ -123,14 +124,14 @@ async function setup() { account.name = user_a; account.email = user_a; user_a_account_details = await rpc_client.account.create_account(account); - account_info_a = user_a_account_details._id ? user_a_account_details : await rpc_client.account.read_account({ email: user_a }); - console.log('user_a_account_details', account_info_a); + console.log('user_a_account_details', user_a_account_details); const user_a_keys = user_a_account_details.access_keys; + user_a_account_id = is_nc_coretest ? user_a_account_details._id : user_a_account_details.id; account.name = user_b; account.email = user_b; user_b_account_details = await rpc_client.account.create_account(account); - account_info_b = user_b_account_details._id ? user_b_account_details : await rpc_client.account.read_account({ email: user_b }); - console.log('user_b_account_details', account_info_b); + console.log('user_b_account_details', user_b_account_details); + user_b_account_id = is_nc_coretest ? user_b_account_details._id : user_b_account_details.id; const user_b_keys = user_b_account_details.access_keys; s3_creds.credentials = { accessKeyId: user_a_keys[0].access_key.unwrap(), @@ -149,11 +150,11 @@ async function setup() { }; /* For coretest nc, principal will have account name and -+ for containerized deployment principal is ARN + for containerized deployment principal is ARN */ admin_principal = is_nc_coretest ? EMAIL : s3_bucket_policy_utils.create_arn_for_root(admin_info._id.toString()); - a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(account_info_a._id.toString()); - b_principal = is_nc_coretest ? user_b : s3_bucket_policy_utils.create_arn_for_root(account_info_b._id.toString()); + a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(user_a_account_details.id.toString()); + b_principal = is_nc_coretest ? user_b : s3_bucket_policy_utils.create_arn_for_root(user_b_account_details.id.toString()); s3_owner = new S3(s3_creds); await s3_owner.createBucket({ Bucket: BKT }); @@ -312,12 +313,11 @@ mocha.describe('s3_bucket_policy', function() { }); mocha.it('should put/get bucket policy - principal by account ID', async function() { - if (!is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this const policy = { Statement: [{ - Sid: `Allow all s3 actions on bucket ${BKT} to principal (by ID) ${user_a_account_details._id}`, + Sid: `Allow all s3 actions on bucket ${BKT} to principal (by ID) ${user_a_account_id}`, Effect: 'Allow', - Principal: { AWS: user_a_account_details._id }, + Principal: { AWS: user_a_account_id }, Action: ['s3:*'], Resource: [`arn:aws:s3:::${BKT}`, `arn:aws:s3:::${BKT}/*`] }] @@ -335,12 +335,11 @@ mocha.describe('s3_bucket_policy', function() { }); mocha.it('should put object to permitted account (bucket policy - principal by account ID', async function() { - if (!is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this const policy = { Statement: [{ - Sid: `Allow all s3 actions on bucket ${BKT} to principal (by ID) ${user_a_account_details._id}`, + Sid: `Allow all s3 actions on bucket ${BKT} to principal (by ID) ${user_a_account_id}`, Effect: 'Allow', - Principal: { AWS: user_a_account_details._id }, + Principal: { AWS: user_a_account_id }, Action: ['s3:*'], Resource: [`arn:aws:s3:::${BKT}`, `arn:aws:s3:::${BKT}/*`] }] @@ -395,7 +394,7 @@ mocha.describe('s3_bucket_policy', function() { }; } // Losing this value in-between, assigning it again - a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(account_info_a._id.toString()); + a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(user_a_account_id.toString()); const deny_account_by_name_all_s3_actions_statement = { Sid: `Do not allow user ${user_a} any s3 action`, Effect: 'Deny', @@ -407,9 +406,8 @@ mocha.describe('s3_bucket_policy', function() { 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); + get_deny_account_by_id_all_s3_actions_statement(user_a_account_id); const policy = { Statement: [ allow_all_principals_all_s3_actions_statement, @@ -481,9 +479,8 @@ mocha.describe('s3_bucket_policy', function() { 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); + get_deny_account_by_id_all_s3_actions_statement(user_a_account_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`; @@ -516,12 +513,11 @@ mocha.describe('s3_bucket_policy', function() { 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); + get_deny_account_by_id_all_s3_actions_statement(user_a_account_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`; + allow_account_by_id_all_s3_actions_statement.Sid = `Allow user ${user_a_account_id} any s3 action`; const policy = { Statement: [ deny_account_by_name_all_s3_actions_statement, @@ -582,12 +578,11 @@ mocha.describe('s3_bucket_policy', function() { 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); + get_deny_account_by_id_all_s3_actions_statement(user_a_account_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`; + allow_account_by_id_all_s3_actions_statement.Sid = `Allow user ${user_a_account_id} any s3 action`; const policy = { Statement: [ allow_account_by_id_all_s3_actions_statement, @@ -2113,4 +2108,200 @@ mocha.describe('s3_bucket_policy', function() { }); }); }); + + mocha.describe('Bucket policy with ARN principal', async function() { + mocha.it('should fail : Bucket policy with invalid principal value ', async function() { + if (is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this + try { + this.timeout(5000); // eslint-disable-line no-invalid-this + const s3_policy = { + Version: '2012-10-17', + Statement: [ + { + Action: ['s3:PutObject'], + Effect: 'Allow', + Principal: { AWS: ['invalid'] }, + Resource: [`arn:aws:s3:::${BKT}/*`], + } + ]}; + await s3_owner.putBucketPolicy({ + Bucket: BKT, + Policy: JSON.stringify(s3_policy) + }); + assert.fail('Test was suppose to fail on ' + S3Error.MalformedPolicy.code); + } catch (err) { + if (err.Code !== S3Error.MalformedPolicy.code) { + throw err; + } + } + }); + + mocha.it('should fail : Bucket policy with invalid principal ARN', async function() { + if (is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this + try { + this.timeout(5000); // eslint-disable-line no-invalid-this + const invalid_arn = 'arn:aws:iamm::hsdbfasdwe534sfdsf:root'; + const s3_policy = { + Version: '2012-10-17', + Statement: [ + { + Action: ['s3:PutObject'], + Effect: 'Allow', + Principal: { AWS: [invalid_arn] }, + Resource: [`arn:aws:s3:::${BKT}/*`], + } + ]}; + await s3_owner.putBucketPolicy({ + Bucket: BKT, + Policy: JSON.stringify(s3_policy) + }); + assert.fail('Test was suppose to fail on ' + S3Error.MalformedPolicy.code); + } catch (err) { + if (err.Code !== S3Error.MalformedPolicy.code) { + throw err; + } + } + }); + + mocha.it('should fail : Bucket policy with invalid principal ARN account ID', async function() { + if (is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this + try { + this.timeout(5000); // eslint-disable-line no-invalid-this + const invalid_arn = `arn:aws:iam::${user_b_account_id}1:root`; + const s3_policy = { + Version: '2012-10-17', + Statement: [ + { + Action: ['s3:PutObject'], + Effect: 'Allow', + Principal: { AWS: [invalid_arn] }, + Resource: [`arn:aws:s3:::${BKT}/*`], + } + ]}; + await s3_owner.putBucketPolicy({ + Bucket: BKT, + Policy: JSON.stringify(s3_policy) + }); + assert.fail('Test was suppose to fail on ' + S3Error.MalformedPolicy.code); + } catch (err) { + if (err.Code !== S3Error.MalformedPolicy.code) { + throw err; + } + } + }); + + mocha.it('should fail : Bucket policy with invalid principal IAM user ARN', async function() { + if (is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this + try { + this.timeout(5000); // eslint-disable-line no-invalid-this + // IAM user ARN formate for account ID(user_b_account_id) + const invalid_arn = `arn:aws:iam::${user_b_account_id}:user/Bob`; + const s3_policy = { + Version: '2012-10-17', + Statement: [ + { + Action: ['s3:PutObject'], + Effect: 'Allow', + Principal: { AWS: [invalid_arn] }, + Resource: [`arn:aws:s3:::${BKT}/*`], + } + ]}; + await s3_owner.putBucketPolicy({ + Bucket: BKT, + Policy: JSON.stringify(s3_policy) + }); + assert.fail('Test was suppose to fail on ' + S3Error.MalformedPolicy.code); + } catch (err) { + if (err.Code !== S3Error.MalformedPolicy.code) { + throw err; + } + } + }); + + mocha.it('should fail : Bucket policy with valid principal account ARN, try to putObject with different account', async function() { + if (is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this + this.timeout(5000); // eslint-disable-line no-invalid-this + const valid_arn_b = s3_bucket_policy_utils.create_arn_for_root(user_b_account_id); + const s3_policy = { + Version: '2012-10-17', + Statement: [ + { + Action: ['s3:GetObject'], + Effect: 'Allow', + Principal: { AWS: [valid_arn_b] }, + Resource: [`arn:aws:s3:::${BKT}/*`], + } + ]}; + await s3_owner.putBucketPolicy({ + Bucket: BKT, + Policy: JSON.stringify(s3_policy) + }); + const res_get_bucket_policy = await s3_owner.getBucketPolicy({ + Bucket: BKT, + }); + assert.equal(res_get_bucket_policy.$metadata.httpStatusCode, 200); + // PutObject with account clinet s3_a + await assert_throws_async(s3_a.putObject({ + Body: BODY, + Bucket: BKT, + Key: KEY, + }), 'Access Denied'); + }); + + mocha.it('Bucket policy with valid principal account ARN', async function() { + if (is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this + this.timeout(5000); // eslint-disable-line no-invalid-this + const valid_arn = s3_bucket_policy_utils.create_arn_for_root(user_b_account_id); + const s3_policy = { + Version: '2012-10-17', + Statement: [ + { + Action: ['s3:GetObject'], + Effect: 'Allow', + Principal: { AWS: [valid_arn] }, + Resource: [`arn:aws:s3:::${BKT}/*`], + } + ]}; + await s3_owner.putBucketPolicy({ + Bucket: BKT, + Policy: JSON.stringify(s3_policy) + }); + const res_get_bucket_policy = await s3_owner.getBucketPolicy({ + Bucket: BKT, + }); + assert.equal(res_get_bucket_policy.$metadata.httpStatusCode, 200); + }); + + + mocha.it('Bucket policy with valid principal ARN, and PutObject', async function() { + if (is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this + this.timeout(5000); // eslint-disable-line no-invalid-this + const valid_arn = s3_bucket_policy_utils.create_arn_for_root(user_b_account_id); + const s3_policy = { + Version: '2012-10-17', + Statement: [ + { + Action: ['s3:PutObject'], + Effect: 'Allow', + Principal: { AWS: [valid_arn] }, + Resource: [`arn:aws:s3:::${BKT}/*`], + } + ]}; + await s3_owner.putBucketPolicy({ + Bucket: BKT, + Policy: JSON.stringify(s3_policy) + }); + const res_get_bucket_policy = await s3_owner.getBucketPolicy({ + Bucket: BKT, + }); + assert.equal(res_get_bucket_policy.$metadata.httpStatusCode, 200); + + const res_put_object = await s3_b.putObject({ + Body: BODY, + Bucket: BKT, + Key: KEY + }); + assert.equal(res_put_object.$metadata.httpStatusCode, 200); + }); + }); }); diff --git a/src/util/account_util.js b/src/util/account_util.js index 9f043afb68..98ee51fa72 100644 --- a/src/util/account_util.js +++ b/src/util/account_util.js @@ -342,7 +342,6 @@ function _check_root_account_owns_user(root_account, user_account) { return root_account_id === owner_account_id; } - function _check_if_requesting_account_is_root_account(action, requesting_account, user_details = {}) { const is_root_account = _check_root_account(requesting_account); dbg.log1(`AccountSpaceNB.${action} requesting_account ID: ${requesting_account._id}` +