Move the module from qemu_command.c to virjson.c and rename to virJSONBuildObjectCommandline. Nothing in the API is "specific" to the qemuBuild* processing and it'll be useful to "share" with upcoming changes in the storage_backend which will build a secret object for the 'qemu-img' command. Also virJSONValueObjectForeachKeyValue and virJSONValueGetArrayAsBitmap can be static helpers. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 3 +- src/qemu/qemu_command.c | 124 +++----------------------------------------- src/qemu/qemu_command.h | 4 -- src/util/virjson.c | 117 +++++++++++++++++++++++++++++++++++++++-- src/util/virjson.h | 12 ++--- tests/qemucommandutiltest.c | 7 +-- 6 files changed, 129 insertions(+), 138 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e325168..e4bddd3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1684,6 +1684,7 @@ virISCSIScanTargets; # util/virjson.h +virJSONBuildObjectCommandline; virJSONValueArrayAppend; virJSONValueArrayGet; virJSONValueArraySize; @@ -1691,7 +1692,6 @@ virJSONValueArraySteal; virJSONValueCopy; virJSONValueFree; virJSONValueFromString; -virJSONValueGetArrayAsBitmap; virJSONValueGetBoolean; virJSONValueGetNumberDouble; virJSONValueGetNumberInt; @@ -1726,7 +1726,6 @@ virJSONValueObjectAppendNumberUlong; virJSONValueObjectAppendString; virJSONValueObjectCreate; virJSONValueObjectCreateVArgs; -virJSONValueObjectForeachKeyValue; virJSONValueObjectGet; virJSONValueObjectGetArray; virJSONValueObjectGetBoolean; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 368bd87..c55f42e 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -286,117 +286,8 @@ qemuVirCommandGetDevSet(virCommandPtr cmd, int fd) } -static int -qemuBuildObjectCommandLinePropsInternal(const char *key, - const virJSONValue *value, - virBufferPtr buf, - bool nested) -{ - virJSONValuePtr elem; - virBitmapPtr bitmap = NULL; - ssize_t pos = -1; - ssize_t end; - size_t i; - - switch ((virJSONType) value->type) { - case VIR_JSON_TYPE_STRING: - virBufferAsprintf(buf, ",%s=%s", key, value->data.string); - break; - - case VIR_JSON_TYPE_NUMBER: - virBufferAsprintf(buf, ",%s=%s", key, value->data.number); - break; - - case VIR_JSON_TYPE_BOOLEAN: - if (value->data.boolean) - virBufferAsprintf(buf, ",%s=yes", key); - else - virBufferAsprintf(buf, ",%s=no", key); - - break; - - case VIR_JSON_TYPE_ARRAY: - if (nested) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("nested -object property arrays are not supported")); - return -1; - } - - if (virJSONValueGetArrayAsBitmap(value, &bitmap) == 0) { - while ((pos = virBitmapNextSetBit(bitmap, pos)) > -1) { - if ((end = virBitmapNextClearBit(bitmap, pos)) < 0) - end = virBitmapLastSetBit(bitmap) + 1; - - if (end - 1 > pos) { - virBufferAsprintf(buf, ",%s=%zd-%zd", key, pos, end - 1); - pos = end; - } else { - virBufferAsprintf(buf, ",%s=%zd", key, pos); - } - } - } else { - /* fallback, treat the array as a non-bitmap, adding the key - * for each member */ - for (i = 0; i < virJSONValueArraySize(value); i++) { - elem = virJSONValueArrayGet((virJSONValuePtr)value, i); - - /* recurse to avoid duplicating code */ - if (qemuBuildObjectCommandLinePropsInternal(key, elem, buf, - true) < 0) - return -1; - } - } - break; - - case VIR_JSON_TYPE_OBJECT: - case VIR_JSON_TYPE_NULL: - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("NULL and OBJECT JSON types can't be converted to " - "commandline string")); - return -1; - } - - virBitmapFree(bitmap); - return 0; -} - - -static int -qemuBuildObjectCommandLineProps(const char *key, - const virJSONValue *value, - void *opaque) -{ - return qemuBuildObjectCommandLinePropsInternal(key, value, opaque, false); -} - - char * -qemuBuildObjectCommandlineFromJSON(const char *type, - const char *alias, - virJSONValuePtr props) -{ - virBuffer buf = VIR_BUFFER_INITIALIZER; - char *ret = NULL; - - virBufferAsprintf(&buf, "%s,id=%s", type, alias); - - if (virJSONValueObjectForeachKeyValue(props, - qemuBuildObjectCommandLineProps, - &buf) < 0) - goto cleanup; - - if (virBufferCheckError(&buf) < 0) - goto cleanup; - - ret = virBufferContentAndReset(&buf); - - cleanup: - virBufferFreeAndReset(&buf); - return ret; -} - - -char *qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk) +qemuDeviceDriveHostAlias(virDomainDiskDefPtr disk) { char *ret; @@ -680,8 +571,8 @@ qemuBuildObjectSecretCommandLine(virCommandPtr cmd, if (qemuBuildSecretInfoProps(secinfo, &type, &props) < 0) return -1; - if (!(tmp = qemuBuildObjectCommandlineFromJSON(type, secinfo->s.aes.alias, - props))) + if (!(tmp = virJSONBuildObjectCommandline(type, secinfo->s.aes.alias, + props))) goto cleanup; virCommandAddArgList(cmd, "-object", tmp, NULL); @@ -3226,9 +3117,8 @@ qemuBuildMemoryCellBackendStr(virDomainDefPtr def, &props, false)) < 0) goto cleanup; - if (!(*backendStr = qemuBuildObjectCommandlineFromJSON(backendType, - alias, - props))) + if (!(*backendStr = virJSONBuildObjectCommandline(backendType, alias, + props))) goto cleanup; ret = rc; @@ -3268,7 +3158,7 @@ qemuBuildMemoryDimmBackendStr(virDomainMemoryDefPtr mem, &backendType, &props, true) < 0) goto cleanup; - ret = qemuBuildObjectCommandlineFromJSON(backendType, alias, props); + ret = virJSONBuildObjectCommandline(backendType, alias, props); cleanup: VIR_FREE(alias); @@ -5411,7 +5301,7 @@ qemuBuildRNGBackendStr(virDomainRNGDefPtr rng, if (qemuBuildRNGBackendProps(rng, qemuCaps, &type, &props) < 0) goto cleanup; - ret = qemuBuildObjectCommandlineFromJSON(type, alias, props); + ret = virJSONBuildObjectCommandline(type, alias, props); cleanup: VIR_FREE(alias); diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 1c22705..d2b7fa7 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -44,10 +44,6 @@ VIR_ENUM_DECL(qemuVideo) -char *qemuBuildObjectCommandlineFromJSON(const char *type, - const char *alias, - virJSONValuePtr props); - virCommandPtr qemuBuildCommandLine(virQEMUDriverPtr driver, virLogManagerPtr logManager, virDomainDefPtr def, diff --git a/src/util/virjson.c b/src/util/virjson.c index 1022cfc..46e4661 100644 --- a/src/util/virjson.c +++ b/src/util/virjson.c @@ -1,7 +1,7 @@ /* * virjson.c: JSON object parsing/formatting * - * Copyright (C) 2009-2010, 2012-2015 Red Hat, Inc. + * Copyright (C) 2009-2010, 2012-2016 Red Hat, Inc. * Copyright (C) 2009 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -25,6 +25,7 @@ #include "virjson.h" #include "viralloc.h" +#include "virbuffer.h" #include "virerror.h" #include "virlog.h" #include "virstring.h" @@ -1000,7 +1001,7 @@ virJSONValueGetBoolean(virJSONValuePtr val, * Returns 0 on success and fills @bitmap; -1 on error and @bitmap is set to * NULL. */ -int +static int virJSONValueGetArrayAsBitmap(const virJSONValue *val, virBitmapPtr *bitmap) { @@ -1219,7 +1220,7 @@ virJSONValueObjectIsNull(virJSONValuePtr object, * Returns 0 if all elements were iterated, -2 if @cb returned negative value * during iteration and -1 on generic errors. */ -int +static int virJSONValueObjectForeachKeyValue(virJSONValuePtr object, virJSONValueObjectIteratorFunc cb, void *opaque) @@ -1817,3 +1818,113 @@ virJSONValueToString(virJSONValuePtr object ATTRIBUTE_UNUSED, return NULL; } #endif + + +static int +virJSONBuildObjectCommandLinePropsInternal(const char *key, + const virJSONValue *value, + virBufferPtr buf, + bool nested) +{ + virJSONValuePtr elem; + virBitmapPtr bitmap = NULL; + ssize_t pos = -1; + ssize_t end; + size_t i; + + switch ((virJSONType) value->type) { + case VIR_JSON_TYPE_STRING: + virBufferAsprintf(buf, ",%s=%s", key, value->data.string); + break; + + case VIR_JSON_TYPE_NUMBER: + virBufferAsprintf(buf, ",%s=%s", key, value->data.number); + break; + + case VIR_JSON_TYPE_BOOLEAN: + if (value->data.boolean) + virBufferAsprintf(buf, ",%s=yes", key); + else + virBufferAsprintf(buf, ",%s=no", key); + + break; + + case VIR_JSON_TYPE_ARRAY: + if (nested) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("nested -object property arrays are not supported")); + return -1; + } + + if (virJSONValueGetArrayAsBitmap(value, &bitmap) == 0) { + while ((pos = virBitmapNextSetBit(bitmap, pos)) > -1) { + if ((end = virBitmapNextClearBit(bitmap, pos)) < 0) + end = virBitmapLastSetBit(bitmap) + 1; + + if (end - 1 > pos) { + virBufferAsprintf(buf, ",%s=%zd-%zd", key, pos, end - 1); + pos = end; + } else { + virBufferAsprintf(buf, ",%s=%zd", key, pos); + } + } + } else { + /* fallback, treat the array as a non-bitmap, adding the key + * for each member */ + for (i = 0; i < virJSONValueArraySize(value); i++) { + elem = virJSONValueArrayGet((virJSONValuePtr)value, i); + + /* recurse to avoid duplicating code */ + if (virJSONBuildObjectCommandLinePropsInternal(key, elem, buf, + true) < 0) + return -1; + } + } + break; + + case VIR_JSON_TYPE_OBJECT: + case VIR_JSON_TYPE_NULL: + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("NULL and OBJECT JSON types can't be converted to " + "commandline string")); + return -1; + } + + virBitmapFree(bitmap); + return 0; +} + + +static int +virJSONBuildObjectCommandLineProps(const char *key, + const virJSONValue *value, + void *opaque) +{ + return virJSONBuildObjectCommandLinePropsInternal(key, value, opaque, false); +} + + +char * +virJSONBuildObjectCommandline(const char *type, + const char *alias, + virJSONValuePtr props) +{ + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *ret = NULL; + + virBufferAsprintf(&buf, "%s,id=%s", type, alias); + + if (virJSONValueObjectForeachKeyValue(props, + virJSONBuildObjectCommandLineProps, + &buf) < 0) + goto cleanup; + + if (virBufferCheckError(&buf) < 0) + goto cleanup; + + ret = virBufferContentAndReset(&buf); + + cleanup: + virBufferFreeAndReset(&buf); + return ret; +} diff --git a/src/util/virjson.h b/src/util/virjson.h index 66ed48a..eb7d097 100644 --- a/src/util/virjson.h +++ b/src/util/virjson.h @@ -1,7 +1,7 @@ /* * virjson.h: JSON object parsing/formatting * - * Copyright (C) 2009, 2012-2015 Red Hat, Inc. + * Copyright (C) 2009, 2012-2016 Red Hat, Inc. * Copyright (C) 2009 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -129,8 +129,6 @@ int virJSONValueGetNumberLong(virJSONValuePtr object, long long *value); int virJSONValueGetNumberUlong(virJSONValuePtr object, unsigned long long *value); int virJSONValueGetNumberDouble(virJSONValuePtr object, double *value); int virJSONValueGetBoolean(virJSONValuePtr object, bool *value); -int virJSONValueGetArrayAsBitmap(const virJSONValue *val, virBitmapPtr *bitmap) - ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); bool virJSONValueIsNull(virJSONValuePtr object); virJSONValuePtr virJSONValueObjectGetObject(virJSONValuePtr object, const char *key); @@ -167,10 +165,10 @@ typedef int (*virJSONValueObjectIteratorFunc)(const char *key, const virJSONValue *value, void *opaque); -int virJSONValueObjectForeachKeyValue(virJSONValuePtr object, - virJSONValueObjectIteratorFunc cb, - void *opaque); - virJSONValuePtr virJSONValueCopy(virJSONValuePtr in); +char *virJSONBuildObjectCommandline(const char *type, + const char *alias, + virJSONValuePtr props); + #endif /* __VIR_JSON_H_ */ diff --git a/tests/qemucommandutiltest.c b/tests/qemucommandutiltest.c index bd457f8..d1a73cf 100644 --- a/tests/qemucommandutiltest.c +++ b/tests/qemucommandutiltest.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2015 Red Hat, Inc. + * Copyright (C) 2015-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 @@ -19,7 +19,6 @@ #include <config.h> -#include "qemu/qemu_command.h" #include "util/virjson.h" #include "testutils.h" #include "testutilsqemu.h" @@ -51,9 +50,7 @@ testQemuCommandBuildObjectFromJSON(const void *opaque) data->expectprops ? data->expectprops : "") < 0) return -1; - result = qemuBuildObjectCommandlineFromJSON("testobject", - "testalias", - val); + result = virJSONBuildObjectCommandline("testobject", "testalias", val); if (STRNEQ_NULLABLE(expect, result)) { fprintf(stderr, "\nFailed to create object string. " -- 2.5.5

