* [PATCH 1/2] [crypto] Load all certificates in X509 CA file
@ 2021-12-22 15:05 Henry Kleynhans
2021-12-22 15:06 ` [PATCH 2/2] [crypto] Only verify CA certs in chain of trust Henry Kleynhans
2022-01-04 18:30 ` [PATCH 1/2] [crypto] Load all certificates in X509 CA file Daniel P. Berrangé
0 siblings, 2 replies; 6+ messages in thread
From: Henry Kleynhans @ 2021-12-22 15:05 UTC (permalink / raw)
To: qemu-devel; +Cc: Henry Kleynhans, berrange, 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.
Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
---
crypto/tlscredsx509.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index 32948a6bdc..d061c68253 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -426,9 +426,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;
@@ -449,14 +448,13 @@ 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,
+ 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;
}
@@ -471,12 +469,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,
@@ -488,8 +485,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;
}
@@ -526,6 +524,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.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] [crypto] Only verify CA certs in chain of trust
2021-12-22 15:05 [PATCH 1/2] [crypto] Load all certificates in X509 CA file Henry Kleynhans
@ 2021-12-22 15:06 ` Henry Kleynhans
2021-12-22 15:54 ` Henry Kleynhans
2022-01-04 18:44 ` Daniel P. Berrangé
2022-01-04 18:30 ` [PATCH 1/2] [crypto] Load all certificates in X509 CA file Daniel P. Berrangé
1 sibling, 2 replies; 6+ messages in thread
From: Henry Kleynhans @ 2021-12-22 15:06 UTC (permalink / raw)
To: qemu-devel; +Cc: Henry Kleynhans, berrange, 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.
Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
---
crypto/tlscredsx509.c | 50 +++++++++++++++++++++++----
tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++++-
2 files changed, 68 insertions(+), 7 deletions(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index d061c68253..841f80aac6 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -315,6 +315,44 @@ 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,
@@ -500,12 +538,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 aab4149b56..e4d657ba61 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.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] [crypto] Only verify CA certs in chain of trust
2021-12-22 15:06 ` [PATCH 2/2] [crypto] Only verify CA certs in chain of trust Henry Kleynhans
@ 2021-12-22 15:54 ` Henry Kleynhans
2022-01-04 18:42 ` Daniel P. Berrangé
2022-01-04 18:44 ` Daniel P. Berrangé
1 sibling, 1 reply; 6+ messages in thread
From: Henry Kleynhans @ 2021-12-22 15:54 UTC (permalink / raw)
To: Henry Kleynhans, qemu-devel; +Cc: berrange, henry.kleynhans
[-- Attachment #1: Type: text/plain, Size: 6928 bytes --]
Hi Daniel,
This patch tightens the CA verification code to only check the issuer chain of the client cert. I think this will still not catch expired/invalid certs if the client and server certs have different issuer chains; so maybe this too is not quite the correct fix. Let me know what you think.
Kind regards, Henry
From: Henry Kleynhans <henry.kleynhans@gmail.com>
Date: Wednesday, 22 December 2021 at 15:06
To: qemu-devel@nongnu.org <qemu-devel@nongnu.org>
Cc: berrange@redhat.com <berrange@redhat.com>, henry.kleynhans@fb.com <henry.kleynhans@fb.com>, Henry Kleynhans <hkleynhans@fb.com>
Subject: [PATCH 2/2] [crypto] Only verify CA certs in chain of trust
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.
Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
---
crypto/tlscredsx509.c | 50 +++++++++++++++++++++++----
tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++++-
2 files changed, 68 insertions(+), 7 deletions(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index d061c68253..841f80aac6 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -315,6 +315,44 @@ 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,
@@ -500,12 +538,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 aab4149b56..e4d657ba61 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.34.1
[-- Attachment #2: Type: text/html, Size: 16903 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] [crypto] Load all certificates in X509 CA file
2021-12-22 15:05 [PATCH 1/2] [crypto] Load all certificates in X509 CA file Henry Kleynhans
2021-12-22 15:06 ` [PATCH 2/2] [crypto] Only verify CA certs in chain of trust Henry Kleynhans
@ 2022-01-04 18:30 ` Daniel P. Berrangé
1 sibling, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2022-01-04 18:30 UTC (permalink / raw)
To: Henry Kleynhans; +Cc: Henry Kleynhans, qemu-devel, henry.kleynhans
On Wed, Dec 22, 2021 at 03:05:59PM +0000, Henry Kleynhans wrote:
> 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.
>
> Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
> ---
> crypto/tlscredsx509.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
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] 6+ messages in thread
* Re: [PATCH 2/2] [crypto] Only verify CA certs in chain of trust
2021-12-22 15:54 ` Henry Kleynhans
@ 2022-01-04 18:42 ` Daniel P. Berrangé
0 siblings, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2022-01-04 18:42 UTC (permalink / raw)
To: Henry Kleynhans; +Cc: Henry Kleynhans, henry.kleynhans, qemu-devel
On Wed, Dec 22, 2021 at 03:54:08PM +0000, Henry Kleynhans wrote:
> Hi Daniel,
>
> This patch tightens the CA verification code to only check the
> issuer chain of the client cert. I think this will still not
> catch expired/invalid certs if the client and server certs have
> different issuer chains; so maybe this too is not quite the
> correct fix. Let me know what you think.
Different issuer chains is not going to be very common/typical.
So what you've done in this patch is at least pretty decent for
the common case, so will catch most user's mistakes. Let me have
a think about whether we can do anything better without making
the code too painful
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] 6+ messages in thread
* Re: [PATCH 2/2] [crypto] Only verify CA certs in chain of trust
2021-12-22 15:06 ` [PATCH 2/2] [crypto] Only verify CA certs in chain of trust Henry Kleynhans
2021-12-22 15:54 ` Henry Kleynhans
@ 2022-01-04 18:44 ` Daniel P. Berrangé
1 sibling, 0 replies; 6+ messages in thread
From: Daniel P. Berrangé @ 2022-01-04 18:44 UTC (permalink / raw)
To: Henry Kleynhans; +Cc: Henry Kleynhans, qemu-devel, henry.kleynhans
On Wed, Dec 22, 2021 at 03:06:00PM +0000, Henry Kleynhans wrote:
> From: Henry Kleynhans <hkleynhans@fb.com>
>
> The CA file provided to qemu may contain CA certificates which do not
> form part of the chain of trust for the specific certificate we are
> sanity checking.
>
> This patch changes the sanity checking from validating every CA
> certificate to only checking the CA certificates which are part of the
> chain of trust (issuer chain). Other certificates are ignored.
>
> Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
> ---
> crypto/tlscredsx509.c | 50 +++++++++++++++++++++++----
> tests/unit/test-crypto-tlscredsx509.c | 25 +++++++++++++-
> 2 files changed, 68 insertions(+), 7 deletions(-)
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
>
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index d061c68253..841f80aac6 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -315,6 +315,44 @@ 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;
I have a feeling this should be 'return 0'. eg consider the case
where the cacert.pem does not contain the issuer of clientcert.pem.
we will only do one iteration of the while(checking_issuer) loop,
not hitting any of the 'return' statements. This is ok, so should
report success I think.
> +}
>
> static int
> qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
> @@ -500,12 +538,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 aab4149b56..e4d657ba61 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.34.1
>
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] 6+ messages in thread
end of thread, other threads:[~2022-01-04 18:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-12-22 15:05 [PATCH 1/2] [crypto] Load all certificates in X509 CA file Henry Kleynhans
2021-12-22 15:06 ` [PATCH 2/2] [crypto] Only verify CA certs in chain of trust Henry Kleynhans
2021-12-22 15:54 ` Henry Kleynhans
2022-01-04 18:42 ` Daniel P. Berrangé
2022-01-04 18:44 ` Daniel P. Berrangé
2022-01-04 18:30 ` [PATCH 1/2] [crypto] Load all certificates in X509 CA file Daniel P. Berrangé
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.