From 4818d5c66b35bb1ee1d1f90935730aae23d04e59 Mon Sep 17 00:00:00 2001 From: Naveen Paul Date: Tue, 11 Nov 2025 17:09:41 +0530 Subject: [PATCH] IAM | Bucket policy Principal validation Signed-off-by: Naveen Paul --- src/api/account_api.js | 4 + src/endpoint/iam/iam_utils.js | 5 + src/endpoint/s3/s3_bucket_policy_utils.js | 49 ++++++++ src/endpoint/s3/s3_rest.js | 16 ++- src/sdk/accountspace_nb.js | 10 +- src/server/common_services/auth_server.js | 6 +- src/server/system_services/account_server.js | 7 +- src/server/system_services/bucket_server.js | 35 +++++- .../api/s3/test_s3_bucket_policy.js | 117 +++++++++++------- .../integration_tests/api/sts/test_sts.js | 32 +++-- .../nsfs/test_nsfs_integration.js | 10 +- src/util/account_util.js | 12 +- src/util/string_utils.js | 2 + 13 files changed, 223 insertions(+), 82 deletions(-) diff --git a/src/api/account_api.js b/src/api/account_api.js index 45c2b1a61e..e9c4596781 100644 --- a/src/api/account_api.js +++ b/src/api/account_api.js @@ -620,6 +620,7 @@ module.exports = { type: 'object', required: ['name', 'email', 'has_s3_access'], properties: { + _id: { type: 'string'}, name: { $ref: 'common_api#/definitions/account_name' }, email: { $ref: 'common_api#/definitions/email' }, is_support: { @@ -661,6 +662,9 @@ module.exports = { bucket_claim_owner: { $ref: 'common_api#/definitions/bucket_name', }, + iam_path: { + type: 'string', + }, external_connections: { type: 'object', properties: { diff --git a/src/endpoint/iam/iam_utils.js b/src/endpoint/iam/iam_utils.js index 2fc5967fc6..e02e9c50f9 100644 --- a/src/endpoint/iam/iam_utils.js +++ b/src/endpoint/iam/iam_utils.js @@ -62,6 +62,10 @@ function check_iam_path_was_set(iam_path) { return iam_path && iam_path !== iam_constants.IAM_DEFAULT_PATH; } +function get_iam_username(requested_account_name) { + return requested_account_name.split(iam_constants.IAM_SPLIT_CHARACTERS)[0]; +} + /** * parse_max_items converts the input to the needed type * assuming that we've got only sting type @@ -818,6 +822,7 @@ exports.create_arn_for_user = create_arn_for_user; exports.create_arn_for_root = create_arn_for_root; exports.get_action_message_title = get_action_message_title; exports.check_iam_path_was_set = check_iam_path_was_set; +exports.get_iam_username = get_iam_username; exports.parse_max_items = parse_max_items; exports.validate_params = validate_params; exports.validate_iam_path = validate_iam_path; diff --git a/src/endpoint/s3/s3_bucket_policy_utils.js b/src/endpoint/s3/s3_bucket_policy_utils.js index 483e47d894..1132a0e33e 100644 --- a/src/endpoint/s3/s3_bucket_policy_utils.js +++ b/src/endpoint/s3/s3_bucket_policy_utils.js @@ -94,6 +94,7 @@ const OP_NAME_TO_ACTION = Object.freeze({ const qm_regex = /\?/g; const ar_regex = /\*/g; +const IAM_DEFAULT_PATH = '/'; const predicate_map = { 'StringEquals': (request_value, policy_value) => request_value === policy_value, @@ -353,7 +354,55 @@ function allows_public_access(policy) { return false; } +/** + * Return IAM ARN for account, if account dont have owner + * and User, if the user do have account ower + * + * @param {Object} account + * @returns {string} + */ +function get_bucket_policy_principal_arn(account) { + const bucket_policy_arn = account.owner ? create_arn_for_user(account.owner, account.name.unwrap().split(':')[0], account.iam_path) : + create_arn_for_root(account._id); + return bucket_policy_arn; +} + +/** + * create_arn_for_root creates the AWS ARN for root account user + * see: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_identifiers.html#identifiers-arns + * @param {nb.ID} account_id (the root user account id) + */ +function create_arn_for_root(account_id) { + return `arn:aws:iam::${account_id}:root`; +} + +/** + * create_arn_for_user creates the AWS ARN for user + * see: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_identifiers.html#identifiers-arns + * @param {nb.ID} account_id (the root user account id) + * @param {string} username + * @param {string} iam_path + */ +function create_arn_for_user(account_id, username, iam_path) { + const basic_structure = `arn:aws:iam::${account_id}:user`; + if (username === undefined) return `${basic_structure}/`; + if (check_iam_path_was_set(iam_path)) { + return `${basic_structure}${iam_path}${username}`; + } + return `${basic_structure}/${username}`; +} + +/** + * check_iam_path_was_set return true if the iam_path was set + * @param {string} iam_path + */ +function check_iam_path_was_set(iam_path) { + return iam_path && iam_path !== IAM_DEFAULT_PATH; +} + exports.OP_NAME_TO_ACTION = OP_NAME_TO_ACTION; exports.has_bucket_policy_permission = has_bucket_policy_permission; exports.validate_s3_policy = validate_s3_policy; exports.allows_public_access = allows_public_access; +exports.get_bucket_policy_principal_arn = get_bucket_policy_principal_arn; +exports.create_arn_for_root = create_arn_for_root; diff --git a/src/endpoint/s3/s3_rest.js b/src/endpoint/s3/s3_rest.js index f3a5d2b97d..a24d52373d 100755 --- a/src/endpoint/s3/s3_rest.js +++ b/src/endpoint/s3/s3_rest.js @@ -233,7 +233,6 @@ async function authorize_request(req) { async function authorize_request_policy(req) { if (!req.params.bucket) return; if (req.op_name === 'put_bucket') return; - // owner_account is { id: bucket.owner_account, email: bucket.bucket_owner }; const { s3_policy, @@ -256,6 +255,7 @@ async function authorize_request_policy(req) { const account = req.object_sdk.requesting_account; const account_identifier_name = req.object_sdk.nsfs_config_root ? account.name.unwrap() : account.email.unwrap(); const account_identifier_id = req.object_sdk.nsfs_config_root ? account._id : undefined; + 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) // the OBC bucket can still be delete by normal accounts according to the access policy which is checked below @@ -294,6 +294,7 @@ async function authorize_request_policy(req) { // in case we have bucket policy let permission_by_id; let permission_by_name; + let permission_by_arn; // In NC, we allow principal to be: // 1. account name (for backwards compatibility) @@ -307,7 +308,6 @@ 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) { permission_by_name = await s3_bucket_policy_utils.has_bucket_policy_permission( s3_policy, account_identifier_name, method, arn_path, req, @@ -316,7 +316,17 @@ 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); - if ((permission_by_id === "ALLOW" || permission_by_name === "ALLOW") || is_owner) return; + // Containerized deployment always will have account_identifier_id undefined + // Policy permission is validated by account arn + if (!account_identifier_id) { + permission_by_arn = await s3_bucket_policy_utils.has_bucket_policy_permission( + s3_policy, account_identifier_arn, method, arn_path, req, public_access_block?.restrict_public_buckets + ); + dbg.log3('authorize_request_policy: permission_by_arn', permission_by_arn); + } + if (permission_by_arn === "DENY") throw new S3Error(S3Error.AccessDenied); + + if ((permission_by_id === "ALLOW" || permission_by_name === "ALLOW" || permission_by_arn === "ALLOW") || is_owner) return; throw new S3Error(S3Error.AccessDenied); } diff --git a/src/sdk/accountspace_nb.js b/src/sdk/accountspace_nb.js index 6ab7878c29..12c938a242 100644 --- a/src/sdk/accountspace_nb.js +++ b/src/sdk/accountspace_nb.js @@ -85,7 +85,7 @@ class AccountSpaceNB { const action = IAM_ACTIONS.GET_USER; const requesting_account = system_store.get_account_by_email(account_sdk.requesting_account.email); const requested_account = validate_and_return_requested_account(params, action, requesting_account); - const username = account_util.get_iam_username(params.username || requested_account.name.unwrap()); + const username = iam_utils.get_iam_username(params.username || requested_account.name.unwrap()); const iam_arn = iam_utils.create_arn_for_user(requesting_account._id.toString(), username, requested_account.iam_path || IAM_DEFAULT_PATH); const reply = { @@ -110,7 +110,7 @@ class AccountSpaceNB { account_util._check_if_account_exists(action, old_username); const requested_account = system_store.get_account_by_email(old_username); let iam_path = requested_account.iam_path; - let user_name = account_util.get_iam_username(requested_account.name.unwrap()); + let user_name = iam_utils.get_iam_username(requested_account.name.unwrap()); account_util._check_username_already_exists(action, { username: params.new_username }, requesting_account); account_util._check_if_requested_account_is_root_account_or_IAM_user(action, requesting_account, requested_account); account_util._check_if_requested_is_owned_by_root_account(action, requesting_account, requested_account); @@ -173,7 +173,7 @@ class AccountSpaceNB { return account.owner?._id.toString() === requesting_account._id.toString(); }); let members = _.map(requesting_account_iam_users, function(iam_user) { - const iam_username = account_util.get_iam_username(iam_user.name.unwrap()); + const iam_username = iam_utils.get_iam_username(iam_user.name.unwrap()); const iam_path = iam_user.iam_path || IAM_DEFAULT_PATH; const member = { user_id: iam_user._id.toString(), @@ -218,7 +218,7 @@ class AccountSpaceNB { } return { - username: account_util.get_iam_username(requested_account.name.unwrap()), + username: iam_utils.get_iam_username(requested_account.name.unwrap()), access_key: iam_access_key.access_key.unwrap(), create_date: iam_access_key.creation_date, status: ACCESS_KEY_STATUS_ENUM.ACTIVE, @@ -238,7 +238,7 @@ class AccountSpaceNB { region: dummy_region, // GAP last_used_date: Date.now(), // GAP service_name: dummy_service_name, // GAP - username: username ? account_util.get_iam_username(username) : undefined, + username: username ? iam_utils.get_iam_username(username) : undefined, }; } diff --git a/src/server/common_services/auth_server.js b/src/server/common_services/auth_server.js index 71838cda80..24330b3549 100644 --- a/src/server/common_services/auth_server.js +++ b/src/server/common_services/auth_server.js @@ -15,6 +15,7 @@ const addr_utils = require('../../util/addr_utils'); const kube_utils = require('../../util/kube_utils'); const jwt_utils = require('../../util/jwt_utils'); const config = require('../../../config'); +const iam_utils = require('../../endpoint/iam/iam_utils'); const s3_bucket_policy_utils = require('../../endpoint/s3/s3_bucket_policy_utils'); @@ -548,10 +549,11 @@ async function has_bucket_action_permission(bucket, account, action, req_query, if (!action) { 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); const result = await s3_bucket_policy_utils.has_bucket_policy_permission( bucket_policy, - account.email.unwrap(), + arn, action, `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`, req_query diff --git a/src/server/system_services/account_server.js b/src/server/system_services/account_server.js index d55fb5af95..70897fc11b 100644 --- a/src/server/system_services/account_server.js +++ b/src/server/system_services/account_server.js @@ -988,7 +988,7 @@ function get_account_info(account, include_connection_cache) { 'has_login', 'allowed_ips', ); - + info._id = account._id.toString(); if (account.is_support) { info.is_support = true; info.has_login = true; @@ -998,7 +998,10 @@ function get_account_info(account, include_connection_cache) { secret_key: 'Not Accesible' }]; } - + if (account.owner) { + info.owner = account.owner._id.toString(); + } + info.iam_path = account.iam_path; if (account.next_password_change) { info.next_password_change = account.next_password_change.getTime(); } diff --git a/src/server/system_services/bucket_server.js b/src/server/system_services/bucket_server.js index 9317dd044b..d4593cd9df 100644 --- a/src/server/system_services/bucket_server.js +++ b/src/server/system_services/bucket_server.js @@ -11,6 +11,7 @@ const GoogleStorage = require('../../util/google_storage_wrap'); const P = require('../../util/promise'); const dbg = require('../../util/debug_module')(__filename); +const SensitiveString = require('../../util/sensitive_string'); const config = require('../../../config'); const MDStore = require('../object_services/md_store').MDStore; const BucketStatsStore = require('../analytic_services/bucket_stats_store').BucketStatsStore; @@ -39,6 +40,7 @@ const bucket_semaphore = new KeysSemaphore(1); const Quota = require('../system_services/objects/quota'); const { STORAGE_CLASS_GLACIER_IR } = require('../../endpoint/s3/s3_utils'); const noobaa_s3_client = require('../../sdk/noobaa_s3_client/noobaa_s3_client'); +const string_utils = require('../../util/string_utils'); const VALID_BUCKET_NAME_REGEXP = /^(([a-z0-9]|[a-z0-9][a-z0-9-]*[a-z0-9])\.)*([a-z0-9]|[a-z0-9][a-z0-9-]*[a-z0-9])$/; @@ -512,12 +514,43 @@ async function get_bucket_policy(req) { }; } +/** + Validate and return account by principal ARN. + 1. validate basic ARN, like arn prefix `arn:aws:iam::` + 2. If principal ARN ends with `root` suffix, it's an account and get account with id + eg: arn:aws:iam::${account_id}:root + 3. if principal ARN contains `user`, it's an IAM user and get account with username and id + 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 +*/ +async function get_account_by_principal(principal) { + const principal_as_string = principal instanceof SensitiveString ? principal.unwrap() : principal; + const root_sufix = 'root'; + const user_sufix = 'user'; + const arn_parts = principal_as_string.split(':'); + if (!string_utils.AWS_IAM_ARN_REGEXP.test(principal_as_string)) { + return; + } + const account_id = arn_parts[4]; + const arn_sufix = arn_parts[5]; + if (principal_as_string.endsWith(root_sufix) && !arn_sufix.startsWith(user_sufix)) { + return system_store.data.accounts.find(account => account._id.toString() === account_id); + } else if (arn_sufix && arn_sufix.startsWith(user_sufix)) { + const arn_path_parts = principal_as_string.split('/'); + const iam_user_name = arn_path_parts[arn_path_parts.length - 1].trim(); + return system_store.get_account_by_email(new SensitiveString(`${iam_user_name}:${account_id}`)); + } //else { + // wrong principal ARN should not return anything. + //} +} async function put_bucket_policy(req) { dbg.log0('put_bucket_policy:', req.rpc_params); const bucket = find_bucket(req, req.rpc_params.name); await bucket_policy_utils.validate_s3_policy(req.rpc_params.policy, bucket.name, - principal => system_store.get_account_by_email(principal)); + principal => get_account_by_principal(principal)); if ( bucket.public_access_block?.block_public_policy && 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 59732cf57f..af2a9b331f 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 @@ -12,6 +12,7 @@ coretest.setup({ pools_to_create: process.env.NC_CORETEST ? undefined : [POOL_LI const path = require('path'); const _ = require('lodash'); const fs_utils = require('../../../../util/fs_utils'); +const s3_bucket_policy_utils = require('../../../../endpoint/s3/s3_bucket_policy_utils'); const { S3 } = require('@aws-sdk/client-s3'); const { NodeHttpHandler } = require("@smithy/node-http-handler"); @@ -64,6 +65,14 @@ const cross_test_store = {}; let user_a_account_details; let user_b_account_details; +let admin_info; +let account_info_a; +let account_info_b; + +let a_principal; +let b_principal; +let admin_principal; + async function setup() { const self = this; // eslint-disable-line no-invalid-this self.timeout(60000); @@ -107,18 +116,21 @@ async function setup() { new_buckets_path: tmp_fs_root }; } - const admin_keys = (await rpc_client.account.read_account({ + admin_info = (await rpc_client.account.read_account({ email: EMAIL, - })).access_keys; + })); + const admin_keys = admin_info.access_keys; account.name = user_a; account.email = user_a; user_a_account_details = await rpc_client.account.create_account(account); - console.log('user_a_account_details', user_a_account_details); + 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); const user_a_keys = user_a_account_details.access_keys; account.name = user_b; account.email = user_b; user_b_account_details = await rpc_client.account.create_account(account); - console.log('user_b_account_details', user_b_account_details); + 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); const user_b_keys = user_b_account_details.access_keys; s3_creds.credentials = { accessKeyId: user_a_keys[0].access_key.unwrap(), @@ -135,6 +147,14 @@ async function setup() { accessKeyId: admin_keys[0].access_key.unwrap(), secretAccessKey: admin_keys[0].secret_key.unwrap(), }; + /* + For coretest nc, principal will have account name and ++ 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()); + s3_owner = new S3(s3_creds); await s3_owner.createBucket({ Bucket: BKT }); await s3_owner.createBucket({ Bucket: BKT_C }); @@ -181,7 +201,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetBucketPolicy'], Resource: [`arn:aws:s3:::${made_up_bucket}`] }] @@ -199,7 +219,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: [made_up_action], Resource: [`arn:aws:s3:::${BKT}`] }] @@ -215,13 +235,13 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetBucketPolicy'], Resource: [`arn:aws:s3:::${BKT}`] }, { Sid: 'id-2', Effect: 'Deny', - Principal: { AWS: user_b }, + Principal: { AWS: b_principal }, Action: ['s3:*'], Resource: [`arn:aws:s3:::${BKT}`] }] @@ -342,7 +362,7 @@ mocha.describe('s3_bucket_policy', function() { })); }); - mocha.describe('s3_bucket_policy with more complex policies (conflict statements)', function() { + mocha.describe('s3_bucket_policy with more complex policies (conflict statements)', async function() { mocha.after(async function() { await s3_owner.deleteBucketPolicy({ Bucket: BKT_D, @@ -374,11 +394,12 @@ mocha.describe('s3_bucket_policy', function() { Resource: [`arn:aws:s3:::${BKT_D}/*`] }; } - + // 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()); 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] }, + Principal: { AWS: [a_principal] }, Action: ['s3:*'], Resource: [`arn:aws:s3:::${BKT_D}/*`] }; @@ -633,7 +654,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetBucketPolicy'], Resource: [`arn:aws:s3:::${BKT}`] }] @@ -654,25 +675,25 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:ListBucket'], Resource: [`arn:aws:s3:::${BKT}`] }, { Sid: 'id-2', Effect: 'Allow', - Principal: { AWS: user_b }, + Principal: { AWS: b_principal }, Action: ['s3:PutObject'], Resource: [`arn:aws:s3:::${BKT}/*`] }, { Sid: 'id-4', Effect: 'Deny', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:PutObject'], Resource: [`arn:aws:s3:::${BKT}/*`] }, { Sid: 'id-5', Effect: 'Deny', - Principal: { AWS: user_b }, + Principal: { AWS: b_principal }, Action: ['s3:ListBucket'], Resource: [`arn:aws:s3:::${BKT}`] }] @@ -713,19 +734,19 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Deny', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObject', 's3:PutObject'], Resource: [`arn:aws:s3:::${BKT}/user_b_files/*`] }, { Sid: 'id-2', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObject', 's3:PutObject'], Resource: [`arn:aws:s3:::${BKT}/*`] }, { Sid: 'id-3', Effect: 'Allow', - Principal: { AWS: user_b }, + Principal: { AWS: b_principal }, Action: ['s3:GetObject', 's3:PutObject', 's3:DeleteObject'], Resource: [`arn:aws:s3:::${BKT}/*`] }] @@ -761,19 +782,19 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Deny', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObject', 's3:PutObject'], Resource: [`arn:aws:s3:::${BKT}/user_?_files/j?st_*.txt`] }, { Sid: 'id-2', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObject', 's3:PutObject'], Resource: [`arn:aws:s3:::${BKT}/*`] }, { Sid: 'id-3', Effect: 'Allow', - Principal: { AWS: user_b }, + Principal: { AWS: b_principal }, Action: ['s3:GetObject', 's3:PutObject', 's3:DeleteObject'], Resource: [`arn:aws:s3:::${BKT}/*`] }] @@ -821,19 +842,19 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:PutObject', 's3:DeleteObjectVersion'], Resource: [`arn:aws:s3:::${BKT}/*`] }, { Sid: 'id-2', Effect: 'Allow', - Principal: { AWS: user_b }, + Principal: { AWS: b_principal }, Action: ['s3:PutBucketVersioning'], Resource: [`arn:aws:s3:::${BKT}`] }, { Sid: 'id-3', Effect: 'Deny', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:DeleteObject'], Resource: [`arn:aws:s3:::${BKT}/*`] }] @@ -907,7 +928,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObject', 's3:GetObjectAttributes'], Resource: [`arn:aws:s3:::${BKT_C}/*`] }] @@ -937,14 +958,14 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObject'], Resource: [`arn:aws:s3:::${BKT_C}/*`] }, { Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObjectAttributes'], Resource: [`arn:aws:s3:::${BKT_C}/*`] }] @@ -974,7 +995,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObject'], // missing 's3:GetObjectAttributes' Resource: [`arn:aws:s3:::${BKT_C}/*`] }] @@ -1031,7 +1052,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObjectVersion', 's3:GetObjectVersionAttributes'], Resource: [`arn:aws:s3:::${BKT_C}/*`] }] @@ -1063,14 +1084,14 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObjectVersion'], Resource: [`arn:aws:s3:::${BKT_C}/*`] }, { Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObjectVersionAttributes'], Resource: [`arn:aws:s3:::${BKT_C}/*`] }] @@ -1102,7 +1123,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObjectVersion'], // missing 's3:GetObjectVersionAttributes' Resource: [`arn:aws:s3:::${BKT_C}/*`] }] @@ -1126,7 +1147,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Deny', - Principal: { AWS: user_b }, + Principal: { AWS: b_principal }, Action: ['s3:ListBucket'], Resource: [`arn:aws:s3:::${BKT_B}`] }] @@ -1159,7 +1180,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [{ Sid: 'id-1', Effect: 'Deny', - Principal: { AWS: EMAIL }, + Principal: { AWS: admin_principal }, Action: ['s3:*'], Resource: [`arn:aws:s3:::${BKT}`] }] @@ -1623,7 +1644,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [ { Effect: 'Allow', - NotPrincipal: { AWS: user_a }, + NotPrincipal: { AWS: a_principal }, Action: ['s3:PutObject'], Resource: [`arn:aws:s3:::${BKT}/*`] } @@ -1654,7 +1675,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [ { Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:PutObject'], NotResource: [`arn:aws:s3:::${BKT}/*`] } @@ -1687,7 +1708,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [ { Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:PutObject'], NotAction: ['s3:GetObject'], Resource: [`arn:aws:s3:::${BKT}/*`] @@ -1714,8 +1735,8 @@ mocha.describe('s3_bucket_policy', function() { Statement: [ { Effect: 'Allow', - Principal: { AWS: user_a }, - NotPrincipal: { AWS: user_a }, + Principal: { AWS: a_principal }, + NotPrincipal: { AWS: a_principal }, Action: ['s3:PutObject'], Resource: [`arn:aws:s3:::${BKT}/*`] } @@ -1741,7 +1762,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [ { Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:PutObject'], Resource: [`arn:aws:s3:::${BKT}/*`], NotResource: [`arn:aws:s3:::${BKT}/*`] @@ -1793,7 +1814,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [ { Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:PutObject'], } ]}; @@ -1818,7 +1839,7 @@ mocha.describe('s3_bucket_policy', function() { Statement: [ { Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Resource: [`arn:aws:s3:::${BKT}/*`], } ]}; @@ -1886,7 +1907,7 @@ mocha.describe('s3_bucket_policy', function() { { Action: ['s3:PutObject'], Effect: 'Allow', - Principal: { AWS: user_a }, //principal is user + Principal: { AWS: a_principal }, //principal is user Resource: [`arn:aws:s3:::${BKT}/*`], } ]}; @@ -2006,21 +2027,21 @@ mocha.describe('s3_bucket_policy', function() { { Sid: 'id-1', Effect: 'Allow', - Principal: { AWS: user_b }, + Principal: { AWS: b_principal }, Action: ['s3:*'], Resource: [`arn:aws:s3:::${VER_BKT}/${object_key}`] }, { Sid: 'id-2', Effect: 'Allow', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:*'], Resource: [`arn:aws:s3:::${VER_BKT}/${object_key}`] }, { Sid: 'id-3', Effect: 'Deny', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:GetObjectVersion'], Resource: [`arn:aws:s3:::${VER_BKT}/${object_key}`], Condition: { @@ -2032,7 +2053,7 @@ mocha.describe('s3_bucket_policy', function() { { Sid: 'id-4', Effect: 'Deny', - Principal: { AWS: user_a }, + Principal: { AWS: a_principal }, Action: ['s3:DeleteObjectVersion'], Resource: [`arn:aws:s3:::${VER_BKT}/${object_key}`], Condition: { diff --git a/src/test/integration_tests/api/sts/test_sts.js b/src/test/integration_tests/api/sts/test_sts.js index dfddde0788..f98bd3a627 100644 --- a/src/test/integration_tests/api/sts/test_sts.js +++ b/src/test/integration_tests/api/sts/test_sts.js @@ -78,6 +78,10 @@ mocha.describe('STS tests', function() { const user_b = 'bob1'; const user_c = 'charlie1'; + let account_info_a; + let account_info_b; + let account_info_c; + let sts_admin; let sts; let sts_c; @@ -117,17 +121,11 @@ mocha.describe('STS tests', function() { action: ['sts:AssumeRole'], }] }; - const s3accesspolicy = { - Version: '2012-10-17', - Statement: [{ - Effect: 'Allow', - Principal: { AWS: [user_a, user_b, user_c] }, - Action: ['s3:*'], - Resource: ['arn:aws:s3:::first.bucket/*', 'arn:aws:s3:::first.bucket'], - }] - }; + const user_a_keys = (await rpc_client.account.create_account(account)).access_keys; + account_info_a = await rpc_client.account.read_account({ email: user_a }); const user_c_keys = (await rpc_client.account.create_account({ ...account, email: user_c, name: user_c })).access_keys; + account_info_c = await rpc_client.account.read_account({ email: user_c }); user_b_key = (await rpc_client.account.create_account({ ...account, email: user_b, @@ -137,6 +135,18 @@ mocha.describe('STS tests', function() { assume_role_policy: policy } })).access_keys[0].access_key.unwrap(); + account_info_b = await rpc_client.account.read_account({ email: user_b }); + + const s3accesspolicy = { + Version: '2012-10-17', + Statement: [{ + Effect: 'Allow', + Principal: { AWS: [`arn:aws:iam::${account_info_a._id.toString()}:root`, `arn:aws:iam::${account_info_b._id.toString()}:root`, + `arn:aws:iam::${account_info_c._id.toString()}:root`] }, + Action: ['s3:*'], + Resource: ['arn:aws:s3:::first.bucket/*', 'arn:aws:s3:::first.bucket'], + }] + }; sts = new AWS.STS({ ...sts_creds, @@ -513,12 +523,12 @@ mocha.describe('Session token tests', function() { assume_role_policy: policy } })); - + const account_info_alice = await rpc_client.account.read_account({ email: alice2 }); const s3accesspolicy = { Version: '2012-10-17', Statement: [{ Effect: 'Allow', - Principal: { AWS: alice2 }, + Principal: { AWS: `arn:aws:iam::${account_info_alice._id.toString()}:root` }, Action: ['s3:*'], Resource: [ 'arn:aws:s3:::first.bucket/*', diff --git a/src/test/integration_tests/nsfs/test_nsfs_integration.js b/src/test/integration_tests/nsfs/test_nsfs_integration.js index 56397cb924..e8c866efce 100644 --- a/src/test/integration_tests/nsfs/test_nsfs_integration.js +++ b/src/test/integration_tests/nsfs/test_nsfs_integration.js @@ -24,6 +24,7 @@ const test_utils = require('../../system_tests/test_utils'); const { stat, open } = require('../../../util/nb_native')().fs; const { get_process_fs_context } = require('../../../util/native_fs_utils'); const { TYPES } = require('../../../manage_nsfs/manage_nsfs_constants'); +const iam_utils = require('../../../endpoint/iam/iam_utils'); const ManageCLIError = require('../../../manage_nsfs/manage_nsfs_cli_errors').ManageCLIError; const { TMP_PATH, IS_GPFS, is_nc_coretest, require_coretest, invalid_nsfs_root_permissions, generate_s3_policy, create_fs_user_by_platform, delete_fs_user_by_platform, get_new_buckets_path_by_test_env, @@ -205,7 +206,14 @@ mocha.describe('bucket operations - namespace_fs', function() { mocha.it('list buckets without uid, gid', async function() { this.timeout(600000); // eslint-disable-line no-invalid-this // Give s3_owner access to the required buckets - const generated = generate_s3_policy(EMAIL, first_bucket, ['s3:*']); + let principal; + if (is_nc_coretest) { + principal = EMAIL; + } else { + const account_info_admin = await rpc_client.account.read_account({ email: EMAIL }); + principal = iam_utils.create_arn_for_root(account_info_admin._id.toString()); + } + const generated = generate_s3_policy(principal, first_bucket, ['s3:*']); await rpc_client.bucket.put_bucket_policy({ name: first_bucket, policy: generated.policy }); const res = await s3_owner.listBuckets({}); diff --git a/src/util/account_util.js b/src/util/account_util.js index f6f57d0e22..c032a7e345 100644 --- a/src/util/account_util.js +++ b/src/util/account_util.js @@ -13,10 +13,9 @@ const system_store = require('..//server/system_services/system_store').get_inst const pool_server = require('../server/system_services/pool_server'); const { OP_NAME_TO_ACTION } = require('../endpoint/sts/sts_rest'); const IamError = require('../endpoint/iam/iam_errors').IamError; -const { create_arn_for_user, get_action_message_title } = require('../endpoint/iam/iam_utils'); -const { IAM_ACTIONS, MAX_NUMBER_OF_ACCESS_KEYS, IAM_DEFAULT_PATH, - ACCESS_KEY_STATUS_ENUM, IAM_SPLIT_CHARACTERS, IAM_ACTIONS_USER_INLINE_POLICY, - AWS_LIMIT_CHARS_USER_INlINE_POLICY } = require('../endpoint/iam/iam_constants'); +const { create_arn_for_user, get_action_message_title, get_iam_username } = require('../endpoint/iam/iam_utils'); +const { IAM_ACTIONS, MAX_NUMBER_OF_ACCESS_KEYS, IAM_DEFAULT_PATH, ACCESS_KEY_STATUS_ENUM, + IAM_ACTIONS_USER_INLINE_POLICY, AWS_LIMIT_CHARS_USER_INlINE_POLICY } = require('../endpoint/iam/iam_constants'); const demo_access_keys = Object.freeze({ access_key: new SensitiveString('123'), @@ -323,10 +322,6 @@ function get_account_name_from_username(username, requesting_account_id) { return new SensitiveString(`${username}:${requesting_account_id}`); } -function get_iam_username(requested_account_name) { - return requested_account_name.split(IAM_SPLIT_CHARACTERS)[0]; -} - function _check_if_account_exists(action, email_wrapped) { const account = system_store.get_account_by_email(email_wrapped); if (!account) { @@ -722,7 +717,6 @@ exports.delete_account = delete_account; exports.create_account = create_account; exports.generate_account_keys = generate_account_keys; exports.get_account_name_from_username = get_account_name_from_username; -exports.get_iam_username = get_iam_username; exports.get_non_updating_access_key = get_non_updating_access_key; exports._check_if_requesting_account_is_root_account = _check_if_requesting_account_is_root_account; exports._check_username_already_exists = _check_username_already_exists; diff --git a/src/util/string_utils.js b/src/util/string_utils.js index aeeb5076f2..b1ecae19c6 100644 --- a/src/util/string_utils.js +++ b/src/util/string_utils.js @@ -18,6 +18,7 @@ const AWS_IAM_ACCESS_KEY_INPUT_REGEXP = /^[\w]+$/; const AWS_IAM_TAG_KEY_AND_VALUE_REGEXP = /^[\p{L}\p{Z}\p{N}_.:/=+\-@]+$/u; const AWS_POLICY_NAME_REGEXP = /^[\w+=,.@-]+$/; const AWS_POLICY_DOCUMENT_REGEXP = /^[\u0009\u000A\u000D\u0020-\u00FF]+$/; +const AWS_IAM_ARN_REGEXP = /^arn:aws:iam::\w{10,}:(?:root|user\/[\w\-\.\/]+)$/; function crypto_random_string(len, charset = ALPHA_NUMERIC_CHARSET) { // In order to not favor any specific chars over others we limit the maximum random value @@ -171,3 +172,4 @@ exports.AWS_IAM_ACCESS_KEY_INPUT_REGEXP = AWS_IAM_ACCESS_KEY_INPUT_REGEXP; exports.AWS_IAM_TAG_KEY_AND_VALUE_REGEXP = AWS_IAM_TAG_KEY_AND_VALUE_REGEXP; exports.AWS_POLICY_NAME_REGEXP = AWS_POLICY_NAME_REGEXP; exports.AWS_POLICY_DOCUMENT_REGEXP = AWS_POLICY_DOCUMENT_REGEXP; +exports.AWS_IAM_ARN_REGEXP = AWS_IAM_ARN_REGEXP;