requests / toolbelt

A toolbelt of useful classes and functions to be used with python-requests
https://toolbelt.readthedocs.org
Other
995 stars 183 forks source link

verify=False per request doesn't work on App Engine #192

Open snarfed opened 7 years ago

snarfed commented 7 years ago

hi all! i noticed recently that the app engine adapter only supports enabling or disabling SSL certification validation globally, not per request. this is a bit of a surprise abstraction leak since it still happily accepts verify=False or =True in e.g. requests.get(), but silently ignores it.

you can reproduce this on https://shell-hrd.appspot.com/ , which has the latest stable versions of requests (2.18.4), toolbelt (0.8.0), and urllib3 (1.22):

>>> import requests
>>> requests.get('https://hnz.io/', verify=False)
...
Traceback (most recent call last):
  File "...//shell.py", line 269, in get
    exec compiled in statement_module.__dict__
  File "<string>", line 1, in <module>
  File ".../local/lib/python2.7/site-packages/requests/api.py", line 72, in get
    return request('get', url, params=params, **kwargs)
  File ".../local/lib/python2.7/site-packages/requests/api.py", line 58, in request
    return session.request(method=method, url=url, **kwargs)
  File ".../local/lib/python2.7/site-packages/requests/sessions.py", line 508, in request
    resp = self.send(prep, **send_kwargs)
  File ".../local/lib/python2.7/site-packages/requests/sessions.py", line 618, in send
    r = adapter.send(request, **kwargs)
  File ".../local/lib/python2.7/site-packages/requests/adapters.py", line 519, in send
    raise SSLError(e, request=request)
SSLError: Invalid and/or missing SSL certificate for URL: https://hnz.io/

urllib3's contrib.appengine.AppEngineManager also seems to set its validate_certificate globally, or at least once per instance (which i'm guessing is usually long lived), so i'm not sure if you'd rather this to be filed here, there, or elsewhere. happy to move it if you want.

thanks in advance!

snarfed commented 7 years ago

forgot to mention, the toolbelt setup code in https://shell-hrd.appspot.com/ is:

from requests_toolbelt.adapters import appengine
appengine.monkeypatch()
sigmavirus24 commented 7 years ago

I'll defer to @jonparrott on this as most of the code is theirs (both in urllib3 and the toolbelt). It looks like the urllib3 connection pool has been behaving that way for 2 years and I think it was an honest oversight rather than anything deliberate or intentional. If Jon can confirm that, I will happily take on the task of updating this if you don't want to, @snarfed

theacodes commented 7 years ago

So verify/validate_certificate is not a usual parameter to urllib3's urlopen method. Looking at requests it seems it modifies the actual connection it gets from the pool to do verification.

It seems we should just add a validate_certificate option to AppEngineManager.urlopen that can override the class-level attribute. Toolbelt's adapter can then just specify that if verify is set.

I am going on vacation for the next week, so if someone else wants to do this go for it. :)

sigmavirus24 commented 6 years ago

I wonder if we could just instantiate two AppEngineManager objects and then route to each one based on verify.

theacodes commented 6 years ago

@sigmavirus24 that seems pretty reasonable.