[libvirt] [REPOST PATCH v6 6/8] qemu: Use secret objects to pass iSCSI passwords

[Peter Krempa]

On Wed, Nov 08, 2017 at 08:15:59 -0500, 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 at redhat.com>
> ---
>  src/qemu/qemu_command.c                            | 65 +++++++++++++++++-----
>  src/qemu/qemu_command.h                            |  3 +-
>  src/qemu/qemu_domain.c                             |  4 ++
>  src/qemu/qemu_hotplug.c                            | 50 ++++++++++++++++-
>  ...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 ++++
>  9 files changed, 292 insertions(+), 17 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_command.c b/src/qemu/qemu_command.c
> index 577c76b44b..f0724223f2 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c

[...]

> @@ -1573,7 +1579,9 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
>              virBufferAsprintf(buf, "file.debug=%d,", cfg->glusterDebugLevel);
>      }
>  
> -    if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES) {
> +    if (secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_AES &&
> +        disk->src->type == VIR_STORAGE_TYPE_NETWORK &&
> +        disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD) {

This hunk is misplaced. if 'srcprops' is present no additional
parameters should be added via this syntax. The same applies also to the
gluster hunk above.

I'll post a patch to move them and then you can commit this patch
without this hunk.

>          /* NB: If libvirt starts using the more modern option based
>           *     syntax to build the command line (e.g., "-drive driver=rbd,
>           *     filename=%s,...") instead of the legacy model (e.g."-drive
> @@ -4892,22 +4900,36 @@ qemuBuildSCSIHostHostdevDrvStr(virDomainHostdevDefPtr dev)
>  }
>  
>  static char *
> -qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev)
> +qemuBuildSCSIiSCSIHostdevDrvStr(virDomainHostdevDefPtr dev,
> +                                virQEMUCapsPtr qemuCaps)
>  {
>      char *source = NULL;
>      char *netsource = NULL;
> +    virJSONValuePtr srcprops = NULL;
>      virDomainHostdevSubsysSCSIPtr scsisrc = &dev->source.subsys.u.scsi;
>      virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi;
>      qemuDomainStorageSourcePrivatePtr srcPriv =
>          QEMU_DOMAIN_STORAGE_SOURCE_PRIVATE(iscsisrc->src);
>  
> -    /* Rather than pull what we think we want - use the network disk code */
> -    netsource = qemuBuildNetworkDriveStr(iscsisrc->src, srcPriv ?
> -                                         srcPriv->secinfo : NULL);
> -    if (!netsource)
> -        goto cleanup;
> -    if (virAsprintf(&source, "file=%s,if=none,format=raw", netsource) < 0)
> -        goto cleanup;
> +    if (qemuDiskSourceNeedsProps(iscsisrc->src, qemuCaps)) {
> +        if (!(srcprops = qemuDiskSourceGetProps(iscsisrc->src))) {
> +            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 {
> +        /* Rather than pull what we think we want - use the network disk code */
> +        if (!(netsource = qemuBuildNetworkDriveStr(iscsisrc->src, srcPriv ?
> +                                                   srcPriv->secinfo : NULL)))
> +            goto cleanup;
> +        if (virAsprintf(&source, "file=%s,if=none,format=raw", netsource) < 0)
> +            goto cleanup;
> +    }
>  
>   cleanup:
>      VIR_FREE(netsource);

This does not clean up srcprops.


[...]


> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-AES.args


I think the test cases for hostdevs and disks should be merged into one
XML testing the new syntax. I don't see a need to have specific ones for
hostdevs and drives.

In general ACK once the test case is sanitized after the drive syntax
formatter is fixed.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20171123/7c45bd36/attachment-0001.sig>