From: matoro <11910244-matoro3@users.noreply.gitlab.com> The existing implementation assumes that client/server certificates are single individual certificates. If using publicly-issued certificates, or internal CAs that use an intermediate issuer, this is unlikely to be the case, and they will instead be certificate chains. While this can be worked around by moving the intermediate certificates to the CA certificate, which DOES currently support multiple certificates, this instead allows the issued certificate chains to be used as-is, without requiring the overhead of shuffling certificates around. See: https://gitlab.com/libvirt/libvirt/-/merge_requests/222 Signed-off-by: matoro <matoro_github@matoro.tk> --- src/rpc/virnettlscontext.c | 97 +++++++++++++----------------------- tests/virnettlscontexttest.c | 72 +++++++++++++++++++++++++- 2 files changed, 104 insertions(+), 65 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index cfd26f0701..78b4b0f187 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -424,7 +424,8 @@ static int virNetTLSContextCheckCert(gnutls_x509_crt_t cert, } -static int virNetTLSContextCheckCertPair(gnutls_x509_crt_t cert, +static int virNetTLSContextCheckCertPair(gnutls_x509_crt_t *certs, + size_t ncerts, const char *certFile, gnutls_x509_crt_t *cacerts, size_t ncacerts, @@ -433,7 +434,7 @@ static int virNetTLSContextCheckCertPair(gnutls_x509_crt_t cert, { unsigned int status; - if (gnutls_x509_crt_list_verify(&cert, 1, + if (gnutls_x509_crt_list_verify(certs, ncerts, cacerts, ncacerts, NULL, 0, 0, &status) < 0) { @@ -469,57 +470,18 @@ static int virNetTLSContextCheckCertPair(gnutls_x509_crt_t cert, } -static gnutls_x509_crt_t virNetTLSContextLoadCertFromFile(const char *certFile, - bool isServer) -{ - gnutls_datum_t data; - gnutls_x509_crt_t cert = NULL; - g_autofree char *buf = NULL; - int ret = -1; - - VIR_DEBUG("isServer %d certFile %s", - isServer, certFile); - - if (gnutls_x509_crt_init(&cert) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, "%s", - _("Unable to initialize certificate")); - goto cleanup; - } - - if (virFileReadAll(certFile, (1<<16), &buf) < 0) - goto cleanup; - - data.data = (unsigned char *)buf; - data.size = strlen(buf); - - if (gnutls_x509_crt_import(cert, &data, GNUTLS_X509_FMT_PEM) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, isServer ? - _("Unable to import server certificate %s") : - _("Unable to import client certificate %s"), - certFile); - goto cleanup; - } - - ret = 0; - - cleanup: - if (ret != 0) { - g_clear_pointer(&cert, gnutls_x509_crt_deinit); - } - return cert; -} - - -static int virNetTLSContextLoadCACertListFromFile(const char *certFile, - gnutls_x509_crt_t *certs, - unsigned int certMax, - size_t *ncerts) +static int virNetTLSContextLoadCertListFromFile(const char *certFile, + gnutls_x509_crt_t *certs, + unsigned int certMax, + size_t *ncerts, + bool isServer, + bool isCA) { gnutls_datum_t data; g_autofree char *buf = NULL; *ncerts = 0; - VIR_DEBUG("certFile %s", certFile); + VIR_DEBUG("isCA %d isServer %d certFile %s", isCA, isServer, certFile); if (virFileReadAll(certFile, (1<<16), &buf) < 0) return -1; @@ -527,9 +489,13 @@ static int virNetTLSContextLoadCACertListFromFile(const char *certFile, data.data = (unsigned char *)buf; data.size = strlen(buf); - if (gnutls_x509_crt_list_import(certs, &certMax, &data, GNUTLS_X509_FMT_PEM, 0) < 0) { + if (gnutls_x509_crt_list_import(certs, &certMax, &data, GNUTLS_X509_FMT_PEM, + isCA ? GNUTLS_X509_CRT_LIST_IMPORT_FAIL_IF_EXCEED : + GNUTLS_X509_CRT_LIST_IMPORT_FAIL_IF_EXCEED | GNUTLS_X509_CRT_LIST_FAIL_IF_UNSORTED) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, - _("Unable to import CA certificate list %s"), + isCA ? _("Unable to import CA certificate list %s") : + (isServer ? _("Unable to import server certificate %s") : + _("Unable to import client certificate %s")), certFile); return -1; } @@ -539,44 +505,49 @@ static int virNetTLSContextLoadCACertListFromFile(const char *certFile, } -#define MAX_CERTS 16 +// Limited by frame size of 4096 bytes. +// Typical system CA bundle contains 140-ish CAs. +#define MAX_CERTS 200 static int virNetTLSContextSanityCheckCredentials(bool isServer, const char *cacertFile, const char *certFile) { - gnutls_x509_crt_t cert = NULL; + gnutls_x509_crt_t certs[MAX_CERTS]; gnutls_x509_crt_t cacerts[MAX_CERTS]; - size_t ncacerts = 0; + size_t ncerts = 0, ncacerts = 0; size_t i; int ret = -1; + memset(certs, 0, sizeof(certs)); memset(cacerts, 0, sizeof(cacerts)); if ((access(certFile, R_OK) == 0) && - !(cert = virNetTLSContextLoadCertFromFile(certFile, isServer))) + virNetTLSContextLoadCertListFromFile(certFile, certs, + MAX_CERTS, &ncerts, isServer, false) < 0) goto cleanup; if ((access(cacertFile, R_OK) == 0) && - virNetTLSContextLoadCACertListFromFile(cacertFile, cacerts, - MAX_CERTS, &ncacerts) < 0) + virNetTLSContextLoadCertListFromFile(cacertFile, cacerts, + MAX_CERTS, &ncacerts, isServer, true) < 0) goto cleanup; - if (cert && - virNetTLSContextCheckCert(cert, certFile, isServer, false) < 0) - goto cleanup; + for (i = 0; i < ncerts; i++) { + if (virNetTLSContextCheckCert(certs[i], certFile, isServer, (i != 0)) < 0) + goto cleanup; + } for (i = 0; i < ncacerts; i++) { if (virNetTLSContextCheckCert(cacerts[i], cacertFile, isServer, true) < 0) goto cleanup; } - if (cert && ncacerts && - virNetTLSContextCheckCertPair(cert, certFile, cacerts, ncacerts, cacertFile, isServer) < 0) + if (ncerts && ncacerts && + virNetTLSContextCheckCertPair(certs, ncerts, certFile, cacerts, ncacerts, cacertFile, isServer) < 0) goto cleanup; ret = 0; cleanup: - if (cert) - gnutls_x509_crt_deinit(cert); + for (i = 0; i < ncerts; i++) + gnutls_x509_crt_deinit(certs[i]); 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 2311524db8..918ff04134 100644 --- a/tests/virnettlscontexttest.c +++ b/tests/virnettlscontexttest.c @@ -517,6 +517,12 @@ mymain(void) true, true, GNUTLS_KEY_KEY_CERT_SIGN, false, false, NULL, NULL, 0, 0); + TLS_ROOT_REQ(someotherrootreq, + "UK", "some other random CA", 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, @@ -555,15 +561,72 @@ mymain(void) cacertlevel2areq.crt, }; + gnutls_x509_crt_t cabundle[] = { + someotherrootreq.crt, + cacertrootreq.crt, + }; + + gnutls_x509_crt_t servercertchain[] = { + servercertlevel3areq.crt, + cacertlevel2areq.crt, + cacertlevel1areq.crt, + }; + + gnutls_x509_crt_t servercertchain_incomplete[] = { + servercertlevel3areq.crt, + cacertlevel2areq.crt, + }; + + gnutls_x509_crt_t servercertchain_unsorted[] = { + servercertlevel3areq.crt, + cacertlevel1areq.crt, + cacertlevel2areq.crt, + }; + + gnutls_x509_crt_t clientcertchain[] = { + clientcertlevel2breq.crt, + cacertlevel1breq.crt, + }; + testTLSWriteCertChain("cacertchain-ctx.pem", certchain, G_N_ELEMENTS(certchain)); - VIR_WARNINGS_RESET - DO_CTX_TEST(true, "cacertchain-ctx.pem", servercertlevel3areq.filename, false); DO_CTX_TEST(false, "cacertchain-ctx.pem", clientcertlevel2breq.filename, false); + testTLSWriteCertChain("servercertchain-ctx.pem", + servercertchain, + G_N_ELEMENTS(servercertchain)); + + DO_CTX_TEST(true, cacertrootreq.filename, "servercertchain-ctx.pem", false); + + testTLSWriteCertChain("cabundle-ctx.pem", + cabundle, + G_N_ELEMENTS(cabundle)); + + DO_CTX_TEST(true, "cabundle-ctx.pem", "servercertchain-ctx.pem", false); + + testTLSWriteCertChain("servercertchain_incomplete-ctx.pem", + servercertchain_incomplete, + G_N_ELEMENTS(servercertchain_incomplete)); + + DO_CTX_TEST(true, cacertrootreq.filename, "servercertchain_incomplete-ctx.pem", true); + + testTLSWriteCertChain("servercertchain_unsorted-ctx.pem", + servercertchain_unsorted, + G_N_ELEMENTS(servercertchain_unsorted)); + + DO_CTX_TEST(true, cacertrootreq.filename, "servercertchain_unsorted-ctx.pem", true); + + testTLSWriteCertChain("clientcertchain-ctx.pem", + clientcertchain, + G_N_ELEMENTS(clientcertchain)); + + VIR_WARNINGS_RESET + + DO_CTX_TEST(false, cacertrootreq.filename, "clientcertchain-ctx.pem", false); + DO_CTX_TEST(false, "cacertdoesnotexist.pem", "servercertdoesnotexist.pem", true); testTLSDiscardCert(&cacertreq); @@ -620,7 +683,12 @@ mymain(void) testTLSDiscardCert(&cacertlevel2areq); testTLSDiscardCert(&servercertlevel3areq); testTLSDiscardCert(&clientcertlevel2breq); + testTLSDiscardCert(&someotherrootreq); unlink("cacertchain-ctx.pem"); + unlink("servercertchain-ctx.pem"); + unlink("servercertchain_incomplete-ctx.pem"); + unlink("servercertchain_unsorted-ctx.pem"); + unlink("clientcertchain.pem"); testTLSCleanup(KEYFILE); -- 2.39.1