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

urllib & urllib2 may fail ssl handshaking under a variety of circumstances #3193

Closed
ZacVawter opened this issue Oct 20, 2017 · 10 comments
Closed

Comments

@ZacVawter
Copy link

I'm not sure of the root cause down in the depths of SSL. It somewhere there is improper use of SSL in (python 2.7, ubuntu 14.04, or my compiled version of openSSL).

If desired, I can provide a patch or pull request that changes the model_zoo.py over to use requests which properly handles this situation, and is aparently the 'recommended' library for python now (instead of urllib or urllib2).

`>>> import torchvision

net = torchvision.models.vgg16(pretrained=True)
Downloading: "https://download.pytorch.org/models/vgg16-397923af.pth" to /home/vawter/.torch/models/vgg16-397923af.pth
Traceback (most recent call last):
File "", line 1, in
File "/home/vawter/.cache/bazel/_bazel_vawter/8a8977c0d8b4d7d21d82bcdeeaf06b5c/execroot/source/bazel-out/release_links/lib/python/torchvision/models/vgg.py", line 142, in vgg16
model.load_state_dict(model_zoo.load_url(model_urls['vgg16']))
File "/home/vawter/.cache/bazel/_bazel_vawter/8a8977c0d8b4d7d21d82bcdeeaf06b5c/execroot/source/bazel-out/release_links/lib/python/torch/utils/model_zoo.py", line 57, in load_url
_download_url_to_file(url, cached_file, hash_prefix)
File "/home/vawter/.cache/bazel/_bazel_vawter/8a8977c0d8b4d7d21d82bcdeeaf06b5c/execroot/source/bazel-out/release_links/lib/python/torch/utils/model_zoo.py", line 62, in _download_url_to_file
u = urlopen(url)
File "/usr/lib/python2.7/urllib2.py", line 127, in urlopen
return _opener.open(url, data, timeout)
File "/usr/lib/python2.7/urllib2.py", line 404, in open
response = self._open(req, data)
File "/usr/lib/python2.7/urllib2.py", line 422, in _open
'_open', req)
File "/usr/lib/python2.7/urllib2.py", line 382, in _call_chain
result = func(*args)
File "/usr/lib/python2.7/urllib2.py", line 1222, in https_open
return self.do_open(httplib.HTTPSConnection, req)
File "/usr/lib/python2.7/urllib2.py", line 1184, in do_open
raise URLError(err)
urllib2.URLError: <urlopen error [Errno 1] _ssl.c:510: error:14077410:SSL routines:SSL23_GET_SERVER_HELLO:sslv3 alert handshake failure>`

@alykhantejani
Copy link
Contributor

@ZacVawter

I'm assuming you are using a python version < 2.7.9?

I can reproduce this issue in earlier versions and can be solved by using server name identification support which was added to python 2.7.9+ so you have two options:

  1. install the dependencies as described here.
  2. upgrade your python version.

@ZacVawter
Copy link
Author

We are using 2.7.6.

  1. model_zoo.py is not using requests, it is directly using urllib or urllib2. See attached patch that would change it to use requests instead. Requests library works (urllib2 & urllib do not).
  2. I'm not in a position to change version of python.

url_2_requests.patch.tar.gz

@soumith
Copy link
Member

soumith commented Oct 20, 2017

requests is yet another dependency right??? it's not part of standard python

We could simply switch all the URLs to http instead of https if that's a better solution

@ZacVawter
Copy link
Author

You are correct, it would be a new dependency on a non-dist-package'ed library. I thought it was part of dist-packages.

I'd prefer to use https - although because there are hash's in the filename & checked on download it doesn't add much security.

I could probably make a conditional dependency on requests if available, and fall back to urllib and urllib2. There is already 1 layer of fallback from urllib2 to urllib - would get a little uglier to do a 3rd.

@alykhantejani
Copy link
Contributor

@soumith Perhaps changing the model URLs to http is the easiest solution, this is the second issue around this, this week (there was another in the vision repo). If this sounds reasonable I can update torchvision.

@colesbury
Copy link
Member

colesbury commented Oct 21, 2017

The hash check on download is to check for corruption & mistakes, not security. It's only 4 bytes so it's very easy to maliciously create collisions.

I'd like to keep https because pickle can run arbitrary code.

@apaszke
Copy link
Contributor

apaszke commented Oct 21, 2017

We could add an optional dependency on requests (i.e. try to use them, if import fails fall back to urllib). A lot of people have it installed anyway, and saying "just install requests if you have old Python" seems like a good enough workaround for this.

@alykhantejani
Copy link
Contributor

Turns out you also need to install the security packages for requests if you want this to work:

pip install requests[security]

which will also install:

  • pyOpenSSL
  • cryptography
  • idna

If you just have requestsand notrequests[security]` this still craps out with the same error.

We can still add an optional dependency on requests, but this might still fail. wdyt?

@apaszke
Copy link
Contributor

apaszke commented Oct 24, 2017

You can still say "pip install requests[security] fill fix it for you". I don't think it makes any difference

@soumith
Copy link
Member

soumith commented Oct 31, 2017

fixed via #3280

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

No branches or pull requests

5 participants