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

check for CA Bundle when True is specified for verify #2339

Closed
koelemay opened this issue Apr 8, 2021 · 8 comments
Closed

check for CA Bundle when True is specified for verify #2339

koelemay opened this issue Apr 8, 2021 · 8 comments
Assignees
Labels
feature-request This issue requests a feature. p2 This is a standard priority issue

Comments

@koelemay
Copy link

koelemay commented Apr 8, 2021

Describe the bug

The verify argument when creating a client is documented to be of type boolean/string and defaults to None. While the longer description explains that it can take on a value of False or a path to a CA bundle, it currently does not allow you to pass a value of True -- in the case that True is passed, the current implementation will not check the ca_bundle configuration variable here.

This is exacerbated when using other tools such as MLFlow and DVC that allow you to swap out storage interfaces. In such tools you can also set a verify parameter to True, False or path/to/ca/bundle however you cannot specify None. In this case it is impossible to use Boto3 when a CA Bundle is required to be used.

The solution to this issue is extremely simple, just checking for verify being either None or True here

Steps to reproduce
If you're behind a corporate firewall that requires custom cert bundle, try to create a session with verify set to True and it will not load the CA bundle from the env vars.

Expected behavior
When a user passes True it should work the same as passing None

Debug logs

@koelemay koelemay added the needs-triage This issue or PR still needs to be triaged. label Apr 8, 2021
@kdaily
Copy link
Member

kdaily commented Apr 8, 2021

Hi @koelemay, thanks for posting. Can you provide a more concrete example of how you are configuring MLFlow or DVC to use boto3 and the verify parameter? The MLFlow documentation shows to use the AWS_CA_BUNDLE environment variable in the case of using a custom certificate bundle:

https://mlflow.org/docs/latest/tracking.html#amazon-s3-and-s3-compatible-storage

If the MinIO server is configured with using SSL self-signed or signed using some internal-only CA certificate, you could set MLFLOW_S3_IGNORE_TLS or AWS_CA_BUNDLE variables (not both at the same time!) to disable certificate signature check, or add a custom CA bundle to perform this check, respectively:

export MLFLOW_S3_IGNORE_TLS=true
#or
export AWS_CA_BUNDLE=/some/ca/bundle.pem

@kdaily kdaily added guidance Question that needs advice or information. response-requested Waiting on additional info and feedback. and removed needs-triage This issue or PR still needs to be triaged. labels Apr 8, 2021
@koelemay
Copy link
Author

koelemay commented Apr 8, 2021

Hi @kdaily , happy to discuss further.

DVC creates a Boto3 session here:
https://github.com/iterative/dvc/blob/master/dvc/fs/s3.py#L137

Using the object's ssl_verify attribute which it pulls from DVC config here:
https://github.com/iterative/dvc/blob/master/dvc/fs/s3.py#L41

That is forced to be a boolean, validated by the config schema here:
https://github.com/iterative/dvc/blob/master/dvc/config_schema.py#L148

That defaults to True, which is the desired value. The path to our CA Bundle is specified in an environment variable, REQUESTS_CA_BUNDLE.

However, because verify is True (and not None), when it gets to here, Boto does not check the env vars for a CA bundle and defaults to the system one, which in my current environment is provided by certifi.

So a couple things:

  1. As a quick test, if I overwrite our enterprise CA Bundle over the certifi bundle, it works fine
  2. If I modify the DVC source code such that it allows me to use None for ssl_verify then it works fine (because Boto gets None instead of True)

MLFlow exhibits the same behavior, as documented in a proposed PR. In that case they were considering updating MLFlow to set the default value of verify to None.

Rather than updating all downstream tools, they would all work if Boto accepted True as a functional value for verify, so we are hoping to take this path instead.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. label Apr 8, 2021
@kdaily kdaily self-assigned this Apr 9, 2021
@kdaily kdaily added feature-request This issue requests a feature. and removed guidance Question that needs advice or information. labels May 3, 2021
@kdaily
Copy link
Member

kdaily commented May 3, 2021

Thanks for the details. Marking this as a feature request for now, but need to do some more research on your use case and what impact it could have.

@kdaily kdaily added the investigating This issue is being investigated and/or work is in progress to resolve the issue. label May 3, 2021
@kdaily kdaily removed the investigating This issue is being investigated and/or work is in progress to resolve the issue. label Aug 27, 2021
@RyanFitzSimmonsAK RyanFitzSimmonsAK added the p2 This is a standard priority issue label Nov 4, 2022
@nngo
Copy link

nngo commented Mar 15, 2023

I encounter this same issue passing verify=True using boto3 1.24.28 and botocore 1.27.59, where I was expecting it to use the CA bundle from REQUESTS_CA_BUNDLE or AWS_CA_BUNDLE env, but it did not. Please put in the suggest simple fix to support True (and None) to be the same behavior

@stephanbertl
Copy link

stephanbertl commented Nov 10, 2023

Any chance we can get the PR merged soon? We are experiencing the same issue. boto never looks at AWS_CA_BUNDLE

@lhoss
Copy link

lhoss commented Mar 4, 2024

Plz merge this PR !
Also have a script where with configurable verify=boolean config value from a settings file (because for local testing need to set verify=false where an ssh tunnel needs to be used)
I could never imagine this "True" (boolean) value to be unsupported (with custom ca certs setting)

( I wasted at least half a day debugging ( initially updating custom local docker images, assuming incomplete root/intermed. cacerts ) .. until I delta-debugged , and removed the verify flag alltogether , and that worked)

@RyanFitzSimmonsAK
Copy link
Contributor

Hey everyone, thanks for opening this feature request and the discussion. This is not a change we're likely to make at this time, and I'm going to close the issue accordingly. This change affects certificate precedence, which is a breaking change. We're closing the pull request for similar reasons. Thanks again for contributing.

@RyanFitzSimmonsAK RyanFitzSimmonsAK closed this as not planned Won't fix, can't repro, duplicate, stale Mar 13, 2024
Copy link

This issue is now closed. Comments on closed issues are hard for our team to see.
If you need more assistance, please open a new issue that references this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request This issue requests a feature. p2 This is a standard priority issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants