On Mon, Jun 13, 2016 at 20:27:56 -0400, John Ferlan wrote:
If the volume xml was looking to create a luks volume take the necessary steps in order to make that happen.
The processing will be: 1. create a temporary file in the storage pool target path 1a. the file name will either be $volname_$UUID.tmp or $volname_$USAGE.tmp depending on how the encryption xml specified fetching the secret 1b. create/open the file, initially setting mode to 0600 with current effective uid:gid as owner 1c. fetch the secret into a buffer and write that into the file 1d. change file protections to 0400
2. create a secret object 2a. use a similarly crafted alias to the file name 2b. add the file to the object
3. create/add luks options to the commandline 3a. at the very least a "key-secret" using the secret object alias 3b. if found in the XML the various "cipher" and "ivgen" options
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/storage/storage_backend.c | 285 ++++++++++++++++++++++++++++++++++++++--- src/storage/storage_backend.h | 3 +- src/util/virqemu.c | 23 ++++ src/util/virqemu.h | 6 + tests/storagevolxml2argvtest.c | 3 +- 6 files changed, 300 insertions(+), 21 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index fdf06ae..0d6d080 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2153,6 +2153,7 @@ virProcessWait;
# util/virqemu.h +virQEMUBuildLuksOpts; virQEMUBuildObjectCommandlineFromJSON;
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index 4965c9e..fc50807 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -55,11 +55,14 @@ #include "viralloc.h" #include "internal.h" #include "secret_conf.h" +#include "secret_util.h" #include "viruuid.h" #include "virstoragefile.h" #include "storage_backend.h" #include "virlog.h" #include "virfile.h" +#include "virjson.h" +#include "virqemu.h" #include "stat-time.h" #include "virstring.h" #include "virxml.h" @@ -880,6 +883,7 @@ virStoragePloopResize(virStorageVolDefPtr vol, enum { QEMU_IMG_BACKING_FORMAT_OPTIONS = 0, QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT, + QEMU_IMG_FORMAT_LUKS, };
static bool @@ -907,6 +911,27 @@ virStorageBackendQemuImgSupportsCompat(const char *qemuimg) return ret; }
+ +static bool +virStorageBackendQemuImgSupportsLuks(const char *qemuimg) +{ + bool ret = false; + int exitstatus = -1; + virCommandPtr cmd = virCommandNewArgList(qemuimg, "create", "-o", "?", + "-f", "luks", "/dev/null", NULL); + + if (virCommandRun(cmd, &exitstatus) < 0) + goto cleanup; + + if (exitstatus == 0) + ret = true; + + cleanup: + virCommandFree(cmd); + return ret; +} + + static int virStorageBackendQEMUImgBackingFormat(const char *qemuimg) { @@ -915,12 +940,18 @@ virStorageBackendQEMUImgBackingFormat(const char *qemuimg) * out what else we have */ int ret = QEMU_IMG_BACKING_FORMAT_OPTIONS;
- /* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer - * understands. Since we still support QEMU 0.12 and newer, we need - * to be able to handle the previous format as can be set via a - * compat=0.10 option. */ - if (virStorageBackendQemuImgSupportsCompat(qemuimg)) - ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT; + /* QEMU 2.6 added support for luks - let's check for that. + * If available, then we can also assume OPTIONS_COMPAT is present */ + if (virStorageBackendQemuImgSupportsLuks(qemuimg)) { + ret = QEMU_IMG_FORMAT_LUKS; + } else { + /* QEMU 2.0 changed to using a format that only QEMU 1.1 and newer + * understands. Since we still support QEMU 0.12 and newer, we need + * to be able to handle the previous format as can be set via a + * compat=0.10 option. */ + if (virStorageBackendQemuImgSupportsCompat(qemuimg)) + ret = QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT; + }
This looks like it's becoming a source of technical debt. Heaps of comments aren't helping.
return ret; } @@ -941,21 +972,31 @@ struct _virStorageBackendQemuImgInfo { const char *inputPath; const char *inputFormatStr; int inputFormat; + + char *secretAlias; + const char *secretPath; };
+ static int -virStorageBackendCreateQemuImgOpts(char **opts, +virStorageBackendCreateQemuImgOpts(virStorageEncryptionPtr enc, + char **opts, struct _virStorageBackendQemuImgInfo info) { virBuffer buf = VIR_BUFFER_INITIALIZER;
- if (info.backingPath) - virBufferAsprintf(&buf, "backing_fmt=%s,", - virStorageFileFormatTypeToString(info.backingFormat)); - if (info.encryption) - virBufferAddLit(&buf, "encryption=on,"); - if (info.preallocate) - virBufferAddLit(&buf, "preallocation=metadata,"); + if (info.format == VIR_STORAGE_FILE_LUKS) { + virQEMUBuildLuksOpts(&buf, enc, info.secretAlias); + } else { + if (info.backingPath) + virBufferAsprintf(&buf, "backing_fmt=%s,", + virStorageFileFormatTypeToString(info.backingFormat)); + if (info.encryption) + virBufferAddLit(&buf, "encryption=on,"); + if (info.preallocate) + virBufferAddLit(&buf, "preallocation=metadata,"); + } + if (info.nocow) virBufferAddLit(&buf, "nocow=on,");
@@ -1025,6 +1066,22 @@ virStorageBackendCreateQemuImgCheckEncryption(int format, if (virStorageGenerateQcowEncryption(conn, vol) < 0) return -1; } + } else if (format == VIR_STORAGE_FILE_LUKS) { + if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("unsupported volume encryption format %d"), + vol->target.encryption->format); + return -1; + } + if (enc->nsecrets > 1) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("too many secrets for luks encryption")); + return -1; + } + if (enc->nsecrets == 0) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("no secret provided for luks encryption")); + } } else { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("volume encryption unsupported with format %s"), type); @@ -1069,6 +1126,12 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, int accessRetCode = -1; char *absolutePath = NULL;
+ if (info->format == VIR_STORAGE_FILE_LUKS) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot set backing store for luks volume")); + return -1; + } + info->backingFormat = vol->target.backingStore->format; info->backingPath = vol->target.backingStore->path;
@@ -1121,6 +1184,7 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool,
static int virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, + virStorageVolDefPtr vol, int imgformat, struct _virStorageBackendQemuImgInfo info) { @@ -1130,7 +1194,8 @@ virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, imgformat >= QEMU_IMG_BACKING_FORMAT_OPTIONS_COMPAT) info.compat = "0.10";
- if (virStorageBackendCreateQemuImgOpts(&opts, info) < 0) + if (virStorageBackendCreateQemuImgOpts(vol->target.encryption, + &opts, info) < 0) return -1; if (opts) virCommandAddArgList(cmd, "-o", opts, NULL); @@ -1140,6 +1205,66 @@ virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, }
+/* Create a hopefully unique enough name to be used for both the + * secretPath and secretAlias generation
This won't fly. There's only one way to guarantee that there won't be a collision. You need to create a file in a private path.
+ */ +static char * +virStorageBackendCreateQemuImgLuksName(const char *volname, + virStorageEncryptionPtr enc) +{ + char *ret; + + if (enc->secrets[0]->secdef.type == VIR_SECRET_LOOKUP_TYPE_UUID) + ignore_value(virAsprintf(&ret, "%s_%s", volname, + enc->secrets[0]->secdef.u.uuid)); + else + ignore_value(virAsprintf(&ret, "%s_%s", volname, + enc->secrets[0]->secdef.u.usage));
usage is user provided text. Also your example in previous patch uses slashes in it. I don't think it will end up well in this case.
+ return ret; +} + + +/* Add a secret object to the command line: + * --object secret,id=$secretAlias,file=$secretPath + * + * NB: format=raw is assumed + */ +static int +virStorageBackendCreateQemuImgSecretObject(virCommandPtr cmd, + virStorageVolDefPtr vol, + struct _virStorageBackendQemuImgInfo *info) +{ + virStorageEncryptionPtr enc = vol->target.encryption; + char *str = NULL; + virJSONValuePtr props = NULL; + char *commandStr = NULL; + + if (!(str = virStorageBackendCreateQemuImgLuksName(vol->name, enc))) + return -1; + + if (virAsprintf(&info->secretAlias, "%s_luks0", str) < 0) { + VIR_FREE(str); + return -1; + } + VIR_FREE(str); + + if (virJSONValueObjectCreate(&props, "s:file", info->secretPath, NULL) < 0) + return -1; + + if (!(commandStr = virQEMUBuildObjectCommandlineFromJSON("secret", + info->secretAlias, + props))) { + virJSONValueFree(props); + return -1; + } + virJSONValueFree(props); + + virCommandAddArgList(cmd, "--object", commandStr, NULL); + + return 0; +} + + /* Create a qemu-img virCommand from the supplied binary path, * volume definitions and imgformat */ @@ -1150,7 +1275,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, virStorageVolDefPtr inputvol, unsigned int flags, const char *create_tool, - int imgformat) + int imgformat, + const char *secretPath) { virCommandPtr cmd = NULL; const char *type; @@ -1162,6 +1288,8 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, .compat = vol->target.compat, .features = vol->target.features, .nocow = vol->target.nocow, + .secretPath = secretPath, + .secretAlias = NULL, };
virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, NULL); @@ -1192,6 +1320,18 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, _("format features only available with qcow2")); return NULL; } + if (info.format == VIR_STORAGE_FILE_LUKS) { + if (inputvol) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("cannot use inputvol with luks volume")); + return NULL; + } + if (!info.encryption) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing encryption description")); + return NULL; + } + }
if (inputvol && virStorageBackendCreateQemuImgSetInput(inputvol, &info) < 0) @@ -1207,7 +1347,6 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, conn, vol) < 0) return NULL;
- /* Size in KB */ info.size_arg = VIR_DIV_UP(vol->target.capacity, 1024);
@@ -1226,10 +1365,20 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, if (info.backingPath) virCommandAddArgList(cmd, "-b", info.backingPath, NULL);
- if (virStorageBackendCreateQemuImgSetOptions(cmd, imgformat, info) < 0) { + if (info.format == VIR_STORAGE_FILE_LUKS && + virStorageBackendCreateQemuImgSecretObject(cmd, vol, &info) < 0) { + VIR_FREE(info.secretAlias); + virCommandFree(cmd); + return NULL; + } + + if (virStorageBackendCreateQemuImgSetOptions(cmd, vol, imgformat, + info) < 0) { + VIR_FREE(info.secretAlias); virCommandFree(cmd); return NULL; } + VIR_FREE(info.secretAlias);
if (info.inputPath) virCommandAddArg(cmd, info.inputPath); @@ -1240,6 +1389,86 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, return cmd; }
+ +static char * +virStorageBackendCreateQemuImgSecretPath(virConnectPtr conn, + virStoragePoolObjPtr pool, + virStorageVolDefPtr vol) +{ + virStorageEncryptionPtr enc = vol->target.encryption; + char *str = NULL; + char *secretPath = NULL; + int fd = -1; + uint8_t *secret = NULL; + size_t secretlen = 0; + + if (!enc) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("missing encryption description")); + return NULL; + } + + if (!conn || !conn->secretDriver || + !conn->secretDriver->secretLookupByUUID || + !conn->secretDriver->secretLookupByUsage || + !conn->secretDriver->secretGetValue) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to look up encryption secret")); + return NULL; + } + + /* Create a temporary secret file in the pool using */ + if (!(str = virStorageBackendCreateQemuImgLuksName(vol->name, enc))) + return NULL; + + /* Since we don't have a file, just go to cleanup using NULL secretPath */ + if (!(secretPath = virFileBuildPath(pool->def->target.path, str, ".tmp")))a
Apart from creating colliding paths and making possibly volumes appear after a refresh this isn't a good idea also due to the fact that creating the file with the secret may not be possible on NETFS pools due to root squashing.
+ goto cleanup; + + if ((fd = open(secretPath, O_WRONLY|O_TRUNC|O_CREAT, 0600)) < 0) { + virReportSystemError(errno, "%s", + _("failed to open luks secret file for write")); + goto error; + } + + if (virSecretGetSecretString(conn, &enc->secrets[0]->secdef, + VIR_SECRET_USAGE_TYPE_KEY, + &secret, &secretlen) < 0) + goto error; + + if (safewrite(fd, secret, secretlen) < 0) { + virReportSystemError(errno, "%s", + _("failed to write luks secret file")); + goto error; + } + VIR_FORCE_CLOSE(fd); + + if (chown(secretPath, geteuid(), getegid()) < 0) {
You also need to take into account the uid and gid the qemu-img process will be using rather than the effective values.
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to chown luks secret file")); + goto error; + } + + if (chmod(secretPath, 0400) < 0) {
You've already created this with mode 600. Is 400 really necessary? If the process manages to destroy the secret, do you care?
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to chown luks secret file")); + goto error; + } + + cleanup: + VIR_FREE(str); + VIR_DISPOSE_N(secret, secretlen); + VIR_FORCE_CLOSE(fd); + + return secretPath; + + error: + unlink(secretPath); + VIR_FREE(secretPath); + goto cleanup; +} + + int virStorageBackendCreateQemuImg(virConnectPtr conn, virStoragePoolObjPtr pool,
[...]