[PATCH 2 of 2] url: debug print ssl certificate if connection 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 1294491295 -32400
> > # Branch stable
> > # Node ID e541b8d268395ece35aff45f8116c6041d22d91f
> > # Parent  160f24a7970a402d0f7df1912b92419d7acbd8f3
> > url: debug print ssl certificate if connection failed
> >
> > If a server provides self-signed certificate,
> 
> More precisely: If a server provides certificate that can't be verified 
> by the configured cacerts ...
> 
> > ssl.wrap_socket raises exception
> > like "routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed".
> >
> > As of Python 2.6 or 2.7, it doesn't provide a legitimate
> 
> More precisely: "supported" or "official"
> 
> > way to decode
> > certificate to human-readable text without established ssl socket.
> > So _printservercert() tries to use _ssl._test_decode_cert(), which is available
> > at least on Python 2.6.6 or 2.7.1.
> 
> Interesting ;-)
> 
> I guess this also could be used to show the content of cacerts files and 
> make them less opaque.
> 
> Code in this area is however not simple to maintain, and using these 
> internal functions will not improve the situation ...

Yeah, that's why I tought this patch could be unacceptable.

> > diff --git a/mercurial/url.py b/mercurial/url.py
> > --- a/mercurial/url.py
> > +++ b/mercurial/url.py
> > @@ -290,8 +290,11 @@ if has_https:
> >           import ssl
> >           _ssl_wrap_socket = ssl.wrap_socket
> >           CERT_REQUIRED = ssl.CERT_REQUIRED
> > +        SSLError = ssl.SSLError
> >       except ImportError:
> >           CERT_REQUIRED = 2
> > +        class SSLError(Exception):
> > +            pass
> >
> >           def _ssl_wrap_socket(sock, key_file, cert_file,
> >                                cert_reqs=CERT_REQUIRED, ca_certs=None):
> > @@ -521,6 +524,27 @@ def _printcert(cert, write):
> >       for key, value in cert.get('subjectAltName', []):
> >           write(' subjectAltName.%s: %s\n' % (key, value))
> >
> > +def _printservercert(addr, write):
> > +    """Fetch server certificate and print human-readable details"""
> > +    import tempfile
> 
> We usually do imports at the top of the file. Demandimport makes it cheap.
> 
> > +    try:  # needs Python 2.6
> > +        import ssl, _ssl
> > +        decode_certfile = _ssl._test_decode_cert
> > +    except (ImportError, AttributeError):
> > +        return
> > +    der = ssl.get_server_certificate(addr)
> > +    if not der:
> > +        return
> > +    fh, fn = tempfile.mkstemp(prefix='hg-servercert-')
> > +    try:
> > +        f = os.fdopen(fh, 'w')
> > +        f.write(der)
> > +        f.close()
> > +        cert = decode_certfile(fn)
> > +    finally:
> > +        os.unlink(fn)
> > +    _printcert(cert, write)
> > +
> >   if has_https:
> >       class BetterHTTPS(httplib.HTTPSConnection):
> >           send = keepalive.safesend
> > @@ -535,9 +559,15 @@ if has_https:
> >
> >               if cacerts:
> >                   sock = _create_connection((self.host, self.port))
> > -                self.sock = _ssl_wrap_socket(sock, self.key_file,
> > -                        self.cert_file, cert_reqs=CERT_REQUIRED,
> > -                        ca_certs=cacerts)
> > +                try:
> > +                    self.sock = _ssl_wrap_socket(sock, self.key_file,
> > +                            self.cert_file, cert_reqs=CERT_REQUIRED,
> > +                            ca_certs=cacerts)
> > +                except SSLError:
> > +                    if self.ui.debugflag:
> > +                        _printservercert((self.host, self.port),
> > +                                         self.ui.debug)
> > +                    raise
> >                   msg = _verifycert(self.sock.getpeercert(), self.host)
> >                   if msg:
> >                       if self.ui.debugflag:
> 
> It would be nice to have some tests for this. It might not be feasible 
> to do it in the test suite as long as we don't have conditional sections 
> inside .t files, but could you either add some tests to test-https.t and 
> disable them (for example by putting "#" at the beginning of the lines) 
> or provide a working fragment in the commit message?

I'll try putting some tests on test-https.t.
For now, I've posted some example output in [PATCH 0] email.

> With this patch it is possible to show much of the server certificate 
> info. Cool. But in which cases can that be used? Can you describe some 
> use cases? What do the user see and how do he understand it and what 
> will he then do?

We could ask a user to attach --debug output on bug report.
Currently it needs extra effort to get to know why Mercurial complains
about certificate.

> (If the subject is the same as the issuer then the certificate is 
> self-signed and the certificate from get_server_certificate can thus 
> probably be used in cacerts. That case can however also just be detected 
> with two consecutive calls to get_server_certificate.)

like this? This seems more solid than poking internal _ssl module.

f.write(ssl.get_server_certificate(addr))
f.close()
ssl.get_server_certificate(ssl.get_server_certificate(addr),
                           ca_certs=f.name)

Yuya,