Enabling S3 bucket share

[anushka-singh]

Feature or Bugfix

• Feature

Detail

• We want to enable bucket sharing along with access point share which already exists in data all right now.
• A user will be able to request shares at bucket level and at the folder level with access points.
• Please NOTE: There is some common code between Access point share managers and processors and S3 Bucket managers and processors. We will send out a separate PR for that refactoring work at a later time.

Relates

• S3 Bucket data sharing #284
• Enabling S3 bucket share #823
• https://github.com/awslabs/aws-dataall/pull/846/files#diff-c1f522a1f50d8bcf7b6e5b2e586e40a8de784caa80345f4e05a6329ae2a372d0

Contributors:

• Contents of this PR have been contributed by @anushka-singh, @blitzmohit, @rbernotas, @TejasRGitHub

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.

[dlpzx]

looks good, only some small clarifications

[dlpzx]

Final remark on the PR: @noah-paige @anmolsgandhi @anushka-singh
At the moment all sharing mechanisms are enabled and there is no configuration on the config.json. So if a customer wants to restrict a particular way of sharing data in data.all it is not possible at the moment. What do you think?

• do we see the need of allowing configuration of the sharing mechanisms allowed?
• and how important is it?

In any case I think we can move forward with this PR and if needed implement the config in another PR

[dlpzx]

Testing in AWS:

• CICD from previous deployment succeeds. Migration script runs successfully.

---

For each of the following checks we test the 3 different scenarios:
- Created Dataset
- Imported Dataset with SSE-S3 encryption
- Imported Dataset with KMS encryption
- Imported Dataset from multiple buckets with KMS encryption

• Before doing anything, we check that pre-existing tables and folders shares still work --> check in Athena and Access points calls --> we see the changes in the UI, with the new command to list s3
• We share the S3 Bucket and Pre-existing tables and folders shares still work --> check in Athena and Access points calls
• Check S3 Bucket share works - we see all share item and its status after share as SHARE_SUCCEEDED for the bucket and get-object downloading of files in the bucket works

---

Now, only the case for imported KMS vs SSE-S3 are checked:
Check removing S3 Bucket share - get object gives an AccessDenied error. Share item status is REVOKE_SUCCEEDED.

• Imported Dataset with SSE-S3 encryption: succeeds but with some error logs (error 1)
• Imported Dataset with KMS encryption

Check removing folder share - get object gives an AccessDenied error. Share item status is REVOKE_SUCCEEDED

• Imported Dataset with SSE-S3 encryption: folders are in REVOKE_SUCCEDDED but share request stays in REVOKE_IN_PROGRESS (details and logs in error 2)
• Imported Dataset with KMS encryption, not tested yet (it will fail like the previous one)

Check re-adding bucket share only

• Imported Dataset with SSE-S3 encryption
• Imported Dataset with KMS encryption

Check re-adding folder share only

• Imported Dataset with SSE-S3 encryption, not tested yet (it will fail with error 3)
• Imported Dataset with KMS encryption, not tested yet (it will fail with error 3)

---

Edge cases. Some not tested yet.
Check add folder + s3 share at the same time

• Created Dataset with KMS encryption --> checked with a role that did not have previous folder share requests to avoid error 3 (get_object works using s3 and s3 access point)
• Imported Dataset with SSE-S3 encryption

Check remove folder + s3 share at the same time --- failing error 2

• Created Dataset with KMS encryption
• Imported Dataset with SSE-S3 encryption

Check adding one and removing the other at the same time

• Created Dataset with KMS encryption
• Imported Dataset with SSE-S3 encryption

Errors found

Error 1 ---> In another issue

This error is just cosmetic and it will addressed in #614

• Approving S3 Bucket sharing log shows failure but the sharing works fine (share item ends in SHARE_SUCCEEDED and we can download objects from S3 bucket):Failed to get kms key id of alias/SSE-S3 : An error occurred (NotFoundException) when calling the DescribeKey operation: Alias arn:aws:kms:eu-west-1:XXXXXXXX:alias/SSE-S3 is not found.
• Revoking S3 Bucket sharing log shows failure but the sharing revoke works fine (share item ends in REVOKE_SUCCEEDED and we cannot download objects from S3 bucket): Failed to get kms key id of alias/SSE-S3 : An error occurred (NotFoundException) when calling the DescribeKey operation: Alias arn:aws:kms:eu-west-1:XXXXXXX:alias/SSE-S3 is not found.

