qemu's behavior in this case is to change the spice server behavior to require secure connection to any channel not otherwise specified as being in plaintext mode. libvirt doesn't currently allow requesting this (via plaintext-channel=<channel name>). RHBZ: 819499 Signed-off-by: Alon Levy <alevy(a)redhat.com&gt; --- src/conf/domain_conf.c | 3 ++- src/conf/domain_conf.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10b023e..140bc63 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -429,7 +429,8 @@ VIR_ENUM_IMPL(virDomainGraphicsSpiceChannelName, "playback", "record", "smartcard", - "usbredir"); + "usbredir", + "default"); VIR_ENUM_IMPL(virDomainGraphicsSpiceChannelMode, VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_LAST, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6581fea..6189efa 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1098,6 +1098,7 @@ enum virDomainGraphicsSpiceChannelName { VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_RECORD, VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_SMARTCARD, VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_USBREDIR, + VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_DEFAULT, VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST }; -- 1.7.10.1

Add "usbredir" channel to list of recognized spice channels. RHBZ: 819498 Signed-off-by: Alon Levy <alevy(a)redhat.com&gt; --- docs/formatdomain.html.in | 5 +- docs/schemas/domaincommon.rng | 1 + src/conf/domain_conf.c | 3 +- src/conf/domain_conf.h | 1 + .../qemuxml2argv-graphics-spice-usb-redir.args | 16 ++++++ .../qemuxml2argv-graphics-spice-usb-redir.xml | 53 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 6 +++ 7 files changed, 82 insertions(+), 3 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index e1fe0c4..0525577 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2939,8 +2939,9 @@ qemu-kvm -net nic,model=? /dev/null include <code>main</code>, <code>display</code>, <code>inputs</code>, <code>cursor</code>, <code>playback</code>, <code>record</code>; - and <span class="since">since - 0.8.8</span>: <code>smartcard</code>. + <span class="since">since + 0.8.8</span>: <code>smartcard</code> and <span class="since"> + since 0.9.12</span> <code>usbredir</code>. </p> <pre> &lt;graphics type='spice' port='-1' tlsPort='-1' autoport='yes'&gt; diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8419ccc..77f2f6a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1787,6 +1787,7 @@ <value>playback</value> <value>record</value> <value>smartcard</value> + <value>usbredir</value> </choice> </attribute> <attribute name="mode"> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 3fce7e5..10b023e 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -428,7 +428,8 @@ VIR_ENUM_IMPL(virDomainGraphicsSpiceChannelName, "cursor", "playback", "record", - "smartcard"); + "smartcard", + "usbredir"); VIR_ENUM_IMPL(virDomainGraphicsSpiceChannelMode, VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_LAST, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 5aa8fc1..6581fea 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1097,6 +1097,7 @@ enum virDomainGraphicsSpiceChannelName { VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_PLAYBACK, VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_RECORD, VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_SMARTCARD, + VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_USBREDIR, VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST }; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.args new file mode 100644 index 0000000..35e51a7 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.args @@ -0,0 +1,16 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice /usr/bin/qemu -S -M pc -m 214 -smp 1 -nodefconfig -nodefaults -chardev socket,id=charmonitor,path=/tmp/test-monitor,server,nowait -mon chardev=charmonitor,id=monitor,mode=readline -no-acpi -boot c \ +-device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x4.0x7 \ +-device ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4 \ +-device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x4.0x1 \ +-device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x4.0x2 \ +-spice port=5903,tls-port=5904,addr=127.0.0.1,\ +x509-dir=/etc/pki/libvirt-spice,tls-channel=main,plaintext-channel=inputs,\ +tls-channel=usbredir,\ +image-compression=auto_glz,jpeg-wan-compression=auto,zlib-glz-wan-compression=auto,\ +playback-compression=on,streaming-video=filter,disable-copy-paste \ +-vga cirrus \ +-chardev socket,id=charredir0,host=localhost,port=4000 \ +-device usb-redir,chardev=charredir0,id=redir0 \ +-chardev spicevmc,id=charredir1,name=usbredir \ +-device usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=4 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.xml new file mode 100644 index 0000000..1dc23bd --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice-usb-redir.xml @@ -0,0 +1,53 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <graphics type='spice' port='5903' tlsPort='5904' autoport='no' listen='127.0.0.1'> + <listen type='address' address='127.0.0.1'/> + <channel name='main' mode='secure'/> + <channel name='inputs' mode='insecure'/> + <channel name='usbredir' mode='secure'/> + <image compression='auto_glz'/> + <jpeg compression='auto'/> + <zlib compression='auto'/> + <playback compression='on'/> + <streaming mode='filter'/> + <clipboard copypaste='no'/> + </graphics> + <controller type='usb' index='0' model='ich9-ehci1'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x7'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci1'> + <master startport='0'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0' multifunction='on'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci2'> + <master startport='2'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x1'/> + </controller> + <controller type='usb' index='0' model='ich9-uhci3'> + <master startport='4'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x2'/> + </controller> + <redirdev bus='usb' type='tcp'> + <source mode='connect' host='localhost' service='4000'/> + <protocol type='raw'/> + </redirdev> + <redirdev bus='usb' type='spicevmc'> + <address type='usb' bus='0' port='4'/> + </redirdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e47a385..4d7cf69 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -543,6 +543,12 @@ mymain(void) QEMU_CAPS_VGA, QEMU_CAPS_VGA_QXL, QEMU_CAPS_DEVICE, QEMU_CAPS_SPICE, QEMU_CAPS_DEVICE_QXL_VGA); + DO_TEST("graphics-spice-usb-redir", false, + QEMU_CAPS_VGA, QEMU_CAPS_SPICE, + QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_PCI_MULTIFUNCTION, QEMU_CAPS_USB_HUB, + QEMU_CAPS_ICH9_USB_EHCI1, QEMU_CAPS_USB_REDIR, + QEMU_CAPS_CHARDEV_SPICEVMC); DO_TEST("input-usbmouse", false, NONE); DO_TEST("input-usbtablet", false, NONE); -- 1.7.10.1

