[PATCH 0/2] conf: Properly handle placement of <auth> and <encryption> after blockjobs
Peter Krempa (2): conf: Sanitize handling of <auth> and <encryption> placement for disks conf: Store 'diskElementAuth' and 'diskElementEnc' properties in status XML src/conf/backup_conf.c | 3 +- src/conf/domain_conf.c | 117 +++++++++++----------- src/conf/domain_conf.h | 6 +- src/conf/snapshot_conf.c | 4 +- src/qemu/qemu_domain.c | 6 +- src/util/virstoragefile.c | 1 - src/util/virstoragefile.h | 2 - tests/qemublocktest.c | 4 +- tests/qemustatusxml2xmldata/modern-in.xml | 4 + tests/virstoragetest.c | 3 +- 10 files changed, 81 insertions(+), 69 deletions(-) -- 2.26.2
Modern way to store <auth> and <encryption> of a <disk> is under <source>. This was added to mirror how <backingStore> handles these and in fact they are relevant to the source rather than to any other part of the disk. Historically we allowed them to be directly under <disk> and we need to keep compatibility. This wasn't a problem until introduction of -blockdev in qemu using of <auth> or <encryption> plainly wouldn't work with backing chains. Now that it works in backing chains and can be moved back and forth using snapshots/block-commit we need to ensure that the original placement is properly kept even if the source changes. To achieve the above semantics we need to store the preferred placement with the disk definition rather than the storage source definitions and also ensure that the modern way is chosen when the VM started with <source/encryption> only in the backing store. https://bugzilla.redhat.com/show_bug.cgi?id=1822878 Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/backup_conf.c | 3 +- src/conf/domain_conf.c | 97 ++++++++++++++++----------------------- src/conf/domain_conf.h | 6 ++- src/conf/snapshot_conf.c | 4 +- src/qemu/qemu_domain.c | 6 ++- src/util/virstoragefile.c | 1 - src/util/virstoragefile.h | 2 - tests/qemublocktest.c | 4 +- tests/virstoragetest.c | 3 +- 9 files changed, 57 insertions(+), 69 deletions(-) diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index c5677cdb05..c0c6f25d10 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -349,7 +349,8 @@ virDomainBackupDiskDefFormat(virBufferPtr buf, virStorageFileFormatTypeToString(disk->store->format)); if (virDomainDiskSourceFormat(&childBuf, disk->store, sourcename, - 0, false, storageSourceFormatFlags, true, NULL) < 0) + 0, false, storageSourceFormatFlags, + false, false, NULL) < 0) return -1; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 83748354b0..84eadb5659 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10469,31 +10469,6 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, if (virDomainStorageSourceParse(cur, ctxt, def->src, flags, xmlopt) < 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 (authdef && def->src->auth) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("an <auth> definition already found for " - "the <disk> definition")); - goto error; - } - - if (def->src->auth) - def->src->authInherited = true; - - /* Similarly for <encryption> - it's a child of <source> too - * and we cannot find in both places */ - if (encryption && def->src->encryption) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("an <encryption> definition already found for " - "the <disk> definition")); - goto error; - } - - if (def->src->encryption) - def->src->encryptionInherited = true; - source = true; startupPolicy = virXMLPropString(cur, "startupPolicy"); @@ -10558,17 +10533,9 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, goto error; } else if (!authdef && virXMLNodeNameEqual(cur, "auth")) { - /* If we've already parsed <source> and found an <auth> child, - * then generate an error to avoid ambiguity */ - if (def->src->authInherited) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("an <auth> definition already found for " - "disk source")); - goto error; - } - if (!(authdef = virStorageAuthDefParse(cur, ctxt))) goto error; + def->diskElementAuth = true; } else if (virXMLNodeNameEqual(cur, "iotune")) { if (virDomainDiskDefIotuneParse(def, ctxt) < 0) goto error; @@ -10580,17 +10547,11 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, def->transient = true; } else if (!encryption && virXMLNodeNameEqual(cur, "encryption")) { - /* If we've already parsed <source> and found an <encryption> child, - * then generate an error to avoid ambiguity */ - if (def->src->encryptionInherited) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("an <encryption> definition already found for " - "disk source")); - goto error; - } - if (!(encryption = virStorageEncryptionParseNode(cur, ctxt))) goto error; + + def->diskElementEnc = true; + } else if (!serial && virXMLNodeNameEqual(cur, "serial")) { serial = (char *)xmlNodeGetContent(cur); @@ -10783,10 +10744,31 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } def->dst = g_steal_pointer(&target); - if (authdef) + if (authdef) { + /* If we've already parsed <source> and found an <auth> child, + * then generate an error to avoid ambiguity */ + if (def->src->auth) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("an <auth> definition already found for " + "disk source")); + goto error; + } + def->src->auth = g_steal_pointer(&authdef); - if (encryption) + } + + if (encryption) { + /* If we've already parsed <source> and found an <encryption> child, + * then generate an error to avoid ambiguity */ + if (def->src->encryption) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("an <encryption> definition already found for " + "disk source")); + goto error; + } + def->src->encryption = g_steal_pointer(&encryption); + } def->domain_name = g_steal_pointer(&domain_name); def->serial = g_steal_pointer(&serial); def->wwn = g_steal_pointer(&wwn); @@ -25044,7 +25026,8 @@ virDomainDiskSourceFormatSlices(virBufferPtr buf, * @policy: startup policy attribute value, if necessary * @attrIndex: the 'index' attribute of <source> is formatted if true * @flags: XML formatter flags - * @formatsecrets: Force formatting of <auth> and <encryption> under <source> + * @skipAuth: Skip formatting of <auth> + * @skipEnc: Skip formatting of <encryption> * regardless of the original definition state * @xmlopt: XML formatter callbacks * @@ -25058,7 +25041,8 @@ virDomainDiskSourceFormat(virBufferPtr buf, int policy, bool attrIndex, unsigned int flags, - bool formatsecrets, + bool skipAuth, + bool skipEnc, virDomainXMLOptionPtr xmlopt) { g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; @@ -25117,13 +25101,10 @@ virDomainDiskSourceFormat(virBufferPtr buf, * <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->authInherited || formatsecrets) && - src->type != VIR_STORAGE_TYPE_VOLUME) + if (src->auth && !skipAuth && src->type != VIR_STORAGE_TYPE_VOLUME) virStorageAuthDefFormat(&childBuf, src->auth); - /* If we found encryption as a child of <source>, then format it - * as we found it. */ - if (src->encryption && (src->encryptionInherited || formatsecrets) && + if (src->encryption && !skipEnc && virStorageEncryptionFormat(&childBuf, src->encryption) < 0) return -1; @@ -25184,7 +25165,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virBufferAsprintf(&childBuf, "<format type='%s'/>\n", virStorageFileFormatTypeToString(backingStore->format)); if (virDomainDiskSourceFormat(&childBuf, backingStore, "source", 0, false, - flags, true, xmlopt) < 0) + flags, false, false, xmlopt) < 0) return -1; if (virDomainDiskBackingStoreFormat(&childBuf, backingStore, xmlopt, flags) < 0) @@ -25346,7 +25327,7 @@ virDomainDiskDefFormatMirror(virBufferPtr buf, virBufferEscapeString(&childBuf, "<format type='%s'/>\n", formatStr); if (virDomainDiskSourceFormat(&childBuf, disk->mirror, "source", 0, true, - flags, true, xmlopt) < 0) + flags, false, false, xmlopt) < 0) return -1; if (virDomainDiskBackingStoreFormat(&childBuf, disk->mirror, xmlopt, flags) < 0) @@ -25441,11 +25422,13 @@ virDomainDiskDefFormat(virBufferPtr buf, /* Format as child of <disk> if defined there; otherwise, * if defined as child of <source>, then format later */ - if (def->src->auth && !def->src->authInherited) + if (def->src->auth && def->diskElementAuth) virStorageAuthDefFormat(buf, def->src->auth); if (virDomainDiskSourceFormat(buf, def->src, "source", def->startupPolicy, - true, flags, false, xmlopt) < 0) + true, flags, + def->diskElementAuth, def->diskElementEnc, + xmlopt) < 0) return -1; /* Don't format backingStore to inactive XMLs until the code for @@ -25491,7 +25474,7 @@ virDomainDiskDefFormat(virBufferPtr buf, /* If originally found as a child of <disk>, then format thusly; * otherwise, will be formatted as child of <source> */ - if (def->src->encryption && !def->src->encryptionInherited && + if (def->src->encryption && def->diskElementEnc && virStorageEncryptionFormat(buf, def->src->encryption) < 0) return -1; if (virDomainDeviceInfoFormat(buf, &def->info, diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 4afd8f04bc..ddc75d8de2 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -579,6 +579,9 @@ struct _virDomainDiskDef { unsigned int queues; int model; /* enum virDomainDiskModel */ virDomainVirtioOptionsPtr virtio; + + bool diskElementAuth; + bool diskElementEnc; }; @@ -3233,7 +3236,8 @@ int virDomainDiskSourceFormat(virBufferPtr buf, int policy, bool attrIndex, unsigned int flags, - bool formatsecrets, + bool skipAuth, + bool skipEnc, virDomainXMLOptionPtr xmlopt); int diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 37b5c2fdf7..98c296b276 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -818,8 +818,8 @@ virDomainSnapshotDiskDefFormat(virBufferPtr buf, if (disk->src->format > 0) virBufferEscapeString(buf, "<driver type='%s'/>\n", virStorageFileFormatTypeToString(disk->src->format)); - if (virDomainDiskSourceFormat(buf, disk->src, "source", 0, false, 0, true, - xmlopt) < 0) + if (virDomainDiskSourceFormat(buf, disk->src, "source", 0, false, 0, + false, false, xmlopt) < 0) return -1; virBufferAdjustIndent(buf, -2); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9c629c31a3..a22cb4ed30 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2603,7 +2603,8 @@ qemuDomainObjPrivateXMLFormatBlockjobFormatSource(virBufferPtr buf, virStorageTypeToString(src->type), virStorageFileFormatTypeToString(src->format)); - if (virDomainDiskSourceFormat(&childBuf, src, "source", 0, true, xmlflags, true, xmlopt) < 0) + if (virDomainDiskSourceFormat(&childBuf, src, "source", 0, true, xmlflags, + false, false, xmlopt) < 0) return -1; if (chain && @@ -2823,7 +2824,8 @@ qemuDomainObjPrivateXMLFormatNBDMigrationSource(virBufferPtr buf, virStorageFileFormatTypeToString(src->format)); if (virDomainDiskSourceFormat(&childBuf, src, "source", 0, false, - VIR_DOMAIN_DEF_FORMAT_STATUS, true, xmlopt) < 0) + VIR_DOMAIN_DEF_FORMAT_STATUS, + false, false, xmlopt) < 0) return -1; virXMLFormatElement(buf, "migrationSource", &attrBuf, &childBuf); diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index ff9a6c6663..535a94e239 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -2999,7 +2999,6 @@ virStorageSourceParseRBDColonString(const char *rbdstr, authdef->secrettype = g_strdup(virSecretUsageTypeToString(VIR_SECRET_USAGE_TYPE_CEPH)); src->auth = g_steal_pointer(&authdef); - src->authInherited = true; /* Cannot formulate a secretType (eg, usage or uuid) given * what is provided. diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index d56d5c45e3..c68bdc9680 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -291,9 +291,7 @@ struct _virStorageSource { virStorageNetCookieDefPtr *cookies; virStorageSourcePoolDefPtr srcpool; virStorageAuthDefPtr auth; - bool authInherited; virStorageEncryptionPtr encryption; - bool encryptionInherited; virStoragePRDefPtr pr; virTristateBool sslverify; /* both values below have 0 as default value */ diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 8001807552..9ac506c78b 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -111,8 +111,8 @@ testBackingXMLjsonXML(const void *args) return -1; } - if (virDomainDiskSourceFormat(&buf, jsonsrc, "source", 0, false, 0, true, - NULL) < 0 || + if (virDomainDiskSourceFormat(&buf, jsonsrc, "source", 0, false, 0, + false, false, NULL) < 0 || !(actualxml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); return -1; diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index ac1480de4e..98f47f0e41 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -614,7 +614,8 @@ testBackingParse(const void *args) return -1; } - if (virDomainDiskSourceFormat(&buf, src, "source", 0, false, xmlformatflags, true, NULL) < 0 || + if (virDomainDiskSourceFormat(&buf, src, "source", 0, false, xmlformatflags, + false, false, NULL) < 0 || !(xml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); return -1; -- 2.26.2
Remember the preferred placement of <auth> and <encryption> for a disk source across libvirtd restarts. Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 20 ++++++++++++++++++++ tests/qemustatusxml2xmldata/modern-in.xml | 4 ++++ 2 files changed, 24 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 84eadb5659..c201fc901d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -10593,6 +10593,13 @@ virDomainDiskDefParseXML(virDomainXMLOptionPtr xmlopt, } } else if (virXMLNodeNameEqual(cur, "boot")) { /* boot is parsed as part of virDomainDeviceInfoParseXML */ + } else if ((flags & VIR_DOMAIN_DEF_PARSE_STATUS) && + virXMLNodeNameEqual(cur, "diskSecretsPlacement")) { + g_autofree char *secretAuth = virXMLPropString(cur, "auth"); + g_autofree char *secretEnc = virXMLPropString(cur, "enc"); + + def->diskElementAuth = !!secretAuth; + def->diskElementEnc = !!secretEnc; } } @@ -25485,6 +25492,19 @@ virDomainDiskDefFormat(virBufferPtr buf, if (virDomainDiskDefFormatPrivateData(buf, def, flags, xmlopt) < 0) return -1; + /* format diskElementAuth and diskElementEnc into status XML to preserve + * formatting */ + if (flags & VIR_DOMAIN_DEF_FORMAT_STATUS) { + g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; + + if (def->diskElementAuth) + virBufferAddLit(&attrBuf, " auth='true'"); + if (def->diskElementEnc) + virBufferAddLit(&attrBuf, " enc='true'"); + + virXMLFormatElement(buf, "diskSecretsPlacement", &attrBuf, NULL); + } + virBufferAdjustIndent(buf, -2); virBufferAddLit(buf, "</disk>\n"); return 0; diff --git a/tests/qemustatusxml2xmldata/modern-in.xml b/tests/qemustatusxml2xmldata/modern-in.xml index cb56cdcef9..64d42200e4 100644 --- a/tests/qemustatusxml2xmldata/modern-in.xml +++ b/tests/qemustatusxml2xmldata/modern-in.xml @@ -308,6 +308,9 @@ <emulator>/usr/bin/qemu-system-x86_64</emulator> <disk type='file' device='disk'> <driver name='qemu' type='qcow2' discard='unmap' detect_zeroes='on'/> + <auth username='testuser'> + <secret type='iscsi' usage='libvirtiscsi'/> + </auth> <source file='/var/lib/libvirt/images/a.qcow2'/> <backingStore type='file' index='1'> <format type='qcow2'/> @@ -348,6 +351,7 @@ <nodename type='copyOnRead' name='cor-node'/> </nodenames> </privateData> + <diskSecretsPlacement auth='true'/> </disk> <disk type='file' device='cdrom'> <driver name='qemu' type='raw'/> -- 2.26.2
On 5/7/20 3:26 PM, Peter Krempa wrote:
Peter Krempa (2): conf: Sanitize handling of <auth> and <encryption> placement for disks conf: Store 'diskElementAuth' and 'diskElementEnc' properties in status XML
src/conf/backup_conf.c | 3 +- src/conf/domain_conf.c | 117 +++++++++++----------- src/conf/domain_conf.h | 6 +- src/conf/snapshot_conf.c | 4 +- src/qemu/qemu_domain.c | 6 +- src/util/virstoragefile.c | 1 - src/util/virstoragefile.h | 2 - tests/qemublocktest.c | 4 +- tests/qemustatusxml2xmldata/modern-in.xml | 4 + tests/virstoragetest.c | 3 +- 10 files changed, 81 insertions(+), 69 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Privoznik -
Peter Krempa