Enhancements related to the share verify and re-apply feature

[anushka-singh]

Is your idea related to a problem? Please describe. In my testing for feature: https://github.com/data-dot-all/dataall/pull/1062/files I found an enhancement that can be made to the backend. We can do this as a part of 2.4 if we so choose. Here are 2 issues:

1. I replaced consumer role directly in the bucket policy with new consumer role. When I re-apply share, the older consumer role I had removed is re-added, but the new consumer role is not removed. The new role was added manually and should have been removed. For example: Earlier bucket policy after successful share was:

{
            "Sid": "DataAll-Bucket-ReadOnly",
            "Effect": "Allow",
            "Principal": {
                "AWS": [
                    "arn:aws:iam::1234567:role/dataall-test-consumer-env",
                ]
            },
            "Action": [
                "s3:List*",
                "s3:GetObject"
            ],
            "Resource": [
                "arn:aws:s3:::test-anushka-imported",
                "arn:aws:s3:::test-anushka-imported/*"
            ]
        },

I manually replaced the Principal with "arn:aws:iam::1234567:role/dataall-test-consumer-env1", in the bucket policy. This role exists, but permission was granted manually. Data.all does not detect this.

Now, I verified status of the share, which became unhealthy. And then when I hit re-apply share, this is the new bucket policy:

{
            "Sid": "DataAll-Bucket-ReadOnly",
            "Effect": "Allow",
            "Principal": {
                "AWS": [
                    "arn:aws:iam::1234567:role/dataall-test-consumer-env",
                    "arn:aws:iam::1234567:role/dataall-test-consumer-env1"
                ]
            },
            "Action": [
                "s3:List*",
                "s3:GetObject"
            ],
            "Resource": [
                "arn:aws:s3:::test-anushka-imported",
                "arn:aws:s3:::test-anushka-imported/*"
            ]
        },

I would expect dataall to be self-cleaning and to remove this extra manually created principal. However, it did not do so. We should have guardrails around people erroneously editing the bucket or kms policies and messing up data all permissions. Dataall should remove stuff it does not recognize, but that is not the case..

2. I completely wiped out bucket policy. Then checked health status. Health status for table and bucket both was unhealthy. Then I revoked access to both items. Table went to revoke_succeeded, but bucket went to revoke_failed.

I think, we should not allow revoke actions when shares are in broken state. Maybe we should tell them to first re-apply share and then try revoking. We can print the same message on the UI as we do when states try to go from a state to an invalid state. Like approved to deleted.

P.S. Don't attach files. Please, prefer add code snippets directly in the message body.

[dlpzx]

I see the point of the issue. Thanks @anushka-singh. It is true that we only check that the objects of the share are present. If there are extra permissions we do not clean them up. There are 2 main challenges/downsides of verifying non-extra statements in share:

1. First a challenge: whenever we re-apply shares we do not focus on a single shares but on all shares on an object. If a bucket has been shared with X principals we need to consider all those in our sharing process
2. And also a downside: removing extra statements disallows users to manage permissions outside of data.all. Which might be what we want for some cases, but on certain cases it might be an issue (e.g. you are adding additional bucket policy statements for other applications, additional LF permissions for column-level security...)

I would like to hear the opinion of others on this one. @noah-paige @petrkalos @SofiaSazonova