Hi Peter, Patch series V5 addresses most of the review comments from this email. I'm working on the other set of review comments from you and John, and will send updates soon. On Fri, Jun 30, 2017 at 1:22 AM, Peter Krempa <pkrempa@redhat.com> wrote:

On Thu, Jun 29, 2017 at 19:02:39 -0700, Ashish Mittal wrote:

...

From: Ashish Mittal <ashish.mittal@veritas.com>

Sample XML for a VxHS disk:

<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>

Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com> --- v2 changelog: (1) Added code for JSON parsing of a VxHS vdisk. (2) Added test case to verify JSON parsing. (3) Added missing switch-case checks for VIR_STORAGE_NET_PROTOCOL_VXHS. (4) Fixed line wrap in qemuxml2argv-disk-drive-network-vxhs.args.

v3 changelog: (1) Implemented the modern syntax for VxHS disk specification. (2) Changed qemuxml2argvdata VxHS test case to verify the new syntax. (3) Added a negative test case to check failure when multiple hosts are specified for a VxHS disk.

v4 changelog: (1) Fixes per review comments from v3. (2) Had to remove a test from the previous version that checked for error when multiple hosts are specified for VxHS device. This started failing virschematest with the error "XML document failed to validate against schema" as the docs/schemas/domain.rng specifies only a single host.

docs/formatdomain.html.in | 15 ++++- docs/schemas/domaincommon.rng | 13 ++++ src/libxl/libxl_conf.c | 1 + src/qemu/qemu_command.c | 70 ++++++++++++++++++++++ src/qemu/qemu_driver.c | 3 + src/qemu/qemu_parse_command.c | 25 ++++++++ src/util/virstoragefile.c | 64 +++++++++++++++++++- src/util/virstoragefile.h | 1 + src/xenconfig/xen_xl.c | 1 + .../qemuargv2xml-disk-drive-network-vxhs-fail.args | 24 ++++++++ tests/qemuargv2xmltest.c | 17 +++++- .../qemuxml2argv-disk-drive-network-vxhs.args | 25 ++++++++ .../qemuxml2argv-disk-drive-network-vxhs.xml | 34 +++++++++++ tests/qemuxml2argvtest.c | 1 + tests/virstoragetest.c | 19 ++++++ 15 files changed, 308 insertions(+), 5 deletions(-) create mode 100644 tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-vxhs.xml

diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 36bea67..62d67f4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in

[...]

...

@@ -2511,6 +2511,9 @@ 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>), thea

3.6.0 at best.

Changed to 3.8.0 in all places.

...

+ <code>name</code> is the UUID of the volume, assigned by the + HyperScale sever. <span class="since">Since 0.8.7</span> </dd> <dt><code>volume</code></dt>

[...]

...

diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index bdf7103..7525a2a 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1613,6 +1613,18 @@ </element> </define>

+ <define name="diskSourceNetworkProtocolVxHS"> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>vxhs</value> + </choice> + </attribute> + <attribute name="name"/> + <ref name="diskSourceNetworkHost"/>

This is misaligned.

Fixed.

...

+ </element> + </define> + <define name="diskSourceNetwork"> <attribute name="type"> <value>network</value>

[...]

...

diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c53ab97..8e00782 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -524,6 +524,7 @@ qemuNetworkDriveGetPort(int protocol, return 0;

case VIR_STORAGE_NET_PROTOCOL_RBD: + case VIR_STORAGE_NET_PROTOCOL_VXHS: case VIR_STORAGE_NET_PROTOCOL_LAST: case VIR_STORAGE_NET_PROTOCOL_NONE: /* not applicable */

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)

qemuNetworkDriveGetPort() NA in the latest code. Added to virStorageSourceNetworkDefaultPort()

...

@@ -931,6 +932,65 @@ qemuBuildGlusterDriveJSON(virStorageSourcePtr src) }

+#define QEMU_DEFAULT_VXHS_PORT "9999"

And here you declare the default port via a ... define. I'd suggest you stick with the common code paths.

The now-defunct qemuNetworkDriveGetPort() was not used in the JSON code path, hence I had omitted it. Added to virStorageSourceNetworkDefaultPort() in the latest code.

...

+ +/* Build the VxHS host object */

If you want to add comments, please make them useful. Document arguments, and return values.

...

+static virJSONValuePtr +qemuBuildVxHSDriveJSONHost(virStorageSourcePtr src) +{ + virJSONValuePtr server = NULL; + virStorageNetHostDefPtr host; + const char *portstr; + + if (src->nhosts != 1) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("protocol VxHS accepts only one host")); + goto cleanup; + } + + host = src->hosts; + portstr = host->port; + + if (!portstr) + portstr = QEMU_DEFAULT_VXHS_PORT; + + if (virJSONValueObjectCreate(&server, + "s:host", host->name, + "s:port", portstr, + NULL) < 0)

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.

I hope this can be deferred for later. I guess this will affect non-vxhs protocols as well.

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.

...

+ server = NULL; + + cleanup:

Why have a cleanup label if there's no cleanup code?

Changed.

...

+ return server; +} + +

[...]

...

@@ -1136,6 +1196,11 @@ qemuBuildNetworkDriveStr(virStorageSourcePtr src, ret = virBufferContentAndReset(&buf); break;

+ case VIR_STORAGE_NET_PROTOCOL_VXHS: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'VxHS' protocol does not support URI syntax")); + goto cleanup;

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.

...

+ case VIR_STORAGE_NET_PROTOCOL_SSH: virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("'ssh' protocol is not yet supported"));

