On 07/11/2017 09:43 PM, ashish mittal wrote:
> 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,
>
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-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
