On Thu, Jun 29, 2017 at 19:02:39 -0700, Ashish Mittal wrote: [...] 3.6.0 at best. [...] This is misaligned. [...] Now we are in the section of stuff which should be split into a separate patch. Also here you declare that there,s no default port ... (and you said) And here you declare the default port via a ... define. I'd suggest you stick with the common code paths. If you want to add comments, please make them useful. Document arguments, and return values. This looks like it's generating a server structure filled with 'InetSocketAddressBase' qemu types. Since vxhs is not the only one using those, you can make this a generic function. Additionally it should return an array of those object in case when there's more than one host. This function should not fill any default ports, that's what the caller should do. Why have a cleanup label if there's no cleanup code? [...] Note that this will cause that external snapshots will not work with VXHS. I'm currently dealing with the same problem with multi-host gluster disks. [...] [...] Okay, fair enough, you expressly declare that you don't need them. Above, you've declared that URI strings are not supported by libvirt, I don't feel we need to parse them then. Umm, how is this even supposed to work then? Above you explicitly parse it as an URI [...] This belongs to the patch adding vxhs globally. [...] Again?!?. No. Define this globally. This never was supported in libvirt, so it's not necessary. Also in qemu this was added post 2.9.0 so I doubt it ever supported the 'filename' syntax. Use virStorageSourceParseBackingJSONInetSocketAddress instead of hand-rolling the code. This should not fill the defaults. This code should parse what's in the backing store. [...] There are at least 4 places where you say that URI syntax is not working .... [...] If you need to make functional changes to the test suite, please put them into a separate patch. This leaks the log buffer if it returns non-null. The fail label here should be called 'cleanup' per our standard, thus you don't need to add any new label. Just set ret to 0 and jump here. [...] This does not seem relevant. Wrong resolution of a merge conflict perhaps? So if this is not supposed to work, why bother adding it at all?

On 06/29/2017 10:02 PM, Ashish Mittal wrote: FWIW: I have another patch upstream: https://www.redhat.com/archives/libvir-list/2017-June/msg01341.html Which is making some textual and code changes to qemu.conf and qemu_conf.c - just so you are aware. In patch 3, the call to qemuBuildTLSx509CommandLine has a @verifypeer parameter = true, thus it's always going to expect to verify the client rather than making that determination using *_tls_x509_verify. So that means you have to describe the environment as requiring the client-cert.pem and client-key.pem files (like the description for default). Since you set the @addpasswordid to false on input, that just means the certificate server-key.pem file cannot be encrypted - something else that would need to be described. Based on these two configuration settings, that means if vxhs_tls=1, then vxhs_tls_x509_cert_dir must be set to something other than the default environment. IOW: both must be defined and that would need to be validated during config read and cause failure (e.g. vxhsTLSx509certdir != defaultTLSx509certdir IOW: You're essentially *requiring* someone to set this up and don't handle the/a default environment. Hopefully this makes sense, it's Friday afternoon and I have a TLS migraine ;-). John

On Fri, Jun 30, 2017 at 1:29 PM, John Ferlan <jferlan(a)redhat.com&gt; wrote: During the QEMU discussions on VxHS support for TLS, it was a agreed that it was not necessary to encrypt the cert files. The ideal way is for VxHS to be able to use the default TLS environment. These settings are for the VxHS client i.e. QEMU. VxHS server would be running on a remote host and would have it's own configuration settings. Could you please give me some pointers on what changes would be required to reuse the default TLS environment? I am hoping these would not necessitate any change in the qemu vxhs code. Thanks, Ashish

