9:37 a.m.
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>
>> ---
>> 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.
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.
Pavel