8000 Breaking change in TLS Handling in 7.3.3 · Issue #201 · arangodb/python-arango · GitHub
[go: up one dir, main page]
Skip to content

Breaking change in TLS Handling in 7.3.3 #201

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.

Sign up for GitHub

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

Jump to bottom
Closed
aquamatthias opened this issue May 4, 2022 · 7 comments · Fixed by #202
Closed

Breaking change in TLS Handling in 7.3.3 #201

aquamatthias opened this issue May 4, 2022 · 7 comments · Fixed by #202

Comments

@aquamatthias
Copy link
Contributor
aquamatthias commented May 4, 2022 •
edited
Loading

After upgrading python-arango from 7.3.1 to 7.3.3 we are no longer able to connect to arangodb servers with a TLS setup using a custom CA certificate.
The related change: #199 overrides the verify flag of the HTTP session with a boolean value and does not respect what has been defined before.

We followed the instructions on the docs page to set up our own HttpClient: https://docs.python-arango.com/en/main/http.html
and provide the CA bundle to the session via the verify property. See code ArangoDBClient

In order to fix the behavior I would suggest one of the following:

  • remove the verify_certificate argument, since the same goal can be accomplished using the HttpClient
  • make the verify_certificate argument optional and default to None

I am happy to provide a fix if you agree with one of the suggestions.
@cw00dw0rd
Copy link
Contributor

Thanks for bringing this up and I would vote for the second option as adding this option has been frequently requested.
It would be great to add support for supplying a bundle or just supplying a boolean as you pointed out.

@lloesche
Copy link
lloesche commented May 4, 2022

requests.Session's verify is not a bool. It can be either that or a string containing path to a CA_BUNDLE file or directory with certificates of trusted CAs. Not super intuitive.

@cw00dw0rd
Copy link
Contributor

I agree it isn't super intuitive from the link you provided and here it seems to be either a boolean or path to cert bundle.

I think defining it similar to @aquamatthias's supplied example verify: Union[str, bool, None] seems like a good approach. WDYT?

@cw00dw0rd
Copy link
Contributor

Based on the above links Requests defaults verify to True and this seems like the more secure option as well. Should we instead allow for it to remain True but be optional? Open to it being None but that seems to go against the expectation of using the requests library, WDYT?

@aquamatthias
Copy link
Contributor Author
aquamatthias commented May 4, 2022 •
edited
Loading

The issue with the argument in that position: if defined it overrides everything that has been set at session level. That is why a default value other than None (no matter which one) is not ideal. So I would propose the following:

def __init__(...verify_override: Union[str, bool, None] = None):
   if verify_override is not None:
      for session in self._sessions:
         session.verify = verify_override

It still would allow for easy disabling of tls validation via verify_override=False or enable or custom bundle path,
but would not break existing sessions if the value is not provided.

wdyt?

@cw00dw0rd
Copy link
Contributor

That is a good point, your suggested solution works for me. Thanks again for pointing it out!

@aquamatthias
Copy link
Contributor Author

Great. I will provide a PR.

@aquamatthias aquamatthias mentioned this issue May 5, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Fix] Make overriding TLS verification optional aquamatthias/python-arango
3 participants
@aquamatthias @lloesche @cw00dw0rd
0