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 <[email protected]> > > 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é <[email protected]> > > Signed-off-by: matoro <[email protected]> > > [DB: adapted for code conflicts with multi-CA patch] > > Signed-off-by: Daniel P. Berrangé <[email protected]> > > --- > > 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 <[email protected]> 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 :|
