[Gh 884] IAM policy splitting for requestor IAM policies #1650

TejasRGitHub · 2024-10-17T19:11:20Z

Feature or Bugfix

Feature

Detail

Updates the process of modifying the IAM policies after approve / revoke to add / delete resources and also split the policies into chunks
Updates the managed IAM policies to have indexes at the end
Contains backward compatibility and other additional checks to make sure correct policies are created and older policies are deleted
Contains a UI update to address this issue - Consumer roles list page is very slow #1459

Relates

Tests

Inviting / removing team to an environment ✅
Adding/ Removing consumption role to an environment ✅
Creating / revoking a share with consumption role for S3 bucket with bucketpolicy sharing ✅
Creating / revoking a share with environment team for S3 bucket with bucketpolicy sharing ✅
Creating / revoking a share with consumption role for S3 access point ✅
Creating / revoking a share with environment team for S3 access point ✅
Creating a share with requestors IAM policy being on the brink of policy length limits and checking if new indexed policy is getting created ✅
Revoking a share with requestors IAM policy being on the brink of policy length limits and checking if extra policies are deleted ✅
Checking if all consumption roles are loaded quickly on the environment teams UI asynchronously ✅
Check if requestor’s receive emails when an IAM role is about to reach the managed policy attachment limit ( Email Notification Changes )
Migrating Shares from old to new policy management. via share validator and share verifier ( Share verifier successfully mentions error message and Share re-applier successfully converts policies to indexed managed policies ) ✅
Creating new environment and deleting an environment ✅

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

Does this PR introduce or modify any input fields or queries - this includes
fetching data from storage outside the application (e.g. a database, an S3 bucket)? No
- Is the input sanitized?
- What precautions are you taking before deserializing the data you consume?
- Is injection prevented by parametrizing queries?
- Have you ensured no eval or similar functions are used?
Does this PR introduce any functionality or component that requires authorization? No
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
- Are you logging failed auth attempts?
Are you using or adding any cryptographic features? No
- Do you use a standard proven implementations?
- Are the used keys controlled by the customer? Where are they stored?No
Are you introducing any new policies/roles/users? yes
- Have you used the least-privilege principle? How? yes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

# Conflicts: # backend/dataall/modules/s3_datasets_shares/services/share_managers/s3_access_point_share_manager.py # backend/requirements.txt # docker-compose.yaml

anushka-singh · 2024-10-21T15:52:33Z

Can we fix the integration tests? Why are they failing?

deploy/stacks/container.py

frontend/src/modules/Environments/services/listAllEnvironmentConsumptionRoles.js

backend/dataall/base/aws/service_quota.py

backend/dataall/core/environment/cdk/pivot_role_core_policies/service_quota.py

backend/dataall/core/environment/cdk/pivot_role_stack.py

backend/dataall/base/utils/iam_policy_utils.py

backend/dataall/base/utils/naming_convention.py

backend/dataall/core/environment/services/managed_iam_policies.py

dlpzx

Hi @TejasRGitHub thanks for the contribution! I reviewed the IAM chunking with the new utilities and left some easy-fix comments. I stopped after reviewing the the ManagedPolicy interface as it requires some re-work that could affect the rest of the implementation.

backend/dataall/modules/shares_base/services/share_notification_service.py

dlpzx

Hi @TejasRGitHub thank you for the changes, it is way more in shape :) I added some additional comments with suggestions and improvements. I am also going to request you to 1) fix the failing integration tests and 2) list in the PR description the testing that you have done to ensure this PR is not introducing any error. include all testing scenarios: approve-verify-revoke new share, approve-verify-revoke old share, upgrade an old share....

docker-compose.yaml

backend/dataall/base/aws/iam.py

TejasRGitHub · 2024-10-30T18:42:47Z

@dlpzx , I have updated the PR and replied to your comments where I thought more discussion is needed.

I have updated the tests that I had done while submitting the PR. I will perform those tests once the PR is ready for merging.
Also, I will fix all the integration test which are failing once the PR is in good shape.

Thanks Again for taking the time to review and providing your insightful review comments

dlpzx

Left one ultra minor change comment about the exceptions name; but other than that the rest looks good! The only missing thing before approve are the integration tests.

Thank you for the great work @TejasRGitHub

TejasRGitHub · 2024-11-05T00:08:59Z

Hi @dlpzx , I have updated the PR and fixed unit tests. ✅ . Also , I have tested the code with the test cases ( as described in the PR details section ).
I have updated the PR with a few code changes - for some fixes I found during testing - and also refactored code to simplify.
Can you please take a look at the latest changes and let me know if this PR still looks good

