config, remote: Made S3 CA bundle customizable#6018
Merged
Conversation
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 treeverse#6012
Contributor
|
@rgvanwesep this PR will require a docs update - in the 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 |
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
treeverse/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.
Contributor
Author
|
The doc PR: |
efiop
reviewed
May 19, 2021
| 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), |
Contributor
There was a problem hiding this comment.
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.
Contributor
Author
There was a problem hiding this comment.
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.
pmrowla
approved these changes
May 20, 2021
jorgeorpinel
added a commit
to treeverse/dvc.org
that referenced
this pull request
Jun 2, 2021
* Updated S3 ssl_verify documentation treeverse/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>
Collaborator
|
I thought this added support for http(s) as well. Looking into it. :) |
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
botocoreallows a path to a custom CA bundle either by passing a path to the CA bundle file into the verify argument ofboto3.session.Session.client(see https://boto3.amazonaws.com/v1/documentation/api/latest/reference/core/session.html) or passingNone(the default) which will fall back to the AWS config. Previously, the DVC config only accepted aboolean into the
ssl_verifyoption in the remote S3 config. This changes the DVC config to accept both string andNonein addition to boolean and defaults toNone. I also changed the default forssl_verfiytoNoneinBaseS3FileSystem. Thus, ifssl_verifyis not provided,botocorewill fall back to the AWS config.Testing
Unit tests to cover the changes to the config schema and additional
ssl_verifytypes that will be passed intoS3FileSystem. Also, ranin my work environment that has a private S3 endpoint that requires a custom CA bundle, both with and without
ssl_verifyspecified in the DVC config. This was successful, showing that communication could be established. And I ranand confirmed that the custom CA bundle path was added to the config.
Fixes #6012
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
config, remote: Made S3 CA bundle customizable #6018
Thank you for the contribution - we'll try to review it as soon as possible. 🙏