Re: [libvirt] [PATCH v3 2/2] qemu: Utilize qemu secret objects for RBD auth/secret

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

> > 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.

> + > + *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.

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

> + * > + * 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.

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

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

> + } > + > 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?

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

> + * > + * 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.

> +} > + > + > /* 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 ...

> * > * 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.

> > @@ -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.