On Mon, Oct 17, 2016 at 11:24:58AM -0400, John Ferlan wrote: > > > 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@xxxxxxxxxx> > >>>> --- > >>>> 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." Don't forget this part of the same review: "There's no reason we can't have a tri-state TLS flag against the chardev in the XML too, to override the default behaviour of cfg->chardevTLS" That also means to override chardev_tls = "0" by "tls='yes'". Pavel > >> 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@xxxxxxxxxx > https://www.redhat.com/mailman/listinfo/libvir-list
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list