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

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(a)redhat.com&gt; --- 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)) { +

+ 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;