[PATCH 00/21] crypto: support multiple parallel certificate identities

[PATCH 00/21] crypto: support multiple parallel certificate identities

[Daniel P. Berrangé]

This series aims to improve the support for post-quantum cryptography
in TLS connections by allowing  multiple sets of certificates to be
loaded. The idea is that during a transition period servers will have
a traditional RSA based certificate in parallel with an MLDSA based
certificate for PQC, and the right one will be dynamically determined
during the TLS handshake.

The first 12 patches are trivial cleanups.

The next 3 patches fix a potential use-after-free problem

The last patches introduce support for multiple certificates.

NB, in terms of testing this will require either CentOS Stream 10,
or Fedora 43. Most other distros will not support PQC out of the
box at this time even if they have new enough gnutls, since they
don't make use of the crypto-policies package which is needed to
enable PQC by default.

Daniel P. Berrangé (21):
  crypto: remove redundant parameter checking CA certs
  crypto: add missing free of certs array
  crypto: replace stat() with access() for credential checks
  crypto: remove redundant access() checks before loading certs
  crypto: move check for TLS creds 'dir' property
  crypto: use g_autofree when loading x509 credentials
  crypto: remove needless indirection via parent_obj field
  crypto: move release of DH parameters into TLS creds parent
  crypto: shorten the endpoint == server check in TLS creds
  crypto: remove duplication loading x509 CA cert
  crypto: reduce duplication in handling TLS priority strings
  crypto: introduce method for reloading TLS creds
  crypto: introduce a wrapper around gnutls credentials
  crypto: fix lifecycle handling of gnutls credentials objects
  crypto: make TLS credentials structs private
  crypto: deprecate use of external dh-params.pem file
  crypto: avoid loading the CA certs twice
  crypto: avoid loading the identity certs twice
  crypto: expand logic to cope with multiple certificate identities
  crypto: support upto 5 parallel certificate identities
  docs: creation of x509 certs compliant with post-quantum crypto

 crypto/meson.build                    |   5 +-
 crypto/tlscreds.c                     |  77 ++--
 crypto/tlscredsanon.c                 |  62 +--
 crypto/tlscredsbox.c                  | 101 +++++
 crypto/tlscredsbox.h                  |  46 ++
 crypto/tlscredspriv.h                 |  36 +-
 crypto/tlscredspsk.c                  |  64 ++-
 crypto/tlscredsx509.c                 | 592 +++++++++++++++++---------
 crypto/tlssession.c                   | 139 ++----
 crypto/trace-events                   |   1 +
 docs/about/deprecated.rst             |   9 +
 docs/system/tls.rst                   | 134 +++++-
 include/crypto/tlscreds.h             |  26 ++
 include/crypto/tlscredsx509.h         |   6 +
 tests/unit/test-crypto-tlscredsx509.c |   8 +-
 ui/vnc.c                              |   9 +-
 16 files changed, 849 insertions(+), 466 deletions(-)
 create mode 100644 crypto/tlscredsbox.c
 create mode 100644 crypto/tlscredsbox.h

-- 
2.51.1

[PATCH 01/21] crypto: remove redundant parameter checking CA certs

[Daniel P. Berrangé]

The only caller of qcrypto_tls_creds_check_authority_chain always
passes 'true' for the 'isCA' parameter. The point of this method
is to check the CA chani, so no other value would ever make sense.

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

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index db2b74bafa..847fd4d9fa 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -315,7 +315,6 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
                                         unsigned int ncacerts,
                                         const char *cacertFile,
                                         bool isServer,
-                                        bool isCA,
                                         Error **errp)
 {
     gnutls_x509_crt_t cert_to_check = certs[ncerts - 1];
@@ -356,7 +355,7 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
              */
             return qcrypto_tls_creds_check_cert(
                 creds, cert_to_check, cacertFile,
-                isServer, isCA, errp);
+                isServer, true, errp);
         }
         for (int i = 0; i < ncacerts; i++) {
             if (gnutls_x509_crt_check_issuer(cert_to_check,
@@ -370,7 +369,7 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
         }
 
         if (qcrypto_tls_creds_check_cert(creds, cert_issuer, cacertFile,
-                                         isServer, isCA, errp) < 0) {
+                                         isServer, true, errp) < 0) {
             return -1;
         }
 
@@ -534,7 +533,7 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
                                                 certs, ncerts,
                                                 cacerts, ncacerts,
                                                 cacertFile, isServer,
-                                                true, errp) < 0) {
+                                                errp) < 0) {
         goto cleanup;
     }
 
