[libvirt] [PATCH v2] qemu: Check for existence of provided *_tls_x509_cert_dir

[Ján Tomko]

On Thu, Jun 29, 2017 at 10:32:35AM -0400, John Ferlan wrote:
>https://bugzilla.redhat.com/show_bug.cgi?id=1458630
>
>Introduce virQEMUDriverConfigSetCertDir which will handle reading the
>qemu.conf config file specific setting for default, vnc, spice, chardev,
>and migrate. If a setting is provided, then validate the existence of the
>directory and overwrite the default set by virQEMUDriverConfigNew.
>
>Update the qemu.conf description for default to describe the consequences
>if the default directory path does not exist and as well as the descriptions
>for each of the *_tls_x509_cert_dir entries.
>
>Signed-off-by: John Ferlan <jferlan at redhat.com>
>---
>
>v1: https://www.redhat.com/archives/libvir-list/2017-June/msg01278.html
>
>- Dropped the former 1/2 patch
>
>- Alter the logic of virQEMUDriverConfigSetCertDir to fail instead of
>  VIR_INFO if an uncommented entry for one of the *_tls_x509_cert_dir
>  has a path that does not exist. This will cause a libvirtd startup
>  failure as opposed to the previous logic which would have failed only
>  when a domain using TLS was started.
>
>- Alter the description for each of the values to more accurately describe
>  what happens.
>
> src/qemu/qemu.conf   | 29 ++++++++++++++++++++---------
> src/qemu/qemu_conf.c | 38 +++++++++++++++++++++++++++++++++-----
> 2 files changed, 53 insertions(+), 14 deletions(-)
>
>diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
>index e6c0832..b0ccffb 100644
>--- a/src/qemu/qemu.conf
>+++ b/src/qemu/qemu.conf
>@@ -3,7 +3,7 @@
> # defaults are used.
>
> # Use of TLS requires that x509 certificates be issued. The default is
>-# to keep them in /etc/pki/qemu. This directory must contain
>+# to keep them in /etc/pki/qemu. This directory must exist and contain:

I suspect the POSIX specification mandates that the directory must exist
in order to contain files. :)

> #
> #  ca-cert.pem - the CA master certificate
> #  server-cert.pem - the server certificate signed with ca-cert.pem
>@@ -13,6 +13,12 @@
> #
> #  dh-params.pem - the DH params configuration file
> #
>+# If the directory does not exist or does not contain the necessary files,
>+# QEMU domains will fail to start if they are configured to use TLS.
>+#

Isn't this sufficiently covered by 'Use of TLS requires that x509
certificates be issued'?

>+# In order to overwrite the default path alter the following. If the provided
>+# path does not exist, then startup will fail.
>+#

To apply the configuration, you need to restart the daemon. And since
daemon startup will fail, I think the user will be able to notice it.
We should error out on incorrect paths as soon as we can, without
mentioning it in the documentation.

> #default_tls_x509_cert_dir = "/etc/pki/qemu"
>
>
>@@ -79,8 +85,9 @@
>
> # In order to override the default TLS certificate location for
> # vnc certificates, supply a valid path to the certificate directory.
>-# If the provided path does not exist then the default_tls_x509_cert_dir
>-# path will be used.
>+# If the default listed here does not exist, then the default /etc/pki/qemu
>+# is used.

If I override default_tls_x509_cert_dir, without overriding all the
specific *_tls_x509_cert_dir values, I expect they will all use my
value, not the hardcoded default of /etc/pki/qemu.
So the behavior described by the original comment makes more sense.

> If uncommented and the provided path does not exist, then startup
>+# will fail.
> #
> #vnc_tls_x509_cert_dir = "/etc/pki/libvirt-vnc"
>
>diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c
>index 73c33d6..4eb6f0c 100644
>--- a/src/qemu/qemu_conf.c
>+++ b/src/qemu/qemu_conf.c
>@@ -440,6 +440,34 @@ virQEMUDriverConfigHugeTLBFSInit(virHugeTLBFSPtr hugetlbfs,
> }
>
>
>+static int
>+virQEMUDriverConfigSetCertDir(virConfPtr conf,
>+                              const char *setting,
>+                              char **value)
>+{
>+    char *tlsCertDir = NULL;
>+
>+    if (virConfGetValueString(conf, setting, &tlsCertDir) < 0)
>+        return -1;
>+
>+    if (!tlsCertDir)
>+        return 0;
>+
>+    if (!virFileExists(tlsCertDir)) {
>+        virReportError(VIR_ERR_CONF_SYNTAX,

This is not a syntax error.
Validating these settings does not belong in virQEMUDriverConfigLoadFile;
qemuStateInitialize or a function called from it would be a better
place.

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170719/71d01bed/attachment-0001.sig>