On Fri, Jun 30, 2017 at 2:21 PM, John Ferlan <jferlan@xxxxxxxxxx> 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,
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;
+ }
+
+ 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-generic-tls-support/
>
> 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>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