[...]

...

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index cdb727b..d43de69 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c

[...]

...

@@ -13746,6 +13747,7 @@ qemuDomainSnapshotPrepareDiskExternalOverlayActive(virDomainSnapshotDiskDefPtr d case VIR_STORAGE_NET_PROTOCOL_FTPS: case VIR_STORAGE_NET_PROTOCOL_TFTP: case VIR_STORAGE_NET_PROTOCOL_SSH: + case VIR_STORAGE_NET_PROTOCOL_VXHS: case VIR_STORAGE_NET_PROTOCOL_LAST: virReportError(VIR_ERR_INTERNAL_ERROR, _("external active snapshots are not supported on "

Okay, fair enough, you expressly declare that you don't need them.

...

diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index af9063c..aa15225 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -263,6 +263,17 @@ qemuParseNBDString(virDomainDiskDefPtr disk) return -1; }

+static int +qemuParseVxHSString(virDomainDiskDefPtr def) +{ + virURIPtr uri = NULL; + + if (!(uri = virURIParse(def->src->path))) + return -1; + + return qemuParseDriveURIString(def, uri, "vxhs");

Above, you've declared that URI strings are not supported by libvirt, I don't feel we need to parse them then.

Removed.

...

+} +

/* * This method takes a string representing a QEMU command line ARGV set @@ -737,6 +748,11 @@ qemuParseCommandLineDisk(virDomainXMLOptionPtr xmlopt, if (VIR_STRDUP(def->src->path, vdi) < 0) goto error; } + } else if (STRPREFIX(def->src->path, "vxhs:")) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("VxHS protocol does not support URI syntax '%s'"), + def->src->path);

Umm, how is this even supposed to work then? Above you explicitly parse it as an URI

Changed accordingly.

...

+ goto error; } else { def->src->type = VIR_STORAGE_TYPE_FILE; }

[...]

...

diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index f0ed5c6..eb36694 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -85,7 +85,8 @@ VIR_ENUM_IMPL(virStorageNetProtocol, VIR_STORAGE_NET_PROTOCOL_LAST, "ftp", "ftps", "tftp", - "ssh") + "ssh", + "vxhs")

VIR_ENUM_IMPL(virStorageNetHostTransport, VIR_STORAGE_NET_HOST_TRANS_LAST, "tcp",

This belongs to the patch adding vxhs globally.

This is the patch adding vxhs functionality. Hopefully this is the correct place for this!

[...]

...

@@ -3219,6 +3221,65 @@ virStorageSourceParseBackingJSONRaw(virStorageSourcePtr src, return virStorageSourceParseBackingJSONInternal(src, json); }

+#define QEMU_DEFAULT_VXHS_PORT "9999"

Again?!?. No. Define this globally.

Removed.

...

+ +static int +virStorageSourceParseBackingJSONVxHS(virStorageSourcePtr src, + virJSONValuePtr json, + int opaque ATTRIBUTE_UNUSED) +{ + const char *uri = virJSONValueObjectGetString(json, "filename"); + const char *vdisk_id = virJSONValueObjectGetString(json, "vdisk-id"); + virJSONValuePtr server = virJSONValueObjectGetObject(json, "server"); + const char *hostname; + const char *port; + + /* Check for legacy URI based syntax passed via 'filename' option */ + if (uri) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("'VxHS' protocol does not support URI syntax")); + return -1; + }

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.

Removed.

...

+ + if (!vdisk_id || !server) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing 'vdisk-id' or 'server' attribute in " + "JSON backing definition for VxHS volume")); + return -1; + } + + hostname = virJSONValueObjectGetString(server, "host"); + port = virJSONValueObjectGetString(server, "port");

Use virStorageSourceParseBackingJSONInetSocketAddress instead of hand-rolling the code.

Changed accordingly.

...

+ + if (!hostname) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("missing hostname for tcp backing server in " + "JSON backing definition for VxHS volume")); + return -1; + } + + if (!port) + port = QEMU_DEFAULT_VXHS_PORT;

This should not fill the defaults. This code should parse what's in the backing store.

Changed.

[...]

...

diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args new file mode 100644 index 0000000..f6e3e37 --- /dev/null +++ b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-vxhs-fail.args @@ -0,0 +1,24 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/libexec/qemu-kvm \ +-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 \ +-drive file=vxhs://192.168.0.1:9999/eb90327c-8302-4725-9e1b-4e85ed4dc251,\

There are at least 4 places where you say that URI syntax is not working ....

This test case was to test failure when URI is encountered, but I see your point. Since the URI syntax will never exist, we don't need to test it.

...

+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/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 1adbcfe..fc15714 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -50,6 +50,7 @@ static int testSanitizeDef(virDomainDefPtr vmdef)

[...]

If you need to make functional changes to the test suite, please put them into a separate patch.

The above failure test has been removed from qemuargv2xmltest. Therefore the test fail changes are no longer needed. Removed.

...

typedef enum { FLAG_EXPECT_WARNING = 1 << 0, + FLAG_EXPECT_FAIL = 1 << 1, } virQemuXML2ArgvTestFlags;

