On Thu, Mar 24, 2016 at 01:53:20PM -0400, John Ferlan wrote:
> Add a masterKey to _qemuDomainObjPrivate to store a random domain master
> key in order to support the ability to encrypt/decrypt sensitive data
> shared between libvirt and qemu. The key will be base64 encoded and written
> to a file to be used by the command line building code to share with qemu.
>
> New API's from this patch:
>
> qemuDomainGetMasterKeyFilePath:
> Return a path to where the key is located
>
> qemuDomainWriteMasterKeyFile: (private)
> Base64 the masterKey and write to the *KeyFilePath
>
> qemuDomainMasterKeyReadFile:
> Using the *KeyFilePath, open/read the file, base64 decode, and
> store in masterKey. Expected use only from qemuProcessReconnect
>
> qemuDomainGenerateRandomKey: (private)
> Generate a random key using available algorithms
>
> The key is generated either from the gnutls_rnd function if it
> exists or a less cryptographically strong mechanisum as a series of 8
> bit random numbers from virRandomBits stored as a byte string.
>
> qemuDomainMasterKeyRemove:
> Remove traces of the master key, remove the *KeyFilePath
>
> qemuDomainMasterKeyCreate:
> Generate the key and save a base64 encoded value of the key
> in the location returned by qemuDomainGetMasterKeyFilePath.
>
> This API will first ensure the QEMU_CAPS_OBJECT_SECRET is set
> in the capabilities. If not, then there's no need to generate
> the secret or file.
>
> The creation of the key will be attempted from qemuProcessPrepareHost
> once the libDir directory structure exists.
>
> The removal of the key will handled from qemuProcessStop just prior
> to deleting the libDir tree.
>
> Since the key will not be written out to the domain object XML file,
> the qemuProcessReconnect will read the saved, decode, and restore
> the masterKey value after a series of checks.
>
> Signed-off-by: John Ferlan <jferlan@xxxxxxxxxx>
> ---
> src/qemu/qemu_domain.c | 269 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_domain.h | 13 +++
> src/qemu/qemu_process.c | 11 ++
> 3 files changed, 293 insertions(+)
>
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 53cb2b6..fecf668 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -44,6 +44,12 @@
> #include "virthreadjob.h"
> #include "viratomic.h"
> #include "virprocess.h"
> +#include "virrandom.h"
> +#include "base64.h"
> +#include <gnutls/gnutls.h>
> +#if HAVE_GNUTLS_CRYPTO_H
> +# include <gnutls/crypto.h>
> +#endif
> #include "logging/log_manager.h"
>
> #include "storage/storage_driver.h"
> @@ -465,6 +471,269 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
> }
>
>
> +/* 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.
> +/* 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.
> +static int
> +qemuDomainWriteMasterKeyFile(qemuDomainObjPrivatePtr priv)
> +{
> + char *path;
> + char *base64Key = NULL;
> + int ret = -1;
> +
> + if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir)))
> + return -1;
> +
> + /* base64 encode the key to store it */
> + base64_encode_alloc(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN,
> + &base64Key);
> + if (!base64Key) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to encode master key"));
> + goto cleanup;
> + }
> +
> + if (virFileWriteStr(path, base64Key, 0600) < 0) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("failed to write master key file for domain"));
> + goto cleanup;
> + }
> +
> + ret = 0;
> +
> + cleanup:
> + if (base64Key) {
> + /* clear heap, free memory */
> + memset(base64Key, 0, strlen(base64Key));
> + VIR_FREE(base64Key);
> + }
> +
> + VIR_FREE(path);
> +
> + return ret;
> +}
> +
> +int
> +qemuDomainMasterKeyReadFile(qemuDomainObjPrivatePtr priv)
> +{
> + char *path;
> + char *base64Key = NULL;
> + int base64KeySize = 0;
> + char *masterKey = NULL;
> + size_t masterKeySize = 0;
> + int ret = -1;
> +
> + /* If we don't have the capability, then do nothing. */
> + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET))
> + return 0;
> +
> + if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir)))
> + return -1;
> +
> + if (!virFileExists(path)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("domain master key file doesn't exist in %s"),
> + priv->libDir);
> + goto cleanup;
> + }
> +
> + /* Read the base64 encooded secret from the file, decode it, and
> + * store in the domain private object.
> + */
> + if ((base64KeySize = virFileReadAll(path, 1024, &base64Key)) < 0)
> + goto cleanup;
> +
> + if (!base64_decode_alloc(base64Key, base64KeySize,
> + &masterKey, &masterKeySize)) {
> + virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> + _("invalid base64 in domain master key file"));
> + goto cleanup;
> + }
> +
> + if (!masterKey || masterKeySize != QEMU_DOMAIN_MASTER_KEY_LEN) {
> + virReportError(VIR_ERR_INTERNAL_ERROR,
> + _("invalid encoded master key read, size=%zd"),
> + masterKeySize);
> + goto cleanup;
> + }
> +
> + priv->masterKey = masterKey;
> + masterKey = NULL;
> +
> + ret = 0;
> +
> + cleanup:
> + if (masterKeySize > 0)
> + memset(masterKey, 0, masterKeySize);
> + VIR_FREE(masterKey);
> +
> + if (base64KeySize > 0)
> + memset(base64Key, 0, base64KeySize);
> + VIR_FREE(base64Key);
> +
> + VIR_FREE(path);
> +
> + return ret;
> +}
> +
> +
> +/* qemuDomainGenerateRandomKey
> + * @nbytes: Size in bytes of random key to generate
> + *
> + * Generate a random key of nbytes length and return it.
> + *
> + * Since the gnutls_rnd could be missing, provide an alternate less
> + * secure mechanism to at least have something.
> + *
> + * Returns pointer memory containing key on success, NULL on failure
> + */
> +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.
> +#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.
> + 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.
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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list