[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v4 3/3] Add TLS support for Veritas HyperScale (VxHS) block device protocol



[...]

>> 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'.

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>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]