Skip to content

IAM | Principal validation and S3 permission updated with ID #9309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

IAM | Principal validation and S3 permission updated with ID #9309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 19 additions & 9 deletions src/endpoint/s3/s3_bucket_policy_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
});
Expand All @@ -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;
Expand Down Expand Up @@ -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);

Expand Down
10 changes: 6 additions & 4 deletions src/endpoint/s3/s3_rest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,7 +252,8 @@ async function authorize_request_policy(req) {
const account = req.object_sdk.requesting_account;
const is_nc_deployment = 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)
Expand Down Expand Up @@ -309,17 +310,18 @@ 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 }
);
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, public_access_block?.restrict_public_buckets
);
Expand Down
2 changes: 1 addition & 1 deletion src/server/common_services/auth_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,7 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
iam_utils.create_arn_for_root(account._id);
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
Expand Down
26 changes: 23 additions & 3 deletions src/server/system_services/bucket_server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(':');
Expand All @@ -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);
Expand Down
4 changes: 3 additions & 1 deletion src/test/integration_tests/api/iam/test_iam_basic_integration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
50 changes: 24 additions & 26 deletions src/test/integration_tests/api/s3/test_s3_bucket_policy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(),
Expand All @@ -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 });
Expand Down Expand Up @@ -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}/*`]
}]
Expand All @@ -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}/*`]
}]
Expand Down Expand Up @@ -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_b_account_id.toString());
Copy link

@coderabbitai coderabbitai bot Nov 25, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Bug: a_principal is constructed using user_b_account_id instead of user_a.

For containerized deployments, a_principal is being set with user_b_account_id, which is incorrect. This will cause the deny_account_by_name_all_s3_actions_statement to target user_b instead of user_a in containerized tests.

Apply this diff to fix the bug:

-        a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(user_b_account_id.toString());
+        a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(user_a_account_id.toString());
📝 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
a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(user_b_account_id.toString());
a_principal = is_nc_coretest ? user_a : s3_bucket_policy_utils.create_arn_for_root(user_a_account_id.toString());
🤖 Prompt for AI Agents 
In src/test/integration_tests/api/s3/test_s3_bucket_policy.js around line 397,
the code constructs a_principal using user_b_account_id when it should use
user_a's account id for containerized deployments; update the ternary fallback
to use s3_bucket_policy_utils.create_arn_for_root(user_a_account_id.toString())
instead of user_b_account_id so the
deny_account_by_name_all_s3_actions_statement targets user_a as intended.

const deny_account_by_name_all_s3_actions_statement = {
Sid: `Do not allow user ${user_a} any s3 action`,
Effect: 'Deny',
Expand All @@ -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,
Expand Down Expand Up @@ -481,9 +479,9 @@ 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
// 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`;
Expand Down Expand Up @@ -516,12 +514,12 @@ 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
// 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,
Expand Down Expand Up @@ -582,12 +580,12 @@ 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
// 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,
Expand Down
1 change: 0 additions & 1 deletion src/util/account_util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}` +
Expand Down