The loop that checks the CA certificate chain can fail to report
an error message if one of the certs in the chain has an issuer
than is not present in the chain. In this case, the outer loop
'while (checking_issuer)' will terminate after failing to find
the issuer, and no error message will be reported.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/tlscredsx509.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 89a8e261d5..d42f2afaea 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -319,7 +319,6 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
                                         Error **errp)
 {
     gnutls_x509_crt_t cert_to_check = certs[ncerts - 1];
-    int checking_issuer = 1;
     int retval = 0;
     gnutls_datum_t dn = {}, dnissuer = {};
 
@@ -346,8 +345,8 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
         }
     }
 
-    while (checking_issuer) {
-        checking_issuer = 0;
+    for (;;) {
+        gnutls_x509_crt_t cert_issuer = NULL;
 
         if (gnutls_x509_crt_check_issuer(cert_to_check,
                                          cert_to_check)) {
@@ -362,19 +361,30 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
         for (int i = 0; i < ncacerts; i++) {
             if (gnutls_x509_crt_check_issuer(cert_to_check,
                                              cacerts[i])) {
-                retval = qcrypto_tls_creds_check_cert(
-                    creds, cacerts[i], cacertFile,
-                    isServer, isCA, errp);
-                if (retval < 0) {
-                    return retval;
-                }
-                cert_to_check = cacerts[i];
-                checking_issuer = 1;
+                cert_issuer = cacerts[i];
                 break;
             }
         }
+        if (!cert_issuer) {
+            break;
+        }
+
+        if (qcrypto_tls_creds_check_cert(creds, cert_issuer, cacertFile,
+                                         isServer, isCA, errp) < 0) {
+            return -1;
+        }
+
+        cert_to_check = cert_issuer;
     }
 
+    retval = gnutls_x509_crt_get_dn2(cert_to_check, &dn);
+    if (retval < 0) {
+        error_setg(errp, "Unable to fetch cert DN: %s",
+                   gnutls_strerror(retval));
+        return -1;
+    }
+    error_setg(errp, "Cert '%s' has no issuer in CA chain", dn.data);
+    gnutls_free(dn.data);
     return -1;
 }
 
-- 
2.50.1

On Fri, Sep 19, 2025 at 11:10:22AM +0100, Daniel P. Berrangé wrote:
> The loop that checks the CA certificate chain can fail to report
> an error message if one of the certs in the chain has an issuer
> than is not present in the chain. In this case, the outer loop

s/than/that/

> 'while (checking_issuer)' will terminate after failing to find
> the issuer, and no error message will be reported.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>  crypto/tlscredsx509.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
> 
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 89a8e261d5..d42f2afaea 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -319,7 +319,6 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
>                                          Error **errp)
>  {
>      gnutls_x509_crt_t cert_to_check = certs[ncerts - 1];
> -    int checking_issuer = 1;

This was the line I mentioned in patch 1/6 as needing a bool.  Should
this cleanup be squashed into that patch rather than having churn in
the series?

>      int retval = 0;
>      gnutls_datum_t dn = {}, dnissuer = {};
>

Should there be a testsuite patch along with this to provoke that
particular failure scenario?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

On Thu, Oct 16, 2025 at 10:50:44AM -0500, Eric Blake wrote:
> On Fri, Sep 19, 2025 at 11:10:22AM +0100, Daniel P. Berrangé wrote:
> > The loop that checks the CA certificate chain can fail to report
> > an error message if one of the certs in the chain has an issuer
> > than is not present in the chain. In this case, the outer loop
> 
> s/than/that/
> 
> > 'while (checking_issuer)' will terminate after failing to find
> > the issuer, and no error message will be reported.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> >  crypto/tlscredsx509.c | 32 +++++++++++++++++++++-----------
> >  1 file changed, 21 insertions(+), 11 deletions(-)
> > 
> > diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> > index 89a8e261d5..d42f2afaea 100644
> > --- a/crypto/tlscredsx509.c
> > +++ b/crypto/tlscredsx509.c
> > @@ -319,7 +319,6 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
> >                                          Error **errp)
> >  {
> >      gnutls_x509_crt_t cert_to_check = certs[ncerts - 1];
> > -    int checking_issuer = 1;
> 
> This was the line I mentioned in patch 1/6 as needing a bool.  Should
> this cleanup be squashed into that patch rather than having churn in
> the series?
> 
> >      int retval = 0;
> >      gnutls_datum_t dn = {}, dnissuer = {};
> >
> 
> Should there be a testsuite patch along with this to provoke that
> particular failure scenario?

Hmm, the test failure is introduced by patch #3 and this patch then
fixes it. IOW, I (reluctantly) need to squash this code into #3.

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