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

PyOpenSSL for SNI-support on Python2 #156

Merged
merged 10 commits into from
Mar 20, 2013
Merged

PyOpenSSL for SNI-support on Python2 #156

merged 10 commits into from
Mar 20, 2013

Conversation

kirkeby
Copy link
Contributor

@kirkeby kirkeby commented Mar 12, 2013

This adds optional SNI-support for Python2, if pyOpenSSL, pyasn1 and ng-httpsclient are installed (pyasn1 and ng-httpsclient are used to parse the subjectAltName of certificates).

This gives us SNI goodies on all Python versions.
The SNI/subjectAltName tests have a one big problem: They access the
great big, bad Internet, because I am not sure how to create the
correct certificates for this.

Also, this only works in all cases if pyasn1 and ndg-httpsclient are
installed (parsing DER-encoded binary data extracted from X.509
extensions is not my idea of fun.)
@shazow
Copy link
Member

shazow commented Mar 15, 2013

Hmm I'd be -1 on merging this as-is. I'm not a fan of environment flags for libraries if we can avoid it, and this seems to depend on a lot of fragile things.

Perhaps a better strategy would be to refactor the API to allow you to plug in your own ssl_wrap_socket easily and then post this as a recipe somewhere? Or maybe even as a helper inside urrlib3/contrib/.

Does anyone else have feelings on this? (@wolever @wallunit @t-8ch, anyone else?)

@snoack
Copy link

snoack commented Mar 15, 2013

Has PyOpenSSL any significant drawbacks? Otherwise I would prefer to automatically pick it, if it is installed, and the built-in ssl module doesn't support SNI. I don't like the environment variable approach either.

@t-8ch
Copy link
Contributor

t-8ch commented Mar 15, 2013

+1 for removing the env variables

@shazow: Wouldn't urllib3.util.ssl_wrap_socket = my_awesome_ssl_wrapping also
work?

@kirkeby:
For testing you may want to look at
test/with_dummyerver/test_socketlevel.py:TestSNI.
It is certainly not pretty, but works.

With the following patch this PR could also use the fingerprint
verification of #144.

From 9505d5673b4a0cbd0f501389401f017fe0c8ed69 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Thomas=20Wei=C3=9Fschuh?= <thomas@t-8ch.de>
Date: Fri, 15 Mar 2013 16:11:24 +0000
Subject: [PATCH] pyopenssl: add param `binary_form` to getpeercert

---
 urllib3/util.py | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/urllib3/util.py b/urllib3/util.py
index 555e753..57977d2 100644
--- a/urllib3/util.py
+++ b/urllib3/util.py
@@ -392,10 +392,16 @@ elif OpenSSL is not None:  # Use PyOpenSSL if installed
         def sendall(self, data):
             return self.connection.sendall(data)

-        def getpeercert(self):
+        def getpeercert(self, binary_form=False):
             x509 = self.connection.get_peer_certificate()
             if not x509:
                 raise SSLError('')
+
+            if binary_form:
+                return OpenSSL.crypto.dump_certificate(
+                    OpenSSL.crypto.FILETYPE_ASN1,
+                    x509)
+
             return {
                 'subject': (
                     (('commonName', x509.get_subject().CN),),
-- 
1.8.2


return dns_name

class wrapped_socket(object):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this classname should use camelcase.

@kirkeby
Copy link
Contributor Author

kirkeby commented Mar 20, 2013

The environment variables are gone now, and the wrapped_socket helper class is renamed WrappedSocket.

I'm open to attempt to refactor the API to allow pluggable ssl_wrap_socket, and moving this code into contrib, if that's what you think is best?

Regarding drawbacks with PyOpenSSL, I can only think of one: This implementation has not been tested as much as the stdlib-backed one.

@shazow
Copy link
Member

shazow commented Mar 20, 2013

Looking better.

Alright, let's do this as a first step:

  1. Put it in a module in the urllib3.contrib namespace (not imported automatically).
  2. For now, we can add something like urllib3.contrib.pyopenssl.inject_into_urllib3() or something which monkeypatches urllib3.util.ssl_wrap_socket with its own version when called.
  3. Add some tests. Maybe we need a contrib testing submodule.
  4. At this point I'd be comfortable merging this first revision into the mainline.

Once we have this, then the next step would be to refactor how urllib3.util.ssl_wrap_socket is used and make it more configurable (so you can specify which socket wrapper to use). This will obsolete the need of monkeypatching. I can help with this.

How does that sound?

@kirkeby
Copy link
Contributor Author

kirkeby commented Mar 20, 2013

Sounds good.

This makes the SNI-test raise SkipTest if SNI is not supported, so
it can be reused by test.contrib.test_pyopenssl even when the stdlib
does not support SNI.

It also tests the urllib3.contrib.pyopenssl module when its
dependencies are installed, by re-exporting HTTPS test-cases and
using module-level setup/teardown code to monkey patch urllib3.
@kirkeby
Copy link
Contributor Author

kirkeby commented Mar 20, 2013

How does it look now?

@shazow
Copy link
Member

shazow commented Mar 20, 2013

Looks decent for a contrib module. :)

Should we revert the changes to test/with_dummyserver/test_socketlevel.py? (Or move them into /test/contrib/...)

If you'd like to add a Sphinx docs section on the contrib module, specifically about your PyOpenSSL addition and how to use it, I would be +1 to that. Could do that later though.

@t-8ch
Copy link
Contributor

t-8ch commented Mar 20, 2013

+1 for pulling in the changes to test_socketlevel.py

@shazow
Copy link
Member

shazow commented Mar 20, 2013

@t-8ch Do you mean test/with_dummyserver/test_socketlevel.py or test/contrib/with_dummyserver/test_socketlevel.py?

@t-8ch
Copy link
Contributor

t-8ch commented Mar 20, 2013

test/with_dummyserver/test_socketlevel.py The SkipTest looks right.

@shazow
Copy link
Member

shazow commented Mar 20, 2013

Ah fair enough. Let's keep it.

@kirkeby
Copy link
Contributor Author

kirkeby commented Mar 20, 2013

Okay. I hope I found the right place to add that contrib section :)

@shazow
Copy link
Member

shazow commented Mar 20, 2013

@kirkeby Perfect, thanks! Want to add a little narrative section for how to use it? (Just a code sample or something.)

@t-8ch Can you think of anything else missing?

Otherwise I think it's ready to go.

@kirkeby
Copy link
Contributor Author

kirkeby commented Mar 20, 2013

Now there's a ready-to-use code snippet in the module's doc-string, which is auto-moduled into the contrib page, is that okay?

shazow added a commit that referenced this pull request Mar 20, 2013
Optional PyOpenSSL for SNI-support on Python2.
@shazow shazow merged commit 9471fe9 into urllib3:master Mar 20, 2013
@shazow
Copy link
Member

shazow commented Mar 20, 2013

Merged. :)

@kirkeby
Copy link
Contributor Author

kirkeby commented Mar 20, 2013

Yay! Thanks :)

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

Successfully merging this pull request may close these issues.

4 participants