[libvirt] [PATCH v2 00/14] Use secret objects to pass iSCSI passwords

v1: https://www.redhat.com/archives/libvir-list/2017-September/msg00100.html Other than patch 1 from v1, everything changed... Don't bother comparing. Highlights - * Two patches of essentially movement of virSecretUsageType because I found (as seen in patch 2) that a previous patch altered the API being used to format the <secret type='%s'.../> field. * The next 8 patches add XML parsing for "auth/secret" and "encryption/ secret" processing as a child of the _virStorageSource and then move the private data from qemuDomainDiskPrivatePtr to a new private data for qemuDomainDiskSrcPrivatePtr. * Patch11 is "somewhat" of an add on and not necessary for this series, but while I was thinking about it and because I believe it'll be useful for some other work - I added a hash lookaside table to be able to map the domain disk source secret objects to the usageType and secret by usage or UUID that was used to generate them. The output in the running XML on my host looks like: <diskSecretObjectAlias> <diskObject alias='hostdev0-secret0'> <secret type='iscsi' usage='libvirtiscsi'/> </diskObject> <diskObject alias='virtio-disk2-secret0'> <secret type='iscsi' usage='libvirtiscsi'/> </diskObject> </diskSecretObjectAlias> My thought was that since the username is included in the object already that it wouldn't have to be saved, but it could be as well. * Patch12 is the capabilities change from patch 1 of v1 with the one minor addition to add the capability to the qemu2.10 replies/xml. * Patch13 is the adjust in virstoragefile and virstoragetest to fetch and format the user/password-secret objects similar to how RBD did this. Guess where I tripped across the virSecretUsageTypeToString issue... * Patch14 handles all the magic in order to use AES secrets for both SCSI disk and hostdev including command line and hotplug. Of "possible concern": * I found no "easy way" manage whether the secret information was a child of disk or disk->src, so I moved everything on output to disk->src even if it was read as a child of disk. The concern here for me is migration and save files... If the domain xml changes - an older libvirt could "lose" the secret information since it wouldn't be a child of disk. It is possible to keep the "top-level" as a child of disk and then any backingStore would be able to have their own. But I figured I'd give the move or else a shot first. John Ferlan (14): util: Move virSecretUsageType to virsecret.h util: Fix secret generation in virStorageSourceParseRBDColonString conf: Add/Allow parsing the auth in the disk source conf: Move auth formatting to disk source docs: Add news article regarding auth placement qemu: Introduce privateData for _virStorageSource conf: Add/Allow parsing the encryption in the disk source conf: Move LUKS encryption formatting to disk source docs: Add news article for encryption in disk source qemu: Move encinfo from private disk to private disk src qemu: Add disk secret object hash table to _qemuDomainObjPrivate qemu: Get capabilities to use iscsi password-secret argument util: Add iSCSI auth/password-secret processing qemu: Use secret objects to pass iSCSI passwords docs/formatdomain.html.in | 81 +++-- docs/news.xml | 24 ++ docs/schemas/domaincommon.rng | 50 ++- src/conf/domain_conf.c | 142 ++++++++- src/conf/domain_conf.h | 1 + src/conf/secret_conf.c | 4 +- src/conf/secret_conf.h | 2 - src/qemu/qemu_block.c | 64 +++- src/qemu/qemu_capabilities.c | 4 + src/qemu/qemu_capabilities.h | 3 + src/qemu/qemu_command.c | 85 ++++- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_domain.c | 351 ++++++++++++++++++++- src/qemu/qemu_domain.h | 25 +- src/qemu/qemu_hotplug.c | 97 +++++- src/qemu/qemu_parse_command.c | 2 +- src/storage/storage_driver.c | 1 + src/util/virsecret.c | 2 + src/util/virsecret.h | 3 + src/util/virstoragefile.c | 34 +- src/util/virstoragefile.h | 3 + .../qemuargv2xml-disk-drive-network-iscsi-auth.xml | 6 +- .../qemuargv2xml-disk-drive-network-rbd-auth.xml | 6 +- tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 41 +++ ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++ ...v-disk-drive-network-iscsi-source-auth-both.xml | 36 +++ ...2argv-disk-drive-network-iscsi-source-auth.args | 31 ++ ...l2argv-disk-drive-network-iscsi-source-auth.xml | 43 +++ ...rgv-disk-drive-network-rbd-source-auth-both.xml | 45 +++ ...ml2argv-disk-drive-network-rbd-source-auth.args | 29 ++ ...xml2argv-disk-drive-network-rbd-source-auth.xml | 42 +++ ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 45 +++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 +++ .../qemuxml2argv-luks-disks-source-both.xml | 40 +++ .../qemuxml2argv-luks-disks-source.args | 62 ++++ .../qemuxml2argv-luks-disks-source.xml | 81 +++++ tests/qemuxml2argvtest.c | 16 + .../qemuxml2xmlout-disk-backing-chains-active.xml | 6 +- ...qemuxml2xmlout-disk-backing-chains-inactive.xml | 6 +- ...emuxml2xmlout-disk-drive-network-iscsi-auth.xml | 12 +- ...xmlout-disk-drive-network-iscsi-source-auth.xml | 47 +++ .../qemuxml2xmlout-disk-drive-network-rbd-auth.xml | 6 +- ...l2xmlout-disk-drive-network-rbd-source-auth.xml | 47 +++ .../qemuxml2xmlout-disk-source-pool-mode.xml | 3 - .../qemuxml2xmlout-luks-disks-source.xml | 84 +++++ .../qemuxml2xmlout-luks-disks.xml | 48 ++- tests/qemuxml2xmltest.c | 3 + tests/virstoragetest.c | 21 ++ tools/virsh-secret.c | 2 +- 53 files changed, 1756 insertions(+), 128 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth-both.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source-both.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-iscsi-source-auth.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-rbd-source-auth.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks-source.xml mode change 120000 => 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml -- 2.13.5

Move the virSecretUsageType into the util. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 1 + src/conf/secret_conf.c | 4 +--- src/conf/secret_conf.h | 2 -- src/qemu/qemu_parse_command.c | 2 +- src/storage/storage_driver.c | 1 + src/util/virsecret.c | 2 ++ src/util/virsecret.h | 3 +++ tools/virsh-secret.c | 2 +- 8 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index cc5e79b70..8dca1357c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -53,6 +53,7 @@ #include "device_conf.h" #include "network_conf.h" #include "virtpm.h" +#include "virsecret.h" #include "virstring.h" #include "virnetdev.h" #include "virnetdevmacvlan.h" diff --git a/src/conf/secret_conf.c b/src/conf/secret_conf.c index bd085b7e4..989705234 100644 --- a/src/conf/secret_conf.c +++ b/src/conf/secret_conf.c @@ -30,6 +30,7 @@ #include "secret_conf.h" #include "virsecretobj.h" #include "virerror.h" +#include "virsecret.h" #include "virstring.h" #include "virxml.h" #include "viruuid.h" @@ -38,9 +39,6 @@ VIR_LOG_INIT("conf.secret_conf"); -VIR_ENUM_IMPL(virSecretUsage, VIR_SECRET_USAGE_TYPE_LAST, - "none", "volume", "ceph", "iscsi", "tls") - void virSecretDefFree(virSecretDefPtr def) { diff --git a/src/conf/secret_conf.h b/src/conf/secret_conf.h index e0d9465a0..aa81651d4 100644 --- a/src/conf/secret_conf.h +++ b/src/conf/secret_conf.h @@ -26,8 +26,6 @@ # include "internal.h" # include "virutil.h" -VIR_ENUM_DECL(virSecretUsage) - typedef struct _virSecretDef virSecretDef; typedef virSecretDef *virSecretDefPtr; struct _virSecretDef { diff --git a/src/qemu/qemu_parse_command.c b/src/qemu/qemu_parse_command.c index 8cb96a24a..ac9076190 100644 --- a/src/qemu/qemu_parse_command.c +++ b/src/qemu/qemu_parse_command.c @@ -28,9 +28,9 @@ #include "dirname.h" #include "viralloc.h" #include "virlog.h" +#include "virsecret.h" #include "virstring.h" #include "c-ctype.h" -#include "secret_conf.h" #define VIR_FROM_THIS VIR_FROM_QEMU diff --git a/src/storage/storage_driver.c b/src/storage/storage_driver.c index 7cf5943cb..59ccc8036 100644 --- a/src/storage/storage_driver.c +++ b/src/storage/storage_driver.c @@ -48,6 +48,7 @@ #include "virfile.h" #include "virfdstream.h" #include "configmake.h" +#include "virsecret.h" #include "virstring.h" #include "viraccessapicheck.h" //#include "dirname.h" diff --git a/src/util/virsecret.c b/src/util/virsecret.c index aded8028b..4dd19cdf5 100644 --- a/src/util/virsecret.c +++ b/src/util/virsecret.c @@ -32,6 +32,8 @@ VIR_LOG_INIT("util.secret"); +VIR_ENUM_IMPL(virSecretUsage, VIR_SECRET_USAGE_TYPE_LAST, + "none", "volume", "ceph", "iscsi", "tls") void virSecretLookupDefClear(virSecretLookupTypeDefPtr def) diff --git a/src/util/virsecret.h b/src/util/virsecret.h index 4506fb36e..a56e0c0c5 100644 --- a/src/util/virsecret.h +++ b/src/util/virsecret.h @@ -24,8 +24,11 @@ # include "internal.h" +# include "virutil.h" # include "virxml.h" +VIR_ENUM_DECL(virSecretUsage) + typedef enum { VIR_SECRET_LOOKUP_TYPE_NONE, VIR_SECRET_LOOKUP_TYPE_UUID, diff --git a/tools/virsh-secret.c b/tools/virsh-secret.c index cd788b687..52f067652 100644 --- a/tools/virsh-secret.c +++ b/tools/virsh-secret.c @@ -32,9 +32,9 @@ #include "viralloc.h" #include "virfile.h" #include "virutil.h" +#include "virsecret.h" #include "virstring.h" #include "virtime.h" -#include "conf/secret_conf.h" static virSecretPtr virshCommandOptSecret(vshControl *ctl, const vshCmd *cmd, const char **name) -- 2.13.5

On Fri, Sep 15, 2017 at 20:30:04 -0400, John Ferlan wrote:
Move the virSecretUsageType into the util.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 1 + src/conf/secret_conf.c | 4 +--- src/conf/secret_conf.h | 2 -- src/qemu/qemu_parse_command.c | 2 +- src/storage/storage_driver.c | 1 + src/util/virsecret.c | 2 ++ src/util/virsecret.h | 3 +++ tools/virsh-secret.c | 2 +- 8 files changed, 10 insertions(+), 7 deletions(-)
ACK

Commit id '5604c056' used the wrong API to generate the <secret type='%s'..." field. The previous code used the correct API as was done in commit id '6887af39'. The data is actually a usage type not an auth type even though the result is the same. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index e94ad32f0..1040e9a17 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -44,6 +44,7 @@ #include "virbuffer.h" #include "virjson.h" #include "virstorageencryption.h" +#include "virsecret.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -2546,7 +2547,7 @@ virStorageSourceParseRBDColonString(const char *rbdstr, goto error; if (VIR_STRDUP(authdef->secrettype, - virStorageAuthTypeToString(VIR_STORAGE_AUTH_TYPE_CEPHX)) < 0) + virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH)) < 0) goto error; src->auth = authdef; authdef = NULL; -- 2.13.5

On Fri, Sep 15, 2017 at 20:30:05 -0400, John Ferlan wrote:
Commit id '5604c056' used the wrong API to generate the
oops
<secret type='%s'..." field. The previous code used the correct API as was done in commit id '6887af39'. The data is actually a usage type not an auth type even though the result is the same.
ACK

Since the virStorageAuthDefPtr auth; is a member of _virStorageSource it really should be allowed to be a subelement of the disk <source> for the RBD and iSCSI prototcols. That way we can set up to allow the <auth> element to be formatted within the disk source. For now just allow the format in the RNG and read it in domain_conf. Modify the qemuxml2argvtest to add a parse failure when there is an <auth> as a child of <disk> *and* an <auth> as a child of <source>. The virschematest will read the new test files and validate from a RNG viewpoint things are fine Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/schemas/domaincommon.rng | 20 +++++++- src/conf/domain_conf.c | 53 ++++++++++++++++++++-- ...v-disk-drive-network-iscsi-source-auth-both.xml | 36 +++++++++++++++ ...l2argv-disk-drive-network-iscsi-source-auth.xml | 43 ++++++++++++++++++ ...rgv-disk-drive-network-rbd-source-auth-both.xml | 45 ++++++++++++++++++ ...xml2argv-disk-drive-network-rbd-source-auth.xml | 42 +++++++++++++++++ tests/qemuxml2argvtest.c | 2 + 7 files changed, 237 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth-both.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.xml diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c9a4f7a9a..139f1eea2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1578,11 +1578,29 @@ <empty/> </element> </optional> + <optional> + <ref name="diskAuth"/> + </optional> <empty/> </interleave> </element> </define> + <define name="diskSourceNetworkProtocolISCSI"> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>iscsi</value> + </choice> + </attribute> + <attribute name="name"/> + <ref name="diskSourceNetworkHost"/> + <optional> + <ref name="diskAuth"/> + </optional> + </element> + </define> + <define name="diskSourceNetworkProtocolHTTP"> <element name="source"> <attribute name="protocol"> @@ -1601,7 +1619,6 @@ <attribute name="protocol"> <choice> <value>sheepdog</value> - <value>iscsi</value> <value>ftp</value> <value>ftps</value> <value>tftp</value> @@ -1644,6 +1661,7 @@ <ref name="diskSourceNetworkProtocolNBD"/> <ref name="diskSourceNetworkProtocolGluster"/> <ref name="diskSourceNetworkProtocolRBD"/> + <ref name="diskSourceNetworkProtocolISCSI"/> <ref name="diskSourceNetworkProtocolHTTP"/> <ref name="diskSourceNetworkProtocolSimple"/> </choice> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8dca1357c..5c0218cdf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8107,6 +8107,29 @@ virDomainDiskSourcePoolDefParse(xmlNodePtr node, } +static int +virDomainDiskSourceAuthParse(xmlNodePtr node, + virStorageAuthDefPtr *authdefsrc) +{ + xmlNodePtr child; + virStorageAuthDefPtr authdef; + + for (child = node->children; child; child = child->next) { + if (child->type == XML_ELEMENT_NODE && + virXMLNodeNameEqual(child, "auth")) { + + if (!(authdef = virStorageAuthDefParse(node->doc, child))) + return -1; + + *authdefsrc = authdef; + return 0; + } + } + + return 0; +} + + int virDomainDiskSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, @@ -8193,6 +8216,9 @@ virDomainDiskSourceParse(xmlNodePtr node, goto cleanup; } + if (virDomainDiskSourceAuthParse(node, &src->auth) < 0) + goto cleanup; + /* People sometimes pass a bogus '' source path when they mean to omit the * source element completely (e.g. CDROM without media). This is just a * little compatibility check to help those broken apps */ @@ -8770,6 +8796,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *serial = NULL; char *startupPolicy = NULL; virStorageAuthDefPtr authdef = NULL; + bool diskAuth = false; char *tray = NULL; char *removable = NULL; char *logical_block_size = NULL; @@ -8819,6 +8846,16 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (virDomainDiskSourceParse(cur, ctxt, def->src) < 0) goto error; + /* If we've already found an <auth> as a child of <disk> and + * we find one as a child of <source>, then force an error to + * avoid ambiguity */ + if (diskAuth && def->src->auth) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("an <auth> definition already found for " + "the <disk> definition")); + goto error; + } + source = true; startupPolicy = virXMLPropString(cur, "startupPolicy"); @@ -8874,10 +8911,20 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, !(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE)) { if (virDomainDiskDefMirrorParse(def, cur, ctxt) < 0) goto error; - } else if (!authdef && + } else if (!diskAuth && virXMLNodeNameEqual(cur, "auth")) { + diskAuth = true; if (!(authdef = virStorageAuthDefParse(node->doc, cur))) goto error; + + /* If we've already parsed <source> and found an <auth> child, + * then generate an error to avoid ambiguity */ + if (source && def->src->auth) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("an <auth> definition already found for " + "disk source")); + goto error; + } } else if (virXMLNodeNameEqual(cur, "iotune")) { if (virDomainDiskDefIotuneParse(def, ctxt) < 0) goto error; @@ -9111,8 +9158,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->dst = target; target = NULL; - def->src->auth = authdef; - authdef = NULL; + if (diskAuth) + VIR_STEAL_PTR(def->src->auth, authdef); def->src->encryption = encryption; encryption = NULL; def->domain_name = domain_name; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth-both.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth-both.xml new file mode 100644 index 000000000..9f14f489f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth-both.xml @@ -0,0 +1,36 @@ +<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-i686</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='6000'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <target dev='vda' bus='virtio'/> + </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-iscsi-source-auth.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml new file mode 100644 index 000000000..af2d51fe9 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml @@ -0,0 +1,43 @@ +<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-i686</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='6000'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> + <host name='example.org' port='6000'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <target dev='vdb' bus='virtio'/> + </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-rbd-source-auth-both.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml new file mode 100644 index 000000000..62a781cd3 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml @@ -0,0 +1,45 @@ +<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-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' 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-rbd-source-auth.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.xml new file mode 100644 index 000000000..d25e4148b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.xml @@ -0,0 +1,42 @@ +<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-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' 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 c8c479cbd..d16b3b7b8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -919,6 +919,8 @@ mymain(void) DO_TEST("disk-drive-network-iscsi-auth", NONE); DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-secrettype-invalid", NONE); DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-wrong-secrettype", NONE); + DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-source-auth-both", NONE); + DO_TEST_PARSE_ERROR("disk-drive-network-rbd-source-auth-both", NONE); DO_TEST("disk-drive-network-iscsi-lun", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_BLOCK); -- 2.13.5

