-
Notifications
You must be signed in to change notification settings - Fork 186
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
HostHeaderSSLAdapter doesn't seem to respect SNI #159
Comments
Do you have pyOpenSSL, ndg-httpsclient, and pyasn1 installed as well? |
This also seems to be dependent on the order, so something is persisting between requests. >>> s.get('https://216.58.218.142', headers={'Host': 'www.google.com'})
<Response [200]>
>>> s.get('https://216.58.218.142', headers={'Host': 'google.com'})
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/Users/matt/.virtualenvs/sentry/lib/python2.7/site-packages/requests/sessions.py", line 487, in get
return self.request('GET', url, **kwargs)
File "/Users/matt/.virtualenvs/sentry/lib/python2.7/site-packages/requests/sessions.py", line 475, in request
resp = self.send(prep, **send_kwargs)
File "/Users/matt/.virtualenvs/sentry/lib/python2.7/site-packages/requests/sessions.py", line 606, in send
history = [resp for resp in gen] if allow_redirects else []
File "/Users/matt/.virtualenvs/sentry/lib/python2.7/site-packages/requests/sessions.py", line 113, in resolve_redirects
raise TooManyRedirects('Exceeded %s redirects.' % self.max_redirects, response=resp)
requests.exceptions.TooManyRedirects: Exceeded 30 redirects. The latter is odd, since it's getting a redirect loop. This behavior is correct otherwise: >>> s.get('https://google.com')
<Response [200]> |
Yes.
|
Cool. Just wanted to make sure :) I'll test this tonight or tomorrow night unless @Lukasa beats me to it. |
And for posterity, latest requests, |
Yes, as written that's exactly what the code does. urllib3 has no hooks for overriding the SNI used: it will always use whatever the connection pool believes that the host is, regardless of the headers. We could accept a patch to add an sni kwarg to the connection pool |
Would that need to be patched in urllib3 itself to expose the API? Or can it be done in requests? |
It would need to be done in urllib3. |
Awesome, thanks. 👍 I'll take a look over on that side then! |
I needed a way to modify the SNI too, so I just subclassed SSLContext, and made the HTTPAdapter use an instance of my subclass: import requests
import ssl
class MySSLContext(ssl.SSLContext):
def __new__(cls, server_hostname):
return super(MySSLContext, cls).__new__(cls, ssl.PROTOCOL_SSLv23)
def __init__(self, server_hostname):
super(MySSLContext, self).__init__(ssl.PROTOCOL_SSLv23)
self._my_server_hostname = server_hostname
def change_server_hostname(self, server_hostname):
self._my_server_hostname = server_hostname
def wrap_socket(self, *args, **kwargs):
kwargs['server_hostname'] = self._my_server_hostname
return super(MySSLContext, self).wrap_socket(*args, **kwargs)
session = requests.Session()
adapter = requests.adapters.HTTPAdapter()
context = MySSLContext(host)
adapter.init_poolmanager(10, 10, ssl_context=context)
session.mount('https://', adapter)
result = session.get(url, headers={'host': host}, verify=False) In this case I didn't care about cert verification, I just wanted to fix 400 errors, hence the verify=False, but having looked at HostHeaderSSLAdapter, this should work with that too, and should allow you to leave verification enabled. This doesn't seem to be python3-only, but I only casually tested it just now (originally wrote this code for python3), so I don't know how well it works in all cases. If you want to include this in requests-toolbelt, you're welcome to (feel free to change up the API a little bit). Legal crap if you plan to include it: this code is my own work, created from reading the requests and urllib3 sources and Python documentation (and probably also the Python source at some point, I forget); I license it to you under Apache License Version 2.0. |
Any chance to get reviews for #293? We have been heavily using this PR's code for a few months now, and it works perfectly. |
@sigmavirus24 thank you for the merge! Could it be possible to release them? Thanks! |
piggybacking on this. Would it be possible to make a release with the fix included? Much appreciated! |
I tried the code fix in PR 293 and still ended up with an error like this:
host.xyz.com is basically acting as a proxy in front of the actual web server that is being accessed. I am very, very confused. My code call looks like this:
Is there just no way to do this? |
On a host that doesn't rely on SNI, it seems to behave correctly:
But on a host that requires SNI, it doesn't seem to do the right thing.
I'm happy to help however I can. :)
--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/36339780-hostheaderssladapter-doesn-t-seem-to-respect-sni?utm_campaign=plugin&utm_content=tracker%2F418367&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F418367&utm_medium=issues&utm_source=github).The text was updated successfully, but these errors were encountered: