Re: [libvirt] [PATCH v3 06/10] storage: Add support to create a luks volume

On Fri, Jun 24, 2016 at 04:53:35PM -0400, John Ferlan wrote: > Partially resolves: > https://bugzilla.redhat.com/show_bug.cgi?id=1301021 > > 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 driver state dir path > 1a. the file name will include the pool name, the volume name, secret, > and XXXXXX for usage in the mkostemp call. > 1b. mkostemp 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 > > 2. create a secret object > 2a. use an alias combinding the volume name and "_luks0" > 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(a)redhat.com&gt; > --- > src/libvirt_private.syms | 1 + > src/storage/storage_backend.c | 254 > ++++++++++++++++++++++++++++++++++++++--- > src/storage/storage_backend.h | 3 +- > src/util/virqemu.c | 23 ++++ > src/util/virqemu.h | 6 + > tests/storagevolxml2argvtest.c | 3 +- > 6 files changed, 269 insertions(+), 21 deletions(-) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index 9ac033d..eea18dd 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -2169,6 +2169,7 @@ virProcessWait; > > > # util/virqemu.h > +virQEMUBuildLuksOpts; > virQEMUBuildObjectCommandlineFromJSON; > > > diff --git a/src/storage/storage_backend.c > b/src/storage/storage_backend.c > index 97f6ffe..4fb77fc 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; > +} > + > + NACK to this function. Old qemu-img supports the -f option and correctly errors out on unsupported LUKS format. There is no reason to probe for it. > 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; > + } > > return ret; > } This won't be needed either.

> @@ -1121,6 +1184,7 @@ > virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool, > > static int > virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, > + virStorageVolDefPtr vol, > int imgformat, > struct > _virStorageBackendQemuImgInfo info) FWIW the whole point of _virStorageBackendQemuImgInfo was to separate command line building from the volume definition, to allow reuse in qemuDomainSnapshotCreateInactiveExternal where we don't deal with a volume. But I never got around to finishing it. Please use virStorageEncryptionInfoDefPtr instead of virStorageVolDefPtr here.

> { > @@ -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->encinfo, Can vol->target.encryption be NULL here?

> + &opts, info) < 0) > return -1; > if (opts) > virCommandAddArgList(cmd, "-o", opts, NULL); > @@ -1140,6 +1205,43 @@ > virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd, > } > > > +/* 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) > +{ > + char *str = NULL; > + virJSONValuePtr props = NULL; > + char *commandStr = NULL; > + > + if (virAsprintf(&info->secretAlias, "%s_luks0", vol->name) < 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 +1252,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 +1265,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 +1297,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 +1324,6 @@ > virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, > conn, vol) < 0) > return NULL; > > - Spurious whitespace change.

> /* Size in KB */ > info.size_arg = VIR_DIV_UP(vol->target.capacity, 1024); > > @@ -1226,11 +1342,21 @@ > 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); > virCommandAddArg(cmd, info.path); > @@ -1240,6 +1366,78 @@ > virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, > return cmd; > } > > + > +static char * > +virStorageBackendCreateQemuImgSecretPath(virConnectPtr conn, > + virStoragePoolObjPtr pool, > + virStorageVolDefPtr vol) > +{ > + virStorageEncryptionPtr enc = vol->target.encryption; > + 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; > + } > + > + /* Since we don't have a file, just go to cleanup using NULL > secretPath */ What is the purpose of this comment?

