Patchwork https: support tls sni (server name indication) for https urls (issue3090)

login
register
mail settings
Submitter Alex Orange
Date Aug. 29, 2013, 5:49 p.m.
Message ID <c9e262317f32cf3253d8.1377798553@localhost>
Download mbox | patch
Permalink /patch/2289/
State Superseded
Commit 40d582ff434f3fdca4f78655503bb177388dda66
Headers show

Comments

Alex Orange - Aug. 29, 2013, 5:49 p.m.
# HG changeset patch
# User Alex Orange <crazycasta@gmail.com>
# Date 1377769698 21600
# Node ID c9e262317f32cf3253d83384f3232577fb565675
# Parent  d4a0055af149cdea20b3136b66cae8a24b2e2a98
https: support tls sni (server name indication) for https urls (issue3090)

SNI is a common way of sharing servers across multiple domains using separate
SSL certificates. Python 2.x does not, and will not, support SNI according to:
http://bugs.python.org/issue5639#msg192234.

In order to support SNI a sepearate package (ssl_sni) was written and uploaded
to PyPI. This code is then used by importing it before the builtin ssl and, if
the import succeeds, using its wrap_socket and constants instead of ssl's. The
new version of wrap_socket takes a parameter server_hostname that specifies
the hostname that gets sent via SNI. Therefore the code in url.py that uses
wrap_socket has been modified to pass the server hostname to wrap_socket.

