Skip to content

Commit ea65377

Browse files
Restructure the code and removed iam policy checks
Signed-off-by: Aayush Chouhan <achouhan@redhat.com>
1 parent 727cf23 commit ea65377

File tree

3 files changed

+142
-100
lines changed

3 files changed

+142
-100
lines changed

src/server/common_services/auth_server.js

Lines changed: 25 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -497,8 +497,8 @@ function _prepare_auth_request(req) {
497497
return has_bucket_action_permission(bucket, req.account, action, req_query, bucket_path);
498498
};
499499

500-
req.has_list_bucket_permission = function(bucket) {
501-
return has_list_bucket_permission(bucket, req.account, req.system);
500+
req.has_bucket_ownership_permission = function(bucket) {
501+
return has_bucket_ownership_permission(bucket, req.account);
502502
};
503503
}
504504

@@ -526,12 +526,12 @@ function _get_auth_info(account, system, authorized_by, role, extra) {
526526

527527
/**
528528
* is_system_owner checks if the account is the system owner
529-
* @param {Record<string, any>} system
529+
* @param {Record<string, any>} bucket
530530
* @param {Record<string, any>} account
531531
* @returns {boolean}
532532
*/
533-
function is_system_owner(system, account) {
534-
return system?.owner?.email?.unwrap() === account?.email?.unwrap();
533+
function is_system_owner(bucket, account) {
534+
return bucket.system.owner.email.unwrap() === account.email.unwrap();
535535
}
536536

537537
/**
@@ -541,79 +541,47 @@ function is_system_owner(system, account) {
541541
* @returns {boolean}
542542
*/
543543
function is_bucket_owner(bucket, account) {
544-
return bucket?.owner_account?.email?.unwrap() === account?.email?.unwrap();
544+
return bucket.owner_account.email.unwrap() === account.email.unwrap();
545545
}
546546

547547
/**
548-
* has_iam_list_buckets_permission checks if the account has IAM policy granting s3:ListAllMyBuckets
549-
* for buckets owned by their root account
548+
* is_bucket_claim_owner checks if the account is the OBC (ObjectBucketClaim) owner of the bucket
550549
* @param {Record<string, any>} bucket
551550
* @param {Record<string, any>} account
552-
* @returns {Promise<boolean>}
551+
* @returns {boolean}
553552
*/
554-
async function has_iam_list_buckets_permission(bucket, account) {
555-
if (!account?.owner) return false;
556-
557-
const iam_policies = account.iam_user_policies || [];
558-
if (iam_policies.length === 0) return false;
559-
560-
// check each IAM policy for s3:ListAllMyBuckets permission
561-
for (const iam_policy of iam_policies) {
562-
const result = await s3_bucket_policy_utils.has_bucket_policy_permission(
563-
iam_policy.policy_document,
564-
undefined,
565-
's3:ListAllMyBuckets',
566-
'arn:aws:s3:::*',
567-
undefined,
568-
{ should_pass_principal: false }
569-
);
570-
571-
if (result === 'DENY') return false;
572-
573-
if (result === 'ALLOW') {
574-
const bucket_owner_id = bucket.owner_account?._id;
575-
const account_owner_id = account.owner?._id;
576-
const ownership_match = account_owner_id !== undefined && bucket_owner_id !== undefined &&
577-
String(bucket_owner_id) === String(account_owner_id);
578-
579-
// special case: check if root is OBC account and this is the claimed bucket
580-
const root_claimed_bucket = account.owner?.bucket_claim_owner?.name?.unwrap();
581-
const obc_claim_match = root_claimed_bucket && root_claimed_bucket === bucket.name?.unwrap();
582-
583-
if (ownership_match || obc_claim_match) return true;
584-
}
585-
}
586-
587-
return false;
553+
function is_bucket_claim_owner(bucket, account) {
554+
return account.bucket_claim_owner && account.bucket_claim_owner.name.unwrap() === bucket.name.unwrap();
588555
}
589556

590557
/**
591-
* has_list_bucket_permission returns true if the account can list the bucket in ListBuckets operation
558+
* has_bucket_ownership_permission returns true if the account can list the bucket in ListBuckets operation
592559
*
593560
* aws-compliant behavior:
594561
* - System owner can list all the buckets
595562
* - Root accounts can list buckets they own
596-
* - IAM users CANNOT inherit ListBuckets visibility from root
597-
* - IAM users need explicit IAM policy with s3:ListAllMyBuckets to list buckets
563+
* - OBC owner can list their buckets
564+
* - IAM users can list their owner buckets
598565
*
599566
* @param {Record<string, any>} bucket
600567
* @param {Record<string, any>} account
601-
* @param {Record<string, any>} system
602568
* @returns {Promise<boolean>}
603569
*/
604-
async function has_list_bucket_permission(bucket, account, system) {
570+
async function has_bucket_ownership_permission(bucket, account) {
605571
// system owner can list all the buckets
606-
if (is_system_owner(bucket.system, account)) return true;
572+
if (is_system_owner(bucket, account)) return true;
607573

608574
// check direct ownership
609575
if (is_bucket_owner(bucket, account)) return true;
610576

611577
// special case: check bucket claim ownership (OBC)
612-
if (account.bucket_claim_owner?.name?.unwrap() === bucket.name.unwrap()) return true;
578+
if (is_bucket_claim_owner(bucket, account)) return true;
613579

614-
// check IAM policy for s3:ListAllMyBuckets
615-
// Note: IAM users need this policy to list buckets owned by their root account
616-
if (await has_iam_list_buckets_permission(bucket, account)) return true;
580+
// special case: iam user can list the buckets of their owner
581+
if (account.owner) {
582+
const account_owner = system_store.data.get_by_id(account.owner._id);
583+
if (is_bucket_owner(bucket, account_owner)) return true;
584+
}
617585

618586
return false;
619587
}
@@ -640,14 +608,8 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
640608
// system owner can access all buckets
641609
if (is_system_owner(bucket.system, account)) return true;
642610

643-
// check ownership: direct owner, IAM user inheritance, or OBC
644-
const is_owner = is_bucket_owner(bucket, account);
645-
const bucket_owner_id = bucket.owner_account?._id;
646-
const account_owner_id = account.owner?._id;
647-
const has_iam_access = account_owner_id !== undefined && bucket_owner_id !== undefined &&
648-
String(bucket_owner_id) === String(account_owner_id);
649-
const has_obc_access = account.bucket_claim_owner?.name?.unwrap() === bucket.name.unwrap();
650-
const owner = is_owner || has_iam_access || has_obc_access;
611+
// check ownership: direct owner or OBC
612+
const has_owner_access = is_bucket_owner(bucket, account) || is_bucket_claim_owner(bucket, account);
651613

652614
const bucket_policy = bucket.s3_policy;
653615

@@ -656,7 +618,7 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
656618
// we allow IAM account to access a bucket that that is owned by their root account
657619
const is_iam_and_same_root_account_owner = account.owner !== undefined &&
658620
account.owner._id.toString() === bucket.owner_account._id.toString();
659-
return is_owner || is_iam_and_same_root_account_owner;
621+
return has_owner_access || is_iam_and_same_root_account_owner;
660622
}
661623
if (!action) {
662624
throw new Error('has_bucket_action_permission: action is required');
@@ -673,7 +635,7 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
673635

674636
if (result === 'DENY') return false;
675637

676-
return owner || result === 'ALLOW';
638+
return has_owner_access || result === 'ALLOW';
677639
}
678640

679641
/**

src/server/system_services/bucket_server.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1096,10 +1096,10 @@ async function list_buckets(req) {
10961096
let continuation_token = req.rpc_params?.continuation_token;
10971097
const max_buckets = req.rpc_params?.max_buckets;
10981098

1099-
// filter buckets based on ownership (async to support IAM policy checks)
1099+
// filter buckets based on ownership
11001100
const bucket_permissions = await P.map(system_store.data.buckets, async bucket => {
11011101
if (bucket.deleting) return null;
1102-
const has_permission = await req.has_list_bucket_permission(bucket);
1102+
const has_permission = await req.has_bucket_ownership_permission(bucket);
11031103
return has_permission ? bucket : null;
11041104
});
11051105
const accessible_bucket_list = bucket_permissions.filter(bucket => bucket !== null);

0 commit comments

Comments
 (0)