* [PATCH v2 0/6] crypto: misc fixes and improvements to cert handling
@ 2025-09-19 10:10 Daniel P. Berrangé
2025-09-19 10:10 ` [PATCH v2 1/6] crypto: only verify CA certs in chain of trust Daniel P. Berrangé
` (5 more replies)
0 siblings, 6 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2025-09-19 10:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P. Berrangé
This series includes three patches that were posted a fairly
long time ago. They are borderline between a feature request
and a bug fix, but I'm classing them more bug fix, since they
addressing issues with cert acceptance that we really should
not have had.
The patches by Henry had outstanding comments from myself,
and I've chosen to simply fix them in two followup commits
of my own now to get this over the line.
The patch from "matoro" was not accepted because they were
contributed under a github alias. With our change to have
a more relaxed interpretation of the DCO allowing any
"known identity", we can now accept this patch. It had
some conflicts with Henry's patch which I've fixed up.
Then there is one other small bug fix and one improvement
to use a newer gnutls API.
Changed in v2:
- Dropped already merged patch & re-arrange remaining
series
Daniel P. Berrangé (4):
crypto: remove extraneous pointer usage in gnutls certs
crypto: stop requiring "key encipherment" usage in x509 certs
crypto: switch to newer gnutls API for distinguished name
crypto: fix error reporting in cert chain checks
Henry Kleynhans (1):
crypto: only verify CA certs in chain of trust
matoro (1):
crypto: allow client/server cert chains
crypto/tlscredsx509.c | 223 +++++++++++++++-----------
crypto/tlssession.c | 12 +-
docs/system/tls.rst | 13 +-
tests/unit/crypto-tls-x509-helpers.h | 6 +-
tests/unit/test-crypto-tlscredsx509.c | 138 +++++++++++++---
tests/unit/test-crypto-tlssession.c | 14 +-
tests/unit/test-io-channel-tls.c | 4 +-
7 files changed, 264 insertions(+), 146 deletions(-)
--
2.50.1
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/6] crypto: only verify CA certs in chain of trust
2025-09-19 10:10 [PATCH v2 0/6] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé
@ 2025-09-19 10:10 ` Daniel P. Berrangé
2025-10-16 14:37 ` Eric Blake
2025-09-19 10:10 ` [PATCH v2 2/6] crypto: remove extraneous pointer usage in gnutls certs Daniel P. Berrangé
` (4 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2025-09-19 10:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P. Berrangé, Henry Kleynhans
From: Henry Kleynhans <hkleynhans@fb.com>
The CA file provided to qemu may contain CA certificates which do not
form part of the chain of trust for the specific certificate we are
sanity checking.
This patch changes the sanity checking from validating every CA
certificate to only checking the CA certificates which are part of the
chain of trust (issuer chain). Other certificates are ignored.
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
crypto/tlscredsx509.c | 57 ++++++++++++++++++++++++---
tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++-
2 files changed, 75 insertions(+), 7 deletions(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index cd1f504471..797854ac89 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -315,6 +315,51 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *creds,
return 0;
}
+static int
+qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
+ gnutls_x509_crt_t cert,
+ gnutls_x509_crt_t *cacerts,
+ unsigned int ncacerts,
+ const char *cacertFile,
+ bool isServer,
+ bool isCA,
+ Error **errp)
+{
+ gnutls_x509_crt_t *cert_to_check = &cert;
+ int checking_issuer = 1;
+ int retval = 0;
+
+ while (checking_issuer) {
+ checking_issuer = 0;
+
+ if (gnutls_x509_crt_check_issuer(*cert_to_check,
+ *cert_to_check)) {
+ /*
+ * The cert is self-signed indicating we have
+ * reached the root of trust.
+ */
+ return qcrypto_tls_creds_check_cert(
+ creds, *cert_to_check, cacertFile,
+ isServer, isCA, errp);
+ }
+ 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;
+ break;
+ }
+ }
+ }
+
+ return -1;
+}
static int
qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
@@ -499,12 +544,12 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
goto cleanup;
}
- for (i = 0; i < ncacerts; i++) {
- if (qcrypto_tls_creds_check_cert(creds,
- cacerts[i], cacertFile,
- isServer, true, errp) < 0) {
- goto cleanup;
- }
+ if (cert &&
+ qcrypto_tls_creds_check_authority_chain(creds, cert,
+ cacerts, ncacerts,
+ cacertFile, isServer,
+ true, errp) < 0) {
+ goto cleanup;
}
if (cert && ncacerts &&
diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c
index 3c25d75ca1..a7ea5f422d 100644
--- a/tests/unit/test-crypto-tlscredsx509.c
+++ b/tests/unit/test-crypto-tlscredsx509.c
@@ -589,6 +589,12 @@ int main(int argc, char **argv)
true, true, GNUTLS_KEY_KEY_CERT_SIGN,
false, false, NULL, NULL,
0, 0);
+ TLS_CERT_REQ(cacertlevel1creq_invalid, cacertrootreq,
+ "UK", "qemu level 1c invalid", NULL, NULL, NULL, NULL,
+ true, true, true,
+ true, true, GNUTLS_KEY_KEY_CERT_SIGN,
+ false, false, NULL, NULL,
+ 360, 400);
TLS_CERT_REQ(cacertlevel2areq, cacertlevel1areq,
"UK", "qemu level 2a", NULL, NULL, NULL, NULL,
true, true, true,
@@ -617,16 +623,32 @@ int main(int argc, char **argv)
cacertlevel2areq.crt,
};
+
test_tls_write_cert_chain(WORKDIR "cacertchain-ctx.pem",
certchain,
G_N_ELEMENTS(certchain));
+ gnutls_x509_crt_t certchain_with_invalid[] = {
+ cacertrootreq.crt,
+ cacertlevel1areq.crt,
+ cacertlevel1breq.crt,
+ cacertlevel1creq_invalid.crt,
+ cacertlevel2areq.crt,
+ };
+
+ test_tls_write_cert_chain(WORKDIR "cacertchain-with-invalid-ctx.pem",
+ certchain_with_invalid,
+ G_N_ELEMENTS(certchain_with_invalid));
+
TLS_TEST_REG(chain1, true,
WORKDIR "cacertchain-ctx.pem",
servercertlevel3areq.filename, false);
TLS_TEST_REG(chain2, false,
WORKDIR "cacertchain-ctx.pem",
clientcertlevel2breq.filename, false);
+ TLS_TEST_REG(certchainwithexpiredcert, false,
+ WORKDIR "cacertchain-with-invalid-ctx.pem",
+ clientcertlevel2breq.filename, false);
/* Some missing certs - first two are fatal, the last
* is ok
@@ -640,7 +662,6 @@ int main(int argc, char **argv)
TLS_TEST_REG(missingclient, false,
cacert1req.filename,
"clientcertdoesnotexist.pem", false);
-
ret = g_test_run();
test_tls_discard_cert(&cacertreq);
@@ -694,10 +715,12 @@ int main(int argc, char **argv)
test_tls_discard_cert(&cacertrootreq);
test_tls_discard_cert(&cacertlevel1areq);
test_tls_discard_cert(&cacertlevel1breq);
+ test_tls_discard_cert(&cacertlevel1creq_invalid);
test_tls_discard_cert(&cacertlevel2areq);
test_tls_discard_cert(&servercertlevel3areq);
test_tls_discard_cert(&clientcertlevel2breq);
unlink(WORKDIR "cacertchain-ctx.pem");
+ unlink(WORKDIR "cacertchain-with-invalid-ctx.pem");
test_tls_cleanup(KEYFILE);
rmdir(WORKDIR);
--
2.50.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/6] crypto: remove extraneous pointer usage in gnutls certs
2025-09-19 10:10 [PATCH v2 0/6] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé
2025-09-19 10:10 ` [PATCH v2 1/6] crypto: only verify CA certs in chain of trust Daniel P. Berrangé
@ 2025-09-19 10:10 ` Daniel P. Berrangé
2025-10-16 14:52 ` Eric Blake
2025-09-19 10:10 ` [PATCH v2 3/6] crypto: allow client/server cert chains Daniel P. Berrangé
` (3 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2025-09-19 10:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P. Berrangé, Philippe Mathieu-Daudé
The 'gnutls_x509_crt_t' type is already a pointer, not a struct,
so the extra level of pointer indirection is not needed.
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
crypto/tlscredsx509.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 797854ac89..91d8dde633 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -325,25 +325,25 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
bool isCA,
Error **errp)
{
- gnutls_x509_crt_t *cert_to_check = &cert;
+ gnutls_x509_crt_t cert_to_check = cert;
int checking_issuer = 1;
int retval = 0;
while (checking_issuer) {
checking_issuer = 0;
- if (gnutls_x509_crt_check_issuer(*cert_to_check,
- *cert_to_check)) {
+ if (gnutls_x509_crt_check_issuer(cert_to_check,
+ cert_to_check)) {
/*
* The cert is self-signed indicating we have
* reached the root of trust.
*/
return qcrypto_tls_creds_check_cert(
- creds, *cert_to_check, cacertFile,
+ creds, cert_to_check, cacertFile,
isServer, isCA, errp);
}
for (int i = 0; i < ncacerts; i++) {
- if (gnutls_x509_crt_check_issuer(*cert_to_check,
+ if (gnutls_x509_crt_check_issuer(cert_to_check,
cacerts[i])) {
retval = qcrypto_tls_creds_check_cert(
creds, cacerts[i], cacertFile,
@@ -351,7 +351,7 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
if (retval < 0) {
return retval;
}
- cert_to_check = &cacerts[i];
+ cert_to_check = cacerts[i];
checking_issuer = 1;
break;
}
--
2.50.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/6] crypto: allow client/server cert chains
2025-09-19 10:10 [PATCH v2 0/6] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé
2025-09-19 10:10 ` [PATCH v2 1/6] crypto: only verify CA certs in chain of trust Daniel P. Berrangé
2025-09-19 10:10 ` [PATCH v2 2/6] crypto: remove extraneous pointer usage in gnutls certs Daniel P. Berrangé
@ 2025-09-19 10:10 ` Daniel P. Berrangé
2025-10-16 15:28 ` Eric Blake
2025-09-19 10:10 ` [PATCH v2 4/6] crypto: stop requiring "key encipherment" usage in x509 certs Daniel P. Berrangé
` (2 subsequent siblings)
5 siblings, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2025-09-19 10:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P. Berrangé, matoro
From: matoro <matoro@users.noreply.github.com>
The existing implementation assumes that client/server certificates are
single individual certificates. If using publicly-issued certificates,
or internal CAs that use an intermediate issuer, this is unlikely to be
the case, and they will instead be certificate chains. While this can
be worked around by moving the intermediate certificates to the CA
certificate, which DOES currently support multiple certificates, this
instead allows the issued certificate chains to be used as-is, without
requiring the overhead of shuffling certificates around.
Corresponding libvirt change is available here:
https://gitlab.com/libvirt/libvirt/-/merge_requests/222
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: matoro <matoro_mailinglist_qemu@matoro.tk>
[DB: adapted for code conflicts with multi-CA patch]
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
crypto/tlscredsx509.c | 156 ++++++++++++--------------
tests/unit/test-crypto-tlscredsx509.c | 77 +++++++++++++
2 files changed, 147 insertions(+), 86 deletions(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 91d8dde633..311de3237d 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -317,7 +317,8 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *creds,
static int
qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
- gnutls_x509_crt_t cert,
+ gnutls_x509_crt_t *certs,
+ unsigned int ncerts,
gnutls_x509_crt_t *cacerts,
unsigned int ncacerts,
const char *cacertFile,
@@ -325,9 +326,33 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
bool isCA,
Error **errp)
{
- gnutls_x509_crt_t cert_to_check = cert;
+ gnutls_x509_crt_t cert_to_check = certs[ncerts - 1];
int checking_issuer = 1;
int retval = 0;
+ gnutls_datum_t dn = {}, dnissuer = {};
+
+ for (int i = 0; i < (ncerts - 1); i++) {
+ if (!gnutls_x509_crt_check_issuer(certs[i], certs[i + 1])) {
+ retval = gnutls_x509_crt_get_dn2(certs[i], &dn);
+ if (retval < 0) {
+ error_setg(errp, "Unable to fetch cert DN: %s",
+ gnutls_strerror(retval));
+ return -1;
+ }
+ retval = gnutls_x509_crt_get_dn2(certs[i + 1], &dnissuer);
+ if (retval < 0) {
+ gnutls_free(dn.data);
+ error_setg(errp, "Unable to fetch cert DN: %s",
+ gnutls_strerror(retval));
+ return -1;
+ }
+ error_setg(errp, "Cert '%s' does not match issuer of cert '%s'",
+ dnissuer.data, dn.data);
+ gnutls_free(dn.data);
+ gnutls_free(dnissuer.data);
+ return -1;
+ }
+ }
while (checking_issuer) {
checking_issuer = 0;
@@ -362,7 +387,8 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
}
static int
-qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
+qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t *certs,
+ size_t ncerts,
const char *certFile,
gnutls_x509_crt_t *cacerts,
size_t ncacerts,
@@ -372,7 +398,7 @@ qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
{
unsigned int status;
- if (gnutls_x509_crt_list_verify(&cert, 1,
+ if (gnutls_x509_crt_list_verify(certs, ncerts,
cacerts, ncacerts,
NULL, 0,
0, &status) < 0) {
@@ -414,66 +440,14 @@ qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
}
-static gnutls_x509_crt_t
-qcrypto_tls_creds_load_cert(QCryptoTLSCredsX509 *creds,
- const char *certFile,
- bool isServer,
- Error **errp)
-{
- gnutls_datum_t data;
- gnutls_x509_crt_t cert = NULL;
- g_autofree char *buf = NULL;
- gsize buflen;
- GError *gerr = NULL;
- int ret = -1;
- int err;
-
- trace_qcrypto_tls_creds_x509_load_cert(creds, isServer, certFile);
-
- err = gnutls_x509_crt_init(&cert);
- if (err < 0) {
- error_setg(errp, "Unable to initialize certificate: %s",
- gnutls_strerror(err));
- goto cleanup;
- }
-
- if (!g_file_get_contents(certFile, &buf, &buflen, &gerr)) {
- error_setg(errp, "Cannot load CA cert list %s: %s",
- certFile, gerr->message);
- g_error_free(gerr);
- goto cleanup;
- }
-
- data.data = (unsigned char *)buf;
- data.size = strlen(buf);
-
- err = gnutls_x509_crt_import(cert, &data, GNUTLS_X509_FMT_PEM);
- if (err < 0) {
- error_setg(errp, isServer ?
- "Unable to import server certificate %s: %s" :
- "Unable to import client certificate %s: %s",
- certFile,
- gnutls_strerror(err));
- goto cleanup;
- }
-
- ret = 0;
-
- cleanup:
- if (ret != 0) {
- gnutls_x509_crt_deinit(cert);
- cert = NULL;
- }
- return cert;
-}
-
-
static int
-qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds,
- const char *certFile,
- gnutls_x509_crt_t **certs,
- unsigned int *ncerts,
- Error **errp)
+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;
@@ -496,7 +470,9 @@ qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds,
if (gnutls_x509_crt_list_import2(certs, ncerts, &data,
GNUTLS_X509_FMT_PEM, 0) < 0) {
error_setg(errp,
- "Unable to import CA certificate list %s",
+ isCA ? "Unable to import CA certificate list %s" :
+ (isServer ? "Unable to import server certificate %s" :
+ "Unable to import client certificate %s"),
certFile);
return -1;
}
@@ -512,7 +488,8 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
const char *certFile,
Error **errp)
{
- gnutls_x509_crt_t cert = NULL;
+ gnutls_x509_crt_t *certs = NULL;
+ unsigned int ncerts = 0;
gnutls_x509_crt_t *cacerts = NULL;
unsigned int ncacerts = 0;
size_t i;
@@ -520,41 +497,48 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
if (certFile &&
access(certFile, R_OK) == 0) {
- cert = qcrypto_tls_creds_load_cert(creds,
- certFile, isServer,
- errp);
- if (!cert) {
+ if (qcrypto_tls_creds_load_cert_list(creds,
+ certFile,
+ &certs,
+ &ncerts,
+ isServer,
+ false,
+ errp) < 0) {
goto cleanup;
}
}
if (access(cacertFile, R_OK) == 0) {
- if (qcrypto_tls_creds_load_ca_cert_list(creds,
- cacertFile,
- &cacerts,
- &ncacerts,
- errp) < 0) {
+ if (qcrypto_tls_creds_load_cert_list(creds,
+ cacertFile,
+ &cacerts,
+ &ncacerts,
+ isServer,
+ true,
+ errp) < 0) {
goto cleanup;
}
}
- if (cert &&
- qcrypto_tls_creds_check_cert(creds,
- cert, certFile, isServer,
- false, errp) < 0) {
- goto cleanup;
+ for (i = 0; i < ncerts; i++) {
+ if (qcrypto_tls_creds_check_cert(creds,
+ certs[i], certFile,
+ isServer, (i != 0), errp) < 0) {
+ goto cleanup;
+ }
}
- if (cert &&
- qcrypto_tls_creds_check_authority_chain(creds, cert,
+ if (ncerts &&
+ qcrypto_tls_creds_check_authority_chain(creds,
+ certs, ncerts,
cacerts, ncacerts,
cacertFile, isServer,
true, errp) < 0) {
goto cleanup;
}
- if (cert && ncacerts &&
- qcrypto_tls_creds_check_cert_pair(cert, certFile, cacerts,
- ncacerts, cacertFile,
+ if (ncerts && ncacerts &&
+ qcrypto_tls_creds_check_cert_pair(certs, ncerts, certFile,
+ cacerts, ncacerts, cacertFile,
isServer, errp) < 0) {
goto cleanup;
}
@@ -562,8 +546,8 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
ret = 0;
cleanup:
- if (cert) {
- gnutls_x509_crt_deinit(cert);
+ for (i = 0; i < ncerts; i++) {
+ gnutls_x509_crt_deinit(certs[i]);
}
for (i = 0; i < ncacerts; i++) {
gnutls_x509_crt_deinit(cacerts[i]);
diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c
index a7ea5f422d..4a32bc4d69 100644
--- a/tests/unit/test-crypto-tlscredsx509.c
+++ b/tests/unit/test-crypto-tlscredsx509.c
@@ -577,6 +577,12 @@ int main(int argc, char **argv)
true, true, GNUTLS_KEY_KEY_CERT_SIGN,
false, false, NULL, NULL,
0, 0);
+ TLS_ROOT_REQ(someotherrootreq,
+ "UK", "some other random CA", NULL, NULL, NULL, NULL,
+ true, true, true,
+ true, true, GNUTLS_KEY_KEY_CERT_SIGN,
+ false, false, NULL, NULL,
+ 0, 0);
TLS_CERT_REQ(cacertlevel1areq, cacertrootreq,
"UK", "qemu level 1a", NULL, NULL, NULL, NULL,
true, true, true,
@@ -623,6 +629,32 @@ int main(int argc, char **argv)
cacertlevel2areq.crt,
};
+ gnutls_x509_crt_t cabundle[] = {
+ someotherrootreq.crt,
+ cacertrootreq.crt,
+ };
+
+ gnutls_x509_crt_t servercertchain[] = {
+ servercertlevel3areq.crt,
+ cacertlevel2areq.crt,
+ cacertlevel1areq.crt,
+ };
+
+ gnutls_x509_crt_t servercertchain_incomplete[] = {
+ servercertlevel3areq.crt,
+ cacertlevel2areq.crt,
+ };
+
+ gnutls_x509_crt_t servercertchain_unsorted[] = {
+ servercertlevel3areq.crt,
+ cacertlevel1areq.crt,
+ cacertlevel2areq.crt,
+ };
+
+ gnutls_x509_crt_t clientcertchain[] = {
+ clientcertlevel2breq.crt,
+ cacertlevel1breq.crt,
+ };
test_tls_write_cert_chain(WORKDIR "cacertchain-ctx.pem",
certchain,
@@ -650,6 +682,46 @@ int main(int argc, char **argv)
WORKDIR "cacertchain-with-invalid-ctx.pem",
clientcertlevel2breq.filename, false);
+ test_tls_write_cert_chain(WORKDIR "servercertchain-ctx.pem",
+ servercertchain,
+ G_N_ELEMENTS(servercertchain));
+
+ TLS_TEST_REG(serverchain, true,
+ cacertrootreq.filename,
+ WORKDIR "servercertchain-ctx.pem", false);
+
+ test_tls_write_cert_chain(WORKDIR "cabundle-ctx.pem",
+ cabundle,
+ G_N_ELEMENTS(cabundle));
+
+ TLS_TEST_REG(multiplecaswithchain, true,
+ WORKDIR "cabundle-ctx.pem",
+ WORKDIR "servercertchain-ctx.pem", false);
+
+ test_tls_write_cert_chain(WORKDIR "servercertchain_incomplete-ctx.pem",
+ servercertchain_incomplete,
+ G_N_ELEMENTS(servercertchain_incomplete));
+
+ TLS_TEST_REG(incompleteserverchain, true,
+ cacertrootreq.filename,
+ WORKDIR "servercertchain_incomplete-ctx.pem", true);
+
+ test_tls_write_cert_chain(WORKDIR "servercertchain_unsorted-ctx.pem",
+ servercertchain_unsorted,
+ G_N_ELEMENTS(servercertchain_unsorted));
+
+ TLS_TEST_REG(unsortedserverchain, true,
+ cacertrootreq.filename,
+ WORKDIR "servercertchain_unsorted-ctx.pem", true);
+
+ test_tls_write_cert_chain(WORKDIR "clientcertchain-ctx.pem",
+ clientcertchain,
+ G_N_ELEMENTS(clientcertchain));
+
+ TLS_TEST_REG(clientchain, false,
+ cacertrootreq.filename,
+ WORKDIR "clientcertchain-ctx.pem", false);
+
/* Some missing certs - first two are fatal, the last
* is ok
*/
@@ -719,8 +791,13 @@ int main(int argc, char **argv)
test_tls_discard_cert(&cacertlevel2areq);
test_tls_discard_cert(&servercertlevel3areq);
test_tls_discard_cert(&clientcertlevel2breq);
+ test_tls_discard_cert(&someotherrootreq);
unlink(WORKDIR "cacertchain-ctx.pem");
unlink(WORKDIR "cacertchain-with-invalid-ctx.pem");
+ unlink(WORKDIR "servercertchain-ctx.pem");
+ unlink(WORKDIR "servercertchain_incomplete-ctx.pem");
+ unlink(WORKDIR "servercertchain_unsorted-ctx.pem");
+ unlink(WORKDIR "clientcertchain-ctx.pem");
test_tls_cleanup(KEYFILE);
rmdir(WORKDIR);
--
2.50.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 4/6] crypto: stop requiring "key encipherment" usage in x509 certs
2025-09-19 10:10 [PATCH v2 0/6] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé
` (2 preceding siblings ...)
2025-09-19 10:10 ` [PATCH v2 3/6] crypto: allow client/server cert chains Daniel P. Berrangé
@ 2025-09-19 10:10 ` Daniel P. Berrangé
2025-10-16 15:41 ` Eric Blake
2025-09-19 10:10 ` [PATCH v2 5/6] crypto: switch to newer gnutls API for distinguished name Daniel P. Berrangé
2025-09-19 10:10 ` [PATCH v2 6/6] crypto: fix error reporting in cert chain checks Daniel P. Berrangé
5 siblings, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2025-09-19 10:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P. Berrangé
This usage flag was deprecated by RFC8813, such that it is
forbidden to be present for certs using ECDSA/ECDH algorithms,
and in TLS 1.3 is conceptually obsolete.
As such many valid certs will no longer have this key usage
flag set, and QEMU should not be rejecting them, as this
prevents use of otherwise valid & desirable algorithms.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
crypto/tlscredsx509.c | 10 +-------
docs/system/tls.rst | 13 +++-------
tests/unit/crypto-tls-x509-helpers.h | 6 ++---
tests/unit/test-crypto-tlscredsx509.c | 36 +++++++++++++--------------
tests/unit/test-crypto-tlssession.c | 14 +++++------
tests/unit/test-io-channel-tls.c | 4 +--
6 files changed, 34 insertions(+), 49 deletions(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 311de3237d..89a8e261d5 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -144,7 +144,7 @@ qcrypto_tls_creds_check_cert_key_usage(QCryptoTLSCredsX509 *creds,
if (status < 0) {
if (status == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) {
usage = isCA ? GNUTLS_KEY_KEY_CERT_SIGN :
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT;
+ GNUTLS_KEY_DIGITAL_SIGNATURE;
} else {
error_setg(errp,
"Unable to query certificate %s key usage: %s",
@@ -171,14 +171,6 @@ qcrypto_tls_creds_check_cert_key_usage(QCryptoTLSCredsX509 *creds,
return -1;
}
}
- if (!(usage & GNUTLS_KEY_KEY_ENCIPHERMENT)) {
- if (critical) {
- error_setg(errp,
- "Certificate %s usage does not permit key "
- "encipherment", certFile);
- return -1;
- }
- }
}
return 0;
diff --git a/docs/system/tls.rst b/docs/system/tls.rst
index e284c82801..a4f6781d62 100644
--- a/docs/system/tls.rst
+++ b/docs/system/tls.rst
@@ -118,7 +118,6 @@ information for each server, and use it to issue server certificates.
ip_address = 2620:0:cafe::87
ip_address = 2001:24::92
tls_www_server
- encryption_key
signing_key
EOF
# certtool --generate-privkey > server-hostNNN-key.pem
@@ -134,9 +133,8 @@ the subject alt name extension data. The ``tls_www_server`` keyword is
the key purpose extension to indicate this certificate is intended for
usage in a web server. Although QEMU network services are not in fact
HTTP servers (except for VNC websockets), setting this key purpose is
-still recommended. The ``encryption_key`` and ``signing_key`` keyword is
-the key usage extension to indicate this certificate is intended for
-usage in the data session.
+still recommended. The ``signing_key`` keyword is the key usage extension
+to indicate this certificate is intended for usage in the data session.
The ``server-hostNNN-key.pem`` and ``server-hostNNN-cert.pem`` files
should now be securely copied to the server for which they were
@@ -171,7 +169,6 @@ certificates.
organization = Name of your organization
cn = hostNNN.foo.example.com
tls_www_client
- encryption_key
signing_key
EOF
# certtool --generate-privkey > client-hostNNN-key.pem
@@ -187,9 +184,8 @@ the ``dns_name`` and ``ip_address`` fields are not included. The
``tls_www_client`` keyword is the key purpose extension to indicate this
certificate is intended for usage in a web client. Although QEMU network
clients are not in fact HTTP clients, setting this key purpose is still
-recommended. The ``encryption_key`` and ``signing_key`` keyword is the
-key usage extension to indicate this certificate is intended for usage
-in the data session.
+recommended. The ``signing_key`` keyword is the key usage extension to
+indicate this certificate is intended for usage in the data session.
The ``client-hostNNN-key.pem`` and ``client-hostNNN-cert.pem`` files
should now be securely copied to the client for which they were
@@ -222,7 +218,6 @@ client and server instructions in one.
ip_address = 2001:24::92
tls_www_server
tls_www_client
- encryption_key
signing_key
EOF
# certtool --generate-privkey > both-hostNNN-key.pem
diff --git a/tests/unit/crypto-tls-x509-helpers.h b/tests/unit/crypto-tls-x509-helpers.h
index 2a0f7c04fd..7e9a508ad6 100644
--- a/tests/unit/crypto-tls-x509-helpers.h
+++ b/tests/unit/crypto-tls-x509-helpers.h
@@ -148,8 +148,7 @@ void test_tls_cleanup(const char *keyfile);
.basicConstraintsIsCA = false, \
.keyUsageEnable = true, \
.keyUsageCritical = true, \
- .keyUsageValue = \
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, \
+ .keyUsageValue = GNUTLS_KEY_DIGITAL_SIGNATURE, \
.keyPurposeEnable = true, \
.keyPurposeCritical = true, \
.keyPurposeOID1 = GNUTLS_KP_TLS_WWW_CLIENT, \
@@ -168,8 +167,7 @@ void test_tls_cleanup(const char *keyfile);
.basicConstraintsIsCA = false, \
.keyUsageEnable = true, \
.keyUsageCritical = true, \
- .keyUsageValue = \
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, \
+ .keyUsageValue = GNUTLS_KEY_DIGITAL_SIGNATURE, \
.keyPurposeEnable = true, \
.keyPurposeCritical = true, \
.keyPurposeOID1 = GNUTLS_KP_TLS_WWW_SERVER, \
diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c
index 4a32bc4d69..fac6c64cad 100644
--- a/tests/unit/test-crypto-tlscredsx509.c
+++ b/tests/unit/test-crypto-tlscredsx509.c
@@ -166,14 +166,14 @@ int main(int argc, char **argv)
"UK", "qemu.org", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
TLS_CERT_REQ(clientcertreq, cacertreq,
"UK", "qemu", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL,
0, 0);
@@ -196,7 +196,7 @@ int main(int argc, char **argv)
"UK", "qemu.org", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
@@ -211,7 +211,7 @@ int main(int argc, char **argv)
"UK", "qemu.org", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
@@ -226,7 +226,7 @@ int main(int argc, char **argv)
"UK", "qemu.org", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
@@ -250,7 +250,7 @@ int main(int argc, char **argv)
"UK", "qemu.org", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
/* no-basic */
@@ -264,7 +264,7 @@ int main(int argc, char **argv)
"UK", "qemu.org", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
/* Key usage:dig-sig:critical */
@@ -278,7 +278,7 @@ int main(int argc, char **argv)
"UK", "qemu.org", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
@@ -303,7 +303,7 @@ int main(int argc, char **argv)
"UK", "qemu", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT |
+ GNUTLS_KEY_DIGITAL_SIGNATURE |
GNUTLS_KEY_KEY_CERT_SIGN,
false, false, NULL, NULL,
0, 0);
@@ -406,7 +406,7 @@ int main(int argc, char **argv)
"UK", "qemu", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT |
+ GNUTLS_KEY_DIGITAL_SIGNATURE |
GNUTLS_KEY_KEY_CERT_SIGN,
false, false, NULL, NULL,
0, 0);
@@ -508,21 +508,21 @@ int main(int argc, char **argv)
"UK", "qemu.org", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
TLS_CERT_REQ(servercertexp1req, cacertreq,
"UK", "qemu", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, -1);
TLS_CERT_REQ(clientcertexp1req, cacertreq,
"UK", "qemu", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL,
0, -1);
@@ -546,21 +546,21 @@ int main(int argc, char **argv)
"UK", "qemu", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
TLS_CERT_REQ(servercertnew1req, cacertreq,
"UK", "qemu", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
1, 2);
TLS_CERT_REQ(clientcertnew1req, cacertreq,
"UK", "qemu", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL,
1, 2);
@@ -611,14 +611,14 @@ int main(int argc, char **argv)
"UK", "qemu.org", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
TLS_CERT_REQ(clientcertlevel2breq, cacertlevel1breq,
"UK", "qemu client level 2b", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL,
0, 0);
diff --git a/tests/unit/test-crypto-tlssession.c b/tests/unit/test-crypto-tlssession.c
index 554054e934..e8b2e0201c 100644
--- a/tests/unit/test-crypto-tlssession.c
+++ b/tests/unit/test-crypto-tlssession.c
@@ -472,14 +472,14 @@ int main(int argc, char **argv)
"UK", "qemu.org", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
TLS_CERT_REQ(clientcertreq, cacertreq,
"UK", "qemu", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL,
0, 0);
@@ -487,7 +487,7 @@ int main(int argc, char **argv)
"UK", "qemu", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL,
0, 0);
@@ -506,7 +506,7 @@ int main(int argc, char **argv)
"192.168.122.1", "fec0::dead:beaf",
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
/* This intentionally doesn't replicate */
@@ -515,7 +515,7 @@ int main(int argc, char **argv)
"192.168.122.1", "fec0::dead:beaf",
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
@@ -619,14 +619,14 @@ int main(int argc, char **argv)
"UK", "qemu.org", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
TLS_CERT_REQ(clientcertlevel2breq, cacertlevel1breq,
"UK", "qemu client level 2b", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL,
0, 0);
diff --git a/tests/unit/test-io-channel-tls.c b/tests/unit/test-io-channel-tls.c
index e036ac5df4..c2115d45fe 100644
--- a/tests/unit/test-io-channel-tls.c
+++ b/tests/unit/test-io-channel-tls.c
@@ -302,14 +302,14 @@ int main(int argc, char **argv)
"UK", "qemu.org", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_SERVER, NULL,
0, 0);
TLS_CERT_REQ(clientcertreq, cacertreq,
"UK", "qemu", NULL, NULL, NULL, NULL,
true, true, false,
true, true,
- GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT,
+ GNUTLS_KEY_DIGITAL_SIGNATURE,
true, true, GNUTLS_KP_TLS_WWW_CLIENT, NULL,
0, 0);
--
2.50.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 5/6] crypto: switch to newer gnutls API for distinguished name
2025-09-19 10:10 [PATCH v2 0/6] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé
` (3 preceding siblings ...)
2025-09-19 10:10 ` [PATCH v2 4/6] crypto: stop requiring "key encipherment" usage in x509 certs Daniel P. Berrangé
@ 2025-09-19 10:10 ` Daniel P. Berrangé
2025-10-16 15:43 ` Eric Blake
2025-09-19 10:10 ` [PATCH v2 6/6] crypto: fix error reporting in cert chain checks Daniel P. Berrangé
5 siblings, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2025-09-19 10:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P. Berrangé
The new API automatically allocates the right amount of memory
to hold the distinguished name, avoiding the need to loop and
realloc.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
crypto/tlssession.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 86d407a142..0f86d1393f 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -409,20 +409,14 @@ qcrypto_tls_session_check_certificate(QCryptoTLSSession *session,
}
if (i == 0) {
- size_t dnameSize = 1024;
- session->peername = g_malloc(dnameSize);
- requery:
- ret = gnutls_x509_crt_get_dn(cert, session->peername, &dnameSize);
+ gnutls_datum_t dname = {};
+ ret = gnutls_x509_crt_get_dn2(cert, &dname);
if (ret < 0) {
- if (ret == GNUTLS_E_SHORT_MEMORY_BUFFER) {
- session->peername = g_realloc(session->peername,
- dnameSize);
- goto requery;
- }
error_setg(errp, "Cannot get client distinguished name: %s",
gnutls_strerror(ret));
goto error;
}
+ session->peername = (char *)g_steal_pointer(&dname.data);
if (session->authzid) {
bool allow;
--
2.50.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 6/6] crypto: fix error reporting in cert chain checks
2025-09-19 10:10 [PATCH v2 0/6] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé
` (4 preceding siblings ...)
2025-09-19 10:10 ` [PATCH v2 5/6] crypto: switch to newer gnutls API for distinguished name Daniel P. Berrangé
@ 2025-09-19 10:10 ` Daniel P. Berrangé
2025-10-16 15:50 ` Eric Blake
5 siblings, 1 reply; 17+ messages in thread
From: Daniel P. Berrangé @ 2025-09-19 10:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Daniel P. Berrangé
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/6] crypto: only verify CA certs in chain of trust
2025-09-19 10:10 ` [PATCH v2 1/6] crypto: only verify CA certs in chain of trust Daniel P. Berrangé
@ 2025-10-16 14:37 ` Eric Blake
2025-10-16 14:38 ` Daniel P. Berrangé
0 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2025-10-16 14:37 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Henry Kleynhans
On Fri, Sep 19, 2025 at 11:10:17AM +0100, Daniel P. Berrangé wrote:
> From: Henry Kleynhans <hkleynhans@fb.com>
>
> The CA file provided to qemu may contain CA certificates which do not
> form part of the chain of trust for the specific certificate we are
> sanity checking.
>
> This patch changes the sanity checking from validating every CA
> certificate to only checking the CA certificates which are part of the
> chain of trust (issuer chain). Other certificates are ignored.
I agree that relaxing this will permit more certs than before (and
possibly with less CPU cycles spent on the irrelevant portions of the
cert), without weakening the security of the chain we are actually
interested in.
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> crypto/tlscredsx509.c | 57 ++++++++++++++++++++++++---
> tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++-
> 2 files changed, 75 insertions(+), 7 deletions(-)
>
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index cd1f504471..797854ac89 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -315,6 +315,51 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *creds,
> return 0;
> }
>
> +static int
> +qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
> + gnutls_x509_crt_t cert,
> + gnutls_x509_crt_t *cacerts,
> + unsigned int ncacerts,
> + const char *cacertFile,
> + bool isServer,
> + bool isCA,
> + Error **errp)
> +{
> + gnutls_x509_crt_t *cert_to_check = &cert;
> + int checking_issuer = 1;
Why is this int instead of bool? It's only assigned 1 or 0, and local
to the function.
That's a trivial cleanup, so I'm okay if you make that change and add:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/6] crypto: only verify CA certs in chain of trust
2025-10-16 14:37 ` Eric Blake
@ 2025-10-16 14:38 ` Daniel P. Berrangé
0 siblings, 0 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2025-10-16 14:38 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, Henry Kleynhans
On Thu, Oct 16, 2025 at 09:37:51AM -0500, Eric Blake wrote:
> On Fri, Sep 19, 2025 at 11:10:17AM +0100, Daniel P. Berrangé wrote:
> > From: Henry Kleynhans <hkleynhans@fb.com>
> >
> > The CA file provided to qemu may contain CA certificates which do not
> > form part of the chain of trust for the specific certificate we are
> > sanity checking.
> >
> > This patch changes the sanity checking from validating every CA
> > certificate to only checking the CA certificates which are part of the
> > chain of trust (issuer chain). Other certificates are ignored.
>
> I agree that relaxing this will permit more certs than before (and
> possibly with less CPU cycles spent on the irrelevant portions of the
> cert), without weakening the security of the chain we are actually
> interested in.
>
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > crypto/tlscredsx509.c | 57 ++++++++++++++++++++++++---
> > tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++-
> > 2 files changed, 75 insertions(+), 7 deletions(-)
> >
> > diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> > index cd1f504471..797854ac89 100644
> > --- a/crypto/tlscredsx509.c
> > +++ b/crypto/tlscredsx509.c
> > @@ -315,6 +315,51 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *creds,
> > return 0;
> > }
> >
> > +static int
> > +qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds,
> > + gnutls_x509_crt_t cert,
> > + gnutls_x509_crt_t *cacerts,
> > + unsigned int ncacerts,
> > + const char *cacertFile,
> > + bool isServer,
> > + bool isCA,
> > + Error **errp)
> > +{
> > + gnutls_x509_crt_t *cert_to_check = &cert;
> > + int checking_issuer = 1;
>
> Why is this int instead of bool? It's only assigned 1 or 0, and local
> to the function.
No good reason.
>
> That's a trivial cleanup, so I'm okay if you make that change and add:
I'll change to bool.
>
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization: qemu.org | libguestfs.org
>
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 :|
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/6] crypto: remove extraneous pointer usage in gnutls certs
2025-09-19 10:10 ` [PATCH v2 2/6] crypto: remove extraneous pointer usage in gnutls certs Daniel P. Berrangé
@ 2025-10-16 14:52 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2025-10-16 14:52 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, Philippe Mathieu-Daudé
On Fri, Sep 19, 2025 at 11:10:18AM +0100, Daniel P. Berrangé wrote:
> The 'gnutls_x509_crt_t' type is already a pointer, not a struct,
> so the extra level of pointer indirection is not needed.
>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> crypto/tlscredsx509.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/6] crypto: allow client/server cert chains
2025-09-19 10:10 ` [PATCH v2 3/6] crypto: allow client/server cert chains Daniel P. Berrangé
@ 2025-10-16 15:28 ` Eric Blake
2025-10-20 11:22 ` Daniel P. Berrangé
0 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2025-10-16 15:28 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel, matoro
On Fri, Sep 19, 2025 at 11:10:19AM +0100, Daniel P. Berrangé wrote:
> From: matoro <matoro@users.noreply.github.com>
The CC: line has a different email address for matoro than the git
author attribution. Does that matter? I'm not a fan of github's
attempt to make it difficult to reach a contributor outside the github
walled garden.
>
> The existing implementation assumes that client/server certificates are
> single individual certificates. If using publicly-issued certificates,
> or internal CAs that use an intermediate issuer, this is unlikely to be
> the case, and they will instead be certificate chains. While this can
> be worked around by moving the intermediate certificates to the CA
> certificate, which DOES currently support multiple certificates, this
> instead allows the issued certificate chains to be used as-is, without
> requiring the overhead of shuffling certificates around.
>
> Corresponding libvirt change is available here:
> https://gitlab.com/libvirt/libvirt/-/merge_requests/222
>
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> Signed-off-by: matoro <matoro_mailinglist_qemu@matoro.tk>
> [DB: adapted for code conflicts with multi-CA patch]
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> crypto/tlscredsx509.c | 156 ++++++++++++--------------
> tests/unit/test-crypto-tlscredsx509.c | 77 +++++++++++++
> 2 files changed, 147 insertions(+), 86 deletions(-)
>
> -static gnutls_x509_crt_t
> -qcrypto_tls_creds_load_cert(QCryptoTLSCredsX509 *creds,
> - const char *certFile,
> - bool isServer,
> - Error **errp)
> -{
> -
> static int
> -qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds,
> - const char *certFile,
> - gnutls_x509_crt_t **certs,
> - unsigned int *ncerts,
> - Error **errp)
> +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)
> {
Nice consolidation to reduce duplication.
> @@ -520,41 +497,48 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
>
> - if (cert &&
> - qcrypto_tls_creds_check_cert(creds,
> - cert, certFile, isServer,
> - false, errp) < 0) {
> - goto cleanup;
> + for (i = 0; i < ncerts; i++) {
> + if (qcrypto_tls_creds_check_cert(creds,
> + certs[i], certFile,
> + isServer, (i != 0), errp) < 0) {
The () around 'i != 0' look extraneous to me; but that's trivial
formatting so I'm not opposed to keeping them. On the other hand...
> + goto cleanup;
> + }
> }
>
> - if (cert &&
> - qcrypto_tls_creds_check_authority_chain(creds, cert,
> + if (ncerts &&
...here you are doing an implicit conversion of ncerts to bool; why
not do the same implicit conversion of 'i' rather than explicit '(i !=
0)' above?
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/6] crypto: stop requiring "key encipherment" usage in x509 certs
2025-09-19 10:10 ` [PATCH v2 4/6] crypto: stop requiring "key encipherment" usage in x509 certs Daniel P. Berrangé
@ 2025-10-16 15:41 ` Eric Blake
2025-10-20 11:27 ` Daniel P. Berrangé
0 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2025-10-16 15:41 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel
On Fri, Sep 19, 2025 at 11:10:20AM +0100, Daniel P. Berrangé wrote:
> This usage flag was deprecated by RFC8813, such that it is
> forbidden to be present for certs using ECDSA/ECDH algorithms,
> and in TLS 1.3 is conceptually obsolete.
>
> As such many valid certs will no longer have this key usage
> flag set, and QEMU should not be rejecting them, as this
> prevents use of otherwise valid & desirable algorithms.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> crypto/tlscredsx509.c | 10 +-------
> docs/system/tls.rst | 13 +++-------
> tests/unit/crypto-tls-x509-helpers.h | 6 ++---
> tests/unit/test-crypto-tlscredsx509.c | 36 +++++++++++++--------------
> tests/unit/test-crypto-tlssession.c | 14 +++++------
> tests/unit/test-io-channel-tls.c | 4 +--
> 6 files changed, 34 insertions(+), 49 deletions(-)
My understanding is that the reason you coded all the sanity checks
into qemu was to provide saner error messages for users that create
invalid keys than what gnutls does (keys are already hard enough to
create securely, so it is nice to be told how to fix your key rather
than just "it didn't work"). I also understand that newer algorithms
really can't use this flag, and we don't want to reject use of better
algorithms, so this patch makes sense as documented.
Still, is there any risk that for older algorithms, where the 'key
encipherment' bit did matter, that we could now end up processing an
incomplete key that we would have previously rejected with a nice
message but which now goes to gnutls and reverts back to the poorer
error message quality or even worse being used despite being a
security risk? I don't think it is a high risk - fewer people would
be generating certificates that explicitly request an older algorithm
but not following all the recommended steps, compared to the more
common case of people following your documentation and getting the
newest defaults that just work; anyone determined enough to get an
older algorithm deserves the breakage if their explicit instructions
to override the default are weaker than normal.
In saying that, I'm hoping that gnutls still diagnoses certs that
cannot be properly used for the purpose at hand (whether or not the
'key encipherment' bit must be set or cleared), even if it gives a
less-than-stellar diagnostic message about rejecting a cert. If I'm
wrong, and an incomplete cert with an older algorithm but missing the
bit turns into a security bypass, it's much more than QEMU that would
be impacted. So, I'm comfortable with:
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/6] crypto: switch to newer gnutls API for distinguished name
2025-09-19 10:10 ` [PATCH v2 5/6] crypto: switch to newer gnutls API for distinguished name Daniel P. Berrangé
@ 2025-10-16 15:43 ` Eric Blake
0 siblings, 0 replies; 17+ messages in thread
From: Eric Blake @ 2025-10-16 15:43 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel
On Fri, Sep 19, 2025 at 11:10:21AM +0100, Daniel P. Berrangé wrote:
> The new API automatically allocates the right amount of memory
> to hold the distinguished name, avoiding the need to loop and
> realloc.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> crypto/tlssession.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
It's nice to see that the library is slowly becoming easier to use.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/6] crypto: fix error reporting in cert chain checks
2025-09-19 10:10 ` [PATCH v2 6/6] crypto: fix error reporting in cert chain checks Daniel P. Berrangé
@ 2025-10-16 15:50 ` Eric Blake
2025-10-20 11:47 ` Daniel P. Berrangé
0 siblings, 1 reply; 17+ messages in thread
From: Eric Blake @ 2025-10-16 15:50 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/6] crypto: allow client/server cert chains
2025-10-16 15:28 ` Eric Blake
@ 2025-10-20 11:22 ` Daniel P. Berrangé
0 siblings, 0 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2025-10-20 11:22 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel, matoro
On Thu, Oct 16, 2025 at 10:28:59AM -0500, Eric Blake wrote:
> On Fri, Sep 19, 2025 at 11:10:19AM +0100, Daniel P. Berrangé wrote:
> > From: matoro <matoro@users.noreply.github.com>
>
> The CC: line has a different email address for matoro than the git
> author attribution. Does that matter? I'm not a fan of github's
> attempt to make it difficult to reach a contributor outside the github
> walled garden.
>
> >
> > The existing implementation assumes that client/server certificates are
> > single individual certificates. If using publicly-issued certificates,
> > or internal CAs that use an intermediate issuer, this is unlikely to be
> > the case, and they will instead be certificate chains. While this can
> > be worked around by moving the intermediate certificates to the CA
> > certificate, which DOES currently support multiple certificates, this
> > instead allows the issued certificate chains to be used as-is, without
> > requiring the overhead of shuffling certificates around.
> >
> > Corresponding libvirt change is available here:
> > https://gitlab.com/libvirt/libvirt/-/merge_requests/222
> >
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> > Signed-off-by: matoro <matoro_mailinglist_qemu@matoro.tk>
> > [DB: adapted for code conflicts with multi-CA patch]
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > crypto/tlscredsx509.c | 156 ++++++++++++--------------
> > tests/unit/test-crypto-tlscredsx509.c | 77 +++++++++++++
> > 2 files changed, 147 insertions(+), 86 deletions(-)
>
> >
> > -static gnutls_x509_crt_t
> > -qcrypto_tls_creds_load_cert(QCryptoTLSCredsX509 *creds,
> > - const char *certFile,
> > - bool isServer,
> > - Error **errp)
> > -{
>
> > -
> > static int
> > -qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds,
> > - const char *certFile,
> > - gnutls_x509_crt_t **certs,
> > - unsigned int *ncerts,
> > - Error **errp)
> > +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)
> > {
>
> Nice consolidation to reduce duplication.
>
> > @@ -520,41 +497,48 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
>
> >
> > - if (cert &&
> > - qcrypto_tls_creds_check_cert(creds,
> > - cert, certFile, isServer,
> > - false, errp) < 0) {
> > - goto cleanup;
> > + for (i = 0; i < ncerts; i++) {
> > + if (qcrypto_tls_creds_check_cert(creds,
> > + certs[i], certFile,
> > + isServer, (i != 0), errp) < 0) {
>
> The () around 'i != 0' look extraneous to me; but that's trivial
> formatting so I'm not opposed to keeping them. On the other hand...
Yeah, I'll loose the ()
>
> > + goto cleanup;
> > + }
> > }
> >
> > - if (cert &&
> > - qcrypto_tls_creds_check_authority_chain(creds, cert,
> > + if (ncerts &&
>
> ...here you are doing an implicit conversion of ncerts to bool; why
> not do the same implicit conversion of 'i' rather than explicit '(i !=
> 0)' above?
IMHO using an int in a conditional expression "if (<int vriable>)"
has pretty clear intent.
Passing an int to a parameter that expects a bool could just as easily be
indicative of a code bug, as it could be intentionally relying on the
type conversion. IOW, it has fuzzy intent.
So although I didn't write this patch, I would be inclined to write it
the same way it is done here.
>
> Reviewed-by: Eric Blake <eblake@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 :|
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 4/6] crypto: stop requiring "key encipherment" usage in x509 certs
2025-10-16 15:41 ` Eric Blake
@ 2025-10-20 11:27 ` Daniel P. Berrangé
0 siblings, 0 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2025-10-20 11:27 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
On Thu, Oct 16, 2025 at 10:41:47AM -0500, Eric Blake wrote:
> On Fri, Sep 19, 2025 at 11:10:20AM +0100, Daniel P. Berrangé wrote:
> > This usage flag was deprecated by RFC8813, such that it is
> > forbidden to be present for certs using ECDSA/ECDH algorithms,
> > and in TLS 1.3 is conceptually obsolete.
> >
> > As such many valid certs will no longer have this key usage
> > flag set, and QEMU should not be rejecting them, as this
> > prevents use of otherwise valid & desirable algorithms.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > crypto/tlscredsx509.c | 10 +-------
> > docs/system/tls.rst | 13 +++-------
> > tests/unit/crypto-tls-x509-helpers.h | 6 ++---
> > tests/unit/test-crypto-tlscredsx509.c | 36 +++++++++++++--------------
> > tests/unit/test-crypto-tlssession.c | 14 +++++------
> > tests/unit/test-io-channel-tls.c | 4 +--
> > 6 files changed, 34 insertions(+), 49 deletions(-)
>
> My understanding is that the reason you coded all the sanity checks
> into qemu was to provide saner error messages for users that create
> invalid keys than what gnutls does (keys are already hard enough to
> create securely, so it is nice to be told how to fix your key rather
> than just "it didn't work"). I also understand that newer algorithms
> really can't use this flag, and we don't want to reject use of better
> algorithms, so this patch makes sense as documented.
Yes, admin configuration mistakes with TLS certs are an incredibly
common problem, frustratingly hard to diagnose, especially when the
failure only happens at runtime, not startup. So this code is mostly
about "sanity checks" on configuration.
> Still, is there any risk that for older algorithms, where the 'key
> encipherment' bit did matter, that we could now end up processing an
> incomplete key that we would have previously rejected with a nice
> message but which now goes to gnutls and reverts back to the poorer
> error message quality or even worse being used despite being a
> security risk? I don't think it is a high risk - fewer people would
> be generating certificates that explicitly request an older algorithm
> but not following all the recommended steps, compared to the more
> common case of people following your documentation and getting the
> newest defaults that just work; anyone determined enough to get an
> older algorithm deserves the breakage if their explicit instructions
> to override the default are weaker than normal.
Yes, it is a slight degradation in the checks we are doing in
certain scenarios. As the world moves increasingly towards
eliptic curves and/or post-quantum crypto, the check is less
and less important. So its an acceptable tradeoff. I've got
another series coming soon that supports post-quantum crypto
better in QEMU
> In saying that, I'm hoping that gnutls still diagnoses certs that
> cannot be properly used for the purpose at hand (whether or not the
> 'key encipherment' bit must be set or cleared), even if it gives a
> less-than-stellar diagnostic message about rejecting a cert. If I'm
> wrong, and an incomplete cert with an older algorithm but missing the
> bit turns into a security bypass, it's much more than QEMU that would
> be impacted. So, I'm comfortable with:
>
> Reviewed-by: Eric Blake <eblake@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 :|
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 6/6] crypto: fix error reporting in cert chain checks
2025-10-16 15:50 ` Eric Blake
@ 2025-10-20 11:47 ` Daniel P. Berrangé
0 siblings, 0 replies; 17+ messages in thread
From: Daniel P. Berrangé @ 2025-10-20 11:47 UTC (permalink / raw)
To: Eric Blake; +Cc: qemu-devel
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 :|
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2025-10-20 11:48 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-19 10:10 [PATCH v2 0/6] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé
2025-09-19 10:10 ` [PATCH v2 1/6] crypto: only verify CA certs in chain of trust Daniel P. Berrangé
2025-10-16 14:37 ` Eric Blake
2025-10-16 14:38 ` Daniel P. Berrangé
2025-09-19 10:10 ` [PATCH v2 2/6] crypto: remove extraneous pointer usage in gnutls certs Daniel P. Berrangé
2025-10-16 14:52 ` Eric Blake
2025-09-19 10:10 ` [PATCH v2 3/6] crypto: allow client/server cert chains Daniel P. Berrangé
2025-10-16 15:28 ` Eric Blake
2025-10-20 11:22 ` Daniel P. Berrangé
2025-09-19 10:10 ` [PATCH v2 4/6] crypto: stop requiring "key encipherment" usage in x509 certs Daniel P. Berrangé
2025-10-16 15:41 ` Eric Blake
2025-10-20 11:27 ` Daniel P. Berrangé
2025-09-19 10:10 ` [PATCH v2 5/6] crypto: switch to newer gnutls API for distinguished name Daniel P. Berrangé
2025-10-16 15:43 ` Eric Blake
2025-09-19 10:10 ` [PATCH v2 6/6] crypto: fix error reporting in cert chain checks Daniel P. Berrangé
2025-10-16 15:50 ` Eric Blake
2025-10-20 11:47 ` Daniel P. Berrangé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).