6:28 p.m.
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
6:28 p.m.
New subject: [PATCH 1/7] rpc: virnettlscontext: Fix formatting of function definitions
From: Peter Krempa <pkrempa(a)redhat.com>
Signed-off-by: Peter Krempa <pkrempa(a)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
7:03 p.m.
New subject: [PATCH 1/7] rpc: virnettlscontext: Fix formatting of function
definitions
On Thu, Jul 17, 2025 at 05:28:04PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa(a)redhat.com>
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/rpc/virnettlscontext.c | 74 +++++++++++++++++++-------------------
1 file changed, 37 insertions(+), 37 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)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 :|
6:28 p.m.
New subject: [PATCH 2/7] virNetTLSContextNewPath: Refactor temporary variable usage
From: Peter Krempa <pkrempa(a)redhat.com>
Use autofree for all temporary variables and return the result directly.
Signed-off-by: Peter Krempa <pkrempa(a)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
7:03 p.m.
New subject: [PATCH 2/7] virNetTLSContextNewPath: Refactor temporary variable
usage
On Thu, Jul 17, 2025 at 05:28:05PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa(a)redhat.com>
Use autofree for all temporary variables and return the result directly.
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/rpc/virnettlscontext.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)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 :|
6:28 p.m.
New subject: [PATCH 3/7] virNetTLSCertCheckPair: Fix function definition formatting
From: Peter Krempa <pkrempa(a)redhat.com>
Signed-off-by: Peter Krempa <pkrempa(a)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
7:03 p.m.
New subject: [PATCH 3/7] virNetTLSCertCheckPair: Fix function definition
formatting
On Thu, Jul 17, 2025 at 05:28:06PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa(a)redhat.com>
Signed-off-by: Peter Krempa <pkrempa(a)redhat.com>
---
src/rpc/virnettlscert.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange(a)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 :|
6:28 p.m.
New subject: [PATCH 4/7] rpc: virnettlscert: Rename virNetTLSCertLoadCAListFromFile to virNetTLSCertLoadListFromFile
From: Peter Krempa <pkrempa(a)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(a)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
7:04 p.m.
New subject: [PATCH 4/7] rpc: virnettlscert: Rename
virNetTLSCertLoadCAListFromFile to virNetTLSCertLoadListFromFile
On Thu, Jul 17, 2025 at 05:28:07PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa(a)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(a)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(a)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 :|
6:28 p.m.
New subject: [PATCH 5/7] virPKIValidateIdentity: Validate all concatenated certificates
From: Peter Krempa <pkrempa(a)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(a)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
6:28 p.m.
New subject: [PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated certs
From: Peter Krempa <pkrempa(a)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(a)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
7:02 p.m.
New subject: [PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated
certs
On Thu, Jul 17, 2025 at 05:28:09PM +0200, Peter Krempa via Devel wrote:
From: Peter Krempa <pkrempa(a)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(a)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 :|
10:39 a.m.
New subject: [PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated
certs
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(a)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(a)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.
11:24 a.m.
New subject: [PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated
certs
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(a)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 :|
1:32 p.m.
New subject: [PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated
certs
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(a)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 :|
2:38 p.m.
New subject: [PATCH 6/7] virNetTLSCertSanityCheck: Validate all concatenated
certs
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(a)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 :|
6:28 p.m.
New subject: [PATCH 7/7] Remove unused 'virNetTLSCertLoadFromFile'
From: Peter Krempa <pkrempa(a)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(a)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
0
days inactive
1
days old
16 comments
2 participants
participants (2)
-
Daniel P. Berrangé -
Peter Krempa