[PATCH] https: support tls sni (server name indication) for https urls (issue3090)
Alex Orange
crazycasta at gmail.com
Wed Apr 23 13:48:59 CDT 2014
I haven't heard anything in quite some time, and since the inbox numbers
are low I've decided to bump. Is there any more action I need to take on
this? Is someone looking at it?
On Mon, Mar 17, 2014 at 1:26 AM, Alex Orange <crazycasta at gmail.com> wrote:
>
>
>
> 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/20140423/1c9ebd98/attachment.html>
More information about the Mercurial-devel
mailing list