fritzy / SleekXMPP

Python 2.6+/3.1+ XMPP Library
http://groups.google.com/group/sleekxmpp-discussion
Other
1.1k stars 299 forks source link

Python >=3.4 TLS refactoring does not use self.certfile and self.keyfile, breaks mutual TLS #460

Open aalba6675 opened 7 years ago

aalba6675 commented 7 years ago

With refactoring of TLS code for Python >=3.4 we don't load certfile and keyfile anymore

in xmlstream/xmlstream.py: XMLStream._create_secure_socket : in code path for python >=3.4 are we mssing ctx.load_cert_chain() somewhere? I don't see where we use self.certfile and self.keyfile in the >=3.4 code path.

I could not find the use of SSLContext.load_cert_chain() anywhere.

This breaks mutual TLS.

woz5999 commented 7 years ago

i seem to be encountering the same issue

aalba6675 commented 7 years ago

@woz5999 Does this work for you? WFM — If it does I will do a PR.

diff --git a/sleekxmpp/xmlstream/xmlstream.py b/sleekxmpp/xmlstream/xmlstream.py
index 061438c..2e0c2b6 100644
--- a/sleekxmpp/xmlstream/xmlstream.py
+++ b/sleekxmpp/xmlstream/xmlstream.py
@@ -470,7 +470,12 @@ class XMLStream(object):
                     ctx.check_hostname = False
                     ctx.verify_mode = ssl.CERT_NONE
                 elif cert_policy == ssl.CERT_REQUIRED:
+                    ctx.check_hostname = True
                     ctx.load_verify_locations(cafile=self.ca_certs)
+
+                if self.certfile:
+                    ctx.load_cert_chain(certfile = self.certfile,
+                                        keyfile = getattr(self, 'keyfile', None))
             else:
                 # Oops, create_default_context() is not supported.
                 if self.ssl_version == ssl.PROTOCOL_SSLv3:
@@ -517,7 +522,10 @@ class XMLStream(object):
         if ctx:
             if self.ciphers:
                 ctx.set_ciphers(self.ciphers)
-            return ctx.wrap_socket(self.socket, do_handshake_on_connect=False)
+            kwargs = {'do_handshake_on_connect': True}
+            if ctx.check_hostname:
+                kwargs['server_hostname'] = self.server_hostname
+            return ctx.wrap_socket(self.socket, **kwargs)
         else:
             if self.ciphers and sys.version_info >= (2, 7):
                 ssl_args['ciphers'] = self.ciphers
woz5999 commented 7 years ago

i'm out of the office for a few days. I'll try next week and let you know.

woz5999 commented 7 years ago

We're using this package as part of a docker image and were able to work around the issue by reverting to the previous version. Given that we have a functional workaround, it's not really worth the time and effort it would take to implement the instrumentation to test this change.

Sorry I couldn't be more helpful.

Neustradamus commented 5 years ago

Have you tested 1.3.2?

Neustradamus commented 5 years ago

@aalba6675 @woz5999: Any news?

Have you tested with "master"?

It works?