[libvirt] [PATCH v2 0/8] Add IV Secret Object support

v1 here (specifically patches 11 & 12) http://www.redhat.com/archives/libvir-list/2016-April/msg01077.html What's new/changed: Patches 1-3 address comments made by Jan in his review of v1. Patch 4 is essentially following through on the Patch 3 comment Patch 5 was part of patch 11, but I split it out for easier review Patch 6 is the remainder of the former patch 11. Other than using the ...SECRET_INFO_TYPE_xxx nomenclature, no other real changes. Patch 7 was part of patch 12, but I split it out to reduce the amount to review. Beyond the aforementioned ...SECRET_INFO_TYPE_xxx changes, I removed qemuDomainSecretInfoGetAlias. Initially created to help with any hotplug code, but that's not necessary, so remove it for now. It was brought up on Jan's comments as well. Create qemuDomainSecretSetup in order to address Jan's other comment about a common API This code could be merged with patch 8, but separating it *and* leaving qemuDomainSecretHaveEncrypt essentially brain-dead worked made for a shorter pile to look at Patch 8 is the remainder of patch 12. I investigated extracting the iSCSI and RBD code out, but it really just didn't make practice sense. I altered qemuBuildSecretIVCommandLine to just access the IV alias directly since the callers were adjusted to only try to build the IV object if necessary. The qemuBuild{Disk|Hostdev}iSCSICommandLine were renamed to qemuBuild{Disk|Hostdev}SecinfoCommandLine and each encapsulated the call to qemuBuildSecretIVCommandLine rather than inlining it in each of the callers The qemuDomainSecretHaveEncrypt will not be braindead. None of the tests changed, so it doesn't seem I messed anything up with all the requested adjustments. John Ferlan (8): qemu: Adjust names of qemuDomainSecretInfoType enums qemu: Split out the master key create and write qemu: Move qemuDomainSecretPrepare to qemuProcessPrepareDomain qemu: Move qemuDomainSecretDestroy to qemuProcessLaunch qemu: Separate network URI command building code qemu: Introduce qemuDomainSecretIV qemu: Introduce new Secret IV API's qemu: Utilize qemu secret objects for SCSI/RBD auth/secret configure.ac | 1 + src/qemu/qemu_alias.c | 23 ++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_command.c | 359 +++++++++++++++++++-- src/qemu/qemu_domain.c | 242 ++++++++++++-- src/qemu/qemu_domain.h | 34 +- src/qemu/qemu_hotplug.c | 6 +- src/qemu/qemu_process.c | 23 +- ...uxml2argv-disk-drive-network-iscsi-auth-IV.args | 39 +++ ...muxml2argv-disk-drive-network-iscsi-auth-IV.xml | 43 +++ ...emuxml2argv-disk-drive-network-rbd-auth-IV.args | 31 ++ ...qemuxml2argv-disk-drive-network-rbd-auth-IV.xml | 42 +++ ...emuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.args | 41 +++ ...qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.xml | 48 +++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-IV.args | 43 +++ ...uxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.xml | 48 +++ tests/qemuxml2argvmock.c | 31 +- tests/qemuxml2argvtest.c | 19 ++ 18 files changed, 1009 insertions(+), 66 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.xml -- 2.5.5

