Bug 790436

For what it's worth, looking at the code I came up with the patch below. I still need to test it, rework the commit log, ... but this will go to the libvirt mailing list soonish if it works as expected

From a843c847cb3c6ae6749834ae510617e7475e7ecf Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau>
Date: Tue, 14 Feb 2012 15:27:36 +0100
Subject: [PATCH] Don't add SPICE TLS channels when TLS is disabled

It's possible to disable SPICE TLS in qemu.conf. When this happens,
libvirt ignores any SPICE TLS port or x509 directory that may have
been set when it builds the qemu command line to use. However, it's
not ignoring the secure channels that may have been set and adds
tls-channel arguments to qemu command line.
qemu doesn't report an error when this happens, but I'm not sure yet
it behaves sanely when we do that, so better not to add these arguments.
---
 src/qemu/qemu_command.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 99d7129..30fb3b1 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -5286,8 +5286,9 @@ qemuBuildCommandLine(virConnectPtr conn,
             int mode = def->graphics[0]->data.spice.channels[i];
             switch (mode) {
             case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE:
-                virBufferAsprintf(&opt, ",tls-channel=%s",
-                                  virDomainGraphicsSpiceChannelNameTypeToString(i));
+                if (driver->spiceTLS)
+                    virBufferAsprintf(&opt, ",tls-channel=%s",
+                                      virDomainGraphicsSpiceChannelNameTypeToString(i));
                 break;
             case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE:
                 virBufferAsprintf(&opt, ",plaintext-channel=%s",
-- 
1.7.7.6

Is it better to silently ignore the user's request for a secure channel, or should libvirt be erroring out on any attempt to start a domain whose XML requested a secure channel when qemu.conf disabled TLS?

I think that error is in this case appropriate. Current behaviour leads to inaccessible guest via Spice (because of bug #790421) which is hardly better than plain error.

See my take on this at
https://www.redhat.com/archives/libvir-list/2012-February/msg00675.html :

"My initial feeling was that we should error out. However, I don't think
it's realistic at this point:
* oVirt is already disabling TLS in qemu.conf if the admin says he does not
  want it during engine-setup, but oVirt still adds the TLS port/secure
  channels to its libvirt VMs even when TLS is disabled. They probably
  won't be that happy if libvirt starts erroring out here. Though we can
  also argue that it's a bug on their side, and that the VMs they start this
  way (with TLS XML bits but TLS disabled in the conf file) are not usable
  anyway because of these extra tls-channel options
* this will also be annoying if someone has setup a bunch of VMs with TLS
  parameters and then decides to change the TLS setting in qemu.conf, these
  VMs will suddenly fail to start until their conf is modified

Because of that, I think we should unfortunately just ignore these TLS bits
when TLS is disabled. We can log a warning though when we detect this. Once
again, regardless of what we decide to do, it's better done in a separate
patch."

Moreover, we currently silently ignore TLS ports set in the XML config when TLS
is disabled in qemu.conf so this would need changing there too.

Due to back-compat for older oVirt requesting secure channels at the same time they disable TLS, maybe we need to introduce a third mode for each spice channel:

mode='insecure' - don't bother with security
mode='secure' - use security if tls is available, but fall back to insecure
mode='mandatory-secure' - use security, and error out if tls not available

(In reply to comment #4)
> I think that error is in this case appropriate. Current behaviour leads to
> inaccessible guest via Spice (because of bug #790421) which is hardly better
> than plain error.

Given that, I'd favor avoiding the third mode and just going with refusing to start the guest.  We should make sure that's ok with the broader ovirt team, though.

(In reply to comment #6)
> mode='insecure' - don't bother with security

By this, you mean plaintext-only setting, or provide tls if certs/keys are available?

> mode='secure' - use security if tls is available, but fall back to insecure

What does falling back mean here? Provide both and let client choose or provide plaintext channel only if tls-port or x509 stuff is not set?

> mode='mandatory-secure'

^^^ FWIW, this is what RHEV means by mode='secure'. They _do want_ to enforce main and input channels to TLS. oVirt's feature of hosts without mandatory TLS is quite new and clearly not yet stabilized.

Another consideration is that sometimes, you could want some channels to be mandatory insecure, like display + playback on hosts that do not support aes-ni.

Given the facts above, I'd propose going with these four modes:

* mode='insecure' - provide plaintext-only (-spice plaintext-channel=...)
* mode='...' - provide both if BOTH x509 stuff is set and TLS port
               will get alocated, else provide plaintext-only
* mode='...' - provide both tls and plaintext, error out if x509 stuff is
               not set
* mode='secure' - provide TLS channel only, error out if x509 stuff is not set

The secure mode name is chosen with respect to compatibility with RHEV usage and I didn't manage to name the middle ones sanely. When spice_tls is == 0, second mode should be default, when spice_tls == 1, third mode should be default.

minor correction:

(In reply to comment #8)
> and I didn't manage to name the middle ones sanely. When spice_tls is == 0,
> second mode should be default, when spice_tls == 1, third mode should be
> default.

with spice_tls set to 1, main and inputs channels should default to fourth mode, the rest to third.

Would you mind making these same suggestions on the upstream list, for a wider audience?  https://www.redhat.com/archives/libvir-list/2012-February/msg00711.html

(In reply to comment #10)
> Would you mind making these same suggestions on the upstream list, for a wider
> audience? 
> https://www.redhat.com/archives/libvir-list/2012-February/msg00711.html

Done: https://www.redhat.com/archives/libvir-list/2012-February/msg00775.html

In POST:
http://post-office.corp.redhat.com/archives/rhvirt-patches/2012-February/msg01338.html

Verify this bug with libvirt-0.9.10-3.el6.x86_64

set /etc/libvirt/qemu.conf   spice_tls = 0  then restart libvirtd 


#virsh define tls.xml 

<domain type='kvm'>
  <name>auto-tls-port</name>
  <memory>65536</memory>
  <os>
    <type arch='x86_64' machine='pc'>hvm</type>
  </os>
  <devices>
    <graphics type='spice' port='5900' tlsPort='-1' autoport='yes' listen='0' keymap='en-us' passwdValidTo='2012-02-14T13:45:54' connected='disconnect'>
      <listen type='address' address='0'/>
      <channel name='main' mode='secure'/>
      <channel name='inputs' mode='secure'/>
    </graphics>
  </devices>
</domain>


# virsh start auto-tls-port 
error: Failed to start domain auto-tls-port
error: unsupported configuration: spice secure channels set in XML configuration, but TLS is disabled in qemu.conf

Back to ASSIGNED, current check stops qemu even in case when 

$ grep -A2 'graphics type' rhel6.xml 
    <graphics type='spice' port='3005' autoport='no' listen='0.0.0.0'>
      <listen type='address' address='0.0.0.0'/>
    </graphics>
$ LANG= virsh domxml-to-native --format qemu-argv --xml rhel6.xml 
error: unsupported configuration: spice TLS port set in XML configuration, but TLS is disabled in qemu.conf

The behaviour is the same under root and libvirtd.log doesn't provide any insight even with debug = 1.

Version information:
$ rpm -qa | grep 'libvirt*\|qemu-kvm*'
qemu-kvm-0.12.1.2-2.236.el6.x86_64
libvirt-0.9.10-3.el6.x86_64
qemu-kvm-debuginfo-0.12.1.2-2.236.el6.x86_64
qemu-kvm-tools-0.12.1.2-2.236.el6.x86_64
libvirt-python-0.9.10-3.el6.x86_64
libvirt-client-0.9.10-3.el6.x86_64

If set autoport='no' and not set tlsPort in domain xml ,in this case the tlsPort is  automatic allocated , so it reports TLS error message.

(In reply to comment #19)
> If set autoport='no' and not set tlsPort in domain xml ,in this case the
> tlsPort is  automatic allocated , so it reports TLS error message.

Hi Wenlong,

Is this behaviour documented somewhere? I can not find it in libvirt documentation and when writing the configuration with no mention of tlsPort, I would expect libvirt not to care about it.

(In reply to comment #20)
> (In reply to comment #19)
> > If set autoport='no' and not set tlsPort in domain xml ,in this case the
> > tlsPort is  automatic allocated , so it reports TLS error message.
> 
> Hi Wenlong,
> 
> Is this behaviour documented somewhere? I can not find it in libvirt
> documentation and when writing the configuration with no mention of tlsPort, I
> would expect libvirt not to care about it.

Hi,David

Sorry ，I can not find document about this,you'd better needinfo developer of libvirt .

Wenlong

I think this is clearly a bug;

From our parsing code:

src/conf/domain_conf.c:5992:        tlsPort = virXMLPropString(node, "tlsPort");
src/conf/domain_conf.c:5993:        if (tlsPort) {
src/conf/domain_conf.c:5994:            if (virStrToLong_i(tlsPort, NULL, 10, &def->data.spice.tlsPort) < 0) {
src/conf/domain_conf.c-5995-                virDomainReportError(VIR_ERR_INTERNAL_ERROR,
src/conf/domain_conf.c:5996:                                     _("cannot parse spice tlsPort %s"), tlsPort);
src/conf/domain_conf.c:5997:                VIR_FREE(tlsPort);
src/conf/domain_conf.c-5998-                goto error;
src/conf/domain_conf.c-5999-            }
src/conf/domain_conf.c:6000:            VIR_FREE(tlsPort);
src/conf/domain_conf.c-6001-        } else {
src/conf/domain_conf.c:6002:            def->data.spice.tlsPort = 0;
src/conf/domain_conf.c-6003-        }

However, if we build qemu command line we test tlsPort against -1:

src/qemu/qemu_command.c:5377:        if (def->graphics[0]->data.spice.tlsPort != -1) {
src/qemu/qemu_command.c-5378-            if (!driver->spiceTLS) {
src/qemu/qemu_command.c-5379-                qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
src/qemu/qemu_command.c-5380-                                _("spice TLS port set in XML configuration,"
src/qemu/qemu_command.c-5381-                                  " but TLS is disabled in qemu.conf"));
src/qemu/qemu_command.c-5382-                goto error;
src/qemu/qemu_command.c-5383-            }
src/qemu/qemu_command.c-5384-            virBufferAsprintf(&opt, ",tls-port=%u",
src/qemu/qemu_command.c:5385:                              def->graphics[0]->data.spice.tlsPort);
src/qemu/qemu_command.c-5386-        }


I've proposed patch upstream:

https://www.redhat.com/archives/libvir-list/2012-March/msg00332.html

Sorry for bad formatting. Maybe this time I've leaned Bugzilla gods onto my side:

From our parsing code:

tlsPort = virXMLPropString(node, "tlsPort");
if (tlsPort) {
    if (virStrToLong_i(tlsPort, NULL, 10, &def->data.spice.tlsPort) < 0) {
        virDomainReportError(VIR_ERR_INTERNAL_ERROR,
                             _("cannot parse spice tlsPort %s"), tlsPort);
        VIR_FREE(tlsPort);
        goto error;
    }
    VIR_FREE(tlsPort);
} else {
    def->data.spice.tlsPort = 0;
}

However, if we build qemu command line we test tlsPort against -1:

if (def->graphics[0]->data.spice.tlsPort != -1) {
    if (!driver->spiceTLS) {
        qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
                        _("spice TLS port set in XML configuration,"
                          " but TLS is disabled in qemu.conf"));
        goto error;
    }
    virBufferAsprintf(&opt, ",tls-port=%u",
                      def->graphics[0]->data.spice.tlsPort);
}

Re-thinking this, we should move this back to VERIFIED as the initial problem is fixed. For the issue David is mentioning in comment #17 I've created a separate bug 801443.

Set Verified this bug due to comment #24

Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHSA-2012-0748.html

*** Bug 834443 has been marked as a duplicate of this bug. ***