Error 2 ---> 🧑‍🏭 needs fixing

This is a generic issue, it is not due to pre-existing resources.
Imported Dataset with SSE-S3 encryption: folders are in REVOKE_SUCCEDDED but share request stays in REVOKE_IN_PROGRESS (error 2)
Looking at the logs, it fails in dataall/modules/dataset_sharing/services/data_sharing_service.py:199 when we pass the variable existing_shared_buckets=existing_shared_buckets to the initialization of the ProcessS3AccessPointShare, but that variable is not declared in the class

Error 3 ---> 🧑‍🏭 needs fixing

This is a backwards compatibility issue.
It fails when the Bucket is encrypted with a KMS key. Looking at the code and the logs I think the issue is in this line dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py:184

share_manager.add_missing_resources_to_policy_statement(
    kms_key_id,
    kms_target_resources,
    existing_policy["Statement"][1],
    IAM_ACCESS_POINT_ROLE_POLICY
)

We are assuming that the IAM role policy has 2 statements, but in pre-existing IAM roles there is one statement only.

Error 4 ---> In another issue

Sometimes we face again the issue that we had in the past of an access point taking a long time to get created and the put access point policy failure. I just want to note it here, but it is unrelated to this PR and it should be solved in a separate PR.

[dlpzx]

I just realized:

1. the PR is pointing to v2m1m0. Changing the source branch results in some additional git-ing that we need to do
2. we need to update the documentation in the userguide. How much bandwidth do you have for these? I don't think it will be much

[anushka-singh]

Final remark on the PR: @noah-paige @anmolsgandhi @anushka-singh At the moment all sharing mechanisms are enabled and there is no configuration on the config.json. So if a customer wants to restrict a particular way of sharing data in data.all it is not possible at the moment. What do you think?

• do we see the need of allowing configuration of the sharing mechanisms allowed?
• and how important is it?

In any case I think we can move forward with this PR and if needed implement the config in another PR

On this comment.
We do have a config for disabling the folder sharing mechanism.
"file_uploads": false, "file_actions": false,
Setting this to false, disables access point sharing.
There is no provision for disabling s3 bucket sharing at the moment though.
I think we dont need the option at the moment since the idea was every dataset should be able to be shared fully with bucket share or folder wise with access point. If in the future, we identify a need for restricting access, we can simply add a config in config.json for it.

[anushka-singh]

I just realized:

1. the PR is pointing to v2m1m0. Changing the source branch results in some additional git-ing that we need to do
2. we need to update the documentation in the userguide. How much bandwidth do you have for these? I don't think it will be much

1. Yes, that will be needed. I might need your help with it.
2. I would like to update the documentation too. Do we have an estimate on how long it will take? I can then maybe try to put in some sprint planning.

[anushka-singh]

Testing in AWS:

• CICD from previous deployment succeeds. Migration script runs successfully.

For each of the following checks we test the 3 different scenarios: - Created Dataset - Imported Dataset with SSE-S3 encryption - Imported Dataset with KMS encryption - Imported Dataset from multiple buckets with KMS encryption

• Before doing anything, we check that pre-existing tables and folders shares still work --> check in Athena and Access points calls --> we see the changes in the UI, with the new command to list s3
• We share the S3 Bucket and Pre-existing tables and folders shares still work --> check in Athena and Access points calls
• Check S3 Bucket share works - we see all share item and its status after share as SHARE_SUCCEEDED for the bucket and get-object downloading of files in the bucket works

Now, only the case for imported KMS vs SSE-S3 are checked: Check removing S3 Bucket share - get object gives an AccessDenied error. Share item status is REVOKE_SUCCEEDED.

• Imported Dataset with SSE-S3 encryption: succeeds but with some error logs (error 1)
• Imported Dataset with KMS encryption

Check removing folder share - get object gives an AccessDenied error. Share item status is REVOKE_SUCCEEDED

• Imported Dataset with SSE-S3 encryption: folders are in REVOKE_SUCCEDDED but share request stays in REVOKE_IN_PROGRESS (details and logs in error 2)
• Imported Dataset with KMS encryption, not tested yet (it will fail like the previous one)

Check re-adding bucket share only

• Imported Dataset with SSE-S3 encryption
• Imported Dataset with KMS encryption

Check re-adding folder share only

