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

Re: [libvirt] [PATCH v3 2/2] qemu: Utilize qemu secret objects for RBD auth/secret



On Thu, May 05, 2016 at 15:00:41 -0400, John Ferlan wrote:
> Partial fix for:
> https://bugzilla.redhat.com/show_bug.cgi?id=1182074
> 
> If they're available and we need to pass secrets to qemu, then use the
> qemu domain secret object in order to pass the secrets for RBD volumes
> instead of passing the base64 encoded secret on the command line.
> 
> The goal is to make IV secrets the default and have no user interaction
> required in order to allow using the IV mechanism. If the mechanism
> is not available, then fall back to the current plain mechanism using
> a base64 encoded secret.
> 
> New API's:
> 
> qemu_command.c:
>   qemuBuildSecretInfoProps: (private)
>     Generate/return a JSON properties object for the IV secret to
>     be used by both the command building and eventually the hotplug
>     code in order to add the secret object. Code was designed so that
>     in the future perhaps hotplug could use it if it made sense.
> 
>   qemuBuildSecretIVCommandLine (private)
>     Generate and add to the command line the -object secret for the
>     IV secret. This will be required for the subsequent RBD reference
>     to the object.
> 
>   qemuBuildDiskSecinfoCommandLine (private)
>     Handle adding the IV secret object.
> 
> qemu_domain.c
>   qemuDomainSecretSetup: (private)
>     Shim to call either the IV or Plain Setup functions based upon
>     whether IV secrets are possible (we have the encryption API) or not,
>     we have secrets, and of course if the protocol source is RBD.
> 
>     Use the qemuDomainSecretSetup in qemuDomainSecretDiskPrepare and
>     qemuDomainSecretHostdevPrepare to add the secret rather than assuming
>     plain. In the future when iSCSI secrets can use this mechanism for
>     either a disk or hostdev there will be less change.
> 
> Command Building:
> 
>   Adjust the qemuBuildRBDSecinfoURI API's in order to generate the
>   specific command options for an IV secret, such as:
> 
>     -object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted,
>             format=base64
>     -drive file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\
>            mon_host=mon1.example.org\:6321,password-secret=$alias,...
> 
>   where the 'id=' value is the secret object alias generated by
>   concatenating the disk alias and "-ivKey0". The 'keyid= $masterKey'
>   is the master key shared with qemu, and the -drive syntax will
>   reference that alias as the 'password-secret'. For the -drive
>   syntax, the 'id=myname' is kept to define the username, while the
>   'key=$base64 encoded secret' is removed.
> 
>   While according to the syntax described for qemu commit '60390a21'
>   or as seen in the email archive:
> 
>     https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04083.html
> 
>   it is possible to pass a plaintext password via a file, the qemu
>   commit 'ac1d8878' describes the more feature rich 'keyid=' option
>   based upon the shared masterKey.
> 
> Tests:
> 
>   Add mock's for virRandomBytes and gnutls_rnd in order to return a
>   constant stream of '0xff' in the bytes for a non random key in order
>   to generate "constant" values for the secrets so that the tests can
>   use those results to compare results.
> 
> Hotplug:
> 
>   Since the hotplug code doesn't add command line arguments, passing
>   the encoded secret directly to the monitor will suffice.
> 
> Signed-off-by: John Ferlan <jferlan redhat com>
> ---
>  src/qemu/qemu_command.c                            | 130 ++++++++++++++++++++-
>  src/qemu/qemu_domain.c                             |  56 +++++++--
>  ...emuxml2argv-disk-drive-network-rbd-auth-IV.args |  31 +++++
>  ...qemuxml2argv-disk-drive-network-rbd-auth-IV.xml |  42 +++++++
>  tests/qemuxml2argvmock.c                           |  31 ++++-
>  tests/qemuxml2argvtest.c                           |  11 ++
>  6 files changed, 290 insertions(+), 11 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml


I think this patch is doing too much in one place. You can definitely
split out the mocking code.

> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 93aaf2a..9b0bd7a 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -607,6 +607,119 @@ qemuNetworkDriveGetPort(int protocol,
>  }
>  
>  
> +/**
> + * qemuBuildSecretInfoProps:
> + * @secinfo: pointer to the secret info object
> + * @type: returns a pointer to a character string for object name
> + * @props: json properties to return
> + *
> + * Build the JSON properties for the secret info type.
> + *
> + * Returns 0 on success with the filled in JSON property; otherwise,
> + * returns -1 on failure error message set.
> + */
> +static int
> +qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo,
> +                         const char **type,
> +                         virJSONValuePtr *propsret)
> +{
> +    int ret = -1;
> +    char *keyid = NULL;
> +    virJSONValuePtr props = NULL;
> +
> +    *type = "secret";
> +
> +    if (!(keyid = qemuDomainGetMasterKeyAlias()))
> +        return -1;
> +
> +    if (!(props = virJSONValueNewObject()))
> +        goto cleanup;
> +
> +    if (virJSONValueObjectAdd(props,
> +                              "s:data", secinfo->s.iv.ciphertext,
> +                              "s:keyid", keyid,
> +                              "s:iv", secinfo->s.iv.iv,
> +                              "s:format", "base64", NULL) < 0)
> +        goto cleanup;

You can use virJSONValueObjectCreate instead of the two calls above.

> +
> +    *propsret = props;
> +    props = NULL;
> +    ret = 0;
> +
> + cleanup:
> +    VIR_FREE(keyid);
> +    virJSONValueFree(props);
> +
> +    return ret;
> +}
> +
> +
> +/**
> + * qemuBuildSecretIVCommandLine:
> + * @cmd: the command to modify
> + * @secinfo: pointer to the secret info object
> + *
> + * If the secinfo is available and associated with an IV secret,
> + * then format the command line for the secret object. This object
> + * will be referenced by the device that needs/uses it, so it needs
> + * to be in place first.
> + *
> + * Returns 0 on success, -1 w/ error message on failure
> + */
> +static int
> +qemuBuildSecretIVCommandLine(virCommandPtr cmd,

Why does this contain "IV" you are creating a secret object.

> +                             qemuDomainSecretInfoPtr secinfo)
> +{
> +    int ret = -1;
> +    virJSONValuePtr props = NULL;
> +    const char *type;
> +    char *tmp = NULL;
> +
> +    if (qemuBuildSecretInfoProps(secinfo, &type, &props) < 0)
> +        return -1;
> +
> +    if (!(tmp = qemuBuildObjectCommandlineFromJSON(type, secinfo->s.iv.alias,
> +                                                   props)))
> +        goto cleanup;
> +
> +    virCommandAddArgList(cmd, "-object", tmp, NULL);
> +    ret = 0;
> +
> + cleanup:
> +    virJSONValueFree(props);
> +    VIR_FREE(tmp);
> +
> +    return ret;
> +}
> +
> +
> +/* qemuBuildDiskSecinfoCommandLine:
> + * @cmd: Pointer to the command string
> + * @secinfo: Pointer to a possible secinfo
> + *
> + * Adding an IV secret to the command line for an iSCSI disk currently
> + * does not work. As of qemu 2.6, in order to add the options "user=" and
> + * "password-secret=", one would have to do so using an "-iscsi" command
> + * line option. However, that requires supplying an "id=" that can be used
> + * by some "-drive" command in order to "find" the correct IQN. Unfortunately,
> + * an IQN consists of characters ':' and '/' that cannot be used for an "id=".

I don't think this kind of information belongs into a function comment.
It also doesn't really describe what is going on.

> + *
> + * Returns 0 on success, -1 w/ error on some sort of failure.
> + */
> +static int
> +qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd,
> +                                qemuDomainSecretInfoPtr secinfo)
> +{
> +    /* Not necessary for non IV secrets */
> +    if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_IV)

Again, what is IV supposed to mean? The declaration of the enum is not
commented.

> +        return 0;
> +
> +    /* For now we'll just build the IV. In the future more may be required
> +     * in order to support iSCSI */

This comment is weird. This patch is dealing with RBD.

> +    return qemuBuildSecretIVCommandLine(cmd, secinfo);
> +}
> +
> +
>  /* qemuBuildGeneralSecinfoURI:
>   * @uri: Pointer to the URI structure to add to
>   * @secinfo: Pointer to the secret info data (if present)
> @@ -677,6 +790,10 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf,
>          break;
>  
>      case VIR_DOMAIN_SECRET_INFO_TYPE_IV:
> +        virBufferEscape(buf, '\\', ":", ":id=%s:auth_supported=cephx\\;none",
> +                        secinfo->s.iv.username);
> +        break;
> +
>      case VIR_DOMAIN_SECRET_INFO_TYPE_LAST:
>          return -1;
>      }
> @@ -1083,6 +1200,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>      char *source = NULL;
>      int actualType = virStorageSourceGetActualType(disk->src);
>      qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> +    qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
>  
>      if (idx < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1164,7 +1282,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>          break;
>      }
>  
> -    if (qemuGetDriveSourceString(disk->src, diskPriv->secinfo, &source) < 0)
> +    if (qemuGetDriveSourceString(disk->src, secinfo, &source) < 0)
>          goto error;
>  
>      if (source &&
> @@ -1215,6 +1333,11 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
>  
>          virBufferEscape(&opt, ',', ",", "%s,", source);
>  
> +        if (disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD &&
> +            secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_IV) {
> +            virBufferAsprintf(&opt, "password-secret=%s,", secinfo->s.iv.alias);

This could be added to the switch above the place where you are doing
this.

> +        }
> +
>          if (disk->src->format > 0 &&
>              disk->src->type != VIR_STORAGE_TYPE_DIR)
>              virBufferAsprintf(&opt, "format=%s,",
> @@ -1905,6 +2028,8 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>          virDomainDiskDefPtr disk = def->disks[i];
>          bool withDeviceArg = false;
>          bool deviceFlagMasked = false;
> +        qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> +        qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo;
>  
>          /* Unless we have -device, then USB disks need special
>             handling */
> @@ -1947,6 +2072,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>              break;
>          }
>  
> +        if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0)

