On Tue, Mar 29, 2016 at 10:33:34AM -0400, John Ferlan wrote:
[...]
+/* 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?
Ok lets just ignore it - we can easily add it later if desired. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|