Introduce check for IAM actions in share_verify bucket and access points + reapply with list of allowed actions #1407

SofiaSazonova · 2024-07-11T15:50:25Z

Feature or Bugfix

Bugfix

Detail

Share verifier now detects "s3:*" permission as an error
Share reapplied with enforced valid actions

Relates

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)?
- 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?
- 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?
- Do you use a standard proven implementations?
- Are the used keys controlled by the customer? Where are they stored?
Are you introducing any new policies/roles/users?
- Have you used the least-privilege principle? How?

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

noah-paige · 2024-07-11T17:33:47Z

backend/dataall/modules/s3_datasets_shares/services/share_managers/s3_bucket_share_manager.py

@@ -118,7 +118,7 @@ def check_s3_iam_access(self) -> None:
            existing_policy_statement=policy_document['Statement'][s3_statement_index],
        ):
            logger.info(
-                f'IAM Policy Statement {IAM_S3_BUCKETS_STATEMENT_SID}KMS does not contain resources {s3_target_resources}'
+                f'IAM Policy Statement {IAM_S3_BUCKETS_STATEMENT_SID}S3 does not contain resources {s3_target_resources}'


noah-paige · 2024-07-11T19:06:53Z

backend/dataall/modules/s3_datasets_shares/services/share_managers/s3_bucket_share_manager.py

+        else:
+            policy_check, missing_policies, extra_policies = share_policy_service.check_s3_actions(
+                existing_policy_statement=policy_document['Statement'][s3_statement_index]
+            )
+            if not policy_check:
+                logger.info(f'IAM Policy Statement {IAM_S3_BUCKETS_STATEMENT_SID}S3 has invalid actions')
+                self.bucket_errors.append(
+                    ShareErrorFormatter.invalid_policy_error_msg(
+                        self.target_requester_IAMRoleName,
+                        'IAM Policy Resource',
+                        f'{IAM_S3_BUCKETS_STATEMENT_SID}S3',
+                        'S3 Bucket',
+                        f'{self.bucket_name}',
+                        missing_actions=missing_policies,
+                        extra_actions=extra_policies,
+                    )
+                )


need to check_s3_actions(...) for folder shares as well

Such as with backend/dataall/modules/s3_datasets_shares/services/share_managers/s3_access_point_share_manager.py file in https://github.com/data-dot-all/dataall/pull/1406/files#diff-1d48910db676c4a11b5ff22dcb45a55eb6c31ebd2d9aa1313f90ce4e13b8ecfc

noah-paige · 2024-07-11T19:25:45Z

Test:

Verify Share Items from UI --> Shows missing permission and Unhealthy Item if s3:* (For Bucket Only - TODO for Folder!!)
Reapply Share Items from UI --> Fixes and removes s3:* for Unhealthy Share Items
ECS Verify ALL Share Item Task --> Shows missing permission and Unhealthy Item if s3:* (For Bucket Only - TODO for Folder!!)
ECS Reapply ALL Share Item Task --> Fixes and removes s3:* for Unhealthy Share Items

Example Verify Task Detecting s3:* Permissions:

backend/dataall/modules/s3_datasets_shares/services/s3_share_managed_policy_service.py

backend/dataall/modules/s3_datasets_shares/services/share_managers/share_manager_utils.py

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

petrkalos

lgtm, I tested it by manually setting s3:* on a folder/share policy and then run the check and the fix and worked fine

dlpzx

Looks good

Sofia Sazonova added 3 commits July 11, 2024 16:42

Share Verifiers detect s3:* as an error

13a1a99

Enforce, that actions are valid for Reapply share

2cd20f9

test and message formating

3c1844a

SofiaSazonova marked this pull request as ready for review July 11, 2024 16:24

noah-paige reviewed Jul 11, 2024

View reviewed changes

noah-paige mentioned this pull request Jul 11, 2024

Introduce check for actions in share_verify bucket and access points #1406

Closed

petrkalos reviewed Jul 12, 2024

View reviewed changes

backend/dataall/modules/s3_datasets_shares/services/s3_share_managed_policy_service.py Outdated Show resolved Hide resolved

backend/dataall/modules/s3_datasets_shares/services/s3_share_managed_policy_service.py Show resolved Hide resolved

s3_access_point policy + tests

3a18d4e

SofiaSazonova requested review from petrkalos and dlpzx July 12, 2024 10:09

dlpzx reviewed Jul 12, 2024

View reviewed changes

backend/dataall/modules/s3_datasets_shares/services/s3_share_managed_policy_service.py Show resolved Hide resolved

dlpzx reviewed Jul 12, 2024

View reviewed changes

backend/dataall/modules/s3_datasets_shares/services/share_managers/share_manager_utils.py Show resolved Hide resolved

rename to check_s3_actions_in_policy_statement

cebed69

dlpzx reviewed Jul 12, 2024

View reviewed changes

.../dataall/modules/s3_datasets_shares/services/share_managers/s3_access_point_share_manager.py Outdated Show resolved Hide resolved

dlpzx changed the title ~~Verify reapply s3 access~~ Introduce check for IAM actions in share_verify bucket and access points + reapply we force only the list of allowed actions Jul 12, 2024

dlpzx changed the title ~~Introduce check for IAM actions in share_verify bucket and access points + reapply we force only the list of allowed actions~~ Introduce check for IAM actions in share_verify bucket and access points + reapply with list of allowed actions Jul 12, 2024

PR comments

c6bf768

petrkalos approved these changes Jul 12, 2024

View reviewed changes

dlpzx approved these changes Jul 12, 2024

View reviewed changes

dlpzx merged commit 19f7981 into data-dot-all:main Jul 12, 2024
9 checks passed

SofiaSazonova mentioned this pull request Aug 12, 2024

S3Bucket WRITE/MODIFY permissions #1472

Merged

SofiaSazonova deleted the verify-reapply-s3-access branch September 30, 2024 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce check for IAM actions in share_verify bucket and access points + reapply with list of allowed actions #1407

Introduce check for IAM actions in share_verify bucket and access points + reapply with list of allowed actions #1407

SofiaSazonova commented Jul 11, 2024

noah-paige Jul 11, 2024

noah-paige Jul 11, 2024

noah-paige Jul 11, 2024

noah-paige commented Jul 11, 2024 •

edited

Loading

petrkalos left a comment

dlpzx left a comment

Introduce check for IAM actions in share_verify bucket and access points + reapply with list of allowed actions #1407

Introduce check for IAM actions in share_verify bucket and access points + reapply with list of allowed actions #1407

Conversation

SofiaSazonova commented Jul 11, 2024

Feature or Bugfix

Detail

Relates

Security

noah-paige Jul 11, 2024

Choose a reason for hiding this comment

noah-paige Jul 11, 2024

Choose a reason for hiding this comment

noah-paige Jul 11, 2024

Choose a reason for hiding this comment

noah-paige commented Jul 11, 2024 • edited Loading

petrkalos left a comment

Choose a reason for hiding this comment

dlpzx left a comment

Choose a reason for hiding this comment

noah-paige commented Jul 11, 2024 •

edited

Loading