Re: [PATCH v2 14/14] qemu: Use secret objects to pass iSCSI passwords

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

 



On Fri, Sep 15, 2017 at 20:30:17 -0400, John Ferlan wrote:
> https://bugzilla.redhat.com/show_bug.cgi?id=1425757
> 
> The blockdev-add code provides a mechanism to sanely provide user
> and password-secret arguments for iscsi without placing them on the
> command line to be viewable by a 'ps -ef' type command or needing
> to create separate -iscsi devices for each disk/volume found.
> 
> So modify the iSCSI command line building to check for the presence
> of the capability in order properly setup and use the domain master
> secret object to encrypt the password in a secret object and alter
> the parameters for the command line to utilize.
> 
> Modify the xml2argvtest to exhibit the syntax for both disk and
> hostdev configurations.
> 
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
>  src/qemu/qemu_block.c                              | 64 ++++++++++++++++++-
>  src/qemu/qemu_command.c                            | 73 +++++++++++++++++++---
>  src/qemu/qemu_command.h                            |  3 +-
>  src/qemu/qemu_domain.c                             |  4 ++
>  src/qemu/qemu_hotplug.c                            | 49 ++++++++++++++-
>  ...xml2argv-disk-drive-network-iscsi-auth-AES.args | 41 ++++++++++++
>  ...uxml2argv-disk-drive-network-iscsi-auth-AES.xml | 43 +++++++++++++
>  ...ml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args | 45 +++++++++++++
>  ...xml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml | 48 ++++++++++++++
>  tests/qemuxml2argvtest.c                           | 10 +++
>  10 files changed, 366 insertions(+), 14 deletions(-)
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.xml
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.args
>  create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-AES.xml
> 
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 7fb12ea5a..057fb8233 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -482,6 +482,64 @@ qemuBlockStorageSourceGetGlusterProps(virStorageSourcePtr src)
>  }
>  
>  
> +static virJSONValuePtr
> +qemuBlockStorageSourceGetISCSIProps(virStorageSourcePtr src)
> +{
> +    const char *protocol = virStorageNetProtocolTypeToString(src->protocol);
> +    char *target = NULL;
> +    char *lunStr = NULL;
> +    char *username = NULL;
> +    char *objalias = NULL;
> +    unsigned int lun = 0;
> +    virJSONValuePtr ret = NULL;
> +    qemuDomainDiskSrcPrivatePtr diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(src);
> +
> +    /* { driver:"iscsi",
> +     *   transport:"tcp"  ("iser" also possible)
> +     *   portal:"example.com",
> +     *   target:"iqn.2017-04.com.example:iscsi-disks",
> +     *   lun:1,
> +     *   [user:"username",
> +     *    password-secret:"secret-alias",]

As I've pointed out in the VxHS series, using array designators in an
json example to mark "optional" fields is not a great idea.

> +     *  }
> +     */
> +
> +    if (VIR_STRDUP(target, src->path) < 0)
> +        goto cleanup;
> +
> +    /* Separate the target and lun */
> +    if ((lunStr = strchr(target, '/'))) {
> +        *(lunStr++) = '\0';
> +        if (virStrToLong_ui(lunStr, NULL, 10, &lun) < 0) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR,
> +                           _("cannot parse target for lunStr '%s'"),
> +                           target);
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (src->auth) {
> +        username = src->auth->username;
> +        objalias = diskSrcPriv->secinfo->s.aes.alias;
> +    }
> +
> +    ignore_value(virJSONValueObjectCreate(&ret,
> +                                          "s:driver", protocol,
> +                                          "s:portal", src->hosts[0].name,
> +                                          "s:target", target,
> +                                          "u:lun", lun,
> +                                          "s:transport", "tcp",
> +                                          "S:user", username,
> +                                          "S:password-secret", objalias,
> +                                          NULL));
> +        goto cleanup;
> +
> + cleanup:
> +    VIR_FREE(target);
> +    return ret;
> +}

[...]

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index c851823e7..f9edf623c 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c

[...]

> @@ -4846,10 +4854,13 @@ qemuBuildSCSIHostHostdevDrvStr(virDomainHostdevDefPtr dev)
>  }
>  
>  static char *
> -qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev)
> +qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
> +                                virQEMUCapsPtr qemuCaps)
>  {
> +    char *netsource = NULL;
>      char *source = NULL;
> -    virStorageSource src;
> +    virStorageSource src = { 0 };
> +    virJSONValuePtr srcprops = NULL;
>      qemuDomainHostdevPrivatePtr hostdevPriv = QEMU_DOMAIN_HOSTDEV_PRIVATE(dev);
>  
>      memset(&src, 0, sizeof(src));
> @@ -4857,14 +4868,51 @@ qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev)
>      virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
>      virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi;
>  
> +    src.type = VIR_STORAGE_TYPE_NETWORK;
>      src.protocol = VIR_STORAGE_NET_PROTOCOL_ISCSI;
>      src.path = iscsisrc->path;
>      src.hosts = iscsisrc->hosts;
>      src.nhosts = iscsisrc->nhosts;
>  
>      /* Rather than pull what we think we want - use the network disk code */
> -    source = qemuBuildNetworkDriveStr(&src, hostdevPriv->secinfo);
> +    if (qemuDiskSourceNeedsProps(&src, qemuCaps)) {
> +        /* The next pile of code hunts and gathers as if @src were a disk.
> +         * In particular, using private data... So a bit more chicanery is
> +         * going to be required */
> +        qemuDomainDiskSrcPrivatePtr diskSrcPriv;
> +
> +        if (!iscsisrc->auth->username) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("missing username for iSCSI auth"));
> +            goto cleanup;
> +        }
> +        src.auth = iscsisrc->auth;
> +
> +        if (VIR_ALLOC(src.privateData) < 0)
> +            goto cleanup;

