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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]