From 59cfc2c4a43e9b9d2fdb583da80d24e82ae39c26 Mon Sep 17 00:00:00 2001 From: rgvanwesep Date: Tue, 1 Jun 2021 06:17:45 -0700 Subject: [PATCH] config, remote: Made S3 CA bundle customizable (#6018) * config, remote: Made S3 CA bundle customizable botocore allows a path to a custom CA bundle either by passing a path to the CA bundle file into the verify argument of boto3.session.Session.client or passing None (the default) which will fall back to the AWS config. Previously, the DVC config only accepted a boolean into the ssl_verify option in the remote S3 config. This changes the DVC config to accept both string and None in addition to boolean and defaults to None. I also changed the default for ssl_verfiy to None in BaseS3FileSystem. Thus, if ssl_verify is not provided, botocore will fall back to the AWS config. Testing Unit tests to cover the changes to the config schema and addition ssl_verify types that will be passed into S3FileSystem. Also, ran dvc push -r object-store data/cifar-10-python.tar.gz in my work environment that has a private S3 endpoint that requires a custom CA bundle, both with and without ssl_verify specified in the config. This was successful, showing that communication could be established. And I ran dvc remote modify object-store ssl_verify "$HOME/.aws/cabundle.pem" and confirmed that the custom CA bundle path was added to the config. Fixes #6012 * Removed default None on ssl_verify Responding to PR comment, removed the Optional, default None on ssl_verify since the config keys are optional by default. Rather than a missing ssl_verify producing a None that eventually gets filtered, it doesn't appear in the parsed config in the first place. Co-authored-by: Robert Van Wesep --- dvc/config_schema.py | 2 +- dvc/fs/s3.py | 2 +- tests/unit/fs/test_s3.py | 33 ++++++++++++++++++++++++++++++++- tests/unit/test_config.py | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 3 deletions(-) diff --git a/dvc/config_schema.py b/dvc/config_schema.py index 19bec8d1d..abfd7c34c 100644 --- a/dvc/config_schema.py +++ b/dvc/config_schema.py @@ -145,7 +145,7 @@ SCHEMA = { "session_token": str, Optional("listobjects", default=False): Bool, # obsoleted Optional("use_ssl", default=True): Bool, - Optional("ssl_verify", default=True): Bool, + "ssl_verify": Any(Bool, str), "sse": str, "sse_kms_key_id": str, "acl": str, diff --git a/dvc/fs/s3.py b/dvc/fs/s3.py index 7dc74c0fa..40bcb1756 100644 --- a/dvc/fs/s3.py +++ b/dvc/fs/s3.py @@ -103,7 +103,7 @@ class BaseS3FileSystem(ObjectFSWrapper): client = login_info["client_kwargs"] client["region_name"] = config.get("region") client["endpoint_url"] = config.get("endpointurl") - client["verify"] = config.get("ssl_verify", True) + client["verify"] = config.get("ssl_verify") # encryptions additional = login_info["s3_additional_kwargs"] diff --git a/tests/unit/fs/test_s3.py b/tests/unit/fs/test_s3.py index 5749d89e4..ba3f0aaec 100644 --- a/tests/unit/fs/test_s3.py +++ b/tests/unit/fs/test_s3.py @@ -37,7 +37,15 @@ def test_verify_ssl_default_param(dvc): config = {"url": url} fs = S3FileSystem(**config) - assert fs.fs_args["client_kwargs"]["verify"] + assert "client_kwargs" not in fs.fs_args + + config = { + "url": url, + "endpointurl": "https://my.custom.s3:1234", + } + fs = S3FileSystem(**config) + + assert "verify" not in fs.fs_args["client_kwargs"] def test_s3_config_credentialpath(dvc, monkeypatch): @@ -72,6 +80,29 @@ def test_ssl_verify_bool_param(dvc): assert fs.fs_args["client_kwargs"]["verify"] == config["ssl_verify"] +def test_ssl_verify_path_param(dvc): + config = {"url": url, "ssl_verify": "/path/to/custom/cabundle.pem"} + fs = S3FileSystem(**config) + + assert fs.fs_args["client_kwargs"]["verify"] == config["ssl_verify"] + + +def test_ssl_verify_none_param(dvc): + config = {"url": url, "ssl_verify": None} + fs = S3FileSystem(**config) + + assert "client_kwargs" not in fs.fs_args + + config = { + "url": url, + "endpointurl": "https://my.custom.s3:1234", + "ssl_verify": None, + } + fs = S3FileSystem(**config) + + assert "verify" not in fs.fs_args["client_kwargs"] + + def test_grants(dvc): config = { "url": url, diff --git a/tests/unit/test_config.py b/tests/unit/test_config.py index 4e8164f4b..a12a9992e 100644 --- a/tests/unit/test_config.py +++ b/tests/unit/test_config.py @@ -1,4 +1,5 @@ import os +import textwrap import pytest @@ -33,3 +34,39 @@ def test_get_fs(tmp_dir, scm): assert config._get_fs("local") == config.wfs assert config._get_fs("global") == config.wfs assert config._get_fs("system") == config.wfs + + +def test_s3_ssl_verify(tmp_dir, dvc): + config = Config(validate=False) + with config.edit() as conf: + conf["remote"]["remote-name"] = {"url": "s3://bucket/dvc"} + + assert "ssl_verify" not in config["remote"]["remote-name"] + + with config.edit() as conf: + section = conf["remote"]["remote-name"] + section["ssl_verify"] = False + + assert (tmp_dir / ".dvc" / "config").read_text() == textwrap.dedent( + """\ + [core] + no_scm = True + ['remote "remote-name"'] + url = s3://bucket/dvc + ssl_verify = False + """ + ) + + with config.edit() as conf: + section = conf["remote"]["remote-name"] + section["ssl_verify"] = "/path/to/custom/cabundle.pem" + + assert (tmp_dir / ".dvc" / "config").read_text() == textwrap.dedent( + """\ + [core] + no_scm = True + ['remote "remote-name"'] + url = s3://bucket/dvc + ssl_verify = /path/to/custom/cabundle.pem + """ + )