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 <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
> >>>>    &lt;/devices&gt;
> >>>>    ...</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@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Reply via email to