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... > + 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? Reviewed-by: Eric Blake <[email protected]> -- Eric Blake, Principal Software Engineer Red Hat, Inc. Virtualization: qemu.org | libguestfs.org
