Re: [PATCH v2 3/6] crypto: allow client/server cert chains

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

On Thu, Oct 16, 2025 at 10:28:59AM -0500, Eric Blake wrote:
> On Fri, Sep 19, 2025 at 11:10:19AM +0100, Daniel P. Berrangé wrote:
> > From: matoro <matoro@users.noreply.github.com>
> 
> The CC: line has a different email address for matoro than the git
> author attribution.  Does that matter?  I'm not a fan of github's
> attempt to make it difficult to reach a contributor outside the github
> walled garden.
> 
> > 
> > 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.
> > 
> > Corresponding libvirt change is available here:
> > https://gitlab.com/libvirt/libvirt/-/merge_requests/222
> > 
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: matoro <matoro_mailinglist_qemu@matoro.tk>
> > [DB: adapted for code conflicts with multi-CA patch]
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  crypto/tlscredsx509.c                 | 156 ++++++++++++--------------
> >  tests/unit/test-crypto-tlscredsx509.c |  77 +++++++++++++
> >  2 files changed, 147 insertions(+), 86 deletions(-)
> 
> >  
> > -static gnutls_x509_crt_t
> > -qcrypto_tls_creds_load_cert(QCryptoTLSCredsX509 *creds,
> > -                            const char *certFile,
> > -                            bool isServer,
> > -                            Error **errp)
> > -{
> 
> > -
> >  static int
> > -qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds,
> > -                                    const char *certFile,
> > -                                    gnutls_x509_crt_t **certs,
> > -                                    unsigned int *ncerts,
> > -                                    Error **errp)
> > +qcrypto_tls_creds_load_cert_list(QCryptoTLSCredsX509 *creds,
> > +                                 const char *certFile,
> > +                                 gnutls_x509_crt_t **certs,
> > +                                 unsigned int *ncerts,
> > +                                 bool isServer,
> > +                                 bool isCA,
> > +                                 Error **errp)
> >  {
> 
> Nice consolidation to reduce duplication.
> 
> > @@ -520,41 +497,48 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
> 
> >  
> > -    if (cert &&
> > -        qcrypto_tls_creds_check_cert(creds,
> > -                                     cert, certFile, isServer,
> > -                                     false, errp) < 0) {
> > -        goto cleanup;
> > +    for (i = 0; i < ncerts; i++) {
> > +        if (qcrypto_tls_creds_check_cert(creds,
> > +                                         certs[i], certFile,
> > +                                         isServer, (i != 0), errp) < 0) {
> 
> The () around 'i != 0' look extraneous to me; but that's trivial
> formatting so I'm not opposed to keeping them.  On the other hand...

Yeah, I'll loose the ()

> 
> > +            goto cleanup;
> > +        }
> >      }
> >  
> > -    if (cert &&
> > -        qcrypto_tls_creds_check_authority_chain(creds, cert,
> > +    if (ncerts &&
> 
> ...here you are doing an implicit conversion of ncerts to bool; why
> not do the same implicit conversion of 'i' rather than explicit '(i !=
> 0)' above?

IMHO using an int in a conditional expression  "if (<int vriable>)"
has pretty clear intent.

Passing an int to a parameter that expects a bool could just as easily be
indicative of a code bug, as it could be intentionally relying on the
type conversion. IOW, it has fuzzy intent.

So although I didn't write this patch, I would be inclined to write it
the same way it is done here.

> 
> Reviewed-by: Eric Blake <eblake@redhat.com>

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