Skip to content

Commit 8be23f7

Browse files
committed
IAM | Principal in Bucket Policy of the Account Root User When the Requesting Account Is IAM User
1. Add additional check after the check of bucket policy by principal ARN of the user to also check the permission in account level. 2. Fix permission_by_arn call to has_bucket_policy_permission so the last argument will be an object. Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
1 parent 0d80b71 commit 8be23f7

File tree

2 files changed

+30
-4
lines changed

2 files changed

+30
-4
lines changed

src/endpoint/s3/s3_rest.js

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,7 @@ async function authorize_request_policy(req) {
250250
}
251251

252252
const account = req.object_sdk.requesting_account;
253-
const is_nc_deployment = req.object_sdk.nsfs_config_root;
253+
const is_nc_deployment = Boolean(req.object_sdk.nsfs_config_root);
254254
const account_identifier_name = is_nc_deployment ? account.name.unwrap() : account.email.unwrap();
255255
const account_identifier_id = is_nc_deployment ? account._id : undefined;
256256
const account_identifier_arn = s3_bucket_policy_utils.get_bucket_policy_principal_arn(account);
@@ -296,6 +296,7 @@ async function authorize_request_policy(req) {
296296
let permission_by_id;
297297
let permission_by_name;
298298
let permission_by_arn;
299+
let permission_by_arn_owner;
299300

300301
// In NC, we allow principal to be:
301302
// 1. account name (for backwards compatibility)
@@ -321,13 +322,26 @@ async function authorize_request_policy(req) {
321322
// Policy permission is validated by account arn
322323
if (!account_identifier_id) {
323324
permission_by_arn = await s3_bucket_policy_utils.has_bucket_policy_permission(
324-
s3_policy, account_identifier_arn, method, arn_path, req, public_access_block?.restrict_public_buckets
325+
s3_policy, account_identifier_arn, method, arn_path, req,
326+
{ disallow_public_access: public_access_block?.restrict_public_buckets }
325327
);
326328
dbg.log3('authorize_request_policy: permission_by_arn', permission_by_arn);
327329
}
328330
if (permission_by_arn === "DENY") throw new S3Error(S3Error.AccessDenied);
329331

330-
if ((permission_by_id === "ALLOW" || permission_by_name === "ALLOW" || permission_by_arn === "ALLOW") || is_owner) return;
332+
// ARN check for users under the account
333+
// ARN check is not implemented in NC yet
334+
if (!is_nc_deployment && account.owner !== undefined) {
335+
const owner_account_identifier_arn = s3_bucket_policy_utils.create_arn_for_root(account.owner);
336+
permission_by_arn_owner = await s3_bucket_policy_utils.has_bucket_policy_permission(
337+
s3_policy, owner_account_identifier_arn, method, arn_path, req,
338+
{ disallow_public_access: public_access_block?.restrict_public_buckets }
339+
);
340+
dbg.log3('authorize_request_policy permission_by_arn_owner', permission_by_arn_owner);
341+
if (permission_by_arn_owner === "DENY") throw new S3Error(S3Error.AccessDenied);
342+
}
343+
if ((permission_by_id === "ALLOW" || permission_by_name === "ALLOW" ||
344+
permission_by_arn === "ALLOW" || permission_by_arn_owner === "ALLOW") || is_owner) return;
331345

332346
throw new S3Error(S3Error.AccessDenied);
333347
}

src/server/common_services/auth_server.js

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -640,7 +640,19 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
640640

641641
if (result === 'DENY') return false;
642642

643-
return has_owner_access || result === 'ALLOW';
643+
let permission_by_arn_owner;
644+
if (account.owner) {
645+
const owner_account_identifier_arn = s3_bucket_policy_utils.create_arn_for_root(account.owner._id.toString());
646+
permission_by_arn_owner = await s3_bucket_policy_utils.has_bucket_policy_permission(
647+
bucket_policy,
648+
owner_account_identifier_arn,
649+
action,
650+
`arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`,
651+
req_query,
652+
);
653+
if (permission_by_arn_owner === 'DENY') return false;
654+
}
655+
return has_owner_access || result === 'ALLOW' || permission_by_arn_owner === 'ALLOW';
644656
}
645657

646658
/**

0 commit comments

Comments
 (0)