Skip to content

Commit 063d384

Browse files
committed
IAM | Bucket policy Principal validation
Signed-off-by: Naveen Paul <napaul@redhat.com>
1 parent ed6b491 commit 063d384

File tree

10 files changed

+162
-67
lines changed

10 files changed

+162
-67
lines changed

src/api/account_api.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,6 +620,7 @@ module.exports = {
620620
type: 'object',
621621
required: ['name', 'email', 'has_s3_access'],
622622
properties: {
623+
_id: { type: 'string'},
623624
name: { $ref: 'common_api#/definitions/account_name' },
624625
email: { $ref: 'common_api#/definitions/email' },
625626
is_support: {
@@ -661,6 +662,12 @@ module.exports = {
661662
bucket_claim_owner: {
662663
$ref: 'common_api#/definitions/bucket_name',
663664
},
665+
owner: {
666+
type: 'string',
667+
},
668+
iam_path: {
669+
type: 'string',
670+
},
664671
external_connections: {
665672
type: 'object',
666673
properties: {

src/endpoint/iam/iam_utils.js

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,15 @@ function create_arn_for_user(account_id, username, iam_path) {
4646
return `${basic_structure}/${username}`;
4747
}
4848

49+
/**
50+
* create_arn_for_account creates the AWS ARN for account
51+
* see: https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_identifiers.html#identifiers-arns
52+
* @param {string} account_id (the root user account id)
53+
*/
54+
function create_arn_for_account(account_id) {
55+
return `arn:aws:iam::${account_id}:root`;
56+
}
57+
4958
/**
5059
* get_action_message_title returns the full action name
5160
* @param {string} action (The action name as it is in AccountSpace)
@@ -815,6 +824,7 @@ function validate_list_user_tags_params(params) {
815824
// EXPORTS
816825
exports.format_iam_xml_date = format_iam_xml_date;
817826
exports.create_arn_for_user = create_arn_for_user;
827+
exports.create_arn_for_account = create_arn_for_account;
818828
exports.create_arn_for_root = create_arn_for_root;
819829
exports.get_action_message_title = get_action_message_title;
820830
exports.check_iam_path_was_set = check_iam_path_was_set;

src/endpoint/s3/s3_rest.js

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ const s3_logging = require('./s3_bucket_logging');
1212
const time_utils = require('../../util/time_utils');
1313
const http_utils = require('../../util/http_utils');
1414
const signature_utils = require('../../util/signature_utils');
15+
const iam_utils = require('../../endpoint/iam/iam_utils');
1516
const config = require('../../../config');
1617
const s3_utils = require('./s3_utils');
1718

@@ -254,6 +255,8 @@ async function authorize_request_policy(req) {
254255
const account = req.object_sdk.requesting_account;
255256
const account_identifier_name = req.object_sdk.nsfs_config_root ? account.name.unwrap() : account.email.unwrap();
256257
const account_identifier_id = req.object_sdk.nsfs_config_root ? account._id : undefined;
258+
const arn = account.owner ? iam_utils.create_arn_for_user(account.owner, account.name.unwrap().split(':')[0], account.iam_path) :
259+
iam_utils.create_arn_for_account(account._id);
257260

258261
// deny delete_bucket permissions from bucket_claim_owner accounts (accounts that were created by OBC from openshift\k8s)
259262
// the OBC bucket can still be delete by normal accounts according to the access policy which is checked below
@@ -292,6 +295,7 @@ async function authorize_request_policy(req) {
292295
// in case we have bucket policy
293296
let permission_by_id;
294297
let permission_by_name;
298+
let permission_by_arn;
295299

296300
// In NC, we allow principal to be:
297301
// 1. account name (for backwards compatibility)
@@ -303,15 +307,20 @@ async function authorize_request_policy(req) {
303307
dbg.log3('authorize_request_policy: permission_by_id', permission_by_id);
304308
}
305309
if (permission_by_id === "DENY") throw new S3Error(S3Error.AccessDenied);
306-
307310
if ((!account_identifier_id || permission_by_id !== "DENY") && account.owner === undefined) {
308311
permission_by_name = await s3_bucket_policy_utils.has_bucket_policy_permission(
309312
s3_policy, account_identifier_name, method, arn_path, req, public_access_block?.restrict_public_buckets
310313
);
311314
dbg.log3('authorize_request_policy: permission_by_name', permission_by_name);
312315
}
313316
if (permission_by_name === "DENY") throw new S3Error(S3Error.AccessDenied);
314-
if ((permission_by_id === "ALLOW" || permission_by_name === "ALLOW") || is_owner) return;
317+
if (!account_identifier_id) {
318+
permission_by_arn = await s3_bucket_policy_utils.has_bucket_policy_permission(
319+
s3_policy, arn, method, arn_path, req, public_access_block?.restrict_public_buckets
320+
);
321+
dbg.log3('authorize_request_policy: permission_by_arn', permission_by_arn);
322+
}
323+
if ((permission_by_id === "ALLOW" || permission_by_name === "ALLOW" || permission_by_arn === "ALLOW") || is_owner) return;
315324

316325
throw new S3Error(S3Error.AccessDenied);
317326
}

src/server/common_services/auth_server.js

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ const addr_utils = require('../../util/addr_utils');
1515
const kube_utils = require('../../util/kube_utils');
1616
const jwt_utils = require('../../util/jwt_utils');
1717
const config = require('../../../config');
18+
const iam_utils = require('../../endpoint/iam/iam_utils');
1819
const s3_bucket_policy_utils = require('../../endpoint/s3/s3_bucket_policy_utils');
1920

2021

@@ -548,10 +549,11 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
548549
if (!action) {
549550
throw new Error('has_bucket_action_permission: action is required');
550551
}
551-
552+
const arn = account.owner ? iam_utils.create_arn_for_user(account.owner._id.toString(), account.name.unwrap().split(':')[0], account.iam_path) :
553+
iam_utils.create_arn_for_account(account._id);
552554
const result = await s3_bucket_policy_utils.has_bucket_policy_permission(
553555
bucket_policy,
554-
account.email.unwrap(),
556+
arn,
555557
action,
556558
`arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`,
557559
req_query

src/server/system_services/account_server.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -988,7 +988,7 @@ function get_account_info(account, include_connection_cache) {
988988
'has_login',
989989
'allowed_ips',
990990
);
991-
991+
info._id = account._id.toString();
992992
if (account.is_support) {
993993
info.is_support = true;
994994
info.has_login = true;
@@ -998,7 +998,8 @@ function get_account_info(account, include_connection_cache) {
998998
secret_key: 'Not Accesible'
999999
}];
10001000
}
1001-
1001+
info.owner = account.owner?._id.toString();
1002+
info.iam_path = account.iam_path;
10021003
if (account.next_password_change) {
10031004
info.next_password_change = account.next_password_change.getTime();
10041005
}

src/server/system_services/bucket_server.js

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ const GoogleStorage = require('../../util/google_storage_wrap');
1111

1212
const P = require('../../util/promise');
1313
const dbg = require('../../util/debug_module')(__filename);
14+
const SensitiveString = require('../../util/sensitive_string');
1415
const config = require('../../../config');
1516
const MDStore = require('../object_services/md_store').MDStore;
1617
const BucketStatsStore = require('../analytic_services/bucket_stats_store').BucketStatsStore;
@@ -39,6 +40,7 @@ const bucket_semaphore = new KeysSemaphore(1);
3940
const Quota = require('../system_services/objects/quota');
4041
const { STORAGE_CLASS_GLACIER_IR } = require('../../endpoint/s3/s3_utils');
4142
const noobaa_s3_client = require('../../sdk/noobaa_s3_client/noobaa_s3_client');
43+
const string_utils = require('../../util/string_utils');
4244

4345
const VALID_BUCKET_NAME_REGEXP =
4446
/^(([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,37 @@ async function get_bucket_policy(req) {
512514
};
513515
}
514516

517+
/*
518+
validate ARN principle
519+
1. validate basic ARN, like arn prefix `arn:aws:iam::`
520+
2. If principal ARN end with `root sufix its a account and get account with id eg : arn:aws:iam::${account_id}:root
521+
3. if principle ARN contains `user` its a IAM user and get account with username and id
522+
eg : arn:aws:iam::${account_id}:user/${iam_path}/${use_name}
523+
account email = ${iam_user_name}:${account_id}
524+
*/
525+
async function principal_validation_handler(principal) {
526+
const principal_as_string = principal instanceof SensitiveString ? principal.unwrap() : principal;
527+
const root_sufix = 'root';
528+
const user_sufix = 'user';
529+
const arn_parts = principal_as_string.split(':');
530+
if (!string_utils.AWS_ARN_REGEXP.test(principal_as_string)) {
531+
return;
532+
}
533+
const account_id = arn_parts[4];
534+
if (principal_as_string.endsWith(root_sufix)) {
535+
return system_store.data.accounts.find(account => account._id.toString() === account_id);
536+
} else if (arn_parts[5] && arn_parts[5].startsWith(user_sufix)) {
537+
const arn_path_parts = principal_as_string.split('/');
538+
const iam_user_name = arn_path_parts[arn_path_parts.length - 1].trim();
539+
return system_store.get_account_by_email(new SensitiveString(`${iam_user_name}:${account_id}`));
540+
}
541+
}
515542

516543
async function put_bucket_policy(req) {
517544
dbg.log0('put_bucket_policy:', req.rpc_params);
518545
const bucket = find_bucket(req, req.rpc_params.name);
519546
await bucket_policy_utils.validate_s3_policy(req.rpc_params.policy, bucket.name,
520-
principal => system_store.get_account_by_email(principal));
547+
principal => principal_validation_handler(principal));
521548

522549
if (
523550
bucket.public_access_block?.block_public_policy &&

0 commit comments

Comments
 (0)