From a review after push, add the "_TYPE" into the name.
Also use qemuDomainSecretInfoType in the struct rather than int with the comment field containing the struct name Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 3f1fbd7..383c735 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -855,7 +855,7 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, int secretType = VIR_SECRET_USAGE_TYPE_ISCSI; const char *protocolstr = virStorageNetProtocolTypeToString(protocol); - secinfo->type = VIR_DOMAIN_SECRET_INFO_PLAIN; + secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN; if (VIR_STRDUP(secinfo->s.plain.username, authdef->username) < 0) return -1; diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 63f98ba..c81921e 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -241,9 +241,9 @@ struct _qemuDomainObjPrivate { /* Type of domain secret */ typedef enum { - VIR_DOMAIN_SECRET_INFO_PLAIN = 0, + VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN = 0, - VIR_DOMAIN_SECRET_INFO_LAST + VIR_DOMAIN_SECRET_INFO_TYPE_LAST } qemuDomainSecretInfoType; typedef struct _qemuDomainSecretPlain qemuDomainSecretPlain; @@ -256,7 +256,7 @@ struct _qemuDomainSecretPlain { typedef struct _qemuDomainSecretInfo qemuDomainSecretInfo; typedef qemuDomainSecretInfo *qemuDomainSecretInfoPtr; struct _qemuDomainSecretInfo { - int type; /* qemuDomainSecretInfoType */ + qemuDomainSecretInfoType type; union { qemuDomainSecretPlain plain; } s; -- 2.5.5

On Mon, May 02, 2016 at 05:51:08PM -0400, John Ferlan wrote:
From a review after push, add the "_TYPE" into the name.
Also use qemuDomainSecretInfoType in the struct rather than int with the comment field containing the struct name
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 2 +- src/qemu/qemu_domain.h | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

A recent review of related changes noted that we should split the creation (or generation) of the master key into the qemuProcessPrepareDomain and leave the writing of the master key for qemuProcessPrepareHost. Made the adjustment and modified some comments to functions that have changed calling parameters, but didn't change the intro doc. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 23 ++++++++++------------- src/qemu/qemu_domain.h | 6 ++++-- src/qemu/qemu_process.c | 10 ++++++++-- 3 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 383c735..2cc08b8 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -498,13 +498,14 @@ qemuDomainGetMasterKeyFilePath(const char *libDir) /* qemuDomainWriteMasterKeyFile: - * @priv: pointer to domain private object + * @driver: qemu driver data + * @vm: Pointer to the vm object * * Get the desired path to the masterKey file and store it in the path. * * Returns 0 on success, -1 on failure with error message indicating failure */ -static int +int qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver, virDomainObjPtr vm) { @@ -513,6 +514,10 @@ qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver, int ret = -1; qemuDomainObjPrivatePtr priv = vm->privateData; + /* Only gets filled in if we have the capability */ + if (!priv->masterKey) + return 0; + if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir))) return -1; @@ -695,7 +700,7 @@ qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv) /* qemuDomainMasterKeyCreate: - * @priv: Pointer to the domain private object + * @vm: Pointer to the domain object * * As long as the underlying qemu has the secret capability, * generate and store 'raw' in a file a random 32-byte key to @@ -704,8 +709,7 @@ qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv) * Returns: 0 on success, -1 w/ error message on failure */ int -qemuDomainMasterKeyCreate(virQEMUDriverPtr driver, - virDomainObjPtr vm) +qemuDomainMasterKeyCreate(virDomainObjPtr vm) { qemuDomainObjPrivatePtr priv = vm->privateData; @@ -715,18 +719,11 @@ qemuDomainMasterKeyCreate(virQEMUDriverPtr driver, if (!(priv->masterKey = qemuDomainGenerateRandomKey(QEMU_DOMAIN_MASTER_KEY_LEN))) - goto error; + return -1; priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN; - if (qemuDomainWriteMasterKeyFile(driver, vm) < 0) - goto error; - return 0; - - error: - qemuDomainMasterKeyRemove(priv); - return -1; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index c81921e..205c47d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -623,8 +623,10 @@ char *qemuDomainGetMasterKeyFilePath(const char *libDir); int qemuDomainMasterKeyReadFile(qemuDomainObjPrivatePtr priv); -int qemuDomainMasterKeyCreate(virQEMUDriverPtr driver, - virDomainObjPtr vm); +int qemuDomainWriteMasterKeyFile(virQEMUDriverPtr driver, + virDomainObjPtr vm); + +int qemuDomainMasterKeyCreate(virDomainObjPtr vm); void qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv); diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 527300a..2a0362c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5107,6 +5107,10 @@ qemuProcessPrepareDomain(virConnectPtr conn, goto cleanup; } + VIR_DEBUG("Create domain masterKey"); + if (qemuDomainMasterKeyCreate(vm) < 0) + goto cleanup; + if (VIR_ALLOC(priv->monConfig) < 0) goto cleanup; @@ -5121,6 +5125,7 @@ qemuProcessPrepareDomain(virConnectPtr conn, ret = 0; cleanup: + qemuDomainMasterKeyRemove(priv); VIR_FREE(nodeset); virObjectUnref(caps); return ret; @@ -5232,12 +5237,13 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, qemuProcessMakeDir(driver, vm, priv->channelTargetDir) < 0) goto cleanup; - VIR_DEBUG("Create domain masterKey"); - if (qemuDomainMasterKeyCreate(driver, vm) < 0) + VIR_DEBUG("Write domain masterKey"); + if (qemuDomainWriteMasterKeyFile(driver, vm) < 0) goto cleanup; ret = 0; cleanup: + qemuDomainMasterKeyRemove(priv); virObjectUnref(cfg); return ret; } -- 2.5.5

On Mon, May 02, 2016 at 05:51:09PM -0400, John Ferlan wrote:
A recent review of related changes noted that we should split the creation (or generation) of the master key into the qemuProcessPrepareDomain and leave the writing of the master key for qemuProcessPrepareHost.
Made the adjustment and modified some comments to functions that have changed calling parameters, but didn't change the intro doc.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 23 ++++++++++------------- src/qemu/qemu_domain.h | 6 ++++-- src/qemu/qemu_process.c | 10 ++++++++-- 3 files changed, 22 insertions(+), 17 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Commit id '40d8e2ba3' added the function to qemuProcessStart because in order to set up some secrets in the future we will need the master key. However, since the previous patch split the master key creation into two parts (create just the key and create the file), we can now call qemuDomainSecretPrepare from qemuProcessPrepareDomain since the file is not necessary. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2a0362c..baabb31 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5111,6 +5111,10 @@ qemuProcessPrepareDomain(virConnectPtr conn, if (qemuDomainMasterKeyCreate(vm) < 0) goto cleanup; + VIR_DEBUG("Add secrets to disks and hostdevs"); + if (qemuDomainSecretPrepare(conn, vm) < 0) + goto cleanup; + if (VIR_ALLOC(priv->monConfig) < 0) goto cleanup; @@ -5671,9 +5675,6 @@ qemuProcessStart(virConnectPtr conn, if (qemuProcessPrepareHost(driver, vm, !!incoming) < 0) goto stop; - if (qemuDomainSecretPrepare(conn, vm) < 0) - goto cleanup; - if ((rv = qemuProcessLaunch(conn, driver, vm, asyncJob, incoming, snapshot, vmop, flags)) < 0) { if (rv == -2) @@ -5745,9 +5746,6 @@ qemuProcessCreatePretendCmd(virConnectPtr conn, if (qemuProcessPrepareDomain(conn, driver, vm, flags) < 0) goto cleanup; - if (qemuDomainSecretPrepare(conn, vm) < 0) - goto cleanup; - VIR_DEBUG("Building emulator command line"); cmd = qemuBuildCommandLine(driver, NULL, -- 2.5.5

On Mon, May 02, 2016 at 05:51:10PM -0400, John Ferlan wrote:
Commit id '40d8e2ba3' added the function to qemuProcessStart because in order to set up some secrets in the future we will need the master key. However, since the previous patch split the master key creation into two parts (create just the key and create the file), we can now call qemuDomainSecretPrepare from qemuProcessPrepareDomain since the file is not necessary.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Rather than need to call qemuDomainSecretDestroy after any call to qemuProcessLaunch, let's do the destroy in qemuProcessLaunch since that's where command line is eventually generated and processed. Once it's generated, we can clear out the secrets. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index baabb31..91b4f45 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5571,6 +5571,7 @@ qemuProcessLaunch(virConnectPtr conn, ret = 0; cleanup: + qemuDomainSecretDestroy(vm); virCommandFree(cmd); qemuDomainLogContextFree(logCtxt); virObjectUnref(cfg); @@ -5683,8 +5684,6 @@ qemuProcessStart(virConnectPtr conn, } relabel = true; - qemuDomainSecretDestroy(vm); - if (incoming && incoming->deferredURI && qemuMigrationRunIncoming(driver, vm, incoming->deferredURI, asyncJob) < 0) -- 2.5.5

On Mon, May 02, 2016 at 05:51:11PM -0400, John Ferlan wrote:
Rather than need to call qemuDomainSecretDestroy after any call to qemuProcessLaunch, let's do the destroy in qemuProcessLaunch since that's where command line is eventually generated and processed. Once it's generated, we can clear out the secrets.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_process.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Create helper API's in order to build the network URI as shortly we will be adding a new SecretInfo type Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 90 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 69 insertions(+), 21 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index bd564db..e821e58 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -606,6 +606,69 @@ qemuNetworkDriveGetPort(int protocol, return -1; } + +/* qemuBuildGeneralSecinfoURI: + * @uri: Pointer to the URI structure to add to + * @secinfo: Pointer to the secret info data (if present) + * + * If we have a secinfo, then build the command line options for + * the secret info for the "general" case (somewhat a misnomer since + * an iscsi disk is the only one with a secinfo). + * + * Returns 0 on success or if no secinfo, + * -1 and error message if fail to add secret information + */ +static int +qemuBuildGeneralSecinfoURI(virURIPtr uri, + qemuDomainSecretInfoPtr secinfo) +{ + if (!secinfo) + return 0; + + if (secinfo->s.plain.secret) { + if (virAsprintf(&uri->user, "%s:%s", + secinfo->s.plain.username, + secinfo->s.plain.secret) < 0) + return -1; + } else { + if (VIR_STRDUP(uri->user, secinfo->s.plain.username) < 0) + return -1; + } + + return 0; +} + + +/* qemuBuildRBDSecinfoURI: + * @uri: Pointer to the URI structure to add to + * @secinfo: Pointer to the secret info data (if present) + * + * If we have a secinfo, then build the command line options for + * the secret info for the RBD network storage. Assumption for this + * is both username and secret exist for plaintext + * + * Returns 0 on success or if no secinfo, + * -1 and error message if fail to add secret information + */ +static int +qemuBuildRBDSecinfoURI(virBufferPtr buf, + qemuDomainSecretInfoPtr secinfo) +{ + if (!secinfo) { + virBufferAddLit(buf, ":auth_supported=none"); + return 0; + } + + virBufferEscape(buf, '\\', ":", ":id=%s", + secinfo->s.plain.username); + virBufferEscape(buf, '\\', ":", + ":key=%s:auth_supported=cephx\\;none", + secinfo->s.plain.secret); + + return 0; +} + + #define QEMU_DEFAULT_NBD_PORT "10809" static char * @@ -701,7 +764,8 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, goto cleanup; } - if ((uri->port = qemuNetworkDriveGetPort(src->protocol, src->hosts->port)) < 0) + if ((uri->port = qemuNetworkDriveGetPort(src->protocol, + src->hosts->port)) < 0) goto cleanup; if (src->path) { @@ -721,17 +785,8 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0) goto cleanup; - if (secinfo) { - if (secinfo->s.plain.secret) { - if (virAsprintf(&uri->user, "%s:%s", - secinfo->s.plain.username, - secinfo->s.plain.secret) < 0) - goto cleanup; - } else { - if (VIR_STRDUP(uri->user, secinfo->s.plain.username) < 0) - goto cleanup; - } - } + if (qemuBuildGeneralSecinfoURI(uri, secinfo) < 0) + goto cleanup; if (VIR_STRDUP(uri->server, src->hosts->name) < 0) goto cleanup; @@ -777,15 +832,8 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, if (src->snapshot) virBufferEscape(&buf, '\\', ":", "@%s", src->snapshot); - if (secinfo) { - virBufferEscape(&buf, '\\', ":", ":id=%s", - secinfo->s.plain.username); - virBufferEscape(&buf, '\\', ":", - ":key=%s:auth_supported=cephx\\;none", - secinfo->s.plain.secret); - } else { - virBufferAddLit(&buf, ":auth_supported=none"); - } + if (qemuBuildRBDSecinfoURI(&buf, secinfo) < 0) + goto cleanup; if (src->nhosts > 0) { virBufferAddLit(&buf, ":mon_host="); -- 2.5.5

On Mon, May 02, 2016 at 05:51:12PM -0400, John Ferlan wrote:
Create helper API's in order to build the network URI as shortly we will be adding a new SecretInfo type
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 90 +++++++++++++++++++++++++++++++++++++------------ 1 file changed, 69 insertions(+), 21 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

Add the data structure and infrastructure to support an initialization vector (IV) secrets. The IV secret generation will need to have access to the domain private master key, so let's make sure the prepare disk and hostdev functions can accept that now. Anywhere that needs to make a decision over which secret type to use in order to fill in or use the IV secret has a switch added. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 42 +++++++++++++++++++++++++++++------------- src/qemu/qemu_domain.c | 34 +++++++++++++++++++++++++++++++--- src/qemu/qemu_domain.h | 22 +++++++++++++++++++--- src/qemu/qemu_hotplug.c | 6 +++--- 4 files changed, 82 insertions(+), 22 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index e821e58..b56277f 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -625,14 +625,22 @@ qemuBuildGeneralSecinfoURI(virURIPtr uri, if (!secinfo) return 0; - if (secinfo->s.plain.secret) { - if (virAsprintf(&uri->user, "%s:%s", - secinfo->s.plain.username, - secinfo->s.plain.secret) < 0) - return -1; - } else { - if (VIR_STRDUP(uri->user, secinfo->s.plain.username) < 0) - return -1; + switch ((qemuDomainSecretInfoType) secinfo->type) { + case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN: + if (secinfo->s.plain.secret) { + if (virAsprintf(&uri->user, "%s:%s", + secinfo->s.plain.username, + secinfo->s.plain.secret) < 0) + return -1; + } else { + if (VIR_STRDUP(uri->user, secinfo->s.plain.username) < 0) + return -1; + } + break; + + case VIR_DOMAIN_SECRET_INFO_TYPE_IV: + case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: + return -1; } return 0; @@ -659,11 +667,19 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, return 0; } - virBufferEscape(buf, '\\', ":", ":id=%s", - secinfo->s.plain.username); - virBufferEscape(buf, '\\', ":", - ":key=%s:auth_supported=cephx\\;none", - secinfo->s.plain.secret); + switch ((qemuDomainSecretInfoType) secinfo->type) { + case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN: + virBufferEscape(buf, '\\', ":", ":id=%s", + secinfo->s.plain.username); + virBufferEscape(buf, '\\', ":", + ":key=%s:auth_supported=cephx\\;none", + secinfo->s.plain.secret); + break; + + case VIR_DOMAIN_SECRET_INFO_TYPE_IV: + case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: + return -1; + } return 0; } diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 2cc08b8..750d32f 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -737,12 +737,34 @@ qemuDomainSecretPlainFree(qemuDomainSecretPlain secret) static void +qemuDomainSecretIVFree(qemuDomainSecretIV secret) +{ + VIR_FREE(secret.username); + VIR_FREE(secret.alias); + VIR_FREE(secret.iv); + VIR_FREE(secret.ciphertext); +} + + +static void qemuDomainSecretInfoFree(qemuDomainSecretInfoPtr *secinfo) { if (!*secinfo) return; - qemuDomainSecretPlainFree((*secinfo)->s.plain); + switch ((qemuDomainSecretInfoType) (*secinfo)->type) { + case VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN: + qemuDomainSecretPlainFree((*secinfo)->s.plain); + break; + + case VIR_DOMAIN_SECRET_INFO_TYPE_IV: + qemuDomainSecretIVFree((*secinfo)->s.iv); + break; + + case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: + break; + } + VIR_FREE(*secinfo); } @@ -890,6 +912,7 @@ qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) /* qemuDomainSecretDiskPrepare: * @conn: Pointer to connection + * @priv: pointer to domain private object * @disk: Pointer to a disk definition * * For the right disk, generate the qemuDomainSecretInfo structure. @@ -898,6 +921,7 @@ qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) */ int qemuDomainSecretDiskPrepare(virConnectPtr conn, + qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED, virDomainDiskDefPtr disk) { virStorageSourcePtr src = disk->src; @@ -949,6 +973,7 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev) /* qemuDomainSecretHostdevPrepare: * @conn: Pointer to connection + * @priv: pointer to domain private object * @hostdev: Pointer to a hostdev definition * * For the right host device, generate the qemuDomainSecretInfo structure. @@ -957,6 +982,7 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev) */ int qemuDomainSecretHostdevPrepare(virConnectPtr conn, + qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED, virDomainHostdevDefPtr hostdev) { virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; @@ -1029,15 +1055,17 @@ int qemuDomainSecretPrepare(virConnectPtr conn, virDomainObjPtr vm) { + qemuDomainObjPrivatePtr priv = vm->privateData; size_t i; for (i = 0; i < vm->def->ndisks; i++) { - if (qemuDomainSecretDiskPrepare(conn, vm->def->disks[i]) < 0) + if (qemuDomainSecretDiskPrepare(conn, priv, vm->def->disks[i]) < 0) return -1; } for (i = 0; i < vm->def->nhostdevs; i++) { - if (qemuDomainSecretHostdevPrepare(conn, vm->def->hostdevs[i]) < 0) + if (qemuDomainSecretHostdevPrepare(conn, priv, + vm->def->hostdevs[i]) < 0) return -1; } diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 205c47d..42e8520 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -242,6 +242,7 @@ struct _qemuDomainObjPrivate { /* Type of domain secret */ typedef enum { VIR_DOMAIN_SECRET_INFO_TYPE_PLAIN = 0, + VIR_DOMAIN_SECRET_INFO_TYPE_IV, VIR_DOMAIN_SECRET_INFO_TYPE_LAST } qemuDomainSecretInfoType; @@ -253,12 +254,24 @@ struct _qemuDomainSecretPlain { char *secret; }; +# define QEMU_DOMAIN_IV_KEY_LEN 16 /* 16 bytes for 128 bit random */ + /* initialization vector key */ +typedef struct _qemuDomainSecretIV qemuDomainSecretIV; +typedef struct _qemuDomainSecretIV *qemuDomainSecretIVPtr; +struct _qemuDomainSecretIV { + char *username; + char *alias; /* generated alias for secret */ + char *iv; /* base64 encoded initialization vector */ + char *ciphertext; /* encoded/encrypted secret */ +}; + typedef struct _qemuDomainSecretInfo qemuDomainSecretInfo; typedef qemuDomainSecretInfo *qemuDomainSecretInfoPtr; struct _qemuDomainSecretInfo { qemuDomainSecretInfoType type; union { qemuDomainSecretPlain plain; + qemuDomainSecretIV iv; } s; }; @@ -633,15 +646,18 @@ void qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv); void qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) ATTRIBUTE_NONNULL(1); -int qemuDomainSecretDiskPrepare(virConnectPtr conn, virDomainDiskDefPtr disk) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); +int qemuDomainSecretDiskPrepare(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, + virDomainDiskDefPtr disk) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); void qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr disk) ATTRIBUTE_NONNULL(1); int qemuDomainSecretHostdevPrepare(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, virDomainHostdevDefPtr hostdev) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(3); void qemuDomainSecretDestroy(virDomainObjPtr vm) ATTRIBUTE_NONNULL(1); diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 03e5309..73e2a67 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -370,7 +370,7 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; - if (qemuDomainSecretDiskPrepare(conn, disk) < 0) + if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) goto error; if (!(drivestr = qemuBuildDriveStr(disk, false, priv->qemuCaps))) @@ -587,7 +587,7 @@ qemuDomainAttachSCSIDisk(virConnectPtr conn, if (qemuAssignDeviceDiskAlias(vm->def, disk, priv->qemuCaps) < 0) goto error; - if (qemuDomainSecretDiskPrepare(conn, disk) < 0) + if (qemuDomainSecretDiskPrepare(conn, priv, disk) < 0) goto error; if (!(devstr = qemuBuildDriveDevStr(vm->def, disk, 0, priv->qemuCaps))) @@ -1933,7 +1933,7 @@ qemuDomainAttachHostSCSIDevice(virConnectPtr conn, if (qemuAssignDeviceHostdevAlias(vm->def, &hostdev->info->alias, -1) < 0) goto cleanup; - if (qemuDomainSecretHostdevPrepare(conn, hostdev) < 0) + if (qemuDomainSecretHostdevPrepare(conn, priv, hostdev) < 0) goto cleanup; if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, priv->qemuCaps))) -- 2.5.5

On Mon, May 02, 2016 at 05:51:13PM -0400, John Ferlan wrote:
Add the data structure and infrastructure to support an initialization vector (IV) secrets. The IV secret generation will need to have access to the domain private master key, so let's make sure the prepare disk and hostdev functions can accept that now.
Anywhere that needs to make a decision over which secret type to use in order to fill in or use the IV secret has a switch added.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 42 +++++++++++++++++++++++++++++------------- src/qemu/qemu_domain.c | 34 +++++++++++++++++++++++++++++++--- src/qemu/qemu_domain.h | 22 +++++++++++++++++++--- src/qemu/qemu_hotplug.c | 6 +++--- 4 files changed, 82 insertions(+), 22 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

New APIs: qemuDomainGetIVKeyAlias: Generate/return the secret object alias for an initialization vector (IV) secret info type. This will be saved in the secret info block. This will be called from qemuDomainSecretIVSetup. qemuDomainSecretHaveEncrypt: Boolean function to determine whether the underly encryption API is available. This function will utilize a similar mechanism as the 'gnutls_rnd' did in configure.ac. For this patch it just returns false. This API is separate from the following one so that it's possible for the caller to determine whether or not it's possible to create an IV secret before trying and if not available fall back to the plain secret mechanism. qemuDomainSecretIVSetup: (private) This API handles the details of the generation of the IV secret and saves the pieces that need to be passed to qemu in order for the secret to be decrypted. The encrypted secret based upon the domain master key, an initialization vector (16 byte random value), and the stored secret. Finally, the requirement from qemu is the IV and encrypted secret are to be base64 encoded. They can be passed either directly or within a file. This implementation chooses to pass directly rather than a file. qemuDomainSecretSetup: (private) Shim to call either the IV or Plain Setup functions based upon whether IV secrets are possible (we have the encryption API) or not. For this patch, the call will still be to set up the Plain since qemuDomainSecretHaveEncrypt hasn't been enabled yet. Use the qemuDomainSecretSetup in qemuDomainSecretDiskPrepare and qemuDomainSecretHostdevPrepare to add the secret rather than assuming plain. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 23 +++++++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_domain.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 201 insertions(+), 7 deletions(-) diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index ade2033..de9a74f 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -556,3 +556,26 @@ qemuDomainGetMasterKeyAlias(void) return alias; } + + +/* qemuDomainGetIVKeyAlias: + * + * Generate and return an initialization vector alias + * + * Returns NULL or a string containing the IV key alias + */ +char * +qemuDomainGetIVKeyAlias(const char *srcalias) +{ + char *alias; + + if (!srcalias) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("secret iv alias requires valid source alias")); + return NULL; + } + + ignore_value(virAsprintf(&alias, "%s-ivKey0", srcalias)); + + return alias; +} diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index 2d7bc9b..435747e 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -69,4 +69,6 @@ char *qemuAliasFromDisk(const virDomainDiskDef *disk); char *qemuDomainGetMasterKeyAlias(void); +char *qemuDomainGetIVKeyAlias(const char *srcalias); + #endif /* __QEMU_ALIAS_H__*/ diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 750d32f..84be8d9 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -893,6 +893,175 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, } +/* qemuDomainSecretHaveEncypt: + * + * Returns true if we can support the encryption code, false otherwise + */ +static bool +qemuDomainSecretHaveEncrypt(void) +{ + return false; +} + + +/* qemuDomainSecretIVSetup: + * @conn: Pointer to connection + * @priv: pointer to domain private object + * @secinfo: Pointer to secret info + * @srcalias: Alias of the disk/hostdev used to generate the secret alias + * @protocol: Protocol for secret + * @authdef: Pointer to auth data + * + * Taking a secinfo, fill in the initialization vector information + * + * Returns 0 on success, -1 on failure with error message + */ +static int +qemuDomainSecretIVSetup(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, + qemuDomainSecretInfoPtr secinfo, + const char *srcalias, + virStorageNetProtocol protocol, + virStorageAuthDefPtr authdef) +{ + int ret = -1; + int rc; + uint8_t *raw_iv = NULL; + char *secret = NULL; + uint8_t *ciphertext = NULL; + size_t secretlen = 0, ciphertextlen = 0, paddinglen; + int secretType = VIR_SECRET_USAGE_TYPE_ISCSI; + const char *protocolstr = virStorageNetProtocolTypeToString(protocol); + gnutls_cipher_hd_t handle = NULL; + gnutls_datum_t enc_key; + gnutls_datum_t iv_key; + + secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_IV; + if (VIR_STRDUP(secinfo->s.iv.username, authdef->username) < 0) + return -1; + + if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD) + secretType = VIR_SECRET_USAGE_TYPE_CEPH; + + if (!(secinfo->s.iv.alias = qemuDomainGetIVKeyAlias(srcalias))) + return -1; + + /* Create a random initialization vector */ + if (!(raw_iv = qemuDomainGenerateRandomKey(QEMU_DOMAIN_IV_KEY_LEN))) + return -1; + + /* Encode the IV and save that since qemu will need it */ + base64_encode_alloc((const char *)raw_iv, QEMU_DOMAIN_IV_KEY_LEN, + &secinfo->s.iv.iv); + if (!secinfo->s.iv.iv) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to encode initialization vector")); + goto cleanup; + } + + /* Grab the unencoded secret */ + if (!(secret = virSecretGetSecretString(conn, protocolstr, false, + authdef, secretType))) + goto cleanup; + + /* Allocate a padded buffer */ + secretlen = strlen(secret); + paddinglen = 16 - (secretlen % 16); + ciphertextlen = secretlen + paddinglen; + if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0) + goto cleanup; + memcpy(ciphertext, secret, secretlen); + memset(ciphertext + secretlen, paddinglen, paddinglen); + + /* clear secret so that it doesn't hang around */ + memset(secret, 0, strlen(secret)); + + /* Initialize the gnutls cipher */ + enc_key.size = QEMU_DOMAIN_MASTER_KEY_LEN; + enc_key.data = priv->masterKey; + iv_key.size = QEMU_DOMAIN_IV_KEY_LEN; + iv_key.data = raw_iv; + if ((rc = gnutls_cipher_init(&handle, GNUTLS_CIPHER_AES_256_CBC, + &enc_key, &iv_key)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to initialize cipher: '%s'"), + gnutls_strerror(rc)); + goto cleanup; + } + + /* Encrypt the secret and free the memory for cipher operations */ + rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen); + gnutls_cipher_deinit(handle); + if (rc < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("failed to encrypt the secret: '%s'"), + gnutls_strerror(rc)); + goto cleanup; + } + + /* Now base64 encode the ciphertext and store to be passed to qemu */ + base64_encode_alloc((const char *)ciphertext, ciphertextlen, + &secinfo->s.iv.ciphertext); + if (!secinfo->s.iv.ciphertext) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to encode ciphertext")); + goto cleanup; + } + + ret = 0; + + cleanup: + + /* Clear and free the raw_iv, plaintext secret, and ciphertext */ + memset(raw_iv, 0, QEMU_DOMAIN_IV_KEY_LEN); + VIR_FREE(raw_iv); + + memset(secret, 0, secretlen); + VIR_FREE(secret); + + memset(ciphertext, 0, ciphertextlen); + VIR_FREE(ciphertext); + + return ret; +} + + +/* qemuDomainSecretSetup: + * @conn: Pointer to connection + * @priv: pointer to domain private object + * @secinfo: Pointer to secret info + * @srcalias: Alias of the disk/hostdev used to generate the secret alias + * @protocol: Protocol for secret + * @authdef: Pointer to auth data + * + * If we have the encryption API present and can support a secret object, then + * build the IV secret; otherwise, build the Plain secret. This is the magic + * decision point for utilizing the IV secrets for a disk whether it's an + * iSCSI or an RBD disk. + * + * Returns 0 on success, -1 on failure + */ +static int +qemuDomainSecretSetup(virConnectPtr conn, + qemuDomainObjPrivatePtr priv, + qemuDomainSecretInfoPtr secinfo, + const char *srcalias, + virStorageNetProtocol protocol, + virStorageAuthDefPtr authdef) +{ + if (qemuDomainSecretHaveEncrypt() && + virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET)) { + if (qemuDomainSecretIVSetup(conn, priv, secinfo, + srcalias, protocol, authdef) < 0) + return -1; + } else { + if (qemuDomainSecretPlainSetup(conn, secinfo, protocol, authdef) < 0) + return -1; + } + return 0; +} + + /* qemuDomainSecretDiskDestroy: * @disk: Pointer to a disk definition * @@ -921,7 +1090,7 @@ qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) */ int qemuDomainSecretDiskPrepare(virConnectPtr conn, - qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED, + qemuDomainObjPrivatePtr priv, virDomainDiskDefPtr disk) { virStorageSourcePtr src = disk->src; @@ -938,8 +1107,8 @@ qemuDomainSecretDiskPrepare(virConnectPtr conn, if (VIR_ALLOC(secinfo) < 0) return -1; - if (qemuDomainSecretPlainSetup(conn, secinfo, src->protocol, - src->auth) < 0) + if (qemuDomainSecretSetup(conn, priv, secinfo, disk->info.alias, + src->protocol, src->auth) < 0) goto error; diskPriv->secinfo = secinfo; @@ -982,7 +1151,7 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev) */ int qemuDomainSecretHostdevPrepare(virConnectPtr conn, - qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED, + qemuDomainObjPrivatePtr priv, virDomainHostdevDefPtr hostdev) { virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; @@ -1003,9 +1172,9 @@ qemuDomainSecretHostdevPrepare(virConnectPtr conn, if (VIR_ALLOC(secinfo) < 0) return -1; - if (qemuDomainSecretPlainSetup(conn, secinfo, - VIR_STORAGE_NET_PROTOCOL_ISCSI, - iscsisrc->auth) < 0) + if (qemuDomainSecretSetup(conn, priv, secinfo, hostdev->info->alias, + VIR_STORAGE_NET_PROTOCOL_ISCSI, + iscsisrc->auth) < 0) goto error; hostdevPriv->secinfo = secinfo; -- 2.5.5

On Mon, May 02, 2016 at 05:51:14PM -0400, John Ferlan wrote:
New APIs:
qemuDomainGetIVKeyAlias: Generate/return the secret object alias for an initialization vector (IV) secret info type. This will be saved in the secret info block. This will be called from qemuDomainSecretIVSetup.
qemuDomainSecretHaveEncrypt: Boolean function to determine whether the underly encryption API is available. This function will utilize a similar mechanism as the 'gnutls_rnd' did in configure.ac. For this patch it just returns false. This API is separate from the following one so that it's possible for the caller to determine whether or not it's possible to create an IV secret before trying and if not available fall back to the plain secret mechanism.
qemuDomainSecretIVSetup: (private) This API handles the details of the generation of the IV secret and saves the pieces that need to be passed to qemu in order for the secret to be decrypted. The encrypted secret based upon the domain master key, an initialization vector (16 byte random value), and the stored secret. Finally, the requirement from qemu is the IV and encrypted secret are to be base64 encoded. They can be passed either directly or within a file. This implementation chooses to pass directly rather than a file.
qemuDomainSecretSetup: (private) Shim to call either the IV or Plain Setup functions based upon whether IV secrets are possible (we have the encryption API) or not. For this patch, the call will still be to set up the Plain since qemuDomainSecretHaveEncrypt hasn't been enabled yet.
Use the qemuDomainSecretSetup in qemuDomainSecretDiskPrepare and qemuDomainSecretHostdevPrepare to add the secret rather than assuming plain.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 23 +++++++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_domain.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 201 insertions(+), 7 deletions(-)
ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

https://bugzilla.redhat.com/show_bug.cgi?id=1182074 If they're available and we need to pass secrets to qemu, then use the qemu domain secret object in order to pass the secrets for iSCSI and RBD volumes instead of passing plaintext or base64 encoded secrets on the command line. Adjust the qemuDomainSecretHaveEncrypt API in order to check for the HAVE_GNUTLS_CIPHER_ENCRYPT being set as the primary decision point to whether an IV secret can be attempted and fall back to plain secret if the API is not available. The goal is to make IV secrets the default and have no user interaction required in order to allow using the IV mechanism. If the mechanism is not available, then fall back to the current mechanism. New API's: qemuBuildSecretInfoProps: (private) Generate/return a JSON properties object for the IV secret to be used by both the command building and eventually the hotplug code in order to add the secret object. Code was designed so that in the future perhaps hotplug could use it if it made sense. qemuBuildSecretIVCommandLine (private) Generate and add to the command line the -object secret for the IV secret. This will be required for the subsequent iSCSI or RBD reference to the object. qemuBuildiSCSICommandLine: (private) Required for iSCSI since qemu only processes the "user=" and "password-secret=" options for an "-iscsi" entry. At some point in a future release, qemu may support those options on the -drive command line for iscsi devices. The one caveat to this code is rather than provide an 'id=' field for the -iscsi command, use the "initiator-name=" argument since it doesn't have the same restrictions regarding characters. The initiator-name is described as taking an IQN, which is the path argument. qemuBuildDiskSecinfoCommandLine qemuBuildHostdevSecretCommandLine These API's will handle adding the IV secret object and if necessary the '-iscsi' command line option. For an RBD disk, only the IV secret object will be required. Command Building: Adjust the qemuBuild{General|RBD}SecinfoURI API's in order to generate the specific command options for an IV secret, such as: For iSCSI: -object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted, format=base64 -iscsi -initiator-name=$iqn,user=user,password-secret=$alias -drive file=iscsi://example.com/$iqn,... For RBD: -object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted, format=base64 -drive file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\ mon_host=mon1.example.org\:6321,password-secret=$alias,... where for both 'id=' value is the secret object alias generated by concatenating the disk/hostdev alias and "-ivKey0". The 'keyid= $masterKey' is the master key shared with qemu, and the -drive syntax will reference that alias as the 'password-secret'. For the iSCSI object 'user=' replaces the URI generated 'user:secret@' prepended to the iSCSI 'host' name (example.com). For the RBD -drive syntax, the 'id=myname' is kept to define the username, while the 'key=$base64 encoded secret' is removed. While according to the syntax described for qemu commits 'b189346eb' (iSCSI) and '60390a21' (RBD) or as seen in the email archive: https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04083.html it is possible to pass a plaintext password via a file, the qemu commit 'ac1d8878' describes the more feature rich 'keyid=' option based upon the shared masterKey. Tests: Add mock's for virRandomBytes and gnutls_rnd in order to return a constant stream of '0xff' in the bytes for a non random key in order to generate "constant" values for the secrets so that the tests can use those results to compare results. Hotplug: Since the hotplug code doesn't add command line arguments, passing the encoded/plaintext secrets directly to the monitor will suffice. Besides, it's passing the IV secret via '-iscsi' won't be possible. Perhaps when the -drive command is modified to accept not only the initiator-name, but -user and -password-secret arguments, then the IV code can be utilized for hotplug secrets. Signed-off-by: John Ferlan <jferlan@redhat.com> --- configure.ac | 1 + src/qemu/qemu_command.c | 257 ++++++++++++++++++++- src/qemu/qemu_domain.c | 4 + ...uxml2argv-disk-drive-network-iscsi-auth-IV.args | 39 ++++ ...muxml2argv-disk-drive-network-iscsi-auth-IV.xml | 43 ++++ ...emuxml2argv-disk-drive-network-rbd-auth-IV.args | 31 +++ ...qemuxml2argv-disk-drive-network-rbd-auth-IV.xml | 42 ++++ ...emuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.args | 41 ++++ ...qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.xml | 48 ++++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-IV.args | 43 ++++ ...uxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.xml | 48 ++++ tests/qemuxml2argvmock.c | 31 ++- tests/qemuxml2argvtest.c | 19 ++ 13 files changed, 643 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.xml diff --git a/configure.ac b/configure.ac index 88e2e20..3cabd5e 100644 --- a/configure.ac +++ b/configure.ac @@ -1264,6 +1264,7 @@ if test "x$with_gnutls" != "xno"; then ]]) AC_CHECK_FUNCS([gnutls_rnd]) + AC_CHECK_FUNCS([gnutls_cipher_encrypt]) CFLAGS="$old_CFLAGS" LIBS="$old_LIBS" diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b56277f..27e31ec 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -607,9 +607,227 @@ qemuNetworkDriveGetPort(int protocol, } +/** + * qemuBuildSecretInfoProps: + * @secinfo: pointer to the secret info object + * @type: returns a pointer to a character string for object name + * @props: json properties to return + * + * Build the JSON properties for the secret info type. + * + * Returns 0 on success with the filled in JSON property; otherwise, + * returns -1 on failure error message set. + */ +static int +qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, + const char **type, + virJSONValuePtr *propsret) +{ + int ret = -1; + char *keyid = NULL; + virJSONValuePtr props = NULL; + + *type = "secret"; + + if (!(keyid = qemuDomainGetMasterKeyAlias())) + return -1; + + if (!(props = virJSONValueNewObject())) + goto cleanup; + + if (virJSONValueObjectAdd(props, + "s:data", secinfo->s.iv.ciphertext, + "s:keyid", keyid, + "s:iv", secinfo->s.iv.iv, + "s:format", "base64", NULL) < 0) + goto cleanup; + + *propsret = props; + props = NULL; + ret = 0; + + cleanup: + VIR_FREE(keyid); + virJSONValueFree(props); + + return ret; +} + + +/** + * qemuBuildSecretIVCommandLine: + * @cmd: the command to modify + * @secinfo: pointer to the secret info object + * + * If the secinfo is available and associated with an IV secret, + * then format the command line for the secret object. This object + * will be referenced by the device that needs/uses it, so it needs + * to be in place first. + * + * Returns 0 on success, -1 w/ error message on failure + */ +static int +qemuBuildSecretIVCommandLine(virCommandPtr cmd, + qemuDomainSecretInfoPtr secinfo) +{ + int ret = -1; + virJSONValuePtr props = NULL; + const char *type; + char *tmp = NULL; + + if (qemuBuildSecretInfoProps(secinfo, &type, &props) < 0) + return -1; + + if (!(tmp = qemuBuildObjectCommandlineFromJSON(type, secinfo->s.iv.alias, + props))) + goto cleanup; + + virCommandAddArgList(cmd, "-object", tmp, NULL); + ret = 0; + + cleanup: + virJSONValueFree(props); + VIR_FREE(tmp); + + return ret; +} + + +/* qemuBuildiSCSICommandLine: + * @cmd: Pointer to the command string + * @src: Pointer to a possible disk source + * @secinfo: Pointer to a possible secinfo + * + * Add an "-iscsi" argument for the command line. This provides the + * capability to add just an "iqn." string to the command line in order + * to override the default IQN (iqn.2008-11.org.linux-kvm) for qemu. + * Additionally, this will be the mechanism used to pass the IV secret + * to qemu since (currently - as of 2.6), the "user=" and "password-secret=" + * arguments are only handled with the "-iscsi" argument. + * + * RFC 3720 indicates that the IQN is made up of at least one part - a + * string starting with "iqn." which describes the naming authority. The + * standard references an optional field after a colon (:) is naming + * authority specific. For libvirt XML, an iSCSI name string typically + * has both the "iqn." and the colon along with a '/#' field denoting the + * iSCSI LUN to use, e.g.: + * + * iqn.2016-04.com.example:storage/1 + * + * We can pass this entire iqn string in the "initiator-name=" option + * of the "-iscsi" switch. As it turns out the "id=" option is not + * required, so no need to break our string. Although it would be + * possible to split at the colon, passing the "iqn." as just the + * "initiator-name=" option and adjusting the "storage/1" to "storage_1" + * and passing in the "id=" option. This second option has limitations + * since the "id=" value must be letters, digits, '-', '.', and "_" + * starting with a letter. + * + * Returns 0 on success or if not necessary, -1 on some sort of failure + */ +static int +qemuBuildiSCSICommandLine(virCommandPtr cmd, + const char *path, + qemuDomainSecretInfoPtr secinfo) +{ + virBuffer opt = VIR_BUFFER_INITIALIZER; + char *bufstr; + + virBufferAsprintf(&opt, "initiator-name=%s", path); + + /* If we have a secinfo, then append on the user/password-secret as + * this will be something for an iSCSI disk using IV secrets */ + if (secinfo) { + virBufferAsprintf(&opt, ",user=%s,password-secret=%s", + secinfo->s.iv.username, + secinfo->s.iv.alias); + } + + if (virBufferCheckError(&opt) < 0) { + virBufferFreeAndReset(&opt); + return -1; + } + + bufstr = virBufferContentAndReset(&opt); + virCommandAddArg(cmd, "-iscsi"); + virCommandAddArg(cmd, bufstr); + VIR_FREE(bufstr); + + return 0; +} + + +/* qemuBuildDiskSecinfoCommandLine: + * @cmd: Pointer to the command string + * @secinfo: Pointer to a possible secinfo + * @src: Pointer to a possible disk source + * + * Adding an IV secret to the command line for an iSCSI disk is a multistep + * and complicated endeavor. It is not possible to simply append "user=name, + * password-secret=alias-id" to a "-drive file=iscsi://..." command. Instead, + * one must first add an -iscsi option with the IQN data in order to utilize + * the "user" and "password-secret" options. + * + * This function will check the storage source to see if it requires + * an '-iscsi' argument and make the call to do. + * + * Returns 0 on success, -1 w/ error on some sort of failure. + */ +static int +qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd, + qemuDomainSecretInfoPtr secinfo, + virStorageSourcePtr src) +{ + /* Not necessary for non IV secrets */ + if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_IV) + return 0; + + /* For a disk this will be necessary for both iSCSI and RBD */ + if (qemuBuildSecretIVCommandLine(cmd, secinfo) < 0) + return -1; + + /* The rest is only necessary for networked, iSCSI disks because + * the qemu parsing logic parses the user and password-secret + * using the -iscsi command argument. + */ + if (virStorageSourceIsEmpty(src) || + src->protocol != VIR_STORAGE_NET_PROTOCOL_ISCSI) + return 0; + + return qemuBuildiSCSICommandLine(cmd, src->path, secinfo); +} + + +/* qemuBuildHostdevSecretCommandLine: + * @cmd: Pointer to the command string + * @hostdev: Pointer to a host device + * @secinfo: Pointer to a possible secinfo + * + * See qemuBuildDiskSecinfoCommandLine for details. + */ +static int +qemuBuildHostdevSecretCommandLine(virCommandPtr cmd, + qemuDomainSecretInfoPtr secinfo, + virDomainHostdevDefPtr hostdev) +{ + virDomainHostdevSubsysSCSIPtr scsisrc = &hostdev->source.subsys.u.scsi; + virDomainHostdevSubsysSCSIiSCSIPtr iscsisrc = &scsisrc->u.iscsi; + + /* Only necessary for hostdev's using the IV secret */ + if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_IV) + return 0; + + if (qemuBuildSecretIVCommandLine(cmd, secinfo) < 0) + return -1; + + return qemuBuildiSCSICommandLine(cmd, iscsisrc->path, secinfo); +} + + /* qemuBuildGeneralSecinfoURI: * @uri: Pointer to the URI structure to add to * @secinfo: Pointer to the secret info data (if present) + * @protocol: Protocol for the source device * * If we have a secinfo, then build the command line options for * the secret info for the "general" case (somewhat a misnomer since @@ -620,7 +838,8 @@ qemuNetworkDriveGetPort(int protocol, */ static int qemuBuildGeneralSecinfoURI(virURIPtr uri, - qemuDomainSecretInfoPtr secinfo) + qemuDomainSecretInfoPtr secinfo, + int protocol) { if (!secinfo) return 0; @@ -639,6 +858,13 @@ qemuBuildGeneralSecinfoURI(virURIPtr uri, break; case VIR_DOMAIN_SECRET_INFO_TYPE_IV: + /* iSCSI has a completely different way to handle this + * see qemuBuildiSCSICommandLine */ + if (protocol != VIR_STORAGE_NET_PROTOCOL_ISCSI && + VIR_STRDUP(uri->user, secinfo->s.iv.username) < 0) + return -1; + break; + case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: return -1; } @@ -677,6 +903,10 @@ qemuBuildRBDSecinfoURI(virBufferPtr buf, break; case VIR_DOMAIN_SECRET_INFO_TYPE_IV: + virBufferEscape(buf, '\\', ":", ":id=%s:auth_supported=cephx\\;none", + secinfo->s.iv.username); + break; + case VIR_DOMAIN_SECRET_INFO_TYPE_LAST: return -1; } @@ -801,7 +1031,7 @@ qemuBuildNetworkDriveURI(virStorageSourcePtr src, virAsprintf(&uri->query, "socket=%s", src->hosts->socket) < 0) goto cleanup; - if (qemuBuildGeneralSecinfoURI(uri, secinfo) < 0) + if (qemuBuildGeneralSecinfoURI(uri, secinfo, src->protocol) < 0) goto cleanup; if (VIR_STRDUP(uri->server, src->hosts->name) < 0) @@ -1083,6 +1313,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, char *source = NULL; int actualType = virStorageSourceGetActualType(disk->src); qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; if (idx < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -1164,7 +1395,7 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, break; } - if (qemuGetDriveSourceString(disk->src, diskPriv->secinfo, &source) < 0) + if (qemuGetDriveSourceString(disk->src, secinfo, &source) < 0) goto error; if (source && @@ -1215,6 +1446,11 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk, virBufferEscape(&opt, ',', ",", "%s,", source); + if (disk->src->protocol == VIR_STORAGE_NET_PROTOCOL_RBD && + secinfo && secinfo->type == VIR_DOMAIN_SECRET_INFO_TYPE_IV) { + virBufferAsprintf(&opt, "password-secret=%s,", secinfo->s.iv.alias); + } + if (disk->src->format > 0 && disk->src->type != VIR_STORAGE_TYPE_DIR) virBufferAsprintf(&opt, "format=%s,", @@ -1888,6 +2124,8 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, virDomainDiskDefPtr disk = def->disks[i]; bool withDeviceArg = false; bool deviceFlagMasked = false; + qemuDomainDiskPrivatePtr diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); + qemuDomainSecretInfoPtr secinfo = diskPriv->secinfo; /* Unless we have -device, then USB disks need special handling */ @@ -1930,6 +2168,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, break; } + if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo, disk->src) < 0) + return -1; + virCommandAddArg(cmd, "-drive"); /* Unfortunately it is not possible to use @@ -4553,7 +4794,9 @@ qemuBuildSCSIHostdevDrvStr(virDomainHostdevDefPtr dev, if (scsisrc->protocol == VIR_DOMAIN_HOSTDEV_SCSI_PROTOCOL_TYPE_ISCSI) { if (!(source = qemuBuildSCSIiSCSIHostdevDrvStr(dev))) goto error; + virBufferAsprintf(&buf, "file=%s,if=none,format=raw", source); + } else { if (!(source = qemuBuildSCSIHostHostdevDrvStr(dev))) goto error; @@ -4994,8 +5237,16 @@ qemuBuildHostdevCommandLine(virCommandPtr cmd, subsys->type == VIR_DOMAIN_HOSTDEV_SUBSYS_TYPE_SCSI) { if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE) && virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_SCSI_GENERIC)) { + + qemuDomainHostdevPrivatePtr hostdevPriv = + QEMU_DOMAIN_HOSTDEV_PRIVATE(hostdev); + qemuDomainSecretInfoPtr secinfo = hostdevPriv->secinfo; char *drvstr; + if (qemuBuildHostdevSecretCommandLine(cmd, secinfo, + hostdev) < 0) + return -1; + virCommandAddArg(cmd, "-drive"); if (!(drvstr = qemuBuildSCSIHostdevDrvStr(hostdev, qemuCaps))) return -1; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 84be8d9..64f9c61 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -900,7 +900,11 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, static bool qemuDomainSecretHaveEncrypt(void) { +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT + return true; +#else return false; +#endif } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.args new file mode 100644 index 0000000..bce8d2f --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.args @@ -0,0 +1,39 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-object secret,id=virtio-disk0-ivKey0,\ +data=/i1QO1S81K4UELoLXmtCNQzxOaE1Tc4DvecFBfyvFKKWUAawbjGD+yZaz9oyEcnW,\ +keyid=masterKey0,iv=/////////////////////w==,format=base64 \ +-iscsi initiator-name=iqn.1992-01.com.example:storage/1,user=myname,\ +password-secret=virtio-disk0-ivKey0 \ +-drive file=iscsi://example.org:6000/iqn.1992-01.com.example%3Astorage/1,\ +format=raw,if=none,id=drive-virtio-disk0 \ +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0 \ +-object secret,id=virtio-disk1-ivKey0,\ +data=/i1QO1S81K4UELoLXmtCNQzxOaE1Tc4DvecFBfyvFKKWUAawbjGD+yZaz9oyEcnW,\ +keyid=masterKey0,iv=/////////////////////w==,format=base64 \ +-iscsi initiator-name=iqn.1992-01.com.example:storage/2,user=myname,\ +password-secret=virtio-disk1-ivKey0 \ +-drive file=iscsi://example.org:6000/iqn.1992-01.com.example%3Astorage/2,\ +format=raw,if=none,id=drive-virtio-disk1 \ +-device virtio-blk-pci,bus=pci.0,addr=0x4,drive=drive-virtio-disk1,\ +id=virtio-disk1 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.xml new file mode 100644 index 0000000..1f80d3b --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.xml @@ -0,0 +1,43 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='6000'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> + <host name='example.org' port='6000'/> + </source> + <target dev='vdb' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args new file mode 100644 index 0000000..f6c0906 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args @@ -0,0 +1,31 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-object secret,id=virtio-disk0-ivKey0,\ +data=/i1QO1S81K4UELoLXmtCNQzxOaE1Tc4DvecFBfyvFKKWUAawbjGD+yZaz9oyEcnW,\ +keyid=masterKey0,iv=/////////////////////w==,format=base64 \ +-drive 'file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\ +mon_host=mon1.example.org\:6321\;mon2.example.org\:6322\;mon3.example.org\:6322,\ +password-secret=virtio-disk0-ivKey0,format=raw,if=none,id=drive-virtio-disk0' \ +-device virtio-blk-pci,bus=pci.0,addr=0x3,drive=drive-virtio-disk0,\ +id=virtio-disk0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml new file mode 100644 index 0000000..ac2e942 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml @@ -0,0 +1,42 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='network' device='disk'> + <driver name='qemu' type='raw'/> + <auth username='myname'> + <secret type='ceph' usage='mycluster_myname'/> + </auth> + <source protocol='rbd' name='pool/image'> + <host name='mon1.example.org' port='6321'/> + <host name='mon2.example.org' port='6322'/> + <host name='mon3.example.org' port='6322'/> + </source> + <target dev='vda' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.args new file mode 100644 index 0000000..6b86aef --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.args @@ -0,0 +1,41 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest2 \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9466-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest2/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-device lsi,id=scsi0,bus=pci.0,addr=0x3 \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-object secret,id=hostdev0-ivKey0,\ +data=/i1QO1S81K4UELoLXmtCNQzxOaE1Tc4DvecFBfyvFKKWUAawbjGD+yZaz9oyEcnW,\ +keyid=masterKey0,iv=/////////////////////w==,format=base64 \ +-iscsi initiator-name=iqn.1992-01.com.example:storage/1,user=myname,\ +password-secret=hostdev0-ivKey0 \ +-drive file=iscsi://example.org:3260/iqn.1992-01.com.example%3Astorage/1,if=none,\ +format=raw,id=drive-hostdev0 \ +-device scsi-generic,bus=scsi0.0,scsi-id=4,drive=drive-hostdev0,id=hostdev0 \ +-object secret,id=hostdev1-ivKey0,\ +data=/i1QO1S81K4UELoLXmtCNQzxOaE1Tc4DvecFBfyvFKKWUAawbjGD+yZaz9oyEcnW,\ +keyid=masterKey0,iv=/////////////////////w==,format=base64 \ +-iscsi initiator-name=iqn.1992-01.com.example:storage/2,user=myname,\ +password-secret=hostdev1-ivKey0 \ +-drive file=iscsi://example.org:3260/iqn.1992-01.com.example%3Astorage/2,if=none,\ +format=raw,id=drive-hostdev1 \ +-device scsi-generic,bus=scsi0.0,scsi-id=5,drive=drive-hostdev1,id=hostdev1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.xml new file mode 100644 index 0000000..f988165 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='3260'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='4'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> + <host name='example.org' port='3260'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <address type='drive' controller='0' bus='0' target='0' unit='5'/> + </hostdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.args b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.args new file mode 100644 index 0000000..4ecbe53 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.args @@ -0,0 +1,43 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest2 \ +-S \ +-object secret,id=masterKey0,format=raw,\ +file=/tmp/lib/domain--1-QEMUGuest2/master-key.aes \ +-M pc \ +-m 214 \ +-smp 1 \ +-uuid c7a5fdbd-edaf-9466-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest2/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-device virtio-scsi-pci,id=scsi0,bus=pci.0,addr=0x3 \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest2,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-object secret,id=hostdev0-ivKey0,\ +data=/i1QO1S81K4UELoLXmtCNQzxOaE1Tc4DvecFBfyvFKKWUAawbjGD+yZaz9oyEcnW,\ +keyid=masterKey0,iv=/////////////////////w==,format=base64 \ +-iscsi initiator-name=iqn.1992-01.com.example:storage/1,user=myname,\ +password-secret=hostdev0-ivKey0 \ +-drive file=iscsi://example.org:3260/iqn.1992-01.com.example%3Astorage/1,if=none,\ +format=raw,id=drive-hostdev0 \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=4,drive=drive-hostdev0,\ +id=hostdev0 \ +-object secret,id=hostdev1-ivKey0,\ +data=/i1QO1S81K4UELoLXmtCNQzxOaE1Tc4DvecFBfyvFKKWUAawbjGD+yZaz9oyEcnW,\ +keyid=masterKey0,iv=/////////////////////w==,format=base64 \ +-iscsi initiator-name=iqn.1992-01.com.example:storage/2,user=myname,\ +password-secret=hostdev1-ivKey0 \ +-drive file=iscsi://example.org:3260/iqn.1992-01.com.example%3Astorage/2,if=none,\ +format=raw,id=drive-hostdev1 \ +-device scsi-generic,bus=scsi0.0,channel=0,scsi-id=2,lun=5,drive=drive-hostdev1,\ +id=hostdev1 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.xml b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.xml new file mode 100644 index 0000000..b70b84e --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.xml @@ -0,0 +1,48 @@ +<domain type='qemu'> + <name>QEMUGuest2</name> + <uuid>c7a5fdbd-edaf-9466-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='scsi' index='0' model='virtio-scsi'/> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/1'> + <host name='example.org' port='3260'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <address type='drive' controller='0' bus='0' target='2' unit='4'/> + </hostdev> + <hostdev mode='subsystem' type='scsi' managed='yes'> + <source protocol='iscsi' name='iqn.1992-01.com.example:storage/2'> + <host name='example.org' port='3260'/> + <auth username='myname'> + <secret type='iscsi' usage='mycluster_myname'/> + </auth> + </source> + <address type='drive' controller='0' bus='0' target='2' unit='5'/> + </hostdev> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvmock.c b/tests/qemuxml2argvmock.c index 1616eed..2bfbf6b 100644 --- a/tests/qemuxml2argvmock.c +++ b/tests/qemuxml2argvmock.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2014 Red Hat, Inc. + * Copyright (C) 2014-2016 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -30,6 +30,13 @@ #include "virstring.h" #include "virtpm.h" #include "virutil.h" +#include "virrandom.h" +#ifdef WITH_GNUTLS +# include <gnutls/gnutls.h> +# if HAVE_GNUTLS_CRYPTO_H +# include <gnutls/crypto.h> +# endif +#endif #include <time.h> #include <unistd.h> @@ -145,3 +152,25 @@ virCommandPassFD(virCommandPtr cmd ATTRIBUTE_UNUSED, { /* nada */ } + +int +virRandomBytes(unsigned char *buf, + size_t buflen) +{ + size_t i; + + for (i = 0; i < buflen; i++) + buf[i] = 0xff; + + return 0; +} + +#ifdef WITH_GNUTLS +int +gnutls_rnd(gnutls_rnd_level_t level ATTRIBUTE_UNUSED, + void *data, + size_t len) +{ + return virRandomBytes(data, len); +#endif +} diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1d13c05..2b8adba 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -26,6 +26,7 @@ # include "virstring.h" # include "storage/storage_driver.h" # include "virmock.h" +# include "virrandom.h" # include "testutilsqemu.h" @@ -330,6 +331,14 @@ static int testCompareXMLToArgvFiles(const char *xml, } } + /* Create a fake master key */ + if (VIR_ALLOC_N(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0) + goto out; + + if (virRandomBytes(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN) < 0) + goto out; + priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN; + if (!(cmd = qemuProcessCreatePretendCmd(conn, &driver, vm, migrateURI, (flags & FLAG_FIPS), false, VIR_QEMU_PROCESS_START_COLD))) { @@ -769,6 +778,8 @@ mymain(void) DO_TEST("disk-drive-network-nbd-unix", NONE); DO_TEST("disk-drive-network-iscsi", NONE); DO_TEST("disk-drive-network-iscsi-auth", NONE); + DO_TEST("disk-drive-network-iscsi-auth-IV", + QEMU_CAPS_OBJECT_SECRET); DO_TEST("disk-drive-network-iscsi-lun", QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_BLK_SG_IO, QEMU_CAPS_SCSI_BLOCK); @@ -776,6 +787,8 @@ mymain(void) DO_TEST("disk-drive-network-rbd", NONE); DO_TEST("disk-drive-network-sheepdog", NONE); DO_TEST("disk-drive-network-rbd-auth", NONE); + DO_TEST("disk-drive-network-rbd-auth-IV", + QEMU_CAPS_OBJECT_SECRET); DO_TEST("disk-drive-network-rbd-ipv6", NONE); DO_TEST_FAILURE("disk-drive-network-rbd-no-colon", NONE); DO_TEST("disk-drive-no-boot", @@ -1615,12 +1628,18 @@ mymain(void) DO_TEST("hostdev-scsi-lsi-iscsi-auth", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_LSI, QEMU_CAPS_DEVICE_SCSI_GENERIC); + DO_TEST("hostdev-scsi-lsi-iscsi-auth-IV", + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_SCSI_LSI, + QEMU_CAPS_DEVICE_SCSI_GENERIC, QEMU_CAPS_OBJECT_SECRET); DO_TEST("hostdev-scsi-virtio-iscsi", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC); DO_TEST("hostdev-scsi-virtio-iscsi-auth", QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_DEVICE_SCSI_GENERIC); + DO_TEST("hostdev-scsi-virtio-iscsi-auth-IV", + QEMU_CAPS_VIRTIO_SCSI, QEMU_CAPS_VIRTIO_SCSI, + QEMU_CAPS_DEVICE_SCSI_GENERIC, QEMU_CAPS_OBJECT_SECRET); DO_TEST("mlock-on", QEMU_CAPS_MLOCK); DO_TEST_FAILURE("mlock-on", NONE); -- 2.5.5

On 02.05.2016 23:51, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1182074
If they're available and we need to pass secrets to qemu, then use the qemu domain secret object in order to pass the secrets for iSCSI and RBD volumes instead of passing plaintext or base64 encoded secrets on the command line.
Adjust the qemuDomainSecretHaveEncrypt API in order to check for the HAVE_GNUTLS_CIPHER_ENCRYPT being set as the primary decision point to whether an IV secret can be attempted and fall back to plain secret if the API is not available.
The goal is to make IV secrets the default and have no user interaction required in order to allow using the IV mechanism. If the mechanism is not available, then fall back to the current mechanism.
New API's: qemuBuildSecretInfoProps: (private) Generate/return a JSON properties object for the IV secret to be used by both the command building and eventually the hotplug code in order to add the secret object. Code was designed so that in the future perhaps hotplug could use it if it made sense.
qemuBuildSecretIVCommandLine (private) Generate and add to the command line the -object secret for the IV secret. This will be required for the subsequent iSCSI or RBD reference to the object.
qemuBuildiSCSICommandLine: (private) Required for iSCSI since qemu only processes the "user=" and "password-secret=" options for an "-iscsi" entry. At some point in a future release, qemu may support those options on the -drive command line for iscsi devices. The one caveat to this code is rather than provide an 'id=' field for the -iscsi command, use the "initiator-name=" argument since it doesn't have the same restrictions regarding characters. The initiator-name is described as taking an IQN, which is the path argument.
qemuBuildDiskSecinfoCommandLine qemuBuildHostdevSecretCommandLine These API's will handle adding the IV secret object and if necessary the '-iscsi' command line option. For an RBD disk, only the IV secret object will be required.
Command Building:
Adjust the qemuBuild{General|RBD}SecinfoURI API's in order to generate the specific command options for an IV secret, such as:
For iSCSI:
-object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted, format=base64 -iscsi -initiator-name=$iqn,user=user,password-secret=$alias -drive file=iscsi://example.com/$iqn,...
For RBD:
-object secret,id=$alias,keyid=$masterKey,data=$base64encodedencrypted, format=base64 -drive file=rbd:pool/image:id=myname:auth_supported=cephx\;none:\ mon_host=mon1.example.org\:6321,password-secret=$alias,...
where for both 'id=' value is the secret object alias generated by concatenating the disk/hostdev alias and "-ivKey0". The 'keyid= $masterKey' is the master key shared with qemu, and the -drive syntax will reference that alias as the 'password-secret'. For the iSCSI object 'user=' replaces the URI generated 'user:secret@' prepended to the iSCSI 'host' name (example.com). For the RBD -drive syntax, the 'id=myname' is kept to define the username, while the 'key=$base64 encoded secret' is removed.
While according to the syntax described for qemu commits 'b189346eb' (iSCSI) and '60390a21' (RBD) or as seen in the email archive:
https://lists.gnu.org/archive/html/qemu-devel/2016-01/msg04083.html
it is possible to pass a plaintext password via a file, the qemu commit 'ac1d8878' describes the more feature rich 'keyid=' option based upon the shared masterKey.
Tests:
Add mock's for virRandomBytes and gnutls_rnd in order to return a constant stream of '0xff' in the bytes for a non random key in order to generate "constant" values for the secrets so that the tests can use those results to compare results.
Hotplug:
Since the hotplug code doesn't add command line arguments, passing the encoded/plaintext secrets directly to the monitor will suffice. Besides, it's passing the IV secret via '-iscsi' won't be possible. Perhaps when the -drive command is modified to accept not only the initiator-name, but -user and -password-secret arguments, then the IV code can be utilized for hotplug secrets.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- configure.ac | 1 + src/qemu/qemu_command.c | 257 ++++++++++++++++++++- src/qemu/qemu_domain.c | 4 + ...uxml2argv-disk-drive-network-iscsi-auth-IV.args | 39 ++++ ...muxml2argv-disk-drive-network-iscsi-auth-IV.xml | 43 ++++ ...emuxml2argv-disk-drive-network-rbd-auth-IV.args | 31 +++ ...qemuxml2argv-disk-drive-network-rbd-auth-IV.xml | 42 ++++ ...emuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.args | 41 ++++ ...qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.xml | 48 ++++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-IV.args | 43 ++++ ...uxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.xml | 48 ++++ tests/qemuxml2argvmock.c | 31 ++- tests/qemuxml2argvtest.c | 19 ++ 13 files changed, 643 insertions(+), 4 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.xml
diff --git a/configure.ac b/configure.ac index 88e2e20..3cabd5e 100644 --- a/configure.ac +++ b/configure.ac @@ -1264,6 +1264,7 @@ if test "x$with_gnutls" != "xno"; then ]])
AC_CHECK_FUNCS([gnutls_rnd]) + AC_CHECK_FUNCS([gnutls_cipher_encrypt])
This change should go into the previous commit since it's that one who uses it.
CFLAGS="$old_CFLAGS" LIBS="$old_LIBS" diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b56277f..27e31ec 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -607,9 +607,227 @@ qemuNetworkDriveGetPort(int protocol, }
Michal

