[PATCH] https: support tls sni (server name indication) for https urls (issue3090)
Augie Fackler
raf at durin42.com
Sat Mar 15 14:34:51 CDT 2014
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 :/)
[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?
> +__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 --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: Message signed with OpenPGP using GPGMail
URL: <http://selenic.com/pipermail/mercurial-devel/attachments/20140315/3125dc4b/attachment.pgp>
More information about the Mercurial-devel
mailing list