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

[John Ferlan]

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 at 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 ...
> 

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.

>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
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.
>