Re: [libvirt] [PATCH 2/3] qemu: Create domain master key
[...]
>
> +/* 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