[libvirt] [PATCH v3 06/10] storage: Add support to create a luks volume
Ján Tomko
jtomko at redhat.com
Sat Jun 25 06:18:48 UTC 2016
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 at redhat.com>
>---
> 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.
Jan
More information about the libvir-list
mailing list