The research I did led me to the conclusion that an OpenSSL based solution
would best replicate that existing python ssl object, as opposed to a GNUTLS
solution like PyGnuTLS (https://gitorious.org/pygnutls). The two packages I
found that perform the basic functions of the python ssl object are pyOpenSSL
(http://pythonhosted.org/pyOpenSSL/) and M2Crypto
(http://www.heikkitoivonen.net/m2crypto/). M2Crypto does not support sending
the host name as far as I can tell, which leaves pyOpenSSL. The shortcoming of
both of these libraries is that they do not provide give the subjectAltName
subobjects as separate objects. They give either the DER encoded raw data or
an openssl command line style string "DNS:a.b.com, DNS:c.d.com, ...". I did not
want to parse this string in case a certificate came up with a dNSName had the
form 'a.b.com, DNS:c.d.com' which could conceivably lead to a security problem.
In order to handle this problem I used pyasn1 to parse the subjectAltName data
along with code taken from ndg-httpsclient. There appears to also be an
way to do this directly through python's ssl module with an undocumented
function called _test_decode_cert. However this would involve writing
certificates to temporary files and then reading them back in. This seems like
a bad idea both because the function is undocumented (and therefore vulnerable
to going away without warning) and a possible security risk: writing to files
and then reading them back in.

Finally, to make all of this as have as little an impact on the mercurial code
as possible I wrapped this functionality into a module that emulates the
python ssl class called SSLSocket in openssl.py. At the suggestion of Augie
Fackler I put this in a separate package on PyPI called ssl_sni.
Augie Fackler - Sept. 3, 2013, 6:50 p.m.
On Thu, Aug 29, 2013 at 11:49:13AM -0600, Alex Orange wrote:
> # HG changeset patch
> # User Alex Orange <crazycasta@gmail.com>
> # Date 1377769698 21600
> # Node ID c9e262317f32cf3253d83384f3232577fb565675
> # Parent  d4a0055af149cdea20b3136b66cae8a24b2e2a98
> https: support tls sni (server name indication) for https urls (issue3090)

This chunk of the solution looks fine. Sadly, since the ssl_sni
package is licensed under the AGPL, I can't use it (and so haven't
looked at it) due to our policies at work.

Can I persuade you to view this package as enough of a public good
that it be licensed under some super-permissive (MIT? Apache2?)
license, rather than the hyper-viral AGPL?

> SNI is a common way of sharing servers across multiple domains using separate
> SSL certificates. Python 2.x does not, and will not, support SNI according to:
> http://bugs.python.org/issue5639#msg192234.
>
> In order to support SNI a sepearate package (ssl_sni) was written and uploaded
> to PyPI. This code is then used by importing it before the builtin ssl and, if
> the import succeeds, using its wrap_socket and constants instead of ssl's. The
> new version of wrap_socket takes a parameter server_hostname that specifies
> the hostname that gets sent via SNI. Therefore the code in url.py that uses
> wrap_socket has been modified to pass the server hostname to wrap_socket.
>
> The research I did led me to the conclusion that an OpenSSL based solution
> would best replicate that existing python ssl object, as opposed to a GNUTLS
> solution like PyGnuTLS (https://gitorious.org/pygnutls). The two packages I
> found that perform the basic functions of the python ssl object are pyOpenSSL
> (http://pythonhosted.org/pyOpenSSL/) and M2Crypto
> (http://www.heikkitoivonen.net/m2crypto/). M2Crypto does not support sending
> the host name as far as I can tell, which leaves pyOpenSSL. The shortcoming of
> both of these libraries is that they do not provide give the subjectAltName
> subobjects as separate objects. They give either the DER encoded raw data or
> an openssl command line style string "DNS:a.b.com, DNS:c.d.com, ...". I did not
> want to parse this string in case a certificate came up with a dNSName had the
> form 'a.b.com, DNS:c.d.com' which could conceivably lead to a security problem.
> In order to handle this problem I used pyasn1 to parse the subjectAltName data
> along with code taken from ndg-httpsclient. There appears to also be an
> way to do this directly through python's ssl module with an undocumented
> function called _test_decode_cert. However this would involve writing
> certificates to temporary files and then reading them back in. This seems like
> a bad idea both because the function is undocumented (and therefore vulnerable
> to going away without warning) and a possible security risk: writing to files
> and then reading them back in.
>
> Finally, to make all of this as have as little an impact on the mercurial code
> as possible I wrapped this functionality into a module that emulates the
> python ssl class called SSLSocket in openssl.py. At the suggestion of Augie
> Fackler I put this in a separate package on PyPI called ssl_sni.
>
> diff -r d4a0055af149 -r c9e262317f32 mercurial/sslutil.py
> --- a/mercurial/sslutil.py	Fri Aug 23 16:16:22 2013 -0400
> +++ b/mercurial/sslutil.py	Thu Aug 29 03:48:18 2013 -0600
> @@ -11,34 +11,51 @@
>  from mercurial import util
>  from mercurial.i18n import _
>  try:
> -    # avoid using deprecated/broken FakeSocket in python 2.6
> -    import ssl
> -    CERT_REQUIRED = ssl.CERT_REQUIRED
> +    # Force the import
> +    from ssl_sni import openssl
> +
> +    CERT_REQUIRED = openssl.CERT_REQUIRED
>      def ssl_wrap_socket(sock, keyfile, certfile,
> -                cert_reqs=ssl.CERT_NONE, ca_certs=None):
> -        sslsocket = ssl.wrap_socket(sock, keyfile, certfile,
> -                cert_reqs=cert_reqs, ca_certs=ca_certs,
> -                ssl_version=ssl.PROTOCOL_SSLv3)
> -        # check if wrap_socket failed silently because socket had been closed
> -        # - see http://bugs.python.org/issue13721
> -        if not sslsocket.cipher():
> -            raise util.Abort(_('ssl connection failed'))
> +                        cert_reqs=openssl.CERT_NONE, ca_certs=None,
> +                        server_hostname=None):
> +        sslsocket = openssl.wrap_socket(sock, keyfile, certfile,
> +                                        cert_reqs=cert_reqs,
> +                                        ca_certs=ca_certs,
> +                                        server_hostname=server_hostname)
>          return sslsocket
>  except ImportError:
> -    CERT_REQUIRED = 2
> +    try:
> +        # avoid using deprecated/broken FakeSocket in python 2.6
> +        import ssl
> +        CERT_REQUIRED = ssl.CERT_REQUIRED
> +        def ssl_wrap_socket(sock, keyfile, certfile,
> +                            cert_reqs=ssl.CERT_NONE, ca_certs=None,
> +                            server_hostname=None):
> +            sslsocket = ssl.wrap_socket(sock, keyfile, certfile,
> +                                        cert_reqs=cert_reqs, ca_certs=ca_certs,
> +                                        ssl_version=ssl.PROTOCOL_SSLv3)
> +            # check if wrap_socket failed silently because socket had been
> +            # closed
> +            # - see http://bugs.python.org/issue13721
> +            if not sslsocket.cipher():
> +                raise util.Abort(_('ssl connection failed'))
> +            return sslsocket
> +    except ImportError:
> +        CERT_REQUIRED = 2
>
> -    import socket, httplib
> +        import socket, httplib
>
> -    def ssl_wrap_socket(sock, key_file, cert_file,
> -                        cert_reqs=CERT_REQUIRED, ca_certs=None):
> -        if not util.safehasattr(socket, 'ssl'):
> -            raise util.Abort(_('Python SSL support not found'))
> -        if ca_certs:
> -            raise util.Abort(_(
> -                'certificate checking requires Python 2.6'))
> +        def ssl_wrap_socket(sock, key_file, cert_file,
> +                            cert_reqs=CERT_REQUIRED, ca_certs=None,
> +                            server_hostname=None):
> +            if not util.safehasattr(socket, 'ssl'):
> +                raise util.Abort(_('Python SSL support not found'))
> +            if ca_certs:
> +                raise util.Abort(_(
> +                    'certificate checking requires Python 2.6'))
>
> -        ssl = socket.ssl(sock, key_file, cert_file)
> -        return httplib.FakeSocket(sock, ssl)
> +            ssl = socket.ssl(sock, key_file, cert_file)
> +            return httplib.FakeSocket(sock, ssl)
>
>  def _verifycert(cert, hostname):
>      '''Verify that cert (in socket.getpeercert() format) matches hostname.
> @@ -115,9 +132,14 @@
>                  self.ui.warn(_("warning: certificate for %s can't be verified "
>                                 "(Python too old)\n") % host)
>              return
> +        try:
> +            # work around http://bugs.python.org/issue13721
> +            if not sock.cipher():
> +                raise util.Abort(_('%s ssl connection error') % host)
> +        except AttributeError:
> +            # This is not the ssl object you are looking for
> +            pass
>
> -        if not sock.cipher(): # work around http://bugs.python.org/issue13721
> -            raise util.Abort(_('%s ssl connection error') % host)
>          try:
>              peercert = sock.getpeercert(True)
>              peercert2 = sock.getpeercert()
> diff -r d4a0055af149 -r c9e262317f32 mercurial/url.py
> --- a/mercurial/url.py	Fri Aug 23 16:16:22 2013 -0400
> +++ b/mercurial/url.py	Thu Aug 29 03:48:18 2013 -0600
> @@ -181,7 +181,8 @@
>              self.sock.connect((self.host, self.port))
>              if _generic_proxytunnel(self):
>                  # we do not support client X.509 certificates
> -                self.sock = sslutil.ssl_wrap_socket(self.sock, None, None)
> +                self.sock = sslutil.ssl_wrap_socket(self.sock, None, None,
> +                                                    server_hostname=self.host)
>          else:
>              keepalive.HTTPConnection.connect(self)
>
> @@ -338,7 +339,7 @@
>                  _generic_proxytunnel(self)
>                  host = self.realhostport.rsplit(':', 1)[0]
>              self.sock = sslutil.ssl_wrap_socket(
> -                self.sock, self.key_file, self.cert_file,
> +                self.sock, self.key_file, self.cert_file, server_hostname=host,
>                  **sslutil.sslkwargs(self.ui, host))
>              sslutil.validator(self.ui, host)(self.sock)
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> http://selenic.com/mailman/listinfo/mercurial-devel
Matt Mackall - Sept. 3, 2013, 10:59 p.m.
On Tue, 2013-09-03 at 14:50 -0400, Augie Fackler wrote:
> On Thu, Aug 29, 2013 at 11:49:13AM -0600, Alex Orange wrote:
> > # HG changeset patch
> > # User Alex Orange <crazycasta@gmail.com>
> > # Date 1377769698 21600
> > # Node ID c9e262317f32cf3253d83384f3232577fb565675
> > # Parent  d4a0055af149cdea20b3136b66cae8a24b2e2a98
> > https: support tls sni (server name indication) for https urls (issue3090)
> 
> This chunk of the solution looks fine. Sadly, since the ssl_sni
> package is licensed under the AGPL, I can't use it (and so haven't
> looked at it) due to our policies at work.
> 
> Can I persuade you to view this package as enough of a public good
> that it be licensed under some super-permissive (MIT? Apache2?)
> license, rather than the hyper-viral AGPL?

Agreed. Generally speaking, the Mercurial project can't take any
contributions that use or link to anything that is not GPL _v2_ or
weaker.
Alex Orange - Sept. 9, 2013, 8:37 p.m.
Well, it has taken me a little bit to think about this. I am a strong
proponent of GPL and AGPL. However, because I have found Mercurial so
useful in the past, I have decided to relicense the current version of my
ssl_sni code (which I believe satisfies all the needs of Mercurial) under
GPLv2 or any later version (as I understand Mercurial is licensed). I will
also provide any support as far as bug fixes go to make sure that it is a
quality product. To be clear however, the ssl_sni package on pypi will
remain AGPLv3, and any further changes I make to it will remain AGPLv3. I
will repackage the code and make the license clear as GPLv2 or later and
send a new patch to the list.


On Tue, Sep 3, 2013 at 4:59 PM, Matt Mackall <mpm@selenic.com> wrote:

> On Tue, 2013-09-03 at 14:50 -0400, Augie Fackler wrote:
> > On Thu, Aug 29, 2013 at 11:49:13AM -0600, Alex Orange wrote:
> > > # HG changeset patch
> > > # User Alex Orange <crazycasta@gmail.com>
> > > # Date 1377769698 21600
> > > # Node ID c9e262317f32cf3253d83384f3232577fb565675
> > > # Parent  d4a0055af149cdea20b3136b66cae8a24b2e2a98
> > > https: support tls sni (server name indication) for https urls
> (issue3090)
> >
> > This chunk of the solution looks fine. Sadly, since the ssl_sni
> > package is licensed under the AGPL, I can't use it (and so haven't
> > looked at it) due to our policies at work.
> >
> > Can I persuade you to view this package as enough of a public good
> > that it be licensed under some super-permissive (MIT? Apache2?)
> > license, rather than the hyper-viral AGPL?
>
> Agreed. Generally speaking, the Mercurial project can't take any
> contributions that use or link to anything that is not GPL _v2_ or
> weaker.
>
> --
> Mathematics is the supreme nostalgia of our time.
>
>
>
Augie Fackler - Sept. 18, 2013, 12:43 p.m.
On Mon, Sep 9, 2013 at 4:37 PM, Alex Orange <crazycasta@gmail.com> wrote:

> Well, it has taken me a little bit to think about this. I am a strong
> proponent of GPL and AGPL. However, because I have found Mercurial so
> useful in the past, I have decided to relicense the current version of my
> ssl_sni code (which I believe satisfies all the needs of Mercurial) under
> GPLv2 or any later version (as I understand Mercurial is licensed).
>

Thanks.


> I will also provide any support as far as bug fixes go to make sure that
> it is a quality product. To be clear however, the ssl_sni package on pypi
> will remain AGPLv3, and any further changes I make to it will remain
> AGPLv3.
>

Honestly, I'm not sure I'd put too much effort into an AGPL library -
there's just not much point, given that relatively few folks can embed
AGPL-licensed anything into their address space.

(This isn't me trying to convince you to not AGPL this, I'm just trying to
point out the relative absurdity of a virally licensed library that forces
any of its clients to be AGPL as well.)


> I will repackage the code and make the license clear as GPLv2 or later and
> send a new patch to the list.
>

Thanks, I look forward to seeing the patch. I think at this point, you
should just go ahead and ignore httpclient entirely, and I'll figure out
what the fixed interface for httpclient should look like upon seeing your
patch (it probably just needs an ssl_factory keyword argument, but I'll
withhold that decision until I see your patch).


>
>
> On Tue, Sep 3, 2013 at 4:59 PM, Matt Mackall <mpm@selenic.com> wrote:
>
>> On Tue, 2013-09-03 at 14:50 -0400, Augie Fackler wrote:
>> > On Thu, Aug 29, 2013 at 11:49:13AM -0600, Alex Orange wrote:
>> > > # HG changeset patch
>> > > # User Alex Orange <crazycasta@gmail.com>
>> > > # Date 1377769698 21600
>> > > # Node ID c9e262317f32cf3253d83384f3232577fb565675
>> > > # Parent  d4a0055af149cdea20b3136b66cae8a24b2e2a98
>> > > https: support tls sni (server name indication) for https urls
>> (issue3090)
>> >
>> > This chunk of the solution looks fine. Sadly, since the ssl_sni
>> > package is licensed under the AGPL, I can't use it (and so haven't
>> > looked at it) due to our policies at work.
>> >
>> > Can I persuade you to view this package as enough of a public good
>> > that it be licensed under some super-permissive (MIT? Apache2?)
>> > license, rather than the hyper-viral AGPL?
>>
>> Agreed. Generally speaking, the Mercurial project can't take any
>> contributions that use or link to anything that is not GPL _v2_ or
>> weaker.
>>
>> --
>> Mathematics is the supreme nostalgia of our time.
>>
>>
>>
>

Patch

diff -r d4a0055af149 -r c9e262317f32 mercurial/sslutil.py
--- a/mercurial/sslutil.py	Fri Aug 23 16:16:22 2013 -0400
+++ b/mercurial/sslutil.py	Thu Aug 29 03:48:18 2013 -0600
@@ -11,34 +11,51 @@ 
 from mercurial import util
 from mercurial.i18n import _
 try:
-    # avoid using deprecated/broken FakeSocket in python 2.6
-    import ssl
-    CERT_REQUIRED = ssl.CERT_REQUIRED
+    # Force the import
+    from ssl_sni import openssl
+
+    CERT_REQUIRED = openssl.CERT_REQUIRED
     def ssl_wrap_socket(sock, keyfile, certfile,
-                cert_reqs=ssl.CERT_NONE, ca_certs=None):
-        sslsocket = ssl.wrap_socket(sock, keyfile, certfile,
-                cert_reqs=cert_reqs, ca_certs=ca_certs,
-                ssl_version=ssl.PROTOCOL_SSLv3)
-        # check if wrap_socket failed silently because socket had been closed
-        # - see http://bugs.python.org/issue13721
-        if not sslsocket.cipher():
-            raise util.Abort(_('ssl connection failed'))
+                        cert_reqs=openssl.CERT_NONE, ca_certs=None,
+                        server_hostname=None):
+        sslsocket = openssl.wrap_socket(sock, keyfile, certfile,
+                                        cert_reqs=cert_reqs,
+                                        ca_certs=ca_certs,
+                                        server_hostname=server_hostname)
         return sslsocket
 except ImportError:
-    CERT_REQUIRED = 2
+    try:
+        # avoid using deprecated/broken FakeSocket in python 2.6
+        import ssl
+        CERT_REQUIRED = ssl.CERT_REQUIRED
+        def ssl_wrap_socket(sock, keyfile, certfile,
+                            cert_reqs=ssl.CERT_NONE, ca_certs=None,
+                            server_hostname=None):
+            sslsocket = ssl.wrap_socket(sock, keyfile, certfile,
+                                        cert_reqs=cert_reqs, ca_certs=ca_certs,
+                                        ssl_version=ssl.PROTOCOL_SSLv3)
+            # check if wrap_socket failed silently because socket had been
+            # closed
+            # - see http://bugs.python.org/issue13721
+            if not sslsocket.cipher():
+                raise util.Abort(_('ssl connection failed'))
+            return sslsocket
+    except ImportError:
+        CERT_REQUIRED = 2
 
-    import socket, httplib
+        import socket, httplib
 
-    def ssl_wrap_socket(sock, key_file, cert_file,
-                        cert_reqs=CERT_REQUIRED, ca_certs=None):
-        if not util.safehasattr(socket, 'ssl'):
-            raise util.Abort(_('Python SSL support not found'))
-        if ca_certs:
-            raise util.Abort(_(
-                'certificate checking requires Python 2.6'))
+        def ssl_wrap_socket(sock, key_file, cert_file,
+                            cert_reqs=CERT_REQUIRED, ca_certs=None,
+                            server_hostname=None):
+            if not util.safehasattr(socket, 'ssl'):
+                raise util.Abort(_('Python SSL support not found'))
+            if ca_certs:
+                raise util.Abort(_(
+                    'certificate checking requires Python 2.6'))
 
-        ssl = socket.ssl(sock, key_file, cert_file)
-        return httplib.FakeSocket(sock, ssl)
+            ssl = socket.ssl(sock, key_file, cert_file)
+            return httplib.FakeSocket(sock, ssl)
 
 def _verifycert(cert, hostname):
     '''Verify that cert (in socket.getpeercert() format) matches hostname.
@@ -115,9 +132,14 @@ 
                 self.ui.warn(_("warning: certificate for %s can't be verified "
                                "(Python too old)\n") % host)
             return
+        try:
+            # work around http://bugs.python.org/issue13721
+            if not sock.cipher():
+                raise util.Abort(_('%s ssl connection error') % host)
+        except AttributeError:
+            # This is not the ssl object you are looking for
+            pass
 
-        if not sock.cipher(): # work around http://bugs.python.org/issue13721
-            raise util.Abort(_('%s ssl connection error') % host)
         try:
             peercert = sock.getpeercert(True)
             peercert2 = sock.getpeercert()
diff -r d4a0055af149 -r c9e262317f32 mercurial/url.py
--- a/mercurial/url.py	Fri Aug 23 16:16:22 2013 -0400
+++ b/mercurial/url.py	Thu Aug 29 03:48:18 2013 -0600
@@ -181,7 +181,8 @@ 
             self.sock.connect((self.host, self.port))
             if _generic_proxytunnel(self):
                 # we do not support client X.509 certificates
-                self.sock = sslutil.ssl_wrap_socket(self.sock, None, None)
+                self.sock = sslutil.ssl_wrap_socket(self.sock, None, None,
+                                                    server_hostname=self.host)
         else:
             keepalive.HTTPConnection.connect(self)
 
@@ -338,7 +339,7 @@ 
                 _generic_proxytunnel(self)
                 host = self.realhostport.rsplit(':', 1)[0]
             self.sock = sslutil.ssl_wrap_socket(
-                self.sock, self.key_file, self.cert_file,
+                self.sock, self.key_file, self.cert_file, server_hostname=host,
                 **sslutil.sslkwargs(self.ui, host))
             sslutil.validator(self.ui, host)(self.sock)