[libvirt] [RFC 1/3] qemu: Create domain master key

[John Ferlan]

On 03/22/2016 10:08 AM, Daniel P. Berrange wrote:
> On Mon, Mar 21, 2016 at 02:29:00PM -0400, John Ferlan wrote:
>> Add a masterKey to _qemuDomainObjPrivate to store a base64 encoded domain
>> master key in order to support the ability to encrypt/decrypt sensitive
>> data shared between libvirt and qemu. The base64 encoded value will be
>> written to the domain XML file for consistency between domain restarts.
> 
> Ohh, no, we don't want the master key to ever appear in any XML file,
> because that in turn leads to compromise of user data when reporting
> bugs. For example if the user provides the CLI args + runtime XML
> then you can decrypt their passwords from the CLI args. The master
> key must only ever be in its own file, which minimises the chance of
> the user ever uploading the master key for their VM with bug reports.
> 

OK - well that simplifies certain things; however, I would think that
means on libvirtd restart we would then have to read the master key file
in order to repopulate the priv->masterKey, right?

>> Two APIs qemuDomainWriteMasterKeyFile and qemuDomainGetMasterKeyFilePath
>> will manage the details of the file manipulation.
>>
>> The Get*Path API can be used in order to return a path to the master
>> secret key file, which will be a combination of the domain's libDir
>> (e.g. /var/lib/libvirt/qemu/domain-#-$NAME/) and a name 'master.key'.
>> Use of the domain's libDir was chosen as opposed to a more generic
>> /var/lib/libvirt/qemu/$NAME-master.key since it's a domain specific
>> area rather than repeating issues found as a result of using the
>> domain name in a file. The Get*Path API doesn't check if the
>> libDir exists, it just generates the path.
>>
>> The Write*File API will be used to create the on disk master secret file
>> once the domain lib infrastructure is generated during qemuProcessLaunch.
>>
>> The masterKey is generated as a series of 8 bit random numbers stored
>> as a 32 byte string and then base64 encoded before saving in the domain
>> private object.
>>
>> A separate function will generate the key as it's expected to be utilized
>> by future patches to support the generation of the initialization vector.
>>
>> Object removal will clear and free the secret as well as check for the
>> presence of the master key file and remove it if necessary (although it
>> shouldn't be necessary by this point in time, but being extra safe).
>>
>> During process launch, the key value will additionally be stored in
>> the domain libDir. This is what will be used to share the secret with
>> qemu via a secret object. The secret file will only present while the
>> domain is active (e.g. create at Launch, delete at Stop).
> 
> Process launch is the place where we should *first* generate the
> key and store it directly into the domain libDir.
> 
> Also we should be generating a new key every time the guest
> starts. There is no reason we need to persist the same key
> forever, as it only needs to be valid for lifetime of a single
> QEMU process. This further mimizes risks of actual user passwords
> leaking, as they'll be encrypted with a new throw-away key every
> time QEMU is started
> 

It didn't dawn on me when first going through this that the priv was
only generated once at define time. But yeah, now that that realization
is fresh in my mind, I certainly see the point.

I guess I was initially thinking that the object was generated later,
but I never revisited once it became more obvious that the private
object was generated when the domain was defined.

I will move it...

>> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
>> index 9f9fae3..507ae9e 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -23,6 +23,7 @@
>>  
>>  #include <config.h>
>>  
>> +#include <assert.h>
> 
> We have a general rule that libvirt should never assert() in its
> code, so don't add this. Errors should always propagate back
> to a virErrorPtr.
> 
> 

OK - although it is used today in virsh/vsh and remote_driver...

>> +/* qemuDomainGenerateRandomKey
>> + * @nbytes: Size in bytes of random key to generate - expect multiple of 8
>> + *
>> + * Generate a random key of nbytes length and return it.
>> + *
>> + * Returns pointer memory containing key on success, NULL on failure
>> + */
>> +static unsigned char *
>> +qemuDomainGenerateRandomKey(int nbytes)
>> +{
>> +    unsigned char *key;
>> +    size_t i;
>> +
>> +    assert((nbytes % 8) == 0);
>> +
>> +    if (VIR_ALLOC_N(key, nbytes) < 0)
>> +        return NULL;
>> +
>> +    /* Generate a random master key based on the nbytes passed */
>> +    for (i = 0; i < nbytes; i++)
>> +        key[i] = virRandomBits(8);
>> +
>> +    return key;
>> +}
> 
> The virRandomBits API doesn't provide cryptographically strong
> random data. Since we already link to gnutls, we should call
> out to
> 
>     ret = gnutls_rnd(GNUTLS_RND_RANDOM, key, nbytes);
> 

Ahhh..  I wasn't aware of that API in gnutls - I was searching on
"other" strings using cscope.

Thanks for the feedback -

John