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

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(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. 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 -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list