IAM | Change Default Behavior of Users Without IAM User Policy #9302
Conversation
WalkthroughDocs updated to require IAM user policies for S3 operations and reflect a reordering of authorization layers. S3 request authorization now runs sequentially: account → IAM user policy → bucket policy; IAM policy check is skipped only when no iam_policies and nsfs_config_root is set. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as S3 Client
participant Handler as s3_rest / authorize_request
participant Account as authorize_request_account
participant IAMPolicy as authorize_request_iam_policy
participant BucketPolicy as authorize_request_policy
Client->>Handler: HTTP S3 request
Note right of Handler: Sequential authorization flow
Handler->>Account: await authorize_request_account(req)
Account-->>Handler: account auth result
Handler->>IAMPolicy: await authorize_request_iam_policy(req)
alt iam_policies empty AND nsfs_config_root set
IAMPolicy-->>Handler: skipped (no IAM check)
else
IAMPolicy-->>Handler: IAM policy decision (allow/deny)
end
Handler->>BucketPolicy: await authorize_request_policy(req)
BucketPolicy-->>Handler: bucket policy decision
Handler-->>Client: final authorization result (allow/deny)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
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: 0
🧹 Nitpick comments (1)
docs/design/IamUserInlinePolicy.md (1)
7-7: Consider more concise phrasing.The phrase "in order to" can be simplified to "to" for improved readability.
Apply this diff:
-User must have IAM policy in order to be authorized for S3 operations. +User must have IAM policy to be authorized for S3 operations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/design/IamUserInlinePolicy.md(2 hunks)src/endpoint/s3/s3_rest.js(3 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
🪛 LanguageTool
docs/design/IamUserInlinePolicy.md
[style] ~7-~7: Consider a more concise word here.
Context: ...M User Policy User must have IAM policy in order to be authorized for S3 operations. ## Us...
(IN_ORDER_TO_PREMIUM)
⏰ 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-jest-unit-tests
- GitHub Check: run-package-lock-validation
- GitHub Check: Build Noobaa Image
🔇 Additional comments (4)
docs/design/IamUserInlinePolicy.md (1)
32-33: LGTM!The reordering of authorization layers correctly reflects the sequential authorization flow implemented in the code changes, where IAM user policy is now checked before bucket policy.
src/endpoint/s3/s3_rest.js (3)
223-227: LGTM! Sequential authorization enforces proper policy precedence.The change from parallel to sequential execution is appropriate for authorization checks. This ensures IAM user policies are evaluated before bucket policies, which aligns with the documented authorization layers and provides fail-fast behavior when policies deny access.
336-336: TODO noted for future refactoring.The TODO comment correctly identifies future work to move this function and use IAM-specific error types. This aligns with the known gaps mentioned in the PR objectives.
349-349: Code change verified as correct.The condition at line 349 properly uses
req.object_sdk.nsfs_config_rootas the discriminator between NC and containerized deployments. The codebase consistently applies this pattern:nsfs_config_rootis truthy in NC/NSFS deployments and falsy in containerized deployments.The logic correctly implements the intended behavior:
- NC deployments skip IAM policy validation (returning early when policies are empty)
- Containerized deployments enforce IAM policies (proceeding to access denial logic when policies are empty)
This change maintains backward compatibility for existing NC deployments while adding proper IAM policy enforcement to containerized deployments.
shirady
force-pushed
the
iam-user-policy-default-behavior
branch
from
e986557 to
c92a0ba
Compare
shirady
changed the title
shirady
force-pushed
the
iam-user-policy-default-behavior
branch
from
c92a0ba to
13b9f15
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/endpoint/s3/s3_rest.js (1)
336-372: IAM vs NC behavior is correct; consider tightening the “no policies” loggingThe IAM check now does what you want:
- IAM users in containerised mode with no
iam_user_policiesgetAccessDeniedhere, before bucket policy.- NC (
nsfs_config_roottruthy) still bypasses IAM wheniam_policies.length === 0, preserving existing behavior while IAM isn’t supported there.One small nit: when
iam_policies.length === 0in containerised mode, we still log “user have inline policies but none of them matched the method”, which is misleading. You could make the zero‑policy case explicit without changing semantics, e.g.:- const iam_policies = account.iam_user_policies || []; - if (iam_policies.length === 0 && req.object_sdk.nsfs_config_root) return; // We do not have IAM policies in NC yet + const iam_policies = account.iam_user_policies || []; + if (iam_policies.length === 0) { + if (req.object_sdk.nsfs_config_root) { + // We do not have IAM policies in NC yet + return; + } + dbg.log1('authorize_request_iam_policy: IAM user has no inline policies configured'); + throw new S3Error(S3Error.AccessDenied); + } @@ - if (has_allow_permission) return; - dbg.log1('authorize_request_iam_policy: user have inline policies but none of them matched the method'); - throw new S3Error(S3Error.AccessDenied); + if (has_allow_permission) return; + dbg.log1('authorize_request_iam_policy: user has inline policies but none of them matched the method'); + throw new S3Error(S3Error.AccessDenied);This keeps NC semantics intact, keeps the deny behavior for containerised users without policies, and makes logs more accurate.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/design/IamUserInlinePolicy.md(2 hunks)src/endpoint/s3/s3_rest.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/design/IamUserInlinePolicy.md
🧰 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 (1)
src/endpoint/s3/s3_rest.js (1)
221-227: Layered authorization flow (account → IAM → bucket) looks consistent with the new semanticsThe new sequencing (
load_requesting_account→authorize_request_account→authorize_request_iam_policy→authorize_request_policy) correctly enforces IAM user checks before bucket policy, while still allowing root/anon paths to behave as before and keeping bucket policy as a final ownership/explicit‑DENY layer. This matches the PR’s goal for containerised deployments without introducing obvious regressions.
Signed-off-by: shirady <[email protected]>
shirady
force-pushed
the
iam-user-policy-default-behavior
branch
from
13b9f15 to
39ed7aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/endpoint/s3/s3_rest.js (4)
218-225: Sequential authorization changes request flow.The authorization flow was changed from parallel to sequential execution. While this ensures the correct evaluation order (account → IAM user policy → bucket policy), it means every request now waits for the IAM policy check to complete before bucket policy evaluation begins.
Consider monitoring the latency impact of this change in production. If the IAM policy check becomes a bottleneck, you may want to optimize policy evaluation or implement caching strategies.
346-346: Verify the early-exit condition is explicit and clear.The condition
req.object_sdk.nsfs_config_rootshould be explicitly checked for boolean truthiness to avoid potential issues with falsy values like empty strings.Apply this diff to make the check more explicit:
- if (iam_policies.length === 0 && req.object_sdk.nsfs_config_root) return; // We do not have IAM policies in NC yet + if (iam_policies.length === 0 && Boolean(req.object_sdk.nsfs_config_root)) return; // We do not have IAM policies in NC yetAlternatively, if
nsfs_config_rootis always expected to be a string path or undefined, consider:- if (iam_policies.length === 0 && req.object_sdk.nsfs_config_root) return; // We do not have IAM policies in NC yet + if (iam_policies.length === 0 && req.object_sdk.nsfs_config_root !== undefined) return; // We do not have IAM policies in NC yet
366-368: Improve error context for IAM policy denial.The error message on line 367 and the thrown error on line 368 don't clearly distinguish between two different denial scenarios:
- User has NO IAM policies (in containerized deployments)
- User has IAM policies but none allow the requested action
This makes debugging and user feedback less clear.
Apply this diff to provide clearer error messages:
- if (has_allow_permission) return; - dbg.log1('authorize_request_iam_policy: user have inline policies but none of them matched the method'); - throw new S3Error(S3Error.AccessDenied); + if (has_allow_permission) return; + + if (iam_policies.length === 0) { + dbg.log1('authorize_request_iam_policy: IAM user must have at least one user policy in containerized deployments'); + } else { + dbg.log1('authorize_request_iam_policy: user has inline policies but none allow the requested action'); + } + throw new S3Error(S3Error.AccessDenied);
333-334: Address the TODO comment before finalizing this feature.The TODO mentions moving this function and improving error messages. Given this is a significant security change affecting user experience, addressing these improvements should be prioritized.
The suggested improvements in the TODO are valid:
- Moving to
iam_restwould allow returningamError.AccessDeniedExceptioninstead ofS3Error.AccessDenied, providing more appropriate IAM-specific error details.- Enhanced error messages would help users understand why their requests are denied.
Do you want me to open an issue to track these improvements, or would you prefer to address them in this PR?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/design/IamUserInlinePolicy.md(2 hunks)src/endpoint/s3/s3_rest.js(3 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: Build Noobaa Image
- GitHub Check: run-package-lock-validation
- GitHub Check: run-jest-unit-tests
🔇 Additional comments (1)
docs/design/IamUserInlinePolicy.md (1)
28-36: LGTM!The authorization layer description accurately reflects the sequential flow implemented in the code changes.
2 participants
Describe the Problem
Currently in Containerised NooBaa deployment, when a user is created under an account (and has access keys), they can perform all S3 operations. Since we added the IAM User Policy APIs, we want to change the default so the user must have a policy for any S3 operations.
Explain the Changes
authorize_requestins3_rest.jsso that the IAM policy check will be afterauthorize_request_accountand beforeauthorize_request_policy.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.AccessDeniedTesting 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-1-s3):alias user-1-s3='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:12443'user-1-s3 s3 ls s3://first.bucket(should not work - should throwAccessDeniederror).policy_allow_list_bucket.json
{ "Version": "2012-10-17", "Statement": [ { "Effect": "Allow", "Action": [ "s3:ListBucket" ], "Resource": "*" } ] }admin-iam iam put-user-policy --user-name Robert --policy-name policy_allow_list_bucket --policy-document file://policy_allow_list_bucketjsonuser-1-s3 s3 ls s3://first.bucket(should work).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, //SDSDNotes:
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
first.bucket).Doc added/updated
Tests added
Summary by CodeRabbit
Documentation
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.