[libvirt] [PATCH v2] storage: Add capability to use LUKS encryption for disk backend

[John Ferlan]

On 05/29/2018 09:34 AM, Peter Krempa wrote:
> On Tue, May 29, 2018 at 08:53:06 -0400, John Ferlan wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1560946
>>
>> Similar to the the Logical backend, use qemu-img on the created
>> disk partition device to set up for LUKS encryption. Secret mgmt
>> for the device can be complicated by a reboot possibly changing
>> the path to the device if the infrastructure changes.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>  Changes in v2: Don't alter the result 'endOffset' of the
>>  DiskPartBoundaries, rather modify the input capacity value.
>>  Alteration of the value for the error path won't matter
>>  since the volume would be deleted anyway. The call to
>>  virStorageBackendDiskPartBoundaries was also slightly
>>  modified to check the return value vs -1 rather than != 0
>>  so that the call is similar to other calls. Separating
>>  that into it's own patch could have been done, but just
>>  felt like too much "busy work" to be worth the trouble.
>>
>>  src/storage/storage_backend_disk.c | 49 ++++++++++++++++++++++----------------
>>  1 file changed, 29 insertions(+), 20 deletions(-)
>>
>> diff --git a/src/storage/storage_backend_disk.c b/src/storage/storage_backend_disk.c
>> index 2e3d1e04a4..e9578d01d6 100644
>> --- a/src/storage/storage_backend_disk.c
>> +++ b/src/storage/storage_backend_disk.c
>> @@ -879,28 +879,26 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
>>      char *partFormat = NULL;
>>      unsigned long long startOffset = 0, endOffset = 0;
>>      virStoragePoolDefPtr def = virStoragePoolObjGetDef(pool);
>> +    virErrorPtr save_err;
>>      virCommandPtr cmd = virCommandNewArgList(PARTED,
>>                                               def->source.devices[0].path,
>>                                               "mkpart",
>>                                               "--script",
>>                                               NULL);
>>  
>> -    if (vol->target.encryption != NULL) {
>> -        virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> -                       "%s", _("storage pool does not support encrypted "
>> -                               "volumes"));
>> -        goto cleanup;
>> -    }
>> -
>>      if (virStorageBackendDiskPartFormat(pool, vol, &partFormat) != 0)
>>          goto cleanup;
>>      virCommandAddArg(cmd, partFormat);
>>  
>> -    if (virStorageBackendDiskPartBoundaries(pool, &startOffset,
>> -                                            &endOffset,
>> -                                            vol->target.capacity) != 0) {
>> +    /* If we're going to encrypt using LUKS, then we could need up to
>> +     * an extra 2MB for the LUKS header - so account for that now */
>> +    if (vol->target.encryption &&
>> +        vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS)
>> +        vol->target.capacity += 2 * 1024 * 1024;
> 
> So this is done only for LUKS encryption ...
> 
>> +
>> +    if (virStorageBackendDiskPartBoundaries(pool, &startOffset, &endOffset,
>> +                                            vol->target.capacity) < 0)
>>          goto cleanup;
>> -    }
>>  
>>      virCommandAddArgFormat(cmd, "%lluB", startOffset);
>>      virCommandAddArgFormat(cmd, "%lluB", endOffset);
>> @@ -919,15 +917,15 @@ virStorageBackendDiskCreateVol(virStoragePoolObjPtr pool,
>>      VIR_FREE(vol->target.path);
>>  
>>      /* Fetch actual extent info, generate key */
>> -    if (virStorageBackendDiskReadPartitions(pool, vol) < 0) {
>> -        /* Best effort to remove the partition. Ignore any errors
>> -         * since we could be calling this with vol->target.path == NULL
>> -         */
>> -        virErrorPtr save_err = virSaveLastError();
>> -        ignore_value(virStorageBackendDiskDeleteVol(pool, vol, 0));
>> -        virSetError(save_err);
>> -        virFreeError(save_err);
>> -        goto cleanup;
>> +    if (virStorageBackendDiskReadPartitions(pool, vol) < 0)
>> +        goto error;
>> +
>> +    if (vol->target.encryption) {
>> +        /* Adjust the sizes to account for the LUKS header */
>> +        vol->target.capacity -= 2 * 1024 * 1024;
>> +        vol->target.allocation -= 2 * 1024 * 1024;
> 
> but this is done for any encryption type, so if VIR_STORAGE_ENCRYPTION_FORMAT_QCOW
> would be used it would rob off two megabytes of the volume size.
> 
> Honestly I think that VIR_STORAGE_ENCRYPTION_FORMAT_QCOW should be
> disallowed for creation code right away, so that nobody creates the
> broken format any more.
> 
> That way we don't have to deal with possibility that somebody with a old
> qemu-img would run into problems here without adding much spurious code.
> 
>> +        if (virStorageBackendCreateVolUsingQemuImg(pool, vol, NULL, 0) < 0)
>> +            goto error;
> 
> ACK if you reject VIR_STORAGE_ENCRYPTION_FORMAT_QCOW right at the
> beginning.
> 

OK... I'll add this to avoid QCOW and/or DEFAULT

    if (vol->target.encryption &&
        vol->target.encryption->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
                       _("storage pool only supports LUKS encrypted volumes"));

Tks,

John

probably need a similar adjustment for the _logical backend too....