[libvirt] [PATCH v2 14/14] qemu: Use secret objects to pass iSCSI passwords
Peter Krempa
pkrempa at redhat.com
Fri Sep 22 10:10:03 UTC 2017
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 at redhat.com>
> ---
> 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;
> }
-------------- 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/20170922/3b078687/attachment-0001.sig>
More information about the libvir-list
mailing list