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

On Thu, Jun 29, 2017 at 07:45:06AM -0400, John Ferlan wrote: > > > On 06/29/2017 06:40 AM, Daniel P. Berrange wrote: >> On Thu, Jun 29, 2017 at 09:24:30AM +0200, 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(a)redhat.com&gt; >>>> --- >>>> 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 >> >> Agreed, I think we need to distinguish between the default dirs for each >> settings, vs user specified dir for each setting. >> >> ie, if the user has *not* set 'chardev_tls_x509_cert_dir' then its default >> value is '/etc/pki/libvirt-chardev'. If that directory does not exist, >> then falling back to "default_tls_x509_cert_dir" is good. > > This essentially what happens in virQEMUDriverConfigNew. The caveat here > is that the default value for default_tls_x509_cert_dir (e.g. > /etc/pki/qemu) is not checked for existence, rather it's assumed. As long as we get an error message it is ok, but it might be wise to explicitly check /etc/pki/qemu if it would give a better error message to the user.

>> If the user *has* set 'chardev_tls_x509_cert_dir' and it does nto exist, >> then we should report an hard error, preferrably at startup so the admin >> sees their mistake immediately. > > OK and that answers most of the questions I had... If the user changes > "default_*" (as processed in virQEMUDriverConfigLoadFile) and it does > not exist, then should startup fail? Yep Regards, Daniel