On 10/14/2016 09:49 AM, Pavel Hrdina wrote:
> On Fri, Oct 14, 2016 at 09:32:45AM -0400, John Ferlan wrote:
>>
>>
>> On 10/14/2016 09:18 AM, Pavel Hrdina wrote:
>>> On Fri, Oct 07, 2016 at 07:00:26AM -0400, John Ferlan wrote:
>>>> Add an optional "tls='yes'" option for a TCP chardev for the
>>>> express purpose to enable setting up TLS for the chardev. This
>>>> will assume that the qemu.conf settings have been adjusted as
>>>> well as the environment configured properly.
>>>>
>>>> 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 +-
>>>> ...emuxml2argv-serial-tcp-tlsx509-chardev-tls.args | 30 +++++++++++++
>>>> ...qemuxml2argv-serial-tcp-tlsx509-chardev-tls.xml | 50
>>>> ++++++++++++++++++++++
>>>> .../qemuxml2argv-serial-tcp-tlsx509-chardev.xml | 2 +-
>>>> tests/qemuxml2argvtest.c | 3 ++
>>>> ...muxml2xmlout-serial-tcp-tlsx509-chardev-tls.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-tls.args
>>>> create mode 100644
>>>> tests/qemuxml2argvdata/qemuxml2argv-serial-tcp-tlsx509-chardev-tls.xml
>>>> create mode 120000
>>>> tests/qemuxml2xmloutdata/qemuxml2xmlout-serial-tcp-tlsx509-chardev-tls.xml
>>>>
>>>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>>>> index 1266e9d..010530e 100644
>>>> --- a/docs/formatdomain.html.in
>>>> +++ b/docs/formatdomain.html.in
>>>> @@ -6203,6 +6203,27 @@ qemu-kvm -net nic,model=? /dev/null
>>>> </devices>
>>>> ...</pre>
>>>>
>>>> + <p>
>>>> + <span class="since">Since 2.4.0,</span> some hypervisors support
>>>> using
>>>> + a TLS X.509 certificate environment in order to encrypt all serial
>>>> TCP
>>>> + connections via a hypervisor configuration option. In order to
>>>> enable
>>>> + TLS for the domain an optional attribute <code>tls</code> can be
>>>> set to
>>>> + "yes". Combined with the hypervisor's capability to utilize the TLS
>>>> + environment allows for the character device to use the encrypted
>>>> + communication. If the attribute is not present, then the default
>>>> + setting is "no".
>>>
>>> This is not correct in case of QEMU. Before this patch the default was
>>> based on
>>> chardev_tls from qemu.conf. After this patch the default will be "no" even
>>> if
>>> you set chardev_tls=1 and that breaks behavior of libvirt. The test case
>>> "serial-tcp-tlsx509-chardev-tls" where you had to add tls="yes" should also
>>> work
>>> without this change.
>>>
>>> This patch needs to be modified to take chardev_tls=1 in account and add
>>> tests
>>> to make sure that we don't break it in the future.
>>>
>>
>> This change was in response to v5 (and v6) review comments from Daniel :
>>
>> v5: http://www.redhat.com/archives/libvir-list/2016-September/msg00294.html
>>
>> v6: http://www.redhat.com/archives/libvir-list/2016-September/msg00317.html
>>
>> I read those review requests as we need a way to be able to disable TLS
>> on a chardev on a domain by domain and even chardev by chardev basis
>> rather than it being the default enabled for every domain and every
>> chardev in the domain.
>>
>> Could you elaborate how it breaks the behavior of libvirt? Currently a
>> chardev doesn't have any TLS attribute - that qemu.conf change is from
>> already agreed upon changes. This set of changes should have been able
>> to go with those, but they've languished on the least through a release
>> cycle (2.3.0), so while it may appear that something is one way - it in
>> a way isn't. Right now it's just pixie/fairy dust and a pipe dream.
>
> The thing is that with libvirt-2.3.0 you can set chardev_tls=1 in qemu.conf
> and
> therefore all chardevs for all domains uses TLS to encrypt communication.
> This
> patch introduces a new 'tls' attribute and its default value is 'no' and that
> means if the user upgrades from libvirt-2.3.0 to libvirt-2.4.0 all chardevs
> stop
> encrypting communication unless he sets tls="yes" for every single chardev in
> every single domain.
UGH... mea culpa - in my mind the TLS hadn't been added to the chardev
yet and that was the rest of this series... Of course the rest of this
series is adding the passphrase for the environment (<sigh>).
If I could have got that review earlier, then this situation wouldn't be
a problem (see in my mind it wasn't).
If a domain is already running, then encryption is occurring and this
setting has no effect except for hotplug.
If we were using encryption in 2.3.0... stopped our domain... installed
2.4.0... started our domain, then yes, I see/agree.... Frustrating
since there's really no "simple" way to determine this.
>
> The correct solution is:
>
> - if chardev_tls is set to 1 and tls attribute is not configured in the
> XML
> the default after starting the domain should be tls=yes
So IOW:
+ if (cfg->chardevTLS &&
+ dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES) {
changes to
+ if (cfg->chardevTLS &&
+ dev->data.tcp.haveTLS != VIR_TRISTATE_BOOL_NO) {
or (haveTLS == YES || haveTLS == ABSENT)
This of course is essentially the logic from v6 which said disableTLS on
a case by case basis.... All we have now is the positive form...
So for both qemuBuildChrChardevStr and qemuDomainAttachChrDevice I'd
have a similar change. Does that suffice and do you need to see the change?
>
> - if chardev_tls is not set and tls attribute is not configured in the XML
> the default after starting the domain should be tls=no
>
We're not getting anywhere if chardev_tls is not set.
John
[...]
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
