add support for SNI

[t-8ch]

this makes urrlib3 use SNI if available.

testing can be done using https://sni.velox.ch/
I didn't include an automatic test, as this would fail on old versions of python or (open)ssl.
(SNI was added in 3.2)
I could however add a test which tests the version of python.

[shazow]

Hi @t-8ch, I haven't heard of SNI, very cool.

Some things I'd like to see before we can merge this into urllib3:

• We require 100% test coverage. The tests can't use any external resources (ie. should run offline). Take a look at some of our existing SSL tests. I'm okay with the tests for SNI only running for Python 3.2+.
• I would suggest breaking out the meat in your commit to a separate standalone helper. Take a look at urllib3.util.is_connection_dropped(conn) for example. Maybe something like urllib3.util.ssl_wrap_socket(conn). This way the core urllib3 core can be unaware of the nuances we're performing here.
• Please try to keep the coding style inline with the surrounding code of the library. Especially in terms of documentation, naming, spacing, etc.

Thank you!

[t-8ch]

@shazow:
If I understand correctly, Python just doesn't offer server side support for SNI yet.

Quote from the docs: "Specifying server_hostname will also raise a ValueError if server_side is true"

I could add config files for apache/nginx if this would be more useful.

[shazow]

Mmm good point. http://bugs.python.org/issue8109

I don't feel very comfortable merging in untested code at this point and I would really like to avoid having to fire up nginx/apache to test some code. Some ideas:

1. We could keep this bug open until it becomes feasible. You're welcome to maintain a separate fork of urllib3 until we can merge it in.
2. You could try to implement some basic socket-level tests (have a look at https://github.com/shazow/urllib3/blob/master/test/with_dummyserver/test_socketlevel.py for example). Not sure how hard this would be.
3. We could put the meat of the code in urllib3.contrib (where untested code lives), but it won't be used by default.

Either way, I'd still perform the suggested refactoring. :)

[t-8ch]

Should have read your last post earlier.
Anyhow, the refactoring is done.
Do you want another pull request?
(you can find the branch at https://github.com/t-8ch/urllib3/tree/sni)

I have also a test ready (using mentioned website).
The layout of this one is a bit messy, so if you want it after all I would like to know where to place it and it's external certificate.

Addition:
I'd like to avoid fiddling with low level SSL stuff. But maybe a dummyserver using plain C and OpenSSL would be doable

[shazow]

If you merge your sni branch into add_sni, it should appear in this pull request. You could merge it and then rename add_sni to sni, if you prefer.

[t-8ch]

Wouldn't the merging include all the useless stuff from add_sni?
sni is much cleaner. All of its commits pass 100% of test coverage.

[shazow]

What do you mean by useless stuff? Commits that got overwritten later? I'm not very obsessive about keeping the commit history super clean. I rather keep context.

[t-8ch]

well, I did a clean stepwise rewrite of the patchset, which introduces utils.ssl_wrap_socket.
I have now merged the new tree into the old, messy one.
The clean one is still valid but both are now equal. Only their history differs.
So you are free to choose. I would prefer merging https://github.com/t-8ch/urllib3/tree/sni but feel free to choose.

[shazow]

Very nice, thank you @t-8ch. Any plans for the unit testing part?

Don't worry about the patchset history. Worst case, I can squash it before I merge. :)

[t-8ch]

Im not sure about handling situations, where the tests are run in an offline or in amanaged environment.
I am in favor of just ignoring everything except urrlib3.exceptions.SSLError.
If something different is wrong this should be handled fine by the remaining tests.

[shazow]

All tests must be able to run offline, ideally completely automated (without explicitly running other commands/servers alongside).

[t-8ch]

What about checking if the desired hostname is present in the raw handshake?
It should only be there if SNI is used and the hostname later in HTTP is encrypted anyway.

This only tests if the hostname somehow ends up in the request but everything else would be unittesting python or openssl.

[shazow]

That would be a great start. :) Especially we can get full code coverage with that, then I'd be happy to merge it in.

[t-8ch]

Do you have any hints on how to eavesdrop my own socket?

[shazow]

I think doing something like the tests here should do the trick: https://github.com/shazow/urllib3/blob/master/test/with_dummyserver/test_socketlevel.py

[t-8ch]

Would you also accept an contrib module, which uses pyopenssl?
I am thinking about something along the lines of:

# Do normal stuff
urllib3.util.ssl_wrap_socket = urllib3.contrib.pyopenssl.ssl_wrap_socket
# begin using urllib3

Some quick testing indicated, overwriting things like this should work.
One could provide two functions in this module:
One which uses the native version if available and the other one always uses
pyopenssl.

For the record:
This would help here: https://github.com/kennethreitz/requests/issues/749

[shazow]

Just took a look at the latest diff for this pull request, looking great! Thank you for seeing it through, @t-8ch.

Regarding pyopenssl:

I'm definitely +1 adding pyopenssl support. As you probably noticed, we already do some conditional support for SSL depending if Python is compiled with +ssl enabled. We could do the same thing for pyopenssl, to use it if it's available.

Regarding whether to put it in contrib:

I'm treating contrib as "experimental" code which isn't used by default but users can import it and use it explicitly if they want. So we could have something like urllib3.contrib.pyopenssl.enable_pyopenssl_wrap_socket() or somesuch which replaces which ssl_wrap_socket urllib3 uses. I'd prefer not to overwrite urllib3.util.ssl_wrap_socket in case people are using it as a standalone helper for other code. Instead, we should have a local urllib3.connectionpool.ssl_wrap_socket which can get reassigned.

That said, if we can get appropriate test coverage on the new pyopenssl code paths, then I'd be happy to include it in the core library. If we're unsure, then we can start with contrib and move it out of contrib later.

If you're up for taking a crack at this, maybe we should start a separate pull request thread for pyopenssl? I'll take a closer look at this SNI pull request over the next week and hopefully merge it in. :)