• Imported Dataset with SSE-S3 encryption, not tested yet (it will fail with error 3)
• Imported Dataset with KMS encryption, not tested yet (it will fail with error 3)

Edge cases. Some not tested yet. Check add folder + s3 share at the same time

• Created Dataset with KMS encryption --> checked with a role that did not have previous folder share requests to avoid error 3 (get_object works using s3 and s3 access point)
• Imported Dataset with SSE-S3 encryption

Check remove folder + s3 share at the same time --- failing error 2

• Created Dataset with KMS encryption
• Imported Dataset with SSE-S3 encryption

Check adding one and removing the other at the same time

• Created Dataset with KMS encryption
• Imported Dataset with SSE-S3 encryption

Errors found

Error 1 ---> In another issue

This error is just cosmetic and it will addressed in #614

• Approving S3 Bucket sharing log shows failure but the sharing works fine (share item ends in SHARE_SUCCEEDED and we can download objects from S3 bucket):Failed to get kms key id of alias/SSE-S3 : An error occurred (NotFoundException) when calling the DescribeKey operation: Alias arn:aws:kms:eu-west-1:XXXXXXXX:alias/SSE-S3 is not found.
• Revoking S3 Bucket sharing log shows failure but the sharing revoke works fine (share item ends in REVOKE_SUCCEEDED and we cannot download objects from S3 bucket): Failed to get kms key id of alias/SSE-S3 : An error occurred (NotFoundException) when calling the DescribeKey operation: Alias arn:aws:kms:eu-west-1:XXXXXXX:alias/SSE-S3 is not found.

Error 2 ---> 🧑‍🏭 needs fixing

This is a generic issue, it is not due to pre-existing resources. Imported Dataset with SSE-S3 encryption: folders are in REVOKE_SUCCEDDED but share request stays in REVOKE_IN_PROGRESS (error 2) Looking at the logs, it fails in dataall/modules/dataset_sharing/services/data_sharing_service.py:199 when we pass the variable existing_shared_buckets=existing_shared_buckets to the initialization of the ProcessS3AccessPointShare, but that variable is not declared in the class

Error 3 ---> 🧑‍🏭 needs fixing

This is a backwards compatibility issue. It fails when the Bucket is encrypted with a KMS key. Looking at the code and the logs I think the issue is in this line dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py:184

share_manager.add_missing_resources_to_policy_statement(
    kms_key_id,
    kms_target_resources,
    existing_policy["Statement"][1],
    IAM_ACCESS_POINT_ROLE_POLICY
)

We are assuming that the IAM role policy has 2 statements, but in pre-existing IAM roles there is one statement only.

Error 4 ---> In another issue

Sometimes we face again the issue that we had in the past of an access point taking a long time to get created and the put access point policy failure. I just want to note it here, but it is unrelated to this PR and it should be solved in a separate PR.

Error 2 and 3 found here have been fixed in next revision

[dlpzx]

Testing in AWS 2:

• CICD from previous deployment succeeds. Migration script runs successfully.

Before doing anything, we check that pre-existing folders shares still work --> check Access points calls --> we see the changes in the UI, with the new command to list s3

• Created Dataset
• Imported Dataset with SSE-S3 encryption
• Imported Dataset with KMS encryption

We share the S3 Bucket and Pre-existing folders shares still work --> check Access points calls

• Created Dataset
• Imported Dataset with SSE-S3 encryption
• Imported Dataset with KMS encryption
  Note: it actually solves issue with KMS keys access

Check S3 Bucket share works - we see all share item and its status after share as SHARE_SUCCEEDED for the bucket and get-object downloading of files in the bucket works

• Created Dataset
• Imported Dataset with SSE-S3 encryption
• Imported Dataset with KMS encryption
  Note: verified IAM policy in AWS Console

Check removing S3 Bucket share - get object gives an AccessDenied error. Share item status is REVOKE_SUCCEEDED. Access points working as initially (with small bug in KMS)

• Created Dataset
• Imported Dataset with SSE-S3 encryption
• Imported Dataset with KMS encryption

Check removing folder share - get object gives an AccessDenied error. Share item status is REVOKE_SUCCEEDED

• Created Dataset 🔴 Error 1
• Imported Dataset with SSE-S3 encryption
• Imported Dataset with KMS encryption 🔴 Error 1

Check re-adding bucket share only

• Created Dataset (tested after fixes)
• Imported Dataset with SSE-S3 encryption
• Imported Dataset with KMS encryption(tested after fixes)

