Re: [PATCH v2 1/6] crypto: only verify CA certs in chain of trust

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Oct 16, 2025 at 09:37:51AM -0500, Eric Blake wrote:
> On Fri, Sep 19, 2025 at 11:10:17AM +0100, Daniel P. Berrangé wrote:
> > From: Henry Kleynhans <hkleynhans@fb.com>
> > 
> > The CA file provided to qemu may contain CA certificates which do not
> > form part of the chain of trust for the specific certificate we are
> > sanity checking.
> > 
> > This patch changes the sanity checking from validating every CA
> > certificate to only checking the CA certificates which are part of the
> > chain of trust (issuer chain).  Other certificates are ignored.
> 
> I agree that relaxing this will permit more certs than before (and
> possibly with less CPU cycles spent on the irrelevant portions of the
> cert), without weakening the security of the chain we are actually
> interested in.
> 
> > 
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  crypto/tlscredsx509.c                 | 57 ++++++++++++++++++++++++---
> >  tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++-
> >  2 files changed, 75 insertions(+), 7 deletions(-)
> > 
> > diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> > index cd1f504471..797854ac89 100644
> > --- a/crypto/tlscredsx509.c
> > +++ b/crypto/tlscredsx509.c
> > @@ -315,6 +315,51 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *creds,
> >      return 0;
> >  }
> >  
> > +static int
> > +qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
> > +                                        gnutls_x509_crt_t cert,
> > +                                        gnutls_x509_crt_t *cacerts,
> > +                                        unsigned int ncacerts,
> > +                                        const char *cacertFile,
> > +                                        bool isServer,
> > +                                        bool isCA,
> > +                                        Error **errp)
> > +{
> > +    gnutls_x509_crt_t *cert_to_check = &cert;
> > +    int checking_issuer = 1;
> 
> Why is this int instead of bool?  It's only assigned 1 or 0, and local
> to the function.

No good reason.

> 
> That's a trivial cleanup, so I'm okay if you make that change and add:

I'll change to bool.

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization:  qemu.org | libguestfs.org
> 

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 :|