[libvirt] [PATCH v5 5/9] Add TLS support for Veritas HyperScale (VxHS) block device protocol
John Ferlan
jferlan at redhat.com
Tue Aug 29 23:20:37 UTC 2017
On 08/29/2017 02:39 AM, Ashish Mittal wrote:
> The following describes the behavior of TLS for VxHS block device:
>
> (1) Two new options have been added in /etc/libvirt/qemu.conf
> to control TLS behavior with VxHS block devices
> "vxhs_tls" and "vxhs_tls_x509_cert_dir".
> (2) Setting "vxhs_tls=1" in /etc/libvirt/qemu.conf will enable
> TLS for VxHS block devices.
> (3) "vxhs_tls_x509_cert_dir" can be set to the full path where the
> TLS certificates and keys are saved. If this value is missing,
TLS CA certificate and the client certificate and keys are saved.
> the "default_tls_x509_cert_dir" will be used instead.
These three points I think belong in the previous patch commit message...
> (4) If the value of "vxhs_tls" is set to 1, TLS creds will be added
> automatically on the qemu command line for every VxHS
> block device.
> (5) With "vxhs_tls=1", TLS may selectively be disabled on individual
> VxHS disks by specifying tls='no' in the device domain
> specification.
(4) and (5) somewhat contradict each other, but we can work through
this. Additionally, let's use cfg->vxhsTLS or disk->source->haveTLS
What this patch does is use the config->vxhsTLS along with the
introduced disk->src->haveTLS in order to add TLS data to the VxHS
client connection.
> (6) Valid values for domain TLS setting are tls='yes|no'.
s/domain/domain disk source/
> (7) tls='yes' can only be specified if "vxhs_tls" is enabled.
> Specifying tls='yes' when "vxhs_tls=0" results in an error.
I'm still not clear why this is a requirement if in fact you can really
use the default qemu environment.
>
> QEMU changes for VxHS (including TLS support) are already upstream.
>
> Sample TLS args generated by libvirt -
> -object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\
> 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
>
> Signed-off-by: Ashish Mittal <Ashish.Mittal at veritas.com>
> ---
> v5 changelog:
> (1) The v4 3/3 patch has been split into smaller chunks.
> (2) Functionally there are no changes in TLS code yet.
>
> TODO: Changes to TLS functionality are pending.
>
> docs/schemas/domaincommon.rng | 5 ++++
> src/conf/domain_conf.c | 19 ++++++++++++
> src/qemu/qemu_block.c | 42 +++++++++++++++++++--------
> src/qemu/qemu_command.c | 67 +++++++++++++++++++++++++++++++++++++++++++
> src/util/virstoragefile.c | 13 +++++++++
> src/util/virstoragefile.h | 9 ++++++
> 6 files changed, 143 insertions(+), 12 deletions(-)
>
So this patch is where things are going to get tricky.
On one hand, trying to create a generic means to add/manage a 'tls'
attribute for a disk source needs to be considered; however, this patch
implements it strictly for VxHS, which may not be the best thing.
Thus, I think there needs to be a patch prior to this that adds a
generic/optional "tls" attribute to diskSourceNetworkHost. No one will
use it yet, but it'll be there.
Part of that patch would modify/introduce qemuDomainPrepareDiskSource to
be run after qemuDomainPrepareChardevSource and before
qemuDomainSecretPrepare. It's sole purpose would be to run through the
disk array looking for disks to set up
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 458b8d8..af38c9a 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1651,6 +1651,11 @@
> </attribute>
> <attribute name="name"/>
> <ref name="diskSourceNetworkHost"/>
> + <optional>
> + <attribute name="tls">
> + <ref name="virYesNo"/>
> + </attribute>
> + </optional>
I think rather than here in the VxHS specific code - perhaps we add tls
to the diskSourceNetworkHost. I need to investigate that. If that's
done, then a patch before this one would be created.
> </element>
> </define>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 5bad397..f3fb3d0 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8017,6 +8017,7 @@ virDomainDiskSourceParse(xmlNodePtr node,
> int ret = -1;
> char *protocol = NULL;
> xmlNodePtr saveNode = ctxt->node;
> + char *haveTLS = NULL;
>
> ctxt->node = node;
>
> @@ -8050,6 +8051,19 @@ virDomainDiskSourceParse(xmlNodePtr node,
> goto cleanup;
> }
>
> + /* Check tls=yes|no domain setting for the block device */
> + /* At present only VxHS. Other block devices may be added later */
Adjust the above to be a multi-line comment... e.g.
/* words
* more words */
It's just a consistency thing.
> + if ((haveTLS = virXMLPropString(node, "tls")) &&
> + src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS) {
flip-flop conditional - no need to check for it for other protocols
(yet)... Besides that's a longer trip - let's make the quick check first.
> + if ((src->haveTLS =
> + virTristateBoolTypeFromString(haveTLS)) <= 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("unknown VxHS 'tls' setting '%s'"),
Adjust to remove "VxHS" since it could be generic some day... Replace
with "disk source"
> + haveTLS);
> + goto cleanup;
> + }
> + }
> +
> /* for historical reasons the volume name for gluster volume is stored
> * as a part of the path. This is hard to work with when dealing with
> * relative names. Split out the volume into a separate variable */
> @@ -8105,6 +8119,7 @@ virDomainDiskSourceParse(xmlNodePtr node,
>
> cleanup:
> VIR_FREE(protocol);
> + VIR_FREE(haveTLS);
> ctxt->node = saveNode;
> return ret;
> }
> @@ -21534,6 +21549,10 @@ virDomainDiskSourceFormatNetwork(virBufferPtr buf,
>
> VIR_FREE(path);
>
> + if (src->haveTLS != VIR_TRISTATE_BOOL_ABSENT)
> + virBufferAsprintf(buf, " tls='%s'",
> + virTristateBoolTypeToString(src->haveTLS));
> +
> if (src->nhosts == 0 && !src->snapshot && !src->configFile) {
> virBufferAddLit(buf, "/>\n");
> } else {
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index a4d0160..766d07f 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
And now we add in the qemu_alias.h include
> @@ -519,20 +519,38 @@ qemuBlockStorageSourceGetVxHSProps(virStorageSourcePtr src)
> if (!(server = qemuBuildVxHSDriveJSONHost(src)))
> return NULL;
>
> - /* VxHS disk specification example:
> - * { driver:"vxhs",
> - * vdisk-id:"eb90327c-8302-4725-4e85ed4dc251",
> - * server.host:"1.2.3.4",
> - * server.port:1234}
> - */
> - if (virJSONValueObjectCreate(&ret,
> - "s:driver", protocol,
> - "s:vdisk-id", src->path,
> - "a:server", server, NULL) < 0) {
> - virJSONValueFree(server);
> - ret = NULL;
> + if (src->addTLS == true) {
> + char *objalias = NULL;
> +
> + if (!(objalias = qemuAliasTLSObjFromSrcAlias("vxhs")))
> + goto cleanup;
> +
> + if (virJSONValueObjectCreate(&ret,
> + "s:driver", protocol,
> + "s:tls-creds", objalias,
> + "s:vdisk-id", src->path,
> + "a:server", server, NULL) < 0) {
> + virJSONValueFree(server);
> + ret = NULL;
NB: The setting of ret = NULL on the error path was unnecessary
> + }
> + VIR_FREE(objalias);
> + } else {
> + /* VxHS disk specification example:
> + * { driver:"vxhs",
> + * vdisk-id:"eb90327c-8302-4725-4e85ed4dc251",
> + * server.host:"1.2.3.4",
> + * server.port:1234}
> + */
> + if (virJSONValueObjectCreate(&ret,
> + "s:driver", protocol,
> + "s:vdisk-id", src->path,
> + "a:server", server, NULL) < 0) {
> + virJSONValueFree(server);
> + ret = NULL;
> + }
Once you dig down into the depths of virJSONValueObjectCreate you'd find
that virJSONValueObjectAddVArgs will allow passing a NULL argument by
using a capital letter. If 'S:tls-creds' is used, then if tls-creds ==
NULL, then we don't print the string. So rather than the hunk above, we
can modify the:
if (virJSONValueObjectCreate(&ret,
"s:driver", protocol,
"s:vdisk-id", src->path,
"a:server", server, NULL) < 0) {
to be
if (virJSONValueObjectCreate(&ret,
"s:driver", protocol,
"S:tls-creds", objalias,
"s:vdisk-id", src->path,
"a:server", server, NULL) < 0) {
and only add:
if (src->addTLS &&
!(objalias = qemuAliasTLSObjFromSrcAlias("vxhs"))) {
virJSONValueFree(server);
return NULL;
}
after the fetching of server. Whether "vxhs" will be unique enough
remains to be thought about too.
> }
>
> + cleanup:
and with all the above done correctly, cleanup: is unnecessary
> return ret;
> }
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 0fd2674..384a489 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -791,6 +791,70 @@ qemuBuildTLSx509CommandLine(virCommandPtr cmd,
> }
>
>
> +
> +/* 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) {
s/ == true//
> + disk->src->addTLS = true;
Comparatively to chardev, see qemuDomainPrepareChardevSourceTLS
Don't believe this will be necessary, the default being if not
configured as "NO" then if the system configures it, it will be
configured used, so no need to modify the domain to say you want it.
I think we'll need to add that - especially if we ever consider hot
plug/unplug.
> + ret = qemuBuildTLSx509CommandLine(cmd, cfg->vxhsTLSx509certdir,
> + false,
> + true,
By setting this to true here requires that the environment that was
configured will have the server verify the client as per the various
verify descriptions in qemu.conf.
> + false,
> + "vxhs",
> + qemuCaps);
There's some extraneous indent/spacing issues...
> + } else if (cfg->vxhsTLS == false &&
s/cfg->vxhsTLS == false &&/!cfg->vxhsTLS/
> + 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;
And this begs the question why? It's the whole reason for a "default"
environment. The error message is also way too long...
Still this type of check is better left to *Prepare* type logic. The
more recent "desire" in the qemu_command code is not fail on
configuration errors, but fail only on command generation errors.
> + }
> +
> + return ret;
> +}
> +
> +
> +/* qemuBuildDiskTLSinfoCommandLine:
> + *
> + * 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
> +qemuBuildDiskTLSinfoCommandLine(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)
> + return qemuBuildDiskVxHSTLSinfoCommandLine(cmd, cfg, disk, qemuCaps);
> +
> + return 0;
> +}
> +
> +
> static char *
> qemuBuildNetworkDriveURI(virStorageSourcePtr src,
> qemuDomainSecretInfoPtr secinfo)
> @@ -2218,6 +2282,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
> if (qemuBuildDiskSecinfoCommandLine(cmd, encinfo) < 0)
> return -1;
>
> + if (qemuBuildDiskTLSinfoCommandLine(cmd, cfg, disk, qemuCaps) < 0)
> + return -1;
> +
> virCommandAddArg(cmd, "-drive");
>
> if (!(optstr = qemuBuildDriveStr(disk, cfg, driveBoot, qemuCaps)))
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index e9a59e0..d4f0fdb 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
> @@ -2039,6 +2039,8 @@ virStorageSourceCopy(const virStorageSource *src,
> ret->physical = src->physical;
> ret->readonly = src->readonly;
> ret->shared = src->shared;
> + ret->haveTLS = src->haveTLS;
> + ret->addTLS = src->addTLS;
>
> /* storage driver metadata are not copied */
> ret->drv = NULL;
> @@ -3219,6 +3221,7 @@ virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src,
> {
> const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id");
> virJSONValuePtr server = virJSONValueObjectGetObject(json, "server");
> + const char *haveTLS = virJSONValueObjectGetString(json, "tls");
>
> if (!vdisk_id || !server) {
> virReportError(VIR_ERR_INVALID_ARG, "%s",
> @@ -3227,6 +3230,16 @@ virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src,
> return -1;
> }
>
> + if (haveTLS) {
> + if ((src->haveTLS =
> + virTristateBoolTypeFromString(haveTLS)) <= 0) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("unknown VxHS 'tls' setting '%s'"),
> + haveTLS);
> + return -1;
> + }
> + }
> +
> src->type = VIR_STORAGE_TYPE_NETWORK;
> src->protocol = VIR_STORAGE_NET_PROTOCOL_VXHS;
>
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index f7e897f..0f363a7 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -281,6 +281,15 @@ struct _virStorageSource {
> /* metadata that allows identifying given storage source */
> char *nodeformat; /* name of the format handler object */
> char *nodestorage; /* name of the storage object */
> +
> + /* This is the domain specific setting.
> + * It may be absent */
> + int haveTLS; /* enum virTristateBool */
> +
> + /* This should be set to "true" only when TLS creds are to be added for
> + * the device. For e.g. this could be based on a combination of
> + * global conf setting + domain specific setting */
> + bool addTLS;
Not convinced yet 'addTLS' is needed - need to do some more thinking on
this. This has already been a day long exercise - so I'll stop at this
patch. I think we can get everything through the TLS changes merged
into libvirt upstream once 3.8.0 opens for business. Whether more is
ready depends on how much progress we can make.
John
> };
>
>
>
More information about the libvir-list
mailing list