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@xxxxxxxxxx>
> ---
> 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(-)
>
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index fdf06ae..0d6d080 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -2153,6 +2153,7 @@ virProcessWait;
>
>
> # util/virqemu.h
> +virQEMUBuildLuksOpts;
> virQEMUBuildObjectCommandlineFromJSON;
>
>
> diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
> index 4965c9e..fc50807 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;
> +}
> +
> +
> 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;
> };
>
> +
> static int
> -virStorageBackendCreateQemuImgOpts(char **opts,
> +virStorageBackendCreateQemuImgOpts(virStorageEncryptionPtr enc,
> + char **opts,
> struct _virStorageBackendQemuImgInfo info)
> {
> virBuffer buf = VIR_BUFFER_INITIALIZER;
>
> - if (info.backingPath)
> - virBufferAsprintf(&buf, "backing_fmt=%s,",
> - virStorageFileFormatTypeToString(info.backingFormat));
> - if (info.encryption)
> - virBufferAddLit(&buf, "encryption=on,");
> - if (info.preallocate)
> - virBufferAddLit(&buf, "preallocation=metadata,");
> + if (info.format == VIR_STORAGE_FILE_LUKS) {
> + virQEMUBuildLuksOpts(&buf, enc, info.secretAlias);
> + } else {
> + if (info.backingPath)
> + virBufferAsprintf(&buf, "backing_fmt=%s,",
> + virStorageFileFormatTypeToString(info.backingFormat));
> + if (info.encryption)
> + virBufferAddLit(&buf, "encryption=on,");
> + if (info.preallocate)
> + virBufferAddLit(&buf, "preallocation=metadata,");
> + }
> +
> if (info.nocow)
> virBufferAddLit(&buf, "nocow=on,");
>
> @@ -1025,6 +1066,22 @@ virStorageBackendCreateQemuImgCheckEncryption(int format,
> if (virStorageGenerateQcowEncryption(conn, vol) < 0)
> return -1;
> }
> + } else if (format == VIR_STORAGE_FILE_LUKS) {
> + if (enc->format != VIR_STORAGE_ENCRYPTION_FORMAT_LUKS) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("unsupported volume encryption format %d"),
> + vol->target.encryption->format);
> + return -1;
> + }
> + if (enc->nsecrets > 1) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("too many secrets for luks encryption"));
> + return -1;
> + }
> + if (enc->nsecrets == 0) {
> + virReportError(VIR_ERR_XML_ERROR, "%s",
> + _("no secret provided for luks encryption"));
> + }
> } else {
> virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> _("volume encryption unsupported with format %s"), type);
> @@ -1069,6 +1126,12 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool,
> int accessRetCode = -1;
> char *absolutePath = NULL;
>
> + if (info->format == VIR_STORAGE_FILE_LUKS) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("cannot set backing store for luks volume"));
> + return -1;
> + }
> +
> info->backingFormat = vol->target.backingStore->format;
> info->backingPath = vol->target.backingStore->path;
>
> @@ -1121,6 +1184,7 @@ virStorageBackendCreateQemuImgSetBacking(virStoragePoolObjPtr pool,
>
> static int
> virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd,
> + virStorageVolDefPtr vol,
> int imgformat,
> struct _virStorageBackendQemuImgInfo info)
> {
> @@ -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,
> + &opts, info) < 0)
> return -1;
> if (opts)
> virCommandAddArgList(cmd, "-o", opts, NULL);
> @@ -1140,6 +1205,66 @@ virStorageBackendCreateQemuImgSetOptions(virCommandPtr cmd,
> }
>
>
> +/* 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;
> +}
> +
> +
> +/* 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)
> +{
> + virStorageEncryptionPtr enc = vol->target.encryption;
> + char *str = NULL;
> + virJSONValuePtr props = NULL;
> + char *commandStr = NULL;
> +
> + if (!(str = virStorageBackendCreateQemuImgLuksName(vol->name, enc)))
> + return -1;
> +
> + if (virAsprintf(&info->secretAlias, "%s_luks0", str) < 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 +1275,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 +1288,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 +1320,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 +1347,6 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
> conn, vol) < 0)
> return NULL;
>
> -
> /* Size in KB */
> info.size_arg = VIR_DIV_UP(vol->target.capacity, 1024);
>
> @@ -1226,10 +1365,20 @@ 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);
> @@ -1240,6 +1389,86 @@ virStorageBackendCreateQemuImgCmdFromVol(virConnectPtr conn,
> return cmd;
> }
>
> +
> +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")))a
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,
[...]
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list