static int testCompareXMLToArgvFiles(const char *xmlfile, @@ -67,7 +68,16 @@ static int testCompareXMLToArgvFiles(const char *xmlfile,

if (!(vmdef = qemuParseCommandLineString(driver.caps, driver.xmlopt, cmd, NULL, NULL, NULL))) - goto fail; + { + if (flags & FLAG_EXPECT_FAIL) { + if (virTestLogContentAndReset() == NULL)

This leaks the log buffer if it returns non-null.

NA now.

...

+ goto fail; + + VIR_TEST_DEBUG("Got expected error from " + "qemuParseCommandLineString:\n"); + goto out; + } + }

if (!virTestOOMActive()) { if ((log = virTestLogContentAndReset()) == NULL) @@ -106,6 +116,7 @@ static int testCompareXMLToArgvFiles(const char *xmlfile, if (virTestCompareToFile(actualxml, xmlfile) < 0) goto fail;

+ out: ret = 0;

fail:

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.

NA anymore.

...

@@ -166,6 +177,9 @@ mymain(void) # define DO_TEST(name) \ DO_TEST_FULL(name, 0)

+# define DO_TEST_FAIL(name) \ + DO_TEST_FULL(name, FLAG_EXPECT_FAIL) + setenv("PATH", "/bin", 1); setenv("USER", "test", 1); setenv("LOGNAME", "test", 1); @@ -220,6 +234,7 @@ mymain(void) /* older format using CEPH_ARGS env var */ DO_TEST("disk-drive-network-rbd-ceph-env"); DO_TEST("disk-drive-network-sheepdog"); + DO_TEST_FAIL("disk-drive-network-vxhs-fail"); DO_TEST("disk-usb"); DO_TEST("graphics-vnc"); DO_TEST("graphics-vnc-socket");

[...]

...

diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index f344083..3a4e03b 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1575,6 +1575,25 @@ mymain(void) "<source protocol='sheepdog' name='test'>\n" " <host name='example.com' port='321'/>\n" "</source>\n"); + TEST_BACKING_PARSE("json:{ \"file\": { " + "\"driver\": \"raw\"," + "\"file\": {" + "\"driver\": \"file\"," + "\"filename\": \"/path/to/file\" } } }", + "<source file='/path/to/file'/>\n");

This does not seem relevant. Wrong resolution of a merge conflict perhaps?

Yes. I have removed this now.

...

+ TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"vxhs\"," + "\"vdisk-id\":\"c6718f6b-0401-441d-a8c3-1f0064d75ee0\"," + "\"server\": { \"host\":\"example.com\"," + "\"port\":\"1234\"" + "}" + "}" + "}", + "<source protocol='vxhs' name='c6718f6b-0401-441d-a8c3-1f0064d75ee0'>\n" + " <host name='example.com' port='1234'/>\n" + "</source>\n"); + TEST_BACKING_PARSE("json:{\"file.driver\":\"vxhs\"," + "\"file.filename\":\"vxhs://192.168.0.1:9999/c6718f6b-0401-441d-a8c3-1f0064d75ee0\"" + "}", NULL);

So if this is not supposed to work, why bother adding it at all?

Removed. Thanks, Ashish

On Fri, Jun 30, 2017 at 1:29 PM, John Ferlan <jferlan@redhat.com> wrote:

On 06/29/2017 10:02 PM, Ashish Mittal wrote:

...

From: Ashish Mittal <ashish.mittal@veritas.com>

Add a new TLS X.509 certificate type - "vxhs". This will handle the creation of a TLS certificate capability for properly configured VxHS network block device clients.

Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com> --- Changelog:

(1) Add two new options 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.

src/qemu/libvirtd_qemu.aug | 4 ++++ src/qemu/qemu.conf | 23 +++++++++++++++++++++++ src/qemu/qemu_conf.c | 7 +++++++ src/qemu/qemu_conf.h | 3 +++ src/qemu/test_libvirtd_qemu.aug.in | 2 ++ 5 files changed, 39 insertions(+)

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.

I guess that means I might have to rebase and resolve any potential conflicts in the near future :) It was for the same reason I was hoping that the base VxHS functionality (patch 1) could be merged first. I could then add TLS support in subsequent patches. QEMU vxhs code does support using VxHS devices without TLS.

...

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index e1983d1..c19bf3a 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -115,6 +115,9 @@ module Libvirtd_qemu =

let memory_entry = str_entry "memory_backing_dir"

+ let vxhs_entry = bool_entry "vxhs_tls" + | str_entry "vxhs_tls_x509_cert_dir" + (* Each entry in the config is one of the following ... *) let entry = default_tls_entry | vnc_entry @@ -133,6 +136,7 @@ module Libvirtd_qemu = | nvram_entry | gluster_debug_level_entry | memory_entry + | vxhs_entry

let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index e6c0832..83c2377 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -250,6 +250,29 @@ #chardev_tls_x509_secret_uuid = "00000000-0000-0000-0000-000000000000"

+# Enable use of TLS encryption on the VxHS network block devices. +# +# When the VxHS network block device server is set up appropriately, +# x509 certificates are used for authentication between the clients +# (qemu processes) and the remote VxHS server. +# +# It is necessary to setup CA and issue client and server certificates +# before enabling this. +# +#vxhs_tls = 1 + + +# In order to override the default TLS certificate location for VxHS +# device TCP certificates, supply a valid path to the certificate directory. +# This is used to authenticate the VxHS block device clients to the VxHS +# server. +# +# If the provided path does not exist then the default_tls_x509_cert_dir +# path will be used. +# +#vxhs_tls_x509_cert_dir = "/etc/pki/libvirt-vxhs" + + # In order to override the default TLS certificate location for migration # certificates, supply a valid path to the certificate directory. If the # provided path does not exist then the default_tls_x509_cert_dir path

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 ;-).

