Account with NotPrincipal should explicitly provide permission #8782

vh05 · 2025-02-10T14:17:18Z

This is the bucket policy with below combination

    
    Effect : DENY,
    Action: $OPERATION,
    NotPrincipal: $ACCOUNT

The permission on the "Action" for the $ACCOUNT mentioned in the "NotPrincipal" should be explicitly given.

Example: For "Action: get_object", if we want to DENY permission for * (all accounts) and we want to give explicit permission to any one or few accounts, then we can include that account as part of part of "NotPrincipal"

Check DFBUGS-1519 for steps to reproduce the issue

Fixes: https://issues.redhat.com/browse/DFBUGS-1519

shirady · 2025-02-10T14:53:28Z

@vh05 please add tests in the file "test_s3_bucket_policy.js"

src/endpoint/s3/s3_rest.js

src/endpoint/s3/s3_bucket_policy_utils.js

shirady · 2025-02-10T15:03:15Z

@vh05 I would also start with testing on AWS the steps that are expected to happen before we change it our logs.

Please also add the full testing instructions that you manually did so we can understand what was done (you can add as comment if you prefer).

nadavMiz · 2025-02-19T08:56:15Z

@vh05 from what I understand this issue is not related to "deny statement using NotPrincipal". can you pleas change the PR name and description to reflect what you are actually doing here

vh05 · 2025-02-27T08:48:26Z

@vh05 from what I understand this issue is not related to "deny statement using NotPrincipal". can you pleas change the PR name and description to reflect what you are actually doing here

Sorry for the delayed response. I have updated the patch. Now the fix and the issue description matches I think

shirady

Added a couple of comments and questions.

src/test/unit_tests/test_s3_bucket_policy.js

shirady · 2025-02-27T13:59:50Z

src/test/unit_tests/test_s3_bucket_policy.js

