[...]
>>
>> +/* 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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list