Re: [PATCH v8 11/11] qemu: Add TLS support for Veritas HyperScale (VxHS)

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

 




On 09/19/2017 10:02 AM, Peter Krempa wrote:
> On Thu, Sep 14, 2017 at 08:51:56 -0400, John Ferlan wrote:
>> From: Ashish Mittal <Ashish.Mittal@xxxxxxxxxxx>
>>
>> Alter qemu command line generation in order to possibly add TLS for
>> a suitably configured domain.
>>
>> Sample TLS args generated by libvirt -
>>
>>     -object tls-creds-x509,id=objvirtio-disk0_tls0,dir=/etc/pki/qemu,\
>>     endpoint=client,verify-peer=yes \
>>     -drive file.driver=vxhs,file.tls-creds=objvirtio-disk0_tls0,\
>>     file.vdisk-id=eb90327c-8302-4725-9e1b-4e85ed4dc251,\
>>     file.server.type=tcp,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
>>
>> Update the qemuxml2argvtest with a couple of examples. One for a
>> simple case and the other a bit more complex where multiple VxHS disks
>> are added where at least one uses a VxHS that doesn't require TLS
>> credentials and thus sets the domain disk source attribute "tls = 'no'".
>>
>> Update the hotplug to be able to handle processing the tlsAlias whether
>> it's to add the TLS object when hotplugging a disk or to remove the TLS
>> object when hot unplugging a disk.  The hot plug/unplug code is largely
>> generic, but the addition code does make the VXHS specific checks only
>> because it needs to grab the correct config directory and generate the
>> object as the command line would do.
>>
>> Signed-off-by: Ashish Mittal <Ashish.Mittal@xxxxxxxxxxx>
>> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
>> ---
>>  src/qemu/qemu_block.c                              |  8 +++
>>  src/qemu/qemu_command.c                            | 29 +++++++++
>>  src/qemu/qemu_hotplug.c                            | 73 ++++++++++++++++++++++
>>  ...-disk-drive-network-tlsx509-multidisk-vxhs.args | 43 +++++++++++++
>>  ...v-disk-drive-network-tlsx509-multidisk-vxhs.xml | 50 +++++++++++++++
>>  ...muxml2argv-disk-drive-network-tlsx509-vxhs.args | 30 +++++++++
>>  tests/qemuxml2argvtest.c                           |  7 +++
>>  7 files changed, 240 insertions(+)
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.xml
>>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args
>>
>> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
>> index ca6e213b4..458b90d8e 100644
>> --- a/src/qemu/qemu_block.c
>> +++ b/src/qemu/qemu_block.c
>> @@ -529,16 +529,24 @@ qemuBlockStorageSourceGetVxHSProps(virStorageSourcePtr src)
>>          return NULL;
>>      }
>>  
>> +    if (src->haveTLS == VIR_TRISTATE_BOOL_YES && !src->tlsAlias) {
>> +        virReportError(VIR_ERR_INVALID_ARG, "%s",
>> +                       _("VxHS disk does not have TLS alias set"));
>> +        return NULL;
>> +    }
>> +
>>      if (!(server = qemuBlockStorageSourceBuildJSONSocketAddress(src->hosts, true)))
>>          return NULL;
>>  
>>      /* VxHS disk specification example:
>>       * { driver:"vxhs",
>> +     *   [tls-creds:"objvirtio-disk0_tls0",]
> 
> Be careful with square brackets in JSON, they denote an array. 
> 

OK - I can remove... Obviously it's the "optional" marking that I'm after...

>>       *   vdisk-id:"eb90327c-8302-4725-4e85ed4dc251",
>>       *   server:[{type:"tcp", host:"1.2.3.4", port:9999}]}
>>       */
>>      if (virJSONValueObjectCreate(&ret,
>>                                   "s:driver", protocol,
>> +                                 "S:tls-creds", src->tlsAlias,
>>                                   "s:vdisk-id", src->path,
>>                                   "a:server", server, NULL) < 0)
>>          virJSONValueFree(server);
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 0a3278510..7b98e1947 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -794,6 +794,32 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd,
>>  }
>>  
>>  
>> +/* qemuBuildDiskTLSx509CommandLine:
>> + *
>> + * Add TLS object if the disk uses a secure communication channel
>> + *
>> + * Returns 0 on success, -1 w/ error on some sort of failure.
>> + */
>> +static int
>> +qemuBuildDiskTLSx509CommandLine(virCommandPtr cmd,
>> +                                virQEMUDriverConfigPtr cfg,
>> +                                virDomainDiskDefPtr disk,
>> +                                virQEMUCapsPtr qemuCaps)
>> +{
>> +    virStorageSourcePtr src = disk->src;
>> +
>> +    /* other protocols may be added later */
>> +    if (src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS &&
>> +        disk->src->haveTLS == VIR_TRISTATE_BOOL_YES) {
>> +        return qemuBuildTLSx509CommandLine(cmd, cfg->vxhsTLSx509certdir,
>> +                                          false, true, false,
>> +                                          disk->info.alias, qemuCaps);
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +
>>  static char *
>>  qemuBuildNetworkDriveURI(virStorageSourcePtr src,
>>                           qemuDomainSecretInfoPtr secinfo)
> 
> [...]
> 
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index b365078ec..e4174af35 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -152,6 +152,55 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver,
>>  
>>  
>>  static int
>> +(virQEMUDriverPtr driver,
>> +                           virDomainObjPtr vm,
>> +                           virDomainDiskDefPtr disk,
>> +                           char **tlsAlias)
> 
> I don't particularly like this. You set it into the disk object, so
> there should be a function which rolls this back for a disk (or
> preferably a virStorageSource).
> 


Sure that's fine - the rollback code is so ugly anyway - this just adds
to its misery.

>> +{
>> +    int ret = -1;
>> +    virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver);
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    virStorageSourcePtr src = disk->src;
>> +    virJSONValuePtr tlsProps = NULL;
>> +
>> +    /* NB: This may alter haveTLS based on cfg */
>> +    qemuDomainPrepareDiskSourceTLS(src, disk->info.alias, cfg);
> 
> This should be executed in the hotplug API function and not in a helper.
> 

I was trying to avoid replication of code for each of the 3 disk types
in the event that at some future point more than one has TLS. I actually
prefer it here.

>> +
>> +    if (src->haveTLS != VIR_TRISTATE_BOOL_YES) {
>> +        ret = 0;
>> +        goto cleanup;
>> +    }
>> +
>> +    /* Initial implementation doesn't require/use a secret to decrypt
>> +     * a server certificate, so there's no need to manage a tlsSecAlias
>> +     * and tlsSecProps. See qemuDomainAddChardevTLSObjects for the
>> +     * methodology required to add a secret object. */
>> +
>> +    /* For a VxHS environment, create a TLS object for the client to
>> +     * connect to the VxHS server. */
>> +    if (src->type == VIR_STORAGE_TYPE_NETWORK &&
>> +        src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS &&
>> +        qemuDomainGetTLSObjects(priv->qemuCaps, NULL,
>> +                                cfg->vxhsTLSx509certdir, false, true,
>> +                                disk->info.alias, &tlsProps, tlsAlias,
>> +                                NULL, NULL) < 0)
> 
> This looks like another place that extracts qemu-specific stuff, which
> need to be duplicated for blockdev add. Can't we make the cert dir part
> of virStorageSource?
> 

Hmmmmm... I think we can do that... The qemuDomainPrepareDiskSourceTLS
would change to fill in some sort of src->TLSx509certdir based on
src->type and protocol which then would be used by this code.

And the only reason why @disk was passed was to get src and srcalias.  I
can adjust that too.

Tks -

John

> 
>> +        goto cleanup;
>> +
>> +    if (qemuDomainAddTLSObjects(driver, vm, QEMU_ASYNC_JOB_NONE,
>> +                                NULL, NULL, *tlsAlias, &tlsProps) < 0)
>> +        goto cleanup;
>> +
>> +    ret = 0;
>> +
>> + cleanup:
>> +    virJSONValueFree(tlsProps);
>> +    virObjectUnref(cfg);
>> +
>> +    return ret;
>> +}
>> +
>> +
>> +static int
>>  qemuHotplugWaitForTrayEject(virQEMUDriverPtr driver,
>>                              virDomainObjPtr vm,
>>                              virDomainDiskDefPtr disk,

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]
  Powered by Linux