[libvirt] [PATCH 2/3] qemu: Create domain master key

[John Ferlan]

[...]

>>  
>> +/* qemuDomainGetMasterKeyFilePath:
>> + * @libDir: Directory path to domain lib files
>> + *
>> + * This API will generate a path of the domain's libDir (e.g.
>> + * (/var/lib/libvirt/qemu/domain-#-$NAME/) and a name 'master.key'.
>> + *
>> + * This API will check and fail if the libDir is NULL as that results
>> + * in an invalid path generated.
>> + *
>> + * This API does not check if the resulting path exists, that is left
>> + * up to the caller.
>> + *
>> + * Returns path to memory containing the name of the file. It is up to the
>> + * caller to free; otherwise, NULL on failure.
>> + */
>> +char *
>> +qemuDomainGetMasterKeyFilePath(const char *libDir)
>> +{
>> +    if (!libDir) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>> +                       _("invalid path for master key file"));
>> +        return NULL;
>> +    }
>> +    return virFileBuildPath(libDir, "master.key", NULL);
>> +}
> 
> How about calling this master-key.aes to make it explicit what this
> key is intended to be used with.
> 
> 

OK

>> +/* qemuDomainWriteMasterKeyFile:
>> + * @priv: pointer to domain private object
>> + *
>> + * Get the desired path to the masterKey file, base64 encode the masterKey,
>> + * and store it in the file.
> 
> The QEMU secrets code is happy to get data in either raw or base64
> format. I wonder if there's a compelling reason to use base64 instead
> of just writing it out as raw bytes.
> 

No compelling reason comes to mind - one extra level of obscurity
mostly. One less place to have an error in the Write/Read functions for
base64 conversions.

[...]

>> +static char *
>> +qemuDomainGenerateRandomKey(size_t nbytes)
>> +{
>> +    char *key;
>> +#if HAVE_GNUTLS_CRYPTO_H
>> +    int ret;
>> +#else
>> +    size_t i;
>> +#endif
>> +
>> +    if (VIR_ALLOC_N(key, nbytes) < 0)
>> +        return NULL;
>> +
>> +#if HAVE_GNUTLS_CRYPTO_H
>> +    /* Generate a master key using gnutls if possible */
>> +    if ((ret = gnutls_rnd(GNUTLS_RND_RANDOM, key, nbytes)) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("failed to generate master key, ret=%d"), ret);
>> +        VIR_FREE(key);
>> +        return NULL;
>> +    }
>> +#else
>> +    /* Generate a less cryptographically strong master key */
>> +    for (i = 0; i < nbytes; i++)
>> +        key[i] = virRandomBits(8);
> 
> It is probably better to just read nbytes from /dev/urandom
> directly, as that's much closer to what gnutls_rnd would
> do as compared to virRandomBits.
> 

OK

>> +#endif
>> +
>> +    return key;
>> +}
>> +
>> +
>> +/* qemuDomainMasterKeyRemove:
>> + * @priv: Pointer to the domain private object
>> + *
>> + * Remove the traces of the master key, clear the heap, clear the file,
>> + * delete the file.
>> + */
>> +void
>> +qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv)
>> +{
>> +    char *path = NULL;
>> +
>> +    if (!priv->masterKey)
>> +        return;
>> +
>> +    /* Clear the heap */
>> +    memset(priv->masterKey, 0, QEMU_DOMAIN_MASTER_KEY_LEN);
>> +    VIR_FREE(priv->masterKey);
>> +
>> +    /* Clear and remove the master key file */
>> +    path = qemuDomainGetMasterKeyFilePath(priv->libDir);
>> +    if (path && virFileExists(path)) {
>> +        /* Write a "0" to the file even though we're about to delete it */
>> +        ignore_value(virFileWriteStr(path, "0", 0600));
> 
> In the src/storage/storage_backend.c we have a fnuction that can use
> scrub to wipe out a file. We should probably put that function into
> src/util/virfile.c as virFileWipe() or something like that.

Given Peter's continued objection, should we just skip this. IDC either
way, but don't

If it was desired, then I assume you are looking for just something that
will move/rename virStorageBackendWipeLocal (adjusting params), right?

> 
>> +        unlink(path);
>> +    }
>> +    VIR_FREE(path);
>> +}
> 
> 
>> @@ -212,6 +213,9 @@ struct _qemuDomainObjPrivate {
>>      char *machineName;
>>      char *libDir;            /* base path for per-domain files */
>>      char *channelTargetDir;  /* base path for per-domain channel targets */
>> +
>> +    char *masterKey; /* random key for encryption (not to be saved in our */
>> +                     /* private XML) - need to restore at process reconnect */
> 
> I'd be inclined to declare this as   uint8_t * to make it clear that its
> binary data, not a null terminated string, and also declare a masterKeyLen
> field to track length, so we only use the constant at time of generation.
> 

OK

Tks -

John