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?