src.privateData is a virObjectPtr. This won't work as you expect it to
work. I'd say it'll crash (or corrupt heap) since ... 

> +        diskSrcPriv = QEMU_DOMAIN_DISK_SRC_PRIVATE(&src);

this typecasts the virObjectPtr into a _qemuDomainDiskSrcPrivate
pointer ..

> +        diskSrcPriv->secinfo = hostdevPriv->secinfo;

And this points 8 bytes after the allocated data, as
_qemuDomainDiskSrcPrivate has a virObject folowed by the "secinfo"
pointer.

> +        srcprops = qemuBlockStorageSourceGetBackendProps(&src);
> +        VIR_FREE(src.privateData);
> +        if (!srcprops) {
> +            virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                           _("failed to build the backend props"));
> +            goto cleanup;
> +        }
>  
> +        if (!(netsource = virQEMUBuildDriveCommandlineFromJSON(srcprops)))
> +            goto cleanup;
> +        if (virAsprintf(&source, "%s,if=none,format=raw", netsource) < 0)
> +            goto cleanup;
> +    } else {
> +        if (!(netsource = qemuBuildNetworkDriveStr(&src, hostdevPriv->secinfo)))
> +            goto cleanup;
> +        if (virAsprintf(&source, "file=%s,if=none,format=raw", netsource) < 0)
> +            goto cleanup;
> +    }
> +
> + cleanup:
> +    VIR_FREE(netsource);
>      return source;
>  }
>  
> @@ -4907,7 +4955,8 @@ qemuBuildSCSIVHostHostdevDevStr(const virDomainDef *def,
>  }
>  
>  char *
> -qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev)
> +qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
> +                           virQEMUCapsPtr qemuCaps)
>  {
>      virBuffer buf = VIR_BUFFER_INITIALIZER;
>      char *source = NULL;
> @@ -4915,9 +4964,9 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev)
>      virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
>  
>      if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) {
> -        if (!(source = qemuBuildSCSIiSCSIHostdevDrvStr(dev)))
> +        if (!(source = qemuBuildSCSIiSCSIHostdevDrvStr(dev, qemuCaps)))
>              goto error;
> -        virBufferAsprintf(&buf, "file=%s,if=none,format=raw", source);
> +        virBufferAsprintf(&buf, "%s", source);
>      } else {
>          if (!(source = qemuBuildSCSIHostHostdevDrvStr(dev)))
>              goto error;
> @@ -5414,10 +5463,14 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd,
>          /* SCSI */
>          if (virHostdevIsSCSIDevice(hostdev)) {
>              if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) {
> +                qemuDomainHostdevPrivatePtr hostdevPriv = QEMU_DOMAIN_HOSTDEV_PRIVATE(hostdev);
>                  char *drvstr;
>  
> +                if (qemuBuildDiskSecinfoCommandLine(cmd, hostdevPriv->secinfo) < 0)
> +                    return -1;
> +
>                  virCommandAddArg(cmd, "-drive");
> -                if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev)))
> +                if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps)))
>                      return -1;
>                  virCommandAddArg(cmd, drvstr);
>                  VIR_FREE(drvstr);

> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index a544cecb9..17228d1b4 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c


[...]

> @@ -3879,8 +3909,22 @@ qemuDomainRemoveHostDevice(virQEMUDriverPtr driver,
>          if (!(drivealias = qemuAliasFromHostdev(hostdev)))
>              goto cleanup;
>  
> +        if (virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_ISCSI_PASSWORD_SECRET)) {
> +            if (!(objAlias =
> +                  qemuDomainGetSecretAESAlias(hostdev->info->alias, false))) {
> +                return -1;
> +            }

This will leak stuff since you skip cleanup. Also don't break that line
above.

> +        }
> +
>          qemuDomainObjEnterMonitor(driver, vm);
>          qemuMonitorDriveDel(priv->mon, drivealias);
> +
> +        /* If it fails, then so be it - it was a best shot */
> +        if (objAlias) {
> +            ignore_value(qemuMonitorDelObject(priv->mon, objAlias));
> +            qemuDomainObjDiskSecretObjectAliasEntryRemove(priv, objAlias);
> +        }
> +
>          if (qemuDomainObjExitMonitor(driver, vm) < 0)
>              goto cleanup;
>      }

Attachment: signature.asc
Description: PGP signature

--
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]
  Powered by Linux