[libvirt] [PATCH v9 1/5] domain: Add optional 'tls' attribute for TCP chardev

[John Ferlan]

On 10/17/2016 10:37 AM, Pavel Hrdina wrote:
> On Mon, Oct 17, 2016 at 09:54:46AM -0400, John Ferlan wrote:
>>
>>
>> On 10/17/2016 04:09 AM, Pavel Hrdina wrote:
>>> On Fri, Oct 14, 2016 at 04:23:04PM -0400, John Ferlan wrote:
>>>> Add an optional "tls='yes|no'" attribute for a TCP chardev for the
>>>> express purpose to disable setting up TLS for the specific chardev in
>>>> the event the qemu.conf settings have enabled hypervisor wide TLS for
>>>> serial TCP chardevs.
>>>>
>>>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>>>> ---
>>>>  docs/formatdomain.html.in                          | 21 +++++++++
>>>>  docs/schemas/domaincommon.rng                      |  5 +++
>>>>  src/conf/domain_conf.c                             | 22 +++++++++-
>>>>  src/conf/domain_conf.h                             |  1 +
>>>>  src/qemu/qemu_command.c                            |  3 +-
>>>>  src/qemu/qemu_hotplug.c                            |  3 +-
>>>>  ...uxml2argv-serial-tcp-tlsx509-chardev-notls.args | 30 +++++++++++++
>>>>  ...muxml2argv-serial-tcp-tlsx509-chardev-notls.xml | 50 ++++++++++++++++++++++
>>>>  .../qemuxml2argv-serial-tcp-tlsx509-chardev.xml    |  2 +-
>>>>  tests/qemuxml2argvtest.c                           |  3 ++
>>>>  ...xml2xmlout-serial-tcp-tlsx509-chardev-notls.xml |  1 +
>>>>  .../qemuxml2xmlout-serial-tcp-tlsx509-chardev.xml  |  2 +-
>>>>  tests/qemuxml2xmltest.c                            |  1 +
>>>>  13 files changed, 139 insertions(+), 5 deletions(-)
>>>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.args
>>>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-notls.xml
>>>>  create mode 120000 tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-notls.xml
>>>>
>>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>>> index 9051178..6145d65 100644
>>>> --- a/docs/formatdomain.html.in
>>>> +++ b/docs/formatdomain.html.in
>>>> @@ -6204,6 +6204,27 @@ qemu-kvm -net nic,model=? /dev/null
>>>>    </devices>
>>>>    ...</pre>
>>>>  
>>>> +    <p>
>>>> +      <span class="since">Since 2.4.0,</span> the optional attribute
>>>> +      <code>tls</code> can be used to control whether a serial chardev
>>>> +      TCP communication channel would utilize a hypervisor configured
>>>> +      TLS X.509 certificate environment in order to encrypt the data
>>>> +      channel. For the QEMU hypervisor usage of TLS is controlled by the
>>>> +      <code>chardev_tls</code> setting in file /etc/libvirt/qemu.conf. If
>>>> +      enabled, then by setting this attribute to "no" will disable usage
>>>> +      of the TLS environment for this particular serial TCP data channel.
>>>> +    </p>
>>>
>>> Previous discussion:
>>>
>>> http://www.redhat.com/archives/libvir-list/2016-October/msg00734.html
>>>
>>> So now there is no functional issue if we agree that this design is what we
>>> want.  I personally thing that there could be also use-case where you want to
>>> configure certificates in qemu.conf and use 'tls' attribute to explicitly
>>> enable TLS encryption only for some VMs and only for some chardev and this is
>>> not currently possible whit this design.  Now user can enable the TLS encryption
>>> for every chardev for every domain and explicitly disable for some chardevs.
>>>
>>> This would require to add all the extra code from that discussion to handle
>>> migration properly and that's why I've started the discussion.
>>>
>>
>> w/r/t: design - re-read what I pulled from Dan's v5 review:
>>
>> http://www.redhat.com/archives/libvir-list/2016-October/msg00698.html
>>
>> While forcing to set "tls='yes'" is certainly a mechanism that could be
>> used and adding extra code is possible, is that something that we want
>> to require of everyone that's using TLS chardev's? If you go through the
>> trouble of configuring for your host, then how would you feel about
>> having to update all your domains (for those that have more than 1 or 2)
>> to set the property in order to be able to use it? Again, I really don't
> 
> I have a feeling that we are having trouble to understand each other.  I'm not
> saying that you will have to set "tls='yes'" for every domain and every chardev.
> Forcing to set "tls='no'" for every domain and every chardevs if you want to use
> TLS only for one domain is not a good mechanism.

Clearly ;-)  Setting "tls='yes'" wasn't my implication either. The way
this patch is written 'YES' or 'ABSENT' have the same meaning to take
the host default.

So "forcing" setting of "tls='no'"
> 
> What I would like to achieve is this:
> 
> 1. Currently there is already existing "chardev_tls" config option that allows
> you to enable/disable TLS for all domain and all chardevs
> 
> 2. This patch improves the current functionality by adding an option to
> explicitly disable TLS for some chardves by using "tls='no'" if "chardev_tls"
> is set to "1".
> 
> 3. My additional proposal is to add another improvement to current functionality
> by allowing to explicitly enable TLS for some chardevs by using "tls='yes'" if
> "chardev_tls" is set to "0".  And I can see the use-case, you have a shared host
> between several people and only one of them would like to use TLS encryption for
> his domain and not affect other users so he will configure certificates for TLS
> encryption and use it explicitly only for his domain.
> 

Feel free to write a competing patch... This patch as written I believe
more closely follows the request from Daniel from patch 5 review:

"As default behaviour I think it is desirable that we can turn TLS on
for every VM at once - I tend to view it as a host network integration
task, rather than a VM configuration task. Same rationale that we use
for TLS wth VNC/SPICE."

>> think it's a good idea to be mucking around with changing XML of running
>> domains, adding extra checks in the migration code, altering the
>> format/parse code to check flags, and/or anything else that may come up.
>> The less we do that the better - introduces less risk for making or
>> missing assumptions.
> 
> This is not a question whether it's a good idea or not.  We simply have to do
> that in order to generate backward compatible XML.  Yes, it adds extra code and
> extra checks but it's a price we have to pay to maintain backward compatibility.
> 

But unnecessary when using "tls='no'"

>> For comparison spice uses an attribute mechanism "tlsPort={-1|0|port}"
>> in order to use TLS, but it also checks cfg->spiceTLS. Alternatively,
>> vnc will just take the cfg->vncTLS to decide whether to use or not
>> (e.g., there's no way to avoid).
> 
> Yes, SPICE uses tlsPort and spiceTLS config option, but the spice is different
> because the tlsPort is separate and there is still non encrypted port.  The VNC
> is not a good example because it has only the vncTLS config option and there is
> not attribute in XML to configure it (the current state for chardev before this
> patch changes it).
> 
>> So one of the existing mechanisms forces you to define something to use
>> TLS configured for the host and the other doesn't. Spice has the
>> additional configuration option to determine specific channels by name
>> that would use the secure port.
>>
>> This patch can still be considered "outside" of the subsequent 4 in this
>> series...
> 
> Yes it could be a separate patch outside of this series but I would rather see
> it as part of this series.
> 

Other than the pointed out spots that need a haveTLS in patch 4 and 5,
the other patches are different.

John