[...]
+/* 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