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 <jfer...@redhat.com> > --- > 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.
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
