* [PATCH] crypto: allow client/server cert chains
@ 2023-02-13 18:00 matoro_mailinglist_qemu--- via
2023-02-20 15:44 ` Daniel P. Berrangé
0 siblings, 1 reply; 4+ messages in thread
From: matoro_mailinglist_qemu--- via @ 2023-02-13 18:00 UTC (permalink / raw)
To: qemu-devel; +Cc: berrange, matoro, matoro
From: matoro <matoro@users.noreply.github.com>
The existing implementation assumes that client/server certificates are
single individual certificates. If using publicly-issued certificates,
or internal CAs that use an intermediate issuer, this is unlikely to be
the case, and they will instead be certificate chains. While this can
be worked around by moving the intermediate certificates to the CA
certificate, which DOES currently support multiple certificates, this
instead allows the issued certificate chains to be used as-is, without
requiring the overhead of shuffling certificates around.
Corresponding libvirt change is available here:
https://gitlab.com/libvirt/libvirt/-/merge_requests/222
Signed-off-by: matoro <matoro_mailinglist_qemu@matoro.tk>
---
crypto/tlscredsx509.c | 130 +++++++++-----------------
tests/unit/test-crypto-tlscredsx509.c | 78 ++++++++++++++++
2 files changed, 122 insertions(+), 86 deletions(-)
diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
index d14313925d..5bb793e2c7 100644
--- a/crypto/tlscredsx509.c
+++ b/crypto/tlscredsx509.c
@@ -317,7 +317,8 @@ qcrypto_tls_creds_check_cert(QCryptoTLSCredsX509 *creds,
static int
-qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
+qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t *certs,
+ size_t ncerts,
const char *certFile,
gnutls_x509_crt_t *cacerts,
size_t ncacerts,
@@ -327,7 +328,7 @@ qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
{
unsigned int status;
- if (gnutls_x509_crt_list_verify(&cert, 1,
+ if (gnutls_x509_crt_list_verify(certs, ncerts,
cacerts, ncacerts,
NULL, 0,
0, &status) < 0) {
@@ -369,67 +370,15 @@ qcrypto_tls_creds_check_cert_pair(gnutls_x509_crt_t cert,
}
-static gnutls_x509_crt_t
-qcrypto_tls_creds_load_cert(QCryptoTLSCredsX509 *creds,
- const char *certFile,
- bool isServer,
- Error **errp)
-{
- gnutls_datum_t data;
- gnutls_x509_crt_t cert = NULL;
- g_autofree char *buf = NULL;
- gsize buflen;
- GError *gerr = NULL;
- int ret = -1;
- int err;
-
- trace_qcrypto_tls_creds_x509_load_cert(creds, isServer, certFile);
-
- err = gnutls_x509_crt_init(&cert);
- if (err < 0) {
- error_setg(errp, "Unable to initialize certificate: %s",
- gnutls_strerror(err));
- goto cleanup;
- }
-
- if (!g_file_get_contents(certFile, &buf, &buflen, &gerr)) {
- error_setg(errp, "Cannot load CA cert list %s: %s",
- certFile, gerr->message);
- g_error_free(gerr);
- goto cleanup;
- }
-
- data.data = (unsigned char *)buf;
- data.size = strlen(buf);
-
- err = gnutls_x509_crt_import(cert, &data, GNUTLS_X509_FMT_PEM);
- if (err < 0) {
- error_setg(errp, isServer ?
- "Unable to import server certificate %s: %s" :
- "Unable to import client certificate %s: %s",
- certFile,
- gnutls_strerror(err));
- goto cleanup;
- }
-
- ret = 0;
-
- cleanup:
- if (ret != 0) {
- gnutls_x509_crt_deinit(cert);
- cert = NULL;
- }
- return cert;
-}
-
-
static int
-qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds,
- const char *certFile,
- gnutls_x509_crt_t *certs,
- unsigned int certMax,
- size_t *ncerts,
- Error **errp)
+qcrypto_tls_creds_load_cert_list(QCryptoTLSCredsX509 *creds,
+ const char *certFile,
+ gnutls_x509_crt_t *certs,
+ unsigned int certMax,
+ size_t *ncerts,
+ bool isServer,
+ bool isCA,
+ Error **errp)
{
gnutls_datum_t data;
g_autofree char *buf = NULL;
@@ -450,9 +399,15 @@ qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds,
data.size = strlen(buf);
if (gnutls_x509_crt_list_import(certs, &certMax, &data,
- GNUTLS_X509_FMT_PEM, 0) < 0) {
+ GNUTLS_X509_FMT_PEM,
+ isCA ?
+ GNUTLS_X509_CRT_LIST_IMPORT_FAIL_IF_EXCEED :
+ GNUTLS_X509_CRT_LIST_IMPORT_FAIL_IF_EXCEED |
+ GNUTLS_X509_CRT_LIST_FAIL_IF_UNSORTED) < 0){
error_setg(errp,
- "Unable to import CA certificate list %s",
+ isCA ? "Unable to import CA certificate list %s" :
+ (isServer ? "Unable to import server certificate %s" :
+ "Unable to import client certificate %s"),
certFile);
return -1;
}
@@ -462,7 +417,7 @@ qcrypto_tls_creds_load_ca_cert_list(QCryptoTLSCredsX509 *creds,
}
-#define MAX_CERTS 16
+#define MAX_CERTS 200
static int
qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
bool isServer,
@@ -470,36 +425,39 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
const char *certFile,
Error **errp)
{
- gnutls_x509_crt_t cert = NULL;
+ gnutls_x509_crt_t certs[MAX_CERTS];
gnutls_x509_crt_t cacerts[MAX_CERTS];
- size_t ncacerts = 0;
+ size_t ncerts = 0, ncacerts = 0;
size_t i;
int ret = -1;
+ memset(certs, 0, sizeof(certs));
memset(cacerts, 0, sizeof(cacerts));
if (certFile &&
access(certFile, R_OK) == 0) {
- cert = qcrypto_tls_creds_load_cert(creds,
- certFile, isServer,
- errp);
- if (!cert) {
+ if (qcrypto_tls_creds_load_cert_list(creds,
+ certFile, certs,
+ MAX_CERTS, &ncerts,
+ isServer, false, errp) < 0) {
goto cleanup;
}
}
- if (access(cacertFile, R_OK) == 0) {
- if (qcrypto_tls_creds_load_ca_cert_list(creds,
- cacertFile, cacerts,
- MAX_CERTS, &ncacerts,
- errp) < 0) {
+ if (cacertFile &&
+ access(cacertFile, R_OK) == 0) {
+ if (qcrypto_tls_creds_load_cert_list(creds,
+ cacertFile, cacerts,
+ MAX_CERTS, &ncacerts,
+ isServer, true, errp) < 0) {
goto cleanup;
}
}
- if (cert &&
- qcrypto_tls_creds_check_cert(creds,
- cert, certFile, isServer,
- false, errp) < 0) {
- goto cleanup;
+ for (i = 0; i < ncerts; i++) {
+ if (qcrypto_tls_creds_check_cert(creds,
+ certs[i], certFile,
+ isServer, (i != 0), errp) < 0) {
+ goto cleanup;
+ }
}
for (i = 0; i < ncacerts; i++) {
@@ -510,9 +468,9 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
}
}
- if (cert && ncacerts &&
- qcrypto_tls_creds_check_cert_pair(cert, certFile, cacerts,
- ncacerts, cacertFile,
+ if (ncerts && ncacerts &&
+ qcrypto_tls_creds_check_cert_pair(certs, ncerts, certFile,
+ cacerts, ncacerts, cacertFile,
isServer, errp) < 0) {
goto cleanup;
}
@@ -520,8 +478,8 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
ret = 0;
cleanup:
- if (cert) {
- gnutls_x509_crt_deinit(cert);
+ for (i = 0; i < ncerts; i++) {
+ gnutls_x509_crt_deinit(certs[i]);
}
for (i = 0; i < ncacerts; i++) {
gnutls_x509_crt_deinit(cacerts[i]);
diff --git a/tests/unit/test-crypto-tlscredsx509.c b/tests/unit/test-crypto-tlscredsx509.c
index 3c25d75ca1..660335ce98 100644
--- a/tests/unit/test-crypto-tlscredsx509.c
+++ b/tests/unit/test-crypto-tlscredsx509.c
@@ -577,6 +577,12 @@ int main(int argc, char **argv)
true, true, GNUTLS_KEY_KEY_CERT_SIGN,
false, false, NULL, NULL,
0, 0);
+ TLS_ROOT_REQ(someotherrootreq,
+ "UK", "some other random CA", NULL, NULL, NULL, NULL,
+ true, true, true,
+ true, true, GNUTLS_KEY_KEY_CERT_SIGN,
+ false, false, NULL, NULL,
+ 0, 0);
TLS_CERT_REQ(cacertlevel1areq, cacertrootreq,
"UK", "qemu level 1a", NULL, NULL, NULL, NULL,
true, true, true,
@@ -617,6 +623,33 @@ int main(int argc, char **argv)
cacertlevel2areq.crt,
};
+ gnutls_x509_crt_t cabundle[] = {
+ someotherrootreq.crt,
+ cacertrootreq.crt,
+ };
+
+ gnutls_x509_crt_t servercertchain[] = {
+ servercertlevel3areq.crt,
+ cacertlevel2areq.crt,
+ cacertlevel1areq.crt,
+ };
+
+ gnutls_x509_crt_t servercertchain_incomplete[] = {
+ servercertlevel3areq.crt,
+ cacertlevel2areq.crt,
+ };
+
+ gnutls_x509_crt_t servercertchain_unsorted[] = {
+ servercertlevel3areq.crt,
+ cacertlevel1areq.crt,
+ cacertlevel2areq.crt,
+ };
+
+ gnutls_x509_crt_t clientcertchain[] = {
+ clientcertlevel2breq.crt,
+ cacertlevel1breq.crt,
+ };
+
test_tls_write_cert_chain(WORKDIR "cacertchain-ctx.pem",
certchain,
G_N_ELEMENTS(certchain));
@@ -628,6 +661,46 @@ int main(int argc, char **argv)
WORKDIR "cacertchain-ctx.pem",
clientcertlevel2breq.filename, false);
+ test_tls_write_cert_chain(WORKDIR "servercertchain-ctx.pem",
+ servercertchain,
+ G_N_ELEMENTS(servercertchain));
+
+ TLS_TEST_REG(serverchain, true,
+ cacertrootreq.filename,
+ WORKDIR "servercertchain-ctx.pem", false);
+
+ test_tls_write_cert_chain(WORKDIR "cabundle-ctx.pem",
+ cabundle,
+ G_N_ELEMENTS(cabundle));
+
+ TLS_TEST_REG(multiplecaswithchain, true,
+ WORKDIR "cabundle-ctx.pem",
+ WORKDIR "servercertchain-ctx.pem", false);
+
+ test_tls_write_cert_chain(WORKDIR "servercertchain_incomplete-ctx.pem",
+ servercertchain_incomplete,
+ G_N_ELEMENTS(servercertchain_incomplete));
+
+ TLS_TEST_REG(incompleteserverchain, true,
+ cacertrootreq.filename,
+ WORKDIR "servercertchain_incomplete-ctx.pem", true);
+
+ test_tls_write_cert_chain(WORKDIR "servercertchain_unsorted-ctx.pem",
+ servercertchain_unsorted,
+ G_N_ELEMENTS(servercertchain_unsorted));
+
+ TLS_TEST_REG(unsortedserverchain, true,
+ cacertrootreq.filename,
+ WORKDIR "servercertchain_unsorted-ctx.pem", true);
+
+ test_tls_write_cert_chain(WORKDIR "clientcertchain-ctx.pem",
+ clientcertchain,
+ G_N_ELEMENTS(clientcertchain));
+
+ TLS_TEST_REG(clientchain, false,
+ cacertrootreq.filename,
+ WORKDIR "clientcertchain-ctx.pem", false);
+
/* Some missing certs - first two are fatal, the last
* is ok
*/
@@ -697,7 +770,12 @@ int main(int argc, char **argv)
test_tls_discard_cert(&cacertlevel2areq);
test_tls_discard_cert(&servercertlevel3areq);
test_tls_discard_cert(&clientcertlevel2breq);
+ test_tls_discard_cert(&someotherrootreq);
unlink(WORKDIR "cacertchain-ctx.pem");
+ unlink(WORKDIR "servercertchain-ctx.pem");
+ unlink(WORKDIR "servercertchain_incomplete-ctx.pem");
+ unlink(WORKDIR "servercertchain_unsorted-ctx.pem");
+ unlink(WORKDIR "clientcertchain-ctx.pem");
test_tls_cleanup(KEYFILE);
rmdir(WORKDIR);
--
2.39.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] crypto: allow client/server cert chains
2023-02-13 18:00 [PATCH] crypto: allow client/server cert chains matoro_mailinglist_qemu--- via
@ 2023-02-20 15:44 ` Daniel P. Berrangé
2023-02-23 18:54 ` matoro via
0 siblings, 1 reply; 4+ messages in thread
From: Daniel P. Berrangé @ 2023-02-20 15:44 UTC (permalink / raw)
To: matoro_mailinglist_qemu; +Cc: qemu-devel
On Mon, Feb 13, 2023 at 01:00:49PM -0500, matoro_mailinglist_qemu--- via wrote:
> From: matoro <matoro@users.noreply.github.com>
>
> The existing implementation assumes that client/server certificates are
> single individual certificates. If using publicly-issued certificates,
> or internal CAs that use an intermediate issuer, this is unlikely to be
> the case, and they will instead be certificate chains. While this can
> be worked around by moving the intermediate certificates to the CA
> certificate, which DOES currently support multiple certificates, this
> instead allows the issued certificate chains to be used as-is, without
> requiring the overhead of shuffling certificates around.
>
> Corresponding libvirt change is available here:
> https://gitlab.com/libvirt/libvirt/-/merge_requests/222
>
> Signed-off-by: matoro <matoro_mailinglist_qemu@matoro.tk>
I'm afraid that because the Signed-off-by is intended as a legal
statement that you're permitted to contribute this change, we
require it to use the person's legal recognised real name (both
forename + surname), not a psuedo-name, nor merely a partial
name. Could you either resend this submission, or just reply
to this mail giving a new Signed-off-by.
The email address can be of your choosing, but should generally
be matched to the git commit authorship
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] crypto: allow client/server cert chains
2023-02-20 15:44 ` Daniel P. Berrangé
@ 2023-02-23 18:54 ` matoro via
2023-03-22 18:49 ` Daniel P. Berrangé
0 siblings, 1 reply; 4+ messages in thread
From: matoro via @ 2023-02-23 18:54 UTC (permalink / raw)
To: Daniel P. Berrangé; +Cc: qemu-devel
On 2023-02-20 10:44, Daniel P. Berrangé wrote:
> On Mon, Feb 13, 2023 at 01:00:49PM -0500, matoro_mailinglist_qemu---
> via wrote:
>> From: matoro <matoro@users.noreply.github.com>
>>
>> The existing implementation assumes that client/server certificates
>> are
>> single individual certificates. If using publicly-issued
>> certificates,
>> or internal CAs that use an intermediate issuer, this is unlikely to
>> be
>> the case, and they will instead be certificate chains. While this can
>> be worked around by moving the intermediate certificates to the CA
>> certificate, which DOES currently support multiple certificates, this
>> instead allows the issued certificate chains to be used as-is, without
>> requiring the overhead of shuffling certificates around.
>>
>> Corresponding libvirt change is available here:
>> https://gitlab.com/libvirt/libvirt/-/merge_requests/222
>>
>> Signed-off-by: matoro <matoro_mailinglist_qemu@matoro.tk>
>
> I'm afraid that because the Signed-off-by is intended as a legal
> statement that you're permitted to contribute this change, we
> require it to use the person's legal recognised real name (both
> forename + surname), not a psuedo-name, nor merely a partial
> name. Could you either resend this submission, or just reply
> to this mail giving a new Signed-off-by.
>
> The email address can be of your choosing, but should generally
> be matched to the git commit authorship
>
>
> With regards,
> Daniel
Hi Daniel, unfortunately I am unable to use my real name with
contributions due to my employment. Is there any way for me to release
copyright on this, or have someone else submit it on my behalf? (I have
done the latter with kernel contributions before)
If not I understand and will continue simply patching this for personal
use, and anybody else who needs this functionality can do the same.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] crypto: allow client/server cert chains
2023-02-23 18:54 ` matoro via
@ 2023-03-22 18:49 ` Daniel P. Berrangé
0 siblings, 0 replies; 4+ messages in thread
From: Daniel P. Berrangé @ 2023-03-22 18:49 UTC (permalink / raw)
To: matoro; +Cc: qemu-devel
On Thu, Feb 23, 2023 at 01:54:56PM -0500, matoro wrote:
> On 2023-02-20 10:44, Daniel P. Berrangé wrote:
> > On Mon, Feb 13, 2023 at 01:00:49PM -0500, matoro_mailinglist_qemu--- via
> > wrote:
> > > From: matoro <matoro@users.noreply.github.com>
> > >
> > > The existing implementation assumes that client/server certificates
> > > are
> > > single individual certificates. If using publicly-issued
> > > certificates,
> > > or internal CAs that use an intermediate issuer, this is unlikely to
> > > be
> > > the case, and they will instead be certificate chains. While this can
> > > be worked around by moving the intermediate certificates to the CA
> > > certificate, which DOES currently support multiple certificates, this
> > > instead allows the issued certificate chains to be used as-is, without
> > > requiring the overhead of shuffling certificates around.
> > >
> > > Corresponding libvirt change is available here:
> > > https://gitlab.com/libvirt/libvirt/-/merge_requests/222
> > >
> > > Signed-off-by: matoro <matoro_mailinglist_qemu@matoro.tk>
> >
> > I'm afraid that because the Signed-off-by is intended as a legal
> > statement that you're permitted to contribute this change, we
> > require it to use the person's legal recognised real name (both
> > forename + surname), not a psuedo-name, nor merely a partial
> > name. Could you either resend this submission, or just reply
> > to this mail giving a new Signed-off-by.
> >
> > The email address can be of your choosing, but should generally
> > be matched to the git commit authorship
>
> Hi Daniel, unfortunately I am unable to use my real name with contributions
> due to my employment. Is there any way for me to release copyright on this,
> or have someone else submit it on my behalf? (I have done the latter with
> kernel contributions before)
Sadly I can't see how to credibly release copyright while remaining
anonymous. In terms of someone submitting it on your behalf, that
depends on the person's willingly to add their own Signed-off-by.
This could be viable for changes which can be considered trivial,
and thus effectively uncopyrightable, but I don't think that applies
here. An alternative would be if some other contributor knows you
well enough to be confident in adding their SoB.
If not, I'll keep this RFE on my list of TODO items to write a
new patch myself.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-03-22 18:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-13 18:00 [PATCH] crypto: allow client/server cert chains matoro_mailinglist_qemu--- via
2023-02-20 15:44 ` Daniel P. Berrangé
2023-02-23 18:54 ` matoro via
2023-03-22 18:49 ` Daniel P. Berrangé
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).