On Fri, Sep 15, 2017 at 20:30:06 -0400, John Ferlan wrote:
Since the virStorageAuthDefPtr auth; is a member of _virStorageSource it really should be allowed to be a subelement of the disk <source> for the RBD and iSCSI prototcols. That way we can set up to allow the <auth> element to be formatted within the disk source.
For now just allow the format in the RNG and read it in domain_conf.
Modify the qemuxml2argvtest to add a parse failure when there is an <auth> as a child of <disk> *and* an <auth> as a child of <source>.
The virschematest will read the new test files and validate from a RNG viewpoint things are fine
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/schemas/domaincommon.rng | 20 +++++++- src/conf/domain_conf.c | 53 ++++++++++++++++++++-- ...v-disk-drive-network-iscsi-source-auth-both.xml | 36 +++++++++++++++ ...l2argv-disk-drive-network-iscsi-source-auth.xml | 43 ++++++++++++++++++ ...rgv-disk-drive-network-rbd-source-auth-both.xml | 45 ++++++++++++++++++ ...xml2argv-disk-drive-network-rbd-source-auth.xml | 42 +++++++++++++++++ tests/qemuxml2argvtest.c | 2 + 7 files changed, 237 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth-both.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.xml
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c9a4f7a9a..139f1eea2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1578,11 +1578,29 @@ <empty/> </element> </optional> + <optional> + <ref name="diskAuth"/> + </optional> <empty/> </interleave> </element> </define>
+ <define name="diskSourceNetworkProtocolISCSI"> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>iscsi</value> + </choice>
AFAIK Michal removed the <choice> here a few days ago.
+ </attribute> + <attribute name="name"/> + <ref name="diskSourceNetworkHost"/> + <optional> + <ref name="diskAuth"/> + </optional> + </element> + </define> + <define name="diskSourceNetworkProtocolHTTP"> <element name="source"> <attribute name="protocol">
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8dca1357c..5c0218cdf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -8770,6 +8796,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *serial = NULL; char *startupPolicy = NULL; virStorageAuthDefPtr authdef = NULL; + bool diskAuth = false;
I don't think you need this bool, since the variable above is non-null only in the correct cases and remains so until the end of the func when you steal the value.
char *tray = NULL; char *removable = NULL; char *logical_block_size = NULL;
[...] I think you will need to remember that the <auth> stuff was part of <disk> and not source, since we can't change that. More on that in review of the next patch.
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml
This file is not used. See below for a comment I had on the same case for RBD.
new file mode 100644 index 000000000..af2d51fe9 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml @@ -0,0 +1,43 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
[...]
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml new file mode 100644 index 000000000..62a781cd3 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml @@ -0,0 +1,45 @@ +<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-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
This disk is not needed.
+ </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' 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-rbd-source-auth.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.xml
This file is added but not used in any test. Also when you are going to make it used later, can't you just add another disk to an existing test case? We have a few of cases which already test RBD auth. There's no need to add so much of the bloat just to test the new syntax.
new file mode 100644 index 000000000..d25e4148b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.xml @@ -0,0 +1,42 @@ +<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-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk>
Lots of unnecessary stuff ...
+ <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + </source> + <target dev='vda' bus='virtio'/> + </disk>
[...]
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c8c479cbd..d16b3b7b8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -919,6 +919,8 @@ mymain(void) DO_TEST("disk-drive-network-iscsi-auth", NONE); DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-secrettype-invalid", NONE); DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-wrong-secrettype", NONE); + DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-source-auth-both", NONE); + DO_TEST_PARSE_ERROR("disk-drive-network-rbd-source-auth-both", NONE);
Do we really need both if the code paths in the parser are the same? ACK with the test file stuff sorted out and the fix to the schema change.

On 09/21/2017 08:38 AM, Peter Krempa wrote:
On Fri, Sep 15, 2017 at 20:30:06 -0400, John Ferlan wrote:
Since the virStorageAuthDefPtr auth; is a member of _virStorageSource it really should be allowed to be a subelement of the disk <source> for the RBD and iSCSI prototcols. That way we can set up to allow the <auth> element to be formatted within the disk source.
For now just allow the format in the RNG and read it in domain_conf.
Modify the qemuxml2argvtest to add a parse failure when there is an <auth> as a child of <disk> *and* an <auth> as a child of <source>.
The virschematest will read the new test files and validate from a RNG viewpoint things are fine
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/schemas/domaincommon.rng | 20 +++++++- src/conf/domain_conf.c | 53 ++++++++++++++++++++-- ...v-disk-drive-network-iscsi-source-auth-both.xml | 36 +++++++++++++++ ...l2argv-disk-drive-network-iscsi-source-auth.xml | 43 ++++++++++++++++++ ...rgv-disk-drive-network-rbd-source-auth-both.xml | 45 ++++++++++++++++++ ...xml2argv-disk-drive-network-rbd-source-auth.xml | 42 +++++++++++++++++ tests/qemuxml2argvtest.c | 2 + 7 files changed, 237 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth-both.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth-both.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.xml
diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index c9a4f7a9a..139f1eea2 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1578,11 +1578,29 @@ <empty/> </element> </optional> + <optional> + <ref name="diskAuth"/> + </optional> <empty/> </interleave> </element> </define>
+ <define name="diskSourceNetworkProtocolISCSI"> + <element name="source"> + <attribute name="protocol"> + <choice> + <value>iscsi</value> + </choice>
AFAIK Michal removed the <choice> here a few days ago.
ah... probably after I had originally copied RBD... I will remove it.
+ </attribute> + <attribute name="name"/> + <ref name="diskSourceNetworkHost"/> + <optional> + <ref name="diskAuth"/> + </optional> + </element> + </define> + <define name="diskSourceNetworkProtocolHTTP"> <element name="source"> <attribute name="protocol">
[...]
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 8dca1357c..5c0218cdf 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c
[...]
@@ -8770,6 +8796,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *serial = NULL; char *startupPolicy = NULL; virStorageAuthDefPtr authdef = NULL; + bool diskAuth = false;
I don't think you need this bool, since the variable above is non-null only in the correct cases and remains so until the end of the func when you steal the value.
char *tray = NULL; char *removable = NULL; char *logical_block_size = NULL;
[...]
I think you will need to remember that the <auth> stuff was part of <disk> and not source, since we can't change that. More on that in review of the next patch.
Right - I figured I'd give it a go with the absolute steal and replace algorithm before going with the determine where <auth> was found and be sure to output exactly as read in.
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml
This file is not used. See below for a comment I had on the same case for RBD.
Dang - thought I caught all those when I split things up.
new file mode 100644 index 000000000..af2d51fe9 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.xml @@ -0,0 +1,43 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
[...]
[...]
diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index c8c479cbd..d16b3b7b8 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -919,6 +919,8 @@ mymain(void) DO_TEST("disk-drive-network-iscsi-auth", NONE); DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-secrettype-invalid", NONE); DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-wrong-secrettype", NONE); + DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-source-auth-both", NONE); + DO_TEST_PARSE_ERROR("disk-drive-network-rbd-source-auth-both", NONE);
Do we really need both if the code paths in the parser are the same?
No - they've been separate "historically", but I combine things and clean up the testing portion a bit between this and the subsequent patch. Same for the encryption options. It'll be a repost effort too... thanks - John

Alter the output of the formatting to be a child of the disk's source rather than a child of the disk. Update the various test outputs for existing disk tests to conform to the new view. Add tests to validate that if the <auth> was found in <source>, then the resulting xml2xml and xml2arg works just fine. The two new .args file are exact copies of the non "-source" version of the file. Update the virstoragetest to handle that the <auth> output will now be part of the <source> stanza in the rbd output. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 68 +++++++++++++--------- src/conf/domain_conf.c | 15 +++-- .../qemuargv2xml-disk-drive-network-iscsi-auth.xml | 6 +- .../qemuargv2xml-disk-drive-network-rbd-auth.xml | 6 +- ...2argv-disk-drive-network-iscsi-source-auth.args | 31 ++++++++++ ...ml2argv-disk-drive-network-rbd-source-auth.args | 29 +++++++++ tests/qemuxml2argvtest.c | 2 + .../qemuxml2xmlout-disk-backing-chains-active.xml | 6 +- ...qemuxml2xmlout-disk-backing-chains-inactive.xml | 6 +- ...emuxml2xmlout-disk-drive-network-iscsi-auth.xml | 12 ++-- ...xmlout-disk-drive-network-iscsi-source-auth.xml | 47 +++++++++++++++ .../qemuxml2xmlout-disk-drive-network-rbd-auth.xml | 6 +- ...l2xmlout-disk-drive-network-rbd-source-auth.xml | 47 +++++++++++++++ .../qemuxml2xmlout-disk-source-pool-mode.xml | 3 - tests/qemuxml2xmltest.c | 2 + tests/virstoragetest.c | 6 ++ 16 files changed, 235 insertions(+), 57 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.args create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-iscsi-source-auth.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-rbd-source-auth.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 3b78bbeb8..f56479953 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2293,11 +2293,11 @@ <host name="hostname" port="7000"/> <snapshot name="snapname"/> <config file="/path/to/file"/> + <auth username='myuser'> + <secret type='ceph' usage='mypassid'/> + </auth> </source> <target dev="hdc" bus="ide"/> - <auth username='myuser'> - <secret type='ceph' usage='mypassid'/> - </auth> </disk> <disk type='block' device='cdrom'> <driver name='qemu' type='raw'/> @@ -2366,20 +2366,20 @@ <driver name='qemu' type='raw'/> <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/2'> <host name='example.com' port='3260'/> + <auth username='myuser'> + <secret type='iscsi' usage='libvirtiscsi'/> + </auth> </source> - <auth username='myuser'> - <secret type='iscsi' usage='libvirtiscsi'/> - </auth> <target dev='vda' bus='virtio'/> </disk> <disk type='network' device='lun'> <driver name='qemu' type='raw'/> <source protocol='iscsi' name='iqn.2013-07.com.example:iscsi-nopool/1'> <host name='example.com' port='3260'/> + <auth username='myuser'> + <secret type='iscsi' usage='libvirtiscsi'/> + </auth> </source> - <auth username='myuser'> - <secret type='iscsi' usage='libvirtiscsi'/> - </auth> <target dev='sdb' bus='scsi'/> </disk> <disk type='volume' device='disk'> @@ -2650,6 +2650,28 @@ protocol. Supported for 'rbd' <span class="since">since 1.2.11 (QEMU only).</span> </dd> + <dt><code>auth</code></dt> + <dd><span class="since">Since libvirt 3.8.0</span>, the + <code>auth</code> element is supported for a disk + <code>type</code> "network" that is using a <code>source</code> + element with the <code>protocol</code> attributes "rbd" or "iscsi". + If present, the <code>auth</code> element provides the + authentication credentials needed to access the source. It + includes a mandatory attribute <code>username</code>, which + identifies the username to use during authentication, as well + as a sub-element <code>secret</code> with mandatory + attribute <code>type</code>, to tie back to + a <a href="formatsecret.html">libvirt secret object</a> that + holds the actual password or other credentials (the domain XML + intentionally does not expose the password, only the reference + to the object that does manage the password). + Known secret types are "ceph" for Ceph RBD network sources and + "iscsi" for CHAP authentication of iSCSI targets. + Both will require either a <code>uuid</code> attribute + with the UUID of the secret object or a <code>usage</code> + attribute matching the key that was specified in the + secret object. + </dd> </dl> <p> @@ -3119,25 +3141,15 @@ are available, each defaulting to 0. </dd> <dt><code>auth</code></dt> - <dd>The <code>auth</code> element is supported for a disk - <code>type</code> "network" that is using a <code>source</code> - element with the <code>protocol</code> attributes "rbd" or "iscsi". - If present, the <code>auth</code> element provides the - authentication credentials needed to access the source. It - includes a mandatory attribute <code>username</code>, which - identifies the username to use during authentication, as well - as a sub-element <code>secret</code> with mandatory - attribute <code>type</code>, to tie back to - a <a href="formatsecret.html">libvirt secret object</a> that - holds the actual password or other credentials (the domain XML - intentionally does not expose the password, only the reference - to the object that does manage the password). - Known secret types are "ceph" for Ceph RBD network sources and - "iscsi" for CHAP authentication of iSCSI targets. - Both will require either a <code>uuid</code> attribute - with the UUID of the secret object or a <code>usage</code> - attribute matching the key that was specified in the - secret object. <span class="since">libvirt 0.9.7</span> + <dd>Starting with <span class="since">libvirt 3.8.0</span> the + <code>auth</code> element moved to be a sub-element of the + <code>source</code> element. The element may still be read as + a <code>disk</code> sub-element, but on output will be moved + to be a <code>source</code> sub-element. It is invalid to use + <code>auth</code> as both a sub-element of <code>disk</code> + and <code>source</code>. The <code>auth</code> element was + introduced as a <code>disk</code> sub-element in + <span class="since">libvirt 0.9.7.</span> </dd> <dt><code>geometry</code></dt> <dd>The optional <code>geometry</code> element provides the diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 5c0218cdf..542d14ed6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21829,6 +21829,16 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, goto error; } + /* Storage Source formatting will not carry through the blunder + * that disk source formatting had at one time to format the + * <auth> for a volume source type. The <auth> information is + * kept in the storage pool and would be overwritten anyway. + * So avoid formatting it for volumes. */ + if (src->auth && src->type != VIR_STORAGE_TYPE_VOLUME) { + if (virStorageAuthDefFormat(&childBuf, src->auth) < 0) + goto error; + } + if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) goto error; } @@ -22014,11 +22024,6 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAddLit(buf, "/>\n"); } - if (def->src->auth) { - if (virStorageAuthDefFormat(buf, def->src->auth) < 0) - return -1; - } - if (virDomainDiskSourceFormat(buf, def->src, def->startupPolicy, flags) < 0) return -1; diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi-auth.xml b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi-auth.xml index 1773b460b..08a82ee2e 100644 --- a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi-auth.xml +++ b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-iscsi-auth.xml @@ -16,11 +16,11 @@ <emulator>/usr/bin/qemu-system-i686</emulator> <disk type='network' device='disk'> <driver name='qemu' type='raw'/> - <auth username='myname'> - <secret type='iscsi' usage='qemuargv2xml_usage'/> - </auth> <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> <host name='example.org' port='6000'/> + <auth username='myname'> + <secret type='iscsi' usage='qemuargv2xml_usage'/> + </auth> </source> <target dev='vda' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-rbd-auth.xml b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-rbd-auth.xml index 3f30296c0..e1326b925 100644 --- a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-rbd-auth.xml +++ b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-rbd-auth.xml @@ -22,13 +22,13 @@ </disk> <disk type='network' device='disk'> <driver name='qemu' type='raw'/> - <auth username='myname'> - <secret type='ceph' usage='qemuargv2xml_usage'/> - </auth> <source protocol='rbd' name='pool/image'> <host name='mon1.example.org' port='6321'/> <host name='mon2.example.org' port='6322'/> <host name='mon3.example.org' port='6322'/> + <auth username='myname'> + <secret type='ceph' usage='qemuargv2xml_usage'/> + </auth> </source> <target dev='vda' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.args new file mode 100644 index 000000000..756e49ca1 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=iscsi://myname:AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A@example.org:\ +6000/iqn.1992-01.com.example%3Astorage/1,format=raw,if=none,\ +id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-drive file=iscsi://myname:AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A@example.org:\ +6000/iqn.1992-01.com.example%3Astorage/2,format=raw,if=none,\ +id=drive-virtio-disk1 \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk1,\ +id=virtio-disk1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.args new file mode 100644 index 000000000..e3958e571 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.args @@ -0,0 +1,29 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest1 \ +-S \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-drive 'file=rbd:pool/image:id=myname:\ +key=QVFDVm41aE82SHpGQWhBQXEwTkN2OGp0SmNJY0UrSE9CbE1RMUE=:\ +auth_supported=cephx\;none:mon_host=mon1.example.org\:6321\;mon2.example.org\:\ +6322\;mon3.example.org\:6322,format=raw,if=none,id=drive-virtio-disk0' \ +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d16b3b7b8..69548cc15 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -917,6 +917,7 @@ mymain(void) DO_TEST("disk-drive-network-nbd-unix", NONE); DO_TEST("disk-drive-network-iscsi", NONE); DO_TEST("disk-drive-network-iscsi-auth", NONE); + DO_TEST("disk-drive-network-iscsi-source-auth", NONE); DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-secrettype-invalid", NONE); DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-wrong-secrettype", NONE); DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-source-auth-both", NONE); @@ -929,6 +930,7 @@ mymain(void) DO_TEST("disk-drive-network-rbd", NONE); DO_TEST("disk-drive-network-sheepdog", NONE); DO_TEST("disk-drive-network-rbd-auth", NONE); + DO_TEST("disk-drive-network-rbd-source-auth", NONE); # ifdef HAVE_GNUTLS_CIPHER_ENCRYPT DO_TEST("disk-drive-network-rbd-auth-AES", QEMU_CAPS_OBJECT_SECRET, QEMU_CAPS_VIRTIO_SCSI); diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-active.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-active.xml index 83d47df56..79fa64d28 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-active.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-active.xml @@ -71,13 +71,13 @@ </disk> <disk type='network' device='disk'> <driver name='qemu' type='qcow2'/> - <auth username='myname'> - <secret type='ceph' usage='mycluster_myname'/> - </auth> <source protocol='rbd' name='pool/image'> <host name='mon1.example.org' port='6321'/> <host name='mon2.example.org' port='6322'/> <host name='mon3.example.org' port='6322'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> </source> <backingStore type='file' index='1'> <format type='qcow2'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-inactive.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-inactive.xml index a9db12ba4..f74c38fec 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-inactive.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-backing-chains-inactive.xml @@ -40,13 +40,13 @@ </disk> <disk type='network' device='disk'> <driver name='qemu' type='qcow2'/> - <auth username='myname'> - <secret type='ceph' usage='mycluster_myname'/> - </auth> <source protocol='rbd' name='pool/image'> <host name='mon1.example.org' port='6321'/> <host name='mon2.example.org' port='6322'/> <host name='mon3.example.org' port='6322'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> </source> <target dev='vdd' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-iscsi-auth.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-iscsi-auth.xml index 543b26e45..24a2ea939 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-iscsi-auth.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-iscsi-auth.xml @@ -16,22 +16,22 @@ <emulator>/usr/bin/qemu-system-i686</emulator> <disk type='network' device='disk'> <driver name='qemu' type='raw'/> - <auth username='myname'> - <secret type='iscsi' usage='mycluster_myname'/> - </auth> <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> <host name='example.org' port='6000'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> </source> <target dev='vda' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> </disk> <disk type='network' device='disk'> <driver name='qemu' type='raw'/> - <auth username='myname'> - <secret type='iscsi' usage='mycluster_myname'/> - </auth> <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> <host name='example.org' port='6000'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> </source> <target dev='vdb' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-iscsi-source-auth.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-iscsi-source-auth.xml new file mode 100644 index 000000000..24a2ea939 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-iscsi-source-auth.xml @@ -0,0 +1,47 @@ +<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-i686</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='6000'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> + <host name='example.org' port='6000'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <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/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-rbd-auth.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-rbd-auth.xml index b18335c1b..c5c40176c 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-rbd-auth.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-rbd-auth.xml @@ -22,13 +22,13 @@ </disk> <disk type='network' device='disk'> <driver name='qemu' type='raw'/> - <auth username='myname'> - <secret type='ceph' usage='mycluster_myname'/> - </auth> <source protocol='rbd' name='pool/image'> <host name='mon1.example.org' port='6321'/> <host name='mon2.example.org' port='6322'/> <host name='mon3.example.org' port='6322'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> </source> <target dev='vda' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-rbd-source-auth.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-rbd-source-auth.xml new file mode 100644 index 000000000..c5c40176c --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-rbd-source-auth.xml @@ -0,0 +1,47 @@ +<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-i686</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + </source> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='ide' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/> + </controller> + <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/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool-mode.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool-mode.xml index a14ed7b97..34fa4cb52 100644 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool-mode.xml +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-source-pool-mode.xml @@ -40,9 +40,6 @@ <address type='drive' controller='0' bus='0' target='0' unit='3'/> </disk> <disk type='volume' device='cdrom'> - <auth username='myname'> - <secret type='iscsi' usage='mycluster_myname'/> - </auth> <source pool='pool-iscsi' volume='unit:0:0:3' mode='direct'> <seclabel model='selinux' relabel='yes'> <label>system_u:system_r:public_content_t:s0</label> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 0a87cedf2..90ffb040b 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -468,9 +468,11 @@ mymain(void) DO_TEST("disk-drive-network-nbd-unix", NONE); DO_TEST("disk-drive-network-iscsi", NONE); DO_TEST("disk-drive-network-iscsi-auth", NONE); + DO_TEST("disk-drive-network-iscsi-source-auth", NONE); DO_TEST("disk-drive-network-gluster", NONE); DO_TEST("disk-drive-network-rbd", NONE); DO_TEST("disk-drive-network-rbd-auth", NONE); + DO_TEST("disk-drive-network-rbd-source-auth", NONE); DO_TEST("disk-drive-network-rbd-ipv6", NONE); DO_TEST("disk-drive-network-rbd-ceph-env", NONE); DO_TEST("disk-drive-network-sheepdog", NONE); diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 60e3164b0..46d12c0e6 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1361,6 +1361,9 @@ mymain(void) TEST_BACKING_PARSE("rbd:testshare:id=asdf:mon_host=example.com", "<source protocol='rbd' name='testshare'>\n" " <host name='example.com'/>\n" + " <auth username='asdf'>\n" + " <secret type='ceph'/>\n" + " </auth>\n" "</source>\n"); TEST_BACKING_PARSE("nbd:example.org:6000:exportname=blah", "<source protocol='nbd' name='blah'>\n" @@ -1526,6 +1529,9 @@ mymain(void) "}", "<source protocol='rbd' name='testshare'>\n" " <host name='example.com'/>\n" + " <auth username='asdf'>\n" + " <secret type='ceph'/>\n" + " </auth>\n" "</source>\n"); TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"rbd\"," "\"image\":\"test\"," -- 2.13.5

