On Fri, Oct 14, 2016 at 10:16:19AM -0400, John Ferlan wrote: > > > 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 <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 +- > >>>> ...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? I think that we should reflect the state in live XML if the attribute exists. Add a code to qemuProcessPrepareDomain() that will set dev->data.tcp.haveTLS to config value if dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_ABSENT and in qemuBuildChrChardevStr() you can just check if dev->data.tcp.haveTLS == VIR_TRISTATE_BOOL_YES. This will also ensure that the live XML contains the 'tls' attribute. But make sure, that migration works properly for all combinations. I guess a another version would be better if you agree with exposing current state into live XML. Pavel > > - 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@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