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

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.

> 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

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

> +/* 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);