[PATCH 17/21] crypto: avoid loading the CA certs twice

Daniel P. Berrangé posted 21 patches 2 weeks, 1 day ago
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>
[PATCH 17/21] crypto: avoid loading the CA certs twice
Posted by Daniel P. Berrangé 2 weeks, 1 day ago
The x509 TLS credentials code will load the CA certs once to perform
sanity chcking on the certs, then discard the certificate objects
and let gnutls load them a second time.

This introduces a new QCryptoTLSCredsX509Files struct which will
hold the CA certificates loaded for sanity checking and pass them on
to gnutls, avoiding the duplicated loading.

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

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 7e79af4266..6a830af50d 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -40,6 +40,35 @@ struct QCryptoTLSCredsX509 {
 };
 
 
+typedef struct QCryptoTLSCredsX509Files QCryptoTLSCredsX509Files;
+struct QCryptoTLSCredsX509Files {
+    char *cacertpath;
+    gnutls_x509_crt_t *cacerts;
+    unsigned int ncacerts;
+};
+
+static QCryptoTLSCredsX509Files *
+qcrypto_tls_creds_x509_files_new(void)
+{
+    return g_new0(QCryptoTLSCredsX509Files, 1);
+}
+
+
+static void
+qcrypto_tls_creds_x509_files_free(QCryptoTLSCredsX509Files *files)
+{
+    size_t i;
+    for (i = 0; i < files->ncacerts; i++) {
+        gnutls_x509_crt_deinit(files->cacerts[i]);
+    }
+    g_free(files->cacerts);
+    g_free(files->cacertpath);
+    g_free(files);
+}
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoTLSCredsX509Files,
+                              qcrypto_tls_creds_x509_files_free);
+
 static int
 qcrypto_tls_creds_check_cert_times(gnutls_x509_crt_t cert,
                                    const char *certFile,
@@ -315,11 +344,9 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *creds,
 
 static int
 qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
+                                        QCryptoTLSCredsX509Files *files,
                                         gnutls_x509_crt_t *certs,
                                         unsigned int ncerts,
-                                        gnutls_x509_crt_t *cacerts,
-                                        unsigned int ncacerts,
-                                        const char *cacertFile,
                                         bool isServer,
                                         Error **errp)
 {
@@ -360,13 +387,13 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
              * reached the root of trust.
              */
             return qcrypto_tls_creds_check_cert(
-                creds, cert_to_check, cacertFile,
+                creds, cert_to_check, files->cacertpath,
                 isServer, true, errp);
         }
-        for (int i = 0; i < ncacerts; i++) {
+        for (int i = 0; i < files->ncacerts; i++) {
             if (gnutls_x509_crt_check_issuer(cert_to_check,
-                                             cacerts[i])) {
-                cert_issuer = cacerts[i];
+                                             files->cacerts[i])) {
+                cert_issuer = files->cacerts[i];
                 break;
             }
         }
@@ -374,7 +401,7 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
             break;
         }
 
-        if (qcrypto_tls_creds_check_cert(creds, cert_issuer, cacertFile,
+        if (qcrypto_tls_creds_check_cert(creds, cert_issuer, files->cacertpath,
                                          isServer, true, errp) < 0) {
             return -1;
         }
@@ -394,19 +421,17 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
 }
 
 static int
-qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t *certs,
+qcrypto_tls_creds_check_cert_pair(QCryptoTLSCredsX509Files *files,
+                                  gnutls_x509_crt_t *certs,
                                   size_t ncerts,
                                   const char *certFile,
-                                  gnutls_x509_crt_t *cacerts,
-                                  size_t ncacerts,
-                                  const char *cacertFile,
                                   bool isServer,
                                   Error **errp)
 {
     unsigned int status;
 
     if (gnutls_x509_crt_list_verify(certs, ncerts,
-                                    cacerts, ncacerts,
+                                    files->cacerts, files->ncacerts,
                                     NULL, 0,
                                     0, &status) < 0) {
         error_setg(errp, isServer ?
@@ -414,7 +439,7 @@ qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t *certs,
                    "CA certificate %s" :
                    "Unable to verify client certificate %s against "
                    "CA certificate %s",
-                   certFile, cacertFile);
+                   certFile, files->cacertpath);
         return -1;
     }
 
@@ -439,7 +464,7 @@ qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t *certs,
 
         error_setg(errp,
                    "Our own certificate %s failed validation against %s: %s",
-                   certFile, cacertFile, reason);
+                   certFile, files->cacertpath, reason);
         return -1;
     }
 