On Fri, Sep 15, 2017 at 20:30:07 -0400, John Ferlan wrote:
Alter the output of the formatting to be a child of the disk's source rather than a child of the disk.
I don't think we can do this unconditionally. Apps which use <auth> and parse the XML will break. NACK to this approach. You need to remember that <auth> was part of the disk and use it that way. (only for the top level image obviously, also the snapshot operation needs to clear that flag, since it would change if you merge the top layer snapshot back according to whether libvirtd was restarted or not)
Update the various test outputs for existing disk tests to conform to the new view.
Add tests to validate that if the <auth> was found in <source>, then the resulting xml2xml and xml2arg works just fine. The two new .args file are exact copies of the non "-source" version of the file.
Update the virstoragetest to handle that the <auth> output will now be part of the <source> stanza in the rbd output.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 68 +++++++++++++--------- src/conf/domain_conf.c | 15 +++-- .../qemuargv2xml-disk-drive-network-iscsi-auth.xml | 6 +- .../qemuargv2xml-disk-drive-network-rbd-auth.xml | 6 +- ...2argv-disk-drive-network-iscsi-source-auth.args | 31 ++++++++++ ...ml2argv-disk-drive-network-rbd-source-auth.args | 29 +++++++++ tests/qemuxml2argvtest.c | 2 + .../qemuxml2xmlout-disk-backing-chains-active.xml | 6 +- ...qemuxml2xmlout-disk-backing-chains-inactive.xml | 6 +- ...emuxml2xmlout-disk-drive-network-iscsi-auth.xml | 12 ++-- ...xmlout-disk-drive-network-iscsi-source-auth.xml | 47 +++++++++++++++ .../qemuxml2xmlout-disk-drive-network-rbd-auth.xml | 6 +- ...l2xmlout-disk-drive-network-rbd-source-auth.xml | 47 +++++++++++++++ .../qemuxml2xmlout-disk-source-pool-mode.xml | 3 - tests/qemuxml2xmltest.c | 2 + tests/virstoragetest.c | 6 ++ 16 files changed, 235 insertions(+), 57 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-source-auth.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-source-auth.args create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-iscsi-source-auth.xml create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-network-rbd-source-auth.xml
[...]
diff --git a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-rbd-auth.xml b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-rbd-auth.xml index 3f30296c0..e1326b925 100644 --- a/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-rbd-auth.xml +++ b/tests/qemuargv2xmldata/qemuargv2xml-disk-drive-network-rbd-auth.xml @@ -22,13 +22,13 @@ </disk> <disk type='network' device='disk'> <driver name='qemu' type='raw'/> - <auth username='myname'> - <secret type='ceph' usage='qemuargv2xml_usage'/> - </auth> <source protocol='rbd' name='pool/image'> <host name='mon1.example.org' port='6321'/> <host name='mon2.example.org' port='6322'/> <host name='mon3.example.org' port='6322'/> + <auth username='myname'> + <secret type='ceph' usage='qemuargv2xml_usage'/> + </auth> </source> <target dev='vda' bus='virtio'/> <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/>
So the files from the previous patch I was complaining about apparently belong here.

Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/news.xml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index a5c3d1d90..e79ff4349 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -25,6 +25,17 @@ <section title="New features"> </section> <section title="Improvements"> + <change> + <summary> + conf: Move the auth definition to disk source + </summary> + <description> + Rather than having the <code>auth</code> element be a child of the + <code>disk</code> element move to a child of a <code>source</code> + element. Still recognize on input the legacy child dependency; + however, when writing out the XML the new format will be generated. + </description> + </change> </section> <section title="Bug fixes"> </section> -- 2.13.5

Since the secret information is really _virStorageSource specific piece of data, let's create a privateData object for _virStorageSource and move the @secinfo from _qemuDomainDiskPrivate into a new _qemuDomainDiskSrcPrivate structure and manage it from there. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 6 ++++-- src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_domain.h | 17 +++++++++++---- src/qemu/qemu_hotplug.c | 11 +++++++--- src/util/virstoragefile.c | 1 + src/util/virstoragefile.h | 3 +++ 8 files changed, 83 insertions(+), 15 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 542d14ed6..a3900488f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1720,6 +1720,11 @@ virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt) !(ret->privateData = xmlopt->privateData.diskNew())) goto error; + if (xmlopt && + xmlopt->privateData.diskSrcNew && + !(ret->src->privateData = xmlopt->privateData.diskSrcNew())) + goto error; + return ret; error: diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bb3b6f0c3..f6c9417b4 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2620,6 +2620,7 @@ struct _virDomainXMLPrivateDataCallbacks { /* note that private data for devices are not copied when using * virDomainDefCopy and similar functions */ virDomainXMLPrivateDataNewFunc diskNew; + virDomainXMLPrivateDataNewFunc diskSrcNew; virDomainXMLPrivateDataNewFunc hostdevNew; virDomainXMLPrivateDataNewFunc vcpuNew; virDomainXMLPrivateDataNewFunc chrSourceNew; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d553df57f..898a60f3b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1340,7 +1340,8 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, { int actualType = virStorageSourceGetActualType(disk->src); qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; + qemuDomainDiskSrcPrivatePtr diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(disk->src); + qemuDomainSecretInfoPtr secinfo = diskSrcPriv->secinfo; qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo; virJSONValuePtr srcprops = NULL; char *source = NULL; @@ -2171,7 +2172,8 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, bool driveBoot = false; virDomainDiskDefPtr disk = def->disks[i]; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; + qemuDomainDiskSrcPrivatePtr diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(disk->src); + qemuDomainSecretInfoPtr secinfo = diskSrcPriv->secinfo; qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo; /* PowerPC pseries based VMs do not support floppy device */ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 05f8e9488..94b6d87d6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -921,11 +921,52 @@ qemuDomainDiskPrivateDispose(void *obj) { qemuDomainDiskPrivatePtr priv = obj; - qemuDomainSecretInfoFree(&priv->secinfo); qemuDomainSecretInfoFree(&priv->encinfo); } +static virClassPtr qemuDomainDiskSrcPrivateClass; +static void qemuDomainDiskSrcPrivateDispose(void *obj); + +static int +qemuDomainDiskSrcPrivateOnceInit(void) +{ + qemuDomainDiskSrcPrivateClass = virClassNew(virClassForObject(), + "qemuDomainDiskSrcPrivate", + sizeof(qemuDomainDiskSrcPrivate), + qemuDomainDiskSrcPrivateDispose); + if (!qemuDomainDiskSrcPrivateClass) + return -1; + else + return 0; +} + +VIR_ONCE_GLOBAL_INIT(qemuDomainDiskSrcPrivate) + +static virObjectPtr +qemuDomainDiskSrcPrivateNew(void) +{ + qemuDomainDiskSrcPrivatePtr priv; + + if (qemuDomainDiskSrcPrivateInitialize() < 0) + return NULL; + + if (!(priv = virObjectNew(qemuDomainDiskSrcPrivateClass))) + return NULL; + + return (virObjectPtr) priv; +} + + +static void +qemuDomainDiskSrcPrivateDispose(void *obj) +{ + qemuDomainDiskSrcPrivatePtr priv = obj; + + qemuDomainSecretInfoFree(&priv->secinfo); +} + + static virClassPtr qemuDomainHostdevPrivateClass; static void qemuDomainHostdevPrivateDispose(void *obj); @@ -1294,12 +1335,11 @@ qemuDomainSecretInfoTLSNew(virConnectPtr conn, void qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) { - qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - - if (!diskPriv || !diskPriv->secinfo) + qemuDomainDiskSrcPrivatePtr diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(disk->src); + if (!diskSrcPriv || !diskSrcPriv->secinfo) return; - qemuDomainSecretInfoFree(&diskPriv->secinfo); + qemuDomainSecretInfoFree(&diskSrcPriv->secinfo); } @@ -1345,6 +1385,7 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, { virStorageSourcePtr src = disk->src; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + qemuDomainDiskSrcPrivatePtr diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(disk->src); if (qemuDomainSecretDiskCapable(src)) { virSecretUsageType usageType = VIR_SECRET_USAGE_TYPE_ISCSI; @@ -1352,7 +1393,7 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) usageType = VIR_SECRET_USAGE_TYPE_CEPH; - if (!(diskPriv->secinfo = + if (!(diskSrcPriv->secinfo = qemuDomainSecretInfoNew(conn, priv, disk->info.alias, usageType, src->auth->username, &src->auth->seclookupdef, false))) @@ -2276,6 +2317,7 @@ virDomainXMLPrivateDataCallbacks virQEMUDriverPrivateDataCallbacks = { .alloc = qemuDomainObjPrivateAlloc, .free = qemuDomainObjPrivateFree, .diskNew = qemuDomainDiskPrivateNew, + .diskSrcNew = qemuDomainDiskSrcPrivateNew, .vcpuNew = qemuDomainVcpuPrivateNew, .hostdevNew = qemuDomainHostdevPrivateNew, .chrSourceNew = qemuDomainChrSourcePrivateNew, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b291dc308..d212812dd 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -346,10 +346,6 @@ struct _qemuDomainDiskPrivate { bool migrating; /* the disk is being migrated */ - /* for storage devices using auth/secret - * NB: *not* to be written to qemu domain object XML */ - qemuDomainSecretInfoPtr secinfo; - /* for storage devices using encryption/secret * Can have both <auth> and <encryption> for some disks * NB:*not* to be written to qemu domain object XML */ @@ -360,6 +356,19 @@ struct _qemuDomainDiskPrivate { bool removable; /* device media can be removed/changed */ }; +# define QEMU_DOMAIN_DISK_SRC_PRIVATE(src) \ + ((qemuDomainDiskSrcPrivatePtr) (src)->privateData) + +typedef struct _qemuDomainDiskSrcPrivate qemuDomainDiskSrcPrivate; +typedef qemuDomainDiskSrcPrivate *qemuDomainDiskSrcPrivatePtr; +struct _qemuDomainDiskSrcPrivate { + virObject parent; + + /* for each storage source using auth/secret + * NB: *not* to be written to qemu domain object XML */ + qemuDomainSecretInfoPtr secinfo; +}; + # define QEMU_DOMAIN_HOSTDEV_PRIVATE(hostdev) \ ((qemuDomainHostdevPrivatePtr) (hostdev)->privateData) diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7dd6e5fd9..2942772c2 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -218,6 +218,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, char *driveAlias = NULL; qemuDomainObjPrivatePtr priv = vm->privateData; qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + qemuDomainDiskSrcPrivatePtr diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(disk->src); const char *format = NULL; char *sourcestr = NULL; @@ -259,7 +260,7 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver, } if (!virStorageSourceIsEmpty(newsrc)) { - if (qemuGetDriveSourceString(newsrc, diskPriv->secinfo, &sourcestr) < 0) + if (qemuGetDriveSourceString(newsrc, diskSrcPriv->secinfo, &sourcestr) < 0) goto error; if (virStorageSourceGetActualType(newsrc) != VIR_STORAGE_TYPE_DIR) { @@ -329,6 +330,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, virJSONValuePtr secobjProps = NULL; virJSONValuePtr encobjProps = NULL; qemuDomainDiskPrivatePtr diskPriv; + qemuDomainDiskSrcPrivatePtr diskSrcPriv; qemuDomainSecretInfoPtr secinfo; qemuDomainSecretInfoPtr encinfo; @@ -366,7 +368,8 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto error; diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - secinfo = diskPriv->secinfo; + diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(disk->src); + secinfo = diskSrcPriv->secinfo; if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0) goto error; @@ -621,6 +624,7 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, virJSONValuePtr encobjProps = NULL; virJSONValuePtr secobjProps = NULL; qemuDomainDiskPrivatePtr diskPriv; + qemuDomainDiskSrcPrivatePtr diskSrcPriv; qemuDomainSecretInfoPtr encinfo; qemuDomainSecretInfoPtr secinfo; @@ -654,7 +658,8 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, goto error; diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); - secinfo = diskPriv->secinfo; + diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(disk->src); + secinfo = diskSrcPriv->secinfo; if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0) goto error; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 1040e9a17..39cda9c89 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2271,6 +2271,7 @@ virStorageSourceClear(virStorageSourcePtr def) virStorageNetHostDefFree(def->nhosts, def->hosts); virStorageAuthDefFree(def->auth); + virObjectUnref(def->privateData); VIR_FREE(def->nodestorage); VIR_FREE(def->nodeformat); diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 6c388b1a5..005f22d8e 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -27,6 +27,7 @@ # include <sys/stat.h> # include "virbitmap.h" +# include "virobject.h" # include "virseclabel.h" # include "virstorageencryption.h" # include "virutil.h" @@ -239,6 +240,8 @@ struct _virStorageSource { virStorageAuthDefPtr auth; virStorageEncryptionPtr encryption; + virObjectPtr privateData; /* Usable to store hypervisor specific data */ + char *driverName; int format; /* virStorageFileFormat in domain backing chains, but * pool-specific enum for storage volumes */ -- 2.13.5

