Re: [libvirt] [PATCH 2/4] qemu: Introduce qemuBuildSecretObjectProps

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(a)redhat.com&gt; >>> --- >>> src/qemu/qemu_command.c | 65 +++++++++++++++++++++++++++++++++++++++++++++---- >>> 1 file changed, 60 insertions(+), 5 deletions(-) >>> >>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c >>> index c55f42e..06d135b 100644 >>> --- a/src/qemu/qemu_command.c >>> +++ b/src/qemu/qemu_command.c >>> @@ -508,6 +508,64 @@ qemuNetworkDriveGetPort(int protocol, >>> >>> >>> /** >>> + * qemuBuildSecretObjectProps >>> + * @data: Pointer to data string >>> + * @isfile: Boolean to indicate whether data is raw data or a filepath string >>> + * @fmt: Format for the data/file (may be NULL) >>> + * @keyid: Master key alias id (may be NULL) >>> + * @iv: Initialization vector (may be NULL) >>> + * @propsret: location to store the created/built property object >>> + * >>> + * There's many ways to build a secret object for qemu depending on need, >>> + * >>> + * -object secret,id=$alias,data=$data >> >> 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.