[PATCH] https: support tls sni (server name indication) for https urls (issue3090)

Alex Orange crazycasta at gmail.com
Mon Mar 17 02:26:03 CDT 2014


On Sat, Mar 15, 2014 at 12:34 PM, Augie Fackler <raf at durin42.com> wrote:

>
> On Feb 2, 2014, at 9:57 PM, Alex Orange <crazycasta at gmail.com> wrote:
>
> > # HG changeset patch
> > # User Alex Orange <crazycasta at gmail.com>
> > # Date 1391402279 25200
> > # Branch stable
> > # Node ID 7fc7d98e8fdad6d82768dd71b8833e04567ea8ba
> > # Parent  9139dedeffa6d54367fcf410d583d401e8fd1318
> > https: support tls sni (server name indication) for https urls
> (issue3090)
>
> Please accept my sincere apologies that it's taken me over a month to get
> to this code review. Life has a habit of getting in the way of larger
> reviews like this at bad times :(
>
> I've got a few questions, and a couple of potential nitpicks below. I've
> tried to leave enough context, but also trim stuff that I'm not worried
> about.
>
> Thanks for doing this.
>
> > 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. The
> code
> > provided here is version 0.1 of the ssl_sni package provided on PyPI.
> The code
> > on PyPI is AGPLv3, however the version in this patch is provided under
> the
> > GPLv2 or later license (consistent with the hg license). 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 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 a
> > 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.
> >
> > diff -r 9139dedeffa6 -r 7fc7d98e8fda mercurial/ssl_sni/openssl.py
> > --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> > +++ b/mercurial/ssl_sni/openssl.py    Sun Feb 02 21:37:59 2014 -0700
>
> [snip]
>
> > +class SSLSocket(object):
>
> The way this is implemented, calling .recv() on this will do Wrong Things,
> right? or does the underlying Connection object offer recv?
>
> (I'm reviewing this on a plane that has no wifi, so unfortunately I can't
> download things and test this at the moment :/)
>

Calling .recv() will invoke __getattr__ which in turn will try to get a
recv function from self.connection which is an OpenSSL.Connection type
object (see
https://pythonhosted.org/pyOpenSSL/api/ssl.html#connection-objects for
details). I believe that is the general way of receiving data (I see
bio_read and want_read, but I think recv works fine for just a general
recv).


>
> [snip rest of that file]
>
> > diff -r 9139dedeffa6 -r 7fc7d98e8fda mercurial/ssl_sni/subjaltname.py
> > --- /dev/null Thu Jan 01 00:00:00 1970 +0000
> > +++ b/mercurial/ssl_sni/subjaltname.py        Sun Feb 02 21:37:59 2014
> -0700
> > @@ -0,0 +1,126 @@
> > +"""NDG HTTPS Client package
> > +
> > +Use pyasn1 to provide support for parsing ASN.1 formatted subjectAltName
> > +content for SSL peer verification.  Code based on:
> > +
> > +
> http://stackoverflow.com/questions/5519958/how-do-i-parse-subjectaltname-extension-data-using-pyasn1
> > +"""
> > +__author__ = "P J Kershaw"
> > +__date__ = "01/02/12"
> > +__copyright__ = "(C) 2012 Science and Technology Facilities Council"
> > +__license__ = "BSD - see LICENSE file in top-level directory"
>
> Which BSD license? There's more than one, and AFAIK there's one that has
> some marketing clauses we couldn't accept. It'd be nice to describe which
> one here for completeness of the license paper trail.
>
> There's some commented out code in this file. Is that from upstream, or
> you? Do you think we should keep it around in case we need those features,
> or should I clean it up in a followup patch?
>

The BSD license appears to be the 3-clause license. I apparently forgot to
document that this time around. It's a bit tricky to find his repo because
there's so many copies of the python around, but I believe this is a
reputable copy (that the LICENSE in it's top-level directory is the license
he's referring to):
http://proj.badc.rl.ac.uk/svn/ndg-security/trunk/ndg_httpsclient/ndg/httpsclient/subj_alt_name.pytop
level:
http://proj.badc.rl.ac.uk/svn/ndg-security/trunk/ndg_httpsclient/

The code is commented out upstream. I'm not clear why it was commented out.
I mainly wanted to modify the original code as little as possible. All of
this being said, I think it's probably unlikely that any further bug fixes
to this code will be made (it seems to be a simple almost translation level
conversion of the RFC spec) so keeping things around for easy future
merging probably isn't a significant factor. In summary, feel free to take
out the commented lines if you like.

The only change I recall making is removing the value size constraints that
limited each type to 64 values. It would appear that the type descriptions
come from section 4.2.1.4 of RFC 5280. That section does specify the
strings should be size 1-200 (characters I believe), which is more than 64
anyway, but also mentions about at least one place (the explicitText) where
some CAs might exceed the 200 limit and therefore clients should gracefully
handle longer explicitText strings. I did this because when testing a cert
coming from google without SNI the dNSName limit easily exceeded the 64
limit and iirc I couldn't come up with a reasonable upper limit. This
google certificate for instance combines all google domains (from other
countries' TLDs) into the subjAltName resulting in a huge string. I figure
for people doing such that certificates could become arbitrarily long
(subject to bandwidth and processing limitations of course). For that
reason I completely removed the size constraints and couldn't seem to find
any adverse side effects through testing (results were the same for
certificates that didn't exceed the 64 length).


>
> > +__contact__ = "Philip.Kershaw at stfc.ac.uk"
> > +__revision__ = '$Id$'
> > +
>
> [snip]
>
> > +
> > +class GeneralName(univ.Choice):
> > +    '''ASN.1 configuration for X.509 certificate subjectAltNames
> fields'''
> > +    componentType = namedtype.NamedTypes(
> > +#        namedtype.NamedType('otherName', AnotherName().subtype(
> > +#                            implicitTag=tag.Tag(tag.tagClassContext,
> > +#                                                tag.tagFormatSimple,
> 0))),
> > +        namedtype.NamedType('rfc822Name', char.IA5String().subtype(
> > +                            implicitTag=tag.Tag(tag.tagClassContext,
> > +                                                tag.tagFormatSimple,
> 1))),
> > +        namedtype.NamedType('dNSName', char.IA5String().subtype(
> > +                            implicitTag=tag.Tag(tag.tagClassContext,
> > +                                                tag.tagFormatSimple,
> 2))),
> > +#        namedtype.NamedType('x400Address', ORAddress().subtype(
> > +#                            implicitTag=tag.Tag(tag.tagClassContext,
> > +#                                                tag.tagFormatSimple,
> 3))),
> > +        namedtype.NamedType('directoryName', Name().subtype(
> > +                            implicitTag=tag.Tag(tag.tagClassContext,
> > +                                                tag.tagFormatSimple,
> 4))),
> > +#        namedtype.NamedType('ediPartyName', EDIPartyName().subtype(
> > +#                            implicitTag=tag.Tag(tag.tagClassContext,
> > +#                                                tag.tagFormatSimple,
> 5))),
> > +        namedtype.NamedType('uniformResourceIdentifier',
> char.IA5String().\
> > +
>  subtype(implicitTag=tag.Tag(tag.tagClassContext,
> > +                                    tag.tagFormatSimple, 6))),
> > +        namedtype.NamedType('iPAddress', univ.OctetString().subtype(
> > +                            implicitTag=tag.Tag(tag.tagClassContext,
> > +                                                tag.tagFormatSimple,
> 7))),
> > +        namedtype.NamedType('registeredID',
> univ.ObjectIdentifier().subtype(
> > +                            implicitTag=tag.Tag(tag.tagClassContext,
> > +                                                tag.tagFormatSimple,
> 8))),
>
> [snip]
>
> > diff -r 9139dedeffa6 -r 7fc7d98e8fda mercurial/sslutil.py
> > --- a/mercurial/sslutil.py    Sat Feb 01 15:20:49 2014 -0600
> > +++ b/mercurial/sslutil.py    Sun Feb 02 21:37:59 2014 -0700
> > @@ -11,39 +11,58 @@
> > 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
> > -    PROTOCOL_SSLv23 = ssl.PROTOCOL_SSLv23
> > -    PROTOCOL_TLSv1 = ssl.PROTOCOL_TLSv1
> > +    # Force the import
> > +    from ssl_sni import openssl
> > +
> > +    CERT_REQUIRED = openssl.CERT_REQUIRED
> > +    PROTOCOL_SSLv23 = openssl.PROTOCOL_SSLv23
> > +    PROTOCOL_TLSv1 = openssl.PROTOCOL_TLSv1
> >     def ssl_wrap_socket(sock, keyfile, certfile,
> ssl_version=PROTOCOL_TLSv1,
> > -                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_version)
> > -        # 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
> > +        PROTOCOL_SSLv23 = ssl.PROTOCOL_SSLv23
> > +        PROTOCOL_TLSv1 = ssl.PROTOCOL_TLSv1
> > +        def ssl_wrap_socket(sock, keyfile, certfile,
> ssl_version=PROTOCOL_TLSv1,
> > +                            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_version)
> > +            # 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
> >
> > -    PROTOCOL_SSLv23 = 2
> > -    PROTOCOL_TLSv1 = 3
> > +        PROTOCOL_SSLv23 = 2
> > +        PROTOCOL_TLSv1 = 3
> >
> > -    import socket, httplib
> > +        import socket, httplib
> >
> > -    def ssl_wrap_socket(sock, keyfile, certfile,
> ssl_version=PROTOCOL_TLSv1,
> > -                        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, keyfile, certfile,
> ssl_version=PROTOCOL_TLSv1,
> > +                            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, keyfile, certfile)
> > -        return httplib.FakeSocket(sock, ssl)
> > +            ssl = socket.ssl(sock, keyfile, certfile)
> > +            return httplib.FakeSocket(sock, ssl)
> >
> > def _verifycert(cert, hostname):
> >     '''Verify that cert (in socket.getpeercert() format) matches
> hostname.
> > @@ -127,9 +146,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 9139dedeffa6 -r 7fc7d98e8fda mercurial/url.py
> > --- a/mercurial/url.py        Sat Feb 01 15:20:49 2014 -0600
> > +++ b/mercurial/url.py        Sun Feb 02 21:37:59 2014 -0700
> > @@ -185,7 +185,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)
> >
> > @@ -342,7 +343,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)
> >
> > diff -r 9139dedeffa6 -r 7fc7d98e8fda tests/test-check-code-hg.t
> > --- a/tests/test-check-code-hg.t      Sat Feb 01 15:20:49 2014 -0600
> > +++ b/tests/test-check-code-hg.t      Sun Feb 02 21:37:59 2014 -0700
> > @@ -34,3 +34,4 @@
> >   Skipping mercurial/httpclient/__init__.py it has no-che?k-code (glob)
> >   Skipping mercurial/httpclient/_readers.py it has no-che?k-code (glob)
> >   Skipping mercurial/httpclient/socketutil.py it has no-che?k-code (glob)
> > +  Skipping mercurial/ssl_sni/subjaltname.py it has no-che?k-code (glob)
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel at selenic.com
> > http://selenic.com/mailman/listinfo/mercurial-devel
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20140317/c7c3d4b1/attachment.html>


More information about the Mercurial-devel mailing list