Thanks for that insight! I will mull over this for a bit and see how I can rework the code. Will get back on this particular point.

John

...

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 73c33d6..f3813d4 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -280,6 +280,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) SET_TLS_X509_CERT_DEFAULT(spice); SET_TLS_X509_CERT_DEFAULT(chardev); SET_TLS_X509_CERT_DEFAULT(migrate); + SET_TLS_X509_CERT_DEFAULT(vxhs);

#undef SET_TLS_X509_CERT_DEFAULT

@@ -395,6 +396,8 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->chardevTLSx509certdir); VIR_FREE(cfg->chardevTLSx509secretUUID);

+ VIR_FREE(cfg->vxhsTLSx509certdir); + VIR_FREE(cfg->migrateTLSx509certdir); VIR_FREE(cfg->migrateTLSx509secretUUID);

@@ -533,6 +536,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "spice_auto_unix_socket", &cfg->spiceAutoUnixSocket) < 0) goto cleanup; + if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) + goto cleanup; + if (virConfGetValueString(conf, "vxhs_tls_x509_cert_dir", &cfg->vxhsTLSx509certdir) < 0) + goto cleanup;

#define GET_CONFIG_TLS_CERTINFO(val) \ do { \ diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 1407eef..96c0225 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -201,6 +201,9 @@ struct _virQEMUDriverConfig { unsigned int glusterDebugLevel;

char *memoryBackingDir; + + bool vxhsTLS; + char *vxhsTLSx509certdir; };

/* Main driver state */ diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 3e317bc..dfe88f4 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -25,6 +25,8 @@ module Test_libvirtd_qemu = { "chardev_tls_x509_cert_dir" = "/etc/pki/libvirt-chardev" } { "chardev_tls_x509_verify" = "1" } { "chardev_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" } +{ "vxhs_tls" = "1" } +{ "vxhs_tls_x509_cert_dir" = "/etc/pki/libvirt-vxhs" } { "migrate_tls_x509_cert_dir" = "/etc/pki/libvirt-migrate" } { "migrate_tls_x509_verify" = "1" } { "migrate_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" }

On 07/12/2017 06:09 PM, ashish mittal wrote:

On Fri, Jun 30, 2017 at 1:29 PM, John Ferlan <jferlan@redhat.com> wrote:

...

On 06/29/2017 10:02 PM, Ashish Mittal wrote:

...

From: Ashish Mittal <ashish.mittal@veritas.com>

Add a new TLS X.509 certificate type - "vxhs". This will handle the creation of a TLS certificate capability for properly configured VxHS network block device clients.

Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com> --- Changelog:

(1) Add two new options 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.

src/qemu/libvirtd_qemu.aug | 4 ++++ src/qemu/qemu.conf | 23 +++++++++++++++++++++++ src/qemu/qemu_conf.c | 7 +++++++ src/qemu/qemu_conf.h | 3 +++ src/qemu/test_libvirtd_qemu.aug.in | 2 ++ 5 files changed, 39 insertions(+)

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.

...

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index e1983d1..c19bf3a 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -115,6 +115,9 @@ module Libvirtd_qemu =

let memory_entry = str_entry "memory_backing_dir"

+ let vxhs_entry = bool_entry "vxhs_tls" + | str_entry "vxhs_tls_x509_cert_dir" + (* Each entry in the config is one of the following ... *) let entry = default_tls_entry | vnc_entry @@ -133,6 +136,7 @@ module Libvirtd_qemu = | nvram_entry | gluster_debug_level_entry | memory_entry + | vxhs_entry

let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index e6c0832..83c2377 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -250,6 +250,29 @@ #chardev_tls_x509_secret_uuid = "00000000-0000-0000-0000-000000000000"

+# Enable use of TLS encryption on the VxHS network block devices. +# +# When the VxHS network block device server is set up appropriately, +# x509 certificates are used for authentication between the clients +# (qemu processes) and the remote VxHS server. +# +# It is necessary to setup CA and issue client and server certificates +# before enabling this. +# +#vxhs_tls = 1 + + +# In order to override the default TLS certificate location for VxHS +# device TCP certificates, supply a valid path to the certificate directory. +# This is used to authenticate the VxHS block device clients to the VxHS +# server. +# +# If the provided path does not exist then the default_tls_x509_cert_dir +# path will be used. +# +#vxhs_tls_x509_cert_dir = "/etc/pki/libvirt-vxhs" + + # In order to override the default TLS certificate location for migration # certificates, supply a valid path to the certificate directory. If the # provided path does not exist then the default_tls_x509_cert_dir path

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.

During the QEMU discussions on VxHS support for TLS, it was a agreed that it was not necessary to encrypt the cert files.

...

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 ;-).

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.

