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

[John Ferlan]

On 06/29/2017 07:49 AM, Daniel P. Berrange wrote:
> 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.
> 

Currently we'd get an error "eventually" when someone tries to start a
guest:

error: internal error: process exited while connecting to monitor:
2017-06-28T19:24:18.810938Z qemu-system-x86_64: -object
tls-creds-x509,id=objcharserial1_tls0,dir=/etc/pki/qemu,endpoint=client,verify-peer=no:
Unable to access credentials /etc/pki/qemu/ca-cert.pem: No such file or
directory

The problem with checking and generating an error at startup is that we
don't create the /etc/pki/qemu directory during install (at least as far
as I can tell). If we do, then something's broken because I have a
couple of environments without it.

John

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