-- 
2.51.1

[PATCH 02/21] crypto: add missing free of certs array

[Daniel P. Berrangé]

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

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 847fd4d9fa..75c70af522 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -550,6 +550,7 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
     for (i = 0; i < ncerts; i++) {
         gnutls_x509_crt_deinit(certs[i]);
     }
+    g_free(certs);
     for (i = 0; i < ncacerts; i++) {
         gnutls_x509_crt_deinit(cacerts[i]);
     }
-- 
2.51.1

[PATCH 03/21] crypto: replace stat() with access() for credential checks

[Daniel P. Berrangé]

Readability of the credential files is what matters for our usage,
so access() is more appropriate than stat().

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/tlscreds.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/crypto/tlscreds.c b/crypto/tlscreds.c
index 9e59594d67..208a7e6d8f 100644
--- a/crypto/tlscreds.c
+++ b/crypto/tlscreds.c
@@ -100,7 +100,6 @@ qcrypto_tls_creds_get_path(QCryptoTLSCreds *creds,
                            char **cred,
                            Error **errp)
 {
-    struct stat sb;
     int ret = -1;
 
     if (!creds->dir) {
@@ -114,7 +113,7 @@ qcrypto_tls_creds_get_path(QCryptoTLSCreds *creds,
 
     *cred = g_strdup_printf("%s/%s", creds->dir, filename);
 
-    if (stat(*cred, &sb) < 0) {
+    if (access(*cred, R_OK) < 0) {
         if (errno == ENOENT && !required) {
             ret = 0;
         } else {
-- 
2.51.1

[PATCH 04/21] crypto: remove redundant access() checks before loading certs

[Daniel P. Berrangé]

The qcrypto_tls_creds_get_path method will perform an access()
check on the file and return a NULL path if it fails. By the
time we get to loading the cert files we know they must exist
on disk and thus the second access() check is redundant.

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

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 75c70af522..0acb17b6ec 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -496,8 +496,7 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
     size_t i;
     int ret = -1;
 
-    if (certFile &&
-        access(certFile, R_OK) == 0) {
+    if (certFile) {
         if (qcrypto_tls_creds_load_cert_list(creds,
                                              certFile,
                                              &certs,
@@ -508,16 +507,15 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
             goto cleanup;
         }
     }
-    if (access(cacertFile, R_OK) == 0) {
-        if (qcrypto_tls_creds_load_cert_list(creds,
-                                             cacertFile,
-                                             &cacerts,
-                                             &ncacerts,
-                                             isServer,
-                                             true,
-                                             errp) < 0) {
-            goto cleanup;
-        }
+
+    if (qcrypto_tls_creds_load_cert_list(creds,
+                                         cacertFile,
+                                         &cacerts,
+                                         &ncacerts,
+                                         isServer,
+                                         true,
+                                         errp) < 0) {
+        goto cleanup;
     }
 
     for (i = 0; i < ncerts; i++) {
-- 
2.51.1

[PATCH 05/21] crypto: move check for TLS creds 'dir' property

[Daniel P. Berrangé]

The check for the 'dir' property is being repeated for every
credential file to be loaded, but this results in incorrect
logic for optional credentials. The 'dir' property is mandatory
for PSK and x509 creds, even if some individual files are
optional. Address this by separating the check for the 'dir'
property.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 crypto/tlscreds.c     | 9 ---------
 crypto/tlscredsanon.c | 3 ++-
 crypto/tlscredspsk.c  | 5 +++++
 crypto/tlscredsx509.c | 8 ++++++--
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/crypto/tlscreds.c b/crypto/tlscreds.c
index 208a7e6d8f..65e97ddd11 100644
--- a/crypto/tlscreds.c
+++ b/crypto/tlscreds.c
@@ -102,15 +102,6 @@ qcrypto_tls_creds_get_path(QCryptoTLSCreds *creds,
 {
     int ret = -1;
 
-    if (!creds->dir) {
-        if (required) {
-            error_setg(errp, "Missing 'dir' property value");
-            return -1;
-        } else {
-            return 0;
-        }
-    }
-
     *cred = g_strdup_printf("%s/%s", creds->dir, filename);
 
     if (access(*cred, R_OK) < 0) {
diff --git a/crypto/tlscredsanon.c b/crypto/tlscredsanon.c
index 44af9e6c9a..bc3351b5d6 100644
--- a/crypto/tlscredsanon.c
+++ b/crypto/tlscredsanon.c
@@ -43,7 +43,8 @@ qcrypto_tls_creds_anon_load(QCryptoTLSCredsAnon *creds,
             creds->parent_obj.dir ? creds->parent_obj.dir : "<nodir>");
 
     if (creds->parent_obj.endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
-        if (qcrypto_tls_creds_get_path(&creds->parent_obj,
+        if (creds->parent_obj.dir &&
+            qcrypto_tls_creds_get_path(&creds->parent_obj,
                                        QCRYPTO_TLS_CREDS_DH_PARAMS,
                                        false, &dhparams, errp) < 0) {
             return -1;
diff --git a/crypto/tlscredspsk.c b/crypto/tlscredspsk.c
index 5b68a6b7ba..545d3e45db 100644
--- a/crypto/tlscredspsk.c
+++ b/crypto/tlscredspsk.c
@@ -81,6 +81,11 @@ qcrypto_tls_creds_psk_load(QCryptoTLSCredsPSK *creds,
     trace_qcrypto_tls_creds_psk_load(creds,
             creds->parent_obj.dir ? creds->parent_obj.dir : "<nodir>");
 
+    if (!creds->parent_obj.dir) {
+        error_setg(errp, "Missing 'dir' property value");
+        goto cleanup;
+    }
+
     if (creds->parent_obj.endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
         if (creds->username) {
             error_setg(errp, "username should not be set when endpoint=server");
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 0acb17b6ec..8fe6cc8e93 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -567,8 +567,12 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
     int ret;
     int rv = -1;
 
-    trace_qcrypto_tls_creds_x509_load(creds,
-            creds->parent_obj.dir ? creds->parent_obj.dir : "<nodir>");
+    if (!creds->parent_obj.dir) {
+        error_setg(errp, "Missing 'dir' property value");
+        return -1;
+    }
+
+    trace_qcrypto_tls_creds_x509_load(creds, creds->parent_obj.dir);
 
     if (creds->parent_obj.endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
         if (qcrypto_tls_creds_get_path(&creds->parent_obj,
-- 
2.51.1

[PATCH 06/21] crypto: use g_autofree when loading x509 credentials

[Daniel P. Berrangé]

This allows removal of goto jumps during loading of the credentials
and will simplify the diff in following commits.

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

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 8fe6cc8e93..e5b869a35f 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -562,10 +562,12 @@ static int
 qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
                             Error **errp)
 {
-    char *cacert = NULL, *cacrl = NULL, *cert = NULL,
-        *key = NULL, *dhparams = NULL;
+    g_autofree char *cacert = NULL;
+    g_autofree char *cacrl = NULL;
+    g_autofree char *cert = NULL;
+    g_autofree char *key = NULL;
+    g_autofree char *dhparams = NULL;
     int ret;
-    int rv = -1;
 
     if (!creds->parent_obj.dir) {
         error_setg(errp, "Missing 'dir' property value");
@@ -590,7 +592,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
             qcrypto_tls_creds_get_path(&creds->parent_obj,
                                        QCRYPTO_TLS_CREDS_DH_PARAMS,
                                        false, &dhparams, errp) < 0) {
-            goto cleanup;
+            return -1;
         }
     } else {
         if (qcrypto_tls_creds_get_path(&creds->parent_obj,
@@ -602,7 +604,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
             qcrypto_tls_creds_get_path(&creds->parent_obj,
                                        QCRYPTO_TLS_CREDS_X509_CLIENT_KEY,
                                        false, &key, errp) < 0) {
-            goto cleanup;
+            return -1;
         }
     }
 
@@ -610,14 +612,14 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
         qcrypto_tls_creds_x509_sanity_check(creds,
             creds->parent_obj.endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
             cacert, cert, errp) < 0) {
-        goto cleanup;
+        return -1;
     }
 
     ret = gnutls_certificate_allocate_credentials(&creds->data);
     if (ret < 0) {
         error_setg(errp, "Cannot allocate credentials: '%s'",
                    gnutls_strerror(ret));
-        goto cleanup;
+        return -1;
     }
 
     ret = gnutls_certificate_set_x509_trust_file(creds->data,
@@ -626,7 +628,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
     if (ret < 0) {
         error_setg(errp, "Cannot load CA certificate '%s': %s",
                    cacert, gnutls_strerror(ret));
-        goto cleanup;
+        return -1;
     }
 
     if (cert != NULL && key != NULL) {
@@ -635,7 +637,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
             password = qcrypto_secret_lookup_as_utf8(creds->passwordid,
                                                      errp);
             if (!password) {
-                goto cleanup;
+                return -1;
             }
         }
         ret = gnutls_certificate_set_x509_key_file2(creds->data,
@@ -647,7 +649,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
         if (ret < 0) {
             error_setg(errp, "Cannot load certificate '%s' & key '%s': %s",
                        cert, key, gnutls_strerror(ret));
-            goto cleanup;
+            return -1;
         }
     }
 
@@ -658,7 +660,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
         if (ret < 0) {
             error_setg(errp, "Cannot load CRL '%s': %s",
                        cacrl, gnutls_strerror(ret));
-            goto cleanup;
+            return -1;
         }
     }
 
@@ -666,20 +668,13 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
         if (qcrypto_tls_creds_get_dh_params_file(&creds->parent_obj, dhparams,
                                                  &creds->parent_obj.dh_params,
                                                  errp) < 0) {
-            goto cleanup;
+            return -1;
         }
         gnutls_certificate_set_dh_params(creds->data,
                                          creds->parent_obj.dh_params);
     }
 
-    rv = 0;
- cleanup:
-    g_free(cacert);
-    g_free(cacrl);
-    g_free(cert);
-    g_free(key);
-    g_free(dhparams);
-    return rv;
+    return 0;
 }
 
 
-- 
2.51.1

[PATCH 01/21] crypto: remove redundant parameter checking CA certs

[Daniel P. Berrangé]

The only caller of qcrypto_tls_creds_check_authority_chain always
passes 'true' for the 'isCA' parameter. The point of this method
is to check the CA chani, so no other value would ever make sense.

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

diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index db2b74bafa..847fd4d9fa 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -315,7 +315,6 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
                                         unsigned int ncacerts,
                                         const char *cacertFile,
                                         bool isServer,
-                                        bool isCA,
                                         Error **errp)
 {
     gnutls_x509_crt_t cert_to_check = certs[ncerts - 1];
@@ -356,7 +355,7 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
              */
             return qcrypto_tls_creds_check_cert(
                 creds, cert_to_check, cacertFile,
-                isServer, isCA, errp);
+                isServer, true, errp);
         }
         for (int i = 0; i < ncacerts; i++) {
             if (gnutls_x509_crt_check_issuer(cert_to_check,
@@ -370,7 +369,7 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
         }
 
         if (qcrypto_tls_creds_check_cert(creds, cert_issuer, cacertFile,
-                                         isServer, isCA, errp) < 0) {
+                                         isServer, true, errp) < 0) {
             return -1;
         }
 
@@ -534,7 +533,7 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
                                                 certs, ncerts,
                                                 cacerts, ncacerts,
                                                 cacertFile, isServer,
-                                                true, errp) < 0) {
+                                                errp) < 0) {
         goto cleanup;
     }
 
-- 
2.51.1

Re: [PATCH 00/21] crypto: support multiple parallel certificate identities

[Daniel P. Berrangé]

Sorry, this series aborted during mail sending so is incomplete.

See the immediately following re-post for the full set of 21 patches

On Thu, Oct 30, 2025 at 02:47:44PM +0000, Daniel P. Berrangé wrote:
> This series aims to improve the support for post-quantum cryptography
> in TLS connections by allowing  multiple sets of certificates to be
> loaded. The idea is that during a transition period servers will have
> a traditional RSA based certificate in parallel with an MLDSA based
> certificate for PQC, and the right one will be dynamically determined
> during the TLS handshake.
> 
> The first 12 patches are trivial cleanups.
> 
> The next 3 patches fix a potential use-after-free problem
> 
> The last patches introduce support for multiple certificates.
> 
> NB, in terms of testing this will require either CentOS Stream 10,
> or Fedora 43. Most other distros will not support PQC out of the
> box at this time even if they have new enough gnutls, since they
> don't make use of the crypto-policies package which is needed to
> enable PQC by default.
> 
> Daniel P. Berrangé (21):
>   crypto: remove redundant parameter checking CA certs
>   crypto: add missing free of certs array
>   crypto: replace stat() with access() for credential checks
>   crypto: remove redundant access() checks before loading certs
>   crypto: move check for TLS creds 'dir' property
>   crypto: use g_autofree when loading x509 credentials
>   crypto: remove needless indirection via parent_obj field
>   crypto: move release of DH parameters into TLS creds parent
>   crypto: shorten the endpoint == server check in TLS creds
>   crypto: remove duplication loading x509 CA cert
>   crypto: reduce duplication in handling TLS priority strings
>   crypto: introduce method for reloading TLS creds
>   crypto: introduce a wrapper around gnutls credentials
>   crypto: fix lifecycle handling of gnutls credentials objects
>   crypto: make TLS credentials structs private
>   crypto: deprecate use of external dh-params.pem file
>   crypto: avoid loading the CA certs twice
>   crypto: avoid loading the identity certs twice
>   crypto: expand logic to cope with multiple certificate identities
>   crypto: support upto 5 parallel certificate identities
>   docs: creation of x509 certs compliant with post-quantum crypto
> 
>  crypto/meson.build                    |   5 +-
>  crypto/tlscreds.c                     |  77 ++--
>  crypto/tlscredsanon.c                 |  62 +--
>  crypto/tlscredsbox.c                  | 101 +++++
>  crypto/tlscredsbox.h                  |  46 ++
>  crypto/tlscredspriv.h                 |  36 +-
>  crypto/tlscredspsk.c                  |  64 ++-
>  crypto/tlscredsx509.c                 | 592 +++++++++++++++++---------
>  crypto/tlssession.c                   | 139 ++----
>  crypto/trace-events                   |   1 +
>  docs/about/deprecated.rst             |   9 +
>  docs/system/tls.rst                   | 134 +++++-
>  include/crypto/tlscreds.h             |  26 ++
>  include/crypto/tlscredsx509.h         |   6 +
>  tests/unit/test-crypto-tlscredsx509.c |   8 +-
>  ui/vnc.c                              |   9 +-
>  16 files changed, 849 insertions(+), 466 deletions(-)
>  create mode 100644 crypto/tlscredsbox.c
>  create mode 100644 crypto/tlscredsbox.h
> 
> -- 
> 2.51.1
> 

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

Re: [PATCH 06/21] crypto: use g_autofree when loading x509 credentials

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 4855 bytes --]

Hi

On Thu, Oct 30, 2025 at 6:48 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> This allows removal of goto jumps during loading of the credentials
> and will simplify the diff in following commits.
>
>
If you want, you could also g_autofree password.


> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>

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


> ---
>  crypto/tlscredsx509.c | 35 +++++++++++++++--------------------
>  1 file changed, 15 insertions(+), 20 deletions(-)
>
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 8fe6cc8e93..e5b869a35f 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -562,10 +562,12 @@ static int
>  qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
>                              Error **errp)
>  {
> -    char *cacert = NULL, *cacrl = NULL, *cert = NULL,
> -        *key = NULL, *dhparams = NULL;
> +    g_autofree char *cacert = NULL;
> +    g_autofree char *cacrl = NULL;
> +    g_autofree char *cert = NULL;
> +    g_autofree char *key = NULL;
> +    g_autofree char *dhparams = NULL;
>      int ret;
> -    int rv = -1;
>
>      if (!creds->parent_obj.dir) {
>          error_setg(errp, "Missing 'dir' property value");
> @@ -590,7 +592,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
>              qcrypto_tls_creds_get_path(&creds->parent_obj,
>                                         QCRYPTO_TLS_CREDS_DH_PARAMS,
>                                         false, &dhparams, errp) < 0) {
> -            goto cleanup;
> +            return -1;
>          }
>      } else {
>          if (qcrypto_tls_creds_get_path(&creds->parent_obj,
> @@ -602,7 +604,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
>              qcrypto_tls_creds_get_path(&creds->parent_obj,
>                                         QCRYPTO_TLS_CREDS_X509_CLIENT_KEY,
>                                         false, &key, errp) < 0) {
> -            goto cleanup;
> +            return -1;
>          }
>      }
>
> @@ -610,14 +612,14 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509
> *creds,
>          qcrypto_tls_creds_x509_sanity_check(creds,
>              creds->parent_obj.endpoint ==
> QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
>              cacert, cert, errp) < 0) {
> -        goto cleanup;
> +        return -1;
>      }
>
>      ret = gnutls_certificate_allocate_credentials(&creds->data);
>      if (ret < 0) {
>          error_setg(errp, "Cannot allocate credentials: '%s'",
>                     gnutls_strerror(ret));
> -        goto cleanup;
> +        return -1;
>      }
>
>      ret = gnutls_certificate_set_x509_trust_file(creds->data,
> @@ -626,7 +628,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
>      if (ret < 0) {
>          error_setg(errp, "Cannot load CA certificate '%s': %s",
>                     cacert, gnutls_strerror(ret));
> -        goto cleanup;
> +        return -1;
>      }
>
>      if (cert != NULL && key != NULL) {
> @@ -635,7 +637,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
>              password = qcrypto_secret_lookup_as_utf8(creds->passwordid,
>                                                       errp);
>              if (!password) {
> -                goto cleanup;
> +                return -1;
>              }
>          }
>          ret = gnutls_certificate_set_x509_key_file2(creds->data,
> @@ -647,7 +649,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
>          if (ret < 0) {
>              error_setg(errp, "Cannot load certificate '%s' & key '%s':
> %s",
>                         cert, key, gnutls_strerror(ret));
> -            goto cleanup;
> +            return -1;
>          }
>      }
>
> @@ -658,7 +660,7 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds,
>          if (ret < 0) {
>              error_setg(errp, "Cannot load CRL '%s': %s",
>                         cacrl, gnutls_strerror(ret));
> -            goto cleanup;
> +            return -1;
>          }
>      }
>
> @@ -666,20 +668,13 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509
> *creds,
>          if (qcrypto_tls_creds_get_dh_params_file(&creds->parent_obj,
> dhparams,
>
> &creds->parent_obj.dh_params,
>                                                   errp) < 0) {
> -            goto cleanup;
> +            return -1;
>          }
>          gnutls_certificate_set_dh_params(creds->data,
>                                           creds->parent_obj.dh_params);
>      }
>
> -    rv = 0;
> - cleanup:
> -    g_free(cacert);
> -    g_free(cacrl);
> -    g_free(cert);
> -    g_free(key);
> -    g_free(dhparams);
> -    return rv;
> +    return 0;
>  }
>
>
> --
> 2.51.1
>
>

[-- Attachment #2: Type: text/html, Size: 6678 bytes --]

Re: [PATCH 01/21] crypto: remove redundant parameter checking CA certs

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2473 bytes --]

On Thu, Oct 30, 2025 at 6:48 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> The only caller of qcrypto_tls_creds_check_authority_chain always
> passes 'true' for the 'isCA' parameter. The point of this method
> is to check the CA chani, so no other value would ever make sense.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>

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


> ---
>  crypto/tlscredsx509.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index db2b74bafa..847fd4d9fa 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -315,7 +315,6 @@
> qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
>                                          unsigned int ncacerts,
>                                          const char *cacertFile,
>                                          bool isServer,
> -                                        bool isCA,
>                                          Error **errp)
>  {
>      gnutls_x509_crt_t cert_to_check = certs[ncerts - 1];
> @@ -356,7 +355,7 @@
> qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
>               */
>              return qcrypto_tls_creds_check_cert(
>                  creds, cert_to_check, cacertFile,
> -                isServer, isCA, errp);
> +                isServer, true, errp);
>          }
>          for (int i = 0; i < ncacerts; i++) {
>              if (gnutls_x509_crt_check_issuer(cert_to_check,
> @@ -370,7 +369,7 @@
> qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
>          }
>
>          if (qcrypto_tls_creds_check_cert(creds, cert_issuer, cacertFile,
> -                                         isServer, isCA, errp) < 0) {
> +                                         isServer, true, errp) < 0) {
>              return -1;
>          }
>
> @@ -534,7 +533,7 @@
> qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
>                                                  certs, ncerts,
>                                                  cacerts, ncacerts,
>                                                  cacertFile, isServer,
> -                                                true, errp) < 0) {
> +                                                errp) < 0) {
>          goto cleanup;
>      }
>
> --
> 2.51.1
>
>

[-- Attachment #2: Type: text/html, Size: 3597 bytes --]

Re: [PATCH 02/21] crypto: add missing free of certs array

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 794 bytes --]

Hi

On Thu, Oct 30, 2025 at 6:48 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>

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


> ---
>  crypto/tlscredsx509.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 847fd4d9fa..75c70af522 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -550,6 +550,7 @@
> qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
>      for (i = 0; i < ncerts; i++) {
>          gnutls_x509_crt_deinit(certs[i]);
>      }
> +    g_free(certs);
>      for (i = 0; i < ncacerts; i++) {
>          gnutls_x509_crt_deinit(cacerts[i]);
>      }
> --
> 2.51.1
>
>

[-- Attachment #2: Type: text/html, Size: 1483 bytes --]

Re: [PATCH 03/21] crypto: replace stat() with access() for credential checks

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1164 bytes --]

Hi

On Thu, Oct 30, 2025 at 6:48 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> Readability of the credential files is what matters for our usage,
> so access() is more appropriate than stat().
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>

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


> ---
>  crypto/tlscreds.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/crypto/tlscreds.c b/crypto/tlscreds.c
> index 9e59594d67..208a7e6d8f 100644
> --- a/crypto/tlscreds.c
> +++ b/crypto/tlscreds.c
> @@ -100,7 +100,6 @@ qcrypto_tls_creds_get_path(QCryptoTLSCreds *creds,
>                             char **cred,
>                             Error **errp)
>  {
> -    struct stat sb;
>      int ret = -1;
>
>      if (!creds->dir) {
> @@ -114,7 +113,7 @@ qcrypto_tls_creds_get_path(QCryptoTLSCreds *creds,
>
>      *cred = g_strdup_printf("%s/%s", creds->dir, filename);
>
> -    if (stat(*cred, &sb) < 0) {
> +    if (access(*cred, R_OK) < 0) {
>          if (errno == ENOENT && !required) {
>              ret = 0;
>          } else {
> --
> 2.51.1
>
>

[-- Attachment #2: Type: text/html, Size: 1949 bytes --]

Re: [PATCH 04/21] crypto: remove redundant access() checks before loading certs

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2296 bytes --]

Hi

On Thu, Oct 30, 2025 at 6:48 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> The qcrypto_tls_creds_get_path method will perform an access()
> check on the file and return a NULL path if it fails. By the
> time we get to loading the cert files we know they must exist
> on disk and thus the second access() check is redundant.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>

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


> ---
>  crypto/tlscredsx509.c | 22 ++++++++++------------
>  1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 75c70af522..0acb17b6ec 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -496,8 +496,7 @@
> qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
>      size_t i;
>      int ret = -1;
>
> -    if (certFile &&
> -        access(certFile, R_OK) == 0) {
> +    if (certFile) {
>          if (qcrypto_tls_creds_load_cert_list(creds,
>                                               certFile,
>                                               &certs,
> @@ -508,16 +507,15 @@
> qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
>              goto cleanup;
>          }
>      }
> -    if (access(cacertFile, R_OK) == 0) {
> -        if (qcrypto_tls_creds_load_cert_list(creds,
> -                                             cacertFile,
> -                                             &cacerts,
> -                                             &ncacerts,
> -                                             isServer,
> -                                             true,
> -                                             errp) < 0) {
> -            goto cleanup;
> -        }
> +
> +    if (qcrypto_tls_creds_load_cert_list(creds,
> +                                         cacertFile,
> +                                         &cacerts,
> +                                         &ncacerts,
> +                                         isServer,
> +                                         true,
> +                                         errp) < 0) {
> +        goto cleanup;
>      }
>
>      for (i = 0; i < ncerts; i++) {
> --
> 2.51.1
>
>

[-- Attachment #2: Type: text/html, Size: 3430 bytes --]

Re: [PATCH 05/21] crypto: move check for TLS creds 'dir' property

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 3590 bytes --]

Hi

On Thu, Oct 30, 2025 at 6:48 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:

> The check for the 'dir' property is being repeated for every
> credential file to be loaded, but this results in incorrect
> logic for optional credentials. The 'dir' property is mandatory
> for PSK and x509 creds, even if some individual files are
> optional. Address this by separating the check for the 'dir'
> property.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>

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


> ---
>  crypto/tlscreds.c     | 9 ---------
>  crypto/tlscredsanon.c | 3 ++-
>  crypto/tlscredspsk.c  | 5 +++++
>  crypto/tlscredsx509.c | 8 ++++++--
>  4 files changed, 13 insertions(+), 12 deletions(-)
>
> diff --git a/crypto/tlscreds.c b/crypto/tlscreds.c
> index 208a7e6d8f..65e97ddd11 100644
> --- a/crypto/tlscreds.c
> +++ b/crypto/tlscreds.c
> @@ -102,15 +102,6 @@ qcrypto_tls_creds_get_path(QCryptoTLSCreds *creds,
>  {
>      int ret = -1;
>
> -    if (!creds->dir) {
> -        if (required) {
> -            error_setg(errp, "Missing 'dir' property value");
> -            return -1;
> -        } else {
> -            return 0;
> -        }
> -    }
> -
>      *cred = g_strdup_printf("%s/%s", creds->dir, filename);
>
>      if (access(*cred, R_OK) < 0) {
> diff --git a/crypto/tlscredsanon.c b/crypto/tlscredsanon.c
> index 44af9e6c9a..bc3351b5d6 100644
> --- a/crypto/tlscredsanon.c
> +++ b/crypto/tlscredsanon.c
> @@ -43,7 +43,8 @@ qcrypto_tls_creds_anon_load(QCryptoTLSCredsAnon *creds,
>              creds->parent_obj.dir ? creds->parent_obj.dir : "<nodir>");
>
>      if (creds->parent_obj.endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
> -        if (qcrypto_tls_creds_get_path(&creds->parent_obj,
> +        if (creds->parent_obj.dir &&
> +            qcrypto_tls_creds_get_path(&creds->parent_obj,
>                                         QCRYPTO_TLS_CREDS_DH_PARAMS,
>                                         false, &dhparams, errp) < 0) {
>              return -1;
> diff --git a/crypto/tlscredspsk.c b/crypto/tlscredspsk.c
> index 5b68a6b7ba..545d3e45db 100644
> --- a/crypto/tlscredspsk.c
> +++ b/crypto/tlscredspsk.c
> @@ -81,6 +81,11 @@ qcrypto_tls_creds_psk_load(QCryptoTLSCredsPSK *creds,
>      trace_qcrypto_tls_creds_psk_load(creds,
>              creds->parent_obj.dir ? creds->parent_obj.dir : "<nodir>");
>
> +    if (!creds->parent_obj.dir) {
> +        error_setg(errp, "Missing 'dir' property value");
> +        goto cleanup;
> +    }
> +
>      if (creds->parent_obj.endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
>          if (creds->username) {
>              error_setg(errp, "username should not be set when
> endpoint=server");
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 0acb17b6ec..8fe6cc8e93 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -567,8 +567,12 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509
> *creds,
>      int ret;
>      int rv = -1;
>
> -    trace_qcrypto_tls_creds_x509_load(creds,
> -            creds->parent_obj.dir ? creds->parent_obj.dir : "<nodir>");
> +    if (!creds->parent_obj.dir) {
> +        error_setg(errp, "Missing 'dir' property value");
> +        return -1;
> +    }
> +
> +    trace_qcrypto_tls_creds_x509_load(creds, creds->parent_obj.dir);
>
>      if (creds->parent_obj.endpoint == QCRYPTO_TLS_CREDS_ENDPOINT_SERVER) {
>          if (qcrypto_tls_creds_get_path(&creds->parent_obj,
> --
> 2.51.1
>
>

[-- Attachment #2: Type: text/html, Size: 4825 bytes --]

Re: [PATCH 01/21] crypto: remove redundant parameter checking CA certs

[Philippe Mathieu-Daudé]

On 30/10/25 15:49, Daniel P. Berrangé wrote:
> The only caller of qcrypto_tls_creds_check_authority_chain always
> passes 'true' for the 'isCA' parameter. The point of this method
> is to check the CA chani, so no other value would ever make sense.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
>   crypto/tlscredsx509.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 06/21] crypto: use g_autofree when loading x509 credentials

[Daniel P. Berrangé]

On Thu, Oct 30, 2025 at 11:23:44PM +0400, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Oct 30, 2025 at 6:48 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > This allows removal of goto jumps during loading of the credentials
> > and will simplify the diff in following commits.
> >
> >
> If you want, you could also g_autofree password.

Yes, I'll add that.

> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> >
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@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 :|