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@xxxxxxxxxx>
---
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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list