@@ -490,15 +515,13 @@ qcrypto_tls_creds_load_cert_list(QCryptoTLSCredsX509 *creds,
 
 static int
 qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
+                                    QCryptoTLSCredsX509Files *files,
                                     bool isServer,
-                                    const char *cacertFile,
                                     const char *certFile,
                                     Error **errp)
 {
     gnutls_x509_crt_t *certs = NULL;
     unsigned int ncerts = 0;
-    gnutls_x509_crt_t *cacerts = NULL;
-    unsigned int ncacerts = 0;
     size_t i;
     int ret = -1;
 
@@ -514,16 +537,6 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
         }
     }
 
-    if (qcrypto_tls_creds_load_cert_list(creds,
-                                         cacertFile,
-                                         &cacerts,
-                                         &ncacerts,
-                                         isServer,
-                                         true,
-                                         errp) < 0) {
-        goto cleanup;
-    }
-
     for (i = 0; i < ncerts; i++) {
         if (qcrypto_tls_creds_check_cert(creds,
                                          certs[i], certFile,
@@ -533,17 +546,14 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
     }
 
     if (ncerts &&
-        qcrypto_tls_creds_check_authority_chain(creds,
+        qcrypto_tls_creds_check_authority_chain(creds, files,
                                                 certs, ncerts,
-                                                cacerts, ncacerts,
-                                                cacertFile, isServer,
-                                                errp) < 0) {
+                                                isServer, errp) < 0) {
         goto cleanup;
     }
 
-    if (ncerts && ncacerts &&
-        qcrypto_tls_creds_check_cert_pair(certs, ncerts, certFile,
-                                          cacerts, ncacerts, cacertFile,
+    if (ncerts &&
+        qcrypto_tls_creds_check_cert_pair(files, certs, ncerts, certFile,
                                           isServer, errp) < 0) {
         goto cleanup;
     }
@@ -555,21 +565,53 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
         gnutls_x509_crt_deinit(certs[i]);
     }
     g_free(certs);
-    for (i = 0; i < ncacerts; i++) {
-        gnutls_x509_crt_deinit(cacerts[i]);
-    }
-    g_free(cacerts);
 
     return ret;
 }
 
 
+static int
+qcrypto_tls_creds_x509_load_ca(QCryptoTLSCredsX509 *creds,
+                               QCryptoTLSCredsBox *box,
+                               QCryptoTLSCredsX509Files *files,
+                               bool isServer,
+                               Error **errp)
+{
+    int ret;
+
+    if (qcrypto_tls_creds_get_path(&creds->parent_obj,
+                                   QCRYPTO_TLS_CREDS_X509_CA_CERT,
+                                   true, &files->cacertpath, errp) < 0) {
+        return -1;
+    }
+
+    if (qcrypto_tls_creds_load_cert_list(creds,
+                                         files->cacertpath,
+                                         &files->cacerts,
+                                         &files->ncacerts,
+                                         isServer,
+                                         true,
+                                         errp) < 0) {
+        return -1;
+    }
+
+    ret = gnutls_certificate_set_x509_trust(box->data.cert,
+                                            files->cacerts, files->ncacerts);
+    if (ret < 0) {
+        error_setg(errp, "Cannot set CA certificate '%s': %s",
+                   files->cacertpath, gnutls_strerror(ret));
+        return -1;
+    }
+
+    return 0;
+}
+
 static int
 qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
                             Error **errp)
 {
     g_autoptr(QCryptoTLSCredsBox) box = NULL;
-    g_autofree char *cacert = NULL;
+    g_autoptr(QCryptoTLSCredsX509Files) files = NULL;
     g_autofree char *cacrl = NULL;
     g_autofree char *cert = NULL;
     g_autofree char *key = NULL;
@@ -598,9 +640,9 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
         return -1;
     }
 
-    if (qcrypto_tls_creds_get_path(&creds->parent_obj,
-                                   QCRYPTO_TLS_CREDS_X509_CA_CERT,
-                                   true, &cacert, errp) < 0) {
+    files = qcrypto_tls_creds_x509_files_new();
+
+    if (qcrypto_tls_creds_x509_load_ca(creds, box, files, isServer, errp) < 0) {
         return -1;
     }
 
@@ -631,17 +673,8 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
     }
 
     if (creds->sanityCheck &&
-        qcrypto_tls_creds_x509_sanity_check(creds, isServer,
-                                            cacert, cert, errp) < 0) {
-        return -1;
-    }
-
-    ret = gnutls_certificate_set_x509_trust_file(box->data.cert,
-                                                 cacert,
-                                                 GNUTLS_X509_FMT_PEM);
-    if (ret < 0) {
-        error_setg(errp, "Cannot load CA certificate '%s': %s",
-                   cacert, gnutls_strerror(ret));
+        qcrypto_tls_creds_x509_sanity_check(creds, files, isServer,
+                                            cert, errp) < 0) {
         return -1;
     }
 
-- 
2.51.1


Re: [PATCH 17/21] crypto: avoid loading the CA certs twice
Posted by Marc-André Lureau via Devel 2 weeks ago
On Thu, Oct 30, 2025 at 6:50 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> The x509 TLS credentials code will load the CA certs once to perform
> sanity chcking on the certs, then discard the certificate objects
> and let gnutls load them a second time.
>
> This introduces a new QCryptoTLSCredsX509Files struct which will
> hold the CA certificates loaded for sanity checking and pass them on
> to gnutls, avoiding the duplicated loading.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>  crypto/tlscredsx509.c | 141 ++++++++++++++++++++++++++----------------
>  1 file changed, 87 insertions(+), 54 deletions(-)
>
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 7e79af4266..6a830af50d 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -40,6 +40,35 @@ struct QCryptoTLSCredsX509 {
>  };
>
>
> +typedef struct QCryptoTLSCredsX509Files QCryptoTLSCredsX509Files;
> +struct QCryptoTLSCredsX509Files {
> +    char *cacertpath;
> +    gnutls_x509_crt_t *cacerts;
> +    unsigned int ncacerts;
> +};
> +
> +static QCryptoTLSCredsX509Files *
> +qcrypto_tls_creds_x509_files_new(void)
> +{
> +    return g_new0(QCryptoTLSCredsX509Files, 1);
> +}
> +
> +
> +static void
> +qcrypto_tls_creds_x509_files_free(QCryptoTLSCredsX509Files *files)
> +{
> +    size_t i;
> +    for (i = 0; i < files->ncacerts; i++) {
> +        gnutls_x509_crt_deinit(files->cacerts[i]);
> +    }
> +    g_free(files->cacerts);
> +    g_free(files->cacertpath);
> +    g_free(files);
> +}
> +
> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoTLSCredsX509Files,
> +                              qcrypto_tls_creds_x509_files_free);
> +
>  static int
>  qcrypto_tls_creds_check_cert_times(gnutls_x509_crt_t cert,
>                                     const char *certFile,
> @@ -315,11 +344,9 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509
> *creds,
>
>  static int
>  qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
> +                                        QCryptoTLSCredsX509Files *files,
>                                          gnutls_x509_crt_t *certs,
>                                          unsigned int ncerts,
> -                                        gnutls_x509_crt_t *cacerts,
> -                                        unsigned int ncacerts,
> -                                        const char *cacertFile,
>                                          bool isServer,
>                                          Error **errp)
>  {
> @@ -360,13 +387,13 @@
> qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
>               * reached the root of trust.
>               */
>              return qcrypto_tls_creds_check_cert(
> -                creds, cert_to_check, cacertFile,
> +                creds, cert_to_check, files->cacertpath,
>                  isServer, true, errp);
>          }
> -        for (int i = 0; i < ncacerts; i++) {
> +        for (int i = 0; i < files->ncacerts; i++) {
>              if (gnutls_x509_crt_check_issuer(cert_to_check,
> -                                             cacerts[i])) {
> -                cert_issuer = cacerts[i];
> +                                             files->cacerts[i])) {
> +                cert_issuer = files->cacerts[i];
>                  break;
>              }
>          }
> @@ -374,7 +401,7 @@
> qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
>              break;
>          }
>
> -        if (qcrypto_tls_creds_check_cert(creds, cert_issuer, cacertFile,
> +        if (qcrypto_tls_creds_check_cert(creds, cert_issuer,
> files->cacertpath,
>                                           isServer, true, errp) < 0) {
>              return -1;
>          }
> @@ -394,19 +421,17 @@
> qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
>  }
>
>  static int
> -qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t *certs,
> +qcrypto_tls_creds_check_cert_pair(QCryptoTLSCredsX509Files *files,
> +                                  gnutls_x509_crt_t *certs,
>                                    size_t ncerts,
>                                    const char *certFile,
> -                                  gnutls_x509_crt_t *cacerts,
> -                                  size_t ncacerts,
> -                                  const char *cacertFile,
>                                    bool isServer,
>                                    Error **errp)
>  {
>      unsigned int status;
>
>      if (gnutls_x509_crt_list_verify(certs, ncerts,
> -                                    cacerts, ncacerts,
> +                                    files->cacerts, files->ncacerts,
>                                      NULL, 0,
>                                      0, &status) < 0) {
>          error_setg(errp, isServer ?
> @@ -414,7 +439,7 @@ qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t
> *certs,
>                     "CA certificate %s" :
>                     "Unable to verify client certificate %s against "
>                     "CA certificate %s",
> -                   certFile, cacertFile);
> +                   certFile, files->cacertpath);
>          return -1;
>      }
>
> @@ -439,7 +464,7 @@ qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t
> *certs,
>
>          error_setg(errp,
>                     "Our own certificate %s failed validation against %s:
> %s",
> -                   certFile, cacertFile, reason);
> +                   certFile, files->cacertpath, reason);
>          return -1;
>      }
>
> @@ -490,15 +515,13 @@ qcrypto_tls_creds_load_cert_list(QCryptoTLSCredsX509
> *creds,
>
>  static int
>  qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
> +                                    QCryptoTLSCredsX509Files *files,
>                                      bool isServer,
> -                                    const char *cacertFile,
>                                      const char *certFile,
>                                      Error **errp)
>  {
>      gnutls_x509_crt_t *certs = NULL;
>      unsigned int ncerts = 0;
> -    gnutls_x509_crt_t *cacerts = NULL;
> -    unsigned int ncacerts = 0;
>      size_t i;
>      int ret = -1;
>
> @@ -514,16 +537,6 @@
> qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
>          }
>      }
>
> -    if (qcrypto_tls_creds_load_cert_list(creds,
> -                                         cacertFile,
> -                                         &cacerts,
> -                                         &ncacerts,
> -                                         isServer,
> -                                         true,
> -                                         errp) < 0) {
> -        goto cleanup;
> -    }
> -
>      for (i = 0; i < ncerts; i++) {
>          if (qcrypto_tls_creds_check_cert(creds,
>                                           certs[i], certFile,
> @@ -533,17 +546,14 @@
> qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
>      }
>
>      if (ncerts &&
> -        qcrypto_tls_creds_check_authority_chain(creds,
> +        qcrypto_tls_creds_check_authority_chain(creds, files,
>                                                  certs, ncerts,
> -                                                cacerts, ncacerts,
> -                                                cacertFile, isServer,
> -                                                errp) < 0) {
> +                                                isServer, errp) < 0) {
>          goto cleanup;
>      }
>
> -    if (ncerts && ncacerts &&
> -        qcrypto_tls_creds_check_cert_pair(certs, ncerts, certFile,
> -                                          cacerts, ncacerts, cacertFile,
> +    if (ncerts &&
> +        qcrypto_tls_creds_check_cert_pair(files, certs, ncerts, certFile,
>                                            isServer, errp) < 0) {
>          goto cleanup;
>      }
> @@ -555,21 +565,53 @@
> qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
>          gnutls_x509_crt_deinit(certs[i]);
>      }
>      g_free(certs);
> -    for (i = 0; i < ncacerts; i++) {
> -        gnutls_x509_crt_deinit(cacerts[i]);
> -    }
> -    g_free(cacerts);
>
>      return ret;
>  }
>
>
> +static int
> +qcrypto_tls_creds_x509_load_ca(QCryptoTLSCredsX509 *creds,
> +                               QCryptoTLSCredsBox *box,
> +                               QCryptoTLSCredsX509Files *files,
> +                               bool isServer,
> +                               Error **errp)
> +{
> +    int ret;
> +
> +    if (qcrypto_tls_creds_get_path(&creds->parent_obj,
> +                                   QCRYPTO_TLS_CREDS_X509_CA_CERT,
> +                                   true, &files->cacertpath, errp) < 0) {
> +        return -1;
> +    }
> +
> +    if (qcrypto_tls_creds_load_cert_list(creds,
> +                                         files->cacertpath,
> +                                         &files->cacerts,
> +                                         &files->ncacerts,
> +                                         isServer,
> +                                         true,
> +                                         errp) < 0) {
> +        return -1;
> +    }
> +
> +    ret = gnutls_certificate_set_x509_trust(box->data.cert,
> +                                            files->cacerts,
> files->ncacerts);
> +    if (ret < 0) {
> +        error_setg(errp, "Cannot set CA certificate '%s': %s",
> +                   files->cacertpath, gnutls_strerror(ret));
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
>  static int
>  qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
>                              Error **errp)
>  {
>      g_autoptr(QCryptoTLSCredsBox) box = NULL;
> -    g_autofree char *cacert = NULL;
> +    g_autoptr(QCryptoTLSCredsX509Files) files = NULL;
>      g_autofree char *cacrl = NULL;
>      g_autofree char *cert = NULL;
>      g_autofree char *key = NULL;
> @@ -598,9 +640,9 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
>          return -1;
>      }
>
> -    if (qcrypto_tls_creds_get_path(&creds->parent_obj,
> -                                   QCRYPTO_TLS_CREDS_X509_CA_CERT,
> -                                   true, &cacert, errp) < 0) {
> +    files = qcrypto_tls_creds_x509_files_new();
> +
> +    if (qcrypto_tls_creds_x509_load_ca(creds, box, files, isServer, errp)
> < 0) {
>          return -1;
>      }
>
> @@ -631,17 +673,8 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509
> *creds,
>      }
>
>      if (creds->sanityCheck &&
> -        qcrypto_tls_creds_x509_sanity_check(creds, isServer,
> -                                            cacert, cert, errp) < 0) {
> -        return -1;
> -    }
> -
> -    ret = gnutls_certificate_set_x509_trust_file(box->data.cert,
> -                                                 cacert,
> -                                                 GNUTLS_X509_FMT_PEM);
> -    if (ret < 0) {
> -        error_setg(errp, "Cannot load CA certificate '%s': %s",
> -                   cacert, gnutls_strerror(ret));
> +        qcrypto_tls_creds_x509_sanity_check(creds, files, isServer,
> +                                            cert, errp) < 0) {
>          return -1;
>      }
>
> --
> 2.51.1
>
>