[libvirt] [PATCHv2] Error out when using SPICE TLS with spice_tls=0
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. Current qemu versions don't report an error when this happens, and try to use TLS for the specified channels. Before this patch <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' ke <listen type='address' address='0'/> <channel name='main' mode='secure'/> <channel name='inputs' mode='secure'/> </graphics> </devices> </domain> generates -spice port=5900,addr=0,disable-ticketing,tls-channel=main,tls-channel=inputs and starts QEMU. After this patch, an error is reported if a TLS port is set in the XML or if secure channels are specified but TLS is disabled in qemu.conf. This is the behaviour the oVirt people (where I spotted this issue) said they would expect. This fixes bug #790436 --- src/qemu/qemu_command.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5a34504..4f3e61e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5231,7 +5231,12 @@ qemuBuildCommandLine(virConnectPtr conn, virBufferAsprintf(&opt, "port=%u", def->graphics[0]->data.spice.port); - if (driver->spiceTLS && def->graphics[0]->data.spice.tlsPort != -1) + if (def->graphics[0]->data.spice.tlsPort != -1) + if (!driver->spiceTLS) { + qemuReportError(VIR_ERR_XML_ERROR, + _("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); switch (virDomainGraphicsListenGetType(def->graphics[0], 0)) { @@ -5287,6 +5292,11 @@ qemuBuildCommandLine(virConnectPtr conn, int mode = def->graphics[0]->data.spice.channels[i]; switch (mode) { case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: + if (!driver->spiceTLS) { + qemuReportError(VIR_ERR_XML_ERROR, + _("spice secure channels set in XML configuration, but TLS is disabled in qemu.conf")); + goto error; + } virBufferAsprintf(&opt, ",tls-channel=%s", virDomainGraphicsSpiceChannelNameTypeToString(i)); break; -- 1.7.7.6
On Fri, Feb 24, 2012 at 11:34:45AM +0100, Christophe Fergeau wrote:
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. Current qemu versions don't report an error when this happens, and try to use TLS for the specified channels.
Before this patch
<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' ke <listen type='address' address='0'/> <channel name='main' mode='secure'/> <channel name='inputs' mode='secure'/> </graphics> </devices> </domain>
generates
-spice port=5900,addr=0,disable-ticketing,tls-channel=main,tls-channel=inputs
and starts QEMU.
After this patch, an error is reported if a TLS port is set in the XML or if secure channels are specified but TLS is disabled in qemu.conf. This is the behaviour the oVirt people (where I spotted this issue) said they would expect.
This fixes bug #790436 --- src/qemu/qemu_command.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5a34504..4f3e61e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5231,7 +5231,12 @@ qemuBuildCommandLine(virConnectPtr conn,
virBufferAsprintf(&opt, "port=%u", def->graphics[0]->data.spice.port);
- if (driver->spiceTLS && def->graphics[0]->data.spice.tlsPort != -1) + if (def->graphics[0]->data.spice.tlsPort != -1) + if (!driver->spiceTLS) { + qemuReportError(VIR_ERR_XML_ERROR, + _("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);
switch (virDomainGraphicsListenGetType(def->graphics[0], 0)) { @@ -5287,6 +5292,11 @@ qemuBuildCommandLine(virConnectPtr conn, int mode = def->graphics[0]->data.spice.channels[i]; switch (mode) { case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: + if (!driver->spiceTLS) { + qemuReportError(VIR_ERR_XML_ERROR, + _("spice secure channels set in XML configuration, but TLS is disabled in qemu.conf")); + goto error; + } virBufferAsprintf(&opt, ",tls-channel=%s", virDomainGraphicsSpiceChannelNameTypeToString(i)); break;
ACK, if we s/XML_ERROR/CONFIG_UNSUPPORTED/ in the those two error messages Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 02/24/2012 03:41 AM, Daniel P. Berrange wrote:
On Fri, Feb 24, 2012 at 11:34:45AM +0100, Christophe Fergeau wrote:
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. Current qemu versions don't report an error when this happens, and try to use TLS for the specified channels.
This fixes bug #790436
Thanks for chasing this down.
ACK, if we s/XML_ERROR/CONFIG_UNSUPPORTED/ in the those two error messages
Changed and pushed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 24.02.2012 11:34, Christophe Fergeau wrote:
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. Current qemu versions don't report an error when this happens, and try to use TLS for the specified channels.
Before this patch
<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' ke <listen type='address' address='0'/> <channel name='main' mode='secure'/> <channel name='inputs' mode='secure'/> </graphics> </devices> </domain>
generates
-spice port=5900,addr=0,disable-ticketing,tls-channel=main,tls-channel=inputs
and starts QEMU.
After this patch, an error is reported if a TLS port is set in the XML or if secure channels are specified but TLS is disabled in qemu.conf. This is the behaviour the oVirt people (where I spotted this issue) said they would expect.
This fixes bug #790436 --- src/qemu/qemu_command.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5a34504..4f3e61e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5231,7 +5231,12 @@ qemuBuildCommandLine(virConnectPtr conn,
virBufferAsprintf(&opt, "port=%u", def->graphics[0]->data.spice.port);
- if (driver->spiceTLS && def->graphics[0]->data.spice.tlsPort != -1) + if (def->graphics[0]->data.spice.tlsPort != -1) + if (!driver->spiceTLS) { + qemuReportError(VIR_ERR_XML_ERROR, + _("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);
In fact, this needs to be wrapped with curly braces as the check for tlsPort != -1 is meant to protect virBufferAsprintf() in the first place. Sorry for not catching this earlier. As an act of repentance I'll send patch. Michal
On Tue, Feb 28, 2012 at 05:04:57PM +0100, Michal Privoznik wrote:
On 24.02.2012 11:34, Christophe Fergeau wrote:
src/qemu/qemu_command.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 5a34504..4f3e61e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5231,7 +5231,12 @@ qemuBuildCommandLine(virConnectPtr conn,
virBufferAsprintf(&opt, "port=%u", def->graphics[0]->data.spice.port);
- if (driver->spiceTLS && def->graphics[0]->data.spice.tlsPort != -1) + if (def->graphics[0]->data.spice.tlsPort != -1) + if (!driver->spiceTLS) { + qemuReportError(VIR_ERR_XML_ERROR, + _("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);
In fact, this needs to be wrapped with curly braces as the check for tlsPort != -1 is meant to protect virBufferAsprintf() in the first place. Sorry for not catching this earlier.
Ugh, sorry about that silly bug, thanks for catching it. Christophe
participants (4)
-
Christophe Fergeau -
Daniel P. Berrange -
Eric Blake -
Michal Privoznik