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

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(a)redhat.com&gt; >>> --- >>> 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 >>> &lt;/devices&gt; >>> ...</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.

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.

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

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