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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

[...]

>> 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



--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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 :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list