The reality is - I don't believe you have to do anything other than describe if the default environment is using a password, then the consumer must create their own vxhs certificate environment that doesn't require a password/secret to decrypt the certificate. Of course this is all predicated on the thought that the certificate being used is not validating libvirt<->qemu, rather its validating qemu<->vxhs-server. There's a bit of black magic that happens that I haven't dug into. I'm assuming that you'd be testing both a cert and non-cert environment... Add to your testing a 'what-if' someone provides a default environment *and* that environment required the secret. In that case, you should expect a failure.

Thanks, Ashish

...

John

...

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 73c33d6..f3813d4 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -280,6 +280,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) SET_TLS_X509_CERT_DEFAULT(spice); SET_TLS_X509_CERT_DEFAULT(chardev); SET_TLS_X509_CERT_DEFAULT(migrate); + SET_TLS_X509_CERT_DEFAULT(vxhs);

#undef SET_TLS_X509_CERT_DEFAULT

@@ -395,6 +396,8 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->chardevTLSx509certdir); VIR_FREE(cfg->chardevTLSx509secretUUID);

+ VIR_FREE(cfg->vxhsTLSx509certdir); + VIR_FREE(cfg->migrateTLSx509certdir); VIR_FREE(cfg->migrateTLSx509secretUUID);

@@ -533,6 +536,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "spice_auto_unix_socket", &cfg->spiceAutoUnixSocket) < 0) goto cleanup; + if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) + goto cleanup; + if (virConfGetValueString(conf, "vxhs_tls_x509_cert_dir", &cfg->vxhsTLSx509certdir) < 0) + goto cleanup;

#define GET_CONFIG_TLS_CERTINFO(val) \ do { \ diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 1407eef..96c0225 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -201,6 +201,9 @@ struct _virQEMUDriverConfig { unsigned int glusterDebugLevel;

char *memoryBackingDir; + + bool vxhsTLS; + char *vxhsTLSx509certdir; };

/* Main driver state */ diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 3e317bc..dfe88f4 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -25,6 +25,8 @@ module Test_libvirtd_qemu = { "chardev_tls_x509_cert_dir" = "/etc/pki/libvirt-chardev" } { "chardev_tls_x509_verify" = "1" } { "chardev_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" } +{ "vxhs_tls" = "1" } +{ "vxhs_tls_x509_cert_dir" = "/etc/pki/libvirt-vxhs" } { "migrate_tls_x509_cert_dir" = "/etc/pki/libvirt-migrate" } { "migrate_tls_x509_verify" = "1" } { "migrate_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" }

On 07/18/2017 11:11 PM, ashish mittal wrote:

On Mon, Jul 17, 2017 at 4:50 PM, John Ferlan <jferlan@redhat.com> wrote:

...

On 07/12/2017 06:09 PM, ashish mittal wrote:

...

On Fri, Jun 30, 2017 at 1:29 PM, John Ferlan <jferlan@redhat.com> wrote:

...

On 06/29/2017 10:02 PM, Ashish Mittal wrote:

...

From: Ashish Mittal <ashish.mittal@veritas.com>

Add a new TLS X.509 certificate type - "vxhs". This will handle the creation of a TLS certificate capability for properly configured VxHS network block device clients.

Signed-off-by: Ashish Mittal <ashish.mittal@veritas.com> --- Changelog:

(1) Add two new options 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.

src/qemu/libvirtd_qemu.aug | 4 ++++ src/qemu/qemu.conf | 23 +++++++++++++++++++++++ src/qemu/qemu_conf.c | 7 +++++++ src/qemu/qemu_conf.h | 3 +++ src/qemu/test_libvirtd_qemu.aug.in | 2 ++ 5 files changed, 39 insertions(+)

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.

...

diff --git a/src/qemu/libvirtd_qemu.aug b/src/qemu/libvirtd_qemu.aug index e1983d1..c19bf3a 100644 --- a/src/qemu/libvirtd_qemu.aug +++ b/src/qemu/libvirtd_qemu.aug @@ -115,6 +115,9 @@ module Libvirtd_qemu =

let memory_entry = str_entry "memory_backing_dir"

+ let vxhs_entry = bool_entry "vxhs_tls" + | str_entry "vxhs_tls_x509_cert_dir" + (* Each entry in the config is one of the following ... *) let entry = default_tls_entry | vnc_entry @@ -133,6 +136,7 @@ module Libvirtd_qemu = | nvram_entry | gluster_debug_level_entry | memory_entry + | vxhs_entry

let comment = [ label "#comment" . del /#[ \t]*/ "# " . store /([^ \t\n][^\n]*)?/ . del /\n/ "\n" ] let empty = [ label "#empty" . eol ] diff --git a/src/qemu/qemu.conf b/src/qemu/qemu.conf index e6c0832..83c2377 100644 --- a/src/qemu/qemu.conf +++ b/src/qemu/qemu.conf @@ -250,6 +250,29 @@ #chardev_tls_x509_secret_uuid = "00000000-0000-0000-0000-000000000000"

+# Enable use of TLS encryption on the VxHS network block devices. +# +# When the VxHS network block device server is set up appropriately, +# x509 certificates are used for authentication between the clients +# (qemu processes) and the remote VxHS server. +# +# It is necessary to setup CA and issue client and server certificates +# before enabling this. +# +#vxhs_tls = 1 + + +# In order to override the default TLS certificate location for VxHS +# device TCP certificates, supply a valid path to the certificate directory. +# This is used to authenticate the VxHS block device clients to the VxHS +# server. +# +# If the provided path does not exist then the default_tls_x509_cert_dir +# path will be used. +# +#vxhs_tls_x509_cert_dir = "/etc/pki/libvirt-vxhs" + + # In order to override the default TLS certificate location for migration # certificates, supply a valid path to the certificate directory. If the # provided path does not exist then the default_tls_x509_cert_dir path

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).

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.

