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

[Eric Blake]

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.

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 <address@hidden>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature