IAM | User ID for principal is not supported #9329
Open
+22
−4
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
WalkthroughAdded ownership-aware principal resolution: bucket principal lookup now avoids treating IAM user accounts (accounts with an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested labels
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, |
Signed-off-by: Naveen Paul <napaul@redhat.com>
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
| * @param {object} account | ||
| */ | ||
| function get_account_identifier_id(is_nc_deployment, account) { | ||
| if (is_nc_deployment || (!is_nc_deployment && account.owner === undefined)) { |
Contributor
There was a problem hiding this comment.
I'm not sure that you need this part !is_nc_deployment:
in case: is_nc_deployment = true, enter the condition on the first part
in case: is_nc_deployment = false enter the condition on the second part only
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
✏️ Tip: You can customize this high-level summary in your review settings.