After doing some more thinking about this, my initial assessment may have missed the mark. Even though you're doing libvirt/qemu/client to vxhs/server processing, you still are processing the QEMU command line arguments, in this case "-object tls-creds-x509" in order to fetch the certificate location (and whatever else that object supports). I could be wrong, but IIUC, QEMU will process those arguments generically and doesn't special case the VxHS environment to "ignore anything". I assume you know the QEMU VxHS command line processing better than I do though! I assume when you pass along the following command line option (from your example) that processing sees: -object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\ endpoint=client,verify-peer=yes \ and QEMU will look in @dir for specific files and will allow the QEMU VxHS code to utilize that certificate directory in order to authenticate to the VxHS server. If verify_peer is 'yes', then that means your properly set up VxHS server will want to do that client handshake (at least that's my understanding). The QEMU code doesn't "present" those certificates, but rather processes them. The presentation is done by the QEMU/VxHS client code using the object that was created (objvxhs_tls0). When the server certificate is presented if it was encrypted, but the QEMU command line processing didn't decrypt it, then the target server won't be able to read it. If it was encrypted, I assume the QEMU command line processing will see that 'passwordid' and get the secret to decrypt the certificate before you have to present it to your VxHS server. In any case, since you have the QEMU/VxHS environment available, it should be very easy for you to make that determination. That is what happens when "verify-peer" is "yes" and what happens when it's "no". Check your @dir - does it have the client*.pem files? If so, then the verification is going on. If not, then either the server isn't doing the verification or I'm missing something. I think the next step would be to test what happens under various scenarios that have come up during the review process: 1. TLS environment that doesn't expect passphrase and verify-peer=no (there's no client*.pem files). I assume you've been using this model for your testing) 2. Same as 1, but alter verify-peer=yes a. With no client*.pem files b. With client*.pem files 3. TLS environment that expects a passphrase a. Without providing an "-object secret" to decode it b. With an "-object secret" to decode it That would mean (at least) 5 separate directories that you can change the @dir argument to test with. There could be more test options, but those 5 came to mind quickly. Again, I suggest looking at Dan Berrange's blog for an example: https://www.berrange.com/posts/2016/04/01/improving-qemu-security-part-3-sec... You should be able to set up a very simple example that uses a clear text secret such as "-object secret,id=sec0,data=letmein" http://wiki.qemu.org/ChangeLog/2.6#Secret_passing_system

...

...

...

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.

During the QEMU discussions on VxHS support for TLS, it was a agreed that it was not necessary to encrypt the cert files.

...

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 ;-).

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.

The reality is - I don't believe you have to do anything other than describe if the default environment is using a password, then the consumer must create their own vxhs certificate environment that doesn't require a password/secret to decrypt the certificate.

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).

