9:49 a.m.
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(a)veritas.com>
>
> 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(a)veritas.com>
> Signed-off-by: John Ferlan <jferlan(a)redhat.com>
> ---
> 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,