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

Peter Krempa via Devel posted 7 patches 1 month, 3 weeks ago
[PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated certs
Posted by Peter Krempa via Devel 1 month, 3 weeks ago
From: Peter Krempa <pkrempa@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.

Signed-off-by: Peter Krempa <pkrempa@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;
+    }

     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;
-- 
2.50.0
Re: [PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated certs
Posted by Daniel P. Berrangé via Devel 1 month, 3 weeks ago
On Thu, Jul 17, 2025 at 05:28:09PM +0200, Peter Krempa via Devel wrote:
> From: Peter Krempa <pkrempa@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 ?

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@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;
> +    }
> 
>      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;
> -- 
> 2.50.0
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated certs
Posted by Peter Krempa via Devel 1 month, 3 weeks ago
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 <pkrempa@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 <pkrempa@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.
Re: [PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated certs
Posted by Daniel P. Berrangé via Devel 1 month, 3 weeks ago
On Fri, Jul 18, 2025 at 09:39:22AM +0200, Peter Krempa wrote:
> 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 <pkrempa@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.

To best of my knowledge that will not work. IIUC when you load
a bundle of certs into a session with gnutls_credentials_set
that is assumed to be a cert chain by GNUTLS.

To load multiple parallel certs, with different algorithms
requires calling gnutls_credentials_set mutliple times, 
providing each distinct cert chain.

Kind of annoying since it means every app using gnutls is
guaranteed to need code changes to support PQC with
parallel offerings of 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.

Yep, even in rawhide the PQC stuff isn't fully ready for gnutls.

The only place I'm aware of which has new enough gnutls is CentOS
Stream, and even then you need custom crypto-policies to enable
it fully.


> Thus if you're going to be fixing both of these I'll just push the
> cleanup patches.

Yep, go ahead and push the cleanup ones

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated certs
Posted by Peter Krempa via Devel 1 month, 3 weeks ago
On Fri, Jul 18, 2025 at 09:24:05 +0100, Daniel P. Berrangé wrote:
> On Fri, Jul 18, 2025 at 09:39:22AM +0200, Peter Krempa wrote:
> > 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 <pkrempa@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.
> 
> To best of my knowledge that will not work. IIUC when you load
> a bundle of certs into a session with gnutls_credentials_set
> that is assumed to be a cert chain by GNUTLS.

Well with my test setup where I've:

  - created 2 CAs (one with RSA one with MLDSA sig) and concatenated them
  - created 4 server certs RSA+MLDSA sig both signed by each of the
    above CAs
  - created 4 client certs same as server certs.

I've then concatenated all 4 server certs into one file and sequentially
tested connecting with each of the 4 client certs.

And (when allowing mldsa sigs for both CA and normal signing [1]) it
worked (allowed connecting via TLS) for all of the 4 client certs.

IIRC I've checked in one scenario that actually MLDSA sigs were used but
didn't bother checking any further for any kind of security problems.

> To load multiple parallel certs, with different algorithms
> requires calling gnutls_credentials_set mutliple times, 
> providing each distinct cert chain.
> 
> Kind of annoying since it means every app using gnutls is
> guaranteed to need code changes to support PQC with
> parallel offerings of 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.
> 
> Yep, even in rawhide the PQC stuff isn't fully ready for gnutls.

[1] Yes this is exactly what tripped me up originally, because MLDSA is
not allowed in the gnutls policy. When I've concatenated the certs
virt-pki-validate tripped up only if the mldsa signed certs were first.
If I put RSA certs first it passed but any of the combination using
MLDSA didn't work.

> 
> The only place I'm aware of which has new enough gnutls is CentOS
> Stream, and even then you need custom crypto-policies to enable
> it fully.
> 
> 
> > Thus if you're going to be fixing both of these I'll just push the
> > cleanup patches.
> 
> Yep, go ahead and push the cleanup ones
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 
Re: [PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated certs
Posted by Daniel P. Berrangé via Devel 1 month, 3 weeks ago
On Fri, Jul 18, 2025 at 12:32:39PM +0200, Peter Krempa wrote:
> On Fri, Jul 18, 2025 at 09:24:05 +0100, Daniel P. Berrangé wrote:
> > On Fri, Jul 18, 2025 at 09:39:22AM +0200, Peter Krempa wrote:
> > > 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 <pkrempa@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.
> > 
> > To best of my knowledge that will not work. IIUC when you load
> > a bundle of certs into a session with gnutls_credentials_set
> > that is assumed to be a cert chain by GNUTLS.
> 
> Well with my test setup where I've:
> 
>   - created 2 CAs (one with RSA one with MLDSA sig) and concatenated them
>   - created 4 server certs RSA+MLDSA sig both signed by each of the
>     above CAs
>   - created 4 client certs same as server certs.
> 
> I've then concatenated all 4 server certs into one file and sequentially
> tested connecting with each of the 4 client certs.

Hmmm, I don't think that is validating things in the way you
expect.

The server certificate algorithm, combined with cipher priorities
on client/server, control what ciphers are negotiated in the TLS
handshake.

The client cert is supplied after the handshake is dnoe and the
server code merely validates it against the CA it holds, but this
doesn't influence the ciphers in use for the session.

IOW I suspect in this test scenario, you had exactly the same
ciphers in use for every session. ie all 4 tests resulted in
use of MLDSA, or all 4 tests used RSA, depending on which one
was first in server-cert.pem.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|