On 04/18/2018 04:29 AM, Daniel P. Berrangé wrote: > On Tue, Apr 17, 2018 at 03:23:33PM -0400, John Ferlan wrote: >> https://bugzilla.redhat.com/show_bug.cgi?id=1526382 >> >> As of QEMU 2.9, qemu-img has enforced using the "key-secret" for >> creation of encrypted volumes. That is, LUKS encryption is now >> required and the old (awful) qcow[2] encryption methodolgy is >> no longer supported. > > Not quite right actually. The 'key-secret' approach can be used to > create both LUKS and the old qcow[2] encryption. > > We only forbid qcow[2] encryption with the system emulators, still > have full support in qemu-img for sake of interoperability. The only > break there was the command line syntax > Oh, OK - well I didn't find that to be obvious... So there is a way using secret objects to create a qcow[2] encrypted volume? Still Jano has NACK'd using help scraping (and posted a separate series removing it completely). So then the question becomes does this change "convert" into a disallow this type of creation going forward? Do we just cause failure in storageBackendCreateQemuImgCheckEncryption when not using LUKS or do we let the qemu-img just be the bad guy and do nothing in our code? >> >> In order to check for this, we scan the qemu-img -o help options >> looking for "key-secret" and if set, we enforce during the create >> volume processing that the about to be encrypted volume doesn't >> attempt to use the old crufty encryption mechanism. >> >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> >> --- >> src/storage/storage_util.c | 32 ++++++++++++++++++++++++++++++-- >> 1 file changed, 30 insertions(+), 2 deletions(-) >> >> diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c >> index 7df52239c2..d2e02a57ca 100644 >> --- a/src/storage/storage_util.c >> +++ b/src/storage/storage_util.c >> @@ -797,6 +797,7 @@ storagePloopResize(virStorageVolDefPtr vol, >> enum { >> QEMU_IMG_BACKING_FORMAT_OPTIONS = 0, >> QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT, >> + QEMU_IMG_BACKING_FORMAT_OPTIONS_KEY_SECRET, >> }; >> >> static char * >> @@ -824,6 +825,14 @@ virStorageBackendQemuImgSupportsCompat(const char *output) >> return strstr(output, "\ncompat "); >> } >> >> + >> +static bool >> +virStorageBackendQemuImgRequiresKeySecret(const char *output) >> +{ >> + return strstr(output, "key-secret"); >> +} >> + >> + >> static int >> virStorageBackendQEMUImgBackingFormat(const char *qemuimg) >> { >> @@ -843,6 +852,11 @@ virStorageBackendQEMUImgBackingFormat(const char *qemuimg) >> if (virStorageBackendQemuImgSupportsCompat(output)) >> ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT; >> >> + /* QEMU 2.9 enforced that qemu-img creation of an encrypted volume >> + * uses LUKS encryption. */ >> + if (virStorageBackendQemuImgRequiresKeySecret(output)) >> + ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_KEY_SECRET; >> + >> cleanup: >> VIR_FREE(output); >> return ret; >> @@ -934,6 +948,7 @@ storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr enc, >> >> /* storageBackendCreateQemuImgCheckEncryption: >> * @format: format of file found >> + * @imgformat: image format capability >> * @conn: pointer to connection >> * @vol: pointer to volume def >> * >> @@ -943,6 +958,7 @@ storageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr enc, >> */ >> static int >> storageBackendCreateQemuImgCheckEncryption(int format, >> + int imgformat, >> const char *type, >> virStorageVolDefPtr vol) >> { >> @@ -956,6 +972,12 @@ storageBackendCreateQemuImgCheckEncryption(int format, >> vol->target.encryption->format); >> return -1; >> } >> + if (imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS_KEY_SECRET) { >> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", >> + _("qemu-img no longer supports qcow encryption, " >> + "use LUKS encryption instead")); >> + return -1; >> + } > > Why is imgformat being compared against QEMU_IMG_BACKING_FORMAT_OPTIONS_KEY_SECRET ? > > Aren't those two sides of the expression from completely different > enum types. > Although perhaps not well named, @imgformat is fetched via virStorageBackendQEMUImgBackingFormat which returns QEMU_IMG_BACKING_FORMAT_OPTIONS* type enum's. John >> if (enc->nsecrets > 1) { >> virReportError(VIR_ERR_XML_ERROR, "%s", >> _("too many secrets for qcow encryption")); >> @@ -1264,8 +1286,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virStoragePoolObjPtr pool, >> return NULL; >> >> if (info.encryption && >> - storageBackendCreateQemuImgCheckEncryption(info.format, type, >> - vol) < 0) >> + storageBackendCreateQemuImgCheckEncryption(info.format, imgformat, >> + type, vol) < 0) >> return NULL; >> >> >> @@ -2359,6 +2381,7 @@ storageBackendResizeQemuImg(virStoragePoolObjPtr pool, >> { >> int ret = -1; >> char *img_tool = NULL; >> + int imgformat; >> virCommandPtr cmd = NULL; >> const char *type; >> char *secretPath = NULL; >> @@ -2371,6 +2394,10 @@ storageBackendResizeQemuImg(virStoragePoolObjPtr pool, >> return -1; >> } >> >> + imgformat = virStorageBackendQEMUImgBackingFormat("qemu-img"); >> + if (imgformat < 0) >> + goto cleanup; >> + >> if (vol->target.encryption) { >> if (vol->target.format == VIR_STORAGE_FILE_RAW) >> type = "luks"; >> @@ -2380,6 +2407,7 @@ storageBackendResizeQemuImg(virStoragePoolObjPtr pool, >> storageBackendLoadDefaultSecrets(vol); >> >> if (storageBackendCreateQemuImgCheckEncryption(vol->target.format, >> + imgformat, >> type, vol) < 0) >> goto cleanup; >> >> -- >> 2.13.6 >> >> -- >> libvir-list mailing list >> libvir-list@xxxxxxxxxx >> https://www.redhat.com/mailman/listinfo/libvir-list > > Regards, > Daniel > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