backend/dataall/base/utils/iam_cdk_utils.py

backend/dataall/modules/shares_base/tasks/share_manager_task.py

backend/dataall/modules/s3_datasets/cdk/pivot_role_datasets_policy.py

backend/dataall/base/utils/iam_cdk_utils.py

.../dataall/modules/s3_datasets_shares/services/share_managers/s3_access_point_share_manager.py

dlpzx

Left some minor changes!

dlpzx

It is reeaady! Thank you @TejasRGitHub 🚀

TejasRGitHub and others added 8 commits October 7, 2024 17:29

Changes for splitting IAM role changes

34ac3e2

Merge branch 'main' into data-dot-allgh-884-IAM-policy-splitting

330dc51

# Conflicts: # backend/dataall/modules/s3_datasets_shares/services/share_managers/s3_access_point_share_manager.py # backend/requirements.txt # docker-compose.yaml

Sycing latest chanegs from aws dev for iam splitting

44d1205

Corrections to unit tests

e131da7

Service Quota file

3087bc6

Adding comments and other changes

db36fb0

Correcting unit tests and making env chanegs for SES emails to work

6af3c2e

Changes observed during testing

e072f25

TejasRGitHub added 3 commits October 21, 2024 19:14

Adding new file and changes for IAM policy utils

d83c805

Corrections

b51a428

Adding converter file

fe6c1fe

TejasRGitHub commented Oct 22, 2024

View reviewed changes

deploy/stacks/container.py Show resolved Hide resolved

TejasRGitHub commented Oct 22, 2024

View reviewed changes

frontend/src/modules/Environments/services/listAllEnvironmentConsumptionRoles.js Show resolved Hide resolved

TejasRGitHub added 3 commits October 21, 2024 19:32

backend linting changes

2070328

Unit test corrections

508f520

Correction in share

9d8583d

dlpzx self-requested a review October 22, 2024 08:30

dlpzx reviewed Oct 22, 2024

View reviewed changes

backend/dataall/base/aws/service_quota.py Outdated Show resolved Hide resolved

dlpzx reviewed Oct 22, 2024

View reviewed changes

backend/dataall/core/environment/cdk/pivot_role_core_policies/service_quota.py Show resolved Hide resolved

dlpzx reviewed Oct 22, 2024

View reviewed changes

backend/dataall/core/environment/cdk/pivot_role_stack.py Outdated Show resolved Hide resolved

dlpzx reviewed Oct 22, 2024

View reviewed changes

backend/dataall/base/utils/iam_policy_utils.py Show resolved Hide resolved

dlpzx reviewed Oct 22, 2024

View reviewed changes

backend/dataall/base/utils/naming_convention.py Outdated Show resolved Hide resolved

dlpzx reviewed Oct 22, 2024

View reviewed changes

backend/dataall/core/environment/services/managed_iam_policies.py Show resolved Hide resolved

dlpzx requested changes Oct 22, 2024

View reviewed changes

TejasRGitHub added 4 commits October 22, 2024 11:07

Adding comments

d40faab

Simplifying interface

f78429c

Fixing few tests

7303bba

Linting

200313b

TejasRGitHub changed the title ~~[Gh 884] IAM policy splitting for requestor IAM policues~~ [Gh 884] IAM policy splitting for requestor IAM policies Oct 24, 2024

dlpzx reviewed Oct 30, 2024

View reviewed changes

backend/dataall/modules/shares_base/services/share_notification_service.py Outdated Show resolved Hide resolved

dlpzx reviewed Oct 30, 2024

View reviewed changes

TejasRGitHub commented Oct 30, 2024

View reviewed changes

docker-compose.yaml Show resolved Hide resolved

TejasRGitHub added 4 commits October 30, 2024 11:35

Change after PR review

21af992

Reverting changes made

73f98b1

More changes

e27b0aa

Minor changes

8abc4d6

TejasRGitHub commented Oct 30, 2024

View reviewed changes

backend/dataall/base/aws/iam.py Show resolved Hide resolved

TejasRGitHub added 3 commits October 30, 2024 13:52

Removing managed policy from unused gql calls

3b4dce2

python linting

1f5ec6e

Corrections

a048fe9

TejasRGitHub requested a review from dlpzx October 30, 2024 22:34

dlpzx reviewed Oct 31, 2024

View reviewed changes

TejasRGitHub added 5 commits October 31, 2024 12:37

Naming changes in exceptions

f48e07f

Refactoring and corrections

99556b3

Fixing unit tests

61f616b

Linting after correcting tests

e84e526

Removing parts not part of this PR

f5825ef

TejasRGitHub requested a review from dlpzx November 5, 2024 18:44