IAM | Principal validation and S3 permission updated with ID

Signed-off-by: Naveen Paul <napaul@redhat.com>

Author: naveenpaul1

src/endpoint/s3/s3_bucket_policy_utils.js

@@ -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
         }
@@ -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
         });
@@ -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;
@@ -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);
 

src/endpoint/s3/s3_rest.js

@@ -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)
@@ -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
         );

src/server/common_services/auth_server.js

@@ -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

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);

src/test/integration_tests/api/iam/test_iam_basic_integration.js

@@ -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() {

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,

src/util/account_util.js

@@ -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}` +