toolbelt icon indicating copy to clipboard operation
toolbelt copied to clipboard

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

Open snarfed opened this issue 8 years ago • 5 comments

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 avatar Aug 28 '17 20:08 snarfed

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

from requests_toolbelt.adapters import appengine
appengine.monkeypatch()

snarfed avatar Aug 28 '17 21:08 snarfed

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

sigmavirus24 avatar Aug 28 '17 22:08 sigmavirus24

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. :)

theacodes avatar Aug 28 '17 22:08 theacodes

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

sigmavirus24 avatar Nov 27 '17 16:11 sigmavirus24

@sigmavirus24 that seems pretty reasonable.

theacodes avatar Nov 27 '17 17:11 theacodes