* [PATCH 0/7] crypto: misc fixes and improvements to cert handling
@ 2025-07-15 9:29 Daniel P. Berrangé
2025-07-15 9:29 ` [PATCH 1/7] crypto: stop requiring "key encipherment" usage in x509 certs Daniel P. Berrangé
` (6 more replies)
0 siblings, 7 replies; 12+ messages in thread
From: Daniel P. Berrangé @ 2025-07-15 9:29 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.
Daniel P. Berrangé (4):
crypto: stop requiring "key encipherment" usage in x509 certs
crypto: switch to newer gnutls API for distinguished name
crypto: remove extraneous pointer usage in gnutls certs
crypto: fix error reporting in cert chain checks
Henry Kleynhans (2):
crypto: load all certificates in X509 CA file
crypto: only verify CA certs in chain of trust
matoro (1):
crypto: allow client/server cert chains
crypto/tlscredsx509.c | 236 +++++++++++++++-----------
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, 270 insertions(+), 153 deletions(-)
--
2.49.0
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 1/7] crypto: stop requiring "key encipherment" usage in x509 certs 2025-07-15 9:29 [PATCH 0/7] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé @ 2025-07-15 9:29 ` Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 2/7] crypto: switch to newer gnutls API for distinguished name Daniel P. Berrangé ` (5 subsequent siblings) 6 siblings, 0 replies; 12+ messages in thread From: Daniel P. Berrangé @ 2025-07-15 9:29 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 63a72fe47c..997602ec6b 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 3c25d75ca1..2025d75365 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); @@ -599,14 +599,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.49.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/7] crypto: switch to newer gnutls API for distinguished name 2025-07-15 9:29 [PATCH 0/7] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 1/7] crypto: stop requiring "key encipherment" usage in x509 certs Daniel P. Berrangé @ 2025-07-15 9:29 ` Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 3/7] crypto: load all certificates in X509 CA file Daniel P. Berrangé ` (4 subsequent siblings) 6 siblings, 0 replies; 12+ messages in thread From: Daniel P. Berrangé @ 2025-07-15 9:29 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 6d8f8df623..5034776922 100644 --- a/crypto/tlssession.c +++ b/crypto/tlssession.c @@ -373,20 +373,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.49.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/7] crypto: load all certificates in X509 CA file 2025-07-15 9:29 [PATCH 0/7] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 1/7] crypto: stop requiring "key encipherment" usage in x509 certs Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 2/7] crypto: switch to newer gnutls API for distinguished name Daniel P. Berrangé @ 2025-07-15 9:29 ` Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 4/7] crypto: only verify CA certs in chain of trust Daniel P. Berrangé ` (3 subsequent siblings) 6 siblings, 0 replies; 12+ messages in thread From: Daniel P. Berrangé @ 2025-07-15 9:29 UTC (permalink / raw) To: qemu-devel; +Cc: Daniel P. Berrangé, Henry Kleynhans From: Henry Kleynhans <hkleynhans@fb.com> Some CA files may contain multiple intermediaries and roots of trust. These may not fit into the hard-coded limit of 16. Extend the validation code to allocate enough space to load all of the certificates present in the CA file and ensure they are cleaned up. Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> Signed-off-by: Henry Kleynhans <hkleynhans@fb.com> [DB: drop MAX_CERTS constant & whitespace tweaks] Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> --- crypto/tlscredsx509.c | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c index 997602ec6b..3e3ec4971f 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -418,9 +418,8 @@ qcrypto_tls_creds_load_cert(QCryptoTLSCredsX509 *creds, static int qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds, const char *certFile, - gnutls_x509_crt_t *certs, - unsigned int certMax, - size_t *ncerts, + gnutls_x509_crt_t **certs, + unsigned int *ncerts, Error **errp) { gnutls_datum_t data; @@ -441,20 +440,18 @@ qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds, data.data = (unsigned char *)buf; data.size = strlen(buf); - if (gnutls_x509_crt_list_import(certs, &certMax, &data, - GNUTLS_X509_FMT_PEM, 0) < 0) { + 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", certFile); return -1; } - *ncerts = certMax; return 0; } -#define MAX_CERTS 16 static int qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds, bool isServer, @@ -463,12 +460,11 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds, Error **errp) { gnutls_x509_crt_t cert = NULL; - gnutls_x509_crt_t cacerts[MAX_CERTS]; - size_t ncacerts = 0; + gnutls_x509_crt_t *cacerts = NULL; + unsigned int ncacerts = 0; size_t i; int ret = -1; - memset(cacerts, 0, sizeof(cacerts)); if (certFile && access(certFile, R_OK) == 0) { cert = qcrypto_tls_creds_load_cert(creds, @@ -480,8 +476,9 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds, } if (access(cacertFile, R_OK) == 0) { if (qcrypto_tls_creds_load_ca_cert_list(creds, - cacertFile, cacerts, - MAX_CERTS, &ncacerts, + cacertFile, + &cacerts, + &ncacerts, errp) < 0) { goto cleanup; } @@ -518,6 +515,8 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds, for (i = 0; i < ncacerts; i++) { gnutls_x509_crt_deinit(cacerts[i]); } + gnutls_free(cacerts); + return ret; } -- 2.49.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/7] crypto: only verify CA certs in chain of trust 2025-07-15 9:29 [PATCH 0/7] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé ` (2 preceding siblings ...) 2025-07-15 9:29 ` [PATCH 3/7] crypto: load all certificates in X509 CA file Daniel P. Berrangé @ 2025-07-15 9:29 ` Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 5/7] crypto: remove extraneous pointer usage in gnutls certs Daniel P. Berrangé ` (2 subsequent siblings) 6 siblings, 0 replies; 12+ messages in thread From: Daniel P. Berrangé @ 2025-07-15 9:29 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 3e3ec4971f..ec6ff43af2 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -307,6 +307,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, @@ -491,12 +536,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 2025d75365..78b00401d1 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.49.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 5/7] crypto: remove extraneous pointer usage in gnutls certs 2025-07-15 9:29 [PATCH 0/7] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé ` (3 preceding siblings ...) 2025-07-15 9:29 ` [PATCH 4/7] crypto: only verify CA certs in chain of trust Daniel P. Berrangé @ 2025-07-15 9:29 ` Daniel P. Berrangé 2025-07-15 9:35 ` Philippe Mathieu-Daudé 2025-07-15 9:29 ` [PATCH 6/7] crypto: fix error reporting in cert chain checks Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 7/7] crypto: allow client/server cert chains Daniel P. Berrangé 6 siblings, 1 reply; 12+ messages in thread From: Daniel P. Berrangé @ 2025-07-15 9:29 UTC (permalink / raw) To: qemu-devel; +Cc: Daniel P. Berrangé The 'gnutls_x509_crt_t' type is already a pointer, not a struct, so the extra level of pointer indirection is not needed. 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 ec6ff43af2..95ddfe2f98 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -317,25 +317,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, @@ -343,7 +343,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.49.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 5/7] crypto: remove extraneous pointer usage in gnutls certs 2025-07-15 9:29 ` [PATCH 5/7] crypto: remove extraneous pointer usage in gnutls certs Daniel P. Berrangé @ 2025-07-15 9:35 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 12+ messages in thread From: Philippe Mathieu-Daudé @ 2025-07-15 9:35 UTC (permalink / raw) To: Daniel P. Berrangé, qemu-devel On 15/7/25 11:29, 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. > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> > --- > crypto/tlscredsx509.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org> ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 6/7] crypto: fix error reporting in cert chain checks 2025-07-15 9:29 [PATCH 0/7] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé ` (4 preceding siblings ...) 2025-07-15 9:29 ` [PATCH 5/7] crypto: remove extraneous pointer usage in gnutls certs Daniel P. Berrangé @ 2025-07-15 9:29 ` Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 7/7] crypto: allow client/server cert chains Daniel P. Berrangé 6 siblings, 0 replies; 12+ messages in thread From: Daniel P. Berrangé @ 2025-07-15 9:29 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 | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c index 95ddfe2f98..d6897ca57b 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -318,11 +318,11 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds, Error **errp) { gnutls_x509_crt_t cert_to_check = cert; - int checking_issuer = 1; - int retval = 0; + gnutls_datum_t dn = {}; + int rv; - 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)) { @@ -337,19 +337,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; } + rv = gnutls_x509_crt_get_dn2(cert_to_check, &dn); + if (rv < 0) { + error_setg(errp, "Unable to fetch cert DN: %s", + gnutls_strerror(rv)); + return -1; + } + error_setg(errp, "Cert '%s' has no issuer in CA chain", dn.data); + gnutls_free(dn.data); return -1; } -- 2.49.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 7/7] crypto: allow client/server cert chains 2025-07-15 9:29 [PATCH 0/7] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé ` (5 preceding siblings ...) 2025-07-15 9:29 ` [PATCH 6/7] crypto: fix error reporting in cert chain checks Daniel P. Berrangé @ 2025-07-15 9:29 ` Daniel P. Berrangé 2025-07-15 9:46 ` Philippe Mathieu-Daudé 6 siblings, 1 reply; 12+ messages in thread From: Daniel P. Berrangé @ 2025-07-15 9:29 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 | 157 ++++++++++++-------------- tests/unit/test-crypto-tlscredsx509.c | 77 +++++++++++++ 2 files changed, 147 insertions(+), 87 deletions(-) diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c index d6897ca57b..0b200c5147 100644 --- a/crypto/tlscredsx509.c +++ b/crypto/tlscredsx509.c @@ -309,7 +309,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, @@ -317,10 +318,33 @@ qcrypto_tls_creds_check_authority_chain(QCryptoTLSCredsX509 *creds, bool isCA, Error **errp) { - gnutls_x509_crt_t cert_to_check = cert; - gnutls_datum_t dn = {}; + gnutls_x509_crt_t cert_to_check = certs[ncerts - 1]; + gnutls_datum_t dn = {}, dnissuer = {}; int rv; + for (int i = 0; i < (ncerts - 1); i++) { + if (!gnutls_x509_crt_check_issuer(certs[i], certs[i + 1])) { + rv = gnutls_x509_crt_get_dn2(certs[i], &dn); + if (rv < 0) { + error_setg(errp, "Unable to fetch cert DN: %s", + gnutls_strerror(rv)); + return -1; + } + rv = gnutls_x509_crt_get_dn2(certs[i + 1], &dnissuer); + if (rv < 0) { + gnutls_free(dn.data); + error_setg(errp, "Unable to fetch cert DN: %s", + gnutls_strerror(rv)); + 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; + } + } + for (;;) { gnutls_x509_crt_t cert_issuer = NULL; @@ -365,7 +389,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, @@ -375,7 +400,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) { @@ -417,66 +442,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; @@ -499,7 +472,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; } @@ -515,7 +490,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; @@ -523,41 +499,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; } @@ -565,8 +548,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 78b00401d1..fac6c64cad 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.49.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 7/7] crypto: allow client/server cert chains 2025-07-15 9:29 ` [PATCH 7/7] crypto: allow client/server cert chains Daniel P. Berrangé @ 2025-07-15 9:46 ` Philippe Mathieu-Daudé 2025-07-15 16:09 ` Daniel P. Berrangé 0 siblings, 1 reply; 12+ messages in thread From: Philippe Mathieu-Daudé @ 2025-07-15 9:46 UTC (permalink / raw) To: Daniel P. Berrangé, qemu-devel; +Cc: matoro On 15/7/25 11:29, Daniel P. Berrangé wrote: > From: matoro <matoro@users.noreply.github.com> Should we use <matoro_mailinglist_qemu@matoro.tk> here? > > 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 | 157 ++++++++++++-------------- > tests/unit/test-crypto-tlscredsx509.c | 77 +++++++++++++ > 2 files changed, 147 insertions(+), 87 deletions(-) ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 7/7] crypto: allow client/server cert chains 2025-07-15 9:46 ` Philippe Mathieu-Daudé @ 2025-07-15 16:09 ` Daniel P. Berrangé 2025-07-15 17:26 ` Philippe Mathieu-Daudé 0 siblings, 1 reply; 12+ messages in thread From: Daniel P. Berrangé @ 2025-07-15 16:09 UTC (permalink / raw) To: Philippe Mathieu-Daudé; +Cc: qemu-devel, matoro On Tue, Jul 15, 2025 at 11:46:31AM +0200, Philippe Mathieu-Daudé wrote: > On 15/7/25 11:29, Daniel P. Berrangé wrote: > > From: matoro <matoro@users.noreply.github.com> > > Should we use <matoro_mailinglist_qemu@matoro.tk> here? I generally don't like to change the git metadata that a user submits with unless it is clearly broken, which I don't think is the case here. > > > > > 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 | 157 ++++++++++++-------------- > > tests/unit/test-crypto-tlscredsx509.c | 77 +++++++++++++ > > 2 files changed, 147 insertions(+), 87 deletions(-) > 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] 12+ messages in thread
* Re: [PATCH 7/7] crypto: allow client/server cert chains 2025-07-15 16:09 ` Daniel P. Berrangé @ 2025-07-15 17:26 ` Philippe Mathieu-Daudé 0 siblings, 0 replies; 12+ messages in thread From: Philippe Mathieu-Daudé @ 2025-07-15 17:26 UTC (permalink / raw) To: Daniel P. Berrangé; +Cc: qemu-devel, matoro On 15/7/25 18:09, Daniel P. Berrangé wrote: > On Tue, Jul 15, 2025 at 11:46:31AM +0200, Philippe Mathieu-Daudé wrote: >> On 15/7/25 11:29, Daniel P. Berrangé wrote: >>> From: matoro <matoro@users.noreply.github.com> >> >> Should we use <matoro_mailinglist_qemu@matoro.tk> here? > > I generally don't like to change the git metadata that a user > submits with unless it is clearly broken, which I don't think > is the case here. I find confusing to have a distinct email for author ... > >> >>> >>> 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> ... and the S-o-b. Anyway this isn't the first case, so I don't mind at all. >>> [DB: adapted for code conflicts with multi-CA patch] >>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com> >>> --- >>> crypto/tlscredsx509.c | 157 ++++++++++++-------------- >>> tests/unit/test-crypto-tlscredsx509.c | 77 +++++++++++++ >>> 2 files changed, 147 insertions(+), 87 deletions(-) >> > > With regards, > Daniel ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2025-07-15 17:59 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-07-15 9:29 [PATCH 0/7] crypto: misc fixes and improvements to cert handling Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 1/7] crypto: stop requiring "key encipherment" usage in x509 certs Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 2/7] crypto: switch to newer gnutls API for distinguished name Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 3/7] crypto: load all certificates in X509 CA file Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 4/7] crypto: only verify CA certs in chain of trust Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 5/7] crypto: remove extraneous pointer usage in gnutls certs Daniel P. Berrangé 2025-07-15 9:35 ` Philippe Mathieu-Daudé 2025-07-15 9:29 ` [PATCH 6/7] crypto: fix error reporting in cert chain checks Daniel P. Berrangé 2025-07-15 9:29 ` [PATCH 7/7] crypto: allow client/server cert chains Daniel P. Berrangé 2025-07-15 9:46 ` Philippe Mathieu-Daudé 2025-07-15 16:09 ` Daniel P. Berrangé 2025-07-15 17:26 ` Philippe Mathieu-Daudé
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).