+            Key: KEY,
+        });
+
+        await assert_throws_async(s3_a.putObject({


Why? I thought s3_a is the only user that can put object?

The account added in NotPrincipal is allowed on the Action. It is like excluding everyone other than account added as NotPrincipal

When you use assert_throws_async it is expected to throw an error.
As you mentioned the user_a is allowed on the action, then you probably need to run it without this function on s3_a and can run it on another user.

Yeah. That expression should be with user_b not user_a. My bad

shirady · 2025-02-27T14:01:05Z

src/test/unit_tests/test_s3_bucket_policy.js

What about fixing the test should be able to use notPrincipal it is in the scope of this PR?

NotPrincipal with Allow is not expected operation as per aws doc

https://docs.aws.amazon.com/IAM/latest/UserGuide/reference_policies_elements_notprincipal.html

from AWS:
NotPrincipal must be used with "Effect":"Deny". Using it with "Effect":"Allow" is not supported.

I agree, it is something that would be fixed in your suggested fix?

shirady · 2025-02-27T14:03:09Z

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') {


Why did you choose to have id as default?
the id is currently only in NC.
Please add JSDoc to this function or a comment about the options of account_identifier_type. I think using a enum can help us.

IIUC, the id is considered to solve issues regarding account name and thats the updated one. We are giving exception to name parameter.

I think using a enum can help us
I feel it is for default argument, string is fine

As you can see in the Jira ticket, it is not solving it completely.
I didn't understand the quote; was it not clear? I meant to add enum so it will be easier to understand the options that we can use.

src/endpoint/s3/s3_bucket_policy_utils.js

shirady · 2025-02-27T14:10:32Z

src/endpoint/s3/s3_bucket_policy_utils.js

+        // 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.


What if the allow is by ids and the deny is by names?
You can add it a test case.
What will happen when we support ARN, it will be more complex.

What if the allow is by ids and the deny is by names?

Is that test case required. IIUC the name is for backward compatibility and the fix almost like a workaround than a feature

The name is another option, we decided to not remove this to be backward compatible.
Anyway, it is a valid option, and in the containerized it is the only option.

This is the bucket policy with below combination ``` Effect : DENY, Action: $OPERATION, NotPrincipal: $ACCOUNT ``` The permission on the "Action" for the $ACCOUNT mentioned in the "NotPrincipal" should be explicitly given. Example: For "Action: get_object", if we want to DENY permission for * (all accounts) and we want to give explicit permission to any one or few accounts, then we can include that account as part of part of "NotPrincipal" Check DFBUGS-1519 for steps to reproduce the issue Fixes: https://issues.redhat.com/browse/DFBUGS-1519 Signed-off-by: Vinayakswami Hariharmath <[email protected]>

pull-request-size bot added the size/XS label Feb 10, 2025

vh05 force-pushed the DFBUGS-1519 branch from 3b28440 to 1103b0e Compare February 10, 2025 14:18

vh05 requested a review from shirady February 10, 2025 14:18

shirady reviewed Feb 10, 2025

View reviewed changes

src/endpoint/s3/s3_rest.js Outdated Show resolved Hide resolved

shirady reviewed Feb 10, 2025

View reviewed changes

src/endpoint/s3/s3_bucket_policy_utils.js Outdated Show resolved Hide resolved

vh05 force-pushed the DFBUGS-1519 branch from 1103b0e to c447d70 Compare February 11, 2025 10:17

pull-request-size bot added size/S and removed size/XS labels Feb 11, 2025

vh05 force-pushed the DFBUGS-1519 branch from c447d70 to 487fb06 Compare February 11, 2025 10:35

pull-request-size bot added size/M and removed size/S labels Feb 11, 2025

vh05 force-pushed the DFBUGS-1519 branch from 487fb06 to 2f8f16a Compare February 12, 2025 03:57

vh05 force-pushed the DFBUGS-1519 branch from 2f8f16a to 4fdf5a5 Compare February 27, 2025 08:46

vh05 force-pushed the DFBUGS-1519 branch from 4fdf5a5 to 22650de Compare February 27, 2025 09:02

vh05 requested a review from nadavMiz February 27, 2025 09:02

vh05 force-pushed the DFBUGS-1519 branch from 22650de to c85df24 Compare February 27, 2025 09:54

vh05 requested a review from shirady February 27, 2025 11:07

shirady reviewed Feb 27, 2025

View reviewed changes

vh05 force-pushed the DFBUGS-1519 branch 5 times, most recently from 48d71ef to 25191b9 Compare March 3, 2025 06:50

vh05 force-pushed the DFBUGS-1519 branch from 25191b9 to a42491f Compare April 3, 2025 08:27

vh05 changed the title ~~[MCG NC] Bucket policy with deny statement using NotPrincipal also denies the principal~~ Account with NotPrincipal should explicitly provide permission Apr 3, 2025

vh05 force-pushed the DFBUGS-1519 branch from a42491f to 2f8a64c Compare April 3, 2025 09:40

vh05 force-pushed the DFBUGS-1519 branch 3 times, most recently from c8a46a9 to e917402 Compare April 8, 2025 11:08

vh05 force-pushed the DFBUGS-1519 branch from e917402 to eab1c20 Compare April 21, 2025 10:07

pull-request-size bot added size/XS and removed size/M labels Apr 21, 2025

vh05 force-pushed the DFBUGS-1519 branch 3 times, most recently from 11caeca to 3d7a476 Compare April 22, 2025 11:45

vh05 force-pushed the DFBUGS-1519 branch from 3d7a476 to 34ee408 Compare April 22, 2025 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Account with NotPrincipal should explicitly provide permission #8782

Account with NotPrincipal should explicitly provide permission #8782

vh05 commented Feb 10, 2025 •

edited

Loading

shirady commented Feb 10, 2025

shirady commented Feb 10, 2025

nadavMiz commented Feb 19, 2025

vh05 commented Feb 27, 2025

shirady left a comment

shirady Feb 27, 2025

vh05 Feb 28, 2025

shirady Mar 2, 2025

vh05 Mar 3, 2025

shirady Feb 27, 2025

vh05 Feb 28, 2025

shirady Mar 2, 2025

shirady Feb 27, 2025

vh05 Feb 28, 2025

shirady Mar 2, 2025

shirady Feb 27, 2025

vh05 Feb 28, 2025

shirady Mar 2, 2025

Account with NotPrincipal should explicitly provide permission #8782

Are you sure you want to change the base?

Account with NotPrincipal should explicitly provide permission #8782

Conversation

vh05 commented Feb 10, 2025 • edited Loading

shirady commented Feb 10, 2025

shirady commented Feb 10, 2025

nadavMiz commented Feb 19, 2025

vh05 commented Feb 27, 2025

shirady left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vh05 commented Feb 10, 2025 •

edited

Loading