Need to commonalize the code a bit more in order to use a common function to build the JSON property from either a qemuDomainSecretInfoPtr or a virSecretKeyDef Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_command.c | 65 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index c55f42e..06d135b 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -508,6 +508,64 @@ qemuNetworkDriveGetPort(int protocol, /** + * qemuBuildSecretObjectProps + * @data: Pointer to data string + * @isfile: Boolean to indicate whether data is raw data or a filepath string + * @fmt: Format for the data/file (may be NULL) + * @keyid: Master key alias id (may be NULL) + * @iv: Initialization vector (may be NULL) + * @propsret: location to store the created/built property object + * + * There's many ways to build a secret object for qemu depending on need, + * + * -object secret,id=$alias,data=$data + * -object secret,id=$alias,data=$data[,format=base64] + * -object secret,id=$alias,file=$file + * -object secret,id=$alias,file=$file[,format=base64] + * -object secret,id=$alias,data=$data,keyid=$keyid,[iv=$iv],format=base64 + * + * When a keyid and/or iv are provided, they are assumed to be base64 encoded + * + * Build the JSON object property thusly and return + * + * Returns 0 on success, -1 on failure w/ error set + */ +static int +qemuBuildSecretObjectProps(const char *data, + bool isfile, + const char *fmt, + const char *keyid, + const char *iv, + virJSONValuePtr *propsret) +{ + if (!(*propsret = virJSONValueNewObject())) + return -1; + + if (isfile && virJSONValueObjectAdd(*propsret, "s:file", data, NULL) < 0) + goto error; + else if (virJSONValueObjectAdd(*propsret, "s:data", data, NULL) < 0) + goto error; + + if (keyid && virJSONValueObjectAdd(*propsret, "s:keyid", keyid, NULL) < 0) + goto error; + + if (iv && virJSONValueObjectAdd(*propsret, "s:iv", iv, NULL) < 0) + goto error; + + /* NB: QEMU will assume "raw" when fmt not provided! */ + if (fmt && virJSONValueObjectAdd(*propsret, "s:format", fmt, NULL) < 0) + goto error; + + return 0; + + error: + virJSONValueFree(*propsret); + + return -1; +} + + +/** * qemuBuildSecretInfoProps: * @secinfo: pointer to the secret info object * @type: returns a pointer to a character string for object name @@ -531,11 +589,8 @@ qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, if (!(keyid = qemuDomainGetMasterKeyAlias())) return -1; - if (virJSONValueObjectCreate(propsret, - "s:data", secinfo->s.aes.ciphertext, - "s:keyid", keyid, - "s:iv", secinfo->s.aes.iv, - "s:format", "base64", NULL) < 0) + if (qemuBuildSecretObjectProps(secinfo->s.aes.ciphertext, false, "base64", + keyid, secinfo->s.aes.iv, propsret) < 0) goto cleanup; ret = 0; -- 2.5.5

On 05/31/2016 08:10 AM, Peter Krempa wrote: True, but it is a "valid" qemu usage, per: wiki.qemu.org/ChangeLog/2.6 Not that I'd expect someone to use it that way. I went that way at first, but then had: if (data && file) and if (!data && !file) error checks as well as if (data) *Add(..."s:data"...) else *Add(..."s:file"...) Going this way I can "require" data to be NON-NULL (eventually). The caller already should know which would be passed, so a bool just felt more useful. IOW: The caller would have to know to pass (data, NULL, ...) or (NULL, file,...), so I see no difference to passing a bool. It's just an "implementation decision" - it can be revisited. True - but this isn't secinfo specific either. The 'secinfo' usage is specific to adding the object for an RBD disk (it would have been useful for iSCSI too, except for the pesky -iscsi parameter requirement). Also, what if there's no keyid or iv? When generating one of these for luks and specifically the create option, things are different. Also a "file" doesn't have the "keyid" and "iv" (it may be the "master key" target for an AES/secinfo). What if some future adds new options? I dunno, this really didn't seem such a bad option. If you're OK with going with repeated code and making a similar call from storage_backend for luks creation, then I can take that route. I haven't got there yet, but the 'tls-creds-x509' setup would seemingly be able to use this same non secinfo specific call. for luks will utilize "--object secret..."... We currently have no way to have a secret luks could use, but that's what I'm working towards. I have also found through experimentation that creating an AES/secinfo like secret won't work. So the options are "raw" text, base64 encoded raw text, or a file (with raw or base64 encoded text password contained). John

On 05/31/2016 10:10 AM, Peter Krempa wrote: Perhaps - only recently discovered though... have yet to determine whether it was an expected way to provide the secret... True, "file" is essentially the methodology used for the domain master key.... And furthermore qemuBuildMasterKeyCommandLine could be modified to actually call this common routine rather than inline building the "object secret,id=$ALIAS,format=raw,file=$FILE". In the case of the master key - the contents of $FILE are the 32 bytes of random data. I would think the storage_backend.c code should use the same "method" as the qemu-kvm code in order to build the secret objects, don't you think? It obviously cannot share the domain master key, it would have to generate it's own. Trying to taking a logical approach to getting there though. Regardless of the AES/luks anomaly I found, the purpose was to have a common way to generate the same format whether being built for qemu_command.c for qemu-kvm or storage_backend.c for qemu-img. I can agree that "secret,id=$alias,data=$data" is unwanted from the viewpoint of disclosing the raw secret on the command line; however, it's still possible to create 32 random bytes and encode it and pass as "data=$data,format=base64" (the example I've taken from the qemu code): openssl rand -base64 32 > key.b64 KEY=$(base64 -d key.b64 | hexdump -v -e '/1 "%02X"') So that one doesn't have to deal with the on disk file permissions. It's simple enough to ensure that "raw" data isn't accepted... It's also possible to ensure that someone providing a base64 encoded value to be used for a master key would provide something valid. Similarly for the contents of a file, it's simple enough to check that the length of data is proper for either raw or base64 encoding, if that's desired. The whole file permissions wasn't as clear to me whether the qemu-img needs to follow at least w/r/t security manager change made in order to allow the domain secret key file to be used. John

Move the function to secret_util.c and rename to virSecretBuildObjectProps. This then can be shared with impending storage backend changes that will need to build up a secret object to pass to qemu-img. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/libvirt_private.syms | 1 + src/qemu/qemu_command.c | 63 +++--------------------------------------------- src/secret/secret_util.c | 59 +++++++++++++++++++++++++++++++++++++++++++++ src/secret/secret_util.h | 10 ++++++++ 4 files changed, 73 insertions(+), 60 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e4bddd3..0cd7a9c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1074,6 +1074,7 @@ nodeSetMemoryParameters; # secret/secret_util.h +virSecretBuildObjectProps; virSecretGetSecretString; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 06d135b..47688e4 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -48,6 +48,7 @@ #include "snapshot_conf.h" #include "storage_conf.h" #include "secret_conf.h" +#include "secret_util.h" #include "network/bridge_driver.h" #include "virnetdevtap.h" #include "device_conf.h" @@ -508,64 +509,6 @@ qemuNetworkDriveGetPort(int protocol, /** - * qemuBuildSecretObjectProps - * @data: Pointer to data string - * @isfile: Boolean to indicate whether data is raw data or a filepath string - * @fmt: Format for the data/file (may be NULL) - * @keyid: Master key alias id (may be NULL) - * @iv: Initialization vector (may be NULL) - * @propsret: location to store the created/built property object - * - * There's many ways to build a secret object for qemu depending on need, - * - * -object secret,id=$alias,data=$data - * -object secret,id=$alias,data=$data[,format=base64] - * -object secret,id=$alias,file=$file - * -object secret,id=$alias,file=$file[,format=base64] - * -object secret,id=$alias,data=$data,keyid=$keyid,[iv=$iv],format=base64 - * - * When a keyid and/or iv are provided, they are assumed to be base64 encoded - * - * Build the JSON object property thusly and return - * - * Returns 0 on success, -1 on failure w/ error set - */ -static int -qemuBuildSecretObjectProps(const char *data, - bool isfile, - const char *fmt, - const char *keyid, - const char *iv, - virJSONValuePtr *propsret) -{ - if (!(*propsret = virJSONValueNewObject())) - return -1; - - if (isfile && virJSONValueObjectAdd(*propsret, "s:file", data, NULL) < 0) - goto error; - else if (virJSONValueObjectAdd(*propsret, "s:data", data, NULL) < 0) - goto error; - - if (keyid && virJSONValueObjectAdd(*propsret, "s:keyid", keyid, NULL) < 0) - goto error; - - if (iv && virJSONValueObjectAdd(*propsret, "s:iv", iv, NULL) < 0) - goto error; - - /* NB: QEMU will assume "raw" when fmt not provided! */ - if (fmt && virJSONValueObjectAdd(*propsret, "s:format", fmt, NULL) < 0) - goto error; - - return 0; - - error: - virJSONValueFree(*propsret); - - return -1; -} - - -/** * qemuBuildSecretInfoProps: * @secinfo: pointer to the secret info object * @type: returns a pointer to a character string for object name @@ -589,8 +532,8 @@ qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, if (!(keyid = qemuDomainGetMasterKeyAlias())) return -1; - if (qemuBuildSecretObjectProps(secinfo->s.aes.ciphertext, false, "base64", - keyid, secinfo->s.aes.iv, propsret) < 0) + if (virSecretBuildObjectProps(secinfo->s.aes.ciphertext, false, "base64", + keyid, secinfo->s.aes.iv, propsret) < 0) goto cleanup; ret = 0; diff --git a/src/secret/secret_util.c b/src/secret/secret_util.c index 5602401..cda8ae6 100644 --- a/src/secret/secret_util.c +++ b/src/secret/secret_util.c @@ -24,6 +24,7 @@ #include "secret_util.h" #include "viralloc.h" #include "virerror.h" +#include "virjson.h" #include "virlog.h" #include "virobject.h" #include "viruuid.h" @@ -83,3 +84,61 @@ virSecretGetSecretString(virConnectPtr conn, virObjectUnref(sec); return ret; } + + +/** + * virSecretBuildObjectProps + * @data: Pointer to data string + * @isfile: Boolean to indicate whether data is raw data or a filepath string + * @fmt: Format for the data/file (may be NULL) + * @keyid: Master key alias id (may be NULL) + * @iv: Initialization vector (may be NULL) + * @propsret: location to store the created/built property object + * + * There's many ways to build a secret object for qemu depending on need, + * + * -object secret,id=$alias,data=$data + * -object secret,id=$alias,data=$data[,format=base64] + * -object secret,id=$alias,file=$file + * -object secret,id=$alias,file=$file[,format=base64] + * -object secret,id=$alias,data=$data,keyid=$keyid,[iv=$iv],format=base64 + * + * When a keyid and/or iv are provided, they are assumed to be base64 encoded + * + * Build the JSON object property thusly and return + * + * Returns 0 on success, -1 on failure w/ error set + */ +int +virSecretBuildObjectProps(const char *data, + bool isfile, + const char *fmt, + const char *keyid, + const char *iv, + virJSONValuePtr *propsret) +{ + if (!(*propsret = virJSONValueNewObject())) + return -1; + + if (isfile && virJSONValueObjectAdd(*propsret, "s:file", data, NULL) < 0) + goto error; + else if (virJSONValueObjectAdd(*propsret, "s:data", data, NULL) < 0) + goto error; + + if (keyid && virJSONValueObjectAdd(*propsret, "s:keyid", keyid, NULL) < 0) + goto error; + + if (iv && virJSONValueObjectAdd(*propsret, "s:iv", iv, NULL) < 0) + goto error; + + /* NB: QEMU will assume "raw" when fmt not provided! */ + if (fmt && virJSONValueObjectAdd(*propsret, "s:format", fmt, NULL) < 0) + goto error; + + return 0; + + error: + virJSONValueFree(*propsret); + + return -1; +} diff --git a/src/secret/secret_util.h b/src/secret/secret_util.h index a039662..88ccbff 100644 --- a/src/secret/secret_util.h +++ b/src/secret/secret_util.h @@ -23,6 +23,7 @@ # define __VIR_SECRET_H__ # include "internal.h" +# include "virjson.h" # include "virstoragefile.h" int virSecretGetSecretString(virConnectPtr conn, @@ -32,4 +33,13 @@ int virSecretGetSecretString(virConnectPtr conn, size_t *ret_secret_size) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2) ATTRIBUTE_NONNULL(4) ATTRIBUTE_NONNULL(5) ATTRIBUTE_RETURN_CHECK; + +int virSecretBuildObjectProps(const char *data, + bool isfile, + const char *fmt, + const char *keyid, + const char *iv, + virJSONValuePtr *propsret) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + #endif /* __VIR_SECRET_H__ */ -- 2.5.5

Rework the logic to remove the various levels of indirection between qemuBuildSecretInfoProps and qemuBuildObjectSecretCommandLine. Merge them into qemuBuildObjectSecinfoCommandLine which will do all the work of building the secinfo secret object command line. This will be the model for building the secretdef secret object command line. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_command.c | 73 ++++++++++++++----------------------------------- 1 file changed, 20 insertions(+), 53 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 47688e4..e49327d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -508,77 +508,43 @@ 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 +/* qemuBuildSecinfoCommandLine: + * @cmd: Pointer to the command string + * @secinfo: Pointer to a possible secinfo * - * Build the JSON properties for the secret info type. + * Build the command line string using the secinfo structure and + * common json object building principles. * - * Returns 0 on success with the filled in JSON property; otherwise, - * returns -1 on failure error message set. + * Returns 0 on success, -1 on failure w/ error message set */ static int -qemuBuildSecretInfoProps(qemuDomainSecretInfoPtr secinfo, - const char **type, - virJSONValuePtr *propsret) +qemuBuildObjectSecinfoCommandLine(virCommandPtr cmd, + qemuDomainSecretInfoPtr secinfo) { int ret = -1; + virJSONValuePtr props = NULL; char *keyid = NULL; - - *type = "secret"; + char *commandStr = NULL; if (!(keyid = qemuDomainGetMasterKeyAlias())) return -1; if (virSecretBuildObjectProps(secinfo->s.aes.ciphertext, false, "base64", - keyid, secinfo->s.aes.iv, propsret) < 0) + keyid, secinfo->s.aes.iv, &props) < 0) goto cleanup; - ret = 0; - - cleanup: - VIR_FREE(keyid); - - return ret; -} - - -/** - * qemuBuildObjectSecretCommandLine: - * @cmd: the command to modify - * @secinfo: pointer to the secret info object - * - * If the secinfo is available and associated with an AES 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 -qemuBuildObjectSecretCommandLine(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 = virJSONBuildObjectCommandline(type, secinfo->s.aes.alias, - props))) + if (!(commandStr = virJSONBuildObjectCommandline("secret", + secinfo->s.aes.alias, + props))) goto cleanup; - virCommandAddArgList(cmd, "-object", tmp, NULL); + virCommandAddArgList(cmd, "-object", commandStr, NULL); ret = 0; cleanup: + VIR_FREE(keyid); virJSONValueFree(props); - VIR_FREE(tmp); + VIR_FREE(commandStr); return ret; } @@ -588,7 +554,8 @@ qemuBuildObjectSecretCommandLine(virCommandPtr cmd, * @cmd: Pointer to the command string * @secinfo: Pointer to a possible secinfo * - * Add the secret object for the disks that will be using it to perform + * If the secinfo is available and associated with an AES secret, then + * add the secret object for the disks that will be using it to perform * their authentication. * * Returns 0 on success, -1 w/ error on some sort of failure. @@ -601,7 +568,7 @@ qemuBuildDiskSecinfoCommandLine(virCommandPtr cmd, if (!secinfo || secinfo->type != VIR_DOMAIN_SECRET_INFO_TYPE_AES) return 0; - return qemuBuildObjectSecretCommandLine(cmd, secinfo); + return qemuBuildObjectSecinfoCommandLine(cmd, secinfo); } -- 2.5.5

On Sat, May 28, 2016 at 09:55:12 -0400, John Ferlan wrote: [...] I'm not a fan of this noise added by the editor. [...] This shouldn't be necessary any more. [...] And this could be removed too. [...] You also don't need @secret any more. ACK with the suggested changes after the release. This patch can be applied stand-alone. Peter

Move the enum into secret_util, rename it to be just virSecretLookupType. This includes quite a bit of collateral damage, but the goal is to remove the "virStorage*" and replace with the virSecretLookupType so that it's easier to to add new lookups that aren't necessarily storage pool related. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- While I was at it - I've been thinking "how" to make virSecretGetSecretString be even more generic so that it doesn't have the 'authdef'. Then I had an epiphany on a Saturday morning and thought, well if we move the secretType and "uuid/usage" union into their own structure, then the code doesn't have to know about 'authdef' and could pass a structure that would describe which lookup (byUUID or byUsage) API to use... This is also going to be used by the luks code when it needs to lookup a new secret format... Probably will be usable in order to handle the tls lookups as well! cfg.mk | 2 +- src/libxl/libxl_conf.c | 2 +- src/qemu/qemu_domain.c | 4 ++-- src/secret/secret_util.c | 18 +++++++++--------- src/secret/secret_util.h | 22 ++++++++++++++++++++-- src/storage/storage_backend_iscsi.c | 7 ++++--- src/storage/storage_backend_rbd.c | 3 ++- src/util/virstoragefile.c | 33 +++++++++++++++++---------------- src/util/virstoragefile.h | 17 +++-------------- tests/qemuargv2xmltest.c | 4 ++-- 10 files changed, 61 insertions(+), 51 deletions(-) diff --git a/cfg.mk b/cfg.mk index a7b7266..0529a4e 100644 --- a/cfg.mk +++ b/cfg.mk @@ -780,7 +780,7 @@ mid_dirs=access|conf|cpu|locking|logging|network|node_device|rpc|security|storag sc_prohibit_cross_inclusion: @for dir in $(cross_dirs); do \ case $$dir in \ - util/) safe="util";; \ + util/) safe="(util|secret)";; \ access/ | conf/) safe="($$dir|conf|util)";; \ locking/) safe="($$dir|util|conf|rpc)";; \ cpu/| network/| node_device/| rpc/| security/| storage/) \ diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index a64b4c1..b3f78f0 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1033,7 +1033,7 @@ libxlMakeNetworkDiskSrc(virStorageSourcePtr src, char **srcstr) if (!(conn = virConnectOpen("xen:///system"))) goto cleanup; - if (virSecretGetSecretString(conn, src->auth, + if (virSecretGetSecretString(conn, &src->auth->secdef, VIR_SECRET_USAGE_TYPE_CEPH, &secret, &secretlen) < 0) goto cleanup; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index c21465d..a871d3e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -829,7 +829,7 @@ qemuDomainSecretPlainSetup(virConnectPtr conn, if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD) secretType = VIR_SECRET_USAGE_TYPE_CEPH; - return virSecretGetSecretString(conn, authdef, secretType, + return virSecretGetSecretString(conn, &authdef->secdef, secretType, &secinfo->s.plain.secret, &secinfo->s.plain.secretlen); } @@ -902,7 +902,7 @@ qemuDomainSecretAESSetup(virConnectPtr conn, goto cleanup; /* Grab the unencoded secret */ - if (virSecretGetSecretString(conn, authdef, secretType, + if (virSecretGetSecretString(conn, &authdef->secdef, secretType, &secret, &secretlen) < 0) goto cleanup; diff --git a/src/secret/secret_util.c b/src/secret/secret_util.c index cda8ae6..bda22c9 100644 --- a/src/secret/secret_util.c +++ b/src/secret/secret_util.c @@ -37,12 +37,12 @@ VIR_LOG_INIT("secret.secret_util"); /* virSecretGetSecretString: * @conn: Pointer to the connection driver to make secret driver call - * @authdef: Pointer to the disk storage authentication - * @secretUsageType: Type of secret usage for authdef lookup + * @secdef: Pointer to a storage type def for uuid/usage lookup + * @secretUsageType: Type of secret usage for usage lookup * @secret: returned secret as a sized stream of unsigned chars * @secret_size: Return size of the secret - either raw text or base64 * - * Lookup the secret for the authdef usage type and return it as raw text. + * Lookup the secret for the usage type and return it as raw text. * It is up to the caller to encode the secret further. * * Returns 0 on success, -1 on failure. On success the memory in secret @@ -50,7 +50,7 @@ VIR_LOG_INIT("secret.secret_util"); */ int virSecretGetSecretString(virConnectPtr conn, - virStorageAuthDefPtr authdef, + virSecretLookupTypeDefPtr secdef, virSecretUsageType secretUsageType, uint8_t **secret, size_t *secret_size) @@ -58,14 +58,14 @@ virSecretGetSecretString(virConnectPtr conn, virSecretPtr sec = NULL; int ret = -1; - switch (authdef->secretType) { - case VIR_STORAGE_SECRET_TYPE_UUID: - sec = conn->secretDriver->secretLookupByUUID(conn, authdef->secret.uuid); + switch (secdef->type) { + case VIR_SECRET_LOOKUP_TYPE_UUID: + sec = conn->secretDriver->secretLookupByUUID(conn, secdef->u.uuid); break; - case VIR_STORAGE_SECRET_TYPE_USAGE: + case VIR_SECRET_LOOKUP_TYPE_USAGE: sec = conn->secretDriver->secretLookupByUsage(conn, secretUsageType, - authdef->secret.usage); + secdef->u.usage); break; } diff --git a/src/secret/secret_util.h b/src/secret/secret_util.h index 88ccbff..7d6b058 100644 --- a/src/secret/secret_util.h +++ b/src/secret/secret_util.h @@ -24,10 +24,28 @@ # include "internal.h" # include "virjson.h" -# include "virstoragefile.h" + +typedef enum { + VIR_SECRET_LOOKUP_TYPE_NONE, + VIR_SECRET_LOOKUP_TYPE_UUID, + VIR_SECRET_LOOKUP_TYPE_USAGE, + + VIR_SECRET_LOOKUP_TYPE_LAST +} virSecretLookupType; + +typedef struct _virSecretLookupTypeDef virSecretLookupTypeDef; +typedef virSecretLookupTypeDef *virSecretLookupTypeDefPtr; +struct _virSecretLookupTypeDef { + int type; /* virSecretLookupType */ + union { + unsigned char uuid[VIR_UUID_BUFLEN]; + char *usage; + } u; + +}; int virSecretGetSecretString(virConnectPtr conn, - virStorageAuthDefPtr authdef, + virSecretLookupTypeDefPtr secdef, virSecretUsageType secretUsageType, uint8_t **ret_secret, size_t *ret_secret_size) diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index 999b610..af4b627 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -286,8 +286,8 @@ virStorageBackendISCSISetAuth(const char *portal, if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) return 0; - VIR_DEBUG("username='%s' authType=%d secretType=%d", - authdef->username, authdef->authType, authdef->secretType); + VIR_DEBUG("username='%s' authType=%d secdef.type=%d", + authdef->username, authdef->authType, authdef->secdef.type); if (authdef->authType != VIR_STORAGE_AUTH_TYPE_CHAP) { virReportError(VIR_ERR_XML_ERROR, "%s", _("iscsi pool only supports 'chap' auth type")); @@ -301,7 +301,8 @@ virStorageBackendISCSISetAuth(const char *portal, return -1; } - if (virSecretGetSecretString(conn, authdef, VIR_SECRET_USAGE_TYPE_ISCSI, + if (virSecretGetSecretString(conn, &authdef->secdef, + VIR_SECRET_USAGE_TYPE_ISCSI, &secret_value, &secret_size) < 0) goto cleanup; diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index 34005ce..4ecdd15 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -86,7 +86,8 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, return -1; } - if (virSecretGetSecretString(conn, authdef, VIR_SECRET_USAGE_TYPE_CEPH, + if (virSecretGetSecretString(conn, &authdef->secdef, + VIR_SECRET_USAGE_TYPE_CEPH, &secret_value, &secret_value_size) < 0) goto cleanup; diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index d2da9e7..54940a0 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -1506,8 +1506,8 @@ virStorageAuthDefFree(virStorageAuthDefPtr authdef) VIR_FREE(authdef->username); VIR_FREE(authdef->secrettype); - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) - VIR_FREE(authdef->secret.usage); + if (authdef->secdef.type == VIR_SECRET_LOOKUP_TYPE_USAGE) + VIR_FREE(authdef->secdef.u.usage); VIR_FREE(authdef); } @@ -1526,11 +1526,12 @@ virStorageAuthDefCopy(const virStorageAuthDef *src) if (VIR_STRDUP(ret->secrettype, src->secrettype) < 0) goto error; ret->authType = src->authType; - ret->secretType = src->secretType; - if (ret->secretType == VIR_STORAGE_SECRET_TYPE_UUID) { - memcpy(ret->secret.uuid, src->secret.uuid, sizeof(ret->secret.uuid)); - } else if (ret->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) { - if (VIR_STRDUP(ret->secret.usage, src->secret.usage) < 0) + ret->secdef.type = src->secdef.type; + if (ret->secdef.type == VIR_SECRET_LOOKUP_TYPE_UUID) { + memcpy(ret->secdef.u.uuid, src->secdef.u.uuid, + sizeof(ret->secdef.u.uuid)); + } else if (ret->secdef.type == VIR_SECRET_LOOKUP_TYPE_USAGE) { + if (VIR_STRDUP(ret->secdef.u.usage, src->secdef.u.usage) < 0) goto error; } return ret; @@ -1573,16 +1574,16 @@ virStorageAuthDefParseSecret(xmlXPathContextPtr ctxt, } if (uuid) { - if (virUUIDParse(uuid, authdef->secret.uuid) < 0) { + if (virUUIDParse(uuid, authdef->secdef.u.uuid) < 0) { virReportError(VIR_ERR_XML_ERROR, "%s", _("invalid auth secret uuid")); goto cleanup; } - authdef->secretType = VIR_STORAGE_SECRET_TYPE_UUID; + authdef->secdef.type = VIR_SECRET_LOOKUP_TYPE_UUID; } else { - authdef->secret.usage = usage; + authdef->secdef.u.usage = usage; usage = NULL; - authdef->secretType = VIR_STORAGE_SECRET_TYPE_USAGE; + authdef->secdef.type = VIR_SECRET_LOOKUP_TYPE_USAGE; } ret = 0; @@ -1625,7 +1626,7 @@ virStorageAuthDefParseXML(xmlXPathContextPtr ctxt) VIR_FREE(authtype); } - authdef->secretType = VIR_STORAGE_SECRET_TYPE_NONE; + authdef->secdef.type = VIR_SECRET_LOOKUP_TYPE_NONE; if (virStorageAuthDefParseSecret(ctxt, authdef) < 0) goto error; @@ -1680,12 +1681,12 @@ virStorageAuthDefFormat(virBufferPtr buf, else virBufferAddLit(buf, "<secret"); - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) { - virUUIDFormat(authdef->secret.uuid, uuidstr); + if (authdef->secdef.type == VIR_SECRET_LOOKUP_TYPE_UUID) { + virUUIDFormat(authdef->secdef.u.uuid, uuidstr); virBufferAsprintf(buf, " uuid='%s'/>\n", uuidstr); - } else if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_USAGE) { + } else if (authdef->secdef.type == VIR_SECRET_LOOKUP_TYPE_USAGE) { virBufferEscapeString(buf, " usage='%s'/>\n", - authdef->secret.usage); + authdef->secdef.u.usage); } else { virBufferAddLit(buf, "/>\n"); } diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h index b88e715..b4ad42e 100644 --- a/src/util/virstoragefile.h +++ b/src/util/virstoragefile.h @@ -1,7 +1,7 @@ /* * virstoragefile.h: file utility functions for FS storage backend * - * Copyright (C) 2007-2009, 2012-2014 Red Hat, Inc. + * Copyright (C) 2007-2009, 2012-2016 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -28,6 +28,7 @@ # include "virseclabel.h" # include "virstorageencryption.h" # include "virutil.h" +# include "secret/secret_util.h" /* Minimum header size required to probe all known formats with * virStorageFileProbeFormat, or obtain metadata from a known format. @@ -201,25 +202,13 @@ typedef enum { } virStorageAuthType; VIR_ENUM_DECL(virStorageAuth) -typedef enum { - VIR_STORAGE_SECRET_TYPE_NONE, - VIR_STORAGE_SECRET_TYPE_UUID, - VIR_STORAGE_SECRET_TYPE_USAGE, - - VIR_STORAGE_SECRET_TYPE_LAST -} virStorageSecretType; - typedef struct _virStorageAuthDef virStorageAuthDef; typedef virStorageAuthDef *virStorageAuthDefPtr; struct _virStorageAuthDef { char *username; char *secrettype; /* <secret type='%s' for disk source */ int authType; /* virStorageAuthType */ - int secretType; /* virStorageSecretType */ - union { - unsigned char uuid[VIR_UUID_BUFLEN]; - char *usage; - } secret; + virSecretLookupTypeDef secdef; }; typedef struct _virStorageDriverData virStorageDriverData; diff --git a/tests/qemuargv2xmltest.c b/tests/qemuargv2xmltest.c index 0fe6b9b..212e6e8 100644 --- a/tests/qemuargv2xmltest.c +++ b/tests/qemuargv2xmltest.c @@ -36,8 +36,8 @@ static int testSanitizeDef(virDomainDefPtr vmdef) virDomainDiskDefPtr disk = vmdef->disks[i]; if (disk->src->auth) { - disk->src->auth->secretType = VIR_STORAGE_SECRET_TYPE_USAGE; - if (VIR_STRDUP(disk->src->auth->secret.usage, + disk->src->auth->secdef.type = VIR_SECRET_LOOKUP_TYPE_USAGE; + if (VIR_STRDUP(disk->src->auth->secdef.u.usage, "qemuargv2xml_usage") < 0) goto fail; } -- 2.5.5