On Fri, Sep 15, 2017 at 20:30:09 -0400, John Ferlan wrote:
Since the secret information is really _virStorageSource specific piece of data, let's create a privateData object for _virStorageSource and move the @secinfo from _qemuDomainDiskPrivate into a new _qemuDomainDiskSrcPrivate structure and manage it from there.
This would be easier to review if you'd split the private data addition and the movement of the auth data to it.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 6 ++++-- src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_domain.h | 17 +++++++++++---- src/qemu/qemu_hotplug.c | 11 +++++++--- src/util/virstoragefile.c | 1 + src/util/virstoragefile.h | 3 +++ 8 files changed, 83 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 542d14ed6..a3900488f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1720,6 +1720,11 @@ virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt) !(ret->privateData = xmlopt->privateData.diskNew())) goto error;
+ if (xmlopt && + xmlopt->privateData.diskSrcNew && + !(ret->src->privateData = xmlopt->privateData.diskSrcNew())) + goto error;
This certainly is not enough. The virStorageSource structure needs to allocate it in it's helper. This will only fix the top level one and not the nested/chained ones for the backing store.
+ return ret;
error:
Rest looks okay.

On 09/21/2017 10:31 AM, Peter Krempa wrote:
On Fri, Sep 15, 2017 at 20:30:09 -0400, John Ferlan wrote:
Since the secret information is really _virStorageSource specific piece of data, let's create a privateData object for _virStorageSource and move the @secinfo from _qemuDomainDiskPrivate into a new _qemuDomainDiskSrcPrivate structure and manage it from there.
This would be easier to review if you'd split the private data addition and the movement of the auth data to it.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 6 ++++-- src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_domain.h | 17 +++++++++++---- src/qemu/qemu_hotplug.c | 11 +++++++--- src/util/virstoragefile.c | 1 + src/util/virstoragefile.h | 3 +++ 8 files changed, 83 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 542d14ed6..a3900488f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1720,6 +1720,11 @@ virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt) !(ret->privateData = xmlopt->privateData.diskNew())) goto error;
+ if (xmlopt && + xmlopt->privateData.diskSrcNew && + !(ret->src->privateData = xmlopt->privateData.diskSrcNew())) + goto error;
This certainly is not enough. The virStorageSource structure needs to allocate it in it's helper. This will only fix the top level one and not the nested/chained ones for the backing store.
Just so it's clear what you're asking... Create virStorageSourceNew or virDomainDiskSourceDefNew? Going with virStorageSourceNew would seem more correct; however, then it should live in virstoragefile.c and thus wouldn't be able to use the domain_conf infrastructure. Seems like it's own infrastructure would need to be build. It would work with the virStorageSourceNewFromBacking* which I believe you are referencing. Going with virDomainDiskSourceDefNew would follow the other models and be able to live in domain_conf making it easy for the domain_conf privateData manipulation code, but it just doesn't feel right. John
+ return ret;
error:
Rest looks okay.

On 09/22/2017 11:12 AM, John Ferlan wrote:
On 09/21/2017 10:31 AM, Peter Krempa wrote:
On Fri, Sep 15, 2017 at 20:30:09 -0400, John Ferlan wrote:
Since the secret information is really _virStorageSource specific piece of data, let's create a privateData object for _virStorageSource and move the @secinfo from _qemuDomainDiskPrivate into a new _qemuDomainDiskSrcPrivate structure and manage it from there.
This would be easier to review if you'd split the private data addition and the movement of the auth data to it.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 1 + src/qemu/qemu_command.c | 6 ++++-- src/qemu/qemu_domain.c | 54 +++++++++++++++++++++++++++++++++++++++++------ src/qemu/qemu_domain.h | 17 +++++++++++---- src/qemu/qemu_hotplug.c | 11 +++++++--- src/util/virstoragefile.c | 1 + src/util/virstoragefile.h | 3 +++ 8 files changed, 83 insertions(+), 15 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 542d14ed6..a3900488f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -1720,6 +1720,11 @@ virDomainDiskDefNew(virDomainXMLOptionPtr xmlopt) !(ret->privateData = xmlopt->privateData.diskNew())) goto error;
+ if (xmlopt && + xmlopt->privateData.diskSrcNew && + !(ret->src->privateData = xmlopt->privateData.diskSrcNew())) + goto error;
This certainly is not enough. The virStorageSource structure needs to allocate it in it's helper. This will only fix the top level one and not the nested/chained ones for the backing store.
Just so it's clear what you're asking...
Create virStorageSourceNew or virDomainDiskSourceDefNew?
Going with virStorageSourceNew would seem more correct; however, then it should live in virstoragefile.c and thus wouldn't be able to use the domain_conf infrastructure. Seems like it's own infrastructure would need to be build. It would work with the virStorageSourceNewFromBacking* which I believe you are referencing.
Going with virDomainDiskSourceDefNew would follow the other models and be able to live in domain_conf making it easy for the domain_conf privateData manipulation code, but it just doesn't feel right.
John
nevermind - I figured it out... Going with a domain_conf allocator for _virStorageSource is mechanism. It'll even be usable for hostdev. I have patches that I'll post tomorrow - just need to check that I've covered the various review comments. John

