From b242ac02023475adade18bbd9afd29c0116749cc Mon Sep 17 00:00:00 2001 From: Nigel Date: Tue, 30 Mar 2021 15:28:34 +0200 Subject: [PATCH 1/8] Added SSL verification option --- dvc/config_schema.py | 1 + dvc/fs/s3.py | 2 ++ 2 files changed, 3 insertions(+) diff --git a/dvc/config_schema.py b/dvc/config_schema.py index 84ee1a1207..43b3e4092d 100644 --- a/dvc/config_schema.py +++ b/dvc/config_schema.py @@ -145,6 +145,7 @@ class RelPath(str): "session_token": str, Optional("listobjects", default=False): Bool, # obsoleted Optional("use_ssl", default=True): Bool, + Optional("verify", default=True): Bool, "sse": str, "sse_kms_key_id": str, "acl": str, diff --git a/dvc/fs/s3.py b/dvc/fs/s3.py index 17176058a3..9f4cbc3125 100644 --- a/dvc/fs/s3.py +++ b/dvc/fs/s3.py @@ -38,6 +38,7 @@ def __init__(self, repo, config): self.endpoint_url = config.get("endpointurl") self.use_ssl = config.get("use_ssl", True) + self.verify = config.get("verify", True) self.extra_args = {} @@ -136,6 +137,7 @@ def s3(self): return session.resource( "s3", endpoint_url=self.endpoint_url, + verify=self.verify, use_ssl=self.use_ssl, config=boto3.session.Config( signature_version="s3v4", s3=s3_config From 3c12ab96faa7cc60763b49224070bf2ce69c4c34 Mon Sep 17 00:00:00 2001 From: Nigel Date: Tue, 30 Mar 2021 17:25:29 +0200 Subject: [PATCH 2/8] Changed param name to avoid conflicts --- dvc/config_schema.py | 2 +- dvc/fs/s3.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dvc/config_schema.py b/dvc/config_schema.py index 43b3e4092d..14b361bf39 100644 --- a/dvc/config_schema.py +++ b/dvc/config_schema.py @@ -145,7 +145,7 @@ class RelPath(str): "session_token": str, Optional("listobjects", default=False): Bool, # obsoleted Optional("use_ssl", default=True): Bool, - Optional("verify", default=True): Bool, + Optional("verify_ssl", default=True): Bool, "sse": str, "sse_kms_key_id": str, "acl": str, diff --git a/dvc/fs/s3.py b/dvc/fs/s3.py index 9f4cbc3125..f803b5948e 100644 --- a/dvc/fs/s3.py +++ b/dvc/fs/s3.py @@ -38,7 +38,7 @@ def __init__(self, repo, config): self.endpoint_url = config.get("endpointurl") self.use_ssl = config.get("use_ssl", True) - self.verify = config.get("verify", True) + self.verify_ssl = config.get("verify_ssl", True) self.extra_args = {} @@ -137,7 +137,7 @@ def s3(self): return session.resource( "s3", endpoint_url=self.endpoint_url, - verify=self.verify, + verify=self.verify_ssl, use_ssl=self.use_ssl, config=boto3.session.Config( signature_version="s3v4", s3=s3_config From 8bac5394d1e08718779b368b17e2d96c8d2ad722 Mon Sep 17 00:00:00 2001 From: Nigel Date: Wed, 31 Mar 2021 09:22:24 +0200 Subject: [PATCH 3/8] verify_ssl now accepts strings --- dvc/config_schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/config_schema.py b/dvc/config_schema.py index 14b361bf39..ae72aee3d6 100644 --- a/dvc/config_schema.py +++ b/dvc/config_schema.py @@ -145,7 +145,7 @@ class RelPath(str): "session_token": str, Optional("listobjects", default=False): Bool, # obsoleted Optional("use_ssl", default=True): Bool, - Optional("verify_ssl", default=True): Bool, + Optional("verify_ssl", default="true"): str, "sse": str, "sse_kms_key_id": str, "acl": str, From 253c5aa7228ffb13772611ab49d96e75c4bb0eca Mon Sep 17 00:00:00 2001 From: Nigel Date: Wed, 31 Mar 2021 09:24:50 +0200 Subject: [PATCH 4/8] Added simple unit tests --- tests/unit/fs/test_s3.py | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/unit/fs/test_s3.py b/tests/unit/fs/test_s3.py index 5685342333..f318e38c82 100644 --- a/tests/unit/fs/test_s3.py +++ b/tests/unit/fs/test_s3.py @@ -29,6 +29,29 @@ def test_init(dvc): assert fs.path_info == url +def test_verify_ssl_default_param(dvc): + config = { + "url": url, + } + fs = S3FileSystem(dvc, config) + + assert fs.verify_ssl + + +def test_verify_ssl_bool_param(dvc): + config = {"url": url, "verify_ssl": False} + fs = S3FileSystem(dvc, config) + + assert fs.verify_ssl == config["verify_ssl"] + + +def test_verify_ssl_str_param(dvc): + config = {"url": url, "verify_ssl": "path/to/certs"} + fs = S3FileSystem(dvc, config) + + assert fs.verify_ssl == config["verify_ssl"] + + def test_grants(dvc): config = { "url": url, From 1f94153bfa0ada1876f845eada0d7f97ec2fee71 Mon Sep 17 00:00:00 2001 From: Nigel Date: Wed, 31 Mar 2021 10:33:11 +0200 Subject: [PATCH 5/8] Changed parameter name fron verify_ssl to ssl_verify so it's consistent with HTTP remotes Forget to change 1 test --- dvc/config_schema.py | 2 +- dvc/fs/s3.py | 4 ++-- tests/unit/fs/test_s3.py | 14 +++++++------- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/dvc/config_schema.py b/dvc/config_schema.py index ae72aee3d6..96b67db2f6 100644 --- a/dvc/config_schema.py +++ b/dvc/config_schema.py @@ -145,7 +145,7 @@ class RelPath(str): "session_token": str, Optional("listobjects", default=False): Bool, # obsoleted Optional("use_ssl", default=True): Bool, - Optional("verify_ssl", default="true"): str, + Optional("ssl_verify", default="true"): str, "sse": str, "sse_kms_key_id": str, "acl": str, diff --git a/dvc/fs/s3.py b/dvc/fs/s3.py index f803b5948e..f201ffeb58 100644 --- a/dvc/fs/s3.py +++ b/dvc/fs/s3.py @@ -38,7 +38,7 @@ def __init__(self, repo, config): self.endpoint_url = config.get("endpointurl") self.use_ssl = config.get("use_ssl", True) - self.verify_ssl = config.get("verify_ssl", True) + self.ssl_verify = config.get("ssl_verify", True) self.extra_args = {} @@ -137,7 +137,7 @@ def s3(self): return session.resource( "s3", endpoint_url=self.endpoint_url, - verify=self.verify_ssl, + verify=self.ssl_verify, use_ssl=self.use_ssl, config=boto3.session.Config( signature_version="s3v4", s3=s3_config diff --git a/tests/unit/fs/test_s3.py b/tests/unit/fs/test_s3.py index f318e38c82..5f36e4992e 100644 --- a/tests/unit/fs/test_s3.py +++ b/tests/unit/fs/test_s3.py @@ -35,21 +35,21 @@ def test_verify_ssl_default_param(dvc): } fs = S3FileSystem(dvc, config) - assert fs.verify_ssl + assert fs.ssl_verify -def test_verify_ssl_bool_param(dvc): - config = {"url": url, "verify_ssl": False} +def test_ssl_verify_bool_param(dvc): + config = {"url": url, "ssl_verify": False} fs = S3FileSystem(dvc, config) - assert fs.verify_ssl == config["verify_ssl"] + assert fs.ssl_verify == config["ssl_verify"] -def test_verify_ssl_str_param(dvc): - config = {"url": url, "verify_ssl": "path/to/certs"} +def test_ssl_verify_str_param(dvc): + config = {"url": url, "ssl_verify": "path/to/certs"} fs = S3FileSystem(dvc, config) - assert fs.verify_ssl == config["verify_ssl"] + assert fs.ssl_verify == config["ssl_verify"] def test_grants(dvc): From 75966c88f1b720d8a67624c48265d06de07c5274 Mon Sep 17 00:00:00 2001 From: Nigel Date: Wed, 31 Mar 2021 14:19:13 +0200 Subject: [PATCH 6/8] add processing on ssl_verify param typo --- dvc/fs/s3.py | 18 +++++++++++++++++- tests/unit/fs/test_s3.py | 19 ++++++++++++++++++- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/dvc/fs/s3.py b/dvc/fs/s3.py index f201ffeb58..43c1fe7e37 100644 --- a/dvc/fs/s3.py +++ b/dvc/fs/s3.py @@ -2,6 +2,7 @@ import os import threading from contextlib import contextmanager +from distutils.util import strtobool from funcy import cached_property, wrap_prop @@ -20,6 +21,19 @@ _AWS_CONFIG_PATH = os.path.join(os.path.expanduser("~"), ".aws", "config") +def process_ssl_verify_param(ssl_verify_config_value): + """ + Checks the type of the input parameter and returns the + boolean or certPath based on the input + """ + if isinstance(ssl_verify_config_value, bool): + return ssl_verify_config_value + try: + return strtobool(ssl_verify_config_value) + except ValueError: + return ssl_verify_config_value + + class S3FileSystem(BaseFileSystem): scheme = Schemes.S3 PATH_CLS = CloudURLInfo @@ -38,7 +52,9 @@ def __init__(self, repo, config): self.endpoint_url = config.get("endpointurl") self.use_ssl = config.get("use_ssl", True) - self.ssl_verify = config.get("ssl_verify", True) + self.ssl_verify = process_ssl_verify_param( + config.get("ssl_verify", True) + ) self.extra_args = {} diff --git a/tests/unit/fs/test_s3.py b/tests/unit/fs/test_s3.py index 5f36e4992e..042e81c6d5 100644 --- a/tests/unit/fs/test_s3.py +++ b/tests/unit/fs/test_s3.py @@ -2,7 +2,7 @@ from dvc.config import ConfigError from dvc.exceptions import DvcException -from dvc.fs.s3 import S3FileSystem +from dvc.fs.s3 import S3FileSystem, process_ssl_verify_param bucket_name = "bucket-name" prefix = "some/prefix" @@ -135,3 +135,20 @@ def test_get_bucket(): with pytest.raises(DvcException, match="Bucket 'mybucket' does not exist"): with fs._get_bucket("mybucket") as bucket: raise bucket.meta.client.exceptions.NoSuchBucket({}, None) + + +@pytest.mark.parametrize( + "ssl_verify_value, expected_output", + [ + (True, True), + (False, False), + ("True", True), + ("true", True), + ("false", False), + ("False", False), + ("path/to/cert", "path/to/cert"), + ("someString", "someString"), + ], +) +def test_process_ssl_verify_param(ssl_verify_value, expected_output): + assert process_ssl_verify_param(ssl_verify_value) == expected_output From a2f71c0c3649f945b3f7b7f6daa1e1673a5cf0e4 Mon Sep 17 00:00:00 2001 From: Nigel Date: Wed, 31 Mar 2021 22:11:51 +0200 Subject: [PATCH 7/8] revert back to bool param --- dvc/config_schema.py | 2 +- dvc/fs/s3.py | 18 +----------------- tests/unit/fs/test_s3.py | 26 +------------------------- 3 files changed, 3 insertions(+), 43 deletions(-) diff --git a/dvc/config_schema.py b/dvc/config_schema.py index 96b67db2f6..783d8f202f 100644 --- a/dvc/config_schema.py +++ b/dvc/config_schema.py @@ -145,7 +145,7 @@ class RelPath(str): "session_token": str, Optional("listobjects", default=False): Bool, # obsoleted Optional("use_ssl", default=True): Bool, - Optional("ssl_verify", default="true"): str, + Optional("ssl_verify", default=True): bool, "sse": str, "sse_kms_key_id": str, "acl": str, diff --git a/dvc/fs/s3.py b/dvc/fs/s3.py index 43c1fe7e37..f201ffeb58 100644 --- a/dvc/fs/s3.py +++ b/dvc/fs/s3.py @@ -2,7 +2,6 @@ import os import threading from contextlib import contextmanager -from distutils.util import strtobool from funcy import cached_property, wrap_prop @@ -21,19 +20,6 @@ _AWS_CONFIG_PATH = os.path.join(os.path.expanduser("~"), ".aws", "config") -def process_ssl_verify_param(ssl_verify_config_value): - """ - Checks the type of the input parameter and returns the - boolean or certPath based on the input - """ - if isinstance(ssl_verify_config_value, bool): - return ssl_verify_config_value - try: - return strtobool(ssl_verify_config_value) - except ValueError: - return ssl_verify_config_value - - class S3FileSystem(BaseFileSystem): scheme = Schemes.S3 PATH_CLS = CloudURLInfo @@ -52,9 +38,7 @@ def __init__(self, repo, config): self.endpoint_url = config.get("endpointurl") self.use_ssl = config.get("use_ssl", True) - self.ssl_verify = process_ssl_verify_param( - config.get("ssl_verify", True) - ) + self.ssl_verify = config.get("ssl_verify", True) self.extra_args = {} diff --git a/tests/unit/fs/test_s3.py b/tests/unit/fs/test_s3.py index 042e81c6d5..030de7a027 100644 --- a/tests/unit/fs/test_s3.py +++ b/tests/unit/fs/test_s3.py @@ -2,7 +2,7 @@ from dvc.config import ConfigError from dvc.exceptions import DvcException -from dvc.fs.s3 import S3FileSystem, process_ssl_verify_param +from dvc.fs.s3 import S3FileSystem bucket_name = "bucket-name" prefix = "some/prefix" @@ -45,13 +45,6 @@ def test_ssl_verify_bool_param(dvc): assert fs.ssl_verify == config["ssl_verify"] -def test_ssl_verify_str_param(dvc): - config = {"url": url, "ssl_verify": "path/to/certs"} - fs = S3FileSystem(dvc, config) - - assert fs.ssl_verify == config["ssl_verify"] - - def test_grants(dvc): config = { "url": url, @@ -135,20 +128,3 @@ def test_get_bucket(): with pytest.raises(DvcException, match="Bucket 'mybucket' does not exist"): with fs._get_bucket("mybucket") as bucket: raise bucket.meta.client.exceptions.NoSuchBucket({}, None) - - -@pytest.mark.parametrize( - "ssl_verify_value, expected_output", - [ - (True, True), - (False, False), - ("True", True), - ("true", True), - ("false", False), - ("False", False), - ("path/to/cert", "path/to/cert"), - ("someString", "someString"), - ], -) -def test_process_ssl_verify_param(ssl_verify_value, expected_output): - assert process_ssl_verify_param(ssl_verify_value) == expected_output From 356f9355fd5156595cfd4c786231ce2f04a5e046 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Wed, 31 Mar 2021 23:13:53 +0300 Subject: [PATCH 8/8] Update dvc/config_schema.py --- dvc/config_schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/config_schema.py b/dvc/config_schema.py index 783d8f202f..41e9d4efbc 100644 --- a/dvc/config_schema.py +++ b/dvc/config_schema.py @@ -145,7 +145,7 @@ class RelPath(str): "session_token": str, Optional("listobjects", default=False): Bool, # obsoleted Optional("use_ssl", default=True): Bool, - Optional("ssl_verify", default=True): bool, + Optional("ssl_verify", default=True): Bool, "sse": str, "sse_kms_key_id": str, "acl": str,