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 <jfer...@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."
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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
