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 <berra...@redhat.com>
> > ---
>
> > +++ 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 :|
