On Thu, Jun 29, 2017 at 19:02:41 -0700, Ashish Mittal wrote:
> From: Ashish Mittal <ashish.mittal@xxxxxxxxxxx>
>
> 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,
> the "default_tls_x509_cert_dir" will be used instead.
> (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.
> (6) Valid values for domain TLS setting are tls='yes|no'.
> (7) tls='yes' can only be specified if "vxhs_tls" is enabled.
> Specifying tls='yes' when "vxhs_tls=0" results in an error.
> (8) Test cases have been added to validate points (4), (5) and (7).
> Test case also added to confirm that JSON arguments containing
> tls attribute are parsed correctly.
>
> 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@xxxxxxxxxxx>
> ---
Too much stuff is going on in this patch. You need to split it.
> docs/formatdomain.html.in | 18 +++-
> docs/schemas/domaincommon.rng | 5 +
> src/conf/domain_conf.c | 19 ++++
> src/qemu/qemu_command.c | 107 ++++++++++++++++++---
The addition to the qemu driver should be separate too.
> src/util/virstoragefile.c | 13 +++
> src/util/virstoragefile.h | 9 ++
Changes to the backing store parser should be in the separate patch.
> ...ml2argv-disk-drive-network-tlsx509-err-vxhs.xml | 34 +++++++
> ...-disk-drive-network-tlsx509-multidisk-vxhs.args | 41 ++++++++
> ...k-drive-network-tlsx509-multidisk-vxhs.args.new | 41 ++++++++
> ...v-disk-drive-network-tlsx509-multidisk-vxhs.xml | 56 +++++++++++
> ...muxml2argv-disk-drive-network-tlsx509-vxhs.args | 28 ++++++
> ...emuxml2argv-disk-drive-network-tlsx509-vxhs.xml | 34 +++++++
> tests/qemuxml2argvtest.c | 9 ++
> tests/virstoragetest.c | 11 +++
Plus these belong to the respective patches.
> 14 files changed, 413 insertions(+), 12 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-err-vxhs.xml
> 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.args.new
> 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
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 62d67f4..86808e5 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -2511,7 +2511,7 @@
> target's name by a slash (e.g.,
> <code>iqn.2013-07.com.example:iscsi-pool/1</code>). If not
> specified, the default LUN is zero.
> - For "vxhs" (<span class="since">since 3.3.0</span>), the
> + For "vxhs" (<span class="since">since 3.3.1</span>), the
There won't be any 3.3.1 release. The upcomming release is 3.5.0, and
your patches will be in 3.6.0 at best since we are already in freeze
state.
Also this hunk is misplaced from one of the earlier patches probably.
> <code>name</code> is the UUID of the volume, assigned by the
> HyperScale sever.
> <span class="since">Since 0.8.7</span>
> @@ -2630,6 +2630,22 @@
> transport is "unix", the socket attribute specifies the path to an
> AF_UNIX socket.
> </p>
> + <p>
> + <span class="since">Since 3.3.1,</span> the optional attribute
> + <code>tls</code> (QEMU only) can be used to control whether a vxhs
> + network block device would utilize a hypervisor configured
> + TLS X.509 certificate environment in order to encrypt the data
> + channel. For the QEMU hypervisor, usage of a TLS environment can
> + be controlled on the host by the <code>vxhs_tls</code> and
> + <code>vxhs_tls_x509_cert_dir</code> or
> + <code>default_tls_x509_cert_dir</code> settings in the file
> + /etc/libvirt/qemu.conf. If <code>vxhs_tls</code> is enabled,
> + then unless the domain <code>tls</code> attribute is set to "no",
> + libvirt will use the host configured TLS environment.
> + If <code>vxhs_tls</code> is disabled, but the <code>tls</code>
> + attribute is set to "yes" in the device domain specification,
> + then libvirt will throw an error.
> + </p>
> </dd>
This seems misplaced too. You really need to split the addition of TLS
stuff to disks from the vxhs impl.
> <dt><code>snapshot</code></dt>
> <dd>
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 7525a2a..909af50 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1622,6 +1622,11 @@
> </attribute>
> <attribute name="name"/>
> <ref name="diskSourceNetworkHost"/>
> + <optional>
> + <attribute name="tls">
> + <ref name="virYesNo"/>
> + </attribute>
Make this a definition for future reuse. Additionally I think that the
TLS part should be a separate element here. Something like
<disk>
<source>
> + </optional>
> </element>
> </define>
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index c3149f9..34d8451 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -7745,6 +7745,7 @@ virDomainDiskSourceParse(xmlNodePtr node,
> int ret = -1;
> char *protocol = NULL;
> xmlNodePtr saveNode = ctxt->node;
> + char *haveTLS = NULL;
>
> ctxt->node = node;
>
> @@ -7778,6 +7779,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 */
[1]
> + if ((haveTLS = virXMLPropString(node, "tls")) &&
> + src->protocol == VIR_STORAGE_NET_PROTOCOL_VXHS) {
> + if ((src->haveTLS =
> + virTristateBoolTypeFromString(haveTLS)) <= 0) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("unknown VxHS 'tls' setting '%s'"),
While this currently is implemented only for vxhs, the TLS protocol can
be uin the future used with other storage, like NBD, thus please don't
hardcode the name here.
[1] You admit it yourself here.
> + haveTLS);
> + goto cleanup;
> + }
> + }
This also looks like it should belong to a separate patch.
> +
> /* 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 */
> @@ -7830,6 +7844,7 @@ virDomainDiskSourceParse(xmlNodePtr node,
>
> cleanup:
> VIR_FREE(protocol);
> + VIR_FREE(haveTLS);
> ctxt->node = saveNode;
> return ret;
> }
[...]
> 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.
> + 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;
> +}
[...]
> @@ -975,18 +1037,38 @@ qemuBuildVxHSDriveJSON(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);
> + 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) {
You can use virJSONValueObjectAdd with the same syntax instead of having
two paths adding most of the same stuff.
> + virJSONValueFree(server);
> + ret = NULL;
> + }
> + 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;
> + }
> + }
> +
> + cleanup:
> return ret;
> }
>
> @@ -2438,6 +2520,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 eb36694..449ace4 100644
> --- a/src/util/virstoragefile.c
> +++ b/src/util/virstoragefile.c
[...]
> @@ -3258,6 +3261,16 @@ virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src,
> return -1;
> }
>
> + if (haveTLS) {
> + if ((src->haveTLS =
> + virTristateBoolTypeFromString(haveTLS)) <= 0) {
This is more a question to the qemu implementation. Why is this a string
and not a boolean?! JSON has native support for booleans.
I suggest you fix the qemu implementation first and use a proper boolean
here.
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("unknown VxHS 'tls' setting '%s'"),
> + haveTLS);
> + return -1;
> + }
> + }
> +
> if (!port)
> port = QEMU_DEFAULT_VXHS_PORT;
>
> diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
> index 0b6e409..e586170 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 *nodebacking; /* name of the backing 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;
I don't quite understand the point of the two flags above. Also any of
the TLS stuff should be in a separate patch.
> };
> 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.
> +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>
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
