[libvirt] [PATCH 4/4] conf: Always format storage source auth and encryption under <source> for backing files

Historically there are two places where we format authentication and encryption for a disk. The logich which formats it for backing files was flawed though and didn't format it at all. This worked if the image became a backing file through the means of a snapshot but not directly. Force formatting of the source and encryption for any non-disk case to fix the issue. This caused problems in many places as we use the formatter to copy the definition. Effectively any copy lost the secret definition. https://bugzilla.redhat.com/show_bug.cgi?id=1789310 https://bugzilla.redhat.com/show_bug.cgi?id=1788898 Signed-off-by: Peter Krempa <pkrempa(a)redhat.com&gt; --- src/conf/backup_conf.c | 2 +- src/conf/domain_conf.c | 13 ++++++++----- src/conf/domain_conf.h | 1 + src/conf/snapshot_conf.c | 2 +- src/qemu/qemu_domain.c | 4 ++-- tests/qemublocktest.c | 2 +- .../luks-disks-source-qcow2.x86_64-latest.xml | 6 +++++- tests/virstoragetest.c | 2 +- 8 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/conf/backup_conf.c b/src/conf/backup_conf.c index aa11967d2a..61dc8cd4b2 100644 --- a/src/conf/backup_conf.c +++ b/src/conf/backup_conf.c @@ -338,7 +338,7 @@ virDomainBackupDiskDefFormat(virBufferPtr buf, virStorageFileFormatTypeToString(disk->store->format)); if (virDomainDiskSourceFormat(&childBuf, disk->store, sourcename, - 0, false, storageSourceFormatFlags, NULL) < 0) + 0, false, storageSourceFormatFlags, true, NULL) < 0) return -1; } diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 1290241923..ca5bbc3f35 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -24189,6 +24189,8 @@ virDomainDiskSourceFormatPrivateData(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> even if they + * were inherited * @xmlopt: XML formatter callbacks * * Formats @src into a <source> element. Note that this doesn't format the @@ -24201,6 +24203,7 @@ virDomainDiskSourceFormat(virBufferPtr buf, int policy, bool attrIndex, unsigned int flags, + bool formatsecrets, virDomainXMLOptionPtr xmlopt) { g_auto(virBuffer) attrBuf = VIR_BUFFER_INITIALIZER; @@ -24257,13 +24260,13 @@ 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 && + if (src->auth && (src->authInherited || formatsecrets) && 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 && + if (src->encryption && (src->encryptionInherited || formatsecrets) && virStorageEncryptionFormat(&childBuf, src->encryption) < 0) return -1; @@ -24324,7 +24327,7 @@ virDomainDiskBackingStoreFormat(virBufferPtr buf, virBufferAsprintf(&childBuf, "<format type='%s'/>\n", virStorageFileFormatTypeToString(backingStore->format)); if (virDomainDiskSourceFormat(&childBuf, backingStore, "source", 0, false, - flags, xmlopt) < 0) + flags, true, xmlopt) < 0) return -1; if (virDomainDiskBackingStoreFormat(&childBuf, backingStore, xmlopt, flags) < 0) @@ -24486,7 +24489,7 @@ virDomainDiskDefFormatMirror(virBufferPtr buf, virBufferEscapeString(&childBuf, "<format type='%s'/>\n", formatStr); if (virDomainDiskSourceFormat(&childBuf, disk->mirror, "source", 0, true, - flags, xmlopt) < 0) + flags, true, xmlopt) < 0) return -1; if (virDomainDiskBackingStoreFormat(&childBuf, disk->mirror, xmlopt, flags) < 0) @@ -24585,7 +24588,7 @@ virDomainDiskDefFormat(virBufferPtr buf, virStorageAuthDefFormat(buf, def->src->auth); if (virDomainDiskSourceFormat(buf, def->src, "source", def->startupPolicy, - true, flags, xmlopt) < 0) + true, flags, false, xmlopt) < 0) return -1; /* Don't format backingStore to inactive XMLs until the code for diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 76d6ef8e48..6ae89fa498 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -3118,6 +3118,7 @@ int virDomainDiskSourceFormat(virBufferPtr buf, int policy, bool attrIndex, unsigned int flags, + bool formatsecrets, virDomainXMLOptionPtr xmlopt); int diff --git a/src/conf/snapshot_conf.c b/src/conf/snapshot_conf.c index 2bd4d6a276..37b5c2fdf7 100644 --- a/src/conf/snapshot_conf.c +++ b/src/conf/snapshot_conf.c @@ -818,7 +818,7 @@ 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, + if (virDomainDiskSourceFormat(buf, disk->src, "source", 0, false, 0, true, xmlopt) < 0) return -1; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 1f358544ab..a6dde15bad 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -2547,7 +2547,7 @@ qemuDomainObjPrivateXMLFormatBlockjobFormatSource(virBufferPtr buf, virStorageTypeToString(src->type), virStorageFileFormatTypeToString(src->format)); - if (virDomainDiskSourceFormat(&childBuf, src, "source", 0, true, xmlflags, xmlopt) < 0) + if (virDomainDiskSourceFormat(&childBuf, src, "source", 0, true, xmlflags, true, xmlopt) < 0) return -1; if (chain && @@ -2746,7 +2746,7 @@ qemuDomainObjPrivateXMLFormatNBDMigrationSource(virBufferPtr buf, virStorageFileFormatTypeToString(src->format)); if (virDomainDiskSourceFormat(&childBuf, src, "source", 0, false, - VIR_DOMAIN_DEF_FORMAT_STATUS, xmlopt) < 0) + VIR_DOMAIN_DEF_FORMAT_STATUS, true, xmlopt) < 0) return -1; virXMLFormatElement(buf, "migrationSource", &attrBuf, &childBuf); diff --git a/tests/qemublocktest.c b/tests/qemublocktest.c index 3e9edb85f0..7ff6a6b17b 100644 --- a/tests/qemublocktest.c +++ b/tests/qemublocktest.c @@ -90,7 +90,7 @@ testBackingXMLjsonXML(const void *args) return -1; } - if (virDomainDiskSourceFormat(&buf, jsonsrc, "source", 0, false, 0, + if (virDomainDiskSourceFormat(&buf, jsonsrc, "source", 0, false, 0, true, NULL) < 0 || !(actualxml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); diff --git a/tests/qemuxml2xmloutdata/luks-disks-source-qcow2.x86_64-latest.xml b/tests/qemuxml2xmloutdata/luks-disks-source-qcow2.x86_64-latest.xml index 33e6d02976..aa13fe5dfa 100644 --- a/tests/qemuxml2xmloutdata/luks-disks-source-qcow2.x86_64-latest.xml +++ b/tests/qemuxml2xmloutdata/luks-disks-source-qcow2.x86_64-latest.xml @@ -83,7 +83,11 @@ </source> <backingStore type='file'> <format type='qcow2'/> - <source file='/storage/guest_disks/base.qcow2'/> + <source file='/storage/guest_disks/base.qcow2'> + <encryption format='luks'> + <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccc2f80d6f'/> + </encryption> + </source> <backingStore/> </backingStore> <target dev='vdf' bus='virtio'/> diff --git a/tests/virstoragetest.c b/tests/virstoragetest.c index 0e274ad1b7..2862758752 100644 --- a/tests/virstoragetest.c +++ b/tests/virstoragetest.c @@ -612,7 +612,7 @@ testBackingParse(const void *args) return -1; } - if (virDomainDiskSourceFormat(&buf, src, "source", 0, false, 0, NULL) < 0 || + if (virDomainDiskSourceFormat(&buf, src, "source", 0, false, 0, true, NULL) < 0 || !(xml = virBufferContentAndReset(&buf))) { fprintf(stderr, "failed to format disk source xml\n"); return -1; -- 2.24.1