On Mon, Jul 17, 2017 at 4:50 PM, John Ferlan <jferlan(a)redhat.com&gt; wrote: As you have rightly noted later, we are actually setting up the client TLS env here (libvirt/qemu = vxhs client). I hope the default libvirt/qemu TLS env can be used for setting up the client connection also! If not, then I guess I just have to mandate a dedicated vxhs TLS env. vxhs server TLS env setup would happen separately. So the @verifypeer parameter = true in qemuBuildTLSx509CommandLine would apply to the client connection and be ignored anyway (is true by default for the client). If, on the vxhs server side, we were to set up the env with @verifypeer parameter = false, then the client (libvirt/qemu) would not need client-cert.pem and client-key.pem files. That said, we do desire authentication of the client, and the client-cert.pem and client-key.pem files would be needed in that case. I hope my understanding is not terribly wrong. Thanks for your response. I think I understand this better now! Please correct me if the understanding below is not accurate - (1) the default TLS environment could potentially use encrypted TLS certificates (I guess it's only the *key.pem files that would need to be encrypted). (2) if it is indeed using encrypted TLS key.pem files, then it passes along a secret password to decrypt the key files. This secret is passed along to QEMU for actual processing. Therefore, if I wanted to support secrets with vxhs, I would first have to change qemu code. (3) VxHS does not accept secrets, therefore it cannot use the default TLS env if it has been encrypted. (4) I guess, as you have pointed out, I would have to add code to check if the default TLS env is using secrets, and fail the operation if a dedicated VxHS env has not been provided in this scenario. That is correct! For the VxHS block device, qemu is the client and the vxhs block device server would be remote. We are setting up the qemu client TLS environment via libvirt. The vxhs server TLS setup would be done separately from within the vxhs server code. I will add try to add this test case. Thanks!

From: Ashish Mittal <ashish.mittal(a)veritas.com&gt; 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(a)veritas.com&gt; --- docs/formatdomain.html.in | 18 +++- docs/schemas/domaincommon.rng | 5 + src/conf/domain_conf.c | 19 ++++ src/qemu/qemu_command.c | 107 ++++++++++++++++++--- src/util/virstoragefile.c | 13 +++ src/util/virstoragefile.h | 9 ++ ...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 +++ 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 <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> <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> + </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 */ + 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'"), + 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 */ @@ -7830,6 +7844,7 @@ virDomainDiskSourceParse(xmlNodePtr node, cleanup: VIR_FREE(protocol); + VIR_FREE(haveTLS); ctxt->node = saveNode; return ret; } @@ -21266,6 +21281,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_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", + 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; +} + + +/* 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; +} + #define QEMU_DEFAULT_VXHS_PORT "9999" @@ -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) { + 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 @@ -2042,6 +2042,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; @@ -3231,6 +3233,7 @@ virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src, const char *uri = virJSONValueObjectGetString(json, "filename"); const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id"); virJSONValuePtr server = virJSONValueObjectGetObject(json, "server"); + const char *haveTLS = virJSONValueObjectGetString(json, "tls"); const char *hostname; const char *port; @@ -3258,6 +3261,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; + } + } + 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; }; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-err-vxhs.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-err-vxhs.xml new file mode 100644 index 0000000..951ad4e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-err-vxhs.xml @@ -0,0 +1,34 @@ +<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' tls='yes'> + <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'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' 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> 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 @@ -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,\ +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,\ +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.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,\ +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,\ +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'/> + <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'/> + <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'/> + <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> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args new file mode 100644 index 0000000..e1ad36e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args @@ -0,0 +1,28 @@ +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,\ +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 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.xml new file mode 100644 index 0000000..a488770 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.xml @@ -0,0 +1,34 @@ +<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'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' 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> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0a1ef01..7459522 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -904,6 +904,15 @@ mymain(void) DO_TEST("disk-drive-network-rbd-ipv6", NONE); DO_TEST_FAILURE("disk-drive-network-rbd-no-colon", NONE); DO_TEST("disk-drive-network-vxhs", NONE); + DO_TEST_FAILURE("disk-drive-network-tlsx509-err-vxhs", + QEMU_CAPS_OBJECT_TLS_CREDS_X509); + driver.config->vxhsTLS = 1; + DO_TEST("disk-drive-network-tlsx509-vxhs", + QEMU_CAPS_OBJECT_TLS_CREDS_X509); + DO_TEST("disk-drive-network-tlsx509-multidisk-vxhs", + QEMU_CAPS_OBJECT_TLS_CREDS_X509); + driver.config->vxhsTLS = 0; + VIR_FREE(driver.config->vxhsTLSx509certdir); DO_TEST("disk-drive-no-boot", QEMU_CAPS_BOOTINDEX); DO_TEST_PARSE_ERROR("disk-device-lun-type-invalid", diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 3a4e03b..28747ff 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1594,6 +1594,17 @@ mymain(void) TEST_BACKING_PARSE("json:{\"file.driver\":\"vxhs\"," "\"file.filename\":\"vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0\"" "}", NULL); + TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\"," + "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\"," + "\"server\": { \"host\":\"example.com\"," + "\"port\":\"1234\"" + "}," + "\"tls\":\"yes\"" + "}" + "}", + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0' tls='yes'>\n" + " <host name='example.com' port='1234'/>\n" + "</source>\n"); #endif /* WITH_YAJL */ cleanup: -- 2.5.5

On Fri, Jun 30, 2017 at 10:44:39 +0200, Peter Krempa wrote: [...] I forgot to finish my thought before sending. I think we want a separate element with an attribute at this point. This allows adding other TLS related stuff to it if such need arises. <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'/> <tls enabled='yes'/> </source> [...] </disk>

[...] So at least the conversation we had last week now makes a bit more sense w/r/t the call to qemuBuildTLSx509CommandLine for chardevTLSx509certdir. As I look at that code now quickly, although having multiple tls-cert-x509 objects for each chardev isn't necessary, it does "work" or start qemu because each would have a different alias (@charAlias is uniquely generated via qemuAliasChardevFromDevAlias). Theoretically speaking two objects wouldn't be required, except for the problem that hotunplug would have to be made aware and we'd have to keep track of when the last one was removed. So leaving with each having their own object is the way the code will stay. So given all that - your alias creation is going to have to contain that uuid or you are going to have to figure out a way to just have one object which each disk uses. You'll have to scan the disks looking to determine if any of the vxhs ones have tls and if so, break to add the object. Then add the 'tls-creds=$object_alias_id'. BTW: if you haven't already, read Dan Berrange's blog on TLS: https://www.berrange.com/posts/2016/04/01/improving-qemu-security-part-2-... that's a link to page 2, read/scan the remaining 5 blogs as well. Some of the exact qemu syntax has changed since the blog was written, but the description is really what helps the frame of reference. [...] Not only that, but dir=/usr/local/etc/pki/qemu causes qemuxml2argvtest to fail for me since the default is supposed to be /etc/pki/libvirt-vxhs John

On Fri, Jun 30, 2017 at 2:58 PM, ashish mittal <ashmit602(a)gmail.com&gt; wrote:

On 07/11/2017 09:43 PM, ashish mittal wrote: Both Peter and I have been wrapped up in something a bit more pressing lately, but figured I'd take a look so you're not left wondering too long. Typically something like this ^^^ would be turned into a helper so there's no need to store @aliasTLS. My suggestion would be to use qemuAliasChardevFromDevAlias as a guide. Create a qemu_alias.c helper/API that allows passing 2 parameters - one the "disk->src->protocol" and the other the "disk->info.alias". Then in the function the protocol would be turned into a string via virStorageNetProtocolTypeToString and the created alias can be "%s_%s" (so you don't have to change your existing .args output). This way if "iscsi" or "rbd" or whatever comes along some day, they'd just pass src->protocol too and magically the string is created with teh "vxhs" or "iscsi" or "rbd" (etc) prefixdisk. BTW: A similar argument could be made for the qemuParseVxHSString change you have. Similar to the @protocol in qemuBuildVxHSDriveJSON Since you can generate the alias at any time, the aliasTLS would be unnecessary. John