[libvirt] [PATCH v3 2/2] qemu: Utilize qemu secret objects for RBD auth/secret
John Ferlan
jferlan at redhat.com
Wed May 11 20:42:36 UTC 2016
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 at 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.
>
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 at redhat.com>
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.
>
More information about the libvir-list
mailing list