> + if (!(secretPath = virStoragePoolObjBuildTempFilePath(pool, vol))) > + goto cleanup; > + > + if ((fd = mkostemp(secretPath, O_CLOEXEC)) < 0) { > + virReportSystemError(errno, "%s", > + _("failed to open luks secret file for > write")); > + goto error; > + } > + > + if (virSecretGetSecretString(conn, &enc->secrets[0]->seclookupdef, > + VIR_SECRET_USAGE_TYPE_PASSPHRASE, > + &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 ((vol->target.perms->uid != (uid_t) -1) && > + (vol->target.perms->gid != (gid_t) -1)) { > + if (chown(secretPath, vol->target.perms->uid, > + vol->target.perms->gid) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", > + _("failed to chown luks secret file")); > + goto error; > + } > + } > + > + cleanup: > + VIR_DISPOSE_N(secret, secretlen); Using VIR_DISPOSE seems superstitious, considering that: we don't use it even once in the secret code and the secrets are in memory all the time.

> + VIR_FORCE_CLOSE(fd); > + > + return secretPath; > + > + error: > + unlink(secretPath); > + VIR_FREE(secretPath); > + goto cleanup; > +} > + > + > int > virStorageBackendCreateQemuImg(virConnectPtr conn, > virStoragePoolObjPtr pool, > @@ -1251,6 +1449,7 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, > char *create_tool; > int imgformat; > virCommandPtr cmd; > + char *secretPath = NULL; > > virCheckFlags(VIR_STORAGE_VOL_CREATE_PREALLOC_METADATA, -1); > > @@ -1266,8 +1465,21 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, > if (imgformat < 0) > goto cleanup; > > + if (vol->target.format == VIR_STORAGE_FILE_LUKS) { > + if (imgformat < QEMU_IMG_FORMAT_LUKS) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > + _("this version of '%s' does not support > luks"), > + create_tool); > + goto cleanup; > + } > + if (!(secretPath = > + virStorageBackendCreateQemuImgSecretPath(conn, pool, > vol))) > + goto cleanup; > + } > + > cmd = virStorageBackendCreateQemuImgCmdFromVol(conn, pool, vol, > inputvol, > - flags, > create_tool, imgformat); > + flags, create_tool, > + imgformat, > secretPath); > if (!cmd) > goto cleanup; > > @@ -1275,6 +1487,10 @@ virStorageBackendCreateQemuImg(virConnectPtr conn, > > virCommandFree(cmd); > cleanup: > + if (secretPath) { > + unlink(secretPath); > + VIR_FREE(secretPath); > + } > VIR_FREE(create_tool); > return ret; > } > diff --git a/src/storage/storage_backend.h > b/src/storage/storage_backend.h > index 5bc622c..28e1a65 100644 > --- a/src/storage/storage_backend.h > +++ b/src/storage/storage_backend.h > @@ -241,7 +241,8 @@ > virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn, > virStorageVolDefPtr inputvol, > unsigned int flags, > const char *create_tool, > - int imgformat); > + int imgformat, > + const char *secretPath); > > /* ------- virStorageFile backends ------------ */ > typedef struct _virStorageFileBackend virStorageFileBackend; > diff --git a/src/util/virqemu.c b/src/util/virqemu.c > index 895168e..4e4a6dd 100644 > --- a/src/util/virqemu.c > +++ b/src/util/virqemu.c > @@ -140,3 +140,26 @@ virQEMUBuildObjectCommandlineFromJSON(const char > *type, > virBufferFreeAndReset(&buf); > return ret; > } > + > + > +void > +virQEMUBuildLuksOpts(virBufferPtr buf, > + virStorageEncryptionInfoDefPtr enc, > + const char *alias) > +{ > + virBufferAsprintf(buf, "key-secret=%s,", alias); > + > + /* If there's any cipher, then add that to the command line */ > + if (enc->cipher_name) { > + virBufferAsprintf(buf, "cipher-alg=%s-%u,", > + enc->cipher_name, enc->cipher_size); > + if (enc->cipher_mode) > + virBufferAsprintf(buf, "cipher-mode=%s,", enc->cipher_mode); > + if (enc->cipher_hash) > + virBufferAsprintf(buf, "hash-alg=%s,", enc->cipher_hash); > + if (enc->ivgen_name) > + virBufferAsprintf(buf, "ivgen-alg=%s,", enc->ivgen_name); > + if (enc->ivgen_hash) > + virBufferAsprintf(buf, "ivgen-hash-alg=%s,", > enc->ivgen_hash); These strings should be escaped or otherwise validated.

ACK with the possible segfault fixed and virStorageBackendQemuImgSupportsLuks removed.