add check for existing permissions before adding new (#15)

Co-authored-by: Mohammad Zuber Khan <[email protected]>

remediation_worker/jobs/s3_enable_access_logging/requirements-dev.txt

@@ -1,6 +1,9 @@
 -r requirements.txt
 -c constraints.txt
 
+mock==4.0.2 \
+    --hash=sha256:3f9b2c0196c60d21838f307f5825a7b86b678cedc58ab9e50a8988187b4d81e0 \
+    --hash=sha256:dd33eb70232b6118298d516bbcecd26704689c386594f0f3c4f13867b2c56f72
 pytest==6.0.1 \
     --hash=sha256:85228d75db9f45e06e57ef9bf4429267f81ac7c0d742cc9ed63d09886a9fe6f4 \
     --hash=sha256:8b6007800c53fdacd5a5c192203f4e531eb2a1540ad9c752e052ec0f7143dbad

remediation_worker/jobs/s3_enable_access_logging/s3_enable_access_logging.py

@@ -75,10 +75,44 @@ def parse(self, payload):
         logging.info(return_dict)
         return return_dict
 
+    def check_log_delivery_permissions(self, acl):
+        write_grant = False
+        read_acp_grant = False
+
+        grants = acl.get("Grants")
+
+        if grants is None:
+            return write_grant, read_acp_grant
+
+        for grant in grants:
+            grantee = grant.get("Grantee")
+            if grantee is None:
+                continue
+            grantee_type = grantee.get("Type")
+            grantee_uri = grantee.get("URI")
+            if grantee_type is None or grantee_uri is None:
+                continue
+            if (
+                grantee_type == "Group"
+                and grantee_uri == "http://acs.amazonaws.com/groups/s3/LogDelivery"
+            ):
+                if grant["Permission"] == "WRITE":
+                    write_grant = True
+                elif grant["Permission"] == "READ_ACP":
+                    read_acp_grant = True
+
+        return write_grant, read_acp_grant
+
     def grant_log_delivery_permissions(self, client, bucket_name):
         # Give the group log-delievery WRITE and READ_ACP permisions to the
         # target bucket
         acl = client.get_bucket_acl(Bucket=bucket_name)
+
+        # check if the required permissions are already granted, and if present return
+        write_granted, read_acp_granted = self.check_log_delivery_permissions(acl)
+        if write_granted and read_acp_granted:
+            return
+
         write_grant = {
             "Grantee": {
                 "URI": "http://acs.amazonaws.com/groups/s3/LogDelivery",
@@ -96,8 +130,10 @@ def grant_log_delivery_permissions(self, client, bucket_name):
         del acl["ResponseMetadata"]
 
         modified_acl = copy.deepcopy(acl)
-        modified_acl["Grants"].append(write_grant)
-        modified_acl["Grants"].append(read_acp_grant)
+        if not write_granted:
+            modified_acl["Grants"].append(write_grant)
+        if not read_acp_granted:
+            modified_acl["Grants"].append(read_acp_grant)
         client.put_bucket_acl(Bucket=bucket_name, AccessControlPolicy=modified_acl)
 
     def ensure_log_target_bucket(self, client, target_bucket, region):

test/unit/test_s3_enable_access_logging.py

@@ -15,6 +15,7 @@
 import json
 
 import pytest
+from mock import Mock
 from botocore.exceptions import ClientError
 
 from remediation_worker.jobs.s3_enable_access_logging.s3_enable_access_logging import (
@@ -242,3 +243,74 @@ def put_bucket_logging(self, **kwargs):
     def test_dont_log_to_self(self, self_payload):
         with pytest.raises(SelfRemediationError):
             assert S3EnableAccessLogging().run([None, self_payload])
+
+    def test_check_log_delivery(self):
+        acl = {
+            "Grants": [
+                {
+                    "Grantee": {
+                        "Type": "Group",
+                        "URI": "http://acs.amazonaws.com/groups/s3/LogDelivery",
+                    },
+                    "Permission": "WRITE",
+                }
+            ]
+        }
+
+        action = S3EnableAccessLogging()
+        write_enabled, read_acp_enabled = action.check_log_delivery_permissions(acl)
+        assert write_enabled
+        assert not read_acp_enabled
+
+    def test_grant_log_delivery_permissions(self):
+        client = Mock()
+        client.get_bucket_acl.return_value = {
+            "ResponseMetadata": {
+                "RequestId": "6B0F579EDDCCAB3C",
+                "HostId": "9Csk0PXuRLyPhcKimBPbhfEmwQywAXPiWVWdpZPV+rjwVZO1DJMEKD/M65RJL+GguB3UMhOmpAQ=",
+                "HTTPStatusCode": 200,
+                "HTTPHeaders": {
+                    "x-amz-id-2": "9Csk0PXuRLyPhcKimBPbhfEmwQywAXPiWVWdpZPV+rjwVZO1DJMEKD/M65RJL+GguB3UMhOmpAQ=",
+                    "x-amz-request-id": "6B0F579EDDCCAB3C",
+                    "date": "Wed, 16 Sep 2020 16:57:36 GMT",
+                },
+                "RetryAttempts": 0,
+            },
+            "Owner": {
+                "DisplayName": "awsmasteremail",
+                "ID": "b101f924005dbb04273644ca983ef2ea93d43ad46757f21f65c40d48d75368c3",
+            },
+            "Grants": [
+                {
+                    "Grantee": {
+                        "DisplayName": "awsmasteremail",
+                        "ID": "b101f924005dbb04273644ca983ef2ea93d43ad46757f21f65c40d48d75368c3",
+                        "Type": "CanonicalUser",
+                    },
+                    "Permission": "FULL_CONTROL",
+                },
+                {
+                    "Grantee": {
+                        "Type": "Group",
+                        "URI": "http://acs.amazonaws.com/groups/s3/LogDelivery",
+                    },
+                    "Permission": "READ_ACP",
+                },
+            ],
+        }
+
+        bucket_name = "my_bucket"
+        action = S3EnableAccessLogging()
+        action.grant_log_delivery_permissions(client, bucket_name)
+        assert client.put_bucket_acl.call_count == 1
+        call_args = client.put_bucket_acl.call_args.kwargs
+
+        assert len(call_args) == 2
+        assert call_args.get("Bucket") == bucket_name
+
+        acp = call_args.get("AccessControlPolicy")
+        assert acp is not None
+        assert len(acp["Grants"]) >= 2
+        write_granted, read_granted = action.check_log_delivery_permissions(acp)
+        assert write_granted
+        assert read_granted