PyOpenSSL for SNI-support on Python2 #156

Merged
merged 10 commits into from Mar 20, 2013

Projects

None yet

4 participants

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

kirkeby added some commits Feb 26, 2013
@kirkeby kirkeby We know best if we support SNI. d299a66
@kirkeby kirkeby Use pyOpenSSL if present.
This gives us SNI goodies on all Python versions.
953b5a4
@kirkeby kirkeby Fix subjectAltName support.
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.)
df1d950
@shazow
Owner
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
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
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
@t-8ch t-8ch commented on an outdated diff Mar 15, 2013
urllib3/util.py
+ ext_dat = ext.get_data()
+ decoded_dat = der_decoder.decode(ext_dat,
+ asn1Spec=general_names)
+
+ for name in decoded_dat:
+ if not isinstance(name, SubjectAltName):
+ continue
+ for entry in range(len(name)):
+ component = name.getComponentByPosition(entry)
+ if component.getName() != 'dNSName':
+ continue
+ dns_name.append(str(component.getComponent()))
+
+ return dns_name
+
+ class wrapped_socket(object):
@t-8ch
t-8ch Mar 15, 2013

I think this classname should use camelcase.

@kirkeby
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
Owner
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
kirkeby commented Mar 20, 2013

Sounds good.

kirkeby added some commits Mar 20, 2013
@kirkeby kirkeby Move PyOpenSSL-backed SSL into contrib. 35485d8
@kirkeby kirkeby Tests for urllib3.contrib.pyopenssl.
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.
a105119
@kirkeby
kirkeby commented Mar 20, 2013

How does it look now?

@shazow
Owner
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
t-8ch commented Mar 20, 2013

+1 for pulling in the changes to test_socketlevel.py

@shazow
Owner
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
t-8ch commented Mar 20, 2013

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

@shazow
Owner
shazow commented Mar 20, 2013

Ah fair enough. Let's keep it.

@kirkeby
kirkeby commented Mar 20, 2013

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

@shazow
Owner
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
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 shazow merged commit 9471fe9 into shazow:master Mar 20, 2013

1 check passed

Details default The Travis build passed
@shazow
Owner
shazow commented Mar 20, 2013

Merged. :)

@kirkeby
kirkeby commented Mar 20, 2013

Yay! Thanks :)

@shazow shazow added a commit that referenced this pull request Mar 20, 2013
@shazow CHANGES.rst += SNI support (#156) e363eaf
@schlamar schlamar added a commit to schlamar/urllib3 that referenced this pull request Apr 5, 2013
@shazow CHANGES.rst += SNI support (#156) 8dd4ec1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment