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

[Mads Kiilerich]

  Thomas Arendsen Hein wrote, On 09/30/2010 11:57 AM:
> 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 :)

That will be a very welcome warning that y2k38 is approaching ;-)

>> +    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)

Right, but it will fail if no '.' is found in dnsname.

The following will fail less often - but is still not perfect:
     certname == '*' + dnsname[dnsname.find('.'):]

> 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

Or just
	certname == '*.' + dnsname.partition('.')[2]
or until 2.5
	certname == '*.' + (dnsname.split('.', 1) + [''])[1]
- which however also isn't very readable ...


Thanks for the comments.

I pushed an updated patch to stable as f2937d6492c5.


Perhaps we should do this to warn that https without cacerts isn't secure:

--- a/mercurial/url.py
+++ b/mercurial/url.py
@@ -532,6 +532,8 @@
                  self.ui.debug(_('%s certificate successfully verified\n') %
                      self.host)
              else:
+                self.ui.warn(_("certificate for %s can't be verified "
+                    "without web.cacerts\n") % self.host)
                  httplib.HTTPSConnection.connect(self)


Finally, as far as I can see all other web.* settings applies to hgweb, so shouldn't web.cacerts have been something like ui.cacerts?

/Mads