From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33659) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUidM-0007L7-4n for qemu-devel@nongnu.org; Wed, 26 Aug 2015 17:53:29 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZUidH-0000NV-43 for qemu-devel@nongnu.org; Wed, 26 Aug 2015 17:53:28 -0400 Received: from mx1.redhat.com ([209.132.183.28]:43096) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZUidG-0000NI-Se for qemu-devel@nongnu.org; Wed, 26 Aug 2015 17:53:23 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 1C55D8E384 for ; Wed, 26 Aug 2015 21:53:22 +0000 (UTC) References: <1440601524-30316-1-git-send-email-berrange@redhat.com> <1440601524-30316-7-git-send-email-berrange@redhat.com> From: Eric Blake Message-ID: <55DE354C.3000401@redhat.com> Date: Wed, 26 Aug 2015 15:53:16 -0600 MIME-Version: 1.0 In-Reply-To: <1440601524-30316-7-git-send-email-berrange@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="8JUAMXe6Baj8WlNvFJWmmBAghM2Ix1vH0" Subject: Re: [Qemu-devel] [PATCH v5 6/9] crypto: add sanity checking of TLS x509 credentials List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Daniel P. Berrange" , qemu-devel@nongnu.org Cc: Paolo Bonzini , Gerd Hoffmann This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --8JUAMXe6Baj8WlNvFJWmmBAghM2Ix1vH0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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. >=20 > 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. >=20 > 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'. >=20 > 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 >=3D 2.0. >=20 > Signed-off-by: Daniel P. Berrange > --- > +++ 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 !=3D 0) { > + const char *reason =3D "Invalid certificate"; > + > + if (status & GNUTLS_CERT_INVALID) { > + reason =3D "The certificate is not trusted."; > + } > + > + if (status & GNUTLS_CERT_SIGNER_NOT_FOUND) { > + reason =3D "The certificate hasn't got a known issuer."; > + } > + > + if (status & GNUTLS_CERT_REVOKED) { > + reason =3D "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 =3D sizeof(buffer); > + char serial[5] =3D { 1, 2, 3, 4, 0 }; > + gnutls_datum_t der; > + time_t start =3D time(NULL) + (60*60*req->start_offset); > + time_t expire =3D 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. Findings are minor, and overall seems nice to have. And adding to the testsuite is always a nice proof of validity. Reviewed-by: Eric Blake --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --8JUAMXe6Baj8WlNvFJWmmBAghM2Ix1vH0 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJV3jVMAAoJEKeha0olJ0Nq5wIH/17qhYMj3oG36N89l0jJnp6M A2sv7u5BcOUiFgU2DBMdH9OSvARME407/CIHzFrC9bgISS3xAeM36I3pOfy3I0/I FsBKn6/Ke2zUhPtaoDI1cJ8Qx9/FLtW/hdp3ck9gjmyDD7sr+tMEubJGwb7YD1VS zyraukgBiXz4vvp6S3xjFO03pc5JmKhcLjVpQTVwHvI/dcVgBSrqlV3KIkfNjnGp Sor6tBIYcOwAFjI6gO6iidntPRoXtQfvnS0q2e0BAocMJylW5tHIH8qA7UcJ4cRp MxisIJZZsSKd6pfOQnDPW/N5SE6bVUUx2xqdvm9tQIBes1R7+KnO1aJLLjt97r8= =sq2L -----END PGP SIGNATURE----- --8JUAMXe6Baj8WlNvFJWmmBAghM2Ix1vH0--