On 05/11/2016 08:21 AM, Peter Krempa wrote: > On Thu, May 05, 2016 at 15:00:41 -0400, John Ferlan wrote: >> Partial fix for: >> 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 RBD volumes >> instead of passing the base64 encoded secret on the command line. >> >> 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 plain mechanism using >> a base64 encoded secret. >> >> New API's: >> >> qemu_command.c: >> 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 RBD reference >> to the object. >> >> qemuBuildDiskSecinfoCommandLine (private) >> Handle adding the IV secret object. >> >> qemu_domain.c >> 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, >> we have secrets, and of course if the protocol source is RBD. >> >> Use the qemuDomainSecretSetup in qemuDomainSecretDiskPrepare and >> qemuDomainSecretHostdevPrepare to add the secret rather than assuming >> plain. In the future when iSCSI secrets can use this mechanism for >> either a disk or hostdev there will be less change. >> >> Command Building: >> >> Adjust the qemuBuildRBDSecinfoURI API's in order to generate the >> specific command options for an IV secret, such as: >> >> -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 the 'id=' value is the secret object alias generated by >> concatenating the disk 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 -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 commit '60390a21' >> 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 secret directly to the monitor will suffice. >> >> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx> >> --- >> src/qemu/qemu_command.c | 130 ++++++++++++++++++++- >> src/qemu/qemu_domain.c | 56 +++++++-- >> ...emuxml2argv-disk-drive-network-rbd-auth-IV.args | 31 +++++ >> ...qemuxml2argv-disk-drive-network-rbd-auth-IV.xml | 42 +++++++ >> tests/qemuxml2argvmock.c | 31 ++++- >> tests/qemuxml2argvtest.c | 11 ++ >> 6 files changed, 290 insertions(+), 11 deletions(-) >> 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 > > > I think this patch is doing too much in one place. You can definitely > split out the mocking code. > Or if your mocking code is the better solution, that's fine as well. >> >> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >> index 93aaf2a..9b0bd7a 100644 >> --- a/src/qemu/qemu_command.c >> +++ b/src/qemu/qemu_command.c >> @@ -607,6 +607,119 @@ 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; > > You can use virJSONValueObjectCreate instead of the two calls above. > OK >> + >> + *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, > > Why does this contain "IV" you are creating a secret object. > It's just a name... I'll change to AES based on the previous patch. >> + 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; >> +} >> + >> + >> +/* qemuBuildDiskSecinfoCommandLine: >> + * @cmd: Pointer to the command string >> + * @secinfo: Pointer to a possible secinfo >> + * >> + * Adding an IV secret to the command line for an iSCSI disk currently >> + * does not work. As of qemu 2.6, in order to add the options "user=" and >> + * "password-secret=", one would have to do so using an "-iscsi" command >> + * line option. However, that requires supplying an "id=" that can be used >> + * by some "-drive" command in order to "find" the correct IQN. Unfortunately, >> + * an IQN consists of characters ':' and '/' that cannot be used for an "id=". > > I don't think this kind of information belongs into a function comment. > It also doesn't really describe what is going on. > OK... >> + * >> + * Returns 0 on success, -1 w/ error on some sort of failure. >> + */ >> +static int >> +qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd, >> + qemuDomainSecretInfoPtr secinfo) >> +{ >> + /* Not necessary for non IV secrets */ >> + if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_IV) > > Again, what is IV supposed to mean? The declaration of the enum is not > commented. > OK - it's changing to AES prior patch response. >> + return 0; >> + >> + /* For now we'll just build the IV. In the future more may be required >> + * in order to support iSCSI */ > > This comment is weird. This patch is dealing with RBD. > If you look at the previous version w/ iSCSI code included it may make sense, but I'll dump it.. >> + return qemuBuildSecretIVCommandLine(cmd, secinfo); >> +} >> + >> + >> /* qemuBuildGeneralSecinfoURI: >> * @uri: Pointer to the URI structure to add to >> * @secinfo: Pointer to the secret info data (if present) >> @@ -677,6 +790,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; >> } >> @@ -1083,6 +1200,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 +1282,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 +1333,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); > > This could be added to the switch above the place where you are doing > this. > Not clear where you mean... We add "file=" first inside the if (source && ... That goes immediately after the "-drive " That switch would add "fat:", "floppy:", then do some escape magic on source while adding it to the line including the trailing comma. Then add the "password-secret=%s," Result is: "-drive file=$source,password-secret..." >> + } >> + >> if (disk->src->format > 0 && >> disk->src->type != VIR_STORAGE_TYPE_DIR) >> virBufferAsprintf(&opt, "format=%s,", >> @@ -1905,6 +2028,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 */ >> @@ -1947,6 +2072,9 @@ qemuBuildDiskDriveCommandLine(virCommandPtr cmd, >> break; >> } >> >> + if (qemuBuildDiskSecinfoCommandLine(cmd, secinfo) < 0) > > qemuBuildDiskSecretCommanLine? > Not sure I follow. The "-object secret" for the IV (or AES) secret must be in place before the "-drive" option for a disk that needs it. Eventually there will be a HostdevSecinfo. >> + return -1; >> + >> virCommandAddArg(cmd, "-drive"); >> >> /* Unfortunately it is not possible to use >> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c >> index b174caa..3e627a5 100644 >> --- a/src/qemu/qemu_domain.c >> +++ b/src/qemu/qemu_domain.c >> @@ -897,7 +897,7 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, >> * >> * Returns true if we can support the encryption code, false otherwise >> */ >> -static bool ATTRIBUTE_UNUSED >> +static bool >> qemuDomainSecretHaveEncrypt(void) >> { >> #ifdef HAVE_GNUTLS_CIPHER_ENCRYPT >> @@ -920,7 +920,7 @@ qemuDomainSecretHaveEncrypt(void) >> * >> * Returns 0 on success, -1 on failure with error message >> */ >> -static int ATTRIBUTE_UNUSED >> +static int >> qemuDomainSecretIVSetup(virConnectPtr conn, >> qemuDomainObjPrivatePtr priv, >> qemuDomainSecretInfoPtr secinfo, >> @@ -1030,6 +1030,44 @@ qemuDomainSecretIVSetup(virConnectPtr conn, >> } >> >> >> +/* 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 an RBD disk. For now iSCSI >> + * disks and hostdevs will not be able to utilize this mechanism. For details, >> + * see qemuBuildDiskSecinfoCommandLine in qemu_command.c I'll adjust this comment too... >> + * >> + * 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) && >> + protocol == VIR_STORAGE_NET_PROTOCOL_RBD) { >> + if (qemuDomainSecretIVSetup(conn, priv, secinfo, >> + srcalias, protocol, authdef) < 0) >> + return -1; >> + } else { >> + if (qemuDomainSecretPlainSetup(conn, secinfo, protocol, authdef) < 0) >> + return -1; >> + } >> + return 0; > > Looks like this should be in the previous patch so that the function > doesn't need to be unused. > AH and that's the rub... If this goes in the previous patch, then we create and expect to use an IV (AES) secret; however, without the rest of the code in this patch we don't know how to build one. So we "assume" it's a Plain secret, which quite obviously it isn't. Perhaps it'll just be easier to combine the two patches - more to review at one time though. >> +} >> + >> + >> /* qemuDomainSecretDiskDestroy: >> * @disk: Pointer to a disk definition >> * >> @@ -1058,7 +1096,7 @@ qemuDomainSecretDiskDestroy(virDomainDiskDefPtr disk) >> */ >> int >> qemuDomainSecretDiskPrepare(virConnectPtr conn, >> - qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED, >> + qemuDomainObjPrivatePtr priv, >> virDomainDiskDefPtr disk) >> { >> virStorageSourcePtr src = disk->src; >> @@ -1075,8 +1113,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; >> @@ -1119,7 +1157,7 @@ qemuDomainSecretHostdevDestroy(virDomainHostdevDefPtr hostdev) >> */ >> int >> qemuDomainSecretHostdevPrepare(virConnectPtr conn, >> - qemuDomainObjPrivatePtr priv ATTRIBUTE_UNUSED, >> + qemuDomainObjPrivatePtr priv, >> virDomainHostdevDefPtr hostdev) >> { >> virDomainHostdevSubsysPtr subsys = &hostdev->source.subsys; >> @@ -1140,9 +1178,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; >> 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/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. > > Doesn't look like we've touched it in 2015 ... > git log -p tests/qemuxml2argvmock.c shows me... ... commit ace1ee225f5cd87fb095054a6a19bdcd0fa57518 Author: Peter Krempa <pkrempa@xxxxxxxxxx> Date: Thu Dec 10 14:36:51 2015 +0100 ,,, >> * >> * 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> > > If you use the approach I've chosen you won't need to include gnutls > headers. > I'm OK with going with your approach... I think I explained why I went with virRandomBytes in response to your patch. John >> >> @@ -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 >> +} > > This won't compile without gnutls. > >> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c >> index e41444d..1f2577a 100644 >> --- a/tests/qemuxml2argvtest.c >> +++ b/tests/qemuxml2argvtest.c > > ... > >> @@ -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))) { > > This calls qemuProcessPrepareDomain which calls > qemuDomainMasterKeyCreate so the above call to generating the master key > is not necessary. > -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list