Check re-adding folder share only

• Created Dataset(tested after fixes)
• Imported Dataset with SSE-S3 encryption -->🔴 works but with error 2 (additional permissions to None kms key id)
• Imported Dataset with KMS encryption(tested after fixes)

---------------------- New shares
Edge cases
Check add folder + s3 share at the same time

• Created Dataset
• Imported Dataset with SSE-S3 encryption
• Imported Dataset with KMS encryption

Check remove folder + s3 share at the same time

• Created Dataset
• Imported Dataset with SSE-S3 encryption 🔴 Error 3
• Imported Dataset with KMS encryption

Check adding one and removing the other at the same time
---> it is not possible by design in the share workflow

Errors found

Error 1

It is the same error as before. For backward compatibility the second statement in the IAM policy does not exist also in the revoke process. The error is in dataall/modules/dataset_sharing/services/share_managers/s3_access_point_share_manager.py:424

        if existing_policy:
            s3_target_resources = [
                f"arn:aws:s3:::{dataset.S3BucketName}",
                f"arn:aws:s3:::{dataset.S3BucketName}/*",
                f"arn:aws:s3:{dataset.region}:{dataset.AwsAccountId}:accesspoint/{access_point_name}",
                f"arn:aws:s3:{dataset.region}:{dataset.AwsAccountId}:accesspoint/{access_point_name}/*"
            ]
            ShareManagerUtils.remove_resource_from_statement(
                existing_policy["Statement"][0],
                s3_target_resources
            )
            if kms_key_id:
                kms_target_resources = [
                    f"arn:aws:kms:{dataset.region}:{dataset.AwsAccountId}:key/{kms_key_id}"
                ]
                ShareManagerUtils.remove_resource_from_statement(
                    existing_policy["Statement"][1],
                    kms_target_resources
                )

Error 2

The functionality works for sharing and revoking because for sharing the extra "arn:aws:kms:eu-west-1:XXXXXXXXX:key/None", does nothing. For the revoke, the access point does not exist so there is no issue.

The issue is in dataall/modules/dataset_sharing/services/share_managers/s3_bucket_share_manager.py:149

In this block (first time policy) we do not check for the kms_key_id

logger.info(
                f'{IAM_S3BUCKET_ROLE_POLICY} does not exists for IAM role {self.target_requester_IAMRoleName}, creating...'
            )
policy = {
                "Version": "2012-10-17",
                "Statement": [
                    {
                        "Effect": "Allow",
                        "Action": [
                            "s3:*"
                        ],
                        "Resource": [
                            f"arn:aws:s3:::{self.bucket_name}",
                            f"arn:aws:s3:::{self.bucket_name}/*"
                        ]
                    },
                    {
                        "Effect": "Allow",
                        "Action": [
                            "kms:*"
                        ],
                        "Resource": [
                            f"arn:aws:kms:{self.bucket_region}:{self.source_account_id}:key/{kms_key_id}"
                        ]
                    }
                ]
            }

Error 3

The biggest issue is that for revoke the following policies still contain resources that should not contain.

This is the dataall-targetDatasetS3Bucket-AccessControlPolicy

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Action": [
                "kms:*"
            ],
            "Resource": [
                "arn:aws:kms:eu-west-1:XXXXXXXXX:key/None",
                "arn:aws:s3:::imported-dataset",
                "arn:aws:s3:::imported-dataset/*"
            ]
        }
    ]
}

And the targetDatasetAccessControlPolicy still contains

"arn:aws:s3:::imported-dataset",
"arn:aws:s3:::imported-dataset/*",
"arn:aws:s3:eu-west-1:XXXXXXXXXXX:accesspoint/XXXXXX-research",
"arn:aws:s3:eu-west-1:XXXXXXXXXXX:accesspoint/XXXXXX-research/*"

[dlpzx]

Completed some of the test above that were blocked by error 1 and re-tested with the latest fixes @anushka-singh

Backwards compatibility test
Error 1 test:

• Created Dataset -- Error 1 fixed
• Imported Dataset with KMS encryption

Error 2 test:

• Imported Dataset with SSE-S3 encryption - Approve S3 bucket share - it does not contain an statement with kms/None

Error 3 test:

• Imported Dataset with SSE-S3 encryption - Revoke S3 Bucket share and access points - it successfully removes items in IAM policy

[dlpzx]

Looks good! All tests have passed. Great job @anushka-singh