Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

config, remote: Made S3 CA bundle customizable #6018

Merged
merged 2 commits into from Jun 1, 2021

Conversation

rgvanwesep
Copy link
Contributor

@rgvanwesep rgvanwesep commented May 16, 2021

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 (see https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html) 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 additional 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 DVC 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

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

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 iterative#6012
@pmrowla pmrowla requested a review from isidentical May 17, 2021 02:23
@pmrowla
Copy link
Contributor

pmrowla commented May 17, 2021

@rgvanwesep this PR will require a docs update - in the Click for Amazon S3 section in https://dvc.org/doc/command-reference/remote/modify#remote-modify should note that ssl_verify can be set to true/false or a ca_bundle path, and that if unset, we will fall back to verifying certificates using the AWS CLI config's ca_bundle (if it exists)

See https://dvc.org/doc/user-guide/contributing/docs#submitting-changes for info on submitting the docs PR, the file you need to update is: https://github.com/iterative/dvc.org/blob/master/content/docs/command-reference/remote/modify.md

@pmrowla pmrowla self-requested a review May 17, 2021 02:27
@skshetry skshetry requested a review from efiop May 17, 2021 04:01
@rgvanwesep
Copy link
Contributor Author

@pmrowla Thanks for pointing that out. I should be able to put up the docs PR today.

rgvanwesep added a commit to rgvanwesep/dvc.org that referenced this pull request May 17, 2021
iterative/dvc#6018 implements the ability to set `ssl_verify` in the S3 remote config to a path to a custom CA bundle file in addition to setting true/false. It also makes the default the same as the `botocore` default, which is to read the CA bundle path from the AWS config. This updates the docs to reflect those changes.
@rgvanwesep
Copy link
Contributor Author

The doc PR:
iterative/dvc.org#2483

@@ -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=None): Any(Bool, str, None),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could just

Suggested change
Optional("ssl_verify", default=None): Any(Bool, str, None),
"ssl_verify": Any(Bool, str, None),

since they are optional by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense. And at that point I think I can get rid of the None so that it is "ssl_verify": Any(Bool, str),. I'm testing the change now and should be able to push soon.

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.
@efiop efiop merged commit 59cfc2c into iterative:master Jun 1, 2021
jorgeorpinel added a commit to iterative/dvc.org that referenced this pull request Jun 2, 2021
* Updated S3 ssl_verify documentation

iterative/dvc#6018 implements the ability to set `ssl_verify` in the S3 remote config to a path to a custom CA bundle file in addition to setting true/false. It also makes the default the same as the `botocore` default, which is to read the CA bundle path from the AWS config. This updates the docs to reflect those changes.

* Update content/docs/command-reference/remote/modify.md

* Update content/docs/command-reference/remote/modify.md

* Apply suggestions from code review

Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>
@efiop efiop added the feature is a feature label Jun 2, 2021
@skshetry
Copy link
Member

skshetry commented Jun 9, 2021

I thought this added support for http(s) as well. Looking into it. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature is a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ssl_verify key in remote config does not accept custom CA bundle path and aws config is ignored
4 participants