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

[John Ferlan]

On 06/29/2017 03:24 AM, Jiri Denemark wrote:
> On Wed, Jun 28, 2017 at 15:30:28 -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. Then if a setting was provided, validating the existence of
>> the directory and overwriting the default set by virQEMUDriverConfigNew.
>>
>> Also update the qemu.conf description for default to indicate the consequences
>> if the default directory does not exist.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  src/qemu/qemu.conf   |  9 ++++++++-
>>  src/qemu/qemu_conf.c | 42 ++++++++++++++++++++++++++++++++++--------
>>  2 files changed, 42 insertions(+), 9 deletions(-)
>>
>> diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf
>> index e6c0832..737fa46 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:
>>  #
>>  #  ca-cert.pem - the CA master certificate
>>  #  server-cert.pem - the server certificate signed with ca-cert.pem
>> @@ -13,6 +13,13 @@
>>  #
>>  #  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.
>> +#
>> +# In order to overwrite the default directory alter the following. If the
>> +# provided directory does not exist, then the setting reverts back to the
>> +# default /etc/pki/qemu.
>> +#
> 
> I don't think this is a good idea. We should use the directory a user
> specified in qemu.conf. If it doesn't exist, well things won't work.
> Sure, we can complain about it in the logs, but we should not fallback
> to any magic default in that case. Anyone setting a custom directory for
> TLS certificates does this because they want to use it. If the directory
> does not exist, it's either because they forgot to create it or they
> made a typo somewhere. It's very unlikely someone actually wants to use
> a default directory even though they set a custom one.
> 
> NACK
> 
> Jirka
> 

It's simple enough to fail, but that's in code which you snipped and not
in text description which essentially matched what the code is/was
doing. Your feeling is then that :

+    if (!virFileExists(tlsCertDir)) {
+        VIR_INFO("%s, directory '%s' does not exist, retain default",
+                 setting, tlsCertDir);
+        VIR_FREE(tlsCertDir);

should fail at libvirtd startup instead of a VIR_INFO, true? Is that
true for default, vnc, spice, chardev, and migrate?

That's different from the current environment which only would fail for
domains that use TLS which is why I was hesitant to make it fail. Also
note that if /etc/pki/qemu doesn't exist and someone used a TLS marker,
then even without changing any of the *cert_dir values, only the domain
will fail to start. That is, /etc/pki/qemu is not checked for existence
and startup failure.

Altering text in qemu.conf to match what really happens will be simple,
but ensuring the approach is agreed upon is what I need to clear up.

Tks -

John