On Thu, Oct 30, 2025 at 6:50 PM Daniel P. Berrangé <[email protected]> wrote:
> The x509 TLS credentials code will load the identity certs once to > perform sanity chcking on the certs, then discard the certificate > objects and let gnutls load them a second time. > > This extends the previous QCryptoTLSCredsX509Files struct to also > hold the identity certificates & key loaded for sanity checking > and pass them on to gnutls, avoiding the duplicated loading. > > The unit tests need updating because we now correctly diagnose the > error scenario where the cert PEM file exists, without its matching > key PEM file. Previously that error was mistakenly ignored. > > Signed-off-by: Daniel P. Berrangé <[email protected]> > lgtm Reviewed-by: Marc-André Lureau <[email protected]> > --- > crypto/tlscredsx509.c | 247 +++++++++++++++++--------- > tests/unit/test-crypto-tlscredsx509.c | 8 +- > 2 files changed, 164 insertions(+), 91 deletions(-) > > diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c > index 6a830af50d..3cb0a6c31f 100644 > --- a/crypto/tlscredsx509.c > +++ b/crypto/tlscredsx509.c > @@ -45,6 +45,12 @@ struct QCryptoTLSCredsX509Files { > char *cacertpath; > gnutls_x509_crt_t *cacerts; > unsigned int ncacerts; > + > + char *certpath; > + char *keypath; > + gnutls_x509_crt_t *certs; > + unsigned int ncerts; > + gnutls_x509_privkey_t key; > }; > > static QCryptoTLSCredsX509Files * > @@ -63,6 +69,13 @@ > qcrypto_tls_creds_x509_files_free(QCryptoTLSCredsX509Files *files) > } > g_free(files->cacerts); > g_free(files->cacertpath); > + for (i = 0; i < files->ncerts; i++) { > + gnutls_x509_crt_deinit(files->certs[i]); > + } > + gnutls_x509_privkey_deinit(files->key); > + g_free(files->certs); > + g_free(files->certpath); > + g_free(files->keypath); > g_free(files); > } > > @@ -477,14 +490,13 @@ 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; > gsize buflen; > GError *gerr = NULL; > + int ret; > > *ncerts = 0; > trace_qcrypto_tls_creds_x509_load_cert_list(creds, certFile); > @@ -499,13 +511,60 @@ qcrypto_tls_creds_load_cert_list(QCryptoTLSCredsX509 > *creds, > data.data = (unsigned char *)buf; > data.size = strlen(buf); > > - if (gnutls_x509_crt_list_import2(certs, ncerts, &data, > - GNUTLS_X509_FMT_PEM, 0) < 0) { > - error_setg(errp, > - isCA ? "Unable to import CA certificate list %s" : > - (isServer ? "Unable to import server certificate %s" : > - "Unable to import client certificate %s"), > - certFile); > + ret = gnutls_x509_crt_list_import2(certs, ncerts, &data, > + GNUTLS_X509_FMT_PEM, 0); > + if (ret < 0) { > + error_setg(errp, "Unable to import certificate %s: %s", > + certFile, gnutls_strerror(ret)); > + return -1; > + } > + > + return 0; > +} > + > + > +static int > +qcrypto_tls_creds_load_privkey(QCryptoTLSCredsX509 *creds, > + const char *keyFile, > + gnutls_x509_privkey_t *key, > + Error **errp) > +{ > + gnutls_datum_t data; > + g_autofree char *buf = NULL; > + g_autofree char *password = NULL; > + gsize buflen; > + GError *gerr = NULL; > + int ret; > + > + ret = gnutls_x509_privkey_init(key); > + if (ret < 0) { > + error_setg(errp, "Unable to initialize private key: %s", > + gnutls_strerror(ret)); > + return -1; > + } > + > + if (!g_file_get_contents(keyFile, &buf, &buflen, &gerr)) { > + error_setg(errp, "Cannot load private key %s: %s", > + keyFile, gerr->message); > + g_error_free(gerr); > + return -1; > + } > + > + data.data = (unsigned char *)buf; > + data.size = strlen(buf); > + > + if (creds->passwordid) { > + password = qcrypto_secret_lookup_as_utf8(creds->passwordid, > + errp); > + if (!password) { > + return -1; > + } > + } > + > + if (gnutls_x509_privkey_import2(*key, &data, > + GNUTLS_X509_FMT_PEM, > + password, 0) < 0) { > + error_setg(errp, "Unable to import private key %s", keyFile); > return -1; > } > > @@ -517,56 +576,34 @@ static int > qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds, > QCryptoTLSCredsX509Files *files, > bool isServer, > - const char *certFile, > Error **errp) > { > - gnutls_x509_crt_t *certs = NULL; > - unsigned int ncerts = 0; > size_t i; > - int ret = -1; > - > - if (certFile) { > - if (qcrypto_tls_creds_load_cert_list(creds, > - certFile, > - &certs, > - &ncerts, > - isServer, > - false, > - errp) < 0) { > - goto cleanup; > - } > - } > > - for (i = 0; i < ncerts; i++) { > + for (i = 0; i < files->ncerts; i++) { > if (qcrypto_tls_creds_check_cert(creds, > - certs[i], certFile, > + files->certs[i], files->certpath, > isServer, i != 0, errp) < 0) { > - goto cleanup; > + return -1; > } > } > > - if (ncerts && > + if (files->ncerts && > qcrypto_tls_creds_check_authority_chain(creds, files, > - certs, ncerts, > + files->certs, > files->ncerts, > isServer, errp) < 0) { > - goto cleanup; > - } > - > - if (ncerts && > - qcrypto_tls_creds_check_cert_pair(files, certs, ncerts, certFile, > - isServer, errp) < 0) { > - goto cleanup; > + return -1; > } > > - ret = 0; > - > - cleanup: > - for (i = 0; i < ncerts; i++) { > - gnutls_x509_crt_deinit(certs[i]); > + if (files->ncerts && > + qcrypto_tls_creds_check_cert_pair(files, > + files->certs, files->ncerts, > + files->certpath, isServer, > + errp) < 0) { > + return -1; > } > - g_free(certs); > > - return ret; > + return 0; > } > > > @@ -589,8 +626,6 @@ qcrypto_tls_creds_x509_load_ca(QCryptoTLSCredsX509 > *creds, > files->cacertpath, > &files->cacerts, > &files->ncacerts, > - isServer, > - true, > errp) < 0) { > return -1; > } > @@ -606,6 +641,79 @@ qcrypto_tls_creds_x509_load_ca(QCryptoTLSCredsX509 > *creds, > return 0; > } > > + > +static int > +qcrypto_tls_creds_x509_load_identity(QCryptoTLSCredsX509 *creds, > + QCryptoTLSCredsBox *box, > + QCryptoTLSCredsX509Files *files, > + bool isServer, > + Error **errp) > +{ > + int ret; > + > + if (isServer) { > + if (qcrypto_tls_creds_get_path(&creds->parent_obj, > + QCRYPTO_TLS_CREDS_X509_SERVER_CERT, > + true, &files->certpath, errp) < 0 > || > + qcrypto_tls_creds_get_path(&creds->parent_obj, > + QCRYPTO_TLS_CREDS_X509_SERVER_KEY, > + true, &files->keypath, errp) < 0) { > + return -1; > + } > + } else { > + if (qcrypto_tls_creds_get_path(&creds->parent_obj, > + QCRYPTO_TLS_CREDS_X509_CLIENT_CERT, > + false, &files->certpath, errp) < 0 > || > + qcrypto_tls_creds_get_path(&creds->parent_obj, > + QCRYPTO_TLS_CREDS_X509_CLIENT_KEY, > + false, &files->keypath, errp) < 0) > { > + return -1; > + } > + } > + > + if (!files->certpath && > + !files->keypath) { > + return 0; > + } > + if (files->certpath && !files->keypath) { > + error_setg(errp, "Cert '%s' without corresponding key", > + files->certpath); > + return -1; > + } > + if (!files->certpath && files->keypath) { > + error_setg(errp, "Key '%s' without corresponding cert", > + files->keypath); > + return -1; > + } > + > + if (qcrypto_tls_creds_load_cert_list(creds, > + files->certpath, > + &files->certs, > + &files->ncerts, > + errp) < 0) { > + return -1; > + } > + > + if (qcrypto_tls_creds_load_privkey(creds, > + files->keypath, > + &files->key, > + errp) < 0) { > + return -1; > + } > + > + ret = gnutls_certificate_set_x509_key(box->data.cert, > + files->certs, > + files->ncerts, > + files->key); > + if (ret < 0) { > + error_setg(errp, "Cannot set certificate '%s' & key '%s': %s", > + files->certpath, files->keypath, gnutls_strerror(ret)); > + return -1; > + } > + return 0; > +} > + > + > static int > qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds, > Error **errp) > @@ -613,8 +721,6 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 *creds, > g_autoptr(QCryptoTLSCredsBox) box = NULL; > g_autoptr(QCryptoTLSCredsX509Files) files = NULL; > g_autofree char *cacrl = NULL; > - g_autofree char *cert = NULL; > - g_autofree char *key = NULL; > g_autofree char *dhparams = NULL; > bool isServer = (creds->parent_obj.endpoint == > QCRYPTO_TLS_CREDS_ENDPOINT_SERVER); > @@ -646,60 +752,27 @@ qcrypto_tls_creds_x509_load(QCryptoTLSCredsX509 > *creds, > return -1; > } > > + if (qcrypto_tls_creds_x509_load_identity(creds, box, files, > + isServer, errp) < 0) { > + return -1; > + } > + > if (isServer) { > if (qcrypto_tls_creds_get_path(&creds->parent_obj, > QCRYPTO_TLS_CREDS_X509_CA_CRL, > false, &cacrl, errp) < 0 || > - qcrypto_tls_creds_get_path(&creds->parent_obj, > - QCRYPTO_TLS_CREDS_X509_SERVER_CERT, > - true, &cert, errp) < 0 || > - qcrypto_tls_creds_get_path(&creds->parent_obj, > - QCRYPTO_TLS_CREDS_X509_SERVER_KEY, > - true, &key, errp) < 0 || > qcrypto_tls_creds_get_path(&creds->parent_obj, > QCRYPTO_TLS_CREDS_DH_PARAMS, > false, &dhparams, errp) < 0) { > return -1; > } > - } else { > - if (qcrypto_tls_creds_get_path(&creds->parent_obj, > - QCRYPTO_TLS_CREDS_X509_CLIENT_CERT, > - false, &cert, errp) < 0 || > - qcrypto_tls_creds_get_path(&creds->parent_obj, > - QCRYPTO_TLS_CREDS_X509_CLIENT_KEY, > - false, &key, errp) < 0) { > - return -1; > - } > } > > if (creds->sanityCheck && > - qcrypto_tls_creds_x509_sanity_check(creds, files, isServer, > - cert, errp) < 0) { > + qcrypto_tls_creds_x509_sanity_check(creds, files, isServer, errp) > < 0) { > return -1; > } > > - if (cert != NULL && key != NULL) { > - char *password = NULL; > - if (creds->passwordid) { > - password = qcrypto_secret_lookup_as_utf8(creds->passwordid, > - errp); > - if (!password) { > - return -1; > - } > - } > - ret = gnutls_certificate_set_x509_key_file2(box->data.cert, > - cert, key, > - GNUTLS_X509_FMT_PEM, > - password, > - 0); > - g_free(password); > - if (ret < 0) { > - error_setg(errp, "Cannot load certificate '%s' & key '%s': > %s", > - cert, key, gnutls_strerror(ret)); > - return -1; > - } > - } > - > if (cacrl != NULL) { > ret = gnutls_certificate_set_x509_crl_file(box->data.cert, > cacrl, > diff --git a/tests/unit/test-crypto-tlscredsx509.c > b/tests/unit/test-crypto-tlscredsx509.c > index a5f21728d4..b1ad7d5c0d 100644 > --- a/tests/unit/test-crypto-tlscredsx509.c > +++ b/tests/unit/test-crypto-tlscredsx509.c > @@ -95,16 +95,16 @@ static void test_tls_creds(const void *opaque) > if (access(data->crt, R_OK) == 0) { > g_assert(link(data->crt, > CERT_DIR QCRYPTO_TLS_CREDS_X509_SERVER_CERT) == > 0); > + g_assert(link(KEYFILE, > + CERT_DIR QCRYPTO_TLS_CREDS_X509_SERVER_KEY) == > 0); > } > - g_assert(link(KEYFILE, > - CERT_DIR QCRYPTO_TLS_CREDS_X509_SERVER_KEY) == 0); > } else { > if (access(data->crt, R_OK) == 0) { > g_assert(link(data->crt, > CERT_DIR QCRYPTO_TLS_CREDS_X509_CLIENT_CERT) == > 0); > + g_assert(link(KEYFILE, > + CERT_DIR QCRYPTO_TLS_CREDS_X509_CLIENT_KEY) == > 0); > } > - g_assert(link(KEYFILE, > - CERT_DIR QCRYPTO_TLS_CREDS_X509_CLIENT_KEY) == 0); > } > > creds = test_tls_creds_create( > -- > 2.51.1 > >
