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

Tue May 31 16:47:52 UTC 2016

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