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

OpenSSL.SSL.Error raised with invalid client cert and openssl 1.1.1. #4961

Closed
jaraco opened this issue Feb 4, 2019 · 15 comments
Closed

OpenSSL.SSL.Error raised with invalid client cert and openssl 1.1.1. #4961

jaraco opened this issue Feb 4, 2019 · 15 comments

Comments

@jaraco
Copy link
Contributor

jaraco commented Feb 4, 2019

In cherrypy/cheroot#173, we discovered that with the release of cryptography 2.5, built against OpenSSL 1.1.1a, a call to requests.get with a client certificate referencing an unknown CA will raise an OpenSSL.SSL.Error not wrapped in a requests.exceptions.SSLError, whereas with cryptography < 2.5 (openssl 1.1.0), the error is wrapped.

Due to the complexities of this situation, I've not yet been able to put together an isolated test case that replicates the issue. It's possible there are other factors in the cheroot test suite that are affecting this behavior.

I wanted to raise this issue here to see if the requests maintainers have some insight into this issue - either how the situation above might arise or how one might be able to simply replicate the conditions that lead to situation so further investigation could be done. Any advice is appreciated.

@jaraco jaraco changed the title OpenSSL.SSL.Error passes with client cert and openssl 1.1.1. OpenSSL.SSL.Error raised with invalid client cert and openssl 1.1.1. Feb 4, 2019
@sethmlarson
Copy link
Member

This is probably something for urllib3 instead of requests?

@sigmavirus24
Copy link
Contributor

@sethmlarson so it's plausible urllib3 isn't catching/re-raising that exception appropriately. It sounds like we're also missing the bit where pyOpenSSL is used here (as that's the only time an OpenSSL.SSL.Error would occur. Once urllib3 raises the right error, we should be able to handle it and raise requests.exceptions.SSLError.

Further, we have no information beyond cryptography and openssl, so we don't know which versions of requests/urllib3/pyOpenSSl are affected by this or how we might even begin investigating this.

@jaraco
Copy link
Contributor Author

jaraco commented Feb 5, 2019

In the downstream issue, the issue occurs with pyOpenSSL 18 and 19. The version of requests used is 2.21.0. I'd love to provide a minimal example, but I've yet been unable to do so. Here's a complete set of dependencies that were present when the issue occurred, which we used to determine that the version of cryptography is the key factor in eliciting the behavior.

In the original report, you can see the stack trace when the pyOpenSSL error is raised and requests and urllib are in the trace. Does that provide a hint as to where the issue might lie?

I'm hoping to avoid putting together an isolated test case due to the complexities of setting up a suitable TLS-enabled web server with client certificate support (I did try using badssl.com, but couldn't replicate the issue there).

@sigmavirus24
Copy link
Contributor

So looking at master briefly, it seems we're not handling any OpenSSL errors in https://github.com/urllib3/urllib3/blob/4239a6d9edbc617439726c7e38cdaf7b24725a29/src/urllib3/connectionpool.py#L597 or in _make_request? I'm not sure if we need to add that handle, or how we've missed it thus far.

@sigmavirus24
Copy link
Contributor

But that's in urllib3's stack so 🤷‍♂️

@sethmlarson
Copy link
Member

sethmlarson commented Feb 5, 2019

That's what I noticed too. I wonder if this is due to TLS 1.3 doing certificate verification differently than versions before it? Either way, us not handling that error as a retryable error is not good.

@jaraco
Copy link
Contributor Author

jaraco commented Feb 5, 2019

But that's in urllib3's stack so

...we should file the ticket upstream? Or maybe just move this ticket to that project (requires admin on one or both)?

@sethmlarson
Copy link
Member

Can't move issues cross-org (yet?) could you reopen an issue and include the exact Python, pyOpenSSL, cryptography versions and cert you're using so I can reproduce exactly what you're seeing? I appreciate your effort looking into this issue.

@webknjaz
Copy link

webknjaz commented Apr 8, 2019

FTR urllib3/urllib3#1496 must've fixed this. But needs validation...

@webknjaz
Copy link

FTR urllib==1.24.2 has been released 5 days ago.

@sethmlarson
Copy link
Member

This should be fixed in urllib3==1.25.

@webknjaz
Copy link

@sethmlarson not in 1.24.2? I thought it's there...

@sethmlarson
Copy link
Member

Nope, 1.24.2 didn't contain any new features only three fixes on the 1.24.x releases.

@webknjaz
Copy link

Oh, thanks for the clarification!

@nateprewitt
Copy link
Member

This was resolved in urllib v1.25.0.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants