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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

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

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]