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

Still need to support hostname verification #1020

Open
njsmith opened this issue May 18, 2021 · 5 comments
Open

Still need to support hostname verification #1020

njsmith opened this issue May 18, 2021 · 5 comments

Comments

@njsmith
Copy link
Contributor

njsmith commented May 18, 2021

It looks like #795 got closed prematurely. #933 made it so that you can skip setting a verify callback, and use OpenSSL's built-in verification functionality. And OpenSSL's built-in verification functionality can now verify hostnames properly on all supported OpenSSL versions. But..... you still have to turn this feature on. And pyopenssl still doesn't expose the APIs to do that. So something like #796 is still needed.

References:

@reaperhulk
Copy link
Member

@njsmith Do we need to expose more than one interface to do this here or is one sufficient? If one, which one?

@njsmith
Copy link
Contributor Author

njsmith commented May 19, 2021

Well, I guess openssl technically exposes a bunch of different interfaces to control different pieces:

  • you can set the expected host name, or add multiple expected host names
  • or, you can set the expected ip address -- that's a different function
  • there are a bunch of flags you can set to do things like get old/new-style wildcard matching, control handling of commonNames, etc.

So I guess we could do anything from just exposing the raw openssl knobs and letting people do whatever they want, to a high-level SSLConnection.set_host that takes care of setting up both the cert verification and SNI hostname, automatically handles IP addresses appropriately, and chooses an opinionated set of verification flags.

The stdlib basically took the latter approach, and that's the last link in my references above.

@mhils
Copy link
Member

mhils commented Feb 20, 2023

The downside of both raw OpenSSL knobs and a high-level Connection.set_host method is that both APIs are unsafe by default. Not all developers will be aware that set_host almost always needs to be called for clients to be secure, and things unfortunately do not break visibly if you omit it.1

One way to fix the API design issue is to add a server_hostname argument to the Connection constructor. If passed, pyOpenSSL defaults to VERIFY_PEER2 + OpenSSL hostname validation. We could do a long transition period in which a DeprecationWarning is raised if the argument is absent, and then later require it to be passed for all client connections. There probably needs to be a INSECURE_NO_CERTIFICATE_VERIFICATION/DANGEROUS_NO_HOSTNAME_CHECKING sentinel values that can be passed to disable VERIFY_PEER or only hostname checking explicitly.

Footnotes

  1. I guess the philosophical question is if pyOpenSSL should merely mirror the awfully insecure OpenSSL APIs, or if we have a duty to protect our users.

  2. I actually have no idea what the default verify mode is at the moment? Ìs it just terrible insecure, just as OpenSSL? If we default to VERIFY_PEER, what guidance do we give on providing default CAs?

@mhils
Copy link
Member

mhils commented Feb 20, 2023

Also FWIW, here is the hostname logic we have in mitmproxy - it's pretty similar to the CPython one, but uses cryptography bindings instead.

@glyph
Copy link
Contributor

glyph commented Jan 12, 2024

Twisted is in the same spot as mitmproxy here, and I think for us we'd just like to have the OpenSSL APIs exposed fairly literally, just a set_host and then some other stuff on top of it, so it is at least possible to build something secure at our layer, which is currently a real hodgepodge.

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

No branches or pull requests

4 participants