[PATCH 1 of 2] url: debug print ssl certificate if verify failed

[Yuya Nishihara]

Mads Kiilerich wrote:
> Yuya Nishihara wrote, On 01/08/2011 02:08 PM:
> > # HG changeset patch
> > # User Yuya Nishihara<yuya at tcha.org>
> > # Date 1294491293 -32400
> > # Branch stable
> > # Node ID 160f24a7970a402d0f7df1912b92419d7acbd8f3
> > # Parent  74ed7f84498b066d6d90c97a3b15240b499365c1
> > url: debug print ssl certificate if verify failed
> 
> Thanks, but ...
> 
> SSLSocket.getpeercert do not return the certificate. It just returns 
> some information from it, some information in its own format which just 
> shows which server the verified certificate applies to and when it expires.
>
> One way to really show the certificate is
> 
>    $ python -c 'import ssl; print 
> ssl.get_server_certificate(("example.com", 443))' | openssl x509 -text
> 
> This patch will not show the certificate but something I would call 
> something else.

Yes, it just shows some info about certificate which getpeercert() picked
out.

> This patch is currently helpful because it cares about subjectAltName 
> and our _verifycert doesn't. That's a bug that should be fixed.
> 
> But if _verifycert fails a certificate it should (and do already) tell 
> why it fails it. I thus don't think this patch will show relevant 
> information in any relevant cases.
> 
> Can you describe some uses cases and include some sample output?

Maybe you're right.
My use case is to show detailed info on verify failure due to missing
subjectAltName support. But once it'll be fixed, this patch will be worth
nothing.

> > diff --git a/mercurial/url.py b/mercurial/url.py
> > --- a/mercurial/url.py
> > +++ b/mercurial/url.py
> > @@ -509,6 +509,18 @@ def _verifycert(cert, hostname):
> >               return _('certificate is for %s') % certname
> >       return _('no commonName found in certificate')
> >
> > +def _printcert(cert, write):
> 
> I think I would prefer to either just pass ui as first parameter or to 
> let the function return a multi-line string.

It's much simple and usable to return a string. Thanks.

> > +    write('ssl certificate:\n')
> > +    for k in ('version', 'notBefore', 'notAfter'):
> 
> AFAIK we will (with this patch) never see anything but notAfter - and we 
> will only get this far if that isn't a problem.
> 
> > +        if k in cert:
> > +            write(' %s: %s\n' % (k, cert[k]))
> > +    for k in ('issuer', 'subject'):
> 
> AFAIK we will never see issuer here.
> 
> > +        for s in cert.get(k, []):
> > +            key, value = s[0]
> > +            write(' %s.%s: %s\n' % (k, key, value.encode('ascii', 'replace')))
> > +    for key, value in cert.get('subjectAltName', []):
> > +        write(' subjectAltName.%s: %s\n' % (key, value))
> > +
> >   if has_https:
> >       class BetterHTTPS(httplib.HTTPSConnection):
> >           send = keepalive.safesend
> > @@ -528,6 +540,8 @@ if has_https:
> >                           ca_certs=cacerts)
> >                   msg = _verifycert(self.sock.getpeercert(), self.host)
> >                   if msg:
> > +                    if self.ui.debugflag:
> > +                        _printcert(self.sock.getpeercert(), self.ui.debug)
> >                       raise util.Abort(_('%s certificate error: %s') %
> >                                        (self.host, msg))
> >                   self.ui.debug('%s certificate successfully verified\n' %
> 
> Finally: This should be tested in test-https.t.

Ah, it looks nice test case. I didn't know it. Thank you.

Yuya,