IAM | Authorize Requests for IAM Users According to IAM User Inline Policy #9281
Conversation
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant Req as S3 Request
participant Auth as authorize_request
participant Acc as account policy check
participant BP as bucket policy check
participant IAM as IAM user policy check
participant Utils as bucket policy utils
Req->>Auth: incoming S3 operation
Auth->>Acc: run account-level check
Auth->>BP: run bucket policy check
Auth->>IAM: run IAM user inline policy check (new, parallel)
alt IAM user inline policy present
IAM->>Utils: evaluate iam_user_policies (passes {disallow_public_access, should_pass_principal})
Utils-->>IAM: DENY / ALLOW / none
alt DENY
IAM-->>Auth: DENY (short-circuit)
else ALLOW
IAM-->>Auth: ALLOW (can permit)
else none
IAM-->>Auth: no decision
end
else no IAM user policies or anonymous
IAM-->>Auth: skipped
end
alt any DENY
Auth-->>Req: Access Denied
else any ALLOW and no DENY
Auth-->>Req: Access Granted
else no decision
Auth-->>Req: Access Denied (default)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-08-08T13:08:38.361ZApplied to files:
📚 Learning: 2025-11-12T04:55:42.193ZApplied to files:
📚 Learning: 2025-11-13T07:56:23.620ZApplied to files:
🧬 Code graph analysis (1)src/endpoint/s3/s3_bucket_policy_utils.js (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/api/account_api.js(1 hunks)src/endpoint/s3/s3_bucket_policy_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(2 hunks)src/server/system_services/account_server.js(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T04:55:42.171Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.171Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/endpoint/s3/s3_rest.jssrc/server/system_services/account_server.jssrc/api/account_api.js
🧬 Code graph analysis (3)
src/endpoint/s3/s3_rest.js (3)
src/endpoint/s3/s3_bucket_policy_utils.js (2)
account(283-283)dbg(5-5)src/endpoint/iam/iam_rest.js (2)
method(217-217)dbg(4-4)src/endpoint/sts/sts_rest.js (3)
method(137-137)method(165-165)dbg(5-5)
src/server/system_services/account_server.js (2)
src/util/account_util.js (6)
account(32-44)account(198-198)account(306-306)account(331-331)account(370-370)account(645-645)src/server/system_services/bucket_server.js (1)
info(1483-1530)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
src/endpoint/s3/s3_rest.js (10)
account(256-256)account(322-322)method(248-248)method(327-327)method(451-451)arn_path(247-247)arn_path(383-383)req(238-244)req(395-395)_(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
- GitHub Check: run-package-lock-validation
shirady
force-pushed
the
iam-user-policy-rest-s3
branch
from
a1d02e4 to
48d50d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/endpoint/s3/s3_rest.js (1)
221-231: IAM inline policy hook and evaluation logic look sound; consider minor refinements
- Adding
authorize_request_iam_policy(req)into the existingPromise.all([...])preserves the “all checks must pass” semantics and keepsload_requesting_account()serialized ahead of all checks, which is good.- In
authorize_request_iam_policythe short‑circuiting for anonymous requests, non‑IAM accounts (account.owner === undefined), and missing/emptyiam_user_policiescorrectly preserves the previous behavior when no IAM user policy is present.- Reusing
has_bucket_policy_permissionwithshould_pass_principal = falseis a reasonable way to leverage existing evaluation logic for IAM inline (identity‑based) policies that lack aPrincipalelement.- The DENY/ALLOW reduction over
permission_resultmatches IAM semantics (any explicit DENY wins; any ALLOW with no DENY allows). The current loop is fine; if you want, you could micro‑optimize by returning immediately upon the first"DENY"and short‑circuiting oncehas_allow_permissionis true and no later"DENY"is possible, but this is purely optional.One small nit: the debug log
"user have inline policies..."is slightly misleading because non‑matching could be due to resource or conditions, not only “method”; if you touch this again, consider including the action and ARN or rephrasing the message, but it’s not a blocker.Also applies to: 321-355
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/api/account_api.js(1 hunks)src/endpoint/s3/s3_bucket_policy_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(2 hunks)src/server/system_services/account_server.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/api/account_api.js
- src/server/system_services/account_server.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/endpoint/s3/s3_rest.js
🧬 Code graph analysis (1)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
src/endpoint/s3/s3_rest.js (10)
account(256-256)account(326-326)method(248-248)method(331-331)method(455-455)arn_path(247-247)arn_path(387-387)req(238-244)req(399-399)_(4-4)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (1)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
152-171: All call sites are backward compatible; no issues foundVerification confirms that all eight call sites of
has_bucket_policy_permissionremain fully compatible:
- Five call sites pass 5 arguments, relying on both new defaults
- Three additional call sites pass 6 arguments (the existing
disallow_public_access), relying on the newshould_pass_principaldefault- One call site (s3_rest.js:339) explicitly passes 7 arguments, correctly passing
should_pass_principal=falsefor the IAM inline-policy pathThe parameter signature extension preserves backward compatibility and is correctly wired throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
238-238: Consider adding a clarifying comment.The conditional principal check correctly bypasses validation when
should_pass_principalis false, which is used for IAM user inline policies that don't have a Principal field. However, a brief comment explaining this use case would improve code clarity for future maintainers.Example:
+ // 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 the policy is attached to) const principal_fit = should_pass_principal ? _is_principal_fit(account, statement, disallow_public_access) : true;src/endpoint/s3/s3_rest.js (1)
341-341: Fix minor grammar issue in comment.Apply this diff:
- // We reusing the bucket policy util function as it is checks the policy document + // We are reusing the bucket policy util function as it checks the policy document
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/endpoint/s3/s3_bucket_policy_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(3 hunks)src/sdk/bucketspace_fs.js(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-12T04:55:42.193Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_rest.js:258-261
Timestamp: 2025-11-12T04:55:42.193Z
Learning: In the context of S3 REST requests (src/endpoint/s3/s3_rest.js), the account.owner field from req.object_sdk.requesting_account is already a string (account ID) because it comes from RPC serialization where owner._id.toString() is applied in account_server.js. No additional .toString() or ._id extraction is needed when passing account.owner to IAM utility functions.
Applied to files:
src/endpoint/s3/s3_rest.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: run-package-lock-validation
- GitHub Check: Build Noobaa Image
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (5)
src/sdk/bucketspace_fs.js (1)
954-954: LGTM!The updated calls correctly pass the
disallow_public_accessflag wrapped in an options object, aligning with the extendedhas_bucket_policy_permissionsignature that now accepts an options parameter.Also applies to: 966-966
src/endpoint/s3/s3_bucket_policy_utils.js (1)
152-153: LGTM!The options object approach with default parameter values maintains backwards compatibility while supporting the new IAM policy evaluation flow. Setting
disallow_public_access: falsespecifically for DENY statement evaluation (line 162) is appropriate since public access restrictions aren't needed when evaluating denials.Also applies to: 161-164, 170-173, 228-229, 231-231, 234-235
src/endpoint/s3/s3_rest.js (3)
227-230: LGTM! Clear parallel authorization flow.The addition of IAM policy checks alongside bucket policy checks is well-structured. The comments clearly distinguish that bucket policy checks affect owners (with explicit DENY overrides) while IAM policy checks apply only to IAM users.
304-306: LGTM! Consistent refactoring to options object.The change from passing a boolean
disallow_public_accessparameter to an options object{ disallow_public_access: ... }is applied consistently across all call sites and improves extensibility for additional options.Also applies to: 313-314, 365-367
324-359: LGTM! Solid IAM policy evaluation logic.The implementation correctly:
- Filters for authenticated IAM users only
- Preserves default behavior when no IAM policies exist (line 336 early return)
- Evaluates policies in parallel for performance
- Enforces explicit DENY precedence (immediate failure on line 351)
- Requires at least one ALLOW to succeed
- Handles IMPLICIT DENY correctly (no ALLOW → AccessDenied)
The reuse of
s3_bucket_policy_utils.has_bucket_policy_permissionwith{ should_pass_principal: false }is appropriate for IAM inline policies that don't contain a Principal element. Theshould_pass_principalparameter correctly skips principal validation when set to false (line 238 of s3_bucket_policy_utils.js), which is the intended behavior for IAM inline policies.
…olicy Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
shirady
force-pushed
the
iam-user-policy-rest-s3
branch
from
cd19105 to
55850a2
Compare
3 participants
Describe the Problem
Add a step in
s3_restto authorize requests for IAM users according to the IAM user inline policy.Explain the Changes
authorize_request_iam_policyand call it fromauthorize_request.account_info(ownerandiam_user_policies).Issues:
List of GAPs:
s3_bucket_policy_utils.jsso the terms will be policy in general and can be used for bucket policy and IAM policy.iam_rest) so the thrown error will beamError.AccessDeniedExceptioninstead ofS3Error.AccessDeniedDesign Decisions:
DENYthrows an error), but I decided to do it in parallel, assuming that the total user policy is limited. If I were to do it sequentially, I would need to wait for every policy check, instead of running all the checks and waiting for all the promises to be resolved and work on the result (DENY,ALLOW,IMPLICIT_DENY). I also decided to check in parallel to the bucket policy, and I had the option to do it after, as in the case where there is an IAM user policy withDENY, we would like it to be thrown as part of the calculation.Testing Instructions:
Note:
nbis an alias that runs the local operator frombuild/_output/bin(alias created bydevenv).kubectl wait --for=condition=available backingstore/noobaa-default-backing-store --timeout=6m -n test1kubectl port-forward -n test1 service/s3 12443:443kubectl port-forward -n test1 service/iam 14443:443nb status --show-secrets -n test1and then:alias admin-s3='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:12443'alias admin-iam='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:14443'admin-iam iam list-users; echo $?first.bucket):admin-iam s3 ls; echo $?admin-iam iam create-user --user-name RobertNote: To validate user creation, you can rerun
admin-iam iam list-usersand expect 1 user in the listadmin-iam iam create-access-keys --user-name Robertuser-2-s3).user-2-s3 s3 mb s3://mybucket(should work).echo 'test_data' | user-2-s3 s3 cp - s3://first.bucket/test_object.txt(should work).policy_deny_create_bucket_allow_put_object.json
{ "Version": "2012-10-17", "Statement": [ { "Effect": "Deny", "Action": [ "s3:CreateBucket" ], "Resource": "*" }, { "Effect": "Allow", "Action": [ "s3:PutObject" ], "Resource": "*" } ] }admin-iam iam put-user-policy --user-name Robert --policy-name policy_deny_create_bucket_allow_put_object --policy-document file://policy_deny_create_bucket_allow_put_object.jsonecho 'test_data' | user-2-s3 s3 cp - s3://first.bucket/test_object2.txt(should work).user-2-s3 mb mybucket2(should throwAccessDeniederror).Code changes for testing:
src/sdk/object_sdk.jsuses cache expiry of 1 millisecond.const account_cache = new LRUCache({ name: 'AccountCache', - expiry_ms: config.OBJECT_SDK_ACCOUNT_CACHE_EXPIRY_MS, + expiry_ms: 1, //SDSDsrc/endpoint/s3/s3_rest.jsin theif (!s3_policy) {remove the error thrown as currentlyowner_accountin NC deployment and is undefined. rest of the code inelsecondition.Notes:
In step 1 - deploying the system, I used
--use-standalone-dbfor simplicity (fewer steps for the system in Ready status).I used the admin account, but for IAM operations, there is no difference between a created account and an admin account. I tested with the admin only because it saves steps (I have the admin account upon system creation and a bucket).
Doc added/updated
Tests added
Summary by CodeRabbit