From: matoro <matoro@users.noreply.github.com>
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(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 91d8dde633..311de3237d 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -317,7 +317,8 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *creds,
static int
qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
- gnutls_x509_crt_t cert,
+ gnutls_x509_crt_t *certs,
+ unsigned int ncerts,
gnutls_x509_crt_t *cacerts,
unsigned int ncacerts,
const char *cacertFile,
@@ -325,9 +326,33 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
bool isCA,
Error **errp)
{
- gnutls_x509_crt_t cert_to_check = cert;
+ gnutls_x509_crt_t cert_to_check = certs[ncerts - 1];
int checking_issuer = 1;
int retval = 0;
+ gnutls_datum_t dn = {}, dnissuer = {};
+
+ for (int i = 0; i < (ncerts - 1); i++) {
+ if (!gnutls_x509_crt_check_issuer(certs[i], certs[i + 1])) {
+ retval = gnutls_x509_crt_get_dn2(certs[i], &dn);
+ if (retval < 0) {
+ error_setg(errp, "Unable to fetch cert DN: %s",
+ gnutls_strerror(retval));
+ return -1;
+ }
+ retval = gnutls_x509_crt_get_dn2(certs[i + 1], &dnissuer);
+ if (retval < 0) {
+ gnutls_free(dn.data);
+ error_setg(errp, "Unable to fetch cert DN: %s",
+ gnutls_strerror(retval));
+ return -1;
+ }
+ error_setg(errp, "Cert '%s' does not match issuer of cert '%s'",
+ dnissuer.data, dn.data);
+ gnutls_free(dn.data);
+ gnutls_free(dnissuer.data);
+ return -1;
+ }
+ }
while (checking_issuer) {
checking_issuer = 0;
@@ -362,7 +387,8 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
}
static int
-qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
+qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t *certs,
+ size_t ncerts,
const char *certFile,
gnutls_x509_crt_t *cacerts,
size_t ncacerts,
@@ -372,7 +398,7 @@ qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
{
unsigned int status;
- if (gnutls_x509_crt_list_verify(&cert, 1,
+ if (gnutls_x509_crt_list_verify(certs, ncerts,
cacerts, ncacerts,
NULL, 0,
0, &status) < 0) {
@@ -414,66 +440,14 @@ qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
}
-static gnutls_x509_crt_t
-qcrypto_tls_creds_load_cert(QCryptoTLSCredsX509 *creds,
- const char *certFile,
- bool isServer,
- Error **errp)
-{
- gnutls_datum_t data;
- gnutls_x509_crt_t cert = NULL;
- g_autofree char *buf = NULL;
- gsize buflen;
- GError *gerr = NULL;
- int ret = -1;
- int err;
-
- trace_qcrypto_tls_creds_x509_load_cert(creds, isServer, certFile);
-
- err = gnutls_x509_crt_init(&cert);
- if (err < 0) {
- error_setg(errp, "Unable to initialize certificate: %s",
- gnutls_strerror(err));
- goto cleanup;
- }
-
- if (!g_file_get_contents(certFile, &buf, &buflen, &gerr)) {
- error_setg(errp, "Cannot load CA cert list %s: %s",
- certFile, gerr->message);
- g_error_free(gerr);
- goto cleanup;
- }
-
- data.data = (unsigned char *)buf;
- data.size = strlen(buf);
-
- err = gnutls_x509_crt_import(cert, &data, GNUTLS_X509_FMT_PEM);
- if (err < 0) {
- error_setg(errp, isServer ?
- "Unable to import server certificate %s: %s" :
- "Unable to import client certificate %s: %s",
- certFile,
- gnutls_strerror(err));
- goto cleanup;
- }
-
- ret = 0;
-
- cleanup:
- if (ret != 0) {
- gnutls_x509_crt_deinit(cert);
- cert = NULL;
- }
- return cert;
-}
-
-
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)
{
gnutls_datum_t data;
g_autofree char *buf = NULL;
@@ -496,7 +470,9 @@ qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds,
if (gnutls_x509_crt_list_import2(certs, ncerts, &data,
GNUTLS_X509_FMT_PEM, 0) < 0) {
error_setg(errp,
- "Unable to import CA certificate list %s",
+ isCA ? "Unable to import CA certificate list %s" :
+ (isServer ? "Unable to import server certificate %s" :
+ "Unable to import client certificate %s"),
certFile);
return -1;
}
@@ -512,7 +488,8 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
const char *certFile,
Error **errp)
{
- gnutls_x509_crt_t cert = NULL;
+ gnutls_x509_crt_t *certs = NULL;
+ unsigned int ncerts = 0;
gnutls_x509_crt_t *cacerts = NULL;
unsigned int ncacerts = 0;
size_t i;
@@ -520,41 +497,48 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
if (certFile &&
access(certFile, R_OK) == 0) {
- cert = qcrypto_tls_creds_load_cert(creds,
- certFile, isServer,
- errp);
- if (!cert) {
+ if (qcrypto_tls_creds_load_cert_list(creds,
+ certFile,
+ &certs,
+ &ncerts,
+ isServer,
+ false,
+ errp) < 0) {
goto cleanup;
}
}
if (access(cacertFile, R_OK) == 0) {
- if (qcrypto_tls_creds_load_ca_cert_list(creds,
- cacertFile,
- &cacerts,
- &ncacerts,
- errp) < 0) {
+ if (qcrypto_tls_creds_load_cert_list(creds,
+ cacertFile,
+ &cacerts,
+ &ncacerts,
+ isServer,
+ true,
+ errp) < 0) {
goto cleanup;
}
}
- 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) {
+ goto cleanup;
+ }
}
- if (cert &&
- qcrypto_tls_creds_check_authority_chain(creds, cert,
+ if (ncerts &&
+ qcrypto_tls_creds_check_authority_chain(creds,
+ certs, ncerts,
cacerts, ncacerts,
cacertFile, isServer,
true, errp) < 0) {
goto cleanup;
}
- if (cert && ncacerts &&
- qcrypto_tls_creds_check_cert_pair(cert, certFile, cacerts,
- ncacerts, cacertFile,
+ if (ncerts && ncacerts &&
+ qcrypto_tls_creds_check_cert_pair(certs, ncerts, certFile,
+ cacerts, ncacerts, cacertFile,
isServer, errp) < 0) {
goto cleanup;
}
@@ -562,8 +546,8 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
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]);
diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c
index a7ea5f422d..4a32bc4d69 100644
--- a/tests/unit/test-crypto-tlscredsx509.c
+++ b/tests/unit/test-crypto-tlscredsx509.c
@@ -577,6 +577,12 @@ int main(int argc, char **argv)
true, true, GNUTLS_KEY_KEY_CERT_SIGN,
false, false, NULL, NULL,
0, 0);
+ TLS_ROOT_REQ(someotherrootreq,
+ "UK", "some other random CA", NULL, NULL, NULL, NULL,
+ true, true, true,
+ true, true, GNUTLS_KEY_KEY_CERT_SIGN,
+ false, false, NULL, NULL,
+ 0, 0);
TLS_CERT_REQ(cacertlevel1areq, cacertrootreq,
"UK", "qemu level 1a", NULL, NULL, NULL, NULL,
true, true, true,
@@ -623,6 +629,32 @@ int main(int argc, char **argv)
cacertlevel2areq.crt,
};
+ gnutls_x509_crt_t cabundle[] = {
+ someotherrootreq.crt,
+ cacertrootreq.crt,
+ };
+
+ gnutls_x509_crt_t servercertchain[] = {
+ servercertlevel3areq.crt,
+ cacertlevel2areq.crt,
+ cacertlevel1areq.crt,
+ };
+
+ gnutls_x509_crt_t servercertchain_incomplete[] = {
+ servercertlevel3areq.crt,
+ cacertlevel2areq.crt,
+ };
+
+ gnutls_x509_crt_t servercertchain_unsorted[] = {
+ servercertlevel3areq.crt,
+ cacertlevel1areq.crt,
+ cacertlevel2areq.crt,
+ };
+
+ gnutls_x509_crt_t clientcertchain[] = {
+ clientcertlevel2breq.crt,
+ cacertlevel1breq.crt,
+ };
test_tls_write_cert_chain(WORKDIR "cacertchain-ctx.pem",
certchain,
@@ -650,6 +682,46 @@ int main(int argc, char **argv)
WORKDIR "cacertchain-with-invalid-ctx.pem",
clientcertlevel2breq.filename, false);
+ test_tls_write_cert_chain(WORKDIR "servercertchain-ctx.pem",
+ servercertchain,
+ G_N_ELEMENTS(servercertchain));
+
+ TLS_TEST_REG(serverchain, true,
+ cacertrootreq.filename,
+ WORKDIR "servercertchain-ctx.pem", false);
+
+ test_tls_write_cert_chain(WORKDIR "cabundle-ctx.pem",
+ cabundle,
+ G_N_ELEMENTS(cabundle));
+
+ TLS_TEST_REG(multiplecaswithchain, true,
+ WORKDIR "cabundle-ctx.pem",
+ WORKDIR "servercertchain-ctx.pem", false);
+
+ test_tls_write_cert_chain(WORKDIR "servercertchain_incomplete-ctx.pem",
+ servercertchain_incomplete,
+ G_N_ELEMENTS(servercertchain_incomplete));
+
+ TLS_TEST_REG(incompleteserverchain, true,
+ cacertrootreq.filename,
+ WORKDIR "servercertchain_incomplete-ctx.pem", true);
+
+ test_tls_write_cert_chain(WORKDIR "servercertchain_unsorted-ctx.pem",
+ servercertchain_unsorted,
+ G_N_ELEMENTS(servercertchain_unsorted));
+
+ TLS_TEST_REG(unsortedserverchain, true,
+ cacertrootreq.filename,
+ WORKDIR "servercertchain_unsorted-ctx.pem", true);
+
+ test_tls_write_cert_chain(WORKDIR "clientcertchain-ctx.pem",
+ clientcertchain,
+ G_N_ELEMENTS(clientcertchain));
+
+ TLS_TEST_REG(clientchain, false,
+ cacertrootreq.filename,
+ WORKDIR "clientcertchain-ctx.pem", false);
+
/* Some missing certs - first two are fatal, the last
* is ok
*/
@@ -719,8 +791,13 @@ int main(int argc, char **argv)
test_tls_discard_cert(&cacertlevel2areq);
test_tls_discard_cert(&servercertlevel3areq);
test_tls_discard_cert(&clientcertlevel2breq);
+ test_tls_discard_cert(&someotherrootreq);
unlink(WORKDIR "cacertchain-ctx.pem");
unlink(WORKDIR "cacertchain-with-invalid-ctx.pem");
+ unlink(WORKDIR "servercertchain-ctx.pem");
+ unlink(WORKDIR "servercertchain_incomplete-ctx.pem");
+ unlink(WORKDIR "servercertchain_unsorted-ctx.pem");
+ unlink(WORKDIR "clientcertchain-ctx.pem");
test_tls_cleanup(KEYFILE);
rmdir(WORKDIR);
--
2.50.1
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...
> + 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 <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
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 :|
© 2016 - 2026 Red Hat, Inc.