Merge pull request #9309 from naveenpaul1/iam_permission_by_id

IAM | Principal validation and S3 permission updated with ID

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

@@ -14,7 +14,7 @@ const http_utils = require('../../util/http_utils');
 const signature_utils = require('../../util/signature_utils');
 const config = require('../../../config');
 const s3_utils = require('./s3_utils');
-const { _create_detailed_message_for_iam_user_access_in_s3 } = require('../iam/iam_utils'); // for IAM policy
+const { _create_detailed_message_for_iam_user_access_in_s3, get_owner_account_id } = require('../iam/iam_utils'); // for IAM policy
 
 const S3_MAX_BODY_LEN = 4 * 1024 * 1024;
 
@@ -252,7 +252,8 @@ async function authorize_request_policy(req) {
     const account = req.object_sdk.requesting_account;
     const is_nc_deployment = Boolean(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)
@@ -296,7 +297,7 @@ async function authorize_request_policy(req) {
     let permission_by_id;
     let permission_by_name;
     let permission_by_arn;
-    let permission_by_arn_owner;
+    let permission_by_owner;
 
     // In NC, we allow principal to be:
     // 1. account name (for backwards compatibility)
@@ -310,17 +311,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,
             { disallow_public_access: public_access_block?.restrict_public_buckets }
@@ -329,19 +331,20 @@ async function authorize_request_policy(req) {
     }
     if (permission_by_arn === "DENY") throw new S3Error(S3Error.AccessDenied);
 
-    // ARN check for users under the account
+    // ARN and ID check for users under the account
     // ARN check is not implemented in NC yet
      if (!is_nc_deployment && account.owner !== undefined) {
-        const owner_account_identifier_arn = s3_bucket_policy_utils.create_arn_for_root(account.owner);
-        permission_by_arn_owner = await s3_bucket_policy_utils.has_bucket_policy_permission(
-            s3_policy, owner_account_identifier_arn, method, arn_path, req,
+        const owner_account_id = get_owner_account_id(account);
+        const owner_account_identifier_arn = s3_bucket_policy_utils.create_arn_for_root(owner_account_id);
+        permission_by_owner = await s3_bucket_policy_utils.has_bucket_policy_permission(
+            s3_policy, [owner_account_identifier_arn, owner_account_id], method, arn_path, req,
             { disallow_public_access: public_access_block?.restrict_public_buckets }
         );
-        dbg.log3('authorize_request_policy permission_by_arn_owner', permission_by_arn_owner);
-        if (permission_by_arn_owner === "DENY") throw new S3Error(S3Error.AccessDenied);
+        dbg.log3('authorize_request_policy permission_by_arn_owner', permission_by_owner);
+        if (permission_by_owner === "DENY") throw new S3Error(S3Error.AccessDenied);
      }
     if ((permission_by_id === "ALLOW" || permission_by_name === "ALLOW" ||
-        permission_by_arn === "ALLOW" || permission_by_arn_owner === "ALLOW") || is_owner) return;
+        permission_by_arn === "ALLOW" || permission_by_owner === "ALLOW") || is_owner) return;
 
     throw new S3Error(S3Error.AccessDenied);
 }

src/server/common_services/auth_server.js

@@ -629,10 +629,10 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
         throw new Error('has_bucket_action_permission: action is required');
     }
     const arn = account.owner ? iam_utils.create_arn_for_user(account.owner._id.toString(), account.name.unwrap().split(':')[0], account.iam_path) :
-                                    iam_utils.create_arn_for_root(account._id);
+                                    iam_utils.create_arn_for_root(account._id.toString());
     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
@@ -641,11 +641,16 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
     if (result === 'DENY') return false;
 
     let permission_by_arn_owner;
+    // Added to verify the IAM users's owner account have bucket access
+    // If yes, IAM user also should get access
     if (account.owner) {
-        const owner_account_identifier_arn = s3_bucket_policy_utils.create_arn_for_root(account.owner._id.toString());
+        const owner_account_id = iam_utils.get_owner_account_id(account);
+        const owner_account_identifier_arn = s3_bucket_policy_utils.create_arn_for_root(owner_account_id);
+        // We support both ARN and account/user ID in bucket policy Principal, So if the bucket policy have only ARN
+        // sharing array of ARN and ID can fix the issue that misses the access just because of bucket policy have ID as principal 
         permission_by_arn_owner = await s3_bucket_policy_utils.has_bucket_policy_permission(
             bucket_policy,
-            owner_account_identifier_arn,
+            [owner_account_identifier_arn, owner_account_id],
             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() {