Skip to content

Conversation

@shirady
Copy link
Contributor

@shirady shirady commented Dec 1, 2025

Describe the Problem

Currently, we didn't block accounts from performing the IAM API on themselves, the main issue it creates is that accounts can use the IAM API of access key i and delete their own access key.

Explain the Changes

  1. Throw and error in case account tries to perfrom IAM operation on himself.
    Note: decide to leave the message of the error simple.

Issues:

  1. none

Testing Instructions:

  1. Build the images and install NooBaa system on Rancher Desktop (see guide).
    Note: nb is an alias that runs the local operator from build/_output/bin (alias created by devenv).
  2. Wait for the default backing store pod to be in state Ready before starting the tests: kubectl wait --for=condition=available backingstore/noobaa-default-backing-store --timeout=6m -n test1
  3. I'm using port-forward (in a different tab):
  • S3 kubectl port-forward -n test1 service/s3 12443:443
  • IAM kubectl port-forward -n test1 service/iam 14443:443
  1. Create the alias for the admin account:
  • alias account-1-iam='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:14443'
  1. Check the connection to the endpoint:
  • try to list the users (should throw an error): account-1-iam iam list-users; echo $?
  1. Try to delete the access key of the admin: admin-iam iam delete-access-key --access-key-id <access-key-of-admin> should fail.
    output:
An error occurred (NotAuthorized) when calling the DeleteAccessKey operation: You do not have permission to perform this action.

Notes:

  • In step 1 - deploying the system, I used --use-standalone-db for 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

Release Notes

  • Bug Fixes
    • Resolved an authorization issue where root accounts could perform certain IAM operations on themselves. These operations are now properly blocked.

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
@shirady shirady self-assigned this Dec 1, 2025
@coderabbitai
Copy link

coderabbitai bot commented Dec 1, 2025

Walkthrough

A guard clause was added to the validate_and_return_requested_account function to deny IAM actions on the requesting account itself when the account owner is undefined, preventing AWS root accounts from performing self-directed IAM operations.

Changes

Cohort / File(s) Summary
Account validation guard
src/util/account_util.js
Added NOT_AUTHORIZED permission check in on_itself path when requesting_account.owner is undefined

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3–5 minutes

  • Single file with a focused guard clause addition
  • Straightforward permission check on a specific condition path
  • No complex logic or multi-component interactions

Possibly related PRs

Suggested reviewers

  • aayushchouhan09
  • naveenpaul1

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main change: blocking accounts from performing IAM API operations on themselves, which directly matches the core implementation change in the pull request.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/util/account_util.js (1)

700-707: Self-IAM guard looks correct; confirm scope and consider using _check_root_account

The new check correctly blocks root-like accounts (owner === undefined) from performing IAM actions on themselves and returns the expected NOT_AUTHORIZED + “You do not have permission to perform this action.”, which matches the PR’s described behavior.

Two minor follow-ups:

  • This currently only blocks root accounts (and any account with owner === undefined); IAM users (with owner defined) can still call IAM APIs on themselves. Please confirm this is intentional and matches your threat model / UX expectations.
  • To avoid duplicating the “root account” condition, you could reuse the existing helper:
-        requested_account = requesting_account;
-        // we do not allow for AWS account root user to perform IAM action on itself
-        if (requesting_account.owner === undefined) {
+        requested_account = requesting_account;
+        // we do not allow root accounts to perform IAM actions on themselves
+        if (_check_root_account(requesting_account) {
             throw new RpcError('NOT_AUTHORIZED', 'You do not have permission to perform this action.');
         }
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7362470 and 68aed49.

📒 Files selected for processing (1)
  • src/util/account_util.js (1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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-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/util/account_util.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/util/account_util.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

@shirady shirady merged commit 321de03 into noobaa:master Dec 2, 2025
18 of 19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants