IAM | User ID for principal is not supported #9329
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughGuard added preventing non-ARN principal lookups from treating an account-with-owner as a valid principal; new helper selects deployment-aware account identifier for bucket policy checks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 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: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/server/system_services/bucket_server.js(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
📚 Learning: 2025-11-19T15:03:42.260Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.260Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.
Applied to files:
src/server/system_services/bucket_server.js
📚 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/server/system_services/bucket_server.js
📚 Learning: 2025-11-18T07:00:17.653Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.653Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
Applied to files:
src/server/system_services/bucket_server.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/server/system_services/bucket_server.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: run-jest-unit-tests
- GitHub Check: Build Noobaa Image
naveenpaul1
force-pushed
the
iam_user_id_deny
branch
from
774f343 to
2ffe3ae
Compare
Contributor
|
@naveenpaul1, |
naveenpaul1
force-pushed
the
iam_user_id_deny
branch
from
2ffe3ae to
96dedf1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
380-391: Simplify boolean logic and fix typo.The conditional logic can be simplified, and there's a typo in the comment.
Apply this diff:
/** - * Both NSFS NC and containerized will validate bucket policy against acccount id - * but in containerized deplyment not against IAM user ID. + * Both NSFS NC and containerized will validate bucket policy against account id + * but in containerized deployment not against IAM user ID. * * @param {boolean} is_nc_deployment * @param {object} account */ function get_account_identifier_id(is_nc_deployment, account) { - if (is_nc_deployment || (!is_nc_deployment && account.owner === undefined)) { + if (is_nc_deployment || account.owner === undefined) { return account._id; } }
📜 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(1 hunks)src/server/system_services/bucket_server.js(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
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.
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.653Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.260Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.
📚 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.jssrc/endpoint/s3/s3_bucket_policy_utils.jssrc/server/system_services/bucket_server.js
📚 Learning: 2025-11-18T07:00:17.653Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.653Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
Applied to files:
src/endpoint/s3/s3_rest.jssrc/endpoint/s3/s3_bucket_policy_utils.jssrc/server/system_services/bucket_server.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
Applied to files:
src/endpoint/s3/s3_rest.jssrc/server/system_services/bucket_server.js
📚 Learning: 2025-11-19T15:03:42.260Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.260Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.
Applied to files:
src/endpoint/s3/s3_rest.jssrc/server/system_services/bucket_server.js
🧬 Code graph analysis (1)
src/endpoint/s3/s3_bucket_policy_utils.js (3)
src/endpoint/s3/s3_rest.js (3)
is_nc_deployment(253-253)account(252-252)account(358-358)src/server/system_services/bucket_server.js (1)
account(561-561)src/util/account_util.js (2)
account(30-42)account(196-196)
⏰ 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
🔇 Additional comments (3)
src/endpoint/s3/s3_bucket_policy_utils.js (1)
432-432: LGTM!The export of the new helper function is correct.
src/server/system_services/bucket_server.js (1)
562-566: LGTM!The guard against IAM user principals identified by ID is correct. The null check for
accountprevents runtime errors, and theaccount.ownercheck properly identifies IAM users. This successfully blocks IAM user IDs from being used as principals in bucket policies.Based on learnings, in bucket_server.js,
account.owneris an object ID reference (not a string), so the truthy check correctly identifies IAM users.src/endpoint/s3/s3_rest.js (1)
255-257: Fix typos in comment.Apply this diff:
- // Both NSFS NC and containerized will validate bucket policy against acccount id - // but in containerized deplyment not against IAM user ID. + // Both NSFS NC and containerized will validate bucket policy against account id + // but in containerized deployment not against IAM user ID. const account_identifier_id = s3_bucket_policy_utils.get_account_identifier_id(is_nc_deployment, account);The comment typos ("acccount" and "deplyment") should be corrected. Regarding the consistency concern about IAM user ID validation between this file and
bucket_server.js: verify this is intentional behavior for NC backward compatibility, as the current implementation allows policy checks against IAM user IDs whilebucket_server.jsblocks them as principals.
shirady
approved these changes
Dec 3, 2025
naveenpaul1
force-pushed
the
iam_user_id_deny
branch
from
96dedf1 to
f242358
Compare
Signed-off-by: Naveen Paul <napaul@redhat.com>
naveenpaul1
force-pushed
the
iam_user_id_deny
branch
from
f242358 to
4585e22
Compare
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 (3)
src/endpoint/s3/s3_bucket_policy_utils.js(2 hunks)src/endpoint/s3/s3_rest.js(1 hunks)src/server/system_services/bucket_server.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/endpoint/s3/s3_bucket_policy_utils.js
- src/server/system_services/bucket_server.js
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.653Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
📚 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
📚 Learning: 2025-11-18T07:00:17.653Z
Learnt from: naveenpaul1
Repo: noobaa/noobaa-core PR: 9277
File: src/endpoint/s3/s3_bucket_policy_utils.js:357-368
Timestamp: 2025-11-18T07:00:17.653Z
Learning: In NooBaa codebase, account.name is always a SensitiveString instance, so calling account.name.unwrap() is safe without defensive type checks in functions like get_bucket_policy_principal_arn in src/endpoint/s3/s3_bucket_policy_utils.js.
Applied to files:
src/endpoint/s3/s3_rest.js
📚 Learning: 2025-11-19T15:03:42.260Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9291
File: src/server/common_services/auth_server.js:548-554
Timestamp: 2025-11-19T15:03:42.260Z
Learning: In src/server/common_services/auth_server.js, account objects are loaded directly from system_store (e.g., system_store.data.get_by_id()), so account.owner is an object ID reference with an ._id property, not a string. This differs from s3_rest.js where account.owner is a string due to RPC serialization.
Applied to files:
src/endpoint/s3/s3_rest.js
📚 Learning: 2025-11-13T07:56:23.620Z
Learnt from: shirady
Repo: noobaa/noobaa-core PR: 9281
File: src/server/system_services/account_server.js:1053-1058
Timestamp: 2025-11-13T07:56:23.620Z
Learning: In noobaa-core, account_server.js is only used in containerized deployments, not in NSFS/NC deployments. NSFS/NC deployments have separate account management code in src/manage_nsfs/ directory. Therefore, account_server.js only processes accounts from account_schema.js where owner is an objectid reference, never from nsfs_account_schema.js where owner is a string.
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)
255-257: Verify thatget_account_identifier_idcorrectly handles IAM users in containerized deployment.The comment states that containerized deployment should NOT validate against IAM user ID, but the implementation of
get_account_identifier_idmust be reviewed to confirm this behavior is correctly enforced. Based on the comment and PR objectives, the function should return:
undefined(or similar falsy value) for IAM users in containerized deployment- The account ID for root accounts in both deployment types
- The account ID for IAM users in NC deployment
Inspect
src/endpoint/s3/s3_bucket_policy_utils.jsto verify the implementation returns the correct values for each scenario.
Comment on lines
+255
to
+256
| // Both NSFS NC and containerized will validate bucket policy against acccount id | ||
| // but in containerized deplyment not against IAM user ID. |
There was a problem hiding this comment.
Fix typos in comments.
Two typos in the comments:
- Line 255: "acccount" should be "account"
- Line 256: "deplyment" should be "deployment"
Apply this diff to fix the typos:
- // Both NSFS NC and containerized will validate bucket policy against acccount id
- // but in containerized deplyment not against IAM user ID.
+ // Both NSFS NC and containerized will validate bucket policy against account id
+ // but in containerized deployment not against IAM user ID.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Both NSFS NC and containerized will validate bucket policy against acccount id | |
| // but in containerized deplyment not against IAM user ID. | |
| // Both NSFS NC and containerized will validate bucket policy against account id | |
| // but in containerized deployment not against IAM user ID. |
🤖 Prompt for AI Agents
In src/endpoint/s3/s3_rest.js around lines 255 to 256, fix two typos in the
comment: change "acccount" to "account" and "deplyment" to "deployment" so the
comment reads correctly about validating bucket policy against account id and
that containerized deployment does not validate against IAM user ID.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Describe the Problem
User ID for principal is not supported
Explain the Changes
Issues: Fixed #xxx / Gap #xxx
Testing Instructions:
GAP: Missing integration test for IAM users, Will add in upcoming PRs
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.