[...]
diff --git a/configure.ac b/configure.ac index 88e2e20..3cabd5e 100644 --- a/configure.ac +++ b/configure.ac @@ -1264,6 +1264,7 @@ if test "x$with_gnutls" != "xno"; then ]])
AC_CHECK_FUNCS([gnutls_rnd]) + AC_CHECK_FUNCS([gnutls_cipher_encrypt])
This change should go into the previous commit since it's that one who uses it.
Putting this in patch 7, means qemuDomainSecretHaveEncrypt will be active and we "could" try to add an IV secret with no teeth. As I noted in my cover - 7 and 8 were split to make for a smaller pile of code to review. I could combine them. FWIW: I tried to "separate" things a bit more for the "last" patch. In doing so I found that having qemuDomainSecretHaveEncrypt and qemuDomainSecretIVSetup in patch 7 as static and "unused" (if I didn't make the modification to add qemuDomainSecretSetup) would cause a build error. I had forgotten about using the "ATTRIBUTE_UNUSED" trick in order to avoid the build error. If I went back and took this approach, then qemuDomainSecretSetup gets introduced in patch 8. John
CFLAGS="$old_CFLAGS" LIBS="$old_LIBS" diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index b56277f..27e31ec 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -607,9 +607,227 @@ qemuNetworkDriveGetPort(int protocol, }
Michal

On Mon, May 02, 2016 at 05:51:15PM -0400, John Ferlan wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1182074
If they're available and we need to pass secrets to qemu, then use the qemu domain secret object in order to pass the secrets for iSCSI and RBD volumes instead of passing plaintext or base64 encoded secrets on the command line.
Adjust the qemuDomainSecretHaveEncrypt API in order to check for the HAVE_GNUTLS_CIPHER_ENCRYPT being set as the primary decision point to whether an IV secret can be attempted and fall back to plain secret if the API is not available.
The goal is to make IV secrets the default and have no user interaction required in order to allow using the IV mechanism. If the mechanism is not available, then fall back to the current mechanism.
New API's: qemuBuildSecretInfoProps: (private) Generate/return a JSON properties object for the IV secret to be used by both the command building and eventually the hotplug code in order to add the secret object. Code was designed so that in the future perhaps hotplug could use it if it made sense.
qemuBuildSecretIVCommandLine (private) Generate and add to the command line the -object secret for the IV secret. This will be required for the subsequent iSCSI or RBD reference to the object.
qemuBuildiSCSICommandLine: (private) Required for iSCSI since qemu only processes the "user=" and "password-secret=" options for an "-iscsi" entry. At some point in a future release, qemu may support those options on the -drive command line for iscsi devices. The one caveat to this code is rather than provide an 'id=' field for the -iscsi command, use the "initiator-name=" argument since it doesn't have the same restrictions regarding characters. The initiator-name is described as taking an IQN, which is the path argument.
I'm not actually convinced this is doing what you think it is. AFAICT from reading the QEMU code, the only way to associated the -drive and -iscsi options is via the 'id' parameter of the -iscsi arg matching the IQN of the -drive option. The 'initiator-name' does not appear to have any effect on handling. If 'id' doesn't match, then QEMU just always falls back to finding the first -iscsi arg. So IIUC with your patch here all -drive options are just going to use the first -iscsi arg, which will be wrong when we are connecting to several different iscsi servers. I'm not really seeing any viable way to deal with this problem, so I think we might need to wait for QEMU 2.7 before we can use secrets with iSCSI :-( Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

qemuBuildiSCSICommandLine: (private) Required for iSCSI since qemu only processes the "user=" and "password-secret=" options for an "-iscsi" entry. At some point in a future release, qemu may support those options on the -drive command line for iscsi devices. The one caveat to this code is rather than provide an 'id=' field for the -iscsi command, use the "initiator-name=" argument since it doesn't have the same restrictions regarding characters. The initiator-name is described as taking an IQN, which is the path argument.
I'm not actually convinced this is doing what you think it is. AFAICT from reading the QEMU code, the only way to associated the -drive and -iscsi options is via the 'id' parameter of the -iscsi arg matching the IQN of the -drive option. The 'initiator-name' does not appear to have any effect on handling. If 'id' doesn't match, then QEMU just always falls back to finding the first -iscsi arg.
So IIUC with your patch here all -drive options are just going to use the first -iscsi arg, which will be wrong when we are connecting to several different iscsi servers.
I'm not really seeing any viable way to deal with this problem, so I think we might need to wait for QEMU 2.7 before we can use secrets with iSCSI :-(
Ahh.. hmm... Trying to figure out how the matching is done wasn't completely clear... Going with the full IQN path seemed to work, but yeah I have a single iSCSI server. Although I have to wonder how multiple servers in the same domain would be handled today from libvirt since without providing the '-iscsi' arg, it seems qemu will create the default IQN "iqn.2008-11.org.linux-kvm%s%s", where the %s%s could be ":name" or empty depending on the uuid_info. I assume that wouldn't change regardless of the number of iSCSI -drive's provided in the libvirt XML. So, if each -drive used a unique iSCSI server, then I wonder if it would work "today". <sigh> I'll adjust and repost patch 8 without iSCSI code. We can at least take advantage of RBD. I can drag in patch 7 to that in order to better handle the qemuDomainSecretHaveEncrypt John

On 02.05.2016 23:51, John Ferlan wrote:
v1 here (specifically patches 11 & 12) http://www.redhat.com/archives/libvir-list/2016-April/msg01077.html
What's new/changed:
Patches 1-3 address comments made by Jan in his review of v1.
Patch 4 is essentially following through on the Patch 3 comment
Patch 5 was part of patch 11, but I split it out for easier review
Patch 6 is the remainder of the former patch 11. Other than using the ...SECRET_INFO_TYPE_xxx nomenclature, no other real changes.
Patch 7 was part of patch 12, but I split it out to reduce the amount to review. Beyond the aforementioned ...SECRET_INFO_TYPE_xxx changes, I removed qemuDomainSecretInfoGetAlias. Initially created to help with any hotplug code, but that's not necessary, so remove it for now. It was brought up on Jan's comments as well. Create qemuDomainSecretSetup in order to address Jan's other comment about a common API
This code could be merged with patch 8, but separating it *and* leaving qemuDomainSecretHaveEncrypt essentially brain-dead worked made for a shorter pile to look at
Patch 8 is the remainder of patch 12. I investigated extracting the iSCSI and RBD code out, but it really just didn't make practice sense. I altered qemuBuildSecretIVCommandLine to just access the IV alias directly since the callers were adjusted to only try to build the IV object if necessary. The qemuBuild{Disk|Hostdev}iSCSICommandLine were renamed to qemuBuild{Disk|Hostdev}SecinfoCommandLine and each encapsulated the call to qemuBuildSecretIVCommandLine rather than inlining it in each of the callers
The qemuDomainSecretHaveEncrypt will not be braindead.
None of the tests changed, so it doesn't seem I messed anything up with all the requested adjustments.
John Ferlan (8): qemu: Adjust names of qemuDomainSecretInfoType enums qemu: Split out the master key create and write qemu: Move qemuDomainSecretPrepare to qemuProcessPrepareDomain qemu: Move qemuDomainSecretDestroy to qemuProcessLaunch qemu: Separate network URI command building code qemu: Introduce qemuDomainSecretIV qemu: Introduce new Secret IV API's qemu: Utilize qemu secret objects for SCSI/RBD auth/secret
configure.ac | 1 + src/qemu/qemu_alias.c | 23 ++ src/qemu/qemu_alias.h | 2 + src/qemu/qemu_command.c | 359 +++++++++++++++++++-- src/qemu/qemu_domain.c | 242 ++++++++++++-- src/qemu/qemu_domain.h | 34 +- src/qemu/qemu_hotplug.c | 6 +- src/qemu/qemu_process.c | 23 +- ...uxml2argv-disk-drive-network-iscsi-auth-IV.args | 39 +++ ...muxml2argv-disk-drive-network-iscsi-auth-IV.xml | 43 +++ ...emuxml2argv-disk-drive-network-rbd-auth-IV.args | 31 ++ ...qemuxml2argv-disk-drive-network-rbd-auth-IV.xml | 42 +++ ...emuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.args | 41 +++ ...qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.xml | 48 +++ ...xml2argv-hostdev-scsi-virtio-iscsi-auth-IV.args | 43 +++ ...uxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.xml | 48 +++ tests/qemuxml2argvmock.c | 31 +- tests/qemuxml2argvtest.c | 19 ++ 18 files changed, 1009 insertions(+), 66 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-iscsi-auth-IV.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-network-rbd-auth-IV.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-lsi-iscsi-auth-IV.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-hostdev-scsi-virtio-iscsi-auth-IV.xml
Looking good. ACK series, but please see my comment to the last patch before pushing. Michal
participants (3)
-
Daniel P. Berrange
-
John Ferlan
-
Michal Privoznik