On Thu, Jul 17, 2025 at 17:02:33 +0100, Daniel P. Berrangé wrote:
> On Thu, Jul 17, 2025 at 05:28:09PM +0200, Peter Krempa via Devel wrote:
> > From: Peter Krempa <pkre...@redhat.com>
> >
> > 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 ?
Yes-ish. I've abused the fact that gnutls loads everything from the file
...
>
> 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
... meant to facilitate the above ....
>
> 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.
... in order to load certs with different (also the fancy new
post-quantum) signature algorithms.
Since I didn't notice that the crypto policy in fedora 42 doesn't yet
trust some of those (e.g. mldsa65), some of the certificates I've
concatenated weren't actually passing the checks.
Based on how the checks are written though it depended on the order of
the certificates for 'virt-pki-validate' and libvirtd to actually report
the error. E.g. if the RSA-signed cert was first it claimed pass but
didn't work with mldsa.
Now that you mention that you are going to allow explicit extra server
certs to be loaded specifically that will make more sense.
>
> >
> > Signed-off-by: Peter Krempa <pkre...@redhat.com>
> > ---
> > 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;
And actually (IIUC) also allows using the proper call with
gnutls_x509_crt_list_verify(certs, ncerts,
instead of
gnutls_x509_crt_list_verify(certs, ncerts,
inside virNetTLSCertCheckPair which didn't work for my case because
(again IIUC) since some the certs I've concatenated belonged to another
CA they couldn't be verified this way.
Thus if you're going to be fixing both of these I'll just push the
cleanup patches.
