Re: [Qemu-devel] [PATCH v5 6/9] crypto: add sanity checking of TLS x509 credentials

[Daniel P. Berrange]

On Wed, Aug 26, 2015 at 03:53:16PM -0600, Eric Blake wrote:
> On 08/26/2015 09:05 AM, Daniel P. Berrange wrote:
> > If the administrator incorrectly sets up their x509 certificates,
> > the errors seen at runtime during connection attempts are very
> > obscure and difficult to diagnose. This has been a particular
> > problem for people using openssl to generate their certificates
> > instead of the gnutls certtool, because the openssl tools don't
> > turn on the various x509 extensions that gnutls expects to be
> > present by default.
> > 
> > This change thus adds support in the TLS credentials object to
> > sanity check the certificates when QEMU first loads them. This
> > gives the administrator immediate feedback for the majority of
> > common configuration mistakes, reducing the pain involved in
> > setting up TLS. The code is derived from equivalent code that
> > has been part of libvirt's TLS support and has been seen to be
> > valuable in assisting admins.
> > 
> > It is possible to disable the sanity checking, however, via
> > the new 'sanity-check' property on the tls-creds object type,
> > with a value of 'no'.
> > 
> > Unit tests are included in this change to verify the correctness
> > of the sanity checking code in all the key scenarios it is
> > intended to cope with. As part of the test suite, the pkix_asn1_tab.c
> > from gnutls is imported. This file is intentionally copied from the
> > (long since obsolete) gnutls 1.6.3 source tree, since that version
> > was still under GPLv2+, rather than the GPLv3+ of gnutls >= 2.0.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> 
> > +++ b/crypto/tlscredsx509.c
> > @@ -38,6 +38,514 @@
> 
> 
> > +static int
> > +qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
> > +                                  const char *certFile,
> > +                                  gnutls_x509_crt_t *cacerts,
> > +                                  size_t ncacerts,
> > +                                  const char *cacertFile,
> > +                                  bool isServer,
> > +                                  Error **errp)
> > +{
> 
> > +    if (status != 0) {
> > +        const char *reason = "Invalid certificate";
> > +
> > +        if (status & GNUTLS_CERT_INVALID) {
> > +            reason = "The certificate is not trusted.";
> > +        }
> > +
> > +        if (status & GNUTLS_CERT_SIGNER_NOT_FOUND) {
> > +            reason = "The certificate hasn't got a known issuer.";
> > +        }
> > +
> > +        if (status & GNUTLS_CERT_REVOKED) {
> > +            reason = "The certificate has been revoked.";
> 
> The trailing dots seem unusual here, since most of your code doesn't
> have them.
> 
> 
> > +++ b/tests/crypto-tls-x509-helpers.c
> 
> 
> > +void
> > +test_tls_generate_cert(QCryptoTLSTestCertReq *req,
> > +                       gnutls_x509_crt_t ca)
> > +{
> > +    gnutls_x509_crt_t crt;
> > +    int err;
> > +    static char buffer[1024*1024];
> 
> Space around operator '*'
> 
> > +    size_t size = sizeof(buffer);
> > +    char serial[5] = { 1, 2, 3, 4, 0 };
> > +    gnutls_datum_t der;
> > +    time_t start = time(NULL) + (60*60*req->start_offset);
> > +    time_t expire = time(NULL) + (60*60*(req->expire_offset
> 
> and again
> 
> > +++ b/tests/pkix_asn1_tab.c
> > @@ -0,0 +1,1103 @@
> > +/*
> > + * This file is taken from gnutls 1.6.3 under the GPLv2+
> > + */
> 
> Is this missing a copyright statement? Even if gnutls 1.6.3 didn't
> mention copyright per-file, it might be nice to mention the copyright
> owner of the overall release of that old tarball.

The original gnutls file didn't have any header at all, so I added
the mention that it was GPLv2+ per the global gnutls license file.
I didn't put any Copyright as it was not clear who exactly it add.
I guess the answer might lie in gnutls git history somewhere if
you think that's important.



Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|