Re: [PATCH v2 02/12] qemu: Introduce qemuDomainSecretPrepare and Destroy

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

 



On 04/16/2016 10:17 AM, John Ferlan wrote:
> Rather than needing to pass the conn parameter to various command
> line building API's, add qemuDomainSecretPrepare just prior to the
> qemuProcessLaunch which calls qemuBuilCommandLine. The function
> must be called after qemuProcessPrepareHost since it's expected
> to eventually need the domain masterKey generated during the prepare
> host call. Additionally, future patches may require device aliases
> (assigned during the prepare domain call) in order to associate
> the secret objects.
> 
> The qemuDomainSecretDestroy is called after the qemuProcessLaunch
> finishes in order to clear and free memory used by the secrets
> that were recently prepared, so they are not kept around in memory
> too long.
> 
> Placing the setup here is beneficial for future patches which will
> need the domain masterKey in order to generate an encrypted secret
> along with an initialization vector to be saved and passed (since
> the masterKey shouldn't be passed around).
> 
> Finally, since the secret is not added during command line build,
> the hotplug code will need to get the secret into the private disk data.
> 
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
>  src/qemu/qemu_command.c |  45 ++++-----------
>  src/qemu/qemu_command.h |   5 +-
>  src/qemu/qemu_domain.c  | 150 ++++++++++++++++++++++++++++++++++++++++++++++--
>  src/qemu/qemu_domain.h  |  15 ++++-
>  src/qemu/qemu_driver.c  |  10 ++--
>  src/qemu/qemu_hotplug.c |  26 +++++----
>  src/qemu/qemu_hotplug.h |   1 -
>  src/qemu/qemu_process.c |   8 +++
>  8 files changed, 202 insertions(+), 58 deletions(-)
> 
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 26c19ff..24ed8ed 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -832,7 +832,7 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src,
>  
>  int
>  qemuGetDriveSourceString(virStorageSourcePtr src,
> -                         virConnectPtr conn,
> +                         qemuDomainSecretInfoPtr secinfo,
>                           char **source)
>  {
>      int actualType = virStorageSourceGetActualType(src);
> @@ -846,31 +846,6 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
>      if (virStorageSourceIsEmpty(src))
>          return 1;
>  
> -    if (conn) {
> -        if (actualType == VIR_STORAGE_TYPE_NETWORK &&
> -            src->auth &&
> -            (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI ||
> -             src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
> -            bool encode = false;
> -            int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
> -            const char *protocol = virStorageNetProtocolTypeToString(src->protocol);
> -            username = src->auth->username;
> -
> -            if (src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
> -                /* qemu requires the secret to be encoded for RBD */
> -                encode = true;
> -                secretType = VIR_SECRET_USAGE_TYPE_CEPH;
> -            }
> -
> -            if (!(secret = virSecretGetSecretString(conn,
> -                                                    protocol,
> -                                                    encode,
> -                                                    src->auth,
> -                                                    secretType)))
> -                goto cleanup;
> -        }
> -    }
> -
>      switch ((virStorageType) actualType) {
>      case VIR_STORAGE_TYPE_BLOCK:
>      case VIR_STORAGE_TYPE_FILE:
> @@ -881,6 +856,11 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
>          break;
>  
>      case VIR_STORAGE_TYPE_NETWORK:
> +        if (secinfo) {
> +            username = secinfo->s.plain.username;
> +            secret = secinfo->s.plain.secret;
> +        }
> +
>          if (!(*source = qemuBuildNetworkDriveURI(src, username, secret)))
>              goto cleanup;
>          break;
> @@ -894,7 +874,6 @@ qemuGetDriveSourceString(virStorageSourcePtr src,
>      ret = 0;
>  
>   cleanup:
> -    VIR_FREE(secret);
>      return ret;
>  }
>  
> @@ -1033,8 +1012,7 @@ qemuCheckFips(void)
>  
>  
>  char *
> -qemuBuildDriveStr(virConnectPtr conn,
> -                  virDomainDiskDefPtr disk,
> +qemuBuildDriveStr(virDomainDiskDefPtr disk,
>                    bool bootable,
>                    virQEMUCapsPtr qemuCaps)
>  {
> @@ -1046,6 +1024,7 @@ qemuBuildDriveStr(virConnectPtr conn,
>      int busid = -1, unitid = -1;
>      char *source = NULL;
>      int actualType = virStorageSourceGetActualType(disk->src);
> +    qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
>  
>      if (idx < 0) {
>          virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -1127,7 +1106,7 @@ qemuBuildDriveStr(virConnectPtr conn,
>          break;
>      }
>  
> -    if (qemuGetDriveSourceString(disk->src, conn, &source) < 0)
> +    if (qemuGetDriveSourceString(disk->src, diskPriv->secinfo, &source) < 0)
>          goto error;
>  
>      if (source &&
> @@ -1816,7 +1795,6 @@ qemuBuildDriveDevStr(const virDomainDef *def,
>  
>  static int
>  qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
> -                              virConnectPtr conn,
>                                const virDomainDef *def,
>                                virQEMUCapsPtr qemuCaps,
>                                bool emitBootindex)
> @@ -1910,7 +1888,7 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd,
>                  deviceFlagMasked = true;
>              }
>          }
> -        optstr = qemuBuildDriveStr(conn, disk,
> +        optstr = qemuBuildDriveStr(disk,
>                                     emitBootindex ? false : !!bootindex,
>                                     qemuCaps);
>          if (deviceFlagMasked)
> @@ -9363,8 +9341,7 @@ qemuBuildCommandLine(virConnectPtr conn,
>      if (qemuBuildHubCommandLine(cmd, def, qemuCaps) < 0)
>          goto error;
>  
> -    if (qemuBuildDiskDriveCommandLine(cmd, conn, def, qemuCaps,
> -                                      emitBootindex) < 0)
> +    if (qemuBuildDiskDriveCommandLine(cmd, def, qemuCaps, emitBootindex) < 0)
>          goto error;
>  
>      if (qemuBuildFSDevCommandLine(cmd, def, qemuCaps) < 0)
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 2e2f133..2662d9b 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -99,8 +99,7 @@ char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk,
>                                 virQEMUCapsPtr qemuCaps);
>  
>  /* Both legacy & current support */
> -char *qemuBuildDriveStr(virConnectPtr conn,
> -                        virDomainDiskDefPtr disk,
> +char *qemuBuildDriveStr(virDomainDiskDefPtr disk,
>                          bool bootable,
>                          virQEMUCapsPtr qemuCaps);
>  
> @@ -177,7 +176,7 @@ char *qemuBuildRedirdevDevStr(const virDomainDef *def,
>  int qemuNetworkPrepareDevices(virDomainDefPtr def);
>  
>  int qemuGetDriveSourceString(virStorageSourcePtr src,
> -                             virConnectPtr conn,
> +                             qemuDomainSecretInfoPtr secinfo,
>                               char **source);
>  
>  int qemuCheckDiskConfig(virDomainDiskDefPtr disk);
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 55cb30f..93033d9 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -46,6 +46,7 @@
>  #include "viratomic.h"
>  #include "virprocess.h"
>  #include "virrandom.h"
> +#include "secret_util.h"
>  #include "base64.h"
>  #ifdef WITH_GNUTLS
>  # include <gnutls/gnutls.h>
> @@ -791,6 +792,146 @@ qemuDomainDiskPrivateDispose(void *obj)
>  }
>  
>  
> +/* qemuDomainSecretPlainSetup:
> + * @conn: Pointer to connection
> + * @secinfo: Pointer to secret info
> + * @protocol: Protocol for secret
> + * @authdef: Pointer to auth data
> + *
> + * Taking a secinfo, fill in the plaintext information
> + *
> + * Returns 0 on success, -1 on failure with error message
> + */
> +static int
> +qemuDomainSecretPlainSetup(virConnectPtr conn,
> +                           qemuDomainSecretInfoPtr secinfo,
> +                           virStorageNetProtocol protocol,
> +                           virStorageAuthDefPtr authdef)
> +{
> +    bool encode = false;
> +    int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
> +    const char *protocolstr = virStorageNetProtocolTypeToString(protocol);
> +
> +    secinfo->type = VIR_DOMAIN_SECRET_INFO_PLAIN;
> +    if (VIR_STRDUP(secinfo->s.plain.username, authdef->username) < 0)
> +        return -1;
> +
> +    if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {
> +        /* qemu requires the secret to be encoded for RBD */
> +        encode = true;
> +        secretType = VIR_SECRET_USAGE_TYPE_CEPH;
> +    }
> +
> +    if (!(secinfo->s.plain.secret =
> +          virSecretGetSecretString(conn, protocolstr, encode,
> +                                   authdef, secretType)))
> +        return -1;
> +
> +    return 0;
> +}
> +
> +
> +/* qemuDomainSecretDiskDestroy:
> + * @disk: Pointer to a disk definition
> + *
> + * Clear and destroy memory associated with the secret
> + */
> +void
> +qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk)
> +{
> +    qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> +
> +    if (!diskPriv->secinfo)
> +        return;
> +
> +    qemuDomainSecretInfoFree(&diskPriv->secinfo);
> +}
> +
> +
> +/* qemuDomainSecretDiskPrepare:
> + * @conn: Pointer to connection
> + * @disk: Pointer to a disk definition
> + *
> + * For the right disk, generate the qemuDomainSecretInfo structure.
> + *
> + * Returns 0 on success, -1 on failure
> + */
> +int
> +qemuDomainSecretDiskPrepare(virConnectPtr conn,
> +                            virDomainDiskDefPtr disk)
> +{
> +    virStorageSourcePtr src = disk->src;
> +    qemuDomainSecretInfoPtr secinfo = NULL;
> +
> +    if (conn && !virStorageSourceIsEmpty(src) &&
> +        virStorageSourceGetActualType(src) == VIR_STORAGE_TYPE_NETWORK &&
> +        src->auth &&
> +        (src->protocol == VIR_STORAGE_NET_PROTOCOL_ISCSI ||
> +         src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD)) {
> +

Is conn ever going to be NULL here?

> +        qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> +
> +        if (VIR_ALLOC(secinfo) < 0)
> +            return -1;
> +
> +        if (qemuDomainSecretPlainSetup(conn, secinfo, src->protocol,
> +                                       src->auth) < 0)
> +            goto error;
> +
> +        diskPriv->secinfo = secinfo;
> +    }
> +
> +    return 0;
> +
> + error:
> +    qemuDomainSecretInfoFree(&secinfo);
> +    return -1;
> +}
> +
> +
> +/* qemuDomainSecretDestroy:
> + * @vm: Domain object
> + *
> + * Once completed with the generation of the command line it is
> + * expect to remove the secrets
> + */
> +void
> +qemuDomainSecretDestroy(virDomainObjPtr vm)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < vm->def->ndisks; i++)
> +        qemuDomainSecretDiskDestroy(vm->def->disks[i]);
> +}
> +
> +
> +/* qemuDomainSecretPrepare:
> + * @conn: Pointer to connection
> + * @vm: Domain object
> + *
> + * For any objects that may require an auth/secret setup, create a
> + * qemuDomainSecretInfo and save it in the approriate place within
> + * the private structures. This will be used by command line build
> + * code in order to pass the secret along to qemu in order to provide
> + * the necessary authentication data.
> + *
> + * Returns 0 on success, -1 on failure with error message set
> + */
> +int
> +qemuDomainSecretPrepare(virConnectPtr conn,
> +                        virDomainObjPtr vm)
> +{
> +    size_t i;
> +
> +    for (i = 0; i < vm->def->ndisks; i++) {
> +        if (qemuDomainSecretDiskPrepare(conn, vm->def->disks[i]) < 0)
> +            return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +
>  /* This is the old way of setting up per-domain directories */
>  static int
>  qemuDomainSetPrivatePathsOld(virQEMUDriverPtr driver,
> @@ -3782,8 +3923,7 @@ qemuDomainDiskChainElementPrepare(virQEMUDriverPtr driver,
>  
>  
>  bool
> -qemuDomainDiskSourceDiffers(virConnectPtr conn,
> -                            virDomainDiskDefPtr disk,
> +qemuDomainDiskSourceDiffers(virDomainDiskDefPtr disk,
>                              virDomainDiskDefPtr origDisk)
>  {
>      char *diskSrc = NULL, *origDiskSrc = NULL;
> @@ -3799,8 +3939,10 @@ qemuDomainDiskSourceDiffers(virConnectPtr conn,
>      if (diskEmpty ^ origDiskEmpty)
>          return true;
>  
> -    if (qemuGetDriveSourceString(disk->src, conn, &diskSrc) < 0 ||
> -        qemuGetDriveSourceString(origDisk->src, conn, &origDiskSrc) < 0)
> +    /* This won't be a network storage, so no need to get the diskPriv
> +     * in order to fetch the secret, thus NULL for param2 */
> +    if (qemuGetDriveSourceString(disk->src, NULL, &diskSrc) < 0 ||
> +        qemuGetDriveSourceString(origDisk->src, NULL, &origDiskSrc) < 0)
>          goto cleanup;
>  
>      /* So far in qemu disk sources are considered different
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 9cfe3e4..bde71a4 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -497,8 +497,7 @@ int qemuDomainDetermineDiskChain(virQEMUDriverPtr driver,
>                                   bool force_probe,
>                                   bool report_broken);
>  
> -bool qemuDomainDiskSourceDiffers(virConnectPtr conn,
> -                                 virDomainDiskDefPtr disk,
> +bool qemuDomainDiskSourceDiffers(virDomainDiskDefPtr disk,
>                                   virDomainDiskDefPtr origDisk);
>  
>  bool qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
> @@ -616,4 +615,16 @@ int qemuDomainMasterKeyCreate(virQEMUDriverPtr driver,
>  
>  void qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv);
>  
> +void qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk)
> +    ATTRIBUTE_NONNULL(1);
> +
> +int qemuDomainSecretDiskPrepare(virConnectPtr conn, virDomainDiskDefPtr disk)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +
> +void qemuDomainSecretDestroy(virDomainObjPtr vm)
> +    ATTRIBUTE_NONNULL(1);
> +
> +int qemuDomainSecretPrepare(virConnectPtr conn, virDomainObjPtr vm)
> +    ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2);
> +
>  #endif /* __QEMU_DOMAIN_H__ */
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index 1ba8ab9..bb55b7d 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -7710,14 +7710,16 @@ qemuDomainChangeDiskLive(virConnectPtr conn,
>  
>          orig_disk->startupPolicy = dev->data.disk->startupPolicy;
>  
> -        if (qemuDomainDiskSourceDiffers(conn, disk, orig_disk)) {
> +        if (qemuDomainDiskSourceDiffers(disk, orig_disk)) {
>              /* Add the new disk src into shared disk hash table */
>              if (qemuAddSharedDevice(driver, dev, vm->def->name) < 0)
>                  goto cleanup;
>  
> -            if (qemuDomainChangeEjectableMedia(driver, conn, vm,
> -                                               orig_disk, dev->data.disk->src, force) < 0) {
> -                ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk, vm->def->name));
> +            if (qemuDomainChangeEjectableMedia(driver, vm, orig_disk,
> +                                               dev->data.disk->src,
> +                                               force) < 0) {
> +                ignore_value(qemuRemoveSharedDisk(driver, dev->data.disk,
> +                                                  vm->def->name));
>                  goto rollback;
>              }
>  
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index ef8696b..692e3e7 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -148,7 +148,6 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver,
>  /**
>   * qemuDomainChangeEjectableMedia:
>   * @driver: qemu driver structure
> - * @conn: connection structure
>   * @vm: domain definition
>   * @disk: disk definition to change the source of
>   * @newsrc: new disk source to change to
> @@ -163,7 +162,6 @@ qemuDomainPrepareDisk(virQEMUDriverPtr driver,
>   */
>  int
>  qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
> -                               virConnectPtr conn,
>                                 virDomainObjPtr vm,
>                                 virDomainDiskDefPtr disk,
>                                 virStorageSourcePtr newsrc,
> @@ -223,7 +221,8 @@ qemuDomainChangeEjectableMedia(virQEMUDriverPtr driver,
>      } while (rc < 0);
>  
>      if (!virStorageSourceIsEmpty(newsrc)) {
> -        if (qemuGetDriveSourceString(newsrc, conn, &sourcestr) < 0)
> +        /* cdrom/floppy won't have secret */
> +        if (qemuGetDriveSourceString(newsrc, NULL, &sourcestr) < 0)
>              goto error;
>  

Why not? Can't you have an rbd backed cdrom device?

ACK otherwise

- Cole

--
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]