Since the virStorageEncryptionPtr encryption; is a member of _virStorageSource it really should be allowed to be a subelement of the disk <source> for various disk formats: Source{File|Dir|Block|Volume} SourceProtocol{RBD|ISCSI|NBD|Gluster|Simple|HTTP} NB: Simple includes sheepdog, ftp, ftps, tftp That way we can set up to allow the <encryption> element to be formatted within the disk source. For now just allow the format in the RNG and read it in domain_conf. Modify the qemuxml2argvtest to add a parse failure when there is an <encryption> as a child of <disk> *and* an <encryption> as a child of <source>. The virschematest will read the new test files and validate from a RNG viewpoint things are fine. The luks-disks-source file has various formats to test, but not all valid/invalid. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/schemas/domaincommon.rng | 30 ++++++++ src/conf/domain_conf.c | 56 +++++++++++++-- .../qemuxml2argv-luks-disks-source-both.xml | 40 +++++++++++ .../qemuxml2argv-luks-disks-source.xml | 81 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 5 files changed, 202 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source-both.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.xml diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 139f1eea2..d1ef25b7b 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1469,6 +1469,9 @@ <optional> <ref name="storageStartupPolicy"/> </optional> + <optional> + <ref name="encryption"/> + </optional> <zeroOrMore> <ref name='devSeclabel'/> </zeroOrMore> @@ -1490,6 +1493,9 @@ <optional> <ref name="storageStartupPolicy"/> </optional> + <optional> + <ref name="encryption"/> + </optional> <zeroOrMore> <ref name='devSeclabel'/> </zeroOrMore> @@ -1509,6 +1515,9 @@ <optional> <ref name="storageStartupPolicy"/> </optional> + <optional> + <ref name="encryption"/> + </optional> <empty/> </element> </optional> @@ -1581,6 +1590,9 @@ <optional> <ref name="diskAuth"/> </optional> + <optional> + <ref name="encryption"/> + </optional> <empty/> </interleave> </element> @@ -1598,6 +1610,9 @@ <optional> <ref name="diskAuth"/> </optional> + <optional> + <ref name="encryption"/> + </optional> </element> </define> @@ -1611,6 +1626,9 @@ </attribute> <attribute name="name"/> <ref name="diskSourceNetworkHost"/> + <optional> + <ref name="encryption"/> + </optional> </element> </define> @@ -1626,6 +1644,9 @@ </attribute> <attribute name="name"/> <ref name="diskSourceNetworkHost"/> + <optional> + <ref name="encryption"/> + </optional> </element> </define> @@ -1638,6 +1659,9 @@ <attribute name="name"/> </optional> <ref name="diskSourceNetworkHost"/> + <optional> + <ref name="encryption"/> + </optional> </element> </define> @@ -1650,6 +1674,9 @@ <oneOrMore> <ref name="diskSourceNetworkHost"/> </oneOrMore> + <optional> + <ref name="encryption"/> + </optional> </element> </define> @@ -1690,6 +1717,9 @@ <optional> <ref name="storageStartupPolicy"/> </optional> + <optional> + <ref name="encryption"/> + </optional> <zeroOrMore> <ref name='devSeclabel'/> </zeroOrMore> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a3900488f..2a52462d0 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -8135,6 +8135,29 @@ virDomainDiskSourceAuthParse(xmlNodePtr node, } +static int +virDomainDiskSourceEncryptionParse(xmlNodePtr node, + virStorageEncryptionPtr *encryptionsrc) +{ + xmlNodePtr child; + virStorageEncryptionPtr encryption = NULL; + + for (child = node->children; child; child = child->next) { + if (child->type == XML_ELEMENT_NODE && + virXMLNodeNameEqual(child, "encryption")) { + + if (!(encryption = virStorageEncryptionParseNode(node->doc, child))) + return -1; + + *encryptionsrc = encryption; + return 0; + } + } + + return 0; +} + + int virDomainDiskSourceParse(xmlNodePtr node, xmlXPathContextPtr ctxt, @@ -8224,6 +8247,9 @@ virDomainDiskSourceParse(xmlNodePtr node, if (virDomainDiskSourceAuthParse(node, &src->auth) < 0) goto cleanup; + if (virDomainDiskSourceEncryptionParse(node, &src->encryption) < 0) + goto cleanup; + /* People sometimes pass a bogus '' source path when they mean to omit the * source element completely (e.g. CDROM without media). This is just a * little compatibility check to help those broken apps */ @@ -8798,6 +8824,7 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, char *bus = NULL; char *devaddr = NULL; virStorageEncryptionPtr encryption = NULL; + bool diskEncryption = false; char *serial = NULL; char *startupPolicy = NULL; virStorageAuthDefPtr authdef = NULL; @@ -8861,6 +8888,15 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } + /* Similarly for <encryption> - it's a child of <source> too + * and we cannot find in both places */ + if (diskEncryption && def->src->encryption) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("an <encryption> definition already found for " + "the <disk> definition")); + goto error; + } + source = true; startupPolicy = virXMLPropString(cur, "startupPolicy"); @@ -8943,12 +8979,20 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, virXMLNodeNameEqual(cur, "state")) { /* Legacy back-compat. Don't add any more attributes here */ devaddr = virXMLPropString(cur, "devaddr"); - } else if (encryption == NULL && + } else if (!diskEncryption && virXMLNodeNameEqual(cur, "encryption")) { - encryption = virStorageEncryptionParseNode(node->doc, - cur); - if (encryption == NULL) + diskEncryption = true; + if (!(encryption = virStorageEncryptionParseNode(node->doc, cur))) goto error; + + /* If we've already parsed <source> and found an <encryption> child, + * then generate an error to avoid ambiguity */ + if (source && def->src->encryption) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("an <encryption> definition already found for " + "disk source")); + goto error; + } } else if (!serial && virXMLNodeNameEqual(cur, "serial")) { serial = (char *)xmlNodeGetContent(cur); @@ -9165,8 +9209,8 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, target = NULL; if (diskAuth) VIR_STEAL_PTR(def->src->auth, authdef); - def->src->encryption = encryption; - encryption = NULL; + if (diskEncryption) + VIR_STEAL_PTR(def->src->encryption, encryption); def->domain_name = domain_name; domain_name = NULL; def->serial = serial; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source-both.xml b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source-both.xml new file mode 100644 index 000000000..c4b762a1e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source-both.xml @@ -0,0 +1,40 @@ +<domain type='qemu'> + <name>encryptdisk</name> + <uuid>496898a6-e6ff-f7c8-5dc2-3cf410945ee9</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.1'>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='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/storage/guest_disks/encryptdisk'> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + </source> + <target dev='vda' bus='virtio'/> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.xml b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.xml new file mode 100644 index 000000000..293877df9 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.xml @@ -0,0 +1,81 @@ +<domain type='qemu'> + <name>encryptdisk</name> + <uuid>496898a6-e6ff-f7c8-5dc2-3cf410945ee9</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.1'>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='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/storage/guest_disks/encryptdisk'> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + </source> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/storage/guest_disks/encryptdisk2'> + <encryption format='luks'> + <secret type='passphrase' usage='/storage/guest_disks/encryptdisk2'/> + </encryption> + </source> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='6000'/> + <auth username='myname'> + <secret type='iscsi' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80e80'/> + </auth> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80f77'/> + </encryption> + </source> + <target dev='vdc' bus='virtio'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='pool-iscsi' volume='unit:0:0:3' mode='direct'> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80f80'/> + </encryption> + </source> + <target dev='vdd' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80fb0'/> + </encryption> + </source> + <target dev='vde' bus='virtio'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 69548cc15..9a8caaa38 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1652,6 +1652,7 @@ mymain(void) DO_TEST_FAILURE("luks-disks", QEMU_CAPS_OBJECT_SECRET); # endif DO_TEST_PARSE_ERROR("luks-disk-invalid", NONE); + DO_TEST_PARSE_ERROR("luks-disks-source-both", QEMU_CAPS_OBJECT_SECRET); DO_TEST("memtune", NONE); DO_TEST("memtune-unlimited", NONE); -- 2.13.5

On Fri, Sep 15, 2017 at 20:30:10 -0400, John Ferlan wrote:
Since the virStorageEncryptionPtr encryption; is a member of _virStorageSource it really should be allowed to be a subelement of the disk <source> for various disk formats:
Source{File|Dir|Block|Volume} SourceProtocol{RBD|ISCSI|NBD|Gluster|Simple|HTTP}
NB: Simple includes sheepdog, ftp, ftps, tftp
That way we can set up to allow the <encryption> element to be formatted within the disk source.
For now just allow the format in the RNG and read it in domain_conf.
Modify the qemuxml2argvtest to add a parse failure when there is an <encryption> as a child of <disk> *and* an <encryption> as a child of <source>.
The virschematest will read the new test files and validate from a RNG viewpoint things are fine. The luks-disks-source file has various formats to test, but not all valid/invalid.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/schemas/domaincommon.rng | 30 ++++++++ src/conf/domain_conf.c | 56 +++++++++++++-- .../qemuxml2argv-luks-disks-source-both.xml | 40 +++++++++++ .../qemuxml2argv-luks-disks-source.xml | 81 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 5 files changed, 202 insertions(+), 6 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source-both.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.xml
Some comments from 2/* can be applied. ACK

Alter the output of the formatting to be a child of the disk's source rather than a child of the disk for LUKS encryption, but keep the legacy QCOW encryption as a child of disk. Update the various test outputs for existing disk tests to conform to the new view. The qemuxml2xmlout-luks-disks.xml used to be a link to the "source" tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml, but with the change to move LUKS output to a child of <source>, the output file now will differ. While a link would still work, that would require changing the source file which wasn't the goal. Add tests to validate that if the <encryption> was found in <source>, then the resulting xml2xml and xml2arg works just fine. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatdomain.html.in | 13 +++- src/conf/domain_conf.c | 12 ++++ .../qemuxml2argv-luks-disks-source.args | 62 ++++++++++++++++ tests/qemuxml2argvtest.c | 1 + .../qemuxml2xmlout-luks-disks-source.xml | 84 ++++++++++++++++++++++ .../qemuxml2xmlout-luks-disks.xml | 48 ++++++++++++- tests/qemuxml2xmltest.c | 1 + 7 files changed, 219 insertions(+), 2 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.args create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks-source.xml mode change 120000 => 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index f56479953..5facb512a 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2672,6 +2672,14 @@ attribute matching the key that was specified in the secret object. </dd> + <dd><span class="since">Since libvirt 3.8.0</span>, the + <code>encryption</code> can be a sub-element of the + <code>source</code> element for non "qcow" encrypted storage + sources (currently only "luks"). If present, specifies how the + storage source is encrypted. See the + <a href="formatstorageencryption.html">Storage Encryption</a> + page for more information. + </dd> </dl> <p> @@ -3073,7 +3081,10 @@ <span class="since">Since 0.8.8</span> </dd> <dt><code>encryption</code></dt> - <dd>If present, specifies how the volume is encrypted. See + <dd>Starting with <span class="since">libvirt 3.8.0</span> the + <code>encryption</code> element for non "qcow" encrypted storage + sources moved to be a sub-element of the <code>source</code> element. + If present, specifies how the volume is encrypted using "qcow". See the <a href="formatstorageencryption.html">Storage Encryption</a> page for more information. </dd> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 2a52462d0..5851bba44 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -21888,6 +21888,14 @@ virDomainDiskSourceFormatInternal(virBufferPtr buf, goto error; } + /* For encryption formatting that's not the old/default QCOW + * format, let's format the <encryption> in source. This started + * with LUKS encryption */ + if (src->encryption && + src->encryption->format >= VIR_STORAGE_ENCRYPTION_FORMAT_LUKS && + virStorageEncryptionFormat(&childBuf, src->encryption) < 0) + return -1; + if (virXMLFormatElement(buf, "source", &attrBuf, &childBuf) < 0) goto error; } @@ -22207,7 +22215,11 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferEscapeString(buf, "<wwn>%s</wwn>\n", def->wwn); virBufferEscapeString(buf, "<vendor>%s</vendor>\n", def->vendor); virBufferEscapeString(buf, "<product>%s</product>\n", def->product); + + /* Only for the older QCOW encryption - format the <encryption> + * as a child of <disk>. Others will now format as child of <source> */ if (def->src->encryption && + def->src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_QCOW && virStorageEncryptionFormat(buf, def->src->encryption) < 0) return -1; virDomainDeviceInfoFormat(buf, &def->info, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.args b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.args new file mode 100644 index 000000000..fec46945c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks-source.args @@ -0,0 +1,62 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-x86_64 \ +-name encryptdisk \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-encryptdisk/master-key.aes \ +-M pc-i440fx-2.1 \ +-m 1024 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid 496898a6-e6ff-f7c8-5dc2-3cf410945ee9 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,\ +path=/tmp/lib/domain--1-encryptdisk/monitor.sock,server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-object secret,id=virtio-disk0-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=/storage/guest_disks/encryptdisk,\ +key-secret=virtio-disk0-luks-secret0,format=luks,if=none,id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-object secret,id=virtio-disk1-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=/storage/guest_disks/encryptdisk2,\ +key-secret=virtio-disk1-luks-secret0,format=luks,if=none,id=drive-virtio-disk1 \ +-device virtio-blk-pci,bus=pci.0,addr=0x5,drive=drive-virtio-disk1,\ +id=virtio-disk1 \ +-object secret,id=virtio-disk2-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=iscsi://myname:AQCVn5hO6HzFAhAAq0NCv8jtJcIcE+HOBlMQ1A@example.org:\ +6000/iqn.1992-01.com.example%3Astorage/1,key-secret=virtio-disk2-luks-secret0,\ +format=luks,if=none,id=drive-virtio-disk2 \ +-device virtio-blk-pci,bus=pci.0,addr=0x6,drive=drive-virtio-disk2,\ +id=virtio-disk2 \ +-object secret,id=virtio-disk3-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file=iscsi://iscsi.example.com:3260/demo-target/3,\ +key-secret=virtio-disk3-luks-secret0,format=luks,if=none,id=drive-virtio-disk3 \ +-device virtio-blk-pci,bus=pci.0,addr=0x7,drive=drive-virtio-disk3,\ +id=virtio-disk3 \ +-object secret,id=virtio-disk4-luks-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive 'file=rbd:pool/image:auth_supported=none:mon_host=mon1.example.org\:\ +6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\ +key-secret=virtio-disk4-luks-secret0,format=luks,if=none,\ +id=drive-virtio-disk4' \ +-device virtio-blk-pci,bus=pci.0,addr=0x8,drive=drive-virtio-disk4,\ +id=virtio-disk4 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 9a8caaa38..d7d9270d6 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1648,6 +1648,7 @@ mymain(void) DO_TEST("encrypted-disk-usage", NONE); # ifdef HAVE_GNUTLS_CIPHER_ENCRYPT DO_TEST("luks-disks", QEMU_CAPS_OBJECT_SECRET); + DO_TEST("luks-disks-source", QEMU_CAPS_OBJECT_SECRET); # else DO_TEST_FAILURE("luks-disks", QEMU_CAPS_OBJECT_SECRET); # endif diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks-source.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks-source.xml new file mode 100644 index 000000000..1cad3af7a --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks-source.xml @@ -0,0 +1,84 @@ +<domain type='qemu'> + <name>encryptdisk</name> + <uuid>496898a6-e6ff-f7c8-5dc2-3cf410945ee9</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.1'>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='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/storage/guest_disks/encryptdisk'> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + </source> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/storage/guest_disks/encryptdisk2'> + <encryption format='luks'> + <secret type='passphrase' usage='/storage/guest_disks/encryptdisk2'/> + </encryption> + </source> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='6000'/> + <auth username='myname'> + <secret type='iscsi' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80e80'/> + </auth> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80f77'/> + </encryption> + </source> + <target dev='vdc' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/> + </disk> + <disk type='volume' device='disk'> + <driver name='qemu' type='raw'/> + <source pool='pool-iscsi' volume='unit:0:0:3' mode='direct'> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80f80'/> + </encryption> + </source> + <target dev='vdd' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x07' function='0x0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80fb0'/> + </encryption> + </source> + <target dev='vde' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x08' function='0x0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml deleted file mode 120000 index b59dc672f..000000000 --- a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml +++ /dev/null @@ -1 +0,0 @@ -../qemuxml2argvdata/qemuxml2argv-luks-disks.xml \ No newline at end of file diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml new file mode 100644 index 000000000..a16a550b0 --- /dev/null +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-luks-disks.xml @@ -0,0 +1,47 @@ +<domain type='qemu'> + <name>encryptdisk</name> + <uuid>496898a6-e6ff-f7c8-5dc2-3cf410945ee9</uuid> + <memory unit='KiB'>1048576</memory> + <currentMemory unit='KiB'>524288</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='x86_64' machine='pc-i440fx-2.1'>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='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/storage/guest_disks/encryptdisk'> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + </source> + <target dev='vda' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw'/> + <source file='/storage/guest_disks/encryptdisk2'> + <encryption format='luks'> + <secret type='passphrase' usage='/storage/guest_disks/encryptdisk2'/> + </encryption> + </source> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x05' function='0x0'/> + </disk> + <controller type='usb' index='0'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='virtio'> + <address type='pci' domain='0x0000' bus='0x00' slot='0x03' function='0x0'/> + </memballoon> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 90ffb040b..110fb12ea 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -576,6 +576,7 @@ mymain(void) DO_TEST("encrypted-disk", NONE); DO_TEST("encrypted-disk-usage", NONE); DO_TEST("luks-disks", NONE); + DO_TEST("luks-disks-source", NONE); DO_TEST("memtune", NONE); DO_TEST("memtune-unlimited", NONE); DO_TEST("blkiotune", NONE); -- 2.13.5

On Fri, Sep 15, 2017 at 20:30:11 -0400, John Ferlan wrote:
Alter the output of the formatting to be a child of the disk's source rather than a child of the disk for LUKS encryption, but keep the legacy QCOW encryption as a child of disk.
NACK, see 4/14

Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/news.xml | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/docs/news.xml b/docs/news.xml index e79ff4349..8ed0509e6 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -36,6 +36,19 @@ however, when writing out the XML the new format will be generated. </description> </change> + <change> + <summary> + conf: Move the encryption definition to disk source + </summary> + <description> + Rather than having the <code>encryption</code> element for the "luks" + encryption format be a child of the <code>disk</code> element move + to a child of a <code>source</code> element. Still recognize on input + the legacy child dependency; however, when writing out the XML the + new format will be generated. This does not apply to the legacy + "qcow" (or default) encryption format. + </description> + </change> </section> <section title="Bug fixes"> </section> -- 2.13.5

Since the encryption information can also be disk source specific move it from _qemuDomainDiskPrivate to _qemuDomainDiskSrcPrivate. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 6 ++---- src/qemu/qemu_domain.c | 16 +++------------- src/qemu/qemu_domain.h | 10 +++++----- src/qemu/qemu_hotplug.c | 8 ++------ 4 files changed, 12 insertions(+), 28 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 898a60f3b..c851823e7 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1339,10 +1339,9 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, virQEMUCapsPtr qemuCaps) { int actualType = virStorageSourceGetActualType(disk->src); - qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainDiskSrcPrivatePtr diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(disk->src); qemuDomainSecretInfoPtr secinfo = diskSrcPriv->secinfo; - qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo; + qemuDomainSecretInfoPtr encinfo = diskSrcPriv->encinfo; virJSONValuePtr srcprops = NULL; char *source = NULL; int ret = -1; @@ -2171,10 +2170,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, unsigned int bootindex = 0; bool driveBoot = false; virDomainDiskDefPtr disk = def->disks[i]; - qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainDiskSrcPrivatePtr diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(disk->src); qemuDomainSecretInfoPtr secinfo = diskSrcPriv->secinfo; - qemuDomainSecretInfoPtr encinfo = diskPriv->encinfo; + qemuDomainSecretInfoPtr encinfo = diskSrcPriv->encinfo; /* PowerPC pseries based VMs do not support floppy device */ if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY && diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 94b6d87d6..9e465aa68 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -884,7 +884,6 @@ qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo) static virClassPtr qemuDomainDiskPrivateClass; -static void qemuDomainDiskPrivateDispose(void *obj); static int qemuDomainDiskPrivateOnceInit(void) @@ -892,7 +891,7 @@ qemuDomainDiskPrivateOnceInit(void) qemuDomainDiskPrivateClass = virClassNew(virClassForObject(), "qemuDomainDiskPrivate", sizeof(qemuDomainDiskPrivate), - qemuDomainDiskPrivateDispose); + NULL); if (!qemuDomainDiskPrivateClass) return -1; else @@ -916,15 +915,6 @@ qemuDomainDiskPrivateNew(void) } -static void -qemuDomainDiskPrivateDispose(void *obj) -{ - qemuDomainDiskPrivatePtr priv = obj; - - qemuDomainSecretInfoFree(&priv->encinfo); -} - - static virClassPtr qemuDomainDiskSrcPrivateClass; static void qemuDomainDiskSrcPrivateDispose(void *obj); @@ -964,6 +954,7 @@ qemuDomainDiskSrcPrivateDispose(void *obj) qemuDomainDiskSrcPrivatePtr priv = obj; qemuDomainSecretInfoFree(&priv->secinfo); + qemuDomainSecretInfoFree(&priv->encinfo); } @@ -1384,7 +1375,6 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, virDomainDiskDefPtr disk) { virStorageSourcePtr src = disk->src; - qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); qemuDomainDiskSrcPrivatePtr diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(disk->src); if (qemuDomainSecretDiskCapable(src)) { @@ -1401,7 +1391,7 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, } if (qemuDomainDiskHasEncryptionSecret(src)) { - if (!(diskPriv->encinfo = + if (!(diskSrcPriv->encinfo = qemuDomainSecretInfoNew(conn, priv, disk->info.alias, VIR_SECRET_USAGE_TYPE_VOLUME, NULL, &src->encryption->secrets[0]->seclookupdef, diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index d212812dd..853384236 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -346,11 +346,6 @@ struct _qemuDomainDiskPrivate { bool migrating; /* the disk is being migrated */ - /* for storage devices using encryption/secret - * Can have both <auth> and <encryption> for some disks - * NB:*not* to be written to qemu domain object XML */ - qemuDomainSecretInfoPtr encinfo; - /* information about the device */ bool tray; /* device has tray */ bool removable; /* device media can be removed/changed */ @@ -367,6 +362,11 @@ struct _qemuDomainDiskSrcPrivate { /* for each storage source using auth/secret * NB: *not* to be written to qemu domain object XML */ qemuDomainSecretInfoPtr secinfo; + + /* for storage devices using encryption/secret + * Can have both <auth> and <encryption> for some disks + * NB:*not* to be written to qemu domain object XML */ + qemuDomainSecretInfoPtr encinfo; }; # define QEMU_DOMAIN_HOSTDEV_PRIVATE(hostdev) \ diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 2942772c2..7cc595161 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -329,7 +329,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, const char *src = virDomainDiskGetSource(disk); virJSONValuePtr secobjProps = NULL; virJSONValuePtr encobjProps = NULL; - qemuDomainDiskPrivatePtr diskPriv; qemuDomainDiskSrcPrivatePtr diskSrcPriv; qemuDomainSecretInfoPtr secinfo; qemuDomainSecretInfoPtr encinfo; @@ -367,7 +366,6 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) goto error; - diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(disk->src); secinfo = diskSrcPriv->secinfo; if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { @@ -375,7 +373,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, goto error; } - encinfo = diskPriv->encinfo; + encinfo = diskSrcPriv->encinfo; if (encinfo && qemuBuildSecretInfoProps(encinfo, &encobjProps) < 0) goto error; @@ -623,7 +621,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); virJSONValuePtr encobjProps = NULL; virJSONValuePtr secobjProps = NULL; - qemuDomainDiskPrivatePtr diskPriv; qemuDomainDiskSrcPrivatePtr diskSrcPriv; qemuDomainSecretInfoPtr encinfo; qemuDomainSecretInfoPtr secinfo; @@ -657,7 +654,6 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) goto error; - diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(disk->src); secinfo = diskSrcPriv->secinfo; if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { @@ -665,7 +661,7 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, goto error; } - encinfo = diskPriv->encinfo; + encinfo = diskSrcPriv->encinfo; if (encinfo && qemuBuildSecretInfoProps(encinfo, &encobjProps) < 0) goto error; -- 2.13.5

On Fri, Sep 15, 2017 at 20:30:13 -0400, John Ferlan wrote:
Since the encryption information can also be disk source specific move it from _qemuDomainDiskPrivate to _qemuDomainDiskSrcPrivate.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 6 ++---- src/qemu/qemu_domain.c | 16 +++------------- src/qemu/qemu_domain.h | 10 +++++----- src/qemu/qemu_hotplug.c | 8 ++------ 4 files changed, 12 insertions(+), 28 deletions(-)
ACK

Currently when an AES secret object is added to the domain for either a network disk, a LUKS encryption secret, or for a SCSI hostdev there is no way for domain restart to be able to connect or determine which secret by secrettype and uuid or usage was used in order to generate the object. So, in order to be able to lookup which secret generated the object, this patch will create and manage a private object hash table that tracks when a disk using the secret object is added or removed in order to be able to "rebuild" the secret. This information will be recorded in the domain private XML file so that when libvirtd restarts, we can rebuild and determine which secret was used. The qemuDomainObjDiskSecretObjectAliasEntryLookup helper is currently unused, but it's purpose would be to find the object alias in the table and return the usageType and seclookupdef that created the object. Could be quite useful for something. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 293 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 6 + src/qemu/qemu_hotplug.c | 29 ++++- 3 files changed, 322 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9e465aa68..29882bbfb 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -43,6 +43,7 @@ #include "domain_event.h" #include "virtime.h" #include "virnetdevopenvswitch.h" +#include "virsecret.h" #include "virstoragefile.h" #include "virstring.h" #include "virthreadjob.h" @@ -123,6 +124,15 @@ struct _qemuDomainLogContext { virLogManagerPtr manager; }; + +typedef struct _qemuDomainDiskSecretObjectAliasEntry qemuDomainDiskSecretObjectAliasEntry; +typedef qemuDomainDiskSecretObjectAliasEntry *qemuDomainDiskSecretObjectAliasEntryPtr; +struct _qemuDomainDiskSecretObjectAliasEntry { + virSecretUsageType usageType; + virSecretLookupTypeDef seclookupdef; +}; + + static virClassPtr qemuDomainLogContextClass; static virClassPtr qemuDomainSaveCookieClass; @@ -626,6 +636,255 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, } +/* Free memory used by an entry */ +static void +qemuDomainObjDiskSecretObjectAliasEntryFree(void *payload, + const void *name ATTRIBUTE_UNUSED) +{ + qemuDomainDiskSecretObjectAliasEntryPtr entry = payload; + + if (!entry) + return; + + virSecretLookupDefClear(&entry->seclookupdef); +} + + +/* qemuDomainObjDiskSecretObjectAliasEntryInsert: + * @priv: Pointer to qemuDomainPrivateData + * @objalias: Alias used to create the Disk Secret object + * @usageType: Secret usage type + * @seclookupdef: Secret lookup def use to create the secret object + * + * Using the provided @objalias, create or update the entry in the + * @priv->diskSecretObjectAlias hash table with the provided + * @usageType and @seclookupdef used to create the disk secret object. + * + * Returns 0 on success, -1 on failure + */ +static int +qemuDomainObjDiskSecretObjectAliasEntryInsert(qemuDomainObjPrivatePtr priv, + const char *objalias, + virSecretUsageType usageType, + virSecretLookupTypeDefPtr seclookupdef) +{ + qemuDomainDiskSecretObjectAliasEntryPtr entry = NULL; + + if ((entry = virHashLookup(priv->diskSecretObjectAlias, objalias))) { + /* Replace the previous */ + virSecretLookupDefClear(&entry->seclookupdef); + } else { + if (VIR_ALLOC(entry) < 0) + return -1; + + if (virHashAddEntry(priv->diskSecretObjectAlias, objalias, entry) < 0) { + VIR_FREE(entry); + return -1; + } + } + entry->usageType = usageType; + virSecretLookupDefCopy(&entry->seclookupdef, seclookupdef); + + return 0; +} + + +/* qemuDomainObjDiskSecretObjectAliasEntryLookup: + * @priv: Pointer to qemuDomainPrivateData + * @alias: Alias used to create the Disk Secret object + * @usageType: Secret usage type + * @seclookupdef: Secret lookup def use to create the secret object + * + * Using the provided @alias, lookup the entry in the + * @priv->diskSecretObjectAlias hash table. If found fill in + * @usageType and @seclookupdef. + * + * Returns 0 on success, -1 on failure + */ +static int ATTRIBUTE_UNUSED +qemuDomainObjDiskSecretObjectAliasEntryLookup(qemuDomainObjPrivatePtr priv, + const char *objalias, + virSecretUsageType *usageType, + virSecretLookupTypeDefPtr seclookupdef) +{ + qemuDomainDiskSecretObjectAliasEntryPtr entry = NULL; + + if (!(entry = virHashLookup(priv->diskSecretObjectAlias, objalias))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find disk secret object alias '%s'"), + objalias); + return -1; + } + + *usageType = entry->usageType; + virSecretLookupDefCopy(seclookupdef, &entry->seclookupdef); + return 0; +} + + +/* qemuDomainObjDiskSecretObjectAliasEntryRemove: + * @priv: Pointer to qemuDomainPrivateData + * @alias: Alias used to create the Disk Secret object + * + * Using the provided @alias, remove the entry in the + * @priv->diskSecretObjectAlias hash table. + * + * Returns 0 on success, -1 on failure + */ +int +qemuDomainObjDiskSecretObjectAliasEntryRemove(qemuDomainObjPrivatePtr priv, + const char *objalias) +{ + qemuDomainDiskSecretObjectAliasEntryPtr entry = NULL; + + if (!(entry = virHashLookup(priv->diskSecretObjectAlias, objalias))) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find disk secret object alias '%s'"), + objalias); + return -1; + } + + ignore_value(virHashRemoveEntry(priv->diskSecretObjectAlias, objalias)); + + return 0; +} + + +/* virHashForEach callback to write out each entry */ +static int +qemuDomainObjDiskSecretObjectAliasEntryFormat(void *payload, + const void *name, + void *opaque) +{ + qemuDomainDiskSecretObjectAliasEntryPtr entry = payload; + const char *objalias = name; + virBufferPtr buf = opaque; + const char *secrettype = virSecretUsageTypeToString(entry->usageType); + + virBufferAsprintf(buf, "<diskObject alias='%s'>\n", objalias); + virBufferAdjustIndent(buf, 2); + virSecretLookupFormatSecret(buf, secrettype, &entry->seclookupdef); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</diskObject>\n"); + return 0; +} + + +/* qemuDomainObjPrivateXMLFormatDiskSecretObjectAlias: + * @buf: data buffer to write into + * @priv: Pointer to qemuDomainPrivateData + * + * Format the table to the domain private XML file - if there's + * any entries + */ +static void +qemuDomainObjPrivateXMLFormatDiskSecretObjectAlias(virBufferPtr buf, + qemuDomainObjPrivatePtr priv) +{ + if (virHashSize(priv->diskSecretObjectAlias) <= 0) + return; + + virBufferAddLit(buf, "<diskSecretObjectAlias>\n"); + virBufferAdjustIndent(buf, 2); + virHashForEach(priv->diskSecretObjectAlias, + qemuDomainObjDiskSecretObjectAliasEntryFormat, buf); + virBufferAdjustIndent(buf, -2); + virBufferAddLit(buf, "</diskSecretObjectAlias>\n"); +} + + +static int +qemuDomainObjPrivateXMLParseDiskObject(xmlXPathContextPtr ctxt, + xmlNodePtr node, + qemuDomainObjPrivatePtr priv) +{ + int ret = -1; + char *objalias = NULL; + xmlNodePtr saved = ctxt->node; + xmlNodePtr secretnode = NULL; + const char *secrettype = NULL; + virSecretUsageType usageType; + virSecretLookupTypeDef seclookupdef = { 0 }; + + ctxt->node = node; + + if (!(objalias = virXMLPropString(node, "alias"))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("missing 'alias' property for diskObject")); + return -1; + } + + if (!(secretnode = virXPathNode("./secret[1]", ctxt))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("missing <secret> element")); + goto cleanup; + } + + if (virSecretLookupParseSecret(secretnode, &seclookupdef) < 0) + goto cleanup; + + if (!(secrettype = virXMLPropString(secretnode, "type"))) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("missing secret type attribute")); + goto cleanup; + } + + if ((usageType = virSecretUsageTypeFromString(secrettype)) < 0) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("invalid secret type %s"), secrettype); + goto cleanup; + } + + if (qemuDomainObjDiskSecretObjectAliasEntryInsert(priv, objalias, + usageType, + &seclookupdef) < 0) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(objalias); + virSecretLookupDefClear(&seclookupdef); + ctxt->node = saved; + return ret; + +} + + +/* qemuDomainObjPrivateXMLParseDiskSecretObjectAlias: + * @ctx: xml context + * @priv: Pointer to qemuDomainPrivateData + * + * Parse the domain object xml looking for disk secret object entries. + * If found, add them into the hash table. + * + * Returns 0 on success, -1 on failure + */ +static int +qemuDomainObjPrivateXMLParseDiskSecretObjectAlias(xmlXPathContextPtr ctxt, + qemuDomainObjPrivatePtr priv) +{ + int ret = -1; + int n; + size_t i; + xmlNodePtr *nodes = NULL; + + if ((n = virXPathNodeSet("./diskSecretObjectAlias/diskObject", + ctxt, &nodes)) < 0) + return -1; + + for (i = 0; i < n; i++) { + if (qemuDomainObjPrivateXMLParseDiskObject(ctxt, nodes[i], priv) < 0) + goto cleanup; + } + ret = 0; + + cleanup: + VIR_FREE(nodes); + return ret; +} + + /* qemuDomainGetMasterKeyFilePath: * @libDir: Directory path to domain lib files * @@ -1388,6 +1647,14 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, usageType, src->auth->username, &src->auth->seclookupdef, false))) return -1; + + if (diskSrcPriv->secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + if (qemuDomainObjDiskSecretObjectAliasEntryInsert(priv, + diskSrcPriv->secinfo->s.aes.alias, + usageType, + &src->auth->seclookupdef) < 0) + return -1; + } } if (qemuDomainDiskHasEncryptionSecret(src)) { @@ -1397,6 +1664,12 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, &src->encryption->secrets[0]->seclookupdef, true))) return -1; + + if (qemuDomainObjDiskSecretObjectAliasEntryInsert(priv, + diskSrcPriv->encinfo->s.aes.alias, + VIR_SECRET_USAGE_TYPE_VOLUME, + &src->encryption->secrets[0]->seclookupdef)) + return -1; } return 0; @@ -1452,6 +1725,14 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, &iscsisrc->auth->seclookupdef, false))) return -1; + + if (hostdevPriv->secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + if (qemuDomainObjDiskSecretObjectAliasEntryInsert(priv, + hostdevPriv->secinfo->s.aes.alias, + VIR_SECRET_USAGE_TYPE_ISCSI, + &iscsisrc->auth->seclookupdef) < 0) + return -1; + } } } @@ -1742,12 +2023,17 @@ qemuDomainObjPrivateAlloc(void *opaque) if (!(priv->devs = virChrdevAlloc())) goto error; + if (!(priv->diskSecretObjectAlias = + virHashCreate(15, qemuDomainObjDiskSecretObjectAliasEntryFree))) + goto error; + priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; priv->driver = opaque; return priv; error: + virChrdevFree(priv->devs); VIR_FREE(priv); return NULL; } @@ -1795,6 +2081,8 @@ qemuDomainObjPrivateFree(void *data) virCPUDefFree(priv->origCPU); + virHashFree(priv->diskSecretObjectAlias); + VIR_FREE(priv); } @@ -1981,6 +2269,8 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, if (priv->chardevStdioLogd) virBufferAddLit(buf, "<chardevStdioLogd/>\n"); + qemuDomainObjPrivateXMLFormatDiskSecretObjectAlias(buf, priv); + return 0; } @@ -2287,6 +2577,9 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, priv->chardevStdioLogd = virXPathBoolean("boolean(./chardevStdioLogd)", ctxt) == 1; + if (qemuDomainObjPrivateXMLParseDiskSecretObjectAlias(ctxt, priv) < 0) + goto error; + return 0; error: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 853384236..3eb528cae 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -321,6 +321,8 @@ struct _qemuDomainObjPrivate { /* If true virtlogd is used as stdio handler for character devices. */ bool chardevStdioLogd; + + virHashTablePtr diskSecretObjectAlias; }; # define QEMU_DOMAIN_PRIVATE(vm) \ @@ -797,6 +799,10 @@ void qemuDomainClearPrivatePaths(virDomainObjPtr vm); virDomainDiskDefPtr qemuDomainDiskByName(virDomainDefPtr def, const char *name); +int +qemuDomainObjDiskSecretObjectAliasEntryRemove(qemuDomainObjPrivatePtr priv, + const char *objalias); + char *qemuDomainGetMasterKeyFilePath(const char *libDir); int qemuDomainMasterKeyReadFile(qemuDomainObjPrivatePtr priv); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 7cc595161..a544cecb9 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -443,10 +443,16 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, VIR_WARN("Unable to remove drive %s (%s) after failed " "qemuMonitorAddDevice", drivealias, drivestr); } - if (secobjAdded) + if (secobjAdded) { ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias)); - if (encobjAdded) + qemuDomainObjDiskSecretObjectAliasEntryRemove(priv, + secinfo->s.aes.alias); + } + if (encobjAdded) { ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); + qemuDomainObjDiskSecretObjectAliasEntryRemove(priv, + encinfo->s.aes.alias); + } if (qemuDomainObjExitMonitor(driver, vm) < 0) releaseaddr = false; virErrorRestore(&orig_err); @@ -728,10 +734,16 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, VIR_WARN("Unable to remove drive %s (%s) after failed " "qemuMonitorAddDevice", drivealias, drivestr); } - if (secobjAdded) + if (secobjAdded) { ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias)); - if (encobjAdded) + qemuDomainObjDiskSecretObjectAliasEntryRemove(priv, + secinfo->s.aes.alias); + } + if (encobjAdded) { ignore_value(qemuMonitorDelObject(priv->mon, encinfo->s.aes.alias)); + qemuDomainObjDiskSecretObjectAliasEntryRemove(priv, + encinfo->s.aes.alias); + } ignore_value(qemuDomainObjExitMonitor(driver, vm)); virErrorRestore(&orig_err); @@ -3669,13 +3681,18 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, VIR_FREE(drivestr); /* If it fails, then so be it - it was a best shot */ - if (objAlias) + if (objAlias) { ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); + qemuDomainObjDiskSecretObjectAliasEntryRemove(priv, objAlias); + } + VIR_FREE(objAlias); /* If it fails, then so be it - it was a best shot */ - if (encAlias) + if (encAlias) { ignore_value(qemuMonitorDelObject(priv->mon, encAlias)); + qemuDomainObjDiskSecretObjectAliasEntryRemove(priv, encAlias); + } VIR_FREE(encAlias); if (qemuDomainObjExitMonitor(driver, vm) < 0) -- 2.13.5

