From 9eb1ac2d959929814cc8e245da779f6b6fd311f2 Mon Sep 17 00:00:00 2001 From: Veselin Borisov Date: Wed, 27 Nov 2019 19:09:43 +0200 Subject: [PATCH 1/4] remote: Extend S3 remote support for custom ACL Besides AWS boto3 extra_arg `ACL` (`x-amz-acl` header), there four more that could be used to grant specific permissions to specific grantees on the uploaded object. These headers are: - `x-amz-grant-full-control` with boto3's extra_arg `GrantFullControl` - `x-amz-grant-read` with boto3's extra_arg `GrantRead` - `x-amz-grant-read-acp` with boto3's extra_arg `GrantReadACP` - `x-amz-grant-write-acp` with boto3's extra_arg `GrantWriteACP` As `ACL` (`x-amz-acl` header) is limited to be used with predefined canned ACLs, adding support for the aformentioned headers will help in applying more customized persmissions solutions according to different project needs. References: - https://docs.aws.amazon.com/AmazonS3/latest/dev/acl-overview.html#permissions - https://boto3.amazonaws.com/v1/documentation/api/latest/reference/customizations/s3.html#boto3.s3.transfer.S3Transfer.ALLOWED_UPLOAD_ARGS - https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutObjectAcl.html Fixes #(2930). --- dvc/config.py | 8 +++++++ dvc/remote/s3.py | 55 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) diff --git a/dvc/config.py b/dvc/config.py index 25b091c67..722d549d8 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -171,6 +171,10 @@ class Config(object): # pylint: disable=too-many-instance-attributes SECTION_AWS_USE_SSL = "use_ssl" SECTION_AWS_SSE = "sse" SECTION_AWS_ACL = "acl" + SECTION_AWS_GRANT_READ = "grant_read" + SECTION_AWS_GRANT_READ_ACP = "grant_read_acp" + SECTION_AWS_GRANT_WRITE_ACP = "grant_write_acp" + SECTION_AWS_GRANT_FULL_CONTROL = "grant_full_control" # gcp specific options SECTION_GCP_CREDENTIALPATH = CREDENTIALPATH @@ -210,6 +214,10 @@ class Config(object): # pylint: disable=too-many-instance-attributes Optional(SECTION_AWS_USE_SSL, default=True): Bool, SECTION_AWS_SSE: str, SECTION_AWS_ACL: str, + SECTION_AWS_GRANT_READ: str, + SECTION_AWS_GRANT_READ_ACP: str, + SECTION_AWS_GRANT_WRITE_ACP: str, + SECTION_AWS_GRANT_FULL_CONTROL: str, SECTION_GCP_PROJECTNAME: str, SECTION_CACHE_TYPE: supported_cache_type, Optional(SECTION_CACHE_PROTECTED, default=False): Bool, diff --git a/dvc/remote/s3.py b/dvc/remote/s3.py index 0b8237549..071b621e4 100644 --- a/dvc/remote/s3.py +++ b/dvc/remote/s3.py @@ -53,6 +53,8 @@ class RemoteS3(RemoteBASE): if self.acl: self.extra_args["ACL"] = self.acl + self._append_aws_grants_to_extra_args(config) + shared_creds = config.get(Config.SECTION_AWS_CREDENTIALPATH) if shared_creds: os.environ.setdefault("AWS_SHARED_CREDENTIALS_FILE", shared_creds) @@ -302,3 +304,56 @@ class RemoteS3(RemoteBASE): continue yield path_info.replace(path=fname) + + def _append_aws_grants_to_extra_args(self, config): + # Keys for extra_args can be one of the following list: + # https://boto3.amazonaws.com/v1/documentation/api/latest/reference/customizations/s3.html#boto3.s3.transfer.S3Transfer.ALLOWED_UPLOAD_ARGS + """ + ALLOWED_UPLOAD_ARGS = [ + 'ACL', 'CacheControl', 'ContentDisposition', 'ContentEncoding', + 'ContentLanguage', 'ContentType', 'Expires', 'GrantFullControl', + 'GrantRead', 'GrantReadACP', 'GrantWriteACP', 'Metadata', + 'RequestPayer', 'ServerSideEncryption', 'StorageClass', + 'SSECustomerAlgorithm', 'SSECustomerKey', 'SSECustomerKeyMD5', + 'SSEKMSKeyId', 'WebsiteRedirectLocation' + ] + """ + overwriting_acl_grantees = False + + self.grant_full_control = config.get( + Config.SECTION_AWS_GRANT_FULL_CONTROL, "" + ) + if self.grant_full_control: + self.extra_args["GrantFullControl"] = self.grant_full_control + overwriting_acl_grantees = True + + self.grant_read = config.get(Config.SECTION_AWS_GRANT_READ, "") + if self.grant_read: + self.extra_args["GrantRead"] = self.grant_read + overwriting_acl_grantees = True + + self.grant_read_acp = config.get(Config.SECTION_AWS_GRANT_READ_ACP, "") + if self.grant_read_acp: + self.extra_args["GrantReadACP"] = self.grant_read_acp + overwriting_acl_grantees = True + + self.grant_write_acp = config.get( + Config.SECTION_AWS_GRANT_WRITE_ACP, "" + ) + if self.grant_write_acp: + self.extra_args["GrantWriteACP"] = self.grant_write_acp + overwriting_acl_grantees = True + + if overwriting_acl_grantees: + overwriting_fields = [ + Config.SECTION_AWS_GRANT_FULL_CONTROL, + Config.SECTION_AWS_GRANT_READ, + Config.SECTION_AWS_GRANT_READ_ACP, + Config.SECTION_AWS_GRANT_WRITE_ACP, + ] + warning_message = [ + f"Using one of the [{', '.join(overwriting_fields)}] will", + "overwrite default ACL Grantees list. Default ACL Grantees", + "list include Owner with FULL_CONTROL permissions.", + ] + logger.warning(" ".join(warning_message)) From a160a8ea7a3cc15b2a84de73755cbf70bc27f2c0 Mon Sep 17 00:00:00 2001 From: Veselin Borisov Date: Tue, 10 Dec 2019 13:44:13 +0200 Subject: [PATCH 2/4] fixup! remote: Extend S3 remote support for custom ACL --- dvc/remote/s3.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dvc/remote/s3.py b/dvc/remote/s3.py index 071b621e4..2594a8a63 100644 --- a/dvc/remote/s3.py +++ b/dvc/remote/s3.py @@ -352,7 +352,9 @@ class RemoteS3(RemoteBASE): Config.SECTION_AWS_GRANT_WRITE_ACP, ] warning_message = [ - f"Using one of the [{', '.join(overwriting_fields)}] will", + "Using one of the [{0}] will".format( + ", ".join(overwriting_fields) + ), "overwrite default ACL Grantees list. Default ACL Grantees", "list include Owner with FULL_CONTROL permissions.", ] From 4c2bc6bf4853ce86b33c49a103af10353727ce0b Mon Sep 17 00:00:00 2001 From: Veselin Borisov Date: Thu, 12 Dec 2019 12:41:38 +0200 Subject: [PATCH 3/4] fixup! remote: Extend S3 remote support for custom ACL --- tests/unit/remote/test_s3.py | 40 ++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 tests/unit/remote/test_s3.py diff --git a/tests/unit/remote/test_s3.py b/tests/unit/remote/test_s3.py new file mode 100644 index 000000000..53e96b1b6 --- /dev/null +++ b/tests/unit/remote/test_s3.py @@ -0,0 +1,40 @@ +from unittest import TestCase + +from dvc.remote.s3 import RemoteS3 + + +class TestRemoteS3(TestCase): + bucket_name = "bucket-name" + prefix = "some/prefix" + url = "s3://{}/{}".format(bucket_name, prefix) + + def test_init(self): + config = {"url": self.url} + remote = RemoteS3(None, config) + + self.assertEqual(remote.path_info, self.url) + + def test_grants(self): + config = { + "url": self.url, + "grant_read": "id=read-permission-id,id=other-read-permission-id", + "grant_read_acp": "id=read-acp-permission-id", + "grant_write_acp": "id=write-acp-permission-id", + "grant_full_control": "id=full-control-permission-id", + } + remote = RemoteS3(None, config) + + self.assertEqual( + remote.extra_args["GrantRead"], + "id=read-permission-id,id=other-read-permission-id", + ) + self.assertEqual( + remote.extra_args["GrantReadACP"], "id=read-acp-permission-id" + ) + self.assertEqual( + remote.extra_args["GrantWriteACP"], "id=write-acp-permission-id" + ) + self.assertEqual( + remote.extra_args["GrantFullControl"], + "id=full-control-permission-id", + ) From cd461aec4fc1618c3c824067f7434258e8c3e923 Mon Sep 17 00:00:00 2001 From: Veselin Borisov Date: Fri, 13 Dec 2019 02:02:45 +0200 Subject: [PATCH 4/4] fixup! remote: Extend S3 remote support for custom ACL --- dvc/remote/s3.py | 54 +++++++----------------- tests/unit/remote/test_s3.py | 80 +++++++++++++++++++++--------------- 2 files changed, 63 insertions(+), 71 deletions(-) diff --git a/dvc/remote/s3.py b/dvc/remote/s3.py index 2594a8a63..6366fc426 100644 --- a/dvc/remote/s3.py +++ b/dvc/remote/s3.py @@ -7,7 +7,7 @@ import threading from funcy import cached_property, wrap_prop -from dvc.config import Config +from dvc.config import Config, ConfigError from dvc.exceptions import DvcException from dvc.exceptions import ETagMismatchError from dvc.path_info import CloudURLInfo @@ -318,44 +318,20 @@ class RemoteS3(RemoteBASE): 'SSEKMSKeyId', 'WebsiteRedirectLocation' ] """ - overwriting_acl_grantees = False - self.grant_full_control = config.get( - Config.SECTION_AWS_GRANT_FULL_CONTROL, "" - ) - if self.grant_full_control: - self.extra_args["GrantFullControl"] = self.grant_full_control - overwriting_acl_grantees = True + grants = { + Config.SECTION_AWS_GRANT_FULL_CONTROL: "GrantFullControl", + Config.SECTION_AWS_GRANT_READ: "GrantRead", + Config.SECTION_AWS_GRANT_READ_ACP: "GrantReadACP", + Config.SECTION_AWS_GRANT_WRITE_ACP: "GrantWriteACP", + } - self.grant_read = config.get(Config.SECTION_AWS_GRANT_READ, "") - if self.grant_read: - self.extra_args["GrantRead"] = self.grant_read - overwriting_acl_grantees = True + for grant_option, extra_args_key in grants.items(): + if config.get(grant_option, ""): + if self.acl: + raise ConfigError( + "`acl` and `grant_*` AWS S3 config options " + "are mutually exclusive" + ) - self.grant_read_acp = config.get(Config.SECTION_AWS_GRANT_READ_ACP, "") - if self.grant_read_acp: - self.extra_args["GrantReadACP"] = self.grant_read_acp - overwriting_acl_grantees = True - - self.grant_write_acp = config.get( - Config.SECTION_AWS_GRANT_WRITE_ACP, "" - ) - if self.grant_write_acp: - self.extra_args["GrantWriteACP"] = self.grant_write_acp - overwriting_acl_grantees = True - - if overwriting_acl_grantees: - overwriting_fields = [ - Config.SECTION_AWS_GRANT_FULL_CONTROL, - Config.SECTION_AWS_GRANT_READ, - Config.SECTION_AWS_GRANT_READ_ACP, - Config.SECTION_AWS_GRANT_WRITE_ACP, - ] - warning_message = [ - "Using one of the [{0}] will".format( - ", ".join(overwriting_fields) - ), - "overwrite default ACL Grantees list. Default ACL Grantees", - "list include Owner with FULL_CONTROL permissions.", - ] - logger.warning(" ".join(warning_message)) + self.extra_args[extra_args_key] = config.get(grant_option) diff --git a/tests/unit/remote/test_s3.py b/tests/unit/remote/test_s3.py index 53e96b1b6..57fcfcec7 100644 --- a/tests/unit/remote/test_s3.py +++ b/tests/unit/remote/test_s3.py @@ -1,40 +1,56 @@ -from unittest import TestCase +import pytest +from dvc.config import ConfigError from dvc.remote.s3 import RemoteS3 -class TestRemoteS3(TestCase): - bucket_name = "bucket-name" - prefix = "some/prefix" - url = "s3://{}/{}".format(bucket_name, prefix) +bucket_name = "bucket-name" +prefix = "some/prefix" +url = "s3://{}/{}".format(bucket_name, prefix) - def test_init(self): - config = {"url": self.url} - remote = RemoteS3(None, config) - self.assertEqual(remote.path_info, self.url) +@pytest.fixture(autouse=True) +def grants(): + return { + "grant_read": "id=read-permission-id,id=other-read-permission-id", + "grant_read_acp": "id=read-acp-permission-id", + "grant_write_acp": "id=write-acp-permission-id", + "grant_full_control": "id=full-control-permission-id", + } - def test_grants(self): - config = { - "url": self.url, - "grant_read": "id=read-permission-id,id=other-read-permission-id", - "grant_read_acp": "id=read-acp-permission-id", - "grant_write_acp": "id=write-acp-permission-id", - "grant_full_control": "id=full-control-permission-id", - } - remote = RemoteS3(None, config) - self.assertEqual( - remote.extra_args["GrantRead"], - "id=read-permission-id,id=other-read-permission-id", - ) - self.assertEqual( - remote.extra_args["GrantReadACP"], "id=read-acp-permission-id" - ) - self.assertEqual( - remote.extra_args["GrantWriteACP"], "id=write-acp-permission-id" - ) - self.assertEqual( - remote.extra_args["GrantFullControl"], - "id=full-control-permission-id", - ) +def test_init(): + config = {"url": url} + remote = RemoteS3(None, config) + + assert remote.path_info == url + + +def test_grants(): + config = { + "url": url, + "grant_read": "id=read-permission-id,id=other-read-permission-id", + "grant_read_acp": "id=read-acp-permission-id", + "grant_write_acp": "id=write-acp-permission-id", + "grant_full_control": "id=full-control-permission-id", + } + remote = RemoteS3(None, config) + + assert ( + remote.extra_args["GrantRead"] + == "id=read-permission-id,id=other-read-permission-id" + ) + assert remote.extra_args["GrantReadACP"] == "id=read-acp-permission-id" + assert remote.extra_args["GrantWriteACP"] == "id=write-acp-permission-id" + assert ( + remote.extra_args["GrantFullControl"] + == "id=full-control-permission-id" + ) + + +def test_grants_mutually_exclusive_acl_error(grants): + for grant_option, grant_value in grants.items(): + config = {"url": url, "acl": "public-read", grant_option: grant_value} + + with pytest.raises(ConfigError): + RemoteS3(None, config)