Well, whomever configured the environment can set it up that way. That's part of the processing done outside of libvirt. Let's not overthink this... Perhaps rather than patch 2 having: + if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) + goto cleanup; + if (virConfGetValueString(conf, "vxhs_tls_x509_cert_dir", &cfg->vxhsTLSx509certdir) < 0) + goto cleanup; Let's change that to if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) goto cleanup; GET_CONFIG_TLS_CERTINFO(vhxs); What does this do? Well if the vxhs* specific parameters aren't set, the macro will use/copy the default* values. So if you modify virQEMUDriverConfig to add vxhsTLSx509secretUUID and vxhsTLSx509verify, then you can just use them. Let's take this a step further, if someone set up a default TLS environment with a password protected certificate, then they'd have to change the default_tls_x509_secret_uuid value to provide the secret UUID for that certificate. That's part of the "processing" required. So now, someone sets up a VxHS environment and wants to use that certificate. So, they define "vxhs_tls = 1" and let the defaults take over. That's it as long as the code is then modified to pass along the vxhsTLSx509verify and vxhsTLSx509SecretUUID when creating the tls-creds-x509 object. NB: If passphrases are used, then the qemuDomainSecretDiskPrepare may need some love to recognize that the VxHS disk would be creating a secret similar to what the similar Chardev routine does. OTOH: if VxHS wants to have it's own environment, someone sets it up, does all the certificate magic, points the vxhs_tls_x509_cert_dir at the environment. If the environment doesn't care about client verification, then verify-peer would be 0. If the environment doesn't care about secrets, then it doesn't uncomment that line (although I think there's a bug in the logic that sets the value to the default if the default exists - I sent a patch today on that). Anyway, if there are VxHS specific values, that doesn't change any other code - those values then get used.

(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.

After much typing and thinking about how QEMU command line probably processes things, I don't believe so. There's a lot of magic happening with the -object tls-creds-x509 and the 'passwordid' argument to reference an -object secret argument. There's lots of libvirt logic to handle creating the objects properly based on settings. There's quite a bit of code you can just copy or adjust to add VxHS processing, don't make it more complicated than it needs to be.

(3) VxHS does not accept secrets, therefore it cannot use the default TLS env if it has been encrypted.

This has nothing to do with secrets and VxHS - I think all this "magic" just happens for you. I could be wrong, but the only way to know is to actually test with the environment and the options.

(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.

I'm merely pointing out you shouldn't attempt to use a default environment that may be configured in such a way that you don't pass/use the correct arguments to make all the magic happen. In that case, you'd have to direct someone to create their own VxHS TLS environment. The best I can say is to work through this logically and test, test, test. I don't believe you should add any code to check whether the default TLS env is being used. QEMU will let you know when you launch it that it cannot decode the server certificate if that's the problem! I really don't see this being much different than the ChardevTLS which does something very similar with respect to presenting certificates to some backend server. Migration is a bit "different" insomuch as it's two qemu processes (source/client and target/server), but it's still processing all values in the config file regardless of whether it's using the default or migration specific environment. At this point it's a matter of getting the code in place and testing.

...

Of course this is all predicated on the thought that the certificate being used is not validating libvirt<->qemu, rather its validating qemu<->vxhs-server. There's a bit of black magic that happens that I

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.

Again, I assume that when passing along those -object's that QEMU will still process them. You just end up *using* the @id for the tls-creds-x509 in order to present that to your server (my assumption again). It's getting very difficult to follow along - we're now 6 levels deeps w/ replies and a few weeks later than when the patches were first posted. My short term memory has other things clogging it up ;-) John

...

haven't dug into. I'm assuming that you'd be testing both a cert and non-cert environment... Add to your testing a 'what-if' someone provides a default environment *and* that environment required the secret. In that case, you should expect a failure.

I will add try to add this test case. Thanks!

...

...

Thanks, Ashish

...

John

...

diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 73c33d6..f3813d4 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -280,6 +280,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) SET_TLS_X509_CERT_DEFAULT(spice); SET_TLS_X509_CERT_DEFAULT(chardev); SET_TLS_X509_CERT_DEFAULT(migrate); + SET_TLS_X509_CERT_DEFAULT(vxhs);

#undef SET_TLS_X509_CERT_DEFAULT

@@ -395,6 +396,8 @@ static void virQEMUDriverConfigDispose(void *obj) VIR_FREE(cfg->chardevTLSx509certdir); VIR_FREE(cfg->chardevTLSx509secretUUID);

+ VIR_FREE(cfg->vxhsTLSx509certdir); + VIR_FREE(cfg->migrateTLSx509certdir); VIR_FREE(cfg->migrateTLSx509secretUUID);

@@ -533,6 +536,10 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, goto cleanup; if (virConfGetValueBool(conf, "spice_auto_unix_socket", &cfg->spiceAutoUnixSocket) < 0) goto cleanup; + if (virConfGetValueBool(conf, "vxhs_tls", &cfg->vxhsTLS) < 0) + goto cleanup; + if (virConfGetValueString(conf, "vxhs_tls_x509_cert_dir", &cfg->vxhsTLSx509certdir) < 0) + goto cleanup;

#define GET_CONFIG_TLS_CERTINFO(val) \ do { \ diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index 1407eef..96c0225 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -201,6 +201,9 @@ struct _virQEMUDriverConfig { unsigned int glusterDebugLevel;

char *memoryBackingDir; + + bool vxhsTLS; + char *vxhsTLSx509certdir; };

/* Main driver state */ diff --git a/src/qemu/test_libvirtd_qemu.aug.in b/src/qemu/test_libvirtd_qemu.aug.in index 3e317bc..dfe88f4 100644 --- a/src/qemu/test_libvirtd_qemu.aug.in +++ b/src/qemu/test_libvirtd_qemu.aug.in @@ -25,6 +25,8 @@ module Test_libvirtd_qemu = { "chardev_tls_x509_cert_dir" = "/etc/pki/libvirt-chardev" } { "chardev_tls_x509_verify" = "1" } { "chardev_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" } +{ "vxhs_tls" = "1" } +{ "vxhs_tls_x509_cert_dir" = "/etc/pki/libvirt-vxhs" } { "migrate_tls_x509_cert_dir" = "/etc/pki/libvirt-migrate" } { "migrate_tls_x509_verify" = "1" } { "migrate_tls_x509_secret_uuid" = "00000000-0000-0000-0000-000000000000" }

On Thu, Jun 29, 2017 at 19:02:41 -0700, Ashish Mittal wrote:

From: Ashish Mittal <ashish.mittal@veritas.com>

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@veritas.com> ---

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>

On 06/30/2017 04:56 AM, Peter Krempa wrote:

On Fri, Jun 30, 2017 at 10:44:39 +0200, Peter Krempa wrote:

...

On Thu, Jun 29, 2017 at 19:02:41 -0700, Ashish Mittal wrote:

...

From: Ashish Mittal <ashish.mittal@veritas.com>

[...]

...

...

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>

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>

I don't like a separate <tls ...> element. What do you mean by other TLS related stuff such as 'verify' or 'secret'? Those would be qemu.conf type settings - they wouldn't change on a disk by disk or domain by domain basis. Why not as a <source> or perhaps more precisely a <host> attribute? If you compare with others it's related to the port as I would assume would be the case for storage as well. If my understanding from the cover letter is valid, then this is how QEMU is going to communicate with some remote host/server in order to provide TLS credentials. John For comparison, other consumers of TLS and their XML: VNC: <devices> ... <graphics type='vnc' port='5904' .../> ... Configured only via qemu.conf AFAICT Spice: <devices> ... <graphics type='spice' port='-1' tlsPort='-1' autoport='yes'> ... Chardev: ... <devices> <serial type="tcp"> <source mode='connect' host="127.0.0.1" service="5555" tls="yes"/> ...

-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list

On Fri, Jun 30, 2017 at 2:21 PM, John Ferlan <jferlan@redhat.com> wrote:

[...]

...

...

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.

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'.

This makes sense. Will get back with the diff that could achieve this.

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-gen...

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.

Will do. Thanks!

...

...

+ 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; +}

