[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/4] Fix validation of CA certificate chains



On Tue, Aug 6, 2013 at 6:35 AM, Daniel P. Berrange <berrange redhat com> wrote:
> From: "Daniel P. Berrange" <berrange redhat com>
>
> The code added to validate CA certificates did not take into
> account the possibility that the cacert.pem file can contain
> multiple (concatenated) cert data blocks. Extend the code for
> loading CA certs to use the gnutls APIs for loading cert lists.
> Add test cases to check that multi-level trees of certs will
> validate correctly.
>
> Signed-off-by: Daniel P. Berrange <berrange redhat com>
> ---
>  src/rpc/virnettlscontext.c   | 73 ++++++++++++++++++++++++++++++++++----------
>  tests/virnettlscontexttest.c | 59 +++++++++++++++++++++++++++++++++++
>  tests/virnettlshelpers.c     | 34 +++++++++++++++++++++
>  tests/virnettlshelpers.h     |  3 ++
>  tests/virnettlssessiontest.c | 63 ++++++++++++++++++++++++++++++++++++--
>  5 files changed, 214 insertions(+), 18 deletions(-)
>
> diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c
> index af0ec21..55fb7d0 100644
> --- a/src/rpc/virnettlscontext.c
> +++ b/src/rpc/virnettlscontext.c
> @@ -455,14 +455,15 @@ static int virNetTLSContextCheckCert(gnutls_x509_crt_t cert,
>
>  static int virNetTLSContextCheckCertPair(gnutls_x509_crt_t cert,
>                                           const char *certFile,
> -                                         gnutls_x509_crt_t cacert,
> +                                         gnutls_x509_crt_t *cacerts,
> +                                         size_t ncacerts,
>                                           const char *cacertFile,
>                                           bool isServer)
>  {
>      unsigned int status;
>
>      if (gnutls_x509_crt_list_verify(&cert, 1,
> -                                    &cacert, 1,
> +                                    cacerts, ncacerts,
>                                      NULL, 0,
>                                      0, &status) < 0) {
>          virReportError(VIR_ERR_SYSTEM_ERROR, isServer ?
> @@ -500,16 +501,15 @@ static int virNetTLSContextCheckCertPair(gnutls_x509_crt_t cert,
>
>
>  static gnutls_x509_crt_t virNetTLSContextLoadCertFromFile(const char *certFile,
> -                                                          bool isServer,
> -                                                          bool isCA ATTRIBUTE_UNUSED)
> +                                                          bool isServer)
>  {
>      gnutls_datum_t data;
>      gnutls_x509_crt_t cert = NULL;
>      char *buf = NULL;
>      int ret = -1;
>
> -    VIR_DEBUG("isServer %d isCA %d certFile %s",
> -              isServer, isCA, certFile);
> +    VIR_DEBUG("isServer %d certFile %s",
> +              isServer, certFile);
>
>      if (gnutls_x509_crt_init(&cert) < 0) {
>          virReportError(VIR_ERR_SYSTEM_ERROR, "%s",
> @@ -543,40 +543,81 @@ cleanup:
>  }
>
>
> +static int virNetTLSContextLoadCACertListFromFile(const char *certFile,
> +                                                  gnutls_x509_crt_t *certs,
> +                                                  size_t *ncerts)
> +{
> +    gnutls_datum_t data;
> +    char *buf = NULL;
> +    int ret = -1;
> +    unsigned int certMax = *ncerts;
> +
> +    *ncerts = 0;
> +    VIR_DEBUG("certFile %s", certFile);
> +
> +    if (virFileReadAll(certFile, (1<<16), &buf) < 0)
> +        goto cleanup;
> +
> +    data.data = (unsigned char *)buf;
> +    data.size = strlen(buf);
> +
> +    if (gnutls_x509_crt_list_import(certs, &certMax, &data, GNUTLS_X509_FMT_PEM, 0) < 0) {
> +        virReportError(VIR_ERR_SYSTEM_ERROR,
> +                       _("Unable to import CA certificate list %s"),
> +                       certFile);
> +        goto cleanup;
> +    }
> +    *ncerts = certMax;
> +
> +    ret = 0;
> +
> +cleanup:
> +    VIR_FREE(buf);
> +    return ret;
> +}
> +
> +
> +#define MAX_CERTS 16
>  static int virNetTLSContextSanityCheckCredentials(bool isServer,
>                                                    const char *cacertFile,
>                                                    const char *certFile)
>  {
>      gnutls_x509_crt_t cert = NULL;
> -    gnutls_x509_crt_t cacert = NULL;
> +    gnutls_x509_crt_t cacerts[MAX_CERTS];
> +    size_t ncacerts = MAX_CERTS;
> +    size_t i;
>      int ret = -1;
>
>      if ((access(certFile, R_OK) == 0) &&
> -        !(cert = virNetTLSContextLoadCertFromFile(certFile, isServer, false)))
> +        !(cert = virNetTLSContextLoadCertFromFile(certFile, isServer)))
>          goto cleanup;
>      if ((access(cacertFile, R_OK) == 0) &&
> -        !(cacert = virNetTLSContextLoadCertFromFile(cacertFile, isServer, false)))
> +        virNetTLSContextLoadCACertListFromFile(cacertFile, cacerts, &ncacerts) < 0)
>          goto cleanup;
>
>      if (cert &&
>          virNetTLSContextCheckCert(cert, certFile, isServer, false) < 0)
>          goto cleanup;
>
> -    if (cacert &&
> -        virNetTLSContextCheckCert(cacert, cacertFile, isServer, true) < 0)
> -        goto cleanup;
> +    for (i = 0 ; i < ncacerts ; i++) {
> +        if (virNetTLSContextCheckCert(cacerts[i], cacertFile, isServer, true) < 0)
> +            goto cleanup;
> +    }
>
> -    if (cert && cacert &&
> -        virNetTLSContextCheckCertPair(cert, certFile, cacert, cacertFile, isServer) < 0)
> +    VIR_DEBUG("Here");
> +    if (cert && ncacerts &&
> +        virNetTLSContextCheckCertPair(cert, certFile, cacerts, ncacerts, cacertFile, isServer) < 0) {
> +        VIR_DEBUG("there");
>          goto cleanup;
> +    }
>
>      ret = 0;
>
>  cleanup:
>      if (cert)
>          gnutls_x509_crt_deinit(cert);
> -    if (cacert)
> -        gnutls_x509_crt_deinit(cacert);
> +    for (i = 0 ; i < ncacerts ; i++)
> +        gnutls_x509_crt_deinit(cacerts[i]);
>      return ret;
>  }
>
> diff --git a/tests/virnettlscontexttest.c b/tests/virnettlscontexttest.c
> index 977a095..234922f 100644
> --- a/tests/virnettlscontexttest.c
> +++ b/tests/virnettlscontexttest.c
> @@ -510,6 +510,57 @@ mymain(void)
>      DO_CTX_TEST(true, cacertreq.filename, servercertnew1req.filename, true);
>      DO_CTX_TEST(false, cacertreq.filename, clientcertnew1req.filename, true);
>
> +    TLS_ROOT_REQ(cacertrootreq,
> +                 "UK", "libvirt root", NULL, NULL, NULL, NULL,
> +                 true, true, true,
> +                 true, true, GNUTLS_KEY_KEY_CERT_SIGN,
> +                 false, false, NULL, NULL,
> +                 0, 0);
> +    TLS_CERT_REQ(cacertlevel1areq, cacertrootreq,
> +                 "UK", "libvirt level 1a", NULL, NULL, NULL, NULL,
> +                 true, true, true,
> +                 true, true, GNUTLS_KEY_KEY_CERT_SIGN,
> +                 false, false, NULL, NULL,
> +                 0, 0);
> +    TLS_CERT_REQ(cacertlevel1breq, cacertrootreq,
> +                 "UK", "libvirt level 1b", NULL, NULL, NULL, NULL,
> +                 true, true, true,
> +                 true, true, GNUTLS_KEY_KEY_CERT_SIGN,
> +                 false, false, NULL, NULL,
> +                 0, 0);
> +    TLS_CERT_REQ(cacertlevel2areq, cacertlevel1areq,
> +                 "UK", "libvirt level 2a", NULL, NULL, NULL, NULL,
> +                 true, true, true,
> +                 true, true, GNUTLS_KEY_KEY_CERT_SIGN,
> +                 false, false, NULL, NULL,
> +                 0, 0);
> +    TLS_CERT_REQ(servercertlevel3areq, cacertlevel2areq,
> +                 "UK", "libvirt.org", NULL, NULL, NULL, NULL,
> +                 true, true, false,
> +                 true, true, GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
> +                 true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
> +                 0, 0);
> +    TLS_CERT_REQ(clientcertlevel2breq, cacertlevel1breq,
> +                 "UK", "libvirt client level 2b", NULL, NULL, NULL, NULL,
> +                 true, true, false,
> +                 true, true, GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
> +                 true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL,
> +                 0, 0);
> +
> +    gnutls_x509_crt_t certchain[] = {
> +        cacertrootreq.crt,
> +        cacertlevel1areq.crt,
> +        cacertlevel1breq.crt,
> +        cacertlevel2areq.crt,
> +    };
> +
> +    testTLSWriteCertChain("cacertchain.pem",
> +                          certchain,
> +                          ARRAY_CARDINALITY(certchain));
> +
> +    DO_CTX_TEST(true, "cacertchain.pem", servercertlevel3areq.filename, false);
> +    DO_CTX_TEST(false, "cacertchain.pem", clientcertlevel2breq.filename, false);
> +
>      testTLSDiscardCert(&cacertreq);
>      testTLSDiscardCert(&cacert1req);
>      testTLSDiscardCert(&cacert2req);
> @@ -558,6 +609,14 @@ mymain(void)
>      testTLSDiscardCert(&servercertnew1req);
>      testTLSDiscardCert(&clientcertnew1req);
>
> +    testTLSDiscardCert(&cacertrootreq);
> +    testTLSDiscardCert(&cacertlevel1areq);
> +    testTLSDiscardCert(&cacertlevel1breq);
> +    testTLSDiscardCert(&cacertlevel2areq);
> +    testTLSDiscardCert(&servercertlevel3areq);
> +    testTLSDiscardCert(&clientcertlevel2breq);
> +    unlink("cacertchain.pem");
> +
>      testTLSCleanup();
>
>      return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE;
> diff --git a/tests/virnettlshelpers.c b/tests/virnettlshelpers.c
> index 8236e82..baf043a 100644
> --- a/tests/virnettlshelpers.c
> +++ b/tests/virnettlshelpers.c
> @@ -406,6 +406,40 @@ testTLSGenerateCert(struct testTLSCertReq *req,
>  }
>
>
> +void testTLSWriteCertChain(const char *filename,
> +                           gnutls_x509_crt_t *certs,
> +                           size_t ncerts)
> +{
> +    size_t i;
> +    int fd;
> +    int err;
> +    static char buffer[1024*1024];
> +    size_t size;
> +
> +    if ((fd = open(filename, O_WRONLY|O_CREAT, 0600)) < 0) {
> +        VIR_WARN("Failed to open %s", filename);
> +        abort();
> +    }
> +
> +    for (i = 0 ; i < ncerts ; i++) {
> +        size = sizeof(buffer);
> +        if ((err = gnutls_x509_crt_export(certs[i], GNUTLS_X509_FMT_PEM, buffer, &size) < 0)) {
> +            VIR_WARN("Failed to export certificate %s", gnutls_strerror(err));
> +            unlink(filename);
> +            abort();
> +        }
> +
> +        if (safewrite(fd, buffer, size) != size) {
> +            VIR_WARN("Failed to write certificate to %s", filename);
> +            unlink(filename);
> +            abort();
> +        }
> +    }
> +
> +    VIR_FORCE_CLOSE(fd);
> +}
> +
> +
>  void testTLSDiscardCert(struct testTLSCertReq *req)
>  {
>      if (!req->crt)
> diff --git a/tests/virnettlshelpers.h b/tests/virnettlshelpers.h
> index 50a4ba1..7c3f8da 100644
> --- a/tests/virnettlshelpers.h
> +++ b/tests/virnettlshelpers.h
> @@ -71,6 +71,9 @@ struct testTLSCertReq {
>
>  void testTLSGenerateCert(struct testTLSCertReq *req,
>                           gnutls_x509_crt_t ca);
> +void testTLSWriteCertChain(const char *filename,
> +                           gnutls_x509_crt_t *certs,
> +                           size_t ncerts);
>  void testTLSDiscardCert(struct testTLSCertReq *req);
>
>  void testTLSInit(void);
> diff --git a/tests/virnettlssessiontest.c b/tests/virnettlssessiontest.c
> index 66df682..c2d34bb 100644
> --- a/tests/virnettlssessiontest.c
> +++ b/tests/virnettlssessiontest.c
> @@ -193,7 +193,7 @@ static int testTLSSessionInit(const void *opaque)
>              VIR_WARN("Expected server cert check fail");
>              goto cleanup;
>          } else {
> -            VIR_DEBUG("Not unexpected server cert fail");
> +            VIR_DEBUG("No unexpected server cert fail");
>          }
>      }
>
> @@ -213,7 +213,7 @@ static int testTLSSessionInit(const void *opaque)
>              VIR_WARN("Expected client cert check fail");
>              goto cleanup;
>          } else {
> -            VIR_DEBUG("Not unexpected client cert fail");
> +            VIR_DEBUG("No unexpected client cert fail");
>          }
>      }
>
> @@ -405,6 +405,57 @@ mymain(void)
>      DO_SESS_TEST(cacertreq.filename, servercertreq.filename, clientcertreq.filename,
>                   false, false, "libvirt.org", wildcards6);
>
> +    TLS_ROOT_REQ(cacertrootreq,
> +                 "UK", "libvirt root", NULL, NULL, NULL, NULL,
> +                 true, true, true,
> +                 true, true, GNUTLS_KEY_KEY_CERT_SIGN,
> +                 false, false, NULL, NULL,
> +                 0, 0);
> +    TLS_CERT_REQ(cacertlevel1areq, cacertrootreq,
> +                 "UK", "libvirt level 1a", NULL, NULL, NULL, NULL,
> +                 true, true, true,
> +                 true, true, GNUTLS_KEY_KEY_CERT_SIGN,
> +                 false, false, NULL, NULL,
> +                 0, 0);
> +    TLS_CERT_REQ(cacertlevel1breq, cacertrootreq,
> +                 "UK", "libvirt level 1b", NULL, NULL, NULL, NULL,
> +                 true, true, true,
> +                 true, true, GNUTLS_KEY_KEY_CERT_SIGN,
> +                 false, false, NULL, NULL,
> +                 0, 0);
> +    TLS_CERT_REQ(cacertlevel2areq, cacertlevel1areq,
> +                 "UK", "libvirt level 2a", NULL, NULL, NULL, NULL,
> +                 true, true, true,
> +                 true, true, GNUTLS_KEY_KEY_CERT_SIGN,
> +                 false, false, NULL, NULL,
> +                 0, 0);
> +    TLS_CERT_REQ(servercertlevel3areq, cacertlevel2areq,
> +                 "UK", "libvirt.org", NULL, NULL, NULL, NULL,
> +                 true, true, false,
> +                 true, true, GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
> +                 true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
> +                 0, 0);
> +    TLS_CERT_REQ(clientcertlevel2breq, cacertlevel1breq,
> +                 "UK", "libvirt client level 2b", NULL, NULL, NULL, NULL,
> +                 true, true, false,
> +                 true, true, GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
> +                 true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL,
> +                 0, 0);
> +
> +    gnutls_x509_crt_t certchain[] = {
> +        cacertrootreq.crt,
> +        cacertlevel1areq.crt,
> +        cacertlevel1breq.crt,
> +        cacertlevel2areq.crt,
> +    };
> +
> +    testTLSWriteCertChain("cacertchain.pem",
> +                          certchain,
> +                          ARRAY_CARDINALITY(certchain));
> +
> +    DO_SESS_TEST("cacertchain.pem", servercertlevel3areq.filename, clientcertlevel2breq.filename,
> +                 false, false, "libvirt.org", NULL);
> +
>      testTLSDiscardCert(&clientcertreq);
>      testTLSDiscardCert(&clientcertaltreq);
>
> @@ -415,6 +466,14 @@ mymain(void)
>      testTLSDiscardCert(&cacertreq);
>      testTLSDiscardCert(&altcacertreq);
>
> +    testTLSDiscardCert(&cacertrootreq);
> +    testTLSDiscardCert(&cacertlevel1areq);
> +    testTLSDiscardCert(&cacertlevel1breq);
> +    testTLSDiscardCert(&cacertlevel2areq);
> +    testTLSDiscardCert(&servercertlevel3areq);
> +    testTLSDiscardCert(&clientcertlevel2breq);
> +    unlink("cacertchain.pem");
> +
>      testTLSCleanup();
>
>      return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE;
> --
> 1.8.3.1

To follow Michal's ACK. I've tried just this patch with a chained
certificate I had and I can confirm this worked.

-- 
Doug Goldstein


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]