[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

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 -


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]