[...]

...

...

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.

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

Will check this.

John

...

...

+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>

On Fri, Jun 30, 2017 at 2:21 PM, John Ferlan <jferlan@redhat.com> wrote:

[...]

...

...

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.

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'.

Hi John, Peter, The problem statement - Alias for the TLS certificate can't be a constant. Two TLS objects cannot have the same ID/alias. This was pointed out by both of you as something that needs to be fixed. To that end, I have made some changes to use the block device domain alias (e.g. virtio-disk0) as a unique identifier for each TLS object. This is similar to how char devices create their TLS aliases. I was hoping to get some feedback on whether this diff would be acceptable to fix the said issue. I will reply to/fix all the other comments. Just thought I'd tackle this one first as this appears to be one of the bigger ones... diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 99bc94f..fc58236 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -952,13 +952,19 @@ qemuBuildDiskVxHSTLSinfoCommandLine(virCommandPtr cmd, 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); + if (virAsprintf(&disk->src->aliasTLS, + "vxhs_%s", disk->info.alias) < 0) { + ret = -1; + goto cleanup; + } + + disk->src->addTLS = true; + ret = qemuBuildTLSx509CommandLine(cmd, cfg->vxhsTLSx509certdir, + false, + true, + false, + disk->src->aliasTLS, + qemuCaps); } else if (cfg->vxhsTLS == false && disk->src->haveTLS == VIR_TRISTATE_BOOL_YES) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -968,6 +974,7 @@ qemuBuildDiskVxHSTLSinfoCommandLine(virCommandPtr cmd, ret = -1; } + cleanup: return ret; } @@ -1040,7 +1047,7 @@ qemuBuildVxHSDriveJSON(virStorageSourcePtr src) if (src->addTLS == true) { char *objalias = NULL; - if (!(objalias = qemuAliasTLSObjFromSrcAlias("vxhs"))) + if (!(objalias = qemuAliasTLSObjFromSrcAlias(src->aliasTLS))) goto cleanup; if (virJSONValueObjectCreate(&ret, diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 449ace4..61cd54a 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2057,7 +2057,8 @@ virStorageSourceCopy(const virStorageSource *src, VIR_STRDUP(ret->configFile, src->configFile) < 0 || VIR_STRDUP(ret->nodeformat, src->nodeformat) < 0 || VIR_STRDUP(ret->nodebacking, src->nodebacking) < 0 || - VIR_STRDUP(ret->compat, src->compat) < 0) + VIR_STRDUP(ret->compat, src->compat) < 0 || + VIR_STRDUP(ret->aliasTLS, src->aliasTLS) < 0) goto error; if (src->nhosts) { @@ -2273,6 +2274,7 @@ virStorageSourceClear(virStorageSourcePtr def) virStorageSourceSeclabelsClear(def); virStoragePermsFree(def->perms); VIR_FREE(def->timestamps); + VIR_FREE(def->aliasTLS); virStorageNetHostDefFree(def->nhosts, def->hosts); virStorageAuthDefFree(def->auth); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index e586170..c1d36bf 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -290,6 +290,9 @@ struct _virStorageSource { * the device. For e.g. this could be based on a combination of * global conf setting + domain specific setting */ bool addTLS; + + /* The alias/ID of the TLS object */ + char *aliasTLS; }; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args index 4cacb61..b474ce3 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-multidisk-vxhs.args @@ -18,17 +18,19 @@ QEMU_AUDIO_DRV=none \ -no-acpi \ -boot c \ -usb \ --object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\ +-object tls-creds-x509,id=objvxhs_virtio-disk0_tls0,\ +dir=/usr/local/etc/pki/qemu,\ endpoint=client,verify-peer=yes \ --drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ +-drive file.driver=vxhs,file.tls-creds=objvxhs_virtio-disk0_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,\ +-object tls-creds-x509,id=objvxhs_virtio-disk1_tls0,\ +dir=/usr/local/etc/pki/qemu,\ endpoint=client,verify-peer=yes \ --drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ +-drive file.driver=vxhs,file.tls-creds=objvxhs_virtio-disk1_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 \ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args index e1ad36e..ad78405 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-tlsx509-vxhs.args @@ -18,9 +18,10 @@ QEMU_AUDIO_DRV=none \ -no-acpi \ -boot c \ -usb \ --object tls-creds-x509,id=objvxhs_tls0,dir=/usr/local/etc/pki/qemu,\ +-object tls-creds-x509,id=objvxhs_virtio-disk0_tls0,\ +dir=/usr/local/etc/pki/qemu,\ endpoint=client,verify-peer=yes \ --drive file.driver=vxhs,file.tls-creds=objvxhs_tls0,\ +-drive file.driver=vxhs,file.tls-creds=objvxhs_virtio-disk0_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 \ Thanks, Ashish

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-gen...

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.

...

...

+ 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; +}

[...]

...

...

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.

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

...

...

+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>