[libvirt] [PATCH 0/2] Disallow qemu-img creation of qcow[2] encryption type
Details in the patches. John Ferlan (2): storage: Separate out the qemu-img help output generation storage: Check qemu-img encryption type capability src/storage/storage_util.c | 63 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 15 deletions(-) -- 2.13.6
Separate out and return the output string for future comparison. Going to need to add new checks shortly. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_util.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/storage/storage_util.c b/src/storage/storage_util.c index 897dfdaaee..7df52239c2 100644 --- a/src/storage/storage_util.c +++ b/src/storage/storage_util.c @@ -799,11 +799,10 @@ enum { QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT, }; -static bool -virStorageBackendQemuImgSupportsCompat(const char *qemuimg) +static char * +virStorageBackendQemuImgCreateHelp(const char *qemuimg) { - bool ret = false; - char *output; + char *output = NULL; virCommandPtr cmd = NULL; cmd = virCommandNewArgList(qemuimg, "create", "-o", "?", "-f", "qcow2", @@ -812,34 +811,40 @@ virStorageBackendQemuImgSupportsCompat(const char *qemuimg) virCommandAddEnvString(cmd, "LC_ALL=C"); virCommandSetOutputBuffer(cmd, &output); - if (virCommandRun(cmd, NULL) < 0) - goto cleanup; - - if (strstr(output, "\ncompat ")) - ret = true; + ignore_value(virCommandRun(cmd, NULL)); - cleanup: virCommandFree(cmd); - VIR_FREE(output); - return ret; + return output; } +static bool +virStorageBackendQemuImgSupportsCompat(const char *output) +{ + return strstr(output, "\ncompat "); +} + static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) { + char *output = NULL; /* As of QEMU 0.11 the [-o options] support was added via qemu * commit id '9ea2ea71', so we start with that base and figure * out what else we have */ int ret = QEMU_IMG_BACKING_FORMAT_OPTIONS; + if (!(output = virStorageBackendQemuImgCreateHelp(qemuimg))) + goto cleanup; + /* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer * understands. Since we still support QEMU 0.12 and newer, we need * to be able to handle the previous format as can be set via a * compat=0.10 option. */ - if (virStorageBackendQemuImgSupportsCompat(qemuimg)) + if (virStorageBackendQemuImgSupportsCompat(output)) ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT; + cleanup: + VIR_FREE(output); return ret; } -- 2.13.6
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. 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@redhat.com> --- 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; + } 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
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.
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@redhat.com> --- 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"); +} + +
NACK adding more -help output scraping just for a nicer error message for a feature noone should be using in the first place is not worth it. Jano
On 04/17/2018 04:47 PM, Ján Tomko 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.
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@redhat.com> --- 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"); +} + +
NACK
adding more -help output scraping just for a nicer error message for a feature noone should be using in the first place is not worth it.
Jano
Fair enough - considering your other series... As a consumer would you expect an error message for any create using qcow[2] then? Or should we just rip out the qcow[2] encryption stuff too? IDC, either way. It's the delta then between qemu 1.5 and 2.9 to consider... John
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
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@redhat.com> --- 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.
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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
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@redhat.com> --- 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Regards, Daniel
On Wed, Apr 18, 2018 at 08:08:41AM -0400, John Ferlan wrote:
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?
Sure, the exact same syntax as with luks volumes - you just specify "qcow" instead of "luks" as the type.
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?
QEMU is likely to support the qcow2 enc format indefinitely, but only in the qemu-img tool for the sake of data liberation. I don't think libvirt should arbitrarily decide to drop it from our qemu-img usage.
+ 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.
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 04/18/2018 08:17 AM, Daniel P. Berrangé wrote:
On Wed, Apr 18, 2018 at 08:08:41AM -0400, John Ferlan wrote:
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?
Sure, the exact same syntax as with luks volumes - you just specify "qcow" instead of "luks" as the type.
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?
QEMU is likely to support the qcow2 enc format indefinitely, but only in the qemu-img tool for the sake of data liberation. I don't think libvirt should arbitrarily decide to drop it from our qemu-img usage.
So that means Jano's series to remove help scraping completely cannot be applied since this code would need to check that the option exists before using it; otherwise, anything inclusive of QEMU 1.5 and 2.9 would fail (the option was introduced in 2.10 - I mistyped above). What could be applied would be the removal of OPTIONS and OPTIONS_COMPAT, but this new one would need to exist since AFAIK there is no other way currently to query qemu-img for what it supports. Going to make for some ugly code... John
+ 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.
Regards, Daniel
On Wed, Apr 18, 2018 at 08:53:09AM -0400, John Ferlan wrote:
On 04/18/2018 08:17 AM, Daniel P. Berrangé wrote:
On Wed, Apr 18, 2018 at 08:08:41AM -0400, John Ferlan wrote:
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?
Sure, the exact same syntax as with luks volumes - you just specify "qcow" instead of "luks" as the type.
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?
QEMU is likely to support the qcow2 enc format indefinitely, but only in the qemu-img tool for the sake of data liberation. I don't think libvirt should arbitrarily decide to drop it from our qemu-img usage.
So that means Jano's series to remove help scraping completely cannot be applied since this code would need to check that the option exists before using it; otherwise, anything inclusive of QEMU 1.5 and 2.9 would fail (the option was introduced in 2.10 - I mistyped above).
Yes & no - some of the stuff Jano is killing can be assumed to be present in 1.5.0 or newer, so that is fine. QEMU has never provided any way to probe capabilities for qemu-img though. For new features that never existed we can blindly invoke qmeu-img and let it report errors itself. We only get a problem for features where the implementation changed. In such cases we have choice of only supporting 1 approach, or doing help scraping or doing version number scraping, all of which suck. In the particular case only, I suggest we say to hell with back compat and just pick the new "secrets" based approach which works with qcow2 built-in crap encryption for QEMU >= 2.6. The number of users who'll be hurt by this will be essentially nil. We might find we need to re-introduce help scraping in future for other reasons, but lets not worry about it now Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
[...]
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?
Sure, the exact same syntax as with luks volumes - you just specify "qcow" instead of "luks" as the type.
So I've been working on doing as suggested, there's slight differences for qcow: 1. Usage of "encrypt.key-secret" instead of just "key-secret" 2. Usage of "encrypt.format=aes" (or qcow works too) instead of "encryption=on" 3. Don't need change the "type" value like is done for "luks" in virStorageBackendCreateQemuImgCmdFromVol. The result is (testing mode only): qemu-img create -f qcow2 \ --object secret,id=x0,format=raw,data=letmein \ -o encrypt.format=aes,encrypt.key-secret=x0 \ x0.img 1048576K NB: Using "-b /dev/null" and ",backing_fmt=raw" works just fine as would usage of other -o options such as "compat=1.1,", "compat=0.10", "lazy_refcounts,", and "preallocation={off|metadata|falloc|full}". So far, so good. However, storagevolxml2argvtest.c also generates qemu-img convert options. And this is where things go down hill. The former commands would for example do: qemu-img convert -f raw -O qcow2 -o encryption=on \ /var/lib/libvirt/images/sparse.img /var/lib/libvirt/images/OtherDemo.img which in the new syntax would conceivably be: qemu-img convert -f raw -O qcow2 \ --object secret,id=OtherDemo.img_encrypt0,file=/path/to/secretFile \ -o encrypt.format=aes,encrypt.key-secret=OtherDemo.img_encrypt0 \ /var/lib/libvirt/images/sparse.img \ /var/lib/libvirt/images/OtherDemo.img But, testing that type of model in practice: qemu-img --version qemu-img version 2.11.93 (v2.12.0-rc4) rm x0.img dd if=/dev/zero of=x0.img bs=1M seek=10 count=0 ls -al x0.img -rw-r--r--. 1 root root 10485760 Apr 19 12:34 x0.img qemu-img convert -f raw -O qcow2 \ --object secret,id=x,format=raw,data=letmein \ -o encrypt.key-secret=x,encrypt.format=aes \ x0.img x1.img qemu-img: Could not open 'x1.img': Parameter 'encrypt.key-secret' is required for cipher NB: The x1.img file does exist: ls -al x1.img -rw-r--r--. 1 root root 196616 Apr 19 12:45 x1.img FWIW: A similar setup to convert to luks also fails: qemu-img convert -f raw -O luks \ --object secret,id=x,format=raw,data=letmein \ -o key-secret=x \ x0.img x1.img qemu-img: Could not open 'x1.img': Parameter 'key-secret' is required for cipher NB: This command takes longer to fail and like before the x1.img exists: ls -al x1.img -rw-r--r--. 1 root root 12554240 Apr 19 12:46 x1.img It seems perhaps qemu-img is not applying the secrets to the target file, rather it may be using the source file, but reading the qemu-img.c code and figuring out how argument processing works isn't clear to me. So - mainly checking - is this the 'expected' syntax? and secondarily, of course, is this a bug in qemu-img? Tks - John
On Thu, Apr 19, 2018 at 02:21:43PM -0400, John Ferlan wrote:
[...]
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?
Sure, the exact same syntax as with luks volumes - you just specify "qcow" instead of "luks" as the type.
So I've been working on doing as suggested, there's slight differences for qcow:
Sorry, when it say it is the same as luks volumes, i mean qcow encryption in qcow2, is the same syntax as luks encryption in qcow2. I didn't mean to refer to luks on raw, which does have different syntax.
1. Usage of "encrypt.key-secret" instead of just "key-secret" 2. Usage of "encrypt.format=aes" (or qcow works too) instead of "encryption=on" 3. Don't need change the "type" value like is done for "luks" in virStorageBackendCreateQemuImgCmdFromVol.
The result is (testing mode only):
qemu-img create -f qcow2 \ --object secret,id=x0,format=raw,data=letmein \ -o encrypt.format=aes,encrypt.key-secret=x0 \ x0.img 1048576K
Yes, that is right.
NB: Using "-b /dev/null" and ",backing_fmt=raw" works just fine as would usage of other -o options such as "compat=1.1,", "compat=0.10", "lazy_refcounts,", and "preallocation={off|metadata|falloc|full}".
However, storagevolxml2argvtest.c also generates qemu-img convert options. And this is where things go down hill. The former commands would for example do:
[snip] Ok, yes, the convert command gets difficult due to qemu code limitations. Essentially the problem is that 'convert' has to do two jobs with the target file, create it and then open it to write data. Unfortunately you need two incompatible syntaxes for these jobs :-( When creating the file, the filename must be a plain name, and any options given via '-o', but to open the file, the filename must be in the dotted syntax with options inline. There is no way to convert between these two syntaxes in QEMU. Essentially you can't run the convert command as you are describing. All is not lost though, you simply have to turn it into a two stage process. First, use 'qemu-img create' to create the target image with the right size, as we already have code todo. Then you pass the '-n' flag to 'convert' which tells it that the image has been pre-created, so it can skip the create step. Now it only has one job todo, which is to open the target image, so you can use the dotted filename syntax. Note, you must pass the '--image-opts' flag to tell it that the source filename is using dotted syntax, and also pass '--target-image-opts' to tell it the target filanem is using dotted syntax. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (3)
-
Daniel P. Berrangé -
John Ferlan -
Ján Tomko