-
Notifications
You must be signed in to change notification settings - Fork 33
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
Ensure we are using pyopenssl (See ome/omero-py#240) #305
Conversation
As of (psf/requests#5443) released in requests 2.24.0 [1] the urllib3 monkeypatching in of pyopenssl now only happens if the SNI is unavailable (mostly just Python 2). The prevailing intent within the requests project seems to be to avoid pyopenssl use where possible. Obviously, this is not what we need so we're going to have to follow the requests and urllib3 advice to do it explictly. I have chosen to only perform the explicit injection for the upgrade check. Other uses of requests which want to avoid the sort of weird OpenSSL errors like those outlined in (ome#240) will need to do the same thing. 1. https://github.com/psf/requests/blob/main/HISTORY.md#2240-2020-06-17
Can be tested by upgrading to requests 2.24.0 or later and following the pattern outlined on #240. You will get the same or similar exception below (depending on your major Python and OpenSSL version) without injecting explicitly:
With injecting explicitly:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested the scenario described in the PR on our CI systems after installing pyopenssl
in the daily virtual environment. In addition to the minimal example, I tested the module patched by this change with the following example snippet:
>>> import omero
>>> client=omero.client('localhost')
>>> client.closeSession()
>>> from omero.util.upgrade_check import UpgradeCheck
>>> uc=UpgradeCheck('test', url='https://www.openmicroscopy.org')
>>> uc.run()
Without this PR, the last call throws SSLError(SSLError(336236785, '[SSL: UNABLE_TO_LOAD_SSL2_MD5_ROUTINES] unknown error (_ssl.c:2830)'
. With this PR included, the final call successfully returns the HTML content.
No objection from side to having this change in an imminent patch release. I'll leave the other requested reviewers to express their views.
As discussed with @sbesson out of band, the fact that this doesn't blow up spectacularly with every use of
|
All 👍 (The only other usage I see in omero-py is the download of jars of |
As of (psf/requests#5443) released in requests 2.24.0 [1] the urllib3
monkeypatching in of pyopenssl now only happens if the SNI is
unavailable (mostly just Python 2). The prevailing intent within
the requests project seems to be to avoid pyopenssl use where
possible. Obviously, this is not what we need so we're going to
have to follow the requests and urllib3 advice to do it explictly.
I have chosen to only perform the explicit injection for the upgrade
check. Other uses of requests which want to avoid the sort of weird
OpenSSL errors like those outlined in (#240) will need to do
the same thing.