[libvirt] [PATCH 0/4] Move secret object command line building helpers

More "extractable" changes from my work on adding luks support. While working through changes in storage_backend.c I found that the building of the 'qemu-img' command to create a luks volume will need to create a secret object. In order to do that I found if I took the qemu_command code and moved things around a bit, then I would be able to access the API's that I need. Moving the command line building from qemu_command.c to virjson.c seemed logical since it was multipurpose/generic anyway. I'm not "tied" to the API name chosen - if there's suggestions, I'll adjust. Then splitting out the generation of the secret object props into their own API and then eventually into secret_util.c seemed to be the best avenue. I can then include secret_util into the storage driver. The last change just cleans up the secinfo command building processing. It was "complicated" when there was going to be hostdev and disk code trying to generate the same objects... Now that just seems less important. When/if iSCSI/hostdev support for the AES secret is added, I'll made the appropriate adjustments then. John Ferlan (4): qemu: Move and rename qemuBuildObjectCommandlineFromJSON qemu: Introduce qemuBuildSecretObjectProps qemu: Move and rename qemuBuildSecretObjectProps qemu: Rework secinfo command line building src/libvirt_private.syms | 4 +- src/qemu/qemu_command.c | 199 ++++++-------------------------------------- src/qemu/qemu_command.h | 4 - src/secret/secret_util.c | 59 +++++++++++++ src/secret/secret_util.h | 10 +++ src/util/virjson.c | 117 +++++++++++++++++++++++++- src/util/virjson.h | 12 ++- tests/qemucommandutiltest.c | 7 +- 8 files changed, 219 insertions(+), 193 deletions(-) -- 2.5.5

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

On Fri, May 27, 2016 at 09:57:07 -0400, John Ferlan wrote:
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.
It's very specific to qemu. Either qemu-img or qemu itself it's still qemu.
Also virJSONValueObjectForeachKeyValue and virJSONValueGetArrayAsBitmap can be static helpers.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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(-)
I'm leaning towards rejecting to add this to the JSON helpers. Maybe we need a new file for qemu specific helper functions to share accross modules. Peter

On 05/31/2016 07:56 AM, Peter Krempa wrote:
On Fri, May 27, 2016 at 09:57:07 -0400, John Ferlan wrote:
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.
It's very specific to qemu. Either qemu-img or qemu itself it's still qemu.
Also virJSONValueObjectForeachKeyValue and virJSONValueGetArrayAsBitmap can be static helpers.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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(-)
I'm leaning towards rejecting to add this to the JSON helpers. Maybe we need a new file for qemu specific helper functions to share accross modules.
Understood - the problem I keep running up against is "how much" to "pull in". Creating some "shared" code library wasn't considered yet, but I can certainly give that a shot (munging with Makefile's is not something I particularly look forward to). It was certainly the easy way out to add code to src/util. John

