Re: [libvirt] [PATCH 17/19] storage: Add support to create a luks volume

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(a)redhat.com&gt; > --- > 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(-) >

> + > 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; > }; >

> > +/* 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; > +} > +

> +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"))) 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, [...]