[PATCH stable] https: verify server certificate with cacert when Python doesn't (issue2407)

[Thomas Arendsen Hein]

* Mads Kiilerich <mads at kiilerich.com> [20100930 02:00]:
> diff --git a/mercurial/url.py b/mercurial/url.py
> --- a/mercurial/url.py
> +++ b/mercurial/url.py
> @@ -469,6 +469,72 @@
>          _generic_start_transaction(self, h, req)
>          return keepalive.HTTPHandler._start_transaction(self, h, req)
>  
> +def _verifycert(cert, hostname):
> +    """
> +    Verify that cert (in socket.getpeercert() format) matches hostname and is valid
> +    at this time.
> +    CRLs and subjectAltName are not handled.
> +    Returns error message if any problems are found and None on success.
> +
> +    >>> _verifycert(
> +    ...     {'subject': ((('commonName', 'example.com'),),)}, 'example.com')
> +    >>> _verifycert(
> +    ...     {'subject': ((('commonName', 'example.com'),),)}, 'www.example.com')
> +    'server certificate error: certificate for example.com from www.example.com'
> +    >>> _verifycert(
> +    ...     {'subject': ((('commonName', 'www.example.com'),),)}, 'example.com')
> +    'server certificate error: certificate for www.example.com from example.com'
> +
> +    >>> _verifycert(
> +    ...     {'subject': ((('commonName', '*.example.com'),),)}, 'www.example.com')
> +    >>> _verifycert(
> +    ...     {'subject': ((('commonName', '*.example.com'),),)}, 'example.com')
> +    'server certificate error: certificate for *.example.com from example.com'
> +    >>> _verifycert(
> +    ...     {'subject': ((('commonName', '*.example.com'),),)}, 'w.w.example.com')
> +    'server certificate error: certificate for *.example.com from w.w.example.com'
> +
> +    >>> _verifycert(
> +    ...     {'notAfter': 'May  9 00:00:00 2007 GMT'}, 'example.com')
> +    'server certificate error: certificate expired May  9 00:00:00 2007 GMT'
> +    >>> _verifycert(
> +    ...     {'notBefore': 'May  9 00:00:00 2037 GMT'}, 'example.com')
> +    'server certificate error: certificate from future May  9 00:00:00 2037 GMT'
> +    >>> _verifycert(
> +    ...     {'notAfter': 'Sep 29 15:29:48 2037 GMT',
> +    ...     'subject': ()}, 'example.com')
> +    'server certificate error: certificate from example.com not recognized'
> +    >>> _verifycert(None, 'example.com')
> +    'server certificate error: no certificate received from example.com'
> +    """

The doctest is quite verbose. Personally I would prefer something in
a tests/test-*.py file. Bonus could be to do the tests for notAfter
and notBefore with a calculated date (e.g. now +- 1
week/month/year), otherwise the test will mysteriously fail when the
system clock is wrong or the test is run in late 2037 :)

> +    notafter = cert.get('notAfter')
> +    if notafter:
> +        if time.time() >= ssl.cert_time_to_seconds(notafter):
> +            return _('server certificate error: certificate expired %s'
> +                ) % notafter

Could be smaller:

    if notafter and time.time() > ssl.cert_time_to_seconds(notafter):
        return _('server certificate error: certificate expired %s'
            ) % notafter

(> instead of >= should be correct and is shorter)

> +    notbefore = cert.get('notBefore')
> +    if notbefore:
> +        if time.time() <= ssl.cert_time_to_seconds(notbefore):
> +            return _('server certificate error: certificate from future %s'
> +                ) % notbefore

Could be smaller, and the text should not assume that there are time
machines, but that someone created a certificate that is only valid
beginning e.g. at the 1st of next month.

    if notbefore and time.time() < ssl.cert_time_to_seconds(notbefore):
        return _('server certificate error: certificate not valid before %s'
            ) % notbefore

(< instead of <= should be correct and is shorter)

> +    for s in cert.get('subject', []):
> +        if s[0][0] == 'commonName':
> +            certname = s[0][1].lower()
> +            if dnsname == certname or (
> +                certname.startswith('*.') and
> +                dnsname.endswith(certname[1:]) and
> +                '.' not in dnsname[:-len(certname) + 1]
> +                ):
> +                return None
> +            return _('server certificate error: certificate for %s from %s'
> +                ) % (certname, dnsname)

Whoa, '.' not in dnsname[:-len(certname) + 1] looks strange, maybe
this is more readable:

    for s in cert.get('subject', []):
        key, value = s[0]
        if key == 'commonName':
            certname = value.lower()
            if dnsname == certname or (
                certname.startswith('*.') and
                dnsname[dnsname.find('.') + 1:] == certname[2:]):
                return None
            return _('server certificate error: certificate for %s from %s'
                ) % (certname, dnsname)

Side note: As soon as we switch to python >= 2.5 we can use:
            if dnsname == certname or (
                certname.startswith('*.') and
                dnsname.partition('.')[2] == certname[2:]):
                return None

> +    return _('server certificate error: certificate from %s not recognized'
> +        ) % dnsname

Not sure about that, but maybe be more specific:
    return _('server certificate error: no commonName in certificate from %s'
        ) % dnsname

> -                self.ui.debug(_('server identity verification succeeded\n'))
> +                msg = _verifycert(self.sock.getpeercert(), self.host)
> +                if msg:
> +                    raise util.Abort(msg)
> +                self.ui.debug(_('server certificate for %s verified\n')
> +                    % self.host)

I think the word "succeeded" is important, because otherwise one
could ask what the result of the verification is. Maybe it would be
good to mention the certificate name, too, so you can see wildcards.

> diff --git a/tests/test-doctest.py b/tests/test-doctest.py
> --- a/tests/test-doctest.py
> +++ b/tests/test-doctest.py
> @@ -5,13 +5,14 @@
>  import doctest
>  
>  import mercurial.changelog
> -# test doctest from changelog
> -

Unrelated change. Since this patch might go to stable or into
distributions, having unrelated changes might be bad, even if they
just remove a comment. Maybe your documentation improvements above
should go into a separate patch, too?

Regards,
Thomas

-- 
thomas at intevation.de - http://intevation.de/~thomas/ - OpenPGP key: 0x5816791A
Intevation GmbH, Neuer Graben 17, 49074 Osnabrueck - AG Osnabrueck, HR B 18998
Geschaeftsfuehrer: Frank Koormann, Bernhard Reiter, Dr. Jan-Oliver Wagner