qemuBuildDiskSecretCommanLine?

> +            return -1;
> +
>          virCommandAddArg(cmd, "-drive");
>  
>          /* Unfortunately it is not possible to use
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index b174caa..3e627a5 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -897,7 +897,7 @@ qemuDomainSecretPlainSetup(virConnectPtr conn,
>   *
>   * Returns true if we can support the encryption code, false otherwise
>   */
> -static bool ATTRIBUTE_UNUSED
> +static bool
>  qemuDomainSecretHaveEncrypt(void)
>  {
>  #ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
> @@ -920,7 +920,7 @@ qemuDomainSecretHaveEncrypt(void)
>   *
>   * Returns 0 on success, -1 on failure with error message
>   */
> -static int ATTRIBUTE_UNUSED
> +static int
>  qemuDomainSecretIVSetup(virConnectPtr conn,
>                          qemuDomainObjPrivatePtr priv,
>                          qemuDomainSecretInfoPtr secinfo,
> @@ -1030,6 +1030,44 @@ qemuDomainSecretIVSetup(virConnectPtr conn,
>  }
>  
>  
> +/* qemuDomainSecretSetup:
> + * @conn: Pointer to connection
> + * @priv: pointer to domain private object
> + * @secinfo: Pointer to secret info
> + * @srcalias: Alias of the disk/hostdev used to generate the secret alias
> + * @protocol: Protocol for secret
> + * @authdef: Pointer to auth data
> + *
> + * If we have the encryption API present and can support a secret object, then
> + * build the IV secret; otherwise, build the Plain secret. This is the magic
> + * decision point for utilizing the IV secrets for an RBD disk. For now iSCSI
> + * disks and hostdevs will not be able to utilize this mechanism. For details,
> + * see qemuBuildDiskSecinfoCommandLine in qemu_command.c
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +static int
> +qemuDomainSecretSetup(virConnectPtr conn,
> +                      qemuDomainObjPrivatePtr priv,
> +                      qemuDomainSecretInfoPtr secinfo,
> +                      const char *srcalias,
> +                      virStorageNetProtocol protocol,
> +                      virStorageAuthDefPtr authdef)
> +{
> +    if (qemuDomainSecretHaveEncrypt() &&
> +        virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET) &&
> +        protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
> +        if (qemuDomainSecretIVSetup(conn, priv, secinfo,
> +                                    srcalias, protocol, authdef) < 0)
> +            return -1;
> +    } else {
> +        if (qemuDomainSecretPlainSetup(conn, secinfo, protocol, authdef) < 0)
> +            return -1;
> +    }
> +    return 0;

Looks like this should be in the previous patch so that the function
doesn't need to be unused.

> +}
> +
> +
>  /* qemuDomainSecretDiskDestroy:
>   * @disk: Pointer to a disk definition
>   *
> @@ -1058,7 +1096,7 @@ qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk)
>   */
>  int
>  qemuDomainSecretDiskPrepare(virConnectPtr conn,
> -                            qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED,
> +                            qemuDomainObjPrivatePtr priv,
>                              virDomainDiskDefPtr disk)
>  {
>      virStorageSourcePtr src = disk->src;
> @@ -1075,8 +1113,8 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn,
>          if (VIR_ALLOC(secinfo) < 0)
>              return -1;
>  
> -        if (qemuDomainSecretPlainSetup(conn, secinfo, src->protocol,
> -                                       src->auth) < 0)
> +        if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias,
> +                                  src->protocol, src->auth) < 0)
>              goto error;
>  
>          diskPriv->secinfo = secinfo;
> @@ -1119,7 +1157,7 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev)
>   */
>  int
>  qemuDomainSecretHostdevPrepare(virConnectPtr conn,
> -                               qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED,
> +                               qemuDomainObjPrivatePtr priv,
>                                 virDomainHostdevDefPtr hostdev)
>  {
>      virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys;
> @@ -1140,9 +1178,9 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn,
>              if (VIR_ALLOC(secinfo) < 0)
>                  return -1;
>  
> -            if (qemuDomainSecretPlainSetup(conn, secinfo,
> -                                           VIR_STORAGE_NET_PROTOCOL_ISCSI,
> -                                           iscsisrc->auth) < 0)
> +            if (qemuDomainSecretSetup(conn, priv, secinfo, hostdev->info->alias,
> +                                      VIR_STORAGE_NET_PROTOCOL_ISCSI,
> +                                      iscsisrc->auth) < 0)
>                  goto error;
>  
>              hostdevPriv->secinfo = secinfo;
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
> new file mode 100644
> index 0000000..f6c0906
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args
> @@ -0,0 +1,31 @@
> +LC_ALL=C \
> +PATH=/bin \
> +HOME=/home/test \
> +USER=test \
> +LOGNAME=test \
> +QEMU_AUDIO_DRV=none \
> +/usr/bin/qemu \
> +-name QEMUGuest1 \
> +-S \
> +-object secret,id=masterKey0,format=raw,\
> +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \
> +-M pc \
> +-m 214 \
> +-smp 1 \
> +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \
> +-nographic \
> +-nodefaults \
> +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \
> +-no-acpi \
> +-boot c \
> +-usb \
> +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \
> +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \
> +-object secret,id=virtio-disk0-ivKey0,\
> +data=/i1QO1S81K4UELoLXmtCNQzxOaE1Tc4DvecFBfyvFKKWUAawbjGD+yZaz9oyEcnW,\
> +keyid=masterKey0,iv=/////////////////////w==,format=base64 \
> +-drive 'file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\
> +mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\
> +password-secret=virtio-disk0-ivKey0,format=raw,if=none,id=drive-virtio-disk0' \
> +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\
> +id=virtio-disk0

[...]

> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c
> index 1616eed..2bfbf6b 100644
> --- a/tests/qemuxml2argvmock.c
> +++ b/tests/qemuxml2argvmock.c
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (C) 2014 Red Hat, Inc.
> + * Copyright (C) 2014-2016 Red Hat, Inc.

Doesn't look like we've touched it in 2015 ...

>   *
>   * This library is free software; you can redistribute it and/or
>   * modify it under the terms of the GNU Lesser General Public
> @@ -30,6 +30,13 @@
>  #include "virstring.h"
>  #include "virtpm.h"
>  #include "virutil.h"
> +#include "virrandom.h"
> +#ifdef WITH_GNUTLS
> +# include <gnutls/gnutls.h>
> +# if HAVE_GNUTLS_CRYPTO_H
> +#  include <gnutls/crypto.h>
> +# endif
> +#endif
>  #include <time.h>
>  #include <unistd.h>

If you use the approach I've chosen you won't need to include gnutls
headers.

>  
> @@ -145,3 +152,25 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED,
>  {
>      /* nada */
>  }
> +
> +int
> +virRandomBytes(unsigned char *buf,
> +               size_t buflen)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < buflen; i++)
> +        buf[i] = 0xff;
> +
> +    return 0;
> +}
> +
> +#ifdef WITH_GNUTLS
> +int
> +gnutls_rnd(gnutls_rnd_level_t level ATTRIBUTE_UNUSED,
> +           void *data,
> +           size_t len)
> +{
> +    return virRandomBytes(data, len);
> +#endif
> +}

This won't compile without gnutls.

> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index e41444d..1f2577a 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c

...

> @@ -330,6 +331,14 @@ static int testCompareXMLToArgvFiles(const char *xml,
>          }
>      }
>  
> +    /* Create a fake master key */
> +    if (VIR_ALLOC_N(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0)
> +        goto out;
> +
> +    if (virRandomBytes(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0)
> +        goto out;
> +    priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN;
> +
>      if (!(cmd = qemuProcessCreatePretendCmd(conn, &driver, vm, migrateURI,
>                                              (flags & FLAG_FIPS), false,
>                                              VIR_QEMU_PROCESS_START_COLD))) {

This calls qemuProcessPrepareDomain which calls
qemuDomainMasterKeyCreate so the above call to generating the master key
is not necessary.

Attachment: signature.asc
Description: Digital signature


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