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

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

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

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

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

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