On 07/11/2017 09:43 PM, ashish mittal wrote:
On Fri, Jun 30, 2017 at 2:21 PM, John Ferlan <jferlan@redhat.com> wrote:
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 8e00782..99bc94f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -931,6 +931,68 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src) return ret; }
+/* qemuBuildDiskVxHSTLSinfoCommandLine: + * @cmd: Pointer to the command string + * @cfg: Pointer to the qemu driver config + * @disk: The disk we are processing + * @qemuCaps: qemu capabilities object + * + * Check if the VxHS disk meets all the criteria to enable TLS. + * If yes, add a new TLS object and mention it's ID on the disk + * command line. + * + * Returns 0 on success, -1 w/ error on some sort of failure. + */ +static int +qemuBuildDiskVxHSTLSinfoCommandLine(virCommandPtr cmd, + virQEMUDriverConfigPtr cfg, + virDomainDiskDefPtr disk, + virQEMUCapsPtr qemuCaps) +{ + int ret = 0; + + if (cfg->vxhsTLS == true && disk->src->haveTLS != VIR_TRISTATE_BOOL_NO) { + disk->src->addTLS = true; + ret = qemuBuildTLSx509CommandLine(cmd, cfg->vxhsTLSx509certdir, + false, + true, + false, + "vxhs",
This argument can't be a constant. This is the alias for the certificate object.
Otherwise you'd had to make sure that there's only one such object, which obviously would make sense here, since (if you don't hotplug disks after libvirtd restart) the TLS credentials are the same for this disk.
In case of hotplug though you need to make sure that the TLS object is removed with the last disk and added if any other disk needing TLS is added.
So at least the conversation we had last week now makes a bit more sense w/r/t the call to qemuBuildTLSx509CommandLine for chardevTLSx509certdir. As I look at that code now quickly, although having multiple tls-cert-x509 objects for each chardev isn't necessary, it does "work" or start qemu because each would have a different alias (@charAlias is uniquely generated via qemuAliasChardevFromDevAlias). Theoretically speaking two objects wouldn't be required, except for the problem that hotunplug would have to be made aware and we'd have to keep track of when the last one was removed. So leaving with each having their own object is the way the code will stay.
So given all that - your alias creation is going to have to contain that uuid or you are going to have to figure out a way to just have one object which each disk uses. You'll have to scan the disks looking to determine if any of the vxhs ones have tls and if so, break to add the object. Then add the 'tls-creds=$object_alias_id'.
Hi John, Peter,
Both Peter and I have been wrapped up in something a bit more pressing lately, but figured I'd take a look so you're not left wondering too long.
The problem statement - Alias for the TLS certificate can't be a constant. Two TLS objects cannot have the same ID/alias.
This was pointed out by both of you as something that needs to be fixed. To that end, I have made some changes to use the block device domain alias (e.g. virtio-disk0) as a unique identifier for each TLS object. This is similar to how char devices create their TLS aliases.
I was hoping to get some feedback on whether this diff would be acceptable to fix the said issue. I will reply to/fix all the other comments. Just thought I'd tackle this one first as this appears to be one of the bigger ones...
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 99bc94f..fc58236 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -952,13 +952,19 @@ qemuBuildDiskVxHSTLSinfoCommandLine(virCommandPtr cmd, int ret = 0;
if (cfg->vxhsTLS == true && disk->src->haveTLS != VIR_TRISTATE_BOOL_NO) { - disk->src->addTLS = true; - ret = qemuBuildTLSx509CommandLine(cmd, cfg->vxhsTLSx509certdir, - false, - true, - false, - "vxhs", - qemuCaps); + if (virAsprintf(&disk->src->aliasTLS, + "vxhs_%s", disk->info.alias) < 0) { + ret = -1; + goto cleanup; + }
Typically something like this ^^^ would be turned into a helper so there's no need to store @aliasTLS. My suggestion would be to use qemuAliasChardevFromDevAlias as a guide. Create a qemu_alias.c helper/API that allows passing 2 parameters - one the "disk->src->protocol" and the other the "disk->info.alias". Then in the function the protocol would be turned into a string via virStorageNetProtocolTypeToString and the created alias can be "%s_%s" (so you don't have to change your existing .args output). This way if "iscsi" or "rbd" or whatever comes along some day, they'd just pass src->protocol too and magically the string is created with teh "vxhs" or "iscsi" or "rbd" (etc) prefixdisk. BTW: A similar argument could be made for the qemuParseVxHSString change you have. Similar to the @protocol in qemuBuildVxHSDriveJSON Since you can generate the alias at any time, the aliasTLS would be unnecessary. John
+ + disk->src->addTLS = true; + ret = qemuBuildTLSx509CommandLine(cmd, cfg->vxhsTLSx509certdir, + false, + true, + false, + disk->src->aliasTLS, + qemuCaps); } else if (cfg->vxhsTLS == false && disk->src->haveTLS == VIR_TRISTATE_BOOL_YES) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -968,6 +974,7 @@ qemuBuildDiskVxHSTLSinfoCommandLine(virCommandPtr cmd, ret = -1; }
+ cleanup: return ret; }
@@ -1040,7 +1047,7 @@ qemuBuildVxHSDriveJSON(virStorageSourcePtr src) if (src->addTLS == true) { char *objalias = NULL;
- if (!(objalias = qemuAliasTLSObjFromSrcAlias("vxhs"))) + if (!(objalias = qemuAliasTLSObjFromSrcAlias(src->aliasTLS))) goto cleanup;
if (virJSONValueObjectCreate(&ret, diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 449ace4..61cd54a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2057,7 +2057,8 @@ virStorageSourceCopy(const virStorageSource *src, VIR_STRDUP(ret->configFile, src->configFile) < 0 || VIR_STRDUP(ret->nodeformat, src->nodeformat) < 0 || VIR_STRDUP(ret->nodebacking, src->nodebacking) < 0 || - VIR_STRDUP(ret->compat, src->compat) < 0) + VIR_STRDUP(ret->compat, src->compat) < 0 || + VIR_STRDUP(ret->aliasTLS, src->aliasTLS) < 0) goto error;
if (src->nhosts) { @@ -2273,6 +2274,7 @@ virStorageSourceClear(virStorageSourcePtr def) virStorageSourceSeclabelsClear(def); virStoragePermsFree(def->perms); VIR_FREE(def->timestamps); + VIR_FREE(def->aliasTLS);
virStorageNetHostDefFree(def->nhosts, def->hosts); virStorageAuthDefFree(def->auth); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index e586170..c1d36bf 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -290,6 +290,9 @@ struct _virStorageSource { * the device. For e.g. this could be based on a combination of * global conf setting + domain specific setting */ bool addTLS; + + /* The alias/ID of the TLS object */ + char *aliasTLS; };
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args index 4cacb61..b474ce3 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args @@ -18,17 +18,19 @@ QEMU_AUDIO_DRV=none \ -no-acpi \ -boot c \ -usb \ --object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\ +-object tls-creds-x509,id=objvxhs_virtio-disk0_tls0,\ +dir=/usr/local/etc/pki/qemu,\ endpoint=client,verify-peer=yes \ --drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ +-drive file.driver=vxhs,file.tls-creds=objvxhs_virtio-disk0_tls0,\ file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\ id=drive-virtio-disk0,cache=none \ -device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ id=virtio-disk0 \ --object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\ +-object tls-creds-x509,id=objvxhs_virtio-disk1_tls0,\ +dir=/usr/local/etc/pki/qemu,\ endpoint=client,verify-peer=yes \ --drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ +-drive file.driver=vxhs,file.tls-creds=objvxhs_virtio-disk1_tls0,\ file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc252,\ file.server.host=192.168.0.2,file.server.port=9999,format=raw,\ if=none,id=drive-virtio-disk1,cache=none \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args index e1ad36e..ad78405 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args @@ -18,9 +18,10 @@ QEMU_AUDIO_DRV=none \ -no-acpi \ -boot c \ -usb \ --object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\ +-object tls-creds-x509,id=objvxhs_virtio-disk0_tls0,\ +dir=/usr/local/etc/pki/qemu,\ endpoint=client,verify-peer=yes \ --drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ +-drive file.driver=vxhs,file.tls-creds=objvxhs_virtio-disk0_tls0,\ file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\ id=drive-virtio-disk0,cache=none \
Thanks, Ashish
BTW: if you haven't already, read Dan Berrange's blog on TLS:
https://www.berrange.com/posts/2016/04/01/improving-qemu-security-part-2-gen...
that's a link to page 2, read/scan the remaining 5 blogs as well. Some of the exact qemu syntax has changed since the blog was written, but the description is really what helps the frame of reference.
+ qemuCaps); + } else if (cfg->vxhsTLS == false && + disk->src->haveTLS == VIR_TRISTATE_BOOL_YES) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("Please enable VxHS specific TLS options in the qemu " + "conf file before using TLS in VxHS device domain " + "specification")); + ret = -1; + } + + return ret; +}
[...]
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args new file mode 100644 index 0000000..960960d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args
[this file has same mistake as the one below]
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args.new b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args.new new file mode 100644 index 0000000..960960d --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args.new @@ -0,0 +1,41 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-cpu qemu32 \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\a
This ...
+endpoint=client,verify-peer=yes \ +-drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ +file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\ +file.server.host=192.168.0.1,file.server.port=9999,format=raw,if=none,\ +id=drive-virtio-disk0,cache=none \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\
... and this looks wrong. You have two tls-creds-x509 with the same alias. I doubt that qemu will be happy wit that.
Not only that, but dir=/usr/local/etc/pki/qemu causes qemuxml2argvtest to fail for me since the default is supposed to be /etc/pki/libvirt-vxhs
John
+endpoint=client,verify-peer=yes \ +-drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ +file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc252,\ +file.server.host=192.168.0.2,file.server.port=9999,format=raw,if=none,\ +id=drive-virtio-disk1,cache=none \ +-device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,\ +id=virtio-disk1 \ +-drive file.driver=vxhs,file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc253,\ +file.server.host=192.168.0.3,file.server.port=9999,format=raw,if=none,\ +id=drive-virtio-disk2,cache=none \ +-device virtio-blk-pci,bus=pci.0,addr=0x6,drive=drive-virtio-disk2,\ +id=virtio-disk2 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.xml new file mode 100644 index 0000000..3d28958 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.xml @@ -0,0 +1,56 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>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-system-x86_64</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw' cache='none'/> + <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc251'> + <host name='192.168.0.1' port='9999'/> + </source> + <backingStore/> + <target dev='vda' bus='virtio'/> + <serial>eb90327c-8302-4725-9e1b-4e85ed4dc251</serial> + <alias name='virtio-disk0'/>
This here ...
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw' cache='none'/> + <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc252'> + <host name='192.168.0.2' port='9999'/> + </source> + <backingStore/> + <target dev='vdb' bus='virtio'/> + <serial>eb90327c-8302-4725-9e1b-4e85ed4dc252</serial> + <alias name='virtio-disk0'/>
... and this have the same alias, which will not happen. Please add proper examples/tests.
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw' cache='none'/> + <source protocol='vxhs' name='eb90327c-8302-4725-9e1b-4e85ed4dc253' tls='no'> + <host name='192.168.0.3' port='9999'/> + </source> + <backingStore/> + <target dev='vdc' bus='virtio'/> + <serial>eb90327c-8302-4725-9e1b-4e85ed4dc252</serial> + <alias name='virtio-disk0'/>
... here too.
+ <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain>