[libvirt] [PATCH 0/2] Updates for LUKS support

Daniel P. Berrange (2): virstoragefile: refactor virStorageFileMatchesNNN methods storage: remove "luks" storage volume type src/qemu/qemu_command.c | 10 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- src/storage/storage_backend.c | 41 ++-- src/storage/storage_backend_fs.c | 17 +- src/storage/storage_backend_gluster.c | 5 - src/util/virstoragefile.c | 265 +++++++++++++++------ src/util/virstoragefile.h | 1 - tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 4 +- tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlin/vol-luks.xml | 2 +- tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlout/vol-luks.xml | 2 +- 13 files changed, 235 insertions(+), 120 deletions(-) -- 2.7.4

Refactor the virStorageFileMatchesNNN methods so that they don't take a struct FileFormatInfo parameter, but instead get the actual raw dat items they needs. This will facilitate reuse in other contexts. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virstoragefile.c | 63 ++++++++++++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 26 deletions(-) diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 16de603..2834baa 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -598,13 +598,12 @@ qedGetBackingStore(char **res, static bool -virStorageFileMatchesMagic(int format, +virStorageFileMatchesMagic(int magicOffset, + const char *magic, char *buf, size_t buflen) { int mlen; - int magicOffset = fileTypeInfo[format].magicOffset; - const char *magic = fileTypeInfo[format].magic; if (magic == NULL) return false; @@ -622,13 +621,13 @@ virStorageFileMatchesMagic(int format, static bool -virStorageFileMatchesExtension(int format, +virStorageFileMatchesExtension(const char *extension, const char *path) { - if (fileTypeInfo[format].extension == NULL) + if (extension == NULL) return false; - if (virFileHasSuffix(path, fileTypeInfo[format].extension)) + if (virFileHasSuffix(path, extension)) return true; return false; @@ -636,7 +635,10 @@ virStorageFileMatchesExtension(int format, static bool -virStorageFileMatchesVersion(int format, +virStorageFileMatchesVersion(int versionOffset, + int versionSize, + const int *versionNumbers, + int endian, char *buf, size_t buflen) { @@ -644,44 +646,44 @@ virStorageFileMatchesVersion(int format, size_t i; /* Validate version number info */ - if (fileTypeInfo[format].versionOffset == -1) + if (versionOffset == -1) return false; /* -2 == non-versioned file format, so trivially match */ - if (fileTypeInfo[format].versionOffset == -2) + if (versionOffset == -2) return true; /* A positive versionOffset, requires using a valid versionSize */ - if (fileTypeInfo[format].versionSize != 2 && - fileTypeInfo[format].versionSize != 4) + if (versionSize != 2 && + versionSize != 4) return false; - if ((fileTypeInfo[format].versionOffset + - fileTypeInfo[format].versionSize) > buflen) + if ((versionOffset + + versionSize) > buflen) return false; - if (fileTypeInfo[format].endian == LV_LITTLE_ENDIAN) { - if (fileTypeInfo[format].versionSize == 4) + if (endian == LV_LITTLE_ENDIAN) { + if (versionSize == 4) version = virReadBufInt32LE(buf + - fileTypeInfo[format].versionOffset); + versionOffset); else version = virReadBufInt16LE(buf + - fileTypeInfo[format].versionOffset); + versionOffset); } else { - if (fileTypeInfo[format].versionSize == 4) + if (versionSize == 4) version = virReadBufInt32BE(buf + - fileTypeInfo[format].versionOffset); + versionOffset); else version = virReadBufInt16BE(buf + - fileTypeInfo[format].versionOffset); + versionOffset); } for (i = 0; - i < FILE_TYPE_VERSIONS_LAST && fileTypeInfo[format].versionNumbers[i]; + i < FILE_TYPE_VERSIONS_LAST && versionNumbers[i]; i++) { VIR_DEBUG("Compare detected version %d vs one of the expected versions %d", - version, fileTypeInfo[format].versionNumbers[i]); - if (version == fileTypeInfo[format].versionNumbers[i]) + version, versionNumbers[i]); + if (version == versionNumbers[i]) return true; } @@ -734,8 +736,16 @@ virStorageFileProbeFormatFromBuf(const char *path, /* First check file magic */ for (i = 0; i < VIR_STORAGE_FILE_LAST; i++) { - if (virStorageFileMatchesMagic(i, buf, buflen)) { - if (!virStorageFileMatchesVersion(i, buf, buflen)) { + if (virStorageFileMatchesMagic( + fileTypeInfo[i].magicOffset, + fileTypeInfo[i].magic, + buf, buflen)) { + if (!virStorageFileMatchesVersion( + fileTypeInfo[i].versionOffset, + fileTypeInfo[i].versionSize, + fileTypeInfo[i].versionNumbers, + fileTypeInfo[i].endian, + buf, buflen)) { possibleFormat = i; continue; } @@ -751,7 +761,8 @@ virStorageFileProbeFormatFromBuf(const char *path, /* No magic, so check file extension */ for (i = 0; i < VIR_STORAGE_FILE_LAST; i++) { - if (virStorageFileMatchesExtension(i, path)) { + if (virStorageFileMatchesExtension( + fileTypeInfo[i].extension, path)) { format = i; goto cleanup; } -- 2.7.4