On Fri, Sep 15, 2017 at 20:30:14 -0400, John Ferlan wrote:
Currently when an AES secret object is added to the domain for either a network disk, a LUKS encryption secret, or for a SCSI hostdev there is no way for domain restart to be able to connect or determine which secret by secrettype and uuid or usage was used in order to generate the object.
So, in order to be able to lookup which secret generated the object, this patch will create and manage a private object hash table that tracks when a disk using the secret object is added or removed in order to be able to "rebuild" the secret.
This information will be recorded in the domain private XML file so that when libvirtd restarts, we can rebuild and determine which secret was used.
The qemuDomainObjDiskSecretObjectAliasEntryLookup helper is currently unused, but it's purpose would be to find the object alias in the table and return the usageType and seclookupdef that created the object. Could be quite useful for something.
I don't quite see the reason to add all this code. All secret related stuff can be stored in the virStorageSource private data. If you need to format and load it, you can iterate them in the private XML formatter and don't need any hash table. Additionally you should not need to be able to identify which secret was used once the VM is running, since we don't access it afterwards. Also the secret data can become stale since user may undefine the secret. Looks like this is quite a lot of code without any real use case, thus I'm inclined to a NACK, if you don't provide any more supporting explanation.

On 09/22/2017 05:44 AM, Peter Krempa wrote:
On Fri, Sep 15, 2017 at 20:30:14 -0400, John Ferlan wrote:
Currently when an AES secret object is added to the domain for either a network disk, a LUKS encryption secret, or for a SCSI hostdev there is no way for domain restart to be able to connect or determine which secret by secrettype and uuid or usage was used in order to generate the object.
So, in order to be able to lookup which secret generated the object, this patch will create and manage a private object hash table that tracks when a disk using the secret object is added or removed in order to be able to "rebuild" the secret.
This information will be recorded in the domain private XML file so that when libvirtd restarts, we can rebuild and determine which secret was used.
The qemuDomainObjDiskSecretObjectAliasEntryLookup helper is currently unused, but it's purpose would be to find the object alias in the table and return the usageType and seclookupdef that created the object. Could be quite useful for something.
I don't quite see the reason to add all this code. All secret related stuff can be stored in the virStorageSource private data. If you need to format and load it, you can iterate them in the private XML formatter and don't need any hash table.
Additionally you should not need to be able to identify which secret was used once the VM is running, since we don't access it afterwards. Also the secret data can become stale since user may undefine the secret.
Looks like this is quite a lot of code without any real use case, thus I'm inclined to a NACK, if you don't provide any more supporting explanation.
No special reason - I wasn't sure if being able to look up the secret from an alias provided was going to be useful or not. I did discuss this in the cover, but IIRC you don't always read those ;-). John

