On 06/21/2016 09:53 AM, Peter Krempa wrote:
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(-)
[...]
+ 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.
It's on the short list of things to deal with.
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.
Ugh... right. I'm almost tempted to avoid a file type of secret and make it be an AES type secret. This goes away anyway.
+ 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.
Using an update I have in my local repository, this does work with the "correct" XML (gotta have the <permissions> set properly).
+ 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?
Not necessarily - the whole generate a file and set protections is a reaction to a comment Dan made in an earlier review: http://www.redhat.com/archives/libvir-list/2016-June/msg00021.html Would mkostemp() be a "reasonable" option here since we're just going to delete the file anyway? I'd have to add something to storage_driver to return a "path" that included "driver->stateDir", then use mkostemp in order to create a unique name... The alias name thus just becomes the vol->name+"_luks0". Once I get some feedback regarding patch 5, 8, & 11 I can create a shorter v2 with the current set of changes. Thanks for taking the plunge... John
+ 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,
[...]