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