[pythonmobile]

I tried to use sni with urllib3 and I see this problem that when I goto https://randomstring.mydomain.com -- it lands on one of the servers that actually is running on mydomain.com and does not fail (randomstring should actually give an error). This is the github install of urllib3. I also tried with https://github.com/t-8ch/urllib3.git and had the same problem. I hope this can be fixed soon.

[t-8ch]

Are you using a version of python >= 3.2?
Is this a public URL, which I could test myself?

[pythonmobile]

Thanks for the quick response. I'm using python 2.7. Is it not supposed to work with 2.xx?

[pythonmobile]

Indeed it will require pyopenssl. I think that should be top priority :-)

[snoack]

If you're going to move that code into a seperate function, I would prefer to do that switch outside of the function definition, that it is evaluated only during import and not on every call.

if SSLContext:
    ssl_wrap_socket(...) # Python 3.2 implementation using SSLContext
else:
    ssl_wrap_socket(...) # Fallback implementation

[t-8ch]

@shazow, @wallunit

What would be nicer? The same API for both method, ignoring server_name for the fallback function,
or something like this:

if SSLContext:
    def ssl_wrap_socket(...): # new implementation, arguments a superset of the fallback one
else:
    ssl_wrap_socket = ssl.wrap_socket

[t-8ch]

I think the best behaviour would be to silently ignore non critical arguments like server_name but fail if security critical arguments get added later.

[shazow]

@wallunit The review is much appreciated, thank you! Do you think it makes sense to merge your changes into this pull? (I agree with much of what you commented.)

[snoack]

Are you talking about the code cleanup changes, I suggested above. Or are you talking about my own SNI patch?

[shazow]

@wallunit Both. :)

[snoack]

Is there any reason, for doing that check? The old/fallback implementation doesn't explicitly ignores ca_certs if cert_reqs is CERT_NONE. So why do we do so, here?

[t-8ch]

Gonnna fix this

[t-8ch]

Got it again:
If ca_certs is None, SSLContext.verify_locations() dies with:

TypeError: cafile and capath cannot be both omitted

whereas ssl.wrap_socket() just doesn't care.

This way people, not wanting to do verification had to specify ca_certs nevertheless.

The try-catch is necessary, as SSLContext.load_verify_location() raises a TypeError whereas the guts and unittests of urllib3 expect a SSLError.

We could remove the check, catch the Error and check if it's about a invalid ca_path, swallow it in this case and reraise it otherwaise. But I think this wouldn't be really nice.

[snoack]

Of course, you have to check whether ca_certs is given, but that's not the check you do in the code I commented on, above. In fact SSLSocket.__init__ as called by wrap_socket in Python 3.2 uses following code:

if ca_certs:
    self.context.load_verify_locations(ca_certs)

But it doesn't check for verify_mode != CERT_NONE, like you do above.

[t-8ch]

Ah, now I got it.

[snoack]

@shazow Of course I think that my suggestions regarding your patch make sense, otherwise I wouldn't have suggested them, right? ;) However my patch is still missing a test case. But feel free to merge my implementation with your test case together.

[shazow]

@wallunit Not my patch so much as @t-8ch's. ;)

Anyways, thanks again for the feedback, keep it coming! I'll take a look at it next week.

[snoack]

Still should be if ca_certs: instead of if context.verify_mode != CERT_NONE:. See discussion form previous version.

[snoack]

The patch looks good now. I hope it will be merged soon.

[t-8ch]

Thanks for your review!
Let's look what @shazow will do with it and what happens with #109.

[t-8ch]

I have pushed a new branch to my repo which provides a new argument ssl_version to util.ssl_wrap_socket() which should make this pull request functional compatible with #109

[t-8ch]

@shazow any updates on this?

[shazow]

namedtuple is a thing. :)

[shazow]

I took a quick look, looks very reasonable. Unfortunately I don't have much time to validate and merge in non-trivial changes right now.

If this is urgently blocking anyone, I suggest maintaining a fork with this patch for the time being. Hopefully I'll have the time for some of the bigger bugs in the next few weeks. Please keep bugging me until then. :)