From 9c7078801f9e87cf3fc291956d47c84ea919e4d3 Mon Sep 17 00:00:00 2001 From: Vinayakswami Hariharmath Date: Mon, 10 Feb 2025 19:40:07 +0530 Subject: [PATCH] [MCG NC] Bucket policy with deny statement using NotPrincipal also denies the principal This is the put-bucket-policy case for combination of Effect : DENY, Action: $OPERATION, NotPrincipal: $ACCOUNT The $ACCOUNT mentioned in the "NotPrincipal" should be excluded from DENy operation and should be allowed on the $OPERATION Example: For operation get_object, if we have DENY effect for * (all accounts) and we want to give access to any one or few accounts, then that account can be part of "NotPrincipal" Fixes: https://issues.redhat.com/browse/DFBUGS-1519 Signed-off-by: Vinayakswami Hariharmath --- src/endpoint/s3/s3_bucket_policy_utils.js | 48 ++++++++++++++++---- src/endpoint/s3/s3_rest.js | 3 +- src/sdk/bucketspace_fs.js | 4 +- src/test/unit_tests/test_s3_bucket_policy.js | 31 +++++++++++++ 4 files changed, 74 insertions(+), 12 deletions(-) diff --git a/src/endpoint/s3/s3_bucket_policy_utils.js b/src/endpoint/s3/s3_bucket_policy_utils.js index 6255159ccb..d8b7df3714 100644 --- a/src/endpoint/s3/s3_bucket_policy_utils.js +++ b/src/endpoint/s3/s3_bucket_policy_utils.js @@ -146,18 +146,31 @@ async function _is_object_version_fit(req, predicate, value) { return res; } -async function has_bucket_policy_permission(policy, account, method, arn_path, req) { +async function has_bucket_policy_permission(policy, account, method, arn_path, req, account_identifier_type = 'id') { 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]; // look for explicit denies - const res_arr_deny = await is_statement_fit_of_method_array(deny_statements, account, method_arr, arn_path, req); + const res_arr_deny = await is_statement_fit_of_method_array(deny_statements, + account, + method_arr, + arn_path, + req, + account_identifier_type + ); if (res_arr_deny.every(item => item)) return 'DENY'; // look for explicit allows - const res_arr_allow = await is_statement_fit_of_method_array(allow_statements, account, method_arr, arn_path, req); + const res_arr_allow = await is_statement_fit_of_method_array( + allow_statements, + account, + method_arr, + arn_path, + req, + account_identifier_type + ); if (res_arr_allow.every(item => item)) return 'ALLOW'; // implicit deny @@ -177,9 +190,8 @@ function _is_action_fit(method, statement) { return statement.Action ? action_fit : !action_fit; } -function _is_principal_fit(account, statement) { +function _is_principal_fit(account, statement, account_identifier_type) { 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])) { @@ -189,7 +201,23 @@ function _is_principal_fit(account, statement) { break; } } - return statement.Principal ? principal_fit : !principal_fit; + + if (statement.NotPrincipal) { + // If the account is categorized as "Not Principal," we need to ALLOW operations + // for that account under the associated principal. + // + // To maintain backward compatibility, we also support name-based comparison for principal matching. + // + // - If `account_identifier_type` is "id" but the `account` parameter contains a name, + // we return `false` to proceed/bypass to the next step, where we check for a name match. + // + // - In the next step, if `account_identifier_type` is "name" and the `account` also contains a name, + // we return the actual value. + + return account_identifier_type === 'id' ? principal_fit : !principal_fit; + } + + return principal_fit; } function _is_resource_fit(arn_path, statement) { @@ -207,15 +235,15 @@ 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, method_arr, arn_path, req, account_identifier_type) { return Promise.all(method_arr.map(method_permission => - _is_statements_fit(statements, account, method_permission, arn_path, req))); + _is_statements_fit(statements, account, method_permission, arn_path, req, account_identifier_type))); } -async function _is_statements_fit(statements, account, method, arn_path, req) { +async function _is_statements_fit(statements, account, method, arn_path, req, account_identifier_type) { for (const statement of statements) { const action_fit = _is_action_fit(method, statement); - const principal_fit = _is_principal_fit(account, statement); + const principal_fit = _is_principal_fit(account, statement, account_identifier_type); const resource_fit = _is_resource_fit(arn_path, statement); const condition_fit = await _is_condition_fit(statement, req, method); diff --git a/src/endpoint/s3/s3_rest.js b/src/endpoint/s3/s3_rest.js index cb9544c9c1..09ab752528 100755 --- a/src/endpoint/s3/s3_rest.js +++ b/src/endpoint/s3/s3_rest.js @@ -293,8 +293,9 @@ async function authorize_request_policy(req) { if (permission_by_id === "DENY") throw new S3Error(S3Error.AccessDenied); if ((!account_identifier_id || permission_by_id !== "DENY") && account.owner === undefined) { + const account_identifier_type = 'name'; permission_by_name = await s3_bucket_policy_utils.has_bucket_policy_permission( - s3_policy, account_identifier_name, method, arn_path, req); + s3_policy, account_identifier_name, method, arn_path, req, account_identifier_type); dbg.log3('authorize_request_policy: permission_by_name', permission_by_name); } if (permission_by_name === "DENY") throw new S3Error(S3Error.AccessDenied); diff --git a/src/sdk/bucketspace_fs.js b/src/sdk/bucketspace_fs.js index da0a33419d..63f6850d9f 100644 --- a/src/sdk/bucketspace_fs.js +++ b/src/sdk/bucketspace_fs.js @@ -819,12 +819,14 @@ 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) { + const account_identifier_type = 'name'; permission_by_name = await bucket_policy_utils.has_bucket_policy_permission( bucket_policy, account.name.unwrap(), action, `arn:aws:s3:::${bucket.name.unwrap()}${bucket_path}`, - undefined + undefined, + account_identifier_type ); } if (permission_by_name === 'DENY') return false; diff --git a/src/test/unit_tests/test_s3_bucket_policy.js b/src/test/unit_tests/test_s3_bucket_policy.js index 580f62c245..e97c1f3077 100644 --- a/src/test/unit_tests/test_s3_bucket_policy.js +++ b/src/test/unit_tests/test_s3_bucket_policy.js @@ -1647,6 +1647,37 @@ mocha.describe('s3_bucket_policy', function() { })); }); + mocha.it('should be able to use notPrincipal with Effect Deny', async function() { + const self = this; // eslint-disable-line no-invalid-this + self.timeout(15000); + const auth_put_policy = { + Version: '2012-10-17', + Statement: [ + { + Effect: 'Deny', + NotPrincipal: { AWS: user_a }, + Action: ['s3:PutObject'], + Resource: [`arn:aws:s3:::${BKT}/*`] + } + ]}; + await s3_owner.putBucketPolicy({ + Bucket: BKT, + Policy: JSON.stringify(auth_put_policy) + }); + + await s3_a.putObject({ + Body: BODY, + Bucket: BKT, + Key: KEY, + }); + + await assert_throws_async(s3_b.putObject({ + Body: BODY, + Bucket: BKT, + Key: KEY, + })); + }); + mocha.it('should be able to use notResource', async function() { const self = this; // eslint-disable-line no-invalid-this self.timeout(15000);