Hi Daniel,
I agree that this would allow QEMU startup with a broken TLS setup. Maybe the better solution is to only validate the chain of trust. Would a patch that does that be acceptable?
Kind regards, Henry
From:
Daniel P. Berrangé <berrange@redhat.com>
Date: Tuesday, 14 December 2021 at 13:16
To: Henry Kleynhans <henry.kleynhans@gmail.com>
Cc: qemu-devel@nongnu.org <qemu-devel@nongnu.org>, Henry Kleynhans <hkleynhans@fb.com>, henry.kleynhans@fb.com <henry.kleynhans@fb.com>
Subject: Re: [PATCH] Relax X509 CA cert sanity checking
On Tue, Dec 14, 2021 at 11:39:30AM +0000, Henry Kleynhans wrote:
> From: Henry Kleynhans <hkleynhans@fb.com>
>
> The sanity checking function attempts to validate all the certificates
> in the provided CA file. These checks are performed on certificates
> which may or may not be part of the signing chain and duplicates checks
> that should be performed by the TLS library.
>
> In real life this causes a problem if the certificate chain I want to
> use is valid, but there exist another expired certificate in the CA
> file.
>
> This patch relaxes the sanity checks to only ensure we have at least one
> valid certificate in the CA certificate file and leave the actual
> validation to the TLS library.
IIUC your scenario is that you have something like
Root CA -> Sub CA1 ---> Server Cert
\ \-> Client Cert
\---> Sub CA2
And the ca-cert.pem file given to QEMU contains all of 'Root CA',
'Sub CA1' and 'Sub CA2', despite 'Sub CA2' being irrelevant
from POV of QEMU's needs for the chain of trust.
Now 'Sub CA2' is expired so QEMU is rejecting the entire
'ca-cert.pem' at startup.
Your suggested change makes it so that we only validate that
one of these three certs is OK, so if 'Sub CA2' is expired
it dosn't block startup.
The trouble is that this equally ignores problems if 'Sub CA1'
is expired (or otherwise invalid), which is exactly the
scenario that we're aiming to detect.
TLS certificate config mistakes are an incredibly common
problem and the error reporting when this happens at time
of TLS session handshake is terrible. This leads to a
stream of support requests from users wondering why their
TLS sessions won't establish, which are very hard for us
to debug as maintainers. The validation made a significant
difference to this by giving users clearer error reports
upfront instead of letting QEMU startup with a broken
TLS cert setup.
Thus I'm loathe to relax the validation in the way proposed
here.
Is there a reason you aren't able to just set the
property 'validate=off' on the tls-creds-x509 object
to skip the validation in the case where you know your
CA bundle is valid overall, but contains broken certs ?
>
> Signed-off-by: Henry Kleynhans <hkleynhans@fb.com>
> ---
> crypto/tlscredsx509.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/crypto/tlscredsx509.c b/crypto/tlscredsx509.c
> index 32948a6bdc..fb056f96a2 100644
> --- a/crypto/tlscredsx509.c
> +++ b/crypto/tlscredsx509.c
> @@ -473,6 +473,7 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
> gnutls_x509_crt_t cert = NULL;
> gnutls_x509_crt_t cacerts[MAX_CERTS];
> size_t ncacerts = 0;
> + size_t nvalidca = 0;
> size_t i;
> int ret = -1;
>
> @@ -505,11 +506,15 @@ qcrypto_tls_creds_x509_sanity_check(QCryptoTLSCredsX509 *creds,
> for (i = 0; i < ncacerts; i++) {
> if (qcrypto_tls_creds_check_cert(creds,
> cacerts[i], cacertFile,
> - isServer, true, errp) < 0) {
> - goto cleanup;
> + isServer, true, errp) == 0) {
> + ++nvalidca;
> }
> }
>
> + if (nvalidca == 0) {
> + goto cleanup;
> + }
> +
> if (cert && ncacerts &&
> qcrypto_tls_creds_check_cert_pair(cert, certFile, cacerts,
> ncacerts, cacertFile,
> --
> 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 :|
|