Add the capability to use the blockdev-add query-qmp-schema option to find the 'password-secret' parameter that will allow the iSCSI code to use the master secret object (a/k/a AES) to encrypt the secret in an object and only need to provide the object id of tha secret on the command line thus obsfuscating the passphrase. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 3 +++ tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 6 files changed, 11 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index c690cb349..acf8799b3 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -439,6 +439,9 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "virtio-net.tx_queue_size", "chardev-reconnect", "virtio-gpu.max_outputs", + + /* 270 */ + "iscsi.password-secret", ); @@ -1810,6 +1813,7 @@ static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsIntelIOMMU[] = { static struct virQEMUCapsStringFlags virQEMUCapsQMPSchemaQueries[] = { { "blockdev-add/arg-type/options/+gluster/debug-level", QEMU_CAPS_GLUSTER_DEBUG_LEVEL}, { "blockdev-add/arg-type/+gluster/debug", QEMU_CAPS_GLUSTER_DEBUG_LEVEL}, + { "blockdev-add/arg-type/+iscsi/password-secret", QEMU_CAPS_ISCSI_PASSWORD_SECRET }, }; struct virQEMUCapsObjectTypeProps { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 85c390abf..c4e09522b 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -426,6 +426,9 @@ typedef enum { QEMU_CAPS_CHARDEV_RECONNECT, /* -chardev reconnect */ QEMU_CAPS_VIRTIO_GPU_MAX_OUTPUTS, /* -device virtio-(vga|gpu-*),max-outputs= */ + /* 270 */ + QEMU_CAPS_ISCSI_PASSWORD_SECRET, /* -drive file.driver=iscsi,...,password-secret= */ + QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml index 604921122..c341e56e9 100644 --- a/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml @@ -222,6 +222,7 @@ <flag name='virtio-net.tx_queue_size'/> <flag name='chardev-reconnect'/> <flag name='virtio-gpu.max_outputs'/> + <flag name='iscsi.password-secret'/> <version>2010000</version> <kvmVersion>0</kvmVersion> <package> (v2.10.0)</package> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml index a373a6db6..c5eb3951f 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml @@ -172,6 +172,7 @@ <flag name='vnc-multi-servers'/> <flag name='chardev-reconnect'/> <flag name='virtio-gpu.max_outputs'/> + <flag name='iscsi.password-secret'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <package> (v2.9.0)</package> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml index e80782cfb..99ad44ac5 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml @@ -137,6 +137,7 @@ <flag name='vnc-multi-servers'/> <flag name='chardev-reconnect'/> <flag name='virtio-gpu.max_outputs'/> + <flag name='iscsi.password-secret'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <package></package> diff --git a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml index 3641d0332..bd446ff27 100644 --- a/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml +++ b/tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml @@ -220,6 +220,7 @@ <flag name='vnc-multi-servers'/> <flag name='chardev-reconnect'/> <flag name='virtio-gpu.max_outputs'/> + <flag name='iscsi.password-secret'/> <version>2009000</version> <kvmVersion>0</kvmVersion> <package> (v2.9.0)</package> -- 2.13.5

On Fri, Sep 15, 2017 at 20:30:15 -0400, John Ferlan wrote:
Add the capability to use the blockdev-add query-qmp-schema option to find the 'password-secret' parameter that will allow the iSCSI code to use the master secret object (a/k/a AES) to encrypt the
AES is a name of the cipher, not an alias for qemu's secret objects.
secret in an object and only need to provide the object id of tha secret on the command line thus obsfuscating the passphrase.
s/tha/the/
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 4 ++++ src/qemu/qemu_capabilities.h | 3 +++ tests/qemucapabilitiesdata/caps_2.10.0.x86_64.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.ppc64le.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.s390x.xml | 1 + tests/qemucapabilitiesdata/caps_2.9.0.x86_64.xml | 1 + 6 files changed, 11 insertions(+)
ACK

Generate the example for the iSCSI auth/password-secret similar to what's done for RBD. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 30 ++++++++++++++++++++++++++++++ tests/virstoragetest.c | 15 +++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 39cda9c89..38aa77f44 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2937,10 +2937,13 @@ virStorageSourceParseBackingJSONiSCSI(virStorageSourcePtr src, const char *transport = virJSONValueObjectGetString(json, "transport"); const char *portal = virJSONValueObjectGetString(json, "portal"); const char *target = virJSONValueObjectGetString(json, "target"); + const char *user = virJSONValueObjectGetString(json, "user"); + const char *secret = virJSONValueObjectGetString(json, "password-secret"); const char *uri; char *port; unsigned int lun = 0; char *fulltarget = NULL; + virStorageAuthDefPtr authdef = NULL; int ret = -1; /* legacy URI based syntax passed via 'filename' option */ @@ -2993,10 +2996,37 @@ virStorageSourceParseBackingJSONiSCSI(virStorageSourcePtr src, VIR_STEAL_PTR(src->path, fulltarget); + if (user) { + if (!secret) { + virReportError(VIR_ERR_INVALID_ARG, + _("missing 'password-secret' in iSCSI backing " + "definition for user '%s'"), user); + goto cleanup; + } + + /* formulate authdef for src->auth */ + if (VIR_ALLOC(authdef) < 0) + goto cleanup; + + if (VIR_STRDUP(authdef->username, user) < 0) + goto cleanup; + + if (VIR_STRDUP(authdef->secrettype, + virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_ISCSI)) < 0) + goto cleanup; + src->auth = authdef; + authdef = NULL; + + /* Cannot formulate a secretType (eg, usage or uuid) given + * what is provided. + */ + } + ret = 0; cleanup: VIR_FREE(fulltarget); + virStorageAuthDefFree(authdef); return ret; } diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 46d12c0e6..1d3fc36c3 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -1577,6 +1577,21 @@ mymain(void) "<source protocol='iscsi' name='iqn.2016-12.com.virttest:emulated-iscsi-noauth.target/6'>\n" " <host name='test.org' port='1234'/>\n" "</source>\n"); + TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"iscsi\"," + "\"transport\":\"tcp\"," + "\"portal\":\"test.org:1234\"," + "\"target\":\"iqn.2016-12.com.virttest:emulated-iscsi-auth.target\"," + "\"lun\":6," + "\"user\":\"myname\"," + "\"password-secret\":\"virtio-disk1-secret0\"" + "}" + "}", + "<source protocol='iscsi' name='iqn.2016-12.com.virttest:emulated-iscsi-auth.target/6'>\n" + " <host name='test.org' port='1234'/>\n" + " <auth username='myname'>\n" + " <secret type='iscsi'/>\n" + " </auth>\n" + "</source>\n"); TEST_BACKING_PARSE("json:{\"file\":{\"driver\":\"sheepdog\"," "\"vdi\":\"test\"," "\"server\":{ \"type\":\"inet\"," -- 2.13.5

On Fri, Sep 15, 2017 at 20:30:16 -0400, John Ferlan wrote:
Generate the example for the iSCSI auth/password-secret similar to what's done for RBD.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virstoragefile.c | 30 ++++++++++++++++++++++++++++++ tests/virstoragetest.c | 15 +++++++++++++++ 2 files changed, 45 insertions(+)
NACK, the authentication data recorded in the backing string may (and most probably will) be stale at the point we are trying to reload it. Libvirt needs to provide fresh data anyways.

https://bugzilla.redhat.com/show_bug.cgi?id=1425757 The blockdev-add code provides a mechanism to sanely provide user and password-secret arguments for iscsi without placing them on the command line to be viewable by a 'ps -ef' type command or needing to create separate -iscsi devices for each disk/volume found. So modify the iSCSI command line building to check for the presence of the capability in order properly setup and use the domain master secret object to encrypt the password in a secret object and alter the parameters for the command line to utilize. Modify the xml2argvtest to exhibit the syntax for both disk and hostdev configurations. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_block.c | 64 ++++++++++++++++++- src/qemu/qemu_command.c | 73 +++++++++++++++++++--- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_domain.c | 4 ++ src/qemu/qemu_hotplug.c | 49 ++++++++++++++- ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 41 ++++++++++++ ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++ ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 45 +++++++++++++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++ tests/qemuxml2argvtest.c | 10 +++ 10 files changed, 366 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 7fb12ea5a..057fb8233 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -482,6 +482,64 @@ qemuBlockStorageSourceGetGlusterProps(virStorageSourcePtr src) } +static virJSONValuePtr +qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src) +{ + const char *protocol = virStorageNetProtocolTypeToString(src->protocol); + char *target = NULL; + char *lunStr = NULL; + char *username = NULL; + char *objalias = NULL; + unsigned int lun = 0; + virJSONValuePtr ret = NULL; + qemuDomainDiskSrcPrivatePtr diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(src); + + /* { driver:"iscsi", + * transport:"tcp" ("iser" also possible) + * portal:"example.com", + * target:"iqn.2017-04.com.example:iscsi-disks", + * lun:1, + * [user:"username", + * password-secret:"secret-alias",] + * } + */ + + if (VIR_STRDUP(target, src->path) < 0) + goto cleanup; + + /* Separate the target and lun */ + if ((lunStr = strchr(target, '/'))) { + *(lunStr++) = '\0'; + if (virStrToLong_ui(lunStr, NULL, 10, &lun) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse target for lunStr '%s'"), + target); + goto cleanup; + } + } + + if (src->auth) { + username = src->auth->username; + objalias = diskSrcPriv->secinfo->s.aes.alias; + } + + ignore_value(virJSONValueObjectCreate(&ret, + "s:driver", protocol, + "s:portal", src->hosts[0].name, + "s:target", target, + "u:lun", lun, + "s:transport", "tcp", + "S:user", username, + "S:password-secret", objalias, + NULL)); + goto cleanup; + + cleanup: + VIR_FREE(target); + return ret; +} + + /** * qemuBlockStorageSourceGetBackendProps: * @src: disk source @@ -512,10 +570,14 @@ qemuBlockStorageSourceGetBackendProps(virStorageSourcePtr src) goto cleanup; break; + case VIR_STORAGE_NET_PROTOCOL_ISCSI: + if (!(fileprops = qemuBlockStorageSourceGetISCSIProps(src))) + goto cleanup; + break; + case VIR_STORAGE_NET_PROTOCOL_NBD: case VIR_STORAGE_NET_PROTOCOL_RBD: case VIR_STORAGE_NET_PROTOCOL_SHEEPDOG: - case VIR_STORAGE_NET_PROTOCOL_ISCSI: case VIR_STORAGE_NET_PROTOCOL_HTTP: case VIR_STORAGE_NET_PROTOCOL_HTTPS: case VIR_STORAGE_NET_PROTOCOL_FTP: diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c851823e7..f9edf623c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1319,7 +1319,8 @@ qemuDiskBusNeedsDeviceArg(int bus) * the legacy representation. */ static bool -qemuDiskSourceNeedsProps(virStorageSourcePtr src) +qemuDiskSourceNeedsProps(virStorageSourcePtr src, + virQEMUCapsPtr qemuCaps) { int actualType = virStorageSourceGetActualType(src); @@ -1328,6 +1329,11 @@ qemuDiskSourceNeedsProps(virStorageSourcePtr src) src->nhosts > 1) return true; + if (actualType == VIR_STORAGE_TYPE_NETWORK && + src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET)) + return true; + return false; } @@ -1346,7 +1352,7 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, char *source = NULL; int ret = -1; - if (qemuDiskSourceNeedsProps(disk->src) && + if (qemuDiskSourceNeedsProps(disk->src, qemuCaps) && !(srcprops = qemuBlockStorageSourceGetBackendProps(disk->src))) goto cleanup; @@ -1412,7 +1418,9 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk, virBufferAsprintf(buf, "file.debug=%d,", cfg->glusterDebugLevel); } - if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES && + disk->src->type == VIR_STORAGE_TYPE_NETWORK && + disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { /* NB: If libvirt starts using the more modern option based * syntax to build the command line (e.g., "-drive driver=rbd, * filename=%s,...") instead of the legacy model (e.g."-drive @@ -4846,10 +4854,13 @@ qemuBuildSCSIHostHostdevDrvStr(virDomainHostdevDefPtr dev) } static char * -qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) +qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps) { + char *netsource = NULL; char *source = NULL; - virStorageSource src; + virStorageSource src = { 0 }; + virJSONValuePtr srcprops = NULL; qemuDomainHostdevPrivatePtr hostdevPriv = QEMU_DOMAIN_HOSTDEV_PRIVATE(dev); memset(&src, 0, sizeof(src)); @@ -4857,14 +4868,51 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; + src.type = VIR_STORAGE_TYPE_NETWORK; src.protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; src.path = iscsisrc->path; src.hosts = iscsisrc->hosts; src.nhosts = iscsisrc->nhosts; /* Rather than pull what we think we want - use the network disk code */ - source = qemuBuildNetworkDriveStr(&src, hostdevPriv->secinfo); + if (qemuDiskSourceNeedsProps(&src, qemuCaps)) { + /* The next pile of code hunts and gathers as if @src were a disk. + * In particular, using private data... So a bit more chicanery is + * going to be required */ + qemuDomainDiskSrcPrivatePtr diskSrcPriv; + + if (!iscsisrc->auth->username) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing username for iSCSI auth")); + goto cleanup; + } + src.auth = iscsisrc->auth; + + if (VIR_ALLOC(src.privateData) < 0) + goto cleanup; + diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(&src); + diskSrcPriv->secinfo = hostdevPriv->secinfo; + srcprops = qemuBlockStorageSourceGetBackendProps(&src); + VIR_FREE(src.privateData); + if (!srcprops) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to build the backend props")); + goto cleanup; + } + if (!(netsource = virQEMUBuildDriveCommandlineFromJSON(srcprops))) + goto cleanup; + if (virAsprintf(&source, "%s,if=none,format=raw", netsource) < 0) + goto cleanup; + } else { + if (!(netsource = qemuBuildNetworkDriveStr(&src, hostdevPriv->secinfo))) + goto cleanup; + if (virAsprintf(&source, "file=%s,if=none,format=raw", netsource) < 0) + goto cleanup; + } + + cleanup: + VIR_FREE(netsource); return source; } @@ -4907,7 +4955,8 @@ qemuBuildSCSIVHostHostdevDevStr(const virDomainDef *def, } char * -qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) +qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; char *source = NULL; @@ -4915,9 +4964,9 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { - if (!(source = qemuBuildSCSIiSCSIHostdevDrvStr(dev))) + if (!(source = qemuBuildSCSIiSCSIHostdevDrvStr(dev, qemuCaps))) goto error; - virBufferAsprintf(&buf, "file=%s,if=none,format=raw", source); + virBufferAsprintf(&buf, "%s", source); } else { if (!(source = qemuBuildSCSIHostHostdevDrvStr(dev))) goto error; @@ -5414,10 +5463,14 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, /* SCSI */ if (virHostdevIsSCSIDevice(hostdev)) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { + qemuDomainHostdevPrivatePtr hostdevPriv = QEMU_DOMAIN_HOSTDEV_PRIVATE(hostdev); char *drvstr; + if (qemuBuildDiskSecinfoCommandLine(cmd, hostdevPriv->secinfo) < 0) + return -1; + virCommandAddArg(cmd, "-drive"); - if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev))) + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps))) return -1; virCommandAddArg(cmd, drvstr); VIR_FREE(drvstr); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 6fbfb3e5f..0008da1cb 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -158,7 +158,8 @@ char *qemuBuildUSBHostdevDevStr(const virDomainDef *def, virDomainHostdevDefPtr dev, virQEMUCapsPtr qemuCaps); -char *qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev); +char *qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps); char *qemuBuildSCSIHostdevDevStr(const virDomainDef *def, virDomainHostdevDefPtr dev, diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 29882bbfb..94135218c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1479,9 +1479,13 @@ qemuDomainSecretSetup(virConnectPtr conn, virSecretLookupTypeDefPtr seclookupdef, bool isLuks) { + bool iscsiHasPS = virQEMUCapsGet(priv->qemuCaps, + QEMU_CAPS_ISCSI_PASSWORD_SECRET); + if (virCryptoHaveCipher(VIR_CRYPTO_CIPHER_AES256CBC) && virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) && (usageType == VIR_SECRET_USAGE_TYPE_CEPH || + (usageType == VIR_SECRET_USAGE_TYPE_ISCSI && iscsiHasPS) || usageType == VIR_SECRET_USAGE_TYPE_VOLUME || usageType == VIR_SECRET_USAGE_TYPE_TLS)) { if (qemuDomainSecretAESSetup(conn, priv, secinfo, srcalias, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a544cecb9..17228d1b4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2449,6 +2449,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, virDomainHostdevDefPtr hostdev) { size_t i; + int rv; int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; virErrorPtr orig_err; @@ -2458,7 +2459,12 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, bool teardowncgroup = false; bool teardownlabel = false; bool teardowndevice = false; + bool teardownsecobj = false; bool driveAdded = false; + bool secobjAdded = false; + virJSONValuePtr secobjProps = NULL; + qemuDomainHostdevPrivatePtr hostdevPriv; + qemuDomainSecretInfoPtr secinfo; if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", @@ -2499,7 +2505,15 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, if (qemuDomainSecretHostdevPrepare(conn, priv, hostdev) < 0) goto cleanup; - if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev))) + hostdevPriv = QEMU_DOMAIN_HOSTDEV_PRIVATE(hostdev); + secinfo = hostdevPriv->secinfo; + if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) { + if (qemuBuildSecretInfoProps(secinfo, &secobjProps) < 0) + goto cleanup; + teardownsecobj = true; + } + + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, priv->qemuCaps))) goto cleanup; if (!(drivealias = qemuAliasFromHostdev(hostdev))) @@ -2513,6 +2527,15 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, qemuDomainObjEnterMonitor(driver, vm); + if (secobjProps) { + rv = qemuMonitorAddObject(priv->mon, "secret", secinfo->s.aes.alias, + secobjProps); + secobjProps = NULL; /* qemuMonitorAddObject consumes */ + if (rv < 0) + goto exit_monitor; + secobjAdded = true; + } + if (qemuMonitorAddDrive(priv->mon, drvstr) < 0) goto exit_monitor; driveAdded = true; @@ -2530,7 +2553,6 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, ret = 0; cleanup: - qemuDomainSecretHostdevDestroy(hostdev); if (ret < 0) { qemuHostdevReAttachSCSIDevices(driver, vm->def->name, &hostdev, 1); if (teardowncgroup && qemuTeardownHostdevCgroup(vm, hostdev) < 0) @@ -2538,10 +2560,15 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, if (teardownlabel && qemuSecurityRestoreHostdevLabel(driver, vm, hostdev) < 0) VIR_WARN("Unable to restore host device labelling on hotplug fail"); + if (teardownsecobj) + qemuDomainObjDiskSecretObjectAliasEntryRemove(priv, + secinfo->s.aes.alias); if (teardowndevice && qemuDomainNamespaceTeardownHostdev(driver, vm, hostdev) < 0) VIR_WARN("Unable to remove host device from /dev"); } + qemuDomainSecretHostdevDestroy(hostdev); + virJSONValueFree(secobjProps); VIR_FREE(drivealias); VIR_FREE(drvstr); VIR_FREE(devstr); @@ -2554,6 +2581,8 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, "qemuMonitorAddDevice", drvstr, devstr); } + if (secobjAdded) + ignore_value(qemuMonitorDelObject(priv->mon, secinfo->s.aes.alias)); ignore_value(qemuDomainObjExitMonitor(driver, vm)); virErrorRestore(&orig_err); @@ -3865,6 +3894,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; char *drivealias = NULL; + char *objAlias = NULL; bool is_vfio = false; VIR_DEBUG("Removing host device %s from domain %p %s", @@ -3879,8 +3909,22 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, if (!(drivealias = qemuAliasFromHostdev(hostdev))) goto cleanup; + if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET)) { + if (!(objAlias = + qemuDomainGetSecretAESAlias(hostdev->info->alias, false))) { + return -1; + } + } + qemuDomainObjEnterMonitor(driver, vm); qemuMonitorDriveDel(priv->mon, drivealias); + + /* If it fails, then so be it - it was a best shot */ + if (objAlias) { + ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); + qemuDomainObjDiskSecretObjectAliasEntryRemove(priv, objAlias); + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; } @@ -3952,6 +3996,7 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, cleanup: VIR_FREE(drivealias); + VIR_FREE(objAlias); virObjectUnref(cfg); return ret; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args new file mode 100644 index 000000000..5bc5f4f47 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.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-i686 \ +-name QEMUGuest1 \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest1/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-usb \ +-object secret,id=virtio-disk0-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file.driver=iscsi,file.portal=example.org,\ +file.target=iqn.1992-01.com.example:storage,file.lun=1,file.transport=tcp,\ +file.user=myname,file.password-secret=virtio-disk0-secret0,format=raw,if=none,\ +id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-object secret,id=virtio-disk1-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file.driver=iscsi,file.portal=example.org,\ +file.target=iqn.1992-01.com.example:storage,file.lun=2,file.transport=tcp,\ +file.user=myname,file.password-secret=virtio-disk1-secret0,format=raw,if=none,\ +id=drive-virtio-disk1 \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk1,\ +id=virtio-disk1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml new file mode 100644 index 000000000..63919f100 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml @@ -0,0 +1,43 @@ +<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-i686</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> + <host name='example.org' port='6000'/> + </source> + <target dev='vdb' bus='virtio'/> + </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-hostdev-scsi-virtio-iscsi-auth-AES.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args new file mode 100644 index 000000000..c6051ecb0 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args @@ -0,0 +1,45 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu-system-i686 \ +-name QEMUGuest2 \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \ +-M pc \ +-m 214 \ +-smp 1,sockets=1,cores=1,threads=1 \ +-uuid c7a5fdbd-edaf-9466-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-chardev socket,id=charmonitor,path=/tmp/lib/domain--1-QEMUGuest2/monitor.sock,\ +server,nowait \ +-mon chardev=charmonitor,id=monitor,mode=readline \ +-no-acpi \ +-boot c \ +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-object secret,id=hostdev0-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file.driver=iscsi,file.portal=example.org,\ +file.target=iqn.1992-01.com.example:storage,file.lun=1,file.transport=tcp,\ +file.user=myname,file.password-secret=hostdev0-secret0,if=none,format=raw,\ +id=drive-hostdev0 \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=4,\ +drive=drive-hostdev0,id=hostdev0 \ +-object secret,id=hostdev1-secret0,\ +data=9eao5F8qtkGt+seB1HYivWIxbtwUu6MQtg1zpj/oDtUsPr1q8wBYM91uEHCn6j/1,\ +keyid=masterKey0,iv=AAECAwQFBgcICQoLDA0ODw==,format=base64 \ +-drive file.driver=iscsi,file.portal=example.org,\ +file.target=iqn.1992-01.com.example:storage,file.lun=2,file.transport=tcp,\ +file.user=myname,file.password-secret=hostdev1-secret0,if=none,format=raw,\ +id=drive-hostdev1 \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=5,\ +drive=drive-hostdev1,id=hostdev1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml new file mode 100644 index 000000000..0f63f9887 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</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-i686</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='3260'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <address type='drive' controller='0' bus='0' target='2' unit='4'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> + <host name='example.org' port='3260'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <address type='drive' controller='0' bus='0' target='2' unit='5'/> + </hostdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index d7d9270d6..f07557f20 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -922,6 +922,10 @@ mymain(void) DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-auth-wrong-secrettype", NONE); DO_TEST_PARSE_ERROR("disk-drive-network-iscsi-source-auth-both", NONE); DO_TEST_PARSE_ERROR("disk-drive-network-rbd-source-auth-both", NONE); +# ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + DO_TEST("disk-drive-network-iscsi-auth-AES", + QEMU_CAPS_OBJECT_SECRET, QEMU_CAPS_ISCSI_PASSWORD_SECRET); +# endif DO_TEST("disk-drive-network-iscsi-lun", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_BLOCK); @@ -2310,6 +2314,12 @@ mymain(void) DO_TEST("hostdev-scsi-virtio-iscsi-auth", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC); +# ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + DO_TEST("hostdev-scsi-virtio-iscsi-auth-AES", + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_DEVICE_SCSI_GENERIC, QEMU_CAPS_OBJECT_SECRET, + QEMU_CAPS_ISCSI_PASSWORD_SECRET); +# endif DO_TEST("hostdev-scsi-vhost-scsi-ccw", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_VHOST_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC, QEMU_CAPS_VIRTIO_CCW); -- 2.13.5

On Fri, Sep 15, 2017 at 20:30:17 -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1425757
The blockdev-add code provides a mechanism to sanely provide user and password-secret arguments for iscsi without placing them on the command line to be viewable by a 'ps -ef' type command or needing to create separate -iscsi devices for each disk/volume found.
So modify the iSCSI command line building to check for the presence of the capability in order properly setup and use the domain master secret object to encrypt the password in a secret object and alter the parameters for the command line to utilize.
Modify the xml2argvtest to exhibit the syntax for both disk and hostdev configurations.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_block.c | 64 ++++++++++++++++++- src/qemu/qemu_command.c | 73 +++++++++++++++++++--- src/qemu/qemu_command.h | 3 +- src/qemu/qemu_domain.c | 4 ++ src/qemu/qemu_hotplug.c | 49 ++++++++++++++- ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 41 ++++++++++++ ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++ ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 45 +++++++++++++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++ tests/qemuxml2argvtest.c | 10 +++ 10 files changed, 366 insertions(+), 14 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml
diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 7fb12ea5a..057fb8233 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -482,6 +482,64 @@ qemuBlockStorageSourceGetGlusterProps(virStorageSourcePtr src) }
+static virJSONValuePtr +qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src) +{ + const char *protocol = virStorageNetProtocolTypeToString(src->protocol); + char *target = NULL; + char *lunStr = NULL; + char *username = NULL; + char *objalias = NULL; + unsigned int lun = 0; + virJSONValuePtr ret = NULL; + qemuDomainDiskSrcPrivatePtr diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(src); + + /* { driver:"iscsi", + * transport:"tcp" ("iser" also possible) + * portal:"example.com", + * target:"iqn.2017-04.com.example:iscsi-disks", + * lun:1, + * [user:"username", + * password-secret:"secret-alias",]
As I've pointed out in the VxHS series, using array designators in an json example to mark "optional" fields is not a great idea.
+ * } + */ + + if (VIR_STRDUP(target, src->path) < 0) + goto cleanup; + + /* Separate the target and lun */ + if ((lunStr = strchr(target, '/'))) { + *(lunStr++) = '\0'; + if (virStrToLong_ui(lunStr, NULL, 10, &lun) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot parse target for lunStr '%s'"), + target); + goto cleanup; + } + } + + if (src->auth) { + username = src->auth->username; + objalias = diskSrcPriv->secinfo->s.aes.alias; + } + + ignore_value(virJSONValueObjectCreate(&ret, + "s:driver", protocol, + "s:portal", src->hosts[0].name, + "s:target", target, + "u:lun", lun, + "s:transport", "tcp", + "S:user", username, + "S:password-secret", objalias, + NULL)); + goto cleanup; + + cleanup: + VIR_FREE(target); + return ret; +}
[...]
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c851823e7..f9edf623c 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c
[...]
@@ -4846,10 +4854,13 @@ qemuBuildSCSIHostHostdevDrvStr(virDomainHostdevDefPtr dev) }
static char * -qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) +qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps) { + char *netsource = NULL; char *source = NULL; - virStorageSource src; + virStorageSource src = { 0 }; + virJSONValuePtr srcprops = NULL; qemuDomainHostdevPrivatePtr hostdevPriv = QEMU_DOMAIN_HOSTDEV_PRIVATE(dev);
memset(&src, 0, sizeof(src)); @@ -4857,14 +4868,51 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi; virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi;
+ src.type = VIR_STORAGE_TYPE_NETWORK; src.protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI; src.path = iscsisrc->path; src.hosts = iscsisrc->hosts; src.nhosts = iscsisrc->nhosts;
/* Rather than pull what we think we want - use the network disk code */ - source = qemuBuildNetworkDriveStr(&src, hostdevPriv->secinfo); + if (qemuDiskSourceNeedsProps(&src, qemuCaps)) { + /* The next pile of code hunts and gathers as if @src were a disk. + * In particular, using private data... So a bit more chicanery is + * going to be required */ + qemuDomainDiskSrcPrivatePtr diskSrcPriv; + + if (!iscsisrc->auth->username) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing username for iSCSI auth")); + goto cleanup; + } + src.auth = iscsisrc->auth; + + if (VIR_ALLOC(src.privateData) < 0) + goto cleanup;
src.privateData is a virObjectPtr. This won't work as you expect it to work. I'd say it'll crash (or corrupt heap) since ...
+ diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(&src);
this typecasts the virObjectPtr into a _qemuDomainDiskSrcPrivate pointer ..
+ diskSrcPriv->secinfo = hostdevPriv->secinfo;
And this points 8 bytes after the allocated data, as _qemuDomainDiskSrcPrivate has a virObject folowed by the "secinfo" pointer.
+ srcprops = qemuBlockStorageSourceGetBackendProps(&src); + VIR_FREE(src.privateData); + if (!srcprops) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to build the backend props")); + goto cleanup; + }
+ if (!(netsource = virQEMUBuildDriveCommandlineFromJSON(srcprops))) + goto cleanup; + if (virAsprintf(&source, "%s,if=none,format=raw", netsource) < 0) + goto cleanup; + } else { + if (!(netsource = qemuBuildNetworkDriveStr(&src, hostdevPriv->secinfo))) + goto cleanup; + if (virAsprintf(&source, "file=%s,if=none,format=raw", netsource) < 0) + goto cleanup; + } + + cleanup: + VIR_FREE(netsource); return source; }
@@ -4907,7 +4955,8 @@ qemuBuildSCSIVHostHostdevDevStr(const virDomainDef *def, }
char * -qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) +qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, + virQEMUCapsPtr qemuCaps) { virBuffer buf = VIR_BUFFER_INITIALIZER; char *source = NULL; @@ -4915,9 +4964,9 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev) virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { - if (!(source = qemuBuildSCSIiSCSIHostdevDrvStr(dev))) + if (!(source = qemuBuildSCSIiSCSIHostdevDrvStr(dev, qemuCaps))) goto error; - virBufferAsprintf(&buf, "file=%s,if=none,format=raw", source); + virBufferAsprintf(&buf, "%s", source); } else { if (!(source = qemuBuildSCSIHostHostdevDrvStr(dev))) goto error; @@ -5414,10 +5463,14 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, /* SCSI */ if (virHostdevIsSCSIDevice(hostdev)) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { + qemuDomainHostdevPrivatePtr hostdevPriv = QEMU_DOMAIN_HOSTDEV_PRIVATE(hostdev); char *drvstr;
+ if (qemuBuildDiskSecinfoCommandLine(cmd, hostdevPriv->secinfo) < 0) + return -1; + virCommandAddArg(cmd, "-drive"); - if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev))) + if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps))) return -1; virCommandAddArg(cmd, drvstr); VIR_FREE(drvstr);
diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a544cecb9..17228d1b4 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c
[...]
@@ -3879,8 +3909,22 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver, if (!(drivealias = qemuAliasFromHostdev(hostdev))) goto cleanup;
+ if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET)) { + if (!(objAlias = + qemuDomainGetSecretAESAlias(hostdev->info->alias, false))) { + return -1; + }
This will leak stuff since you skip cleanup. Also don't break that line above.
+ } + qemuDomainObjEnterMonitor(driver, vm); qemuMonitorDriveDel(priv->mon, drivealias); + + /* If it fails, then so be it - it was a best shot */ + if (objAlias) { + ignore_value(qemuMonitorDelObject(priv->mon, objAlias)); + qemuDomainObjDiskSecretObjectAliasEntryRemove(priv, objAlias); + } + if (qemuDomainObjExitMonitor(driver, vm) < 0) goto cleanup; }
participants (2)
-
John Ferlan
-
Peter Krempa