noobaa-core icon indicating copy to clipboard operation
noobaa-core copied to clipboard

Remove `allowed_buckets`

Open tangledbytes opened this issue 2 years ago • 2 comments

Explain the changes

This PR attempts to remove allowed_buckets construct from Noobaa core:

  1. The construct of allowed_buckets.permission_list is replaced with Bucket Policy.
  2. All the files that belonged to lambdas and were touched in the PR are deleted.
  3. Tests were modified to use Bucket Policies wherever either allowed_buckets.full_permission or allowed_buckets.permission_list was being used.
  4. Upgrade script has been added for version 5.12 which removes allowed_buckets field from the accounts in the database.

Issues: Fixed BZ1929108

  1. https://bugzilla.redhat.com/show_bug.cgi?id=1929108

Testing Instructions:

  1. Run make test
  2. Run make test-postgres
  3. Run docker run --user root --rm --name test1 noobaa-tester ./src/test/unit_tests/run_npm_test_on_test_container.sh -s sudo_index.js
  • [ ] Doc added/updated
  • [ ] Tests added

tangledbytes avatar Jun 30 '22 11:06 tangledbytes

I think they can

nimrod-becker avatar Jun 30 '22 13:06 nimrod-becker

Streamlining discussion (TLDR of above comments) for reviewers

All of the "resolved" comments have been addressed

There are 2 items still needs to be discussed in this PR:

  1. has_bucket_permission has become fairly complicated because
    • Earlier it used to rely on allowed_buckets which now no longer exists. It has been now replaced with bucket policy.
    • Regardless of the intended action, has_bucket_permission verifies every action because this function is directly and indirectly called in multiple other portions where the intended action may or may not be accessible.
  2. Removal of allowed_buckets.permission_list (NooBaa Operator uses the above field to grant access to accounts to the buckets created via OBC) causes the following issues :
    1. NooBaa operator will fail to give access to accounts associated with the OBC.
      • Proposed Solution: Replace allowed_buckets.permission_list with bucket policy.
      • Issue with proposed solution: Creating a bucket policy without user consent.
    2. Users will not be able to access their previous buckets:
      • Proposed Solution: Translate all of the allowed_buckets.permission_list to bucket policy in the 5.12 upgrade script.
      • Issue with proposed solution: Creating a bucket policy without user consent.

tangledbytes avatar Aug 03 '22 09:08 tangledbytes

After discussions over the items mentioned in the previous comment:

  1. has_bucket_permission rigorous permission checks if can be avoided or reduced must be reduced. This may involve removing redundant permission checks.
  2. Removal allowed_buckets.full_permission may cause regression but the goal is to ensure that at least 90% of the users should be able to upgrade without any hiccups.
    • 2 flows that needs to be fully functional are:
      1. Creation of OBC and verifying if the bucket is accessible and the user can perform operations on the bucket.
      2. Creation of S3 bucket by the user and verifying that the user can perform operations on the bucket.

tangledbytes avatar Aug 18 '22 15:08 tangledbytes

After discussion with @jackyalbo, load_bucket and find_bucket in the bucket_server and object_server no longer rely on has_bucket_permission which essentially means, that if an RPC is performed and the user is authenticated then they will be able to access buckets/objects without further authorization. The exception to this is list_bucket which will list only the buckets to which the requesting account has access. Hence, authz is now only limited to s3_rest sts_rest.

PS: For some reason the test is failing here although passing on the local system.

tangledbytes avatar Sep 07 '22 09:09 tangledbytes

Rebased the branch ✔️. Thank you @jackyalbo for reviewing this PR 😄!

Merging the PR...

tangledbytes avatar Sep 12 '22 13:09 tangledbytes