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

[Daniel P. Berrange]

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 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
> > 
> > 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
-- 
|: 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 :|