On Tue, Jul 26, 2016 at 07:12:29PM +0100, Daniel P. Berrange wrote:
Refactor the virStorageFileMatchesNNN methods so that they don't take a struct FileFormatInfo parameter, but instead get the actual raw dat items they needs. This will facilitate reuse in other contexts.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virstoragefile.c | 63 ++++++++++++++++++++++++++++------------------- 1 file changed, 37 insertions(+), 26 deletions(-)
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 16de603..2834baa 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -644,44 +646,44 @@ virStorageFileMatchesVersion(int format, size_t i;
/* Validate version number info */ - if (fileTypeInfo[format].versionOffset == -1) + if (versionOffset == -1) return false;
/* -2 == non-versioned file format, so trivially match */ - if (fileTypeInfo[format].versionOffset == -2) + if (versionOffset == -2) return true;
/* A positive versionOffset, requires using a valid versionSize */ - if (fileTypeInfo[format].versionSize != 2 && - fileTypeInfo[format].versionSize != 4) + if (versionSize != 2 && + versionSize != 4) return false;
- if ((fileTypeInfo[format].versionOffset + - fileTypeInfo[format].versionSize) > buflen) + if ((versionOffset + + versionSize) > buflen)
Unwrap shortened lines like this ^^ so it's readable, please. ACK with that changed.

The current LUKS support has a "luks" volume type which has a "luks" encryption format. This partially makes sense if you consider the QEMU shorthand syntax only requires you to specify a format=luks, and it'll automagically uses "raw" as the next level driver. QEMU will however let you override the "raw" with any other driver it supports (vmdk, qcow, rbd, iscsi, etc, etc) IOW the intention though is that the "luks" encryption format is applied to all disk formats (whether raw, qcow2, rbd, gluster or whatever). As such it doesn't make much sense for libvirt to say the volume type is "luks" - we should be saying that it is a "raw" file, but with "luks" encryption applied. IOW, when creating a storage volume we should use this XML <volume> <name>demo.raw</name> <capacity>5368709120</capacity> <target> <format type='raw'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </target> </volume> and when configuring a guest disk we should use <disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/home/berrange/VirtualMachines/demo.raw'/> <target dev='sda' bus='scsi'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </disk> This commit thus removes the "luks" storage volume type added in commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd Author: John Ferlan <jferlan@redhat.com> Date: Tue Jun 21 12:59:54 2016 -0400 util: Add 'luks' to the FileTypeInfo The storage file probing code is modified so that it can probe the actual encryption formats explicitly, rather than merely probing existance of encryption and letting the storage driver guess the format. The rest of the code is then adapted to deal with VIR_STORAGE_FILE_RAW w/ VIR_STORAGE_ENCRYPTION_FORMAT_LUKS instead of just VIR_STORAGE_FILE_LUKS. The commit mentioned above was included in libvirt v2.0.0. So when querying volume XML this will be a change in behaviour vs the 2.0.0 release - it'll report 'raw' instead of 'luks' for the volume format, but still report 'luks' for encryption format. I think this change is OK because the storage driver did not include any support for creating volumes, nor starting guets with luks volumes in v2.0.0 - that only since then. Clearly if we change this we must do it before v2.1.0 though. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 10 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- src/storage/storage_backend.c | 41 +++-- src/storage/storage_backend_fs.c | 17 +- src/storage/storage_backend_gluster.c | 5 - src/util/virstoragefile.c | 202 ++++++++++++++++----- src/util/virstoragefile.h | 1 - tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 4 +- tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlin/vol-luks.xml | 2 +- tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlout/vol-luks.xml | 2 +- 13 files changed, 198 insertions(+), 94 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 4558b9f..d31fde3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1232,9 +1232,13 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, encinfo->s.aes.alias); if (disk->src->format > 0 && - disk->src->type != VIR_STORAGE_TYPE_DIR) - virBufferAsprintf(&opt, "format=%s,", - virStorageFileFormatTypeToString(disk->src->format)); + disk->src->type != VIR_STORAGE_TYPE_DIR) { + const char *qemuformat = virStorageFileFormatTypeToString(disk->src->format); + if (disk->src->encryption && + disk->src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) + qemuformat = "luks"; + virBufferAsprintf(&opt, "format=%s,", qemuformat); + } } VIR_FREE(source); diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9045f37..b3ca993 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1078,7 +1078,7 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, } if (!virStorageSourceIsEmpty(src) && src->encryption && - src->format == VIR_STORAGE_FILE_LUKS) { + src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { if (VIR_ALLOC(secinfo) < 0) return -1; diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 34a1424..b970448 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -2989,7 +2989,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, * can remove the luks object password too */ if (!virStorageSourceIsEmpty(disk->src) && disk->src->encryption && - disk->src->format == VIR_STORAGE_FILE_LUKS) { + disk->src->encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { if (!(encAlias = qemuDomainGetSecretAESAlias(disk->info.alias, true))) { diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 862fb29..5ef536a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -963,12 +963,7 @@ virStorageBackendCreateQemuImgOpts(virStorageEncryptionInfoDefPtr enc, { virBuffer buf = VIR_BUFFER_INITIALIZER; - if (info.format == VIR_STORAGE_FILE_LUKS) { - if (!enc) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("missing luks encryption information")); - goto error; - } + if (info.format == VIR_STORAGE_FILE_RAW && enc) { virQEMUBuildLuksOpts(&buf, enc, info.secretAlias); } else { if (info.backingPath) @@ -1049,7 +1044,7 @@ virStorageBackendCreateQemuImgCheckEncryption(int format, if (virStorageGenerateQcowEncryption(conn, vol) < 0) return -1; } - } else if (format == VIR_STORAGE_FILE_LUKS) { + } else if (format == VIR_STORAGE_FILE_RAW) { if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported volume encryption format %d"), @@ -1116,9 +1111,9 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, int accessRetCode = -1; char *absolutePath = NULL; - if (info->format == VIR_STORAGE_FILE_LUKS) { + if (info->format == VIR_STORAGE_FILE_RAW) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cannot set backing store for luks volume")); + _("cannot set backing store for raw volume")); return -1; } @@ -1283,7 +1278,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, _("format features only available with qcow2")); return NULL; } - if (info.format == VIR_STORAGE_FILE_LUKS) { + if (info.format == VIR_STORAGE_FILE_RAW && + vol->target.encryption != NULL) { if (inputvol) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("cannot use inputvol with luks volume")); @@ -1294,6 +1290,13 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, _("missing encryption description")); return NULL; } + if (vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { + type = "luks"; + } else { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only luks encryption is supported for raw files")); + return NULL; + } } if (inputvol && @@ -1329,7 +1332,9 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, if (info.backingPath) virCommandAddArgList(cmd, "-b", info.backingPath, NULL); - if (info.format == VIR_STORAGE_FILE_LUKS) { + if (info.format == VIR_STORAGE_FILE_RAW && + vol->target.encryption != NULL && + vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { if (virStorageBackendCreateQemuImgSecretObject(cmd, vol, &info) < 0) { VIR_FREE(info.secretAlias); virCommandFree(cmd); @@ -1453,7 +1458,8 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, if (imgformat < 0) goto cleanup; - if (vol->target.format == VIR_STORAGE_FILE_LUKS) { + if (vol->target.format == VIR_STORAGE_FILE_RAW && + vol->target.encryption->format == VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { if (!(secretPath = virStorageBackendCreateQemuImgSecretPath(conn, pool, vol))) goto cleanup; @@ -1484,13 +1490,16 @@ virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol, if (!inputvol) return NULL; - /* If either volume is a non-raw file vol, we need to use an external - * tool for converting + VIR_WARN("BUild vol from func"); + /* If either volume is a non-raw file vol, or uses encryption, + * we need to use an external tool for converting */ if ((vol->type == VIR_STORAGE_VOL_FILE && - vol->target.format != VIR_STORAGE_FILE_RAW) || + (vol->target.format != VIR_STORAGE_FILE_RAW || + vol->target.encryption != NULL)) || (inputvol->type == VIR_STORAGE_VOL_FILE && - inputvol->target.format != VIR_STORAGE_FILE_RAW)) { + (inputvol->target.format != VIR_STORAGE_FILE_RAW || + inputvol->target.encryption != NULL))) { return virStorageBackendCreateQemuImg; } diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index 0a12ecb..ac6abbb 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -152,20 +152,6 @@ virStorageBackendProbeTarget(virStorageSourcePtr target, *encryption = meta->encryption; meta->encryption = NULL; - switch (target->format) { - case VIR_STORAGE_FILE_QCOW: - case VIR_STORAGE_FILE_QCOW2: - (*encryption)->format = VIR_STORAGE_ENCRYPTION_FORMAT_QCOW; - break; - - case VIR_STORAGE_FILE_LUKS: - (*encryption)->format = VIR_STORAGE_ENCRYPTION_FORMAT_LUKS; - break; - - case VIR_STORAGE_ENCRYPTION_FORMAT_LAST: - break; - } - /* XXX ideally we'd fill in secret UUID here * but we cannot guarantee 'conn' is non-NULL * at this point in time :-( So we only fill @@ -1182,7 +1168,8 @@ _virStorageBackendFileSystemVolBuild(virConnectPtr conn, inputvol); if (!create_func) return -1; - } else if (vol->target.format == VIR_STORAGE_FILE_RAW) { + } else if (vol->target.format == VIR_STORAGE_FILE_RAW && + vol->target.encryption == NULL) { create_func = virStorageBackendCreateRaw; } else if (vol->target.format == VIR_STORAGE_FILE_DIR) { create_func = createFileDir; diff --git a/src/storage/storage_backend_gluster.c b/src/storage/storage_backend_gluster.c index eda060d..01e1b6b 100644 --- a/src/storage/storage_backend_gluster.c +++ b/src/storage/storage_backend_gluster.c @@ -318,11 +318,6 @@ virStorageBackendGlusterRefreshVol(virStorageBackendGlusterStatePtr state, if (meta->encryption) { vol->target.encryption = meta->encryption; meta->encryption = NULL; - if (vol->target.format == VIR_STORAGE_FILE_QCOW || - vol->target.format == VIR_STORAGE_FILE_QCOW2) - vol->target.encryption->format = VIR_STORAGE_ENCRYPTION_FORMAT_QCOW; - if (vol->target.format == VIR_STORAGE_FILE_LUKS) - vol->target.encryption->format = VIR_STORAGE_ENCRYPTION_FORMAT_LUKS; } vol->target.features = meta->features; meta->features = NULL; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2834baa..c264041 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -43,6 +43,7 @@ #include "viruri.h" #include "dirname.h" #include "virbuffer.h" +#include "virstorageencryption.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -63,7 +64,7 @@ VIR_ENUM_IMPL(virStorageFileFormat, "cloop", "dmg", "iso", "vpc", "vdi", /* Not direct file formats, but used for various drivers */ - "fat", "vhd", "ploop", "luks", + "fat", "vhd", "ploop", /* Formats with backing file below here */ "cow", "qcow", "qcow2", "qed", "vmdk") @@ -113,6 +114,26 @@ enum { #define FILE_TYPE_VERSIONS_LAST 2 +struct FileEncryptionInfo { + int format; /* Encryption format to assign */ + + int magicOffset; /* Byte offset of the magic */ + const char *magic; /* Optional string of magic */ + + enum lv_endian endian; /* Endianness of file format */ + + int versionOffset; /* Byte offset from start of file + * where we find version number, + * -1 to always fail the version test, + * -2 to always pass the version test */ + int versionSize; /* Size in bytes of version data (0, 2, or 4) */ + int versionNumbers[FILE_TYPE_VERSIONS_LAST]; + /* Version numbers to validate. Zeroes are ignored. */ + + int modeOffset; /* Byte offset of the format native encryption mode */ + char modeValue; /* Value expected at offset */ +}; + /* Either 'magic' or 'extension' *must* be provided */ struct FileTypeInfo { int magicOffset; /* Byte offset of the magic */ @@ -136,15 +157,14 @@ struct FileTypeInfo { /* Store a COW base image path (possibly relative), * or NULL if there is no COW base image, to RES; * return BACKING_STORE_* */ - int qcowCryptOffset; /* Byte offset from start of file - * where to find encryption mode, - * -1 if encryption is not used */ + const struct FileEncryptionInfo *cryptInfo; /* Encryption info */ int (*getBackingStore)(char **res, int *format, const char *buf, size_t buf_size); int (*getFeatures)(virBitmapPtr *features, int format, char *buf, ssize_t len); }; + static int cowGetBackingStore(char **, int *, const char *, size_t); static int qcow1GetBackingStore(char **, int *, @@ -195,19 +215,75 @@ qedGetBackingStore(char **, int *, const char *, size_t); /* Format described by qemu commit id '3e308f20e' */ #define LUKS_HDR_VERSION_OFFSET LUKS_HDR_MAGIC_LEN +static struct FileEncryptionInfo const luksEncryptionInfo[] = { + { + .format = VIR_STORAGE_ENCRYPTION_FORMAT_LUKS, + + /* Magic is 'L','U','K','S', 0xBA, 0xBE */ + .magicOffset = 0, + .magic = "\x4c\x55\x4b\x53\xba\xbe", + .endian = LV_BIG_ENDIAN, + + .versionOffset = LUKS_HDR_VERSION_OFFSET, + .versionSize = LUKS_HDR_VERSION_LEN, + .versionNumbers = {1}, + + .modeOffset = -1, + .modeValue = -1, + }, + { 0 } +}; + +static struct FileEncryptionInfo const qcow1EncryptionInfo[] = { + { + .format = VIR_STORAGE_ENCRYPTION_FORMAT_QCOW, + + .magicOffset = 0, + .magic = NULL, + .endian = LV_BIG_ENDIAN, + + .versionOffset = -1, + .versionSize = 0, + .versionNumbers = {}, + + .modeOffset = QCOW1_HDR_CRYPT, + .modeValue = 1, + }, + { 0 } +}; + +static struct FileEncryptionInfo const qcow2EncryptionInfo[] = { + { + .format = VIR_STORAGE_ENCRYPTION_FORMAT_QCOW, + + .magicOffset = 0, + .magic = NULL, + .endian = LV_BIG_ENDIAN, + + .versionOffset = -1, + .versionSize = 0, + .versionNumbers = {}, + + .modeOffset = QCOW2_HDR_CRYPT, + .modeValue = 1, + }, + { 0 } +}; static struct FileTypeInfo const fileTypeInfo[] = { [VIR_STORAGE_FILE_NONE] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, 0, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_RAW] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, 0, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, + luksEncryptionInfo, + NULL, NULL }, [VIR_STORAGE_FILE_DIR] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, 0, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_BOCHS] = { /*"Bochs Virtual HD Image", */ /* Untested */ 0, NULL, NULL, LV_LITTLE_ENDIAN, 64, 4, {0x20000}, - 32+16+16+4+4+4+4+4, 8, 1, -1, NULL, NULL + 32+16+16+4+4+4+4+4, 8, 1, NULL, NULL, NULL }, [VIR_STORAGE_FILE_CLOOP] = { /* #!/bin/sh @@ -216,7 +292,7 @@ static struct FileTypeInfo const fileTypeInfo[] = { */ /* Untested */ 0, NULL, NULL, LV_LITTLE_ENDIAN, -1, 0, {0}, - -1, 0, 0, -1, NULL, NULL + -1, 0, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_DMG] = { /* XXX QEMU says there's no magic for dmg, @@ -224,71 +300,69 @@ static struct FileTypeInfo const fileTypeInfo[] = { * would have to match) but then disables that check. */ 0, NULL, ".dmg", 0, -1, 0, {0}, - -1, 0, 0, -1, NULL, NULL + -1, 0, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_ISO] = { 32769, "CD001", ".iso", LV_LITTLE_ENDIAN, -2, 0, {0}, - -1, 0, 0, -1, NULL, NULL + -1, 0, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_VPC] = { 0, "conectix", NULL, LV_BIG_ENDIAN, 12, 4, {0x10000}, - 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, -1, NULL, NULL + 8 + 4 + 4 + 8 + 4 + 4 + 2 + 2 + 4, 8, 1, NULL, NULL, NULL }, /* TODO: add getBackingStore function */ [VIR_STORAGE_FILE_VDI] = { 64, "\x7f\x10\xda\xbe", ".vdi", LV_LITTLE_ENDIAN, 68, 4, {0x00010001}, - 64 + 5 * 4 + 256 + 7 * 4, 8, 1, -1, NULL, NULL}, + 64 + 5 * 4 + 256 + 7 * 4, 8, 1, NULL, NULL, NULL}, /* Not direct file formats, but used for various drivers */ [VIR_STORAGE_FILE_FAT] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, 0, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_VHD] = { 0, NULL, NULL, LV_LITTLE_ENDIAN, - -1, 0, {0}, 0, 0, 0, 0, NULL, NULL }, + -1, 0, {0}, 0, 0, 0, NULL, NULL, NULL }, [VIR_STORAGE_FILE_PLOOP] = { 0, "WithouFreSpacExt", NULL, LV_LITTLE_ENDIAN, -2, 0, {0}, PLOOP_IMAGE_SIZE_OFFSET, 0, - PLOOP_SIZE_MULTIPLIER, -1, NULL, NULL }, - - /* Magic is 'L','U','K','S', 0xBA, 0xBE - * Set sizeOffset = -1 and let hypervisor handle */ - [VIR_STORAGE_FILE_LUKS] = { - 0, "\x4c\x55\x4b\x53\xba\xbe", NULL, - LV_BIG_ENDIAN, LUKS_HDR_VERSION_OFFSET, 2, {1}, - -1, 0, 0, -1, NULL, NULL - }, + PLOOP_SIZE_MULTIPLIER, NULL, NULL, NULL }, + /* All formats with a backing store probe below here */ [VIR_STORAGE_FILE_COW] = { 0, "OOOM", NULL, LV_BIG_ENDIAN, 4, 4, {2}, - 4+4+1024+4, 8, 1, -1, cowGetBackingStore, NULL + 4+4+1024+4, 8, 1, NULL, cowGetBackingStore, NULL }, [VIR_STORAGE_FILE_QCOW] = { 0, "QFI", NULL, LV_BIG_ENDIAN, 4, 4, {1}, - QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW1_HDR_CRYPT, qcow1GetBackingStore, NULL + QCOWX_HDR_IMAGE_SIZE, 8, 1, + qcow1EncryptionInfo, + qcow1GetBackingStore, NULL }, [VIR_STORAGE_FILE_QCOW2] = { 0, "QFI", NULL, LV_BIG_ENDIAN, 4, 4, {2, 3}, - QCOWX_HDR_IMAGE_SIZE, 8, 1, QCOW2_HDR_CRYPT, qcow2GetBackingStore, + QCOWX_HDR_IMAGE_SIZE, 8, 1, + qcow2EncryptionInfo, + qcow2GetBackingStore, qcow2GetFeatures }, [VIR_STORAGE_FILE_QED] = { /* http://wiki.qemu.org/Features/QED */ 0, "QED", NULL, LV_LITTLE_ENDIAN, -2, 0, {0}, - QED_HDR_IMAGE_SIZE, 8, 1, -1, qedGetBackingStore, NULL + QED_HDR_IMAGE_SIZE, 8, 1, NULL, qedGetBackingStore, NULL }, [VIR_STORAGE_FILE_VMDK] = { 0, "KDMV", NULL, LV_LITTLE_ENDIAN, 4, 4, {1, 2}, - 4+4+4, 8, 512, -1, vmdk4GetBackingStore, NULL + 4+4+4, 8, 512, NULL, vmdk4GetBackingStore, NULL }, }; verify(ARRAY_CARDINALITY(fileTypeInfo) == VIR_STORAGE_FILE_LAST); + /* qcow2 compatible features in the order they appear on-disk */ enum qcow2CompatibleFeature { QCOW2_COMPATIBLE_FEATURE_LAZY_REFCOUNTS = 0, @@ -642,7 +716,7 @@ virStorageFileMatchesVersion(int versionOffset, char *buf, size_t buflen) { - int version = 0; + int version; size_t i; /* Validate version number info */ @@ -808,6 +882,45 @@ qcow2GetFeatures(virBitmapPtr *features, } +static bool +virStorageFileHasEncryptionFormat(const struct FileEncryptionInfo *info, + char *buf, + size_t len) +{ + if (!info->magic && info->modeOffset == -1) + return 0; /* Shouldn't happen - expect at least one */ + + if (info->magic) { + if (!virStorageFileMatchesMagic(info->magicOffset, + info->magic, + buf, len)) + return false; + + if (info->versionOffset != -1 && + !virStorageFileMatchesVersion(info->versionOffset, + info->versionSize, + info->versionNumbers, + info->endian, + buf, len)) + return false; + + return true; + } else if (info->modeOffset != -1) { + if (info->modeOffset >= len) + return false; + + if (buf[info->modeOffset] != info->modeValue) + return false; + + return true; + } else { + return false; + } +} + + + + /* Given a header in BUF with length LEN, as parsed from the storage file * assuming it has the given FORMAT, populate information into META * with information about the file and its backing store. Return format @@ -820,6 +933,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, int *backingFormat) { int ret = -1; + size_t j; VIR_DEBUG("path=%s, buf=%p, len=%zu, meta->format=%d", meta->path, buf, len, meta->format); @@ -834,6 +948,18 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, goto cleanup; } + if (fileTypeInfo[meta->format].cryptInfo != NULL) { + for (j = 0; fileTypeInfo[meta->format].cryptInfo[j].format != 0; j++) { + if (virStorageFileHasEncryptionFormat(&fileTypeInfo[meta->format].cryptInfo[j], + buf, len)) { + if (VIR_ALLOC(meta->encryption) < 0) + goto cleanup; + + meta->encryption->format = fileTypeInfo[meta->format].cryptInfo[j].format; + } + } + } + /* XXX we should consider moving virStorageBackendUpdateVolInfo * code into this method, for non-magic files */ @@ -858,22 +984,6 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, meta->capacity *= fileTypeInfo[meta->format].sizeMultiplier; } - if (fileTypeInfo[meta->format].qcowCryptOffset != -1) { - int crypt_format; - - crypt_format = virReadBufInt32BE(buf + - fileTypeInfo[meta->format].qcowCryptOffset); - if (crypt_format && !meta->encryption && - VIR_ALLOC(meta->encryption) < 0) - goto cleanup; - } - - if (meta->format == VIR_STORAGE_FILE_LUKS) { - /* By definition, this is encrypted */ - if (!meta->encryption && VIR_ALLOC(meta->encryption) < 0) - goto cleanup; - } - VIR_FREE(meta->backingStoreRaw); if (fileTypeInfo[meta->format].getBackingStore != NULL) { int store = fileTypeInfo[meta->format].getBackingStore(&meta->backingStoreRaw, diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index 78beaf4..71a8b3a 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -74,7 +74,6 @@ typedef enum { VIR_STORAGE_FILE_FAT, VIR_STORAGE_FILE_VHD, VIR_STORAGE_FILE_PLOOP, - VIR_STORAGE_FILE_LUKS, /* Not a format, but a marker: all formats below this point have * libvirt support for following a backing chain */ diff --git a/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml index 4c9c4c7..3ae0a40 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml @@ -15,7 +15,7 @@ <devices> <emulator>/usr/bin/qemu</emulator> <disk type='file' device='disk'> - <driver name='qemu' type='luks'/> + <driver name='qemu' type='raw'/> <source file='/storage/guest_disks/encryptdisk'/> <target dev='vda' bus='virtio'/> <encryption format='luks'> @@ -24,7 +24,7 @@ <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> </disk> <disk type='file' device='disk'> - <driver name='qemu' type='luks'/> + <driver name='qemu' type='raw'/> <source file='/storage/guest_disks/encryptdisk2'/> <target dev='vdb' bus='virtio'/> <encryption format='luks'> diff --git a/tests/storagevolxml2xmlin/vol-luks-cipher.xml b/tests/storagevolxml2xmlin/vol-luks-cipher.xml index da28a27..94789c6 100644 --- a/tests/storagevolxml2xmlin/vol-luks-cipher.xml +++ b/tests/storagevolxml2xmlin/vol-luks-cipher.xml @@ -7,7 +7,7 @@ <allocation>294912</allocation> <target> <path>/var/lib/libvirt/images/LuksDemo.img</path> - <format type='luks'/> + <format type='raw'/> <permissions> <mode>0644</mode> <owner>0</owner> diff --git a/tests/storagevolxml2xmlin/vol-luks.xml b/tests/storagevolxml2xmlin/vol-luks.xml index bf3c519..9ab1429 100644 --- a/tests/storagevolxml2xmlin/vol-luks.xml +++ b/tests/storagevolxml2xmlin/vol-luks.xml @@ -7,7 +7,7 @@ <allocation>294912</allocation> <target> <path>/var/lib/libvirt/images/LuksDemo.img</path> - <format type='luks'/> + <format type='raw'/> <permissions> <mode>0644</mode> <owner>0</owner> diff --git a/tests/storagevolxml2xmlout/vol-luks-cipher.xml b/tests/storagevolxml2xmlout/vol-luks-cipher.xml index 1ac7424..2b58850 100644 --- a/tests/storagevolxml2xmlout/vol-luks-cipher.xml +++ b/tests/storagevolxml2xmlout/vol-luks-cipher.xml @@ -7,7 +7,7 @@ <allocation unit='bytes'>294912</allocation> <target> <path>/var/lib/libvirt/images/LuksDemo.img</path> - <format type='luks'/> + <format type='raw'/> <permissions> <mode>0644</mode> <owner>0</owner> diff --git a/tests/storagevolxml2xmlout/vol-luks.xml b/tests/storagevolxml2xmlout/vol-luks.xml index 7b82866..599b3c5 100644 --- a/tests/storagevolxml2xmlout/vol-luks.xml +++ b/tests/storagevolxml2xmlout/vol-luks.xml @@ -7,7 +7,7 @@ <allocation unit='bytes'>294912</allocation> <target> <path>/var/lib/libvirt/images/LuksDemo.img</path> - <format type='luks'/> + <format type='raw'/> <permissions> <mode>0644</mode> <owner>0</owner> -- 2.7.4

On Tue, Jul 26, 2016 at 07:12:30PM +0100, Daniel P. Berrange wrote:
The current LUKS support has a "luks" volume type which has a "luks" encryption format.
This partially makes sense if you consider the QEMU shorthand syntax only requires you to specify a format=luks, and it'll automagically uses "raw" as the next level driver. QEMU will however let you override the "raw" with any other driver it supports (vmdk, qcow, rbd, iscsi, etc, etc)
IOW the intention though is that the "luks" encryption format is applied to all disk formats (whether raw, qcow2, rbd, gluster or whatever). As such it doesn't make much sense for libvirt to say the volume type is "luks" - we should be saying that it is a "raw" file, but with "luks" encryption applied.
IOW, when creating a storage volume we should use this XML
<volume> <name>demo.raw</name> <capacity>5368709120</capacity> <target> <format type='raw'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </target> </volume>
and when configuring a guest disk we should use
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/home/berrange/VirtualMachines/demo.raw'/> <target dev='sda' bus='scsi'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </disk>
This commit thus removes the "luks" storage volume type added in
commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd Author: John Ferlan <jferlan@redhat.com> Date: Tue Jun 21 12:59:54 2016 -0400
util: Add 'luks' to the FileTypeInfo
The storage file probing code is modified so that it can probe the actual encryption formats explicitly, rather than merely probing existance of encryption and letting the storage driver guess the format.
The rest of the code is then adapted to deal with VIR_STORAGE_FILE_RAW w/ VIR_STORAGE_ENCRYPTION_FORMAT_LUKS instead of just VIR_STORAGE_FILE_LUKS.
The commit mentioned above was included in libvirt v2.0.0. So when querying volume XML this will be a change in behaviour vs the 2.0.0 release - it'll report 'raw' instead of 'luks' for the volume format, but still report 'luks' for encryption format. I think this change is OK because the storage driver did not include any support for creating volumes, nor starting guets with luks volumes in v2.0.0 - that only since then. Clearly if we change this we must do it before v2.1.0 though.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 10 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- src/storage/storage_backend.c | 41 +++-- src/storage/storage_backend_fs.c | 17 +- src/storage/storage_backend_gluster.c | 5 - src/util/virstoragefile.c | 202 ++++++++++++++++----- src/util/virstoragefile.h | 1 - tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 4 +- tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlin/vol-luks.xml | 2 +- tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlout/vol-luks.xml | 2 +- 13 files changed, 198 insertions(+), 94 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 862fb29..5ef536a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1116,9 +1111,9 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, int accessRetCode = -1; char *absolutePath = NULL;
- if (info->format == VIR_STORAGE_FILE_LUKS) { + if (info->format == VIR_STORAGE_FILE_RAW) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cannot set backing store for luks volume")); + _("cannot set backing store for raw volume")); return -1; }
I think this whole condition can be removed as it wasn't there before luks volumes.
@@ -1283,7 +1278,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, _("format features only available with qcow2")); return NULL; } - if (info.format == VIR_STORAGE_FILE_LUKS) { + if (info.format == VIR_STORAGE_FILE_RAW && + vol->target.encryption != NULL) { if (inputvol) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("cannot use inputvol with luks volume"));
You're still reporting the error for "luks volume" here.
@@ -1484,13 +1490,16 @@ virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol, if (!inputvol) return NULL;
- /* If either volume is a non-raw file vol, we need to use an external - * tool for converting + VIR_WARN("BUild vol from func");
Leftover from debugging?
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2834baa..c264041 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -808,6 +882,45 @@ qcow2GetFeatures(virBitmapPtr *features, }
+static bool +virStorageFileHasEncryptionFormat(const struct FileEncryptionInfo *info, + char *buf, + size_t len) +{ + if (!info->magic && info->modeOffset == -1) + return 0; /* Shouldn't happen - expect at least one */ + + if (info->magic) { + if (!virStorageFileMatchesMagic(info->magicOffset, + info->magic, + buf, len)) + return false; + + if (info->versionOffset != -1 && + !virStorageFileMatchesVersion(info->versionOffset, + info->versionSize, + info->versionNumbers, + info->endian, + buf, len)) + return false; + + return true; + } else if (info->modeOffset != -1) { + if (info->modeOffset >= len) + return false; + + if (buf[info->modeOffset] != info->modeValue) + return false; + + return true; + } else { + return false; + } +} + + + +
Getting used to 2 empty lines took me some time, but we're going four now? Should I look for campaigns proclaiming "Four is the new Two!"? =)
@@ -820,6 +933,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, int *backingFormat) { int ret = -1; + size_t j;
Why 'j'? Apart from those first three nits pointed out the patch is fine. I also like the fact that it automatically get's rid of a problem with format='luks' and no encryption specified (where we just waited for QEMU to fail at startup). The problem is that, as you said, it was released in v2.0.0 and you can have a domain with such disk. I think we need to make a workaround where "luks" is some kind of alias to "raw" (but also make sure it will require encryption because you could have luks without encryption secret). At least we don't have that in the documentation (even though it should've been). Martin

On Wed, Jul 27, 2016 at 08:44:53AM +0200, Martin Kletzander wrote:
On Tue, Jul 26, 2016 at 07:12:30PM +0100, Daniel P. Berrange wrote:
The current LUKS support has a "luks" volume type which has a "luks" encryption format.
This partially makes sense if you consider the QEMU shorthand syntax only requires you to specify a format=luks, and it'll automagically uses "raw" as the next level driver. QEMU will however let you override the "raw" with any other driver it supports (vmdk, qcow, rbd, iscsi, etc, etc)
IOW the intention though is that the "luks" encryption format is applied to all disk formats (whether raw, qcow2, rbd, gluster or whatever). As such it doesn't make much sense for libvirt to say the volume type is "luks" - we should be saying that it is a "raw" file, but with "luks" encryption applied.
IOW, when creating a storage volume we should use this XML
<volume> <name>demo.raw</name> <capacity>5368709120</capacity> <target> <format type='raw'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </target> </volume>
and when configuring a guest disk we should use
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/home/berrange/VirtualMachines/demo.raw'/> <target dev='sda' bus='scsi'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </disk>
This commit thus removes the "luks" storage volume type added in
commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd Author: John Ferlan <jferlan@redhat.com> Date: Tue Jun 21 12:59:54 2016 -0400
util: Add 'luks' to the FileTypeInfo
The storage file probing code is modified so that it can probe the actual encryption formats explicitly, rather than merely probing existance of encryption and letting the storage driver guess the format.
The rest of the code is then adapted to deal with VIR_STORAGE_FILE_RAW w/ VIR_STORAGE_ENCRYPTION_FORMAT_LUKS instead of just VIR_STORAGE_FILE_LUKS.
The commit mentioned above was included in libvirt v2.0.0. So when querying volume XML this will be a change in behaviour vs the 2.0.0 release - it'll report 'raw' instead of 'luks' for the volume format, but still report 'luks' for encryption format. I think this change is OK because the storage driver did not include any support for creating volumes, nor starting guets with luks volumes in v2.0.0 - that only since then. Clearly if we change this we must do it before v2.1.0 though.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 10 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- src/storage/storage_backend.c | 41 +++-- src/storage/storage_backend_fs.c | 17 +- src/storage/storage_backend_gluster.c | 5 - src/util/virstoragefile.c | 202 ++++++++++++++++----- src/util/virstoragefile.h | 1 - tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 4 +- tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlin/vol-luks.xml | 2 +- tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlout/vol-luks.xml | 2 +- 13 files changed, 198 insertions(+), 94 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 862fb29..5ef536a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1116,9 +1111,9 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, int accessRetCode = -1; char *absolutePath = NULL;
- if (info->format == VIR_STORAGE_FILE_LUKS) { + if (info->format == VIR_STORAGE_FILE_RAW) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cannot set backing store for luks volume")); + _("cannot set backing store for raw volume")); return -1; }
I think this whole condition can be removed as it wasn't there before luks volumes.
@@ -1283,7 +1278,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, _("format features only available with qcow2")); return NULL; } - if (info.format == VIR_STORAGE_FILE_LUKS) { + if (info.format == VIR_STORAGE_FILE_RAW && + vol->target.encryption != NULL) { if (inputvol) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("cannot use inputvol with luks volume"));
You're still reporting the error for "luks volume" here.
@@ -1484,13 +1490,16 @@ virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol, if (!inputvol) return NULL;
- /* If either volume is a non-raw file vol, we need to use an external - * tool for converting + VIR_WARN("BUild vol from func");
Leftover from debugging?
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2834baa..c264041 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -808,6 +882,45 @@ qcow2GetFeatures(virBitmapPtr *features, }
+static bool +virStorageFileHasEncryptionFormat(const struct FileEncryptionInfo *info, + char *buf, + size_t len) +{ + if (!info->magic && info->modeOffset == -1) + return 0; /* Shouldn't happen - expect at least one */ + + if (info->magic) { + if (!virStorageFileMatchesMagic(info->magicOffset, + info->magic, + buf, len)) + return false; + + if (info->versionOffset != -1 && + !virStorageFileMatchesVersion(info->versionOffset, + info->versionSize, + info->versionNumbers, + info->endian, + buf, len)) + return false; + + return true; + } else if (info->modeOffset != -1) { + if (info->modeOffset >= len) + return false; + + if (buf[info->modeOffset] != info->modeValue) + return false; + + return true; + } else { + return false; + } +} + + + +
Getting used to 2 empty lines took me some time, but we're going four now? Should I look for campaigns proclaiming "Four is the new Two!"? =)
@@ -820,6 +933,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, int *backingFormat) { int ret = -1; + size_t j;
Why 'j'?
Apart from those first three nits pointed out the patch is fine. I also like the fact that it automatically get's rid of a problem with format='luks' and no encryption specified (where we just waited for QEMU to fail at startup). The problem is that, as you said, it was released in v2.0.0 and you can have a domain with such disk. I think we need to make a workaround where "luks" is some kind of alias to "raw" (but also make sure it will require encryption because you could have luks without encryption secret). At least we don't have that in the documentation (even though it should've been).
No, you can't have a domain with type="luks" AFAICT. QEMU LUKS support was added in: commit da86c6c22674ccc147224afa2740e33d8cbdbf22 Author: John Ferlan <jferlan@redhat.com> Date: Thu Jun 2 16:28:28 2016 -0400 qemu: Add luks support for domain disk Which is not yet released as its post-2.0.0: $ git describe da86c6c22674ccc147224afa2740e33d8cbdbf22 v2.0.0-204-gda86c6c AFAIK, all that v2.0.0 did was allow 'vol-dumpxml' to show "luks" for pre-existing files formatted with luks outside of libvirt. I think the number of people using that will be approximately zero, and even then I think its reasonable to argue that behaviour was a regression from pre-2.0.0, since people's 'raw' disks suddenly changed to 'luks' Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Wed, Jul 27, 2016 at 09:29:29AM +0100, Daniel P. Berrange wrote:
On Wed, Jul 27, 2016 at 08:44:53AM +0200, Martin Kletzander wrote:
On Tue, Jul 26, 2016 at 07:12:30PM +0100, Daniel P. Berrange wrote:
The current LUKS support has a "luks" volume type which has a "luks" encryption format.
This partially makes sense if you consider the QEMU shorthand syntax only requires you to specify a format=luks, and it'll automagically uses "raw" as the next level driver. QEMU will however let you override the "raw" with any other driver it supports (vmdk, qcow, rbd, iscsi, etc, etc)
IOW the intention though is that the "luks" encryption format is applied to all disk formats (whether raw, qcow2, rbd, gluster or whatever). As such it doesn't make much sense for libvirt to say the volume type is "luks" - we should be saying that it is a "raw" file, but with "luks" encryption applied.
IOW, when creating a storage volume we should use this XML
<volume> <name>demo.raw</name> <capacity>5368709120</capacity> <target> <format type='raw'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </target> </volume>
and when configuring a guest disk we should use
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/home/berrange/VirtualMachines/demo.raw'/> <target dev='sda' bus='scsi'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </disk>
This commit thus removes the "luks" storage volume type added in
commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd Author: John Ferlan <jferlan@redhat.com> Date: Tue Jun 21 12:59:54 2016 -0400
util: Add 'luks' to the FileTypeInfo
The storage file probing code is modified so that it can probe the actual encryption formats explicitly, rather than merely probing existance of encryption and letting the storage driver guess the format.
The rest of the code is then adapted to deal with VIR_STORAGE_FILE_RAW w/ VIR_STORAGE_ENCRYPTION_FORMAT_LUKS instead of just VIR_STORAGE_FILE_LUKS.
The commit mentioned above was included in libvirt v2.0.0. So when querying volume XML this will be a change in behaviour vs the 2.0.0 release - it'll report 'raw' instead of 'luks' for the volume format, but still report 'luks' for encryption format. I think this change is OK because the storage driver did not include any support for creating volumes, nor starting guets with luks volumes in v2.0.0 - that only since then. Clearly if we change this we must do it before v2.1.0 though.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 10 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- src/storage/storage_backend.c | 41 +++-- src/storage/storage_backend_fs.c | 17 +- src/storage/storage_backend_gluster.c | 5 - src/util/virstoragefile.c | 202 ++++++++++++++++----- src/util/virstoragefile.h | 1 - tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 4 +- tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlin/vol-luks.xml | 2 +- tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlout/vol-luks.xml | 2 +- 13 files changed, 198 insertions(+), 94 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 862fb29..5ef536a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1116,9 +1111,9 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, int accessRetCode = -1; char *absolutePath = NULL;
- if (info->format == VIR_STORAGE_FILE_LUKS) { + if (info->format == VIR_STORAGE_FILE_RAW) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cannot set backing store for luks volume")); + _("cannot set backing store for raw volume")); return -1; }
I think this whole condition can be removed as it wasn't there before luks volumes.
@@ -1283,7 +1278,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, _("format features only available with qcow2")); return NULL; } - if (info.format == VIR_STORAGE_FILE_LUKS) { + if (info.format == VIR_STORAGE_FILE_RAW && + vol->target.encryption != NULL) { if (inputvol) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("cannot use inputvol with luks volume"));
You're still reporting the error for "luks volume" here.
@@ -1484,13 +1490,16 @@ virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol, if (!inputvol) return NULL;
- /* If either volume is a non-raw file vol, we need to use an external - * tool for converting + VIR_WARN("BUild vol from func");
Leftover from debugging?
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2834baa..c264041 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -808,6 +882,45 @@ qcow2GetFeatures(virBitmapPtr *features, }
+static bool +virStorageFileHasEncryptionFormat(const struct FileEncryptionInfo *info, + char *buf, + size_t len) +{ + if (!info->magic && info->modeOffset == -1) + return 0; /* Shouldn't happen - expect at least one */ + + if (info->magic) { + if (!virStorageFileMatchesMagic(info->magicOffset, + info->magic, + buf, len)) + return false; + + if (info->versionOffset != -1 && + !virStorageFileMatchesVersion(info->versionOffset, + info->versionSize, + info->versionNumbers, + info->endian, + buf, len)) + return false; + + return true; + } else if (info->modeOffset != -1) { + if (info->modeOffset >= len) + return false; + + if (buf[info->modeOffset] != info->modeValue) + return false; + + return true; + } else { + return false; + } +} + + + +
Getting used to 2 empty lines took me some time, but we're going four now? Should I look for campaigns proclaiming "Four is the new Two!"? =)
@@ -820,6 +933,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, int *backingFormat) { int ret = -1; + size_t j;
Why 'j'?
Apart from those first three nits pointed out the patch is fine. I also like the fact that it automatically get's rid of a problem with format='luks' and no encryption specified (where we just waited for QEMU to fail at startup). The problem is that, as you said, it was released in v2.0.0 and you can have a domain with such disk. I think we need to make a workaround where "luks" is some kind of alias to "raw" (but also make sure it will require encryption because you could have luks without encryption secret). At least we don't have that in the documentation (even though it should've been).
No, you can't have a domain with type="luks" AFAICT. QEMU LUKS support was added in:
commit da86c6c22674ccc147224afa2740e33d8cbdbf22 Author: John Ferlan <jferlan@redhat.com> Date: Thu Jun 2 16:28:28 2016 -0400
qemu: Add luks support for domain disk
Which is not yet released as its post-2.0.0:
$ git describe da86c6c22674ccc147224afa2740e33d8cbdbf22 v2.0.0-204-gda86c6c
This only added the support in qemu driver, but it can be defined whenever it's added to the enum.
AFAIK, all that v2.0.0 did was allow 'vol-dumpxml' to show "luks" for pre-existing files formatted with luks outside of libvirt. I think the number of people using that will be approximately zero, and even then I think its reasonable to argue that behaviour was a regression from pre-2.0.0, since people's 'raw' disks suddenly changed to 'luks'
That commit was post-v2.0.0, so that means we don't have a release in which you could be able to start such VM. Unfortunately we have such one that allows you to define the domain. I'm not sure how I feel about such things. I tried forcing the back-compat in the past so that we don't lose domains even though they were not sensibly configured at all, but OTOH I understand it adds a lot of code that's just mess. Maybe we could have a better policy for it. And mention it somewhere becuase this is not the first time I'm having such discussion. Or someone could come up with a way how to solve some of the issues in my patchset for parsing invalid domains that would deal with not only this issue.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Wed, Jul 27, 2016 at 11:16:57AM +0200, Martin Kletzander wrote:
On Wed, Jul 27, 2016 at 09:29:29AM +0100, Daniel P. Berrange wrote:
On Wed, Jul 27, 2016 at 08:44:53AM +0200, Martin Kletzander wrote:
On Tue, Jul 26, 2016 at 07:12:30PM +0100, Daniel P. Berrange wrote:
The current LUKS support has a "luks" volume type which has a "luks" encryption format.
This partially makes sense if you consider the QEMU shorthand syntax only requires you to specify a format=luks, and it'll automagically uses "raw" as the next level driver. QEMU will however let you override the "raw" with any other driver it supports (vmdk, qcow, rbd, iscsi, etc, etc)
IOW the intention though is that the "luks" encryption format is applied to all disk formats (whether raw, qcow2, rbd, gluster or whatever). As such it doesn't make much sense for libvirt to say the volume type is "luks" - we should be saying that it is a "raw" file, but with "luks" encryption applied.
IOW, when creating a storage volume we should use this XML
<volume> <name>demo.raw</name> <capacity>5368709120</capacity> <target> <format type='raw'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </target> </volume>
and when configuring a guest disk we should use
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/home/berrange/VirtualMachines/demo.raw'/> <target dev='sda' bus='scsi'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </disk>
This commit thus removes the "luks" storage volume type added in
commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd Author: John Ferlan <jferlan@redhat.com> Date: Tue Jun 21 12:59:54 2016 -0400
util: Add 'luks' to the FileTypeInfo
The storage file probing code is modified so that it can probe the actual encryption formats explicitly, rather than merely probing existance of encryption and letting the storage driver guess the format.
The rest of the code is then adapted to deal with VIR_STORAGE_FILE_RAW w/ VIR_STORAGE_ENCRYPTION_FORMAT_LUKS instead of just VIR_STORAGE_FILE_LUKS.
The commit mentioned above was included in libvirt v2.0.0. So when querying volume XML this will be a change in behaviour vs the 2.0.0 release - it'll report 'raw' instead of 'luks' for the volume format, but still report 'luks' for encryption format. I think this change is OK because the storage driver did not include any support for creating volumes, nor starting guets with luks volumes in v2.0.0 - that only since then. Clearly if we change this we must do it before v2.1.0 though.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 10 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- src/storage/storage_backend.c | 41 +++-- src/storage/storage_backend_fs.c | 17 +- src/storage/storage_backend_gluster.c | 5 - src/util/virstoragefile.c | 202 ++++++++++++++++----- src/util/virstoragefile.h | 1 - tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 4 +- tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlin/vol-luks.xml | 2 +- tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlout/vol-luks.xml | 2 +- 13 files changed, 198 insertions(+), 94 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 862fb29..5ef536a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1116,9 +1111,9 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, int accessRetCode = -1; char *absolutePath = NULL;
- if (info->format == VIR_STORAGE_FILE_LUKS) { + if (info->format == VIR_STORAGE_FILE_RAW) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cannot set backing store for luks volume")); + _("cannot set backing store for raw volume")); return -1; }
I think this whole condition can be removed as it wasn't there before luks volumes.
@@ -1283,7 +1278,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, _("format features only available with qcow2")); return NULL; } - if (info.format == VIR_STORAGE_FILE_LUKS) { + if (info.format == VIR_STORAGE_FILE_RAW && + vol->target.encryption != NULL) { if (inputvol) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("cannot use inputvol with luks volume"));
You're still reporting the error for "luks volume" here.
@@ -1484,13 +1490,16 @@ virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol, if (!inputvol) return NULL;
- /* If either volume is a non-raw file vol, we need to use an external - * tool for converting + VIR_WARN("BUild vol from func");
Leftover from debugging?
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2834baa..c264041 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -808,6 +882,45 @@ qcow2GetFeatures(virBitmapPtr *features, }
+static bool +virStorageFileHasEncryptionFormat(const struct FileEncryptionInfo *info, + char *buf, + size_t len) +{ + if (!info->magic && info->modeOffset == -1) + return 0; /* Shouldn't happen - expect at least one */ + + if (info->magic) { + if (!virStorageFileMatchesMagic(info->magicOffset, + info->magic, + buf, len)) + return false; + + if (info->versionOffset != -1 && + !virStorageFileMatchesVersion(info->versionOffset, + info->versionSize, + info->versionNumbers, + info->endian, + buf, len)) + return false; + + return true; + } else if (info->modeOffset != -1) { + if (info->modeOffset >= len) + return false; + + if (buf[info->modeOffset] != info->modeValue) + return false; + + return true; + } else { + return false; + } +} + + + +
Getting used to 2 empty lines took me some time, but we're going four now? Should I look for campaigns proclaiming "Four is the new Two!"? =)
@@ -820,6 +933,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, int *backingFormat) { int ret = -1; + size_t j;
Why 'j'?
Apart from those first three nits pointed out the patch is fine. I also like the fact that it automatically get's rid of a problem with format='luks' and no encryption specified (where we just waited for QEMU to fail at startup). The problem is that, as you said, it was released in v2.0.0 and you can have a domain with such disk. I think we need to make a workaround where "luks" is some kind of alias to "raw" (but also make sure it will require encryption because you could have luks without encryption secret). At least we don't have that in the documentation (even though it should've been).
No, you can't have a domain with type="luks" AFAICT. QEMU LUKS support was added in:
commit da86c6c22674ccc147224afa2740e33d8cbdbf22 Author: John Ferlan <jferlan@redhat.com> Date: Thu Jun 2 16:28:28 2016 -0400
qemu: Add luks support for domain disk
Which is not yet released as its post-2.0.0:
$ git describe da86c6c22674ccc147224afa2740e33d8cbdbf22 v2.0.0-204-gda86c6c
This only added the support in qemu driver, but it can be defined whenever it's added to the enum.
AFAIK, all that v2.0.0 did was allow 'vol-dumpxml' to show "luks" for pre-existing files formatted with luks outside of libvirt. I think the number of people using that will be approximately zero, and even then I think its reasonable to argue that behaviour was a regression from pre-2.0.0, since people's 'raw' disks suddenly changed to 'luks'
That commit was post-v2.0.0, so that means we don't have a release in which you could be able to start such VM. Unfortunately we have such one that allows you to define the domain. I'm not sure how I feel about such things. I tried forcing the back-compat in the past so that we don't lose domains even though they were not sensibly configured at all, but OTOH I understand it adds a lot of code that's just mess. Maybe we could have a better policy for it. And mention it somewhere becuase this is not the first time I'm having such discussion. Or someone could come up with a way how to solve some of the issues in my patchset for parsing invalid domains that would deal with not only this issue.
Personally I'm not fussed about compat for something where you could not actually use the thing you defined, because it means its highly unlikely anyone would have bothered to define the thing in the first place, or if they did they would have undefined it shortly afterwards on the basis that it didn't work. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Wed, Jul 27, 2016 at 10:23:18AM +0100, Daniel P. Berrange wrote:
On Wed, Jul 27, 2016 at 11:16:57AM +0200, Martin Kletzander wrote:
On Wed, Jul 27, 2016 at 09:29:29AM +0100, Daniel P. Berrange wrote:
On Wed, Jul 27, 2016 at 08:44:53AM +0200, Martin Kletzander wrote:
On Tue, Jul 26, 2016 at 07:12:30PM +0100, Daniel P. Berrange wrote:
The current LUKS support has a "luks" volume type which has a "luks" encryption format.
This partially makes sense if you consider the QEMU shorthand syntax only requires you to specify a format=luks, and it'll automagically uses "raw" as the next level driver. QEMU will however let you override the "raw" with any other driver it supports (vmdk, qcow, rbd, iscsi, etc, etc)
IOW the intention though is that the "luks" encryption format is applied to all disk formats (whether raw, qcow2, rbd, gluster or whatever). As such it doesn't make much sense for libvirt to say the volume type is "luks" - we should be saying that it is a "raw" file, but with "luks" encryption applied.
IOW, when creating a storage volume we should use this XML
<volume> <name>demo.raw</name> <capacity>5368709120</capacity> <target> <format type='raw'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </target> </volume>
and when configuring a guest disk we should use
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/home/berrange/VirtualMachines/demo.raw'/> <target dev='sda' bus='scsi'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </disk>
This commit thus removes the "luks" storage volume type added in
commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd Author: John Ferlan <jferlan@redhat.com> Date: Tue Jun 21 12:59:54 2016 -0400
util: Add 'luks' to the FileTypeInfo
The storage file probing code is modified so that it can probe the actual encryption formats explicitly, rather than merely probing existance of encryption and letting the storage driver guess the format.
The rest of the code is then adapted to deal with VIR_STORAGE_FILE_RAW w/ VIR_STORAGE_ENCRYPTION_FORMAT_LUKS instead of just VIR_STORAGE_FILE_LUKS.
The commit mentioned above was included in libvirt v2.0.0. So when querying volume XML this will be a change in behaviour vs the 2.0.0 release - it'll report 'raw' instead of 'luks' for the volume format, but still report 'luks' for encryption format. I think this change is OK because the storage driver did not include any support for creating volumes, nor starting guets with luks volumes in v2.0.0 - that only since then. Clearly if we change this we must do it before v2.1.0 though.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 10 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- src/storage/storage_backend.c | 41 +++-- src/storage/storage_backend_fs.c | 17 +- src/storage/storage_backend_gluster.c | 5 - src/util/virstoragefile.c | 202 ++++++++++++++++----- src/util/virstoragefile.h | 1 - tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 4 +- tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlin/vol-luks.xml | 2 +- tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlout/vol-luks.xml | 2 +- 13 files changed, 198 insertions(+), 94 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 862fb29..5ef536a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1116,9 +1111,9 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, int accessRetCode = -1; char *absolutePath = NULL;
- if (info->format == VIR_STORAGE_FILE_LUKS) { + if (info->format == VIR_STORAGE_FILE_RAW) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cannot set backing store for luks volume")); + _("cannot set backing store for raw volume")); return -1; }
I think this whole condition can be removed as it wasn't there before luks volumes.
@@ -1283,7 +1278,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, _("format features only available with qcow2")); return NULL; } - if (info.format == VIR_STORAGE_FILE_LUKS) { + if (info.format == VIR_STORAGE_FILE_RAW && + vol->target.encryption != NULL) { if (inputvol) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("cannot use inputvol with luks volume"));
You're still reporting the error for "luks volume" here.
@@ -1484,13 +1490,16 @@ virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol, if (!inputvol) return NULL;
- /* If either volume is a non-raw file vol, we need to use an external - * tool for converting + VIR_WARN("BUild vol from func");
Leftover from debugging?
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 2834baa..c264041 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -808,6 +882,45 @@ qcow2GetFeatures(virBitmapPtr *features, }
+static bool +virStorageFileHasEncryptionFormat(const struct FileEncryptionInfo *info, + char *buf, + size_t len) +{ + if (!info->magic && info->modeOffset == -1) + return 0; /* Shouldn't happen - expect at least one */ + + if (info->magic) { + if (!virStorageFileMatchesMagic(info->magicOffset, + info->magic, + buf, len)) + return false; + + if (info->versionOffset != -1 && + !virStorageFileMatchesVersion(info->versionOffset, + info->versionSize, + info->versionNumbers, + info->endian, + buf, len)) + return false; + + return true; + } else if (info->modeOffset != -1) { + if (info->modeOffset >= len) + return false; + + if (buf[info->modeOffset] != info->modeValue) + return false; + + return true; + } else { + return false; + } +} + + + +
Getting used to 2 empty lines took me some time, but we're going four now? Should I look for campaigns proclaiming "Four is the new Two!"? =)
@@ -820,6 +933,7 @@ virStorageFileGetMetadataInternal(virStorageSourcePtr meta, int *backingFormat) { int ret = -1; + size_t j;
Why 'j'?
Apart from those first three nits pointed out the patch is fine. I also like the fact that it automatically get's rid of a problem with format='luks' and no encryption specified (where we just waited for QEMU to fail at startup). The problem is that, as you said, it was released in v2.0.0 and you can have a domain with such disk. I think we need to make a workaround where "luks" is some kind of alias to "raw" (but also make sure it will require encryption because you could have luks without encryption secret). At least we don't have that in the documentation (even though it should've been).
No, you can't have a domain with type="luks" AFAICT. QEMU LUKS support was added in:
commit da86c6c22674ccc147224afa2740e33d8cbdbf22 Author: John Ferlan <jferlan@redhat.com> Date: Thu Jun 2 16:28:28 2016 -0400
qemu: Add luks support for domain disk
Which is not yet released as its post-2.0.0:
$ git describe da86c6c22674ccc147224afa2740e33d8cbdbf22 v2.0.0-204-gda86c6c
This only added the support in qemu driver, but it can be defined whenever it's added to the enum.
AFAIK, all that v2.0.0 did was allow 'vol-dumpxml' to show "luks" for pre-existing files formatted with luks outside of libvirt. I think the number of people using that will be approximately zero, and even then I think its reasonable to argue that behaviour was a regression from pre-2.0.0, since people's 'raw' disks suddenly changed to 'luks'
That commit was post-v2.0.0, so that means we don't have a release in which you could be able to start such VM. Unfortunately we have such one that allows you to define the domain. I'm not sure how I feel about such things. I tried forcing the back-compat in the past so that we don't lose domains even though they were not sensibly configured at all, but OTOH I understand it adds a lot of code that's just mess. Maybe we could have a better policy for it. And mention it somewhere becuase this is not the first time I'm having such discussion. Or someone could come up with a way how to solve some of the issues in my patchset for parsing invalid domains that would deal with not only this issue.
Personally I'm not fussed about compat for something where you could not actually use the thing you defined, because it means its highly unlikely anyone would have bothered to define the thing in the first place, or if they did they would have undefined it shortly afterwards on the basis that it didn't work.
OK then, I mean I feel kinda the same. Could we mention it somewhere that doing this is fine and under what circumstances? Nobody would probably look for it or find it, so I'll just save a link to your mail from the archive so that I have something to refer to ;) ACK with those aforementioned details fixed. Martin
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Wed, Jul 27, 2016 at 08:44:53AM +0200, Martin Kletzander wrote:
On Tue, Jul 26, 2016 at 07:12:30PM +0100, Daniel P. Berrange wrote:
The current LUKS support has a "luks" volume type which has a "luks" encryption format.
This partially makes sense if you consider the QEMU shorthand syntax only requires you to specify a format=luks, and it'll automagically uses "raw" as the next level driver. QEMU will however let you override the "raw" with any other driver it supports (vmdk, qcow, rbd, iscsi, etc, etc)
IOW the intention though is that the "luks" encryption format is applied to all disk formats (whether raw, qcow2, rbd, gluster or whatever). As such it doesn't make much sense for libvirt to say the volume type is "luks" - we should be saying that it is a "raw" file, but with "luks" encryption applied.
IOW, when creating a storage volume we should use this XML
<volume> <name>demo.raw</name> <capacity>5368709120</capacity> <target> <format type='raw'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </target> </volume>
and when configuring a guest disk we should use
<disk type='file' device='disk'> <driver name='qemu' type='raw'/> <source file='/home/berrange/VirtualMachines/demo.raw'/> <target dev='sda' bus='scsi'/> <encryption format='luks'> <secret type='passphrase' uuid='0a81f5b2-8403-7b23-c8d6-21ccd2f80d6f'/> </encryption> </disk>
This commit thus removes the "luks" storage volume type added in
commit 318ebb36f1027b3357a32d6f781bd77d7a9043fd Author: John Ferlan <jferlan@redhat.com> Date: Tue Jun 21 12:59:54 2016 -0400
util: Add 'luks' to the FileTypeInfo
The storage file probing code is modified so that it can probe the actual encryption formats explicitly, rather than merely probing existance of encryption and letting the storage driver guess the format.
The rest of the code is then adapted to deal with VIR_STORAGE_FILE_RAW w/ VIR_STORAGE_ENCRYPTION_FORMAT_LUKS instead of just VIR_STORAGE_FILE_LUKS.
The commit mentioned above was included in libvirt v2.0.0. So when querying volume XML this will be a change in behaviour vs the 2.0.0 release - it'll report 'raw' instead of 'luks' for the volume format, but still report 'luks' for encryption format. I think this change is OK because the storage driver did not include any support for creating volumes, nor starting guets with luks volumes in v2.0.0 - that only since then. Clearly if we change this we must do it before v2.1.0 though.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/qemu/qemu_command.c | 10 +- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_hotplug.c | 2 +- src/storage/storage_backend.c | 41 +++-- src/storage/storage_backend_fs.c | 17 +- src/storage/storage_backend_gluster.c | 5 - src/util/virstoragefile.c | 202 ++++++++++++++++----- src/util/virstoragefile.h | 1 - tests/qemuxml2argvdata/qemuxml2argv-luks-disks.xml | 4 +- tests/storagevolxml2xmlin/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlin/vol-luks.xml | 2 +- tests/storagevolxml2xmlout/vol-luks-cipher.xml | 2 +- tests/storagevolxml2xmlout/vol-luks.xml | 2 +- 13 files changed, 198 insertions(+), 94 deletions(-)
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 862fb29..5ef536a 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1116,9 +1111,9 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, int accessRetCode = -1; char *absolutePath = NULL;
- if (info->format == VIR_STORAGE_FILE_LUKS) { + if (info->format == VIR_STORAGE_FILE_RAW) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("cannot set backing store for luks volume")); + _("cannot set backing store for raw volume")); return -1; }
I think this whole condition can be removed as it wasn't there before luks volumes.
Previously it wasn't possible to reach this method when format == raw. With LUKS now being treated as raw, we can reach this method in theory and so we do still need to have this error check.
@@ -1283,7 +1278,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, _("format features only available with qcow2")); return NULL; } - if (info.format == VIR_STORAGE_FILE_LUKS) { + if (info.format == VIR_STORAGE_FILE_RAW && + vol->target.encryption != NULL) { if (inputvol) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("cannot use inputvol with luks volume"));
You're still reporting the error for "luks volume" here.
Yes, will make this more generic.
@@ -1484,13 +1490,16 @@ virStorageBackendGetBuildVolFromFunction(virStorageVolDefPtr vol, if (!inputvol) return NULL;
- /* If either volume is a non-raw file vol, we need to use an external - * tool for converting + VIR_WARN("BUild vol from func");
Leftover from debugging?
Opps, yes. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange
-
Martin Kletzander