IAM | Principal validation and S3 permission updated with ID

naveenpaul1 · naveenpaul1 · commit c1a440ae57c7 · 2025-11-24T17:04:14.000+05:30
diff --git a/src/endpoint/s3/s3_rest.js b/src/endpoint/s3/s3_rest.js
@@ -256,7 +256,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)
@@ -310,24 +311,25 @@ async function authorize_request_policy(req) {
             s3_policy, account_identifier_id, method, arn_path, req,
             { disallow_public_access: public_access_block?.restrict_public_buckets }
         );
-        dbg.log3('authorize_request_policy: permission_by_id', permission_by_id);
+        dbg.log0('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 && (!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,
             { disallow_public_access: public_access_block?.restrict_public_buckets }
         );
-        dbg.log3('authorize_request_policy: permission_by_name', permission_by_name);
+        dbg.log0('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
         );
-        dbg.log3('authorize_request_policy: permission_by_arn', permission_by_arn);
+        dbg.log0('authorize_request_policy: permission_by_arn', permission_by_arn);
     }
     if (permission_by_arn === "DENY") throw new S3Error(S3Error.AccessDenied);
 
diff --git a/src/server/system_services/bucket_server.js b/src/server/system_services/bucket_server.js
@@ -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(':');
@@ -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);
diff --git a/src/test/integration_tests/api/s3/test_s3_bucket_policy.js b/src/test/integration_tests/api/s3/test_s3_bucket_policy.js
@@ -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;
@@ -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(),
@@ -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 });
@@ -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}/*`]
             }]
@@ -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}/*`]
             }]
@@ -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());
         const deny_account_by_name_all_s3_actions_statement = {
             Sid: `Do not allow user ${user_a} any s3 action`,
             Effect: 'Deny',
@@ -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,
@@ -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`;
@@ -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,
@@ -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,
