Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/api/account_api.js
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand Down Expand Up @@ -661,6 +662,9 @@ module.exports = {
bucket_claim_owner: {
$ref: 'common_api#/definitions/bucket_name',
},
iam_path: {
type: 'string',
},
external_connections: {
type: 'object',
properties: {
Expand Down
5 changes: 5 additions & 0 deletions src/endpoint/iam/iam_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down
49 changes: 49 additions & 0 deletions src/endpoint/s3/s3_bucket_policy_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
16 changes: 13 additions & 3 deletions src/endpoint/s3/s3_rest.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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,
Expand All @@ -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);
}
Expand Down
10 changes: 5 additions & 5 deletions src/sdk/accountspace_nb.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand All @@ -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);
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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,
Expand All @@ -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,
};
}

Expand Down
6 changes: 4 additions & 2 deletions src/server/common_services/auth_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');


Expand Down Expand Up @@ -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(
Comment on lines +552 to 554
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Handle string and object owners when building ARN

In S3 REST flows the requesting account arrives over RPC with account.owner already serialized to a string (see prior learnings). Accessing account.owner._id throws, breaking authorization. Normalize the owner value the same way you do in the policy helper so both string and object forms work.

-    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 owner_id = typeof account.owner === 'object' ? account.owner._id : account.owner;
+    const iam_user_name = account.name.unwrap().split(':')[0];
+    const arn = owner_id ?
+        iam_utils.create_arn_for_user(owner_id.toString(), iam_user_name, account.iam_path) :
+        iam_utils.create_arn_for_root(account._id);

Based on learnings

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
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(
const owner_id = typeof account.owner === 'object' ? account.owner._id : account.owner;
const iam_user_name = account.name.unwrap().split(':')[0];
const arn = owner_id ?
iam_utils.create_arn_for_user(owner_id.toString(), iam_user_name, account.iam_path) :
iam_utils.create_arn_for_root(account._id);
const result = await s3_bucket_policy_utils.has_bucket_policy_permission(
🤖 Prompt for AI Agents
In src/server/common_services/auth_server.js around lines 552 to 554, the code
assumes account.owner is an object and calls account.owner._id, which breaks
when owner is a serialized string; update to normalize owner first (e.g., if
typeof account.owner === 'string' use that string as ownerId, else extract
account.owner._id.toString()), then pass the normalized ownerId into
iam_utils.create_arn_for_user (keeping the existing
account.name.unwrap().split(':')[0] and account.iam_path), and preserve the
existing fallback to iam_utils.create_arn_for_root when account.owner is falsy.

bucket_policy,
account.email.unwrap(),
arn,
action,
`arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`,
req_query
Expand Down
7 changes: 5 additions & 2 deletions src/server/system_services/account_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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();
}
Expand Down
35 changes: 34 additions & 1 deletion src/server/system_services/bucket_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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])$/;
Expand Down Expand Up @@ -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 &&
Expand Down
Loading
Loading