[PATCH 0/7] tls: Improve validation of certificates if multiple certs are concatenated in one file
Our code handled properly only multiple CA certs in one file. This patch extends the validation also to multiple client/server certs in one file. Peter Krempa (7): rpc: virnettlscontext: Fix formatting of function definitions virNetTLSContextNewPath: Refactor temporary variable usage virNetTLSCertCheckPair: Fix function definition formatting rpc: virnettlscert: Rename virNetTLSCertLoadCAListFromFile to virNetTLSCertLoadListFromFile virPKIValidateIdentity: Validate all concatenated certificates virNetTLSCertSanityCheck: Validate all concatenated certs Remove unused 'virNetTLSCertLoadFromFile' src/rpc/virnettlscert.c | 94 ++++++++++++-------------------------- src/rpc/virnettlscert.h | 6 ++- src/rpc/virnettlscontext.c | 89 +++++++++++++++++------------------- tools/virt-pki-validate.c | 20 ++++++-- 4 files changed, 90 insertions(+), 119 deletions(-) -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscontext.c | 74 +++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index e8023133b4..bf83857a05 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -188,14 +188,14 @@ static int virNetTLSContextLoadCredentials(virNetTLSContext *ctxt, static virNetTLSContext *virNetTLSContextNew(const char *cacert, - const char *cacrl, - const char *cert, - const char *key, - const char *const *x509dnACL, - const char *priority, - bool sanityCheckCert, - bool requireValidCert, - bool isServer) + const char *cacrl, + const char *cert, + const char *key, + const char *const *x509dnACL, + const char *priority, + bool sanityCheckCert, + bool requireValidCert, + bool isServer) { virNetTLSContext *ctxt; int err; @@ -301,12 +301,12 @@ static int virNetTLSContextLocateCredentials(const char *pkipath, static virNetTLSContext *virNetTLSContextNewPath(const char *pkipath, - bool tryUserPkiPath, - const char *const *x509dnACL, - const char *priority, - bool sanityCheckCert, - bool requireValidCert, - bool isServer) + bool tryUserPkiPath, + const char *const *x509dnACL, + const char *priority, + bool sanityCheckCert, + bool requireValidCert, + bool isServer) { char *cacert = NULL, *cacrl = NULL, *key = NULL, *cert = NULL; virNetTLSContext *ctxt = NULL; @@ -328,21 +328,21 @@ static virNetTLSContext *virNetTLSContextNewPath(const char *pkipath, } virNetTLSContext *virNetTLSContextNewServerPath(const char *pkipath, - bool tryUserPkiPath, - const char *const *x509dnACL, - const char *priority, - bool sanityCheckCert, - bool requireValidCert) + bool tryUserPkiPath, + const char *const *x509dnACL, + const char *priority, + bool sanityCheckCert, + bool requireValidCert) { return virNetTLSContextNewPath(pkipath, tryUserPkiPath, x509dnACL, priority, sanityCheckCert, requireValidCert, true); } virNetTLSContext *virNetTLSContextNewClientPath(const char *pkipath, - bool tryUserPkiPath, - const char *priority, - bool sanityCheckCert, - bool requireValidCert) + bool tryUserPkiPath, + const char *priority, + bool sanityCheckCert, + bool requireValidCert) { return virNetTLSContextNewPath(pkipath, tryUserPkiPath, NULL, priority, sanityCheckCert, requireValidCert, false); @@ -350,13 +350,13 @@ virNetTLSContext *virNetTLSContextNewClientPath(const char *pkipath, virNetTLSContext *virNetTLSContextNewServer(const char *cacert, - const char *cacrl, - const char *cert, - const char *key, - const char *const *x509dnACL, - const char *priority, - bool sanityCheckCert, - bool requireValidCert) + const char *cacrl, + const char *cert, + const char *key, + const char *const *x509dnACL, + const char *priority, + bool sanityCheckCert, + bool requireValidCert) { return virNetTLSContextNew(cacert, cacrl, cert, key, x509dnACL, priority, sanityCheckCert, requireValidCert, true); @@ -406,12 +406,12 @@ int virNetTLSContextReloadForServer(virNetTLSContext *ctxt, virNetTLSContext *virNetTLSContextNewClient(const char *cacert, - const char *cacrl, - const char *cert, - const char *key, - const char *priority, - bool sanityCheckCert, - bool requireValidCert) + const char *cacrl, + const char *cert, + const char *key, + const char *priority, + bool sanityCheckCert, + bool requireValidCert) { return virNetTLSContextNew(cacert, cacrl, cert, key, NULL, priority, sanityCheckCert, requireValidCert, false); @@ -594,7 +594,7 @@ virNetTLSSessionPull(void *opaque, void *buf, size_t len) virNetTLSSession *virNetTLSSessionNew(virNetTLSContext *ctxt, - const char *hostname) + const char *hostname) { virNetTLSSession *sess; int err; -- 2.50.0
On Thu, Jul 17, 2025 at 05:28:04PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscontext.c | 74 +++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 37 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@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 :|
From: Peter Krempa <pkrempa@redhat.com> Use autofree for all temporary variables and return the result directly. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscontext.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index bf83857a05..f857bb2339 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -308,23 +308,18 @@ static virNetTLSContext *virNetTLSContextNewPath(const char *pkipath, bool requireValidCert, bool isServer) { - char *cacert = NULL, *cacrl = NULL, *key = NULL, *cert = NULL; - virNetTLSContext *ctxt = NULL; + g_autofree char *cacert = NULL; + g_autofree char *cacrl = NULL; + g_autofree char *key = NULL; + g_autofree char *cert = NULL; if (virNetTLSContextLocateCredentials(pkipath, tryUserPkiPath, isServer, &cacert, &cacrl, &cert, &key) < 0) return NULL; - ctxt = virNetTLSContextNew(cacert, cacrl, cert, key, + return virNetTLSContextNew(cacert, cacrl, cert, key, x509dnACL, priority, sanityCheckCert, requireValidCert, isServer); - - VIR_FREE(cacert); - VIR_FREE(cacrl); - VIR_FREE(key); - VIR_FREE(cert); - - return ctxt; } virNetTLSContext *virNetTLSContextNewServerPath(const char *pkipath, -- 2.50.0
On Thu, Jul 17, 2025 at 05:28:05PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Use autofree for all temporary variables and return the result directly.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscontext.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@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 :|
From: Peter Krempa <pkrempa@redhat.com> Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscert.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/rpc/virnettlscert.c b/src/rpc/virnettlscert.c index 6a723c1ed4..774dd5989a 100644 --- a/src/rpc/virnettlscert.c +++ b/src/rpc/virnettlscert.c @@ -323,11 +323,11 @@ static int virNetTLSCertCheck(gnutls_x509_crt_t cert, static int virNetTLSCertCheckPair(gnutls_x509_crt_t cert, - const char *certFile, - gnutls_x509_crt_t *cacerts, - size_t ncacerts, - const char *cacertFile, - bool isServer) + const char *certFile, + gnutls_x509_crt_t *cacerts, + size_t ncacerts, + const char *cacertFile, + bool isServer) { unsigned int status; -- 2.50.0
On Thu, Jul 17, 2025 at 05:28:06PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscert.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@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 :|
From: Peter Krempa <pkrempa@redhat.com> The function can load a generic list of certs, it doesn't necessarily have to be the list of CAs. Rename the function, and change error to be generic. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscert.c | 14 +++++++------- src/rpc/virnettlscert.h | 5 +++++ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/src/rpc/virnettlscert.c b/src/rpc/virnettlscert.c index 774dd5989a..3efc4f0716 100644 --- a/src/rpc/virnettlscert.c +++ b/src/rpc/virnettlscert.c @@ -408,10 +408,10 @@ gnutls_x509_crt_t virNetTLSCertLoadFromFile(const char *certFile, } -static int virNetTLSCertLoadCAListFromFile(const char *certFile, - gnutls_x509_crt_t *certs, - unsigned int certMax, - size_t *ncerts) +int virNetTLSCertLoadListFromFile(const char *certFile, + gnutls_x509_crt_t *certs, + unsigned int certMax, + size_t *ncerts) { gnutls_datum_t data; g_autofree char *buf = NULL; @@ -427,7 +427,7 @@ static int virNetTLSCertLoadCAListFromFile(const char *certFile, if (gnutls_x509_crt_list_import(certs, &certMax, &data, GNUTLS_X509_FMT_PEM, 0) < 0) { virReportError(VIR_ERR_SYSTEM_ERROR, - _("Unable to import CA certificate list %1$s"), + _("Unable to import certificate list %1$s"), certFile); return -1; } @@ -452,8 +452,8 @@ int virNetTLSCertSanityCheck(bool isServer, !(cert = virNetTLSCertLoadFromFile(certFile, isServer))) goto cleanup; if ((access(cacertFile, R_OK) == 0) && - virNetTLSCertLoadCAListFromFile(cacertFile, cacerts, - MAX_CERTS, &ncacerts) < 0) + virNetTLSCertLoadListFromFile(cacertFile, cacerts, + MAX_CERTS, &ncacerts) < 0) goto cleanup; if (cert && diff --git a/src/rpc/virnettlscert.h b/src/rpc/virnettlscert.h index 0ac511a141..a2f591d172 100644 --- a/src/rpc/virnettlscert.h +++ b/src/rpc/virnettlscert.h @@ -40,3 +40,8 @@ char *virNetTLSCertValidate(gnutls_x509_crt_t cert, gnutls_x509_crt_t virNetTLSCertLoadFromFile(const char *certFile, bool isServer); + +int virNetTLSCertLoadListFromFile(const char *certFile, + gnutls_x509_crt_t *certs, + unsigned int certMax, + size_t *ncerts); -- 2.50.0
On Thu, Jul 17, 2025 at 05:28:07PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
The function can load a generic list of certs, it doesn't necessarily have to be the list of CAs. Rename the function, and change error to be generic.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscert.c | 14 +++++++------- src/rpc/virnettlscert.h | 5 +++++ 2 files changed, 12 insertions(+), 7 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@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 :|
From: Peter Krempa <pkrempa@redhat.com> Since gnutls and thus by extension libvirt allows passing multiple certificates in one file by concatenating them, virt-pki-validate ought to validate the hostname of all of them, instead of only the first one to prevent issues when wrong certs are concatenated. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- tools/virt-pki-validate.c | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tools/virt-pki-validate.c b/tools/virt-pki-validate.c index e693ffaed6..a8ea396550 100644 --- a/tools/virt-pki-validate.c +++ b/tools/virt-pki-validate.c @@ -283,19 +283,29 @@ virPKIValidateIdentity(bool isServer, bool system, const char *path) } if (isServer) { - gnutls_x509_crt_t crt; + gnutls_x509_crt_t crts[16] = { 0 }; + size_t ncrts = 0; virValidateCheck(scope, "%s", _("Checking cert hostname match")); - if (!(crt = virNetTLSCertLoadFromFile(cert, true))) { + if (virNetTLSCertLoadListFromFile(cert, crts, 16, &ncrts) < 0) { virValidateFail(VIR_VALIDATE_FAIL, _("Unable to load %1$s: %2$s"), cert, virGetLastErrorMessage()); + ok = false; } else { g_autofree char *hostname = virGetHostname(); - int ret = gnutls_x509_crt_check_hostname(crt, hostname); - gnutls_x509_crt_deinit(crt); - if (!ret) { + bool mismatch = false; + size_t i; + + for (i = 0; i < ncrts; i++) { + if (gnutls_x509_crt_check_hostname(crts[i], hostname) == 0) + mismatch = true; + + gnutls_x509_crt_deinit(crts[i]); + } + + if (mismatch) { /* Only warning, since there can be valid reasons for mis-match */ virValidateFail(VIR_VALIDATE_WARN, _("Certificate %1$s owner does not match the hostname %2$s"), -- 2.50.0
From: Peter Krempa <pkrempa@redhat.com> Similarly to how we iterate the list of CAs in the concatenated bundle there's a possibility of the server/client certificates to be concatenated as well. If for some case the first certificate is okay but the further one have e.g. invalid signatures the validation code would not reject them but we'd encounter failures later when gnutls tries to use them. Iterate also the client/server certs rather than just the CAs. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscert.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/src/rpc/virnettlscert.c b/src/rpc/virnettlscert.c index 3efc4f0716..2724f55bbe 100644 --- a/src/rpc/virnettlscert.c +++ b/src/rpc/virnettlscert.c @@ -442,38 +442,43 @@ int virNetTLSCertSanityCheck(bool isServer, const char *cacertFile, const char *certFile) { - gnutls_x509_crt_t cert = NULL; + gnutls_x509_crt_t certs[MAX_CERTS] = { 0 }; + size_t ncerts = 0; gnutls_x509_crt_t cacerts[MAX_CERTS] = { 0 }; size_t ncacerts = 0; size_t i; int ret = -1; if ((access(certFile, R_OK) == 0) && - !(cert = virNetTLSCertLoadFromFile(certFile, isServer))) + virNetTLSCertLoadListFromFile(certFile, certs, MAX_CERTS, &ncerts) < 0) goto cleanup; + if ((access(cacertFile, R_OK) == 0) && virNetTLSCertLoadListFromFile(cacertFile, cacerts, MAX_CERTS, &ncacerts) < 0) goto cleanup; - if (cert && - virNetTLSCertCheck(cert, certFile, isServer, false) < 0) - goto cleanup; - for (i = 0; i < ncacerts; i++) { - if (virNetTLSCertCheck(cacerts[i], cacertFile, isServer, true) < 0) + g_autofree char *cacertid = g_strdup_printf("%s[%zu]", cacertFile, i); + if (virNetTLSCertCheck(cacerts[i], cacertid, isServer, true) < 0) goto cleanup; } - if (cert && ncacerts && - virNetTLSCertCheckPair(cert, certFile, cacerts, ncacerts, cacertFile, isServer) < 0) - goto cleanup; + for (i = 0; i < ncerts; i++) { + g_autofree char *certid = g_strdup_printf("%s[%zu]", certFile, i); + if (virNetTLSCertCheck(certs[i], certid, isServer, false) < 0) + goto cleanup; + + if (ncacerts && + virNetTLSCertCheckPair(certs[i], certid, cacerts, ncacerts, cacertFile, isServer) < 0) + goto cleanup; + } 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]); return ret; -- 2.50.0
On Thu, Jul 17, 2025 at 05:28:09PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Similarly to how we iterate the list of CAs in the concatenated bundle there's a possibility of the server/client certificates to be concatenated as well.
If for some case the first certificate is okay but the further one have e.g. invalid signatures the validation code would not reject them but we'd encounter failures later when gnutls tries to use them.
Iterate also the client/server certs rather than just the CAs.
Was there some bug that motivated this change ? I'd like to keep libvirt's behaviour in sync with QEMU's behaviour to the greatest extent possible. I've just been finalizing changes to QEMU to cope with mutliple certs in the server-cert.pem file, but the semantics there are the certs are a chain of intermediate certs, all of which must be valid. ie, currently we allow ca-cert.pem server-cert.pem | | |------+--------------------------| |---+-------| Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert but the intent is to support ca-cert.pem server-cert.pem | | |------+--| |----------------------------+-------| Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert Or ca-cert.pem server-cert.pem | | |------+-------------| |---------------+-------| Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert the rational is that these splits reflect how some CA will give you your certs to begin with, and we want to allow them to be used directly. My intent was to copy the QEMU change into libvirt once it was merged in QEMU, so from that POV I'm not a fan of making the some of the changes in this series. Beyond that I'm also working post-quantum crypto support, which will require us to load multiple distinct server-cert-NNN.pem files, each with an independant set of certs, which are selected at runtime based on negotiated ciphers in the TLS handshake.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscert.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/src/rpc/virnettlscert.c b/src/rpc/virnettlscert.c index 3efc4f0716..2724f55bbe 100644 --- a/src/rpc/virnettlscert.c +++ b/src/rpc/virnettlscert.c @@ -442,38 +442,43 @@ int virNetTLSCertSanityCheck(bool isServer, const char *cacertFile, const char *certFile) { - gnutls_x509_crt_t cert = NULL; + gnutls_x509_crt_t certs[MAX_CERTS] = { 0 }; + size_t ncerts = 0; gnutls_x509_crt_t cacerts[MAX_CERTS] = { 0 }; size_t ncacerts = 0; size_t i; int ret = -1;
if ((access(certFile, R_OK) == 0) && - !(cert = virNetTLSCertLoadFromFile(certFile, isServer))) + virNetTLSCertLoadListFromFile(certFile, certs, MAX_CERTS, &ncerts) < 0) goto cleanup; + if ((access(cacertFile, R_OK) == 0) && virNetTLSCertLoadListFromFile(cacertFile, cacerts, MAX_CERTS, &ncacerts) < 0) goto cleanup;
- if (cert && - virNetTLSCertCheck(cert, certFile, isServer, false) < 0) - goto cleanup; - for (i = 0; i < ncacerts; i++) { - if (virNetTLSCertCheck(cacerts[i], cacertFile, isServer, true) < 0) + g_autofree char *cacertid = g_strdup_printf("%s[%zu]", cacertFile, i); + if (virNetTLSCertCheck(cacerts[i], cacertid, isServer, true) < 0) goto cleanup; }
- if (cert && ncacerts && - virNetTLSCertCheckPair(cert, certFile, cacerts, ncacerts, cacertFile, isServer) < 0) - goto cleanup; + for (i = 0; i < ncerts; i++) { + g_autofree char *certid = g_strdup_printf("%s[%zu]", certFile, i); + if (virNetTLSCertCheck(certs[i], certid, isServer, false) < 0) + goto cleanup; + + if (ncacerts && + virNetTLSCertCheckPair(certs[i], certid, cacerts, ncacerts, cacertFile, isServer) < 0) + goto cleanup; + }
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]); return ret; -- 2.50.0
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 :|
On Thu, Jul 17, 2025 at 17:02:33 +0100, Daniel P. Berrangé wrote:
On Thu, Jul 17, 2025 at 05:28:09PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Similarly to how we iterate the list of CAs in the concatenated bundle there's a possibility of the server/client certificates to be concatenated as well.
If for some case the first certificate is okay but the further one have e.g. invalid signatures the validation code would not reject them but we'd encounter failures later when gnutls tries to use them.
Iterate also the client/server certs rather than just the CAs.
Was there some bug that motivated this change ?
Yes-ish. I've abused the fact that gnutls loads everything from the file ...
I'd like to keep libvirt's behaviour in sync with QEMU's behaviour to the greatest extent possible. I've just been finalizing changes to QEMU to cope with mutliple certs in the server-cert.pem file, but the semantics there are the certs are a chain of intermediate certs, all of which must be valid.
ie, currently we allow
ca-cert.pem server-cert.pem | | |------+--------------------------| |---+-------|
Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert
but the intent is to support
ca-cert.pem server-cert.pem | | |------+--| |----------------------------+-------|
Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert
Or
ca-cert.pem server-cert.pem | | |------+-------------| |---------------+-------|
Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert
... meant to facilitate the above ....
the rational is that these splits reflect how some CA will give you your certs to begin with, and we want to allow them to be used directly.
My intent was to copy the QEMU change into libvirt once it was merged in QEMU, so from that POV I'm not a fan of making the some of the changes in this series.
Beyond that I'm also working post-quantum crypto support, which will require us to load multiple distinct server-cert-NNN.pem files, each with an independant set of certs, which are selected at runtime based on negotiated ciphers in the TLS handshake.
... in order to load certs with different (also the fancy new post-quantum) signature algorithms. Since I didn't notice that the crypto policy in fedora 42 doesn't yet trust some of those (e.g. mldsa65), some of the certificates I've concatenated weren't actually passing the checks. Based on how the checks are written though it depended on the order of the certificates for 'virt-pki-validate' and libvirtd to actually report the error. E.g. if the RSA-signed cert was first it claimed pass but didn't work with mldsa. Now that you mention that you are going to allow explicit extra server certs to be loaded specifically that will make more sense.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscert.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
diff --git a/src/rpc/virnettlscert.c b/src/rpc/virnettlscert.c index 3efc4f0716..2724f55bbe 100644 --- a/src/rpc/virnettlscert.c +++ b/src/rpc/virnettlscert.c @@ -442,38 +442,43 @@ int virNetTLSCertSanityCheck(bool isServer, const char *cacertFile, const char *certFile) { - gnutls_x509_crt_t cert = NULL; + gnutls_x509_crt_t certs[MAX_CERTS] = { 0 }; + size_t ncerts = 0; gnutls_x509_crt_t cacerts[MAX_CERTS] = { 0 }; size_t ncacerts = 0; size_t i; int ret = -1;
if ((access(certFile, R_OK) == 0) && - !(cert = virNetTLSCertLoadFromFile(certFile, isServer))) + virNetTLSCertLoadListFromFile(certFile, certs, MAX_CERTS, &ncerts) < 0) goto cleanup; + if ((access(cacertFile, R_OK) == 0) && virNetTLSCertLoadListFromFile(cacertFile, cacerts, MAX_CERTS, &ncacerts) < 0) goto cleanup;
- if (cert && - virNetTLSCertCheck(cert, certFile, isServer, false) < 0) - goto cleanup; - for (i = 0; i < ncacerts; i++) { - if (virNetTLSCertCheck(cacerts[i], cacertFile, isServer, true) < 0) + g_autofree char *cacertid = g_strdup_printf("%s[%zu]", cacertFile, i); + if (virNetTLSCertCheck(cacerts[i], cacertid, isServer, true) < 0) goto cleanup; }
- if (cert && ncacerts && - virNetTLSCertCheckPair(cert, certFile, cacerts, ncacerts, cacertFile, isServer) < 0) - goto cleanup; + for (i = 0; i < ncerts; i++) { + g_autofree char *certid = g_strdup_printf("%s[%zu]", certFile, i); + if (virNetTLSCertCheck(certs[i], certid, isServer, false) < 0) + goto cleanup; + + if (ncacerts && + virNetTLSCertCheckPair(certs[i], certid, cacerts, ncacerts, cacertFile, isServer) < 0) + goto cleanup;
And actually (IIUC) also allows using the proper call with gnutls_x509_crt_list_verify(certs, ncerts, instead of gnutls_x509_crt_list_verify(certs, ncerts, inside virNetTLSCertCheckPair which didn't work for my case because (again IIUC) since some the certs I've concatenated belonged to another CA they couldn't be verified this way. Thus if you're going to be fixing both of these I'll just push the cleanup patches.
On Fri, Jul 18, 2025 at 09:39:22AM +0200, Peter Krempa wrote:
On Thu, Jul 17, 2025 at 17:02:33 +0100, Daniel P. Berrangé wrote:
On Thu, Jul 17, 2025 at 05:28:09PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Similarly to how we iterate the list of CAs in the concatenated bundle there's a possibility of the server/client certificates to be concatenated as well.
If for some case the first certificate is okay but the further one have e.g. invalid signatures the validation code would not reject them but we'd encounter failures later when gnutls tries to use them.
Iterate also the client/server certs rather than just the CAs.
Was there some bug that motivated this change ?
Yes-ish. I've abused the fact that gnutls loads everything from the file ...
I'd like to keep libvirt's behaviour in sync with QEMU's behaviour to the greatest extent possible. I've just been finalizing changes to QEMU to cope with mutliple certs in the server-cert.pem file, but the semantics there are the certs are a chain of intermediate certs, all of which must be valid.
ie, currently we allow
ca-cert.pem server-cert.pem | | |------+--------------------------| |---+-------|
Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert
but the intent is to support
ca-cert.pem server-cert.pem | | |------+--| |----------------------------+-------|
Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert
Or
ca-cert.pem server-cert.pem | | |------+-------------| |---------------+-------|
Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert
... meant to facilitate the above ....
the rational is that these splits reflect how some CA will give you your certs to begin with, and we want to allow them to be used directly.
My intent was to copy the QEMU change into libvirt once it was merged in QEMU, so from that POV I'm not a fan of making the some of the changes in this series.
Beyond that I'm also working post-quantum crypto support, which will require us to load multiple distinct server-cert-NNN.pem files, each with an independant set of certs, which are selected at runtime based on negotiated ciphers in the TLS handshake.
... in order to load certs with different (also the fancy new post-quantum) signature algorithms.
To best of my knowledge that will not work. IIUC when you load a bundle of certs into a session with gnutls_credentials_set that is assumed to be a cert chain by GNUTLS. To load multiple parallel certs, with different algorithms requires calling gnutls_credentials_set mutliple times, providing each distinct cert chain. Kind of annoying since it means every app using gnutls is guaranteed to need code changes to support PQC with parallel offerings of algorithms.
Since I didn't notice that the crypto policy in fedora 42 doesn't yet trust some of those (e.g. mldsa65), some of the certificates I've concatenated weren't actually passing the checks.
Yep, even in rawhide the PQC stuff isn't fully ready for gnutls. The only place I'm aware of which has new enough gnutls is CentOS Stream, and even then you need custom crypto-policies to enable it fully.
Thus if you're going to be fixing both of these I'll just push the cleanup patches.
Yep, go ahead and push the cleanup ones 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 :|
On Fri, Jul 18, 2025 at 09:24:05 +0100, Daniel P. Berrangé wrote:
On Fri, Jul 18, 2025 at 09:39:22AM +0200, Peter Krempa wrote:
On Thu, Jul 17, 2025 at 17:02:33 +0100, Daniel P. Berrangé wrote:
On Thu, Jul 17, 2025 at 05:28:09PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Similarly to how we iterate the list of CAs in the concatenated bundle there's a possibility of the server/client certificates to be concatenated as well.
If for some case the first certificate is okay but the further one have e.g. invalid signatures the validation code would not reject them but we'd encounter failures later when gnutls tries to use them.
Iterate also the client/server certs rather than just the CAs.
Was there some bug that motivated this change ?
Yes-ish. I've abused the fact that gnutls loads everything from the file ...
I'd like to keep libvirt's behaviour in sync with QEMU's behaviour to the greatest extent possible. I've just been finalizing changes to QEMU to cope with mutliple certs in the server-cert.pem file, but the semantics there are the certs are a chain of intermediate certs, all of which must be valid.
ie, currently we allow
ca-cert.pem server-cert.pem | | |------+--------------------------| |---+-------|
Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert
but the intent is to support
ca-cert.pem server-cert.pem | | |------+--| |----------------------------+-------|
Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert
Or
ca-cert.pem server-cert.pem | | |------+-------------| |---------------+-------|
Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert
... meant to facilitate the above ....
the rational is that these splits reflect how some CA will give you your certs to begin with, and we want to allow them to be used directly.
My intent was to copy the QEMU change into libvirt once it was merged in QEMU, so from that POV I'm not a fan of making the some of the changes in this series.
Beyond that I'm also working post-quantum crypto support, which will require us to load multiple distinct server-cert-NNN.pem files, each with an independant set of certs, which are selected at runtime based on negotiated ciphers in the TLS handshake.
... in order to load certs with different (also the fancy new post-quantum) signature algorithms.
To best of my knowledge that will not work. IIUC when you load a bundle of certs into a session with gnutls_credentials_set that is assumed to be a cert chain by GNUTLS.
Well with my test setup where I've: - created 2 CAs (one with RSA one with MLDSA sig) and concatenated them - created 4 server certs RSA+MLDSA sig both signed by each of the above CAs - created 4 client certs same as server certs. I've then concatenated all 4 server certs into one file and sequentially tested connecting with each of the 4 client certs. And (when allowing mldsa sigs for both CA and normal signing [1]) it worked (allowed connecting via TLS) for all of the 4 client certs. IIRC I've checked in one scenario that actually MLDSA sigs were used but didn't bother checking any further for any kind of security problems.
To load multiple parallel certs, with different algorithms requires calling gnutls_credentials_set mutliple times, providing each distinct cert chain.
Kind of annoying since it means every app using gnutls is guaranteed to need code changes to support PQC with parallel offerings of algorithms.
Since I didn't notice that the crypto policy in fedora 42 doesn't yet trust some of those (e.g. mldsa65), some of the certificates I've concatenated weren't actually passing the checks.
Yep, even in rawhide the PQC stuff isn't fully ready for gnutls.
[1] Yes this is exactly what tripped me up originally, because MLDSA is not allowed in the gnutls policy. When I've concatenated the certs virt-pki-validate tripped up only if the mldsa signed certs were first. If I put RSA certs first it passed but any of the combination using MLDSA didn't work.
The only place I'm aware of which has new enough gnutls is CentOS Stream, and even then you need custom crypto-policies to enable it fully.
Thus if you're going to be fixing both of these I'll just push the cleanup patches.
Yep, go ahead and push the cleanup ones
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 :|
On Fri, Jul 18, 2025 at 12:32:39PM +0200, Peter Krempa wrote:
On Fri, Jul 18, 2025 at 09:24:05 +0100, Daniel P. Berrangé wrote:
On Fri, Jul 18, 2025 at 09:39:22AM +0200, Peter Krempa wrote:
On Thu, Jul 17, 2025 at 17:02:33 +0100, Daniel P. Berrangé wrote:
On Thu, Jul 17, 2025 at 05:28:09PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa@redhat.com>
Similarly to how we iterate the list of CAs in the concatenated bundle there's a possibility of the server/client certificates to be concatenated as well.
If for some case the first certificate is okay but the further one have e.g. invalid signatures the validation code would not reject them but we'd encounter failures later when gnutls tries to use them.
Iterate also the client/server certs rather than just the CAs.
Was there some bug that motivated this change ?
Yes-ish. I've abused the fact that gnutls loads everything from the file ...
I'd like to keep libvirt's behaviour in sync with QEMU's behaviour to the greatest extent possible. I've just been finalizing changes to QEMU to cope with mutliple certs in the server-cert.pem file, but the semantics there are the certs are a chain of intermediate certs, all of which must be valid.
ie, currently we allow
ca-cert.pem server-cert.pem | | |------+--------------------------| |---+-------|
Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert
but the intent is to support
ca-cert.pem server-cert.pem | | |------+--| |----------------------------+-------|
Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert
Or
ca-cert.pem server-cert.pem | | |------+-------------| |---------------+-------|
Root CA -> Sub CA 1 -> Sub CA 2 -> Server Cert
... meant to facilitate the above ....
the rational is that these splits reflect how some CA will give you your certs to begin with, and we want to allow them to be used directly.
My intent was to copy the QEMU change into libvirt once it was merged in QEMU, so from that POV I'm not a fan of making the some of the changes in this series.
Beyond that I'm also working post-quantum crypto support, which will require us to load multiple distinct server-cert-NNN.pem files, each with an independant set of certs, which are selected at runtime based on negotiated ciphers in the TLS handshake.
... in order to load certs with different (also the fancy new post-quantum) signature algorithms.
To best of my knowledge that will not work. IIUC when you load a bundle of certs into a session with gnutls_credentials_set that is assumed to be a cert chain by GNUTLS.
Well with my test setup where I've:
- created 2 CAs (one with RSA one with MLDSA sig) and concatenated them - created 4 server certs RSA+MLDSA sig both signed by each of the above CAs - created 4 client certs same as server certs.
I've then concatenated all 4 server certs into one file and sequentially tested connecting with each of the 4 client certs.
Hmmm, I don't think that is validating things in the way you expect. The server certificate algorithm, combined with cipher priorities on client/server, control what ciphers are negotiated in the TLS handshake. The client cert is supplied after the handshake is dnoe and the server code merely validates it against the CA it holds, but this doesn't influence the ciphers in use for the session. IOW I suspect in this test scenario, you had exactly the same ciphers in use for every session. ie all 4 tests resulted in use of MLDSA, or all 4 tests used RSA, depending on which one was first in server-cert.pem. 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 :|
From: Peter Krempa <pkrempa@redhat.com> We now always load a list of certificates rather than only the first one so this function is not used any more. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/rpc/virnettlscert.c | 41 ----------------------------------------- src/rpc/virnettlscert.h | 3 --- 2 files changed, 44 deletions(-) diff --git a/src/rpc/virnettlscert.c b/src/rpc/virnettlscert.c index 2724f55bbe..5e036a4f2b 100644 --- a/src/rpc/virnettlscert.c +++ b/src/rpc/virnettlscert.c @@ -367,47 +367,6 @@ static int virNetTLSCertCheckPair(gnutls_x509_crt_t cert, } -gnutls_x509_crt_t virNetTLSCertLoadFromFile(const char *certFile, - bool isServer) -{ - gnutls_datum_t data; - gnutls_x509_crt_t cert = NULL; - g_autofree char *buf = NULL; - int ret = -1; - - VIR_DEBUG("isServer %d certFile %s", - isServer, certFile); - - if (gnutls_x509_crt_init(&cert) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, "%s", - _("Unable to initialize certificate")); - goto cleanup; - } - - if (virFileReadAll(certFile, (1<<16), &buf) < 0) - goto cleanup; - - data.data = (unsigned char *)buf; - data.size = strlen(buf); - - if (gnutls_x509_crt_import(cert, &data, GNUTLS_X509_FMT_PEM) < 0) { - virReportError(VIR_ERR_SYSTEM_ERROR, isServer ? - _("Unable to import server certificate %1$s") : - _("Unable to import client certificate %1$s"), - certFile); - goto cleanup; - } - - ret = 0; - - cleanup: - if (ret != 0) { - g_clear_pointer(&cert, gnutls_x509_crt_deinit); - } - return cert; -} - - int virNetTLSCertLoadListFromFile(const char *certFile, gnutls_x509_crt_t *certs, unsigned int certMax, diff --git a/src/rpc/virnettlscert.h b/src/rpc/virnettlscert.h index a2f591d172..aa0fe16a91 100644 --- a/src/rpc/virnettlscert.h +++ b/src/rpc/virnettlscert.h @@ -38,9 +38,6 @@ char *virNetTLSCertValidate(gnutls_x509_crt_t cert, const char *hostname, const char *const *x509dnACL); -gnutls_x509_crt_t virNetTLSCertLoadFromFile(const char *certFile, - bool isServer); - int virNetTLSCertLoadListFromFile(const char *certFile, gnutls_x509_crt_t *certs, unsigned int certMax, -- 2.50.0
participants (2)
-
Daniel P. Berrangé -
Peter Krempa