Skip to content

Conversation

@shirady
Copy link
Contributor

@shirady shirady commented Nov 25, 2025

Describe the Problem

Currently, bucket policy principal of ARN was for root user account only, after this change it would be effective for IAM users under this account as well.

Explain the Changes

  1. Add additional check after the check of bucket policy by principal ARN of the user to also check the permission in account level.
  2. Fix permission_by_arn call to has_bucket_policy_permission so the last argument will be an object.

Issues:

List of GAPs:

  1. This code does not handle a case where there is a user under an OBC account, in case we decide to add this layer.
  2. The tests are only manual at this point; there is a plan to add automated tests after we have the design change.

Testing Instructions:

  1. Build the images and install the 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 2 accounts:
  • nb account create shira-acc01 -n test1 --show-secrets
  • nb account create shira-acc02 -n test1 --show-secrets
  1. Create the alias for the accounts:
    account-1-s3='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:12443'
    account-2-s3='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:12443'
    account-2-iam='AWS_ACCESS_KEY=<access-key> AWS_SECRET_ACCESS_KEY=<secret-key> aws --no-verify-ssl --endpoint-url https://localhost:14443'
  2. Check the connection to the endpoint:
  • try to list the buckets: account-1-s3 s3 ls; echo $? account-2-s3 s3 ls; echo $?
  1. Create a bucket for each account:
  • account-1-s3 s3 mb s3://buc-acc1
  • account-2-s3 s3 mb s3://buc-acc2
  1. Create a user under account 2: account-2-iam iam create-user --user-name Robert
    Note: To validate user creation, you can run account-2-iam iam list-users and expect 1 user in the list
  2. Create access keys: account-2-iam iam create-access-key --user-name Robert
  3. Add IAM policy to the user: account-2-iam iam put-user-policy --user-name Robert --policy-name policy_allow_s3 --policy-document file://~/Documents/iam-tests/iam_policies/policy_allow_s3.json

policy_allow_s3.json

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Action": [
                "s3:*"
            ],
            "Resource": "*"
        }
    ]
}
  1. Create the alias for the user (like in step 5 with alias user-2-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'
  2. The case with no bucket policy, for example:
  • The user cannot put an object in the bucket in the second account:
    echo 'test_data' | user-1-s3 s3 cp - s3://buc-acc1/test_object.txt (should not work)
  • The user cannot list objects in the bucket (to check the flow of the RPC call of has_bucket_action_permission function that was created in the auth_server):
    user-1-s3 s3 ls s3://buc-acc1 (should not work)
  1. Find the ID of account 2 for the ARN, for example by running: account-2-s3 s3api get-bucket-acl --bucket buc-acc2 and taking the ID.
  2. Add bucket policy of the root account user as principal:
    account-1-s3 s3api put-bucket-policy --bucket buc-acc1 --policy file://policy_principal_arn_owner.json

policy_principal_arn_owner.json

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": [
                    "arn:aws:iam::69259fff8b07c200228c4fb3:root"
                ]
            },
            "Action": [
                "s3:*"
            ],
            "Resource": "*"
        }
    ]
}
  1. The case no bucket policy, for example:
  • The user can put an object in the bucket in the second account:
    echo 'test_data' | user-1-s3 s3 cp - s3://buc-acc1/test_object.txt (should work)
  • The user can list objects in the bucket (to check the flow of the RPC call of has_bucket_action_permission function that was created in the auth_server):
    user-1-s3 s3 ls s3://buc-acc1 (should not work)

Code changes for testing:

  1. To see the account (of a user) in the cache after changes, src/sdk/object_sdk.js uses cache expiry of 1 millisecond.
const account_cache = new LRUCache({
    name: 'AccountCache',
-    expiry_ms: config.OBJECT_SDK_ACCOUNT_CACHE_EXPIRY_MS,
+   expiry_ms: 1, //SDSD 

Notes:

  • In step 1 - deploying the system, I used --use-standalone-db for simplicity (fewer steps for the system in Ready status).

  • Doc added/updated

  • Tests added

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced bucket permission validation to include additional owner-based authorization checks, ensuring more comprehensive access control for S3 operations.

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

@coderabbitai
Copy link

coderabbitai bot commented Nov 25, 2025

Walkthrough

Adds an owner-root ARN permission check path to bucket authorization: s3_rest.js computes owner ARN for non-NC deployments and checks it alongside id/name/arn checks; auth_server.js consults the bucket owner's root ARN policy and enforces DENY if that owner-specific check returns DENY.

Changes

Cohort / File(s) Summary
Authorization permission logic
src/server/common_services/auth_server.js
Adds owner-root-ARN policy evaluation inside has_bucket_action_permission: when account.owner exists, computes owner root ARN, calls has_bucket_policy_permission for that ARN (with disallow_public_access option), returns false on DENY, and treats owner-ARN ALLOW as an additional ALLOW pathway.
S3 endpoint authorization flow
src/endpoint/s3/s3_rest.js
Wraps is_nc_deployment as Boolean and introduces permission_by_arn_owner. For non-NC deployments (and when account.owner present) builds owner ARN via create_arn_for_root(account.owner) and calls has_bucket_policy_permission with { disallow_public_access }. Final allow/deny considers permission_by_arn_owner along with id/name/arn checks and is_owner. NC behavior remains unchanged for ARN owner checks.

Sequence Diagram

sequenceDiagram
    participant Client
    participant S3REST as S3 REST Endpoint
    participant Auth as AuthServer
    participant Policy as Bucket Policy

    Client->>S3REST: Request
    S3REST->>S3REST: Determine deployment (is_nc_deployment)
    S3REST->>S3REST: Check permission_by_id/name/arn (pass {disallow_public_access})
    alt Non-NC and account.owner exists
        S3REST->>S3REST: Build owner ARN (create_arn_for_root)
        S3REST->>Auth: has_bucket_action_permission(owner ARN, {disallow_public_access})
    else NC or no owner
        S3REST->>Auth: has_bucket_action_permission(normal principal)
    end

    Auth->>Auth: If account.owner -> compute owner root ARN
    Auth->>Policy: has_bucket_policy_permission(owner ARN, {disallow_public_access})
    Policy-->>Auth: ALLOW / DENY / NONE
    alt Owner DENY
        Auth-->>S3REST: false (deny)
        S3REST->>Client: Access Denied
    else Owner ALLOW or not applicable
        Auth-->>S3REST: true/continue
        S3REST->>Client: Allow or Deny based on combined checks (id/name/arn/owner/is_owner)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Areas needing extra attention:
    • Correctness of create_arn_for_root(account.owner) usage and ARN formatting.
    • Consistency of { disallow_public_access } option propagation to all has_bucket_policy_permission calls.
    • Interaction between new permission_by_arn_owner path and existing NC branching and is_owner logic.

Possibly related PRs

Suggested reviewers

  • aayushchouhan09
  • liranmauda
  • naveenpaul1

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.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
Title check ✅ Passed The title accurately reflects the main change: adding IAM principal bucket policy support for account root users when accessed by IAM users under that account.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ 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.

@shirady shirady self-assigned this Nov 25, 2025
…questing Account Is IAM User

1. Add additional check after the check of bucket policy by principal ARN of the user to also check the permission in account level.
2. Fix permission_by_arn call to has_bucket_policy_permission so the last argument will be an object.

Signed-off-by: shirady <57721533+shirady@users.noreply.github.com>
@shirady shirady force-pushed the iam-bucket-policy-of-account-for-user branch from 3672155 to 8be23f7 Compare November 26, 2025 08:44
@shirady shirady merged commit 33924d7 into noobaa:master Nov 26, 2025
18 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