Merge pull request #9336 from shirady/iam-refactor-bucket-policy-utils

IAM | Refactor `src/endpoint/s3/s3_bucket_policy_utils.js` (Used in Bucket Policy and IAM User Inline Policy)

Author: shirady

src/endpoint/s3/s3_rest.js

@@ -7,7 +7,7 @@ const net = require('net');
 const dbg = require('../../util/debug_module')(__filename);
 const s3_ops = require('./ops');
 const S3Error = require('./s3_errors').S3Error;
-const s3_bucket_policy_utils = require('./s3_bucket_policy_utils');
+const access_policy_utils = require('../../util/access_policy_utils');
 const s3_logging = require('./s3_bucket_logging');
 const time_utils = require('../../util/time_utils');
 const http_utils = require('../../util/http_utils');
@@ -254,8 +254,8 @@ async function authorize_request_policy(req) {
     const account_identifier_name = is_nc_deployment ? account.name.unwrap() : account.email.unwrap();
     // Both NSFS NC and containerized will validate bucket policy against acccount id 
     // but in containerized deplyment not against IAM user ID.
-    const account_identifier_id = s3_bucket_policy_utils.get_account_identifier_id(is_nc_deployment, account);
-    const account_identifier_arn = s3_bucket_policy_utils.get_bucket_policy_principal_arn(account);
+    const account_identifier_id = access_policy_utils.get_account_identifier_id(is_nc_deployment, account);
+    const account_identifier_arn = access_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)
     // the OBC bucket can still be delete by normal accounts according to the access policy which is checked below
     if (req.op_name === 'delete_bucket' && account.bucket_claim_owner) {
@@ -304,7 +304,7 @@ async function authorize_request_policy(req) {
     // 2. account id
     // we start the permission check on account identifier intentionally
     if (account_identifier_id) {
-        permission_by_id = await s3_bucket_policy_utils.has_bucket_policy_permission(
+        permission_by_id = await access_policy_utils.has_access_policy_permission(
             s3_policy, account_identifier_id, method, arn_path, req,
             { disallow_public_access: public_access_block?.restrict_public_buckets }
         );
@@ -313,7 +313,7 @@ async function authorize_request_policy(req) {
     if (permission_by_id === "DENY") throw new S3Error(S3Error.AccessDenied);
     // 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(
+        permission_by_name = await access_policy_utils.has_access_policy_permission(
             s3_policy, account_identifier_name, method, arn_path, req,
             { disallow_public_access: public_access_block?.restrict_public_buckets }
         );
@@ -323,7 +323,7 @@ async function authorize_request_policy(req) {
     // Check bucket policy permission against ARN only for containerized deployments.
     // Policy permission is validated by account arn
     if (!is_nc_deployment && permission_by_id !== "DENY") {
-        permission_by_arn = await s3_bucket_policy_utils.has_bucket_policy_permission(
+        permission_by_arn = await access_policy_utils.has_access_policy_permission(
             s3_policy, account_identifier_arn, method, arn_path, req,
             { disallow_public_access: public_access_block?.restrict_public_buckets }
         );
@@ -335,8 +335,8 @@ async function authorize_request_policy(req) {
     // ARN check is not implemented in NC yet
      if (!is_nc_deployment && account.owner !== undefined) {
         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(
+        const owner_account_identifier_arn = access_policy_utils.create_arn_for_root(owner_account_id);
+        permission_by_owner = await access_policy_utils.has_access_policy_permission(
             s3_policy, [owner_account_identifier_arn, owner_account_id], method, arn_path, req,
             { disallow_public_access: public_access_block?.restrict_public_buckets }
         );
@@ -371,8 +371,7 @@ async function authorize_request_iam_policy(req) {
     // parallel policy check
     const promises = [];
     for (const iam_policy of iam_policies) {
-        // We are reusing the bucket policy util function as it checks the policy document
-        const promise = s3_bucket_policy_utils.has_bucket_policy_permission(
+        const promise = access_policy_utils.has_access_policy_permission(
             iam_policy.policy_document, undefined, method, resource_arn, req,
             { should_pass_principal: false }
         );
@@ -403,7 +402,7 @@ function _throw_iam_access_denied_error_for_s3_operation(requesting_account, met
 async function authorize_anonymous_access(s3_policy, method, arn_path, req, public_access_block) {
     if (!s3_policy) throw new S3Error(S3Error.AccessDenied);
 
-    const permission = await s3_bucket_policy_utils.has_bucket_policy_permission(
+    const permission = await access_policy_utils.has_access_policy_permission(
         s3_policy, undefined, method, arn_path, req,
         { disallow_public_access: public_access_block?.restrict_public_buckets }
     );
@@ -418,7 +417,7 @@ async function authorize_anonymous_access(s3_policy, method, arn_path, req, publ
  * @returns {string|string[]}
  */
 function _get_method_from_req(req) {
-    const s3_op = s3_bucket_policy_utils.OP_NAME_TO_ACTION[req.op_name];
+    const s3_op = access_policy_utils.OP_NAME_TO_ACTION[req.op_name];
     if (!s3_op) {
         dbg.error(`Got a not supported S3 op ${req.op_name} - doesn't suppose to happen`);
         throw new S3Error(S3Error.InternalError);

src/manage_nsfs/manage_nsfs_validations.js

@@ -9,7 +9,7 @@ const P = require('../util/promise');
 const string_utils = require('../util/string_utils');
 const native_fs_utils = require('../util/native_fs_utils');
 const ManageCLIError = require('../manage_nsfs/manage_nsfs_cli_errors').ManageCLIError;
-const bucket_policy_utils = require('../endpoint/s3/s3_bucket_policy_utils');
+const access_policy_utils = require('../util/access_policy_utils');
 const { throw_cli_error, get_options_from_file, get_boolean_or_string_value, get_bucket_owner_account_by_id,
     is_name_update, is_access_key_update } = require('../manage_nsfs/manage_nsfs_cli_utils');
 const { TYPES, ACTIONS, VALID_OPTIONS, OPTION_TYPE, FROM_FILE, BOOLEAN_STRING_VALUES, BOOLEAN_STRING_OPTIONS,
@@ -487,7 +487,7 @@ async function validate_bucket_args(config_fs, data, action) {
         }
         if (data.s3_policy) {
             try {
-                await bucket_policy_utils.validate_s3_policy(data.s3_policy, data.name,
+                await access_policy_utils.validate_bucket_policy(data.s3_policy, data.name,
                     async principal => config_fs.is_account_exists_by_principal(principal, { silent_if_missing: true })
                 );
             } catch (err) {

src/sdk/bucketspace_fs.js

@@ -29,7 +29,7 @@ const SensitiveString = require('../util/sensitive_string');
 const BucketSpaceSimpleFS = require('./bucketspace_simple_fs');
 const { account_id_cache } = require('../sdk/accountspace_fs');
 const nsfs_schema_utils = require('../manage_nsfs/nsfs_schema_utils');
-const bucket_policy_utils = require('../endpoint/s3/s3_bucket_policy_utils');
+const access_policy_utils = require('../util/access_policy_utils');
 const nc_mkm = require('../manage_nsfs/nc_master_key_manager').get_instance();
 const NoobaaEvent = require('../manage_nsfs/manage_nsfs_events_utils').NoobaaEvent;
 const native_fs_utils = require('../util/native_fs_utils');
@@ -766,15 +766,15 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
 
             if (
                 bucket.public_access_block?.block_public_policy &&
-                bucket_policy_utils.allows_public_access(policy)
+                access_policy_utils.allows_public_access(policy)
             ) {
                 throw new S3Error(S3Error.AccessDenied);
             }
 
             bucket.s3_policy = policy;
             // We need to validate bucket schema here as well for checking the policy schema
             nsfs_schema_utils.validate_bucket_schema(_.omitBy(bucket, _.isUndefined));
-            await bucket_policy_utils.validate_s3_policy(bucket.s3_policy, bucket.name, async principal =>
+            await access_policy_utils.validate_bucket_policy(bucket.s3_policy, bucket.name, async principal =>
                 this.config_fs.is_account_exists_by_principal(principal, { silent_if_missing: true }));
             await this.config_fs.update_bucket_config_file(bucket);
         } catch (err) {
@@ -1109,7 +1109,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
         }
 
         let permission_by_name;
-        const permission_by_id = await bucket_policy_utils.has_bucket_policy_permission(
+        const permission_by_id = await access_policy_utils.has_access_policy_permission(
             bucket_policy,
             account._id,
             action,
@@ -1121,7 +1121,7 @@ class BucketSpaceFS extends BucketSpaceSimpleFS {
         // we (currently) allow account identified to be both id and name,
         // so if by-id failed, try also name
         if (account.owner === undefined) {
-            permission_by_name = await bucket_policy_utils.has_bucket_policy_permission(
+            permission_by_name = await access_policy_utils.has_access_policy_permission(
                 bucket_policy,
                 account.name.unwrap(),
                 action,

src/server/common_services/auth_server.js

@@ -16,7 +16,7 @@ const kube_utils = require('../../util/kube_utils');
 const jwt_utils = require('../../util/jwt_utils');
 const config = require('../../../config');
 const iam_utils = require('../../endpoint/iam/iam_utils');
-const s3_bucket_policy_utils = require('../../endpoint/s3/s3_bucket_policy_utils');
+const access_policy_utils = require('../../util/access_policy_utils');
 
 
 /**
@@ -638,7 +638,7 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
     }
     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.toString());
-    const result = await s3_bucket_policy_utils.has_bucket_policy_permission(
+    const result = await access_policy_utils.has_access_policy_permission(
         bucket_policy,
         [arn, account._id.toString()],
         action,
@@ -653,10 +653,10 @@ async function has_bucket_action_permission(bucket, account, action, req_query,
     // If yes, IAM user also should get access
     if (account.owner) {
         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);
+        const owner_account_identifier_arn = access_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(
+        permission_by_arn_owner = await access_policy_utils.has_access_policy_permission(
             bucket_policy,
             [owner_account_identifier_arn, owner_account_id],
             action,
@@ -680,7 +680,7 @@ async function has_bucket_anonymous_permission(bucket, action, bucket_path, req_
     bucket_path = bucket_path ?? "";
     const bucket_policy = bucket.s3_policy;
     if (!bucket_policy) return false;
-    return await s3_bucket_policy_utils.has_bucket_policy_permission(
+    return await access_policy_utils.has_access_policy_permission(
         bucket_policy,
         // Account is anonymous
         undefined,

src/server/system_services/bucket_server.js

@@ -33,7 +33,7 @@ const azure_storage = require('../../util/azure_storage_wrap');
 const usage_aggregator = require('../bg_services/usage_aggregator');
 const chunk_config_utils = require('../utils/chunk_config_utils');
 const NetStorage = require('../../util/NetStorageKit-Node-master/lib/netstorage');
-const bucket_policy_utils = require('../../endpoint/s3/s3_bucket_policy_utils');
+const access_policy_utils = require('../../util/access_policy_utils');
 const path = require('path');
 const KeysSemaphore = require('../../util/keys_semaphore');
 const bucket_semaphore = new KeysSemaphore(1);
@@ -573,12 +573,12 @@ async function get_account_by_principal(principal) {
 async function put_bucket_policy(req) {
     dbg.log0('put_bucket_policy:', req.rpc_params);
     const bucket = find_bucket(req, req.rpc_params.name);
-    await bucket_policy_utils.validate_s3_policy(req.rpc_params.policy, bucket.name,
+    await access_policy_utils.validate_bucket_policy(req.rpc_params.policy, bucket.name,
         principal => get_account_by_principal(principal));
 
     if (
         bucket.public_access_block?.block_public_policy &&
-        bucket_policy_utils.allows_public_access(req.rpc_params.policy)
+        access_policy_utils.allows_public_access(req.rpc_params.policy)
     ) {
             // Should result in AccessDenied error
             throw new RpcError('UNAUTHORIZED');

src/test/integration_tests/api/s3/test_s3_bucket_policy.js

@@ -12,7 +12,7 @@ coretest.setup({ pools_to_create: process.env.NC_CORETEST ? undefined : [POOL_LI
 const path = require('path');
 const _ = require('lodash');
 const fs_utils = require('../../../../util/fs_utils');
-const s3_bucket_policy_utils = require('../../../../endpoint/s3/s3_bucket_policy_utils');
+const access_policy_utils = require('../../../../util/access_policy_utils');
 
 const { S3 } = require('@aws-sdk/client-s3');
 const { NodeHttpHandler } = require("@smithy/node-http-handler");
@@ -152,9 +152,9 @@ async function setup() {
         For coretest nc, principal will have account name and
         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(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());
+    admin_principal = is_nc_coretest ? EMAIL : access_policy_utils.create_arn_for_root(admin_info._id.toString());
+    a_principal = is_nc_coretest ? user_a : access_policy_utils.create_arn_for_root(user_a_account_details.id.toString());
+    b_principal = is_nc_coretest ? user_b : access_policy_utils.create_arn_for_root(user_b_account_details.id.toString());
 
     s3_owner = new S3(s3_creds);
     await s3_owner.createBucket({ Bucket: BKT });
@@ -394,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(user_a_account_id.toString());
+        a_principal = is_nc_coretest ? user_a : access_policy_utils.create_arn_for_root(user_a_account_id.toString());
         const deny_account_by_name_all_s3_actions_statement = {
             Sid: `Do not allow user ${user_a} any s3 action`,
             Effect: 'Deny',
@@ -2221,7 +2221,7 @@ mocha.describe('s3_bucket_policy', function() {
         mocha.it('should fail : Bucket policy with valid principal account ARN, try to putObject with different account', async function() {
             if (is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this
             this.timeout(5000); // eslint-disable-line no-invalid-this
-            const valid_arn_b = s3_bucket_policy_utils.create_arn_for_root(user_b_account_id);
+            const valid_arn_b = access_policy_utils.create_arn_for_root(user_b_account_id);
             const s3_policy = {
                 Version: '2012-10-17',
                 Statement: [
@@ -2251,7 +2251,7 @@ mocha.describe('s3_bucket_policy', function() {
         mocha.it('Bucket policy with valid principal account ARN', async function() {
             if (is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this
             this.timeout(5000); // eslint-disable-line no-invalid-this
-            const valid_arn = s3_bucket_policy_utils.create_arn_for_root(user_b_account_id);
+            const valid_arn = access_policy_utils.create_arn_for_root(user_b_account_id);
             const s3_policy = {
                 Version: '2012-10-17',
                 Statement: [
@@ -2276,7 +2276,7 @@ mocha.describe('s3_bucket_policy', function() {
         mocha.it('Bucket policy with valid principal ARN, and PutObject', async function() {
             if (is_nc_coretest) this.skip(); // eslint-disable-line no-invalid-this
             this.timeout(5000); // eslint-disable-line no-invalid-this
-            const valid_arn = s3_bucket_policy_utils.create_arn_for_root(user_b_account_id);
+            const valid_arn = access_policy_utils.create_arn_for_root(user_b_account_id);
             const s3_policy = {
                 Version: '2012-10-17',
                 Statement: [

src/upgrade/upgrade_scripts/5.15.6/upgrade_bucket_policy.js

@@ -2,7 +2,7 @@
 "use strict";
 
 const util = require('util');
-const { OP_NAME_TO_ACTION } = require('../../../endpoint/s3/s3_bucket_policy_utils');
+const { OP_NAME_TO_ACTION } = require('../../../util/access_policy_utils');
 const _ = require('lodash');