Re: [PATCH 2/2] storage: Check qemu-img encryption type capability

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux