Re: [PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated certs

On Thu, Jul 17, 2025 at 05:28:09PM +0200, Peter Krempa via Devel wrote: > From: Peter Krempa <pkrempa(a)redhat.com&gt; > > Similarly to how we iterate the list of CAs in the concatenated bundle > there's a possibility of the server/client certificates to be > concatenated as well. > > If for some case the first certificate is okay but the further one have > e.g. invalid signatures the validation code would not reject them but > we'd encounter failures later when gnutls tries to use them. > > Iterate also the client/server certs rather than just the CAs. Was there some bug that motivated this change ?

I'd like to keep libvirt's behaviour in sync with QEMU's behaviour to the greatest extent possible. I've just been finalizing changes to QEMU to cope with mutliple certs in the server-cert.pem file, but the semantics there are the certs are a chain of intermediate certs, all of which must be valid. ie, currently we allow ca-cert.pem server-cert.pem | | |------+--------------------------| |---+-------| Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert but the intent is to support ca-cert.pem server-cert.pem | | |------+--| |----------------------------+-------| Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert Or ca-cert.pem server-cert.pem | | |------+-------------| |---------------+-------| Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert

the rational is that these splits reflect how some CA will give you your certs to begin with, and we want to allow them to be used directly. My intent was to copy the QEMU change into libvirt once it was merged in QEMU, so from that POV I'm not a fan of making the some of the changes in this series. Beyond that I'm also working post-quantum crypto support, which will require us to load multiple distinct server-cert-NNN.pem files, each with an independant set of certs, which are selected at runtime based on negotiated ciphers in the TLS handshake.

> > Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; > --- > src/rpc/virnettlscert.c | 29 +++++++++++++++++------------ > 1 file changed, 17 insertions(+), 12 deletions(-) > > diff --git a/src/rpc/virnettlscert.c b/src/rpc/virnettlscert.c > index 3efc4f0716..2724f55bbe 100644 > --- a/src/rpc/virnettlscert.c > +++ b/src/rpc/virnettlscert.c > @@ -442,38 +442,43 @@ int virNetTLSCertSanityCheck(bool isServer, > const char *cacertFile, > const char *certFile) > { > - gnutls_x509_crt_t cert = NULL; > + gnutls_x509_crt_t certs[MAX_CERTS] = { 0 }; > + size_t ncerts = 0; > gnutls_x509_crt_t cacerts[MAX_CERTS] = { 0 }; > size_t ncacerts = 0; > size_t i; > int ret = -1; > > if ((access(certFile, R_OK) == 0) && > - !(cert = virNetTLSCertLoadFromFile(certFile, isServer))) > + virNetTLSCertLoadListFromFile(certFile, certs, MAX_CERTS, &ncerts) < 0) > goto cleanup; > + > if ((access(cacertFile, R_OK) == 0) && > virNetTLSCertLoadListFromFile(cacertFile, cacerts, > MAX_CERTS, &ncacerts) < 0) > goto cleanup; > > - if (cert && > - virNetTLSCertCheck(cert, certFile, isServer, false) < 0) > - goto cleanup; > - > for (i = 0; i < ncacerts; i++) { > - if (virNetTLSCertCheck(cacerts[i], cacertFile, isServer, true) < 0) > + g_autofree char *cacertid = g_strdup_printf("%s[%zu]", cacertFile, i); > + if (virNetTLSCertCheck(cacerts[i], cacertid, isServer, true) < 0) > goto cleanup; > } > > - if (cert && ncacerts && > - virNetTLSCertCheckPair(cert, certFile, cacerts, ncacerts, cacertFile, isServer) < 0) > - goto cleanup; > + for (i = 0; i < ncerts; i++) { > + g_autofree char *certid = g_strdup_printf("%s[%zu]", certFile, i); > + if (virNetTLSCertCheck(certs[i], certid, isServer, false) < 0) > + goto cleanup; > + > + if (ncacerts && > + virNetTLSCertCheckPair(certs[i], certid, cacerts, ncacerts, cacertFile, isServer) < 0) > + goto cleanup;