On 05/08/2012 07:00 AM, Alon Levy wrote: I decided to keep your approach of having default appear as a channel rather than a meta-annotation on the parent element (if only so that native-to-xml parsing is easier), but also thought that additional wording in the documentatino would help: diff --git c/docs/formatdomain.html.in i/docs/formatdomain.html.in index 1ccf7a3..4defcf1 100644 --- c/docs/formatdomain.html.in +++ i/docs/formatdomain.html.in @@ -2942,7 +2942,10 @@ qemu-kvm -net nic,model=? /dev/null (all <span class="since"> since 0.8.6</span>); <code>smartcard</code> (<span class="since">since 0.8.8</span>); and <code>usbredir</code> - (<span class="since">since 0.9.12</span>). + and <code>default</code> (<span class="since">since + 0.9.12</span>). The <code>default</code> channel is not + an actual channel, but specifies the settings for all + other channels not explicitly listed. </p> <pre> &lt;graphics type='spice' port='-1' tlsPort='-1' autoport='yes'&gt; ACK with that tweak. However, since I spoke with you on IRC about the possibility of an alternate implementation (https://www.redhat.com/archives/libvir-list/2012-May/msg00393.html), I'll wait to push until we can compare the two ideas. I'm sure that treating 'default' as a special case will be more lines of code, but might make the end-result XML more legible. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

qemu's behavior in this case is to change the spice server behavior to require secure connection to any channel not otherwise specified as being in plaintext mode. libvirt doesn't currently allow requesting this (via plaintext-channel=<channel name>). RHBZ: 819499 Signed-off-by: Alon Levy <alevy(a)redhat.com&gt; --- docs/formatdomain.html.in | 3 +++ docs/schemas/domaincommon.rng | 9 +++++++++ src/conf/domain_conf.c | 20 ++++++++++++++++++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 13 +++++++++++++ .../qemuxml2argv-graphics-spice.args | 2 +- .../qemuxml2argv-graphics-spice.xml | 2 +- 7 files changed, 48 insertions(+), 2 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 0525577..c0268b2 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2929,6 +2929,9 @@ qemu-kvm -net nic,model=? /dev/null <span class="since">Since 0.9.3</span> NB, this may not be supported by all hypervisors. <span class="since">"spice" since 0.8.6</span>. + The <code>defaultMode</code> attribute sets the default channel + security policy, valid values are <code>secure</code>, + <code>insecure</code> and the default <code>any</code>. </p> <p> When SPICE has both a normal and TLS secured TCP port diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 77f2f6a..84369c7 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1774,6 +1774,15 @@ </choice> </attribute> </optional> + <optional> + <attribute name="defaultMode"> + <choice> + <value>any</value> + <value>secure</value> + <value>insecure</value> + </choice> + </attribute> + </optional> <interleave> <ref name="listenElements"/> <zeroOrMore> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 10b023e..a60ef5a 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6071,6 +6071,8 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, char *port = virXMLPropString(node, "port"); char *tlsPort; char *autoport; + char *defaultMode; + int defaultModeVal; if (port) { if (virStrToLong_i(port, NULL, 10, &def->data.spice.port) < 0) { @@ -6103,6 +6105,20 @@ virDomainGraphicsDefParseXML(xmlNodePtr node, VIR_FREE(autoport); } + def->data.spice.defaultMode = VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY; + + if ((defaultMode = virXMLPropString(node, "defaultMode")) != NULL) { + if ((defaultModeVal = virDomainGraphicsSpiceChannelModeTypeFromString(defaultMode)) < 0) { + virDomainReportError(VIR_ERR_INTERNAL_ERROR, + _("unknown default spice channel mode %s"), + defaultMode); + VIR_FREE(defaultMode); + goto error; + } + def->data.spice.defaultMode = defaultModeVal; + VIR_FREE(defaultMode); + } + if (def->data.spice.port == -1 && def->data.spice.tlsPort == -1) { /* Legacy compat syntax, used -1 for auto-port */ def->data.spice.autoport = 1; @@ -12124,6 +12140,10 @@ virDomainGraphicsDefFormat(virBufferPtr buf, virBufferEscapeString(buf, " keymap='%s'", def->data.spice.keymap); + if (def->data.spice.defaultMode != VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY) + virBufferAsprintf(buf, " defaultMode='%s'", + virDomainGraphicsSpiceChannelModeTypeToString(def->data.spice.defaultMode)); + virDomainGraphicsAuthDefFormatAttr(buf, &def->data.spice.auth, flags); break; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 6581fea..895ddc4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1233,6 +1233,7 @@ struct _virDomainGraphicsDef { virDomainGraphicsAuthDef auth; unsigned int autoport :1; int channels[VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST]; + int defaultMode; int image; int jpeg; int zlib; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 070d13e..117542f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -5463,6 +5463,7 @@ qemuBuildCommandLine(virConnectPtr conn, const char *listenAddr = NULL; char *netAddr = NULL; int ret; + int defaultMode = def->graphics[0]->data.spice.defaultMode; if (!qemuCapsGet(qemuCaps, QEMU_CAPS_SPICE)) { qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -5546,6 +5547,18 @@ qemuBuildCommandLine(virConnectPtr conn, virBufferAsprintf(&opt, ",x509-dir=%s", driver->spiceTLSx509certdir); + switch (defaultMode) { + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_SECURE: + virBufferAsprintf(&opt, ",tls-channel=default"); + break; + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_INSECURE: + virBufferAsprintf(&opt, ",plaintext-channel=default"); + break; + case VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_MODE_ANY: + /* nothing */ + break; + } + for (i = 0 ; i < VIR_DOMAIN_GRAPHICS_SPICE_CHANNEL_LAST ; i++) { int mode = def->graphics[0]->data.spice.channels[i]; switch (mode) { diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args index c9fdb99..698e39c 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.args @@ -2,7 +2,7 @@ LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=spice \ /usr/bin/qemu -S -M pc -m 214 -smp 1 -nodefaults -monitor \ unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -hda \ /dev/HostVG/QEMUGuest1 -usb -spice port=5903,tls-port=5904,addr=127.0.0.1,\ -x509-dir=/etc/pki/libvirt-spice,tls-channel=main,plaintext-channel=inputs,\ +x509-dir=/etc/pki/libvirt-spice,tls-channel=default,tls-channel=main,plaintext-channel=inputs,\ image-compression=auto_glz,jpeg-wan-compression=auto,zlib-glz-wan-compression=auto,\ playback-compression=on,streaming-video=filter,disable-copy-paste -vga \ qxl -global qxl.vram_size=18874368 -device qxl,id=video1,vram_size=33554432,bus=pci.0,addr=0x4 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.xml b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.xml index 8930b60..a3789f2 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-graphics-spice.xml @@ -22,7 +22,7 @@ <controller type='usb' index='0'/> <controller type='ide' index='0'/> <input type='mouse' bus='ps2'/> - <graphics type='spice' port='5903' tlsPort='5904' autoport='no' listen='127.0.0.1'> + <graphics type='spice' port='5903' tlsPort='5904' autoport='no' listen='127.0.0.1' defaultMode='secure'> <listen type='address' address='127.0.0.1'/> <channel name='main' mode='secure'/> <channel name='inputs' mode='insecure'/> -- 1.7.10.1

On Tue, May 08, 2012 at 12:19:45PM -0600, Eric Blake wrote: Your table is correct and a good summary. But to be clear the "any" value is not a valid value to put in qemu's command line, it's the internal default state (actually it's oring of the two valid channel modes, insecure and secure). But it behaves exactly like your table explains, i.e. not setting tls-channel=default or plaintext-channel=default results in the middle column, setting tls-channel=default in the right, and plaintext-channel=default in the left. You could have expanded the table because it's also possible to start spice with only tls-port set, and no port, then you would get the reverse of the right column: insecure any secure both plaintext tls tls only clear plaintext plaintext error only tls error plaintext tls Looks good.

On 05/08/2012 07:00 AM, Alon Levy wrote: I've been convinced that this is a bug worth fixing for 0.9.12 (without properly listing all channels, you can inadvertently end up with insecure channels, and security fixes outweigh the risk of new XML after rc1 freeze). Missing a colon; I tweaked this slightly. ACK and pushed. -- Eric Blake eblake(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org