On Tue, May 31, 2016 at 09:43:12 -0400, John Ferlan wrote:
On 05/31/2016 07:56 AM, Peter Krempa wrote:
On Fri, May 27, 2016 at 09:57:07 -0400, John Ferlan wrote:
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.
It's very specific to qemu. Either qemu-img or qemu itself it's still qemu.
Also virJSONValueObjectForeachKeyValue and virJSONValueGetArrayAsBitmap can be static helpers.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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(-)
I'm leaning towards rejecting to add this to the JSON helpers. Maybe we need a new file for qemu specific helper functions to share accross modules.
Understood - the problem I keep running up against is "how much" to "pull in". Creating some "shared" code library wasn't considered yet, but I can certainly give that a shot (munging with Makefile's is not something I particularly look forward to). It was certainly the easy way out to add code to src/util.
src/util/virqemu.(ch) ? It just doesn't belong to util/virjson.h in any way.

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@redhat.com> --- 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 Fri, May 27, 2016 at 09:57:08 -0400, John Ferlan wrote:
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@redhat.com> --- 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
This doesn't make sense to use since it doesn't support binary data and puts the secret on the command line.
+ * -object secret,id=$alias,data=$data[,format=base64]
Almost the same as above except for binary data. Both should not be allowed at all.
+ * -object secret,id=$alias,file=$file
This makes sense.
+ * -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,
Hm why not pass the file as a different pointer rather than a bool?
+ 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;
I don't quite see a reason for this helper since the same can be achieved ...
+} + + +/** * 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)
... with a similar single call to a function.

On 05/31/2016 08:10 AM, Peter Krempa wrote:
On Fri, May 27, 2016 at 09:57:08 -0400, John Ferlan wrote:
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@redhat.com> --- 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
This doesn't make sense to use since it doesn't support binary data and puts the secret on the command line.
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.
+ * -object secret,id=$alias,data=$data[,format=base64]
Almost the same as above except for binary data. Both should not be allowed at all.
+ * -object secret,id=$alias,file=$file
This makes sense.
+ * -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,
Hm why not pass the file as a different pointer rather than a bool?
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.
+ 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;
I don't quite see a reason for this helper since the same can be achieved ...
From that same 2.6 wiki pointer, you'll note that the qemu-img format 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
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. 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
+} + + +/** * 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)
... with a similar single call to a function.

On Tue, May 31, 2016 at 09:49:45 -0400, John Ferlan wrote:
On 05/31/2016 08:10 AM, Peter Krempa wrote:
On Fri, May 27, 2016 at 09:57:08 -0400, John Ferlan wrote:
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@redhat.com> --- 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
This doesn't make sense to use since it doesn't support binary data and puts the secret on the command line.
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.
That still doesn't make it a good idea to use. We should not support any mode that may leak the secret.
+ * -object secret,id=$alias,data=$data[,format=base64]
Almost the same as above except for binary data. Both should not be allowed at all.
+ * -object secret,id=$alias,file=$file
This makes sense.
+ * -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,
Hm why not pass the file as a different pointer rather than a bool?
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.
That looks as yet another reason for not using such helper at all then.
It's just an "implementation decision" - it can be revisited.
+ 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;
I don't quite see a reason for this helper since the same can be achieved ...
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
Then you are doing something else apparently which warrants a different approach to configuring the secret object.
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"
As the file can be kept secret there's no need to encrypt it. That's the reason and yet another point against a super-universal helper.
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
As long as you going to explode options from the secinfo struct then it doesn't make sense to have a helper.
haven't got there yet, but the 'tls-creds-x509' setup would seemingly be able to use this same non secinfo specific call.
From that same 2.6 wiki pointer, you'll note that the qemu-img format 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
That sounds like a bug to me if it doesn't work.
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).
As said, first two are a non-option since it discloses the passphrase. File is an option but it requires setup to put it on disk with correct permissions and such which basically re-implemets the same thing we do for adding the master key. So basically it's desired to be able to do AES encrypted secrets for the LUKS key too. Peter

On 05/31/2016 10:10 AM, Peter Krempa wrote:
On Tue, May 31, 2016 at 09:49:45 -0400, John Ferlan wrote:
On 05/31/2016 08:10 AM, Peter Krempa wrote:
On Fri, May 27, 2016 at 09:57:08 -0400, John Ferlan wrote:
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@redhat.com> --- 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
This doesn't make sense to use since it doesn't support binary data and puts the secret on the command line.
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.
That still doesn't make it a good idea to use. We should not support any mode that may leak the secret.
+ * -object secret,id=$alias,data=$data[,format=base64]
Almost the same as above except for binary data. Both should not be allowed at all.
+ * -object secret,id=$alias,file=$file
This makes sense.
+ * -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,
Hm why not pass the file as a different pointer rather than a bool?
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.
That looks as yet another reason for not using such helper at all then.
It's just an "implementation decision" - it can be revisited.
+ 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;
I don't quite see a reason for this helper since the same can be achieved ...
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
Then you are doing something else apparently which warrants a different approach to configuring the secret object.
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"
As the file can be kept secret there's no need to encrypt it. That's the reason and yet another point against a super-universal helper.
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
As long as you going to explode options from the secinfo struct then it doesn't make sense to have a helper.
haven't got there yet, but the 'tls-creds-x509' setup would seemingly be able to use this same non secinfo specific call.
From that same 2.6 wiki pointer, you'll note that the qemu-img format 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
That sounds like a bug to me if it doesn't work.
Perhaps - only recently discovered though... have yet to determine whether it was an expected way to provide the secret...
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).
As said, first two are a non-option since it discloses the passphrase. File is an option but it requires setup to put it on disk with correct permissions and such which basically re-implemets the same thing we do for adding the master key. So basically it's desired to be able to do AES encrypted secrets for the LUKS key too.
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

On Tue, May 31, 2016 at 12:47:52 -0400, John Ferlan wrote:
On 05/31/2016 10:10 AM, Peter Krempa wrote:
On Tue, May 31, 2016 at 09:49:45 -0400, John Ferlan wrote:
On 05/31/2016 08:10 AM, Peter Krempa wrote:
[...]
From that same 2.6 wiki pointer, you'll note that the qemu-img format 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
That sounds like a bug to me if it doesn't work.
Perhaps - only recently discovered though... have yet to determine whether it was an expected way to provide the secret...
I'd say it makes sense to provide a secret that way. Especially since you need it for disk hotplug of LUKS volumes where you need to instantiate the secret objects via the monitor and in case of migration then via the command line. I don't think it's desired to use unencrypted secrets on the monitor though since it would require to use different code to create them.
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).
As said, first two are a non-option since it discloses the passphrase. File is an option but it requires setup to put it on disk with correct permissions and such which basically re-implemets the same thing we do for adding the master key. So basically it's desired to be able to do AES encrypted secrets for the LUKS key too.
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
Thats more due to the fact that storage volumes are not inherently tied to any domain so there isn't anything you could reuse.
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):
That depens on what you are trying to use the random bits for. If it's for generating encryption keys they need to be treated the same as passwords since the encryption keys are directly derived from them.
openssl rand -base64 32 > key.b64
Erm. How exactly is that not creating a temporary file?
KEY=$(base64 -d key.b64 | hexdump -v -e '/1 "%02X"')
semi-granted. environment variables are in fact kept somewhat secret using unix permissions. (/proc/$PID/environ/ has 600 perms) but not using selinux. Also as a side effect, environment may be leaked to child processes unless explicitly sanitized, whereas memory is by default sanitized.
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.
Libvirt shall generate the master secret so why do you need to check it?
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.
Why shouldn't it. As long as qemu-img supports the same infrastructure for the secret objects then we should treat both equally including the master key and commandline secret object instantiation.

On 06/01/2016 06:43 AM, Peter Krempa wrote:
On Tue, May 31, 2016 at 12:47:52 -0400, John Ferlan wrote:
On 05/31/2016 10:10 AM, Peter Krempa wrote:
On Tue, May 31, 2016 at 09:49:45 -0400, John Ferlan wrote:
On 05/31/2016 08:10 AM, Peter Krempa wrote:
[...]
From that same 2.6 wiki pointer, you'll note that the qemu-img format 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
That sounds like a bug to me if it doesn't work.
Perhaps - only recently discovered though... have yet to determine whether it was an expected way to provide the secret...
I'd say it makes sense to provide a secret that way. Especially since you need it for disk hotplug of LUKS volumes where you need to instantiate the secret objects via the monitor and in case of migration then via the command line. I don't think it's desired to use unencrypted secrets on the monitor though since it would require to use different code to create them.
Turned out to be a fat-finger in my setup - Dan B pointed it out to me. Difference between a "," and an "=" in a multi-line command which resulted in the obtuse error message "qemu-img: Parameter 'qom-type' is missing" - mystery solved.
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).
As said, first two are a non-option since it discloses the passphrase. File is an option but it requires setup to put it on disk with correct permissions and such which basically re-implemets the same thing we do for adding the master key. So basically it's desired to be able to do AES encrypted secrets for the LUKS key too.
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
Thats more due to the fact that storage volumes are not inherently tied to any domain so there isn't anything you could reuse.
Right - I know that. That wasn't the question. What I was pointing out is that wouldn't it be better to have one code path generating that secret object than multiple? If the common code isn't available, then there will be three different virJSONValueObjectCreate calls (and four if a change was made to qemuBuildMasterKeyCommandLine to use virJSONValueObjectCreate instead of open coding as is done now). I guess I just assumed wrongly that it would be desirable to have one place to set things up. Easily changed and less patches.
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):
That depens on what you are trying to use the random bits for. If it's for generating encryption keys they need to be treated the same as passwords since the encryption keys are directly derived from them.
The storage/qemu-img key would be shorter lived than the domain key. It would be needed only to encrypt/encode the secret for the "qemu-img create -f luks..." command. Later when the domain code goes to open the luks file, I would think it would use the domain master key in order to encrypt/encode the secret when it creates the qemu-kvm command (that part of the code isn't written yet - went with create first).
openssl rand -base64 32 > key.b64
Erm. How exactly is that not creating a temporary file?
KEY=$(base64 -d key.b64 | hexdump -v -e '/1 "%02X"')
semi-granted. environment variables are in fact kept somewhat secret using unix permissions. (/proc/$PID/environ/ has 600 perms) but not using selinux.
Also as a side effect, environment may be leaked to child processes unless explicitly sanitized, whereas memory is by default sanitized.
I was hoping you would "C" beyond the typed words... There is no need to create a file nor is there a need to create environment variables, but it's a lot easier to "show" than a code example. Although true that passing an encoded key on the command line via the ",data=" parameter is less desirable than the ",file=" option.
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.
Libvirt shall generate the master secret so why do you need to check it?
True - it was an extrapolation of the semantic "someone" at least with respect to the "common" code when building the object. IOW: in that common secret object building helper. Moot point now that I'll dump the helper.
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.
Why shouldn't it. As long as qemu-img supports the same infrastructure for the secret objects then we should treat both equally including the master key and commandline secret object instantiation.
My quandary is less about the qemu-img infrastructure than the storage driver code. That is, it's "less clear" in my mind how the storage_backend.c code would need to handle a ",file=" for its short lived master key. Where to create the file? What issues around permissions will there be? Basically anything that virSecurityManagerDomainSetPathLabel handles for the domain master key. I've been working under the assumption that it'll need to be done, but hadn't quite got that far yet. In a way I was hoping that the ",data=" option could have been used, but that leaves a base64 encoded master key on the command line along with the base64 encoded secret and iv, which yes, would allow someone sufficiently privileged enough to read any logs the ability to decipher the secret. FWIW: The current "working" plan is a command such as: qemu-img create -f luks \ --object secret,id=m0,file=$FILE \ --object secret,id=s0,data=$SECRET,keyid=m0,iv=$IV,format=base64 \ -o key-secret=s0 $LUKSFILE $SIZE Thanks for the feedback so far... trying to make sure I don't get too far off into the weeds and the dog just keeps looking at me like I'm crazy when I ask her. John

On Wed, Jun 01, 2016 at 09:04:00AM -0400, John Ferlan wrote:
My quandary is less about the qemu-img infrastructure than the storage driver code.
That is, it's "less clear" in my mind how the storage_backend.c code would need to handle a ",file=" for its short lived master key. Where to create the file? What issues around permissions will there be? Basically anything that virSecurityManagerDomainSetPathLabel handles for the domain master key. I've been working under the assumption that it'll need to be done, but hadn't quite got that far yet.
I see three possible options 1 Have storage driver create a random master key when libvirtd starts up and use that with all qemu-img invokations 2 Have storage driver create a random master key per pool and use that with all qemu-img invokations against that pool 3 Ignore master keys entirely and just put the encryption key for the luks volume in a file directly. ie just one secret object using file= instead of two secret objects I think any of these are valid approachs really, so pick whatever you think fits best
FWIW: The current "working" plan is a command such as:
qemu-img create -f luks \ --object secret,id=m0,file=$FILE \ --object secret,id=s0,data=$SECRET,keyid=m0,iv=$IV,format=base64 \ -o key-secret=s0 $LUKSFILE $SIZE
That's certainly a valid cli arg set if you pick my options 1/2 above
Thanks for the feedback so far... trying to make sure I don't get too far off into the weeds and the dog just keeps looking at me like I'm crazy when I ask her.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On 06/01/2016 09:10 AM, Daniel P. Berrange wrote:
On Wed, Jun 01, 2016 at 09:04:00AM -0400, John Ferlan wrote:
My quandary is less about the qemu-img infrastructure than the storage driver code.
That is, it's "less clear" in my mind how the storage_backend.c code would need to handle a ",file=" for its short lived master key. Where to create the file? What issues around permissions will there be? Basically anything that virSecurityManagerDomainSetPathLabel handles for the domain master key. I've been working under the assumption that it'll need to be done, but hadn't quite got that far yet.
I see three possible options
1 Have storage driver create a random master key when libvirtd starts up and use that with all qemu-img invokations
2 Have storage driver create a random master key per pool and use that with all qemu-img invokations against that pool
I wasn't thinking that this key would need to be stored long term, although I suppose that works too using the driver->stateDir as the location and building the file name using master-key.aes. I had been leaning towards generating it on the fly as needed. It was more the mgmt of the file for what amounts to one operation. Of course while typing and thinking generating a temporary file on the fly has drawbacks too.
3 Ignore master keys entirely and just put the encryption key for the luks volume in a file directly. ie just one secret object using file= instead of two secret objects
Just to be clear - the key could be base64 encoded raw text in a file? Or just the raw text in a file. Too bad we couldn't link to the secret driver file directly... In either case it's still not clear what protections are sufficient. That is would there need to be a virSecurityManagerDomainSetPathLabel type call in order for qemu-img to use the file or would creating the file in driver->stateDir be sufficient? Assuming that the ownership, protection, and other security objects I have in my environment will work in all the various environments hasn't worked well. Tks - John
I think any of these are valid approachs really, so pick whatever you think fits best
FWIW: The current "working" plan is a command such as:
qemu-img create -f luks \ --object secret,id=m0,file=$FILE \ --object secret,id=s0,data=$SECRET,keyid=m0,iv=$IV,format=base64 \ -o key-secret=s0 $LUKSFILE $SIZE
That's certainly a valid cli arg set if you pick my options 1/2 above
Thanks for the feedback so far... trying to make sure I don't get too far off into the weeds and the dog just keeps looking at me like I'm crazy when I ask her.
Regards, Daniel

On Wed, Jun 01, 2016 at 11:11:40AM -0400, John Ferlan wrote:
On 06/01/2016 09:10 AM, Daniel P. Berrange wrote:
On Wed, Jun 01, 2016 at 09:04:00AM -0400, John Ferlan wrote:
My quandary is less about the qemu-img infrastructure than the storage driver code.
That is, it's "less clear" in my mind how the storage_backend.c code would need to handle a ",file=" for its short lived master key. Where to create the file? What issues around permissions will there be? Basically anything that virSecurityManagerDomainSetPathLabel handles for the domain master key. I've been working under the assumption that it'll need to be done, but hadn't quite got that far yet.
I see three possible options
1 Have storage driver create a random master key when libvirtd starts up and use that with all qemu-img invokations
2 Have storage driver create a random master key per pool and use that with all qemu-img invokations against that pool
I wasn't thinking that this key would need to be stored long term, although I suppose that works too using the driver->stateDir as the location and building the file name using master-key.aes.
I had been leaning towards generating it on the fly as needed. It was more the mgmt of the file for what amounts to one operation. Of course while typing and thinking generating a temporary file on the fly has drawbacks too.
3 Ignore master keys entirely and just put the encryption key for the luks volume in a file directly. ie just one secret object using file= instead of two secret objects
Just to be clear - the key could be base64 encoded raw text in a file? Or just the raw text in a file. Too bad we couldn't link to the secret driver file directly...
In either case it's still not clear what protections are sufficient. That is would there need to be a virSecurityManagerDomainSetPathLabel type call in order for qemu-img to use the file or would creating the file in driver->stateDir be sufficient?
Yep, using a master secret does not really buy us any benefit when using qemu-img in one-shot mode. The main rational for the master secret feature was to facilitate QEMU hotplug, so that you could securely pass secrets to a running QEMU without having to create files on disk for every single secret you pass over the QMP monitor. With qemu-img being a short-lived process doing individual specific tasks, it is fine to just save the real secret on disk in either raw or base64 format, and directly reference it without using a master secret. In terms of permissions it is sufficient to make sure the file is simply -r------- & owned by the uid/gid that we're invoking qemu-img under. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Wed, Jun 01, 2016 at 11:11:40AM -0400, John Ferlan wrote:
Or just the raw text in a file. Too bad we couldn't link to the secret driver file directly...
BTW the real for never directly pointing qemu/qemu-img at the files maintained by the secret driver itself is that we don't want to bake in an assumption that they're in a format directly readable by qemu code. Currently the secret driver is lame and just stores the individual files in base64 plain text. At some point I want to integrate the secret driver with DEO[1], so that those files are encrypted. The thing is that with DEO, the virtualization host would never actually have access to the decryption key. The secret driver would read the encrypted file, send it to DEO which decrypts it and sends back the plain text. We would *not* want QEMU to ever talk to DEO directly, so there would be no way for QEMU to directly read the secret driver files and decrypt the data. Regards, Daniel [1] https://blog-ftweedal.rhcloud.com/2015/09/automatic-decryption-of-tls-privat... -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Wed, Jun 01, 2016 at 09:04:00 -0400, John Ferlan wrote: [...]
In a way I was hoping that the ",data=" option could have been used, but that leaves a base64 encoded master key on the command line along with the base64 encoded secret and iv, which yes, would allow someone sufficiently privileged enough to read any logs the ability to decipher the secret.
Not only log files. A straight ps -ef would disclose everything needed for somebody to know the password. As it was iterated a few times already, the passwords need to be kept secret by either encrypting them by a secret key (which needs to be passed via a file, there is no other way) or by passing them via a file. If you disclose the key along with the encrypted data it's no longer a secret. It's basically the same as base64 encoding. Humans can't read it. Hackers can. I thought that was clear enough. So you will never get around using a file. Also that's the reason why I object in supporting any insecure way to pass the data. Peter

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

On Fri, May 27, 2016 at 09:57:09 -0400, John Ferlan wrote:
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.
This should be squashed with the previous commit.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- 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;
Also this is again too qemu specific for a generic helper.

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

Rather than inline code secret lookup for rbd/iscsi, use the common function. Signed-off-by: John Ferlan <jferlan@redhat.com> --- This just makes the iscsi/rbd storage driver use the common function... work that was started by pkrempa in commit id '1d632c39' src/Makefile.am | 1 + src/storage/storage_backend_iscsi.c | 48 +++++-------------------------------- src/storage/storage_backend_rbd.c | 45 +++------------------------------- 3 files changed, 10 insertions(+), 84 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index 12b66c2..2711ca5 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -1614,6 +1614,7 @@ libvirt_driver_storage_impl_la_SOURCES = libvirt_driver_storage_impl_la_CFLAGS = \ -I$(srcdir)/access \ -I$(srcdir)/conf \ + -I$(srcdir)/secret \ $(AM_CFLAGS) libvirt_driver_storage_impl_la_LDFLAGS = $(AM_LDFLAGS) libvirt_driver_storage_impl_la_LIBADD = diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index bccfba3..999b610 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -1,7 +1,7 @@ /* * storage_backend_iscsi.c: storage backend for iSCSI handling * - * Copyright (C) 2007-2014 Red Hat, Inc. + * Copyright (C) 2007-2016 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -43,7 +43,7 @@ #include "virobject.h" #include "virstring.h" #include "viruuid.h" - +#include "secret_util.h" #define VIR_FROM_THIS VIR_FROM_STORAGE VIR_LOG_INIT("storage.storage_backend_iscsi"); @@ -279,9 +279,9 @@ virStorageBackendISCSISetAuth(const char *portal, { virSecretPtr secret = NULL; unsigned char *secret_value = NULL; + size_t secret_size; virStorageAuthDefPtr authdef = source->auth; int ret = -1; - char uuidStr[VIR_UUID_STRING_BUFLEN]; if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) return 0; @@ -301,45 +301,9 @@ virStorageBackendISCSISetAuth(const char *portal, return -1; } - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) - secret = virSecretLookupByUUID(conn, authdef->secret.uuid); - else - secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_ISCSI, - authdef->secret.usage); - - if (secret) { - size_t secret_size; - secret_value = - conn->secretDriver->secretGetValue(secret, &secret_size, 0, - VIR_SECRET_GET_VALUE_INTERNAL_CALL); - if (!secret_value) { - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) { - virUUIDFormat(authdef->secret.uuid, uuidStr); - virReportError(VIR_ERR_INTERNAL_ERROR, - _("could not get the value of the secret " - "for username %s using uuid '%s'"), - authdef->username, uuidStr); - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("could not get the value of the secret " - "for username %s using usage value '%s'"), - authdef->username, authdef->secret.usage); - } - goto cleanup; - } - } else { - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) { - virUUIDFormat(authdef->secret.uuid, uuidStr); - virReportError(VIR_ERR_NO_SECRET, - _("no secret matches uuid '%s'"), - uuidStr); - } else { - virReportError(VIR_ERR_NO_SECRET, - _("no secret matches usage value '%s'"), - authdef->secret.usage); - } + if (virSecretGetSecretString(conn, authdef, VIR_SECRET_USAGE_TYPE_ISCSI, + &secret_value, &secret_size) < 0) goto cleanup; - } if (virISCSINodeUpdate(portal, source->devices[0].path, @@ -359,7 +323,7 @@ virStorageBackendISCSISetAuth(const char *portal, cleanup: virObjectUnref(secret); - VIR_FREE(secret_value); + VIR_DISPOSE_N(secret_value, secret_size); return ret; } diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index ac46b9d..34005ce 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c @@ -36,6 +36,7 @@ #include "virrandom.h" #include "rados/librados.h" #include "rbd/librbd.h" +#include "secret_util.h" #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -63,7 +64,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, char *rados_key = NULL; virBuffer mon_host = VIR_BUFFER_INITIALIZER; virSecretPtr secret = NULL; - char secretUuid[VIR_UUID_STRING_BUFLEN]; size_t i; char *mon_buff = NULL; const char *client_mount_timeout = "30"; @@ -86,48 +86,9 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, return -1; } - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) { - virUUIDFormat(authdef->secret.uuid, secretUuid); - VIR_DEBUG("Looking up secret by UUID: %s", secretUuid); - secret = virSecretLookupByUUIDString(conn, secretUuid); - } else if (authdef->secret.usage != NULL) { - VIR_DEBUG("Looking up secret by usage: %s", - authdef->secret.usage); - secret = virSecretLookupByUsage(conn, VIR_SECRET_USAGE_TYPE_CEPH, - authdef->secret.usage); - } - - if (secret == NULL) { - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) { - virReportError(VIR_ERR_NO_SECRET, - _("no secret matches uuid '%s'"), - secretUuid); - } else { - virReportError(VIR_ERR_NO_SECRET, - _("no secret matches usage value '%s'"), - authdef->secret.usage); - } - goto cleanup; - } - - secret_value = conn->secretDriver->secretGetValue(secret, - &secret_value_size, 0, - VIR_SECRET_GET_VALUE_INTERNAL_CALL); - - if (!secret_value) { - if (authdef->secretType == VIR_STORAGE_SECRET_TYPE_UUID) { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("could not get the value of the secret " - "for username '%s' using uuid '%s'"), - authdef->username, secretUuid); - } else { - virReportError(VIR_ERR_INTERNAL_ERROR, - _("could not get the value of the secret " - "for username '%s' using usage value '%s'"), - authdef->username, authdef->secret.usage); - } + if (virSecretGetSecretString(conn, authdef, VIR_SECRET_USAGE_TYPE_CEPH, + &secret_value, &secret_value_size) < 0) goto cleanup; - } if (!(rados_key = virStringEncodeBase64(secret_value, secret_value_size))) goto cleanup; -- 2.5.5

On Sat, May 28, 2016 at 09:55:12 -0400, John Ferlan wrote:
Rather than inline code secret lookup for rbd/iscsi, use the common function.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- This just makes the iscsi/rbd storage driver use the common function... work that was started by pkrempa in commit id '1d632c39'
src/Makefile.am | 1 + src/storage/storage_backend_iscsi.c | 48 +++++-------------------------------- src/storage/storage_backend_rbd.c | 45 +++------------------------------- 3 files changed, 10 insertions(+), 84 deletions(-)
[...]
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index bccfba3..999b610 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -1,7 +1,7 @@ /* * storage_backend_iscsi.c: storage backend for iSCSI handling * - * Copyright (C) 2007-2014 Red Hat, Inc. + * Copyright (C) 2007-2016 Red Hat, Inc.
I'm not a fan of this noise added by the editor.
* Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or
[...]
@@ -279,9 +279,9 @@ virStorageBackendISCSISetAuth(const char *portal, { virSecretPtr secret = NULL;
This shouldn't be necessary any more.
unsigned char *secret_value = NULL; + size_t secret_size; virStorageAuthDefPtr authdef = source->auth; int ret = -1; - char uuidStr[VIR_UUID_STRING_BUFLEN];
if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) return 0;
[...]
@@ -359,7 +323,7 @@ virStorageBackendISCSISetAuth(const char *portal,
cleanup: virObjectUnref(secret);
And this could be removed too.
- VIR_FREE(secret_value); + VIR_DISPOSE_N(secret_value, secret_size); return ret; }
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index ac46b9d..34005ce 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c
[...]
@@ -63,7 +64,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, char *rados_key = NULL; virBuffer mon_host = VIR_BUFFER_INITIALIZER; virSecretPtr secret = NULL;
You also don't need @secret any more.
- char secretUuid[VIR_UUID_STRING_BUFLEN]; size_t i; char *mon_buff = NULL; const char *client_mount_timeout = "30";
ACK with the suggested changes after the release. This patch can be applied stand-alone. Peter

On 05/31/2016 09:37 AM, Peter Krempa wrote:
On Sat, May 28, 2016 at 09:55:12 -0400, John Ferlan wrote:
Rather than inline code secret lookup for rbd/iscsi, use the common function.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- This just makes the iscsi/rbd storage driver use the common function... work that was started by pkrempa in commit id '1d632c39'
src/Makefile.am | 1 + src/storage/storage_backend_iscsi.c | 48 +++++-------------------------------- src/storage/storage_backend_rbd.c | 45 +++------------------------------- 3 files changed, 10 insertions(+), 84 deletions(-)
[...]
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index bccfba3..999b610 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -1,7 +1,7 @@ /* * storage_backend_iscsi.c: storage backend for iSCSI handling * - * Copyright (C) 2007-2014 Red Hat, Inc. + * Copyright (C) 2007-2016 Red Hat, Inc.
I'm not a fan of this noise added by the editor.
I don't have some editor macro. It's done by hand if I remember to look. As long as I've been doing this (whether here or at some other company), when I've updated a module in a new year, then as I understand it, one is supposed to update the copyright. Whether that's a "hard" requirement or required legally is above my pay grade ;-).
* Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or
[...]
@@ -279,9 +279,9 @@ virStorageBackendISCSISetAuth(const char *portal, { virSecretPtr secret = NULL;
This shouldn't be necessary any more.
unsigned char *secret_value = NULL; + size_t secret_size; virStorageAuthDefPtr authdef = source->auth; int ret = -1; - char uuidStr[VIR_UUID_STRING_BUFLEN];
if (!authdef || authdef->authType == VIR_STORAGE_AUTH_TYPE_NONE) return 0;
[...]
@@ -359,7 +323,7 @@ virStorageBackendISCSISetAuth(const char *portal,
cleanup: virObjectUnref(secret);
And this could be removed too.
- VIR_FREE(secret_value); + VIR_DISPOSE_N(secret_value, secret_size); return ret; }
diff --git a/src/storage/storage_backend_rbd.c b/src/storage/storage_backend_rbd.c index ac46b9d..34005ce 100644 --- a/src/storage/storage_backend_rbd.c +++ b/src/storage/storage_backend_rbd.c
[...]
@@ -63,7 +64,6 @@ virStorageBackendRBDOpenRADOSConn(virStorageBackendRBDStatePtr ptr, char *rados_key = NULL; virBuffer mon_host = VIR_BUFFER_INITIALIZER; virSecretPtr secret = NULL;
You also don't need @secret any more.
- char secretUuid[VIR_UUID_STRING_BUFLEN]; size_t i; char *mon_buff = NULL; const char *client_mount_timeout = "30";
ACK with the suggested changes after the release. This patch can be applied stand-alone.
I adjusted the two instances... I didn't expect any of this to make it prior to the release. As noted in a cover letter or two - it's part of a much larger series that I'm able to grab stuff out of to lessen future review pain and ensure that the direction this is headed is OK... John

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@redhat.com> --- 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
participants (3)
-
Daniel P. Berrange
-
John Ferlan
-
Peter Krempa