From 306a16330dc100fd72461971e4c34545ebca587c Mon Sep 17 00:00:00 2001 From: Mohammad Zuber Khan Date: Wed, 16 Sep 2020 10:42:56 -0700 Subject: [PATCH] add check for existing permissions before adding new --- .../requirements-dev.txt | 3 + .../s3_enable_access_logging.py | 40 ++++++++++- test/unit/test_s3_enable_access_logging.py | 72 +++++++++++++++++++ 3 files changed, 113 insertions(+), 2 deletions(-) diff --git a/remediation_worker/jobs/s3_enable_access_logging/requirements-dev.txt b/remediation_worker/jobs/s3_enable_access_logging/requirements-dev.txt index 9412e93..cf03a93 100644 --- a/remediation_worker/jobs/s3_enable_access_logging/requirements-dev.txt +++ b/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 diff --git a/remediation_worker/jobs/s3_enable_access_logging/s3_enable_access_logging.py b/remediation_worker/jobs/s3_enable_access_logging/s3_enable_access_logging.py index 875fbaa..dd57755 100644 --- a/remediation_worker/jobs/s3_enable_access_logging/s3_enable_access_logging.py +++ b/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): diff --git a/test/unit/test_s3_enable_access_logging.py b/test/unit/test_s3_enable_access_logging.py index 711d678..9918c84 100644 --- a/test/unit/test_s3_enable_access_logging.py +++ b/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