[libvirt] [PATCH 0/3] Add a domain masterKey secret for qemu

Differences to RFC: http://www.redhat.com/archives/libvir-list/2016-March/msg00966.html (many, but highlights are): * Patch2 is Patch1 and I figured out the 'qom-list-types' magic in order to fill in the .replies file * Use of qemuDomainObjPrivate rather than passing 'masterKey' * The masterKey is not stored in XML output nor is it stored as a base64 value * The masterKey is created during qemuProcessPrepareHost right after the libDir is created. It is only created if the capability exists. * The masterKey is removed during qemuProcessStop. There is no call to qemuDomainMasterKeyRemove from qemuDomainObjPrivateFree since the assumption is the *Stop code will handle things. * Created a qemuDomainMasterKeyReadFile to handle reading the master.key file during qemuProcessReconnect rather than the prior code which would have taken the masterKey from the XML file in base64 format. * The MasterKeyCreate and MasterKeyRemove incorporate the file manipulation as well. The *Create code will save the base64 encoded secret. The callers need only call those API's. * The masterKey is generated using gnutls_rnd if that's available (following examples w/ HAVE_GNUTLS_CRYPTO_H); otherwise, the fallback position is the virRandomBits. * Moved alias code to qemu_alias.c instead of qemu_domain.c. Notes on comments where I didn't change things: * Since qemuBuildCommandLine doesn't get the qemuDomainObjPrivate, I kept the qemuBuildHasMasterKey checking the capability bit only. Other *CommandLine callers would use the same decision point. If the key was available, then "assume" it's already present on the command line since there's no failure path from building the master key command line. * I kept the write "0" to the key file just before deletion. I wasn't sure with file caches being as they can be if there could ever be that 100% guarantee that what was unlink()'d really "went away" completely. Something I noted and wasn't "clear" how to resolve, so I followed existing practices of not checking if something exists: * The recent changes to have a qemuProcessCreatePretendCmd which will allow libDir and channelTargetDir creation using "/tmp/", but not actually creating the directory inhibit this code from being able to check if the path to the masterKey exists before sending it to qemu. Now if the 'flags' was passed to qemuBuildCommandLine and the VIR_QEMU_PROCESS_START_PRETEND could be checked while building the MasterKey command line, then I could have the check in. Otherwise, I just have to assume everything's worked "right" up to this point. A secondary note on that... Why "/tmp/" - shouldn't have been "tmp/"? John Ferlan (3): qemu: Add capability bit for qemu secret object qemu: Create domain master key qemu: Introduce qemuBuildMasterKeyCommandLine src/qemu/qemu_alias.c | 17 ++ src/qemu/qemu_alias.h | 3 + src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 68 ++++++ src/qemu/qemu_domain.c | 269 +++++++++++++++++++++ src/qemu/qemu_domain.h | 13 + src/qemu/qemu_process.c | 11 + tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.6.0-1.replies | 3 + .../qemuxml2argvdata/qemuxml2argv-master-key.args | 23 ++ tests/qemuxml2argvdata/qemuxml2argv-master-key.xml | 30 +++ tests/qemuxml2argvtest.c | 2 + 13 files changed, 443 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.xml -- 2.5.0

Add a capability bit for the qemu secret object. Adjust the 2.6.0-1 caps/replies to add the secret object. For the .replies it's take from the '{"execute":"qom-list-types"}' output. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.6.0-1.replies | 3 +++ 4 files changed, 7 insertions(+) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index b223837..efbbad6 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -321,6 +321,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "qxl-vga.vram64_size_mb", /* 215 */ "chardev-logfile", "debug-threads", + "secret", ); @@ -1575,6 +1576,7 @@ struct virQEMUCapsStringFlags virQEMUCapsObjectTypes[] = { { "virtio-input-host-device", QEMU_CAPS_VIRTIO_INPUT_HOST }, { "virtio-input-host-pci", QEMU_CAPS_VIRTIO_INPUT_HOST }, { "mptsas1068", QEMU_CAPS_SCSI_MPTSAS1068 }, + { "secret", QEMU_CAPS_OBJECT_SECRET }, }; static struct virQEMUCapsStringFlags virQEMUCapsObjectPropsVirtioBalloon[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index caf3d1b..ae0d9b3 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -351,6 +351,7 @@ typedef enum { QEMU_CAPS_QXL_VGA_VRAM64, /* -device qxl-vga.vram64_size_mb */ QEMU_CAPS_CHARDEV_LOGFILE, /* -chardev logfile=xxxx */ QEMU_CAPS_NAME_DEBUG_THREADS, /* Is -name debug-threads= available */ + QEMU_CAPS_OBJECT_SECRET, /* -object secret */ QEMU_CAPS_LAST /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-1.caps b/tests/qemucapabilitiesdata/caps_2.6.0-1.caps index 549759c..a43293d 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-1.caps +++ b/tests/qemucapabilitiesdata/caps_2.6.0-1.caps @@ -178,4 +178,5 @@ <flag name='qxl.vram64_size_mb'/> <flag name='qxl-vga.vram64_size_mb'/> <flag name='debug-threads'/> + <flag name='secret'/> </qemuCaps> diff --git a/tests/qemucapabilitiesdata/caps_2.6.0-1.replies b/tests/qemucapabilitiesdata/caps_2.6.0-1.replies index d2b58b5..7590b5b 100644 --- a/tests/qemucapabilitiesdata/caps_2.6.0-1.replies +++ b/tests/qemucapabilitiesdata/caps_2.6.0-1.replies @@ -1224,6 +1224,9 @@ "name": "kvm-accel" }, { + "name": "secret" + }, + { "name": "i82559c" }, { -- 2.5.0

On Thu, Mar 24, 2016 at 01:53:19PM -0400, John Ferlan wrote:
Add a capability bit for the qemu secret object.
Adjust the 2.6.0-1 caps/replies to add the secret object. For the .replies it's take from the '{"execute":"qom-list-types"}' output.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + tests/qemucapabilitiesdata/caps_2.6.0-1.caps | 1 + tests/qemucapabilitiesdata/caps_2.6.0-1.replies | 3 +++ 4 files changed, 7 insertions(+)
ACK 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 :|

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@redhat.com> --- 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); +} + + +/* 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. + * + * Returns 0 on success, -1 on failure with error message indicating failure + */ +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; +} + + +/* qemuDomainMasterKeyReadFile: + * @priv: pointer to domain private object + * + * Expected to be called during qemuProcessReconnect once the domain + * libDir has been generated through qemuStateInitialize calling + * virDomainObjListLoadAllConfigs which will restore the libDir path + * to the domain private object. + * + * This function will get the path to the master key file and if it + * exists, it will read the file, base64 decode the read value, and + * save it in priv->masterKey. + * + * Once the file exists, the validity checks may cause failures; however, + * if the file doesn't exist or the capability doesn't exist, we just + * return (mostly) quietly. + * + * Returns 0 on success or lack of capability + * -1 on failure with error message indicating failure + */ +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); +#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)); + unlink(path); + } + VIR_FREE(path); +} + + +/* qemuDomainMasterKeyCreate: + * @priv: Pointer to the domain private object + * + * As long as the underlying qemu has the secret capability, + * generate and store as a base64 encoded value a random 32-byte + * key to be used as a secret shared with qemu to share sensitive data. + * + * Returns: 0 on success, -1 w/ error message on failure + */ +int +qemuDomainMasterKeyCreate(qemuDomainObjPrivatePtr priv) +{ + char *key = NULL; + + /* If we don't have the capability, then do nothing. */ + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET)) + return 0; + + if (!(key = qemuDomainGenerateRandomKey(QEMU_DOMAIN_MASTER_KEY_LEN))) + goto error; + + priv->masterKey = key; + key = NULL; + + if (qemuDomainWriteMasterKeyFile(priv) < 0) + goto error; + + return 0; + + error: + qemuDomainMasterKeyRemove(priv); + return -1; +} + + static virClassPtr qemuDomainDiskPrivateClass; static int diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 573968c..f85f17f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -147,6 +147,7 @@ struct qemuDomainJobObj { typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver, virDomainObjPtr vm); +# define QEMU_DOMAIN_MASTER_KEY_LEN 32 /* For a 32-byte random masterKey */ typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate { @@ -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 */ }; # define QEMU_DOMAIN_DISK_PRIVATE(disk) \ @@ -546,4 +550,13 @@ int qemuDomainSetPrivatePaths(char **domainLibDir, const char *confChannelDir, const char *domainName, int domainId); + +char *qemuDomainGetMasterKeyFilePath(const char *libDir); + +int qemuDomainMasterKeyReadFile(qemuDomainObjPrivatePtr priv); + +int qemuDomainMasterKeyCreate(qemuDomainObjPrivatePtr priv); + +void qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 6ec7b76..3abe16a 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3481,6 +3481,10 @@ qemuProcessReconnect(void *opaque) if (!(priv->pidfile = virPidFileBuildPath(cfg->stateDir, obj->def->name))) goto error; + /* Restore the masterKey */ + if (qemuDomainMasterKeyReadFile(priv) < 0) + goto error; + virNWFilterReadLockFilterUpdates(); VIR_DEBUG("Reconnect monitor to %p '%s'", obj, obj->def->name); @@ -5086,6 +5090,10 @@ qemuProcessPrepareHost(virQEMUDriverPtr driver, qemuProcessMakeDir(driver, vm, priv->channelTargetDir) < 0) goto cleanup; + VIR_DEBUG("Create domain masterKey"); + if (qemuDomainMasterKeyCreate(priv) < 0) + goto cleanup; + ret = 0; cleanup: virObjectUnref(cfg); @@ -5763,6 +5771,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, priv->monConfig = NULL; } + /* Remove the master key */ + qemuDomainMasterKeyRemove(priv); + virFileDeleteTree(priv->libDir); virFileDeleteTree(priv->channelTargetDir); -- 2.5.0

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@redhat.com> --- 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 :|

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

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 :|

On Thu, Mar 24, 2016 at 13:53:20 -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@redhat.com> --- 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
[...]
@@ -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.
Whoah, docs longer than the code itself.
+ */ +char * +qemuDomainGetMasterKeyFilePath(const char *libDir) +{ + if (!libDir) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid path for master key file")); + return NULL;
And the code is rather self-explanatory.
+ } + return virFileBuildPath(libDir, "master.key", NULL); +} + + +/* 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. + * + * Returns 0 on success, -1 on failure with error message indicating failure + */ +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; +} + + +/* qemuDomainMasterKeyReadFile: + * @priv: pointer to domain private object + * + * Expected to be called during qemuProcessReconnect once the domain + * libDir has been generated through qemuStateInitialize calling + * virDomainObjListLoadAllConfigs which will restore the libDir path + * to the domain private object. + * + * This function will get the path to the master key file and if it + * exists, it will read the file, base64 decode the read value, and + * save it in priv->masterKey. + * + * Once the file exists, the validity checks may cause failures; however, + * if the file doesn't exist or the capability doesn't exist, we just + * return (mostly) quietly. + * + * Returns 0 on success or lack of capability + * -1 on failure with error message indicating failure + */ +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;
so this is NULL now and masterKeySize > 0
+ + ret = 0; + + cleanup: + if (masterKeySize > 0) + memset(masterKey, 0, masterKeySize);
... memset(NULL, 0, 32); This crashes on success path.
+ 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); +#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 */
This still doesn't make any sense. The filesystem needs to guarantee this anyways. This is just a false sense of security.
+ ignore_value(virFileWriteStr(path, "0", 0600)); + unlink(path); + } + VIR_FREE(path); +} + + +/* qemuDomainMasterKeyCreate: + * @priv: Pointer to the domain private object + * + * As long as the underlying qemu has the secret capability, + * generate and store as a base64 encoded value a random 32-byte + * key to be used as a secret shared with qemu to share sensitive data. + * + * Returns: 0 on success, -1 w/ error message on failure + */ +int +qemuDomainMasterKeyCreate(qemuDomainObjPrivatePtr priv) +{ + char *key = NULL; + + /* If we don't have the capability, then do nothing. */ + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET)) + return 0; + + if (!(key = qemuDomainGenerateRandomKey(QEMU_DOMAIN_MASTER_KEY_LEN))) + goto error; + + priv->masterKey = key; + key = NULL;
key isn't touched after the previous line, thus it doesn't make much sense to clear that variable.
+ + if (qemuDomainWriteMasterKeyFile(priv) < 0) + goto error; + + return 0; + + error: + qemuDomainMasterKeyRemove(priv); + return -1; +} + + static virClassPtr qemuDomainDiskPrivateClass;
static int diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 573968c..f85f17f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -147,6 +147,7 @@ struct qemuDomainJobObj { typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver, virDomainObjPtr vm);
+# define QEMU_DOMAIN_MASTER_KEY_LEN 32 /* For a 32-byte random masterKey */
256 bit.
typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate {

On 03/29/2016 08:56 AM, Peter Krempa 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.
Whoah, docs longer than the code itself.
Setting future expectations. I'll shorten it though.
+ */ +char * +qemuDomainGetMasterKeyFilePath(const char *libDir) +{ + if (!libDir) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("invalid path for master key file")); + return NULL;
And the code is rather self-explanatory.
+ } + return virFileBuildPath(libDir, "master.key", NULL); +} +
[...]
+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;
so this is NULL now and masterKeySize > 0
doh. Yep. and that's what happens when adding the cleanup: late
+ + ret = 0; + + cleanup: + if (masterKeySize > 0) + memset(masterKey, 0, masterKeySize);
... memset(NULL, 0, 32);
This crashes on success path.
+ VIR_FREE(masterKey); + + if (base64KeySize > 0) + memset(base64Key, 0, base64KeySize); + VIR_FREE(base64Key); + + VIR_FREE(path); + + return ret; +} +
[...]
+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 */
This still doesn't make any sense. The filesystem needs to guarantee this anyways. This is just a false sense of security.
See my note in response to Dan's review. IDC either way.
+ ignore_value(virFileWriteStr(path, "0", 0600)); + unlink(path); + } + VIR_FREE(path); +} + + +/* qemuDomainMasterKeyCreate: + * @priv: Pointer to the domain private object + * + * As long as the underlying qemu has the secret capability, + * generate and store as a base64 encoded value a random 32-byte + * key to be used as a secret shared with qemu to share sensitive data. + * + * Returns: 0 on success, -1 w/ error message on failure + */ +int +qemuDomainMasterKeyCreate(qemuDomainObjPrivatePtr priv) +{ + char *key = NULL; + + /* If we don't have the capability, then do nothing. */ + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET)) + return 0; + + if (!(key = qemuDomainGenerateRandomKey(QEMU_DOMAIN_MASTER_KEY_LEN))) + goto error; + + priv->masterKey = key; + key = NULL;
key isn't touched after the previous line, thus it doesn't make much sense to clear that variable.
It was at one time, but there's no need for it now. No need for local either.
+ + if (qemuDomainWriteMasterKeyFile(priv) < 0) + goto error; + + return 0; + + error: + qemuDomainMasterKeyRemove(priv); + return -1; +} + + static virClassPtr qemuDomainDiskPrivateClass;
static int diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 573968c..f85f17f 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -147,6 +147,7 @@ struct qemuDomainJobObj { typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver, virDomainObjPtr vm);
+# define QEMU_DOMAIN_MASTER_KEY_LEN 32 /* For a 32-byte random masterKey */
256 bit.
6 of one, half dozen of another. Tks - John

If the -object secret capability exists, then get the path to the base64 encoded masterKey file and provide that to qemu. Checking for the existence of the file before passing to qemu could be done, but causes issues in mock test environment. Since the qemuDomainObjPrivate is not available when building the command line, the qemuBuildHasMasterKey API will have to suffice as the primary arbiter for whether the capability exists in order to find/return the path to the master key for usage. Created the qemuDomainGetMasterKeyAlias API which will be used by later patches to define the 'keyid' (eg, masterKey) to be used by other secrets to provide the id to qemu for the master key. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 17 ++++++ src/qemu/qemu_alias.h | 3 + src/qemu/qemu_command.c | 68 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-master-key.args | 23 ++++++++ tests/qemuxml2argvdata/qemuxml2argv-master-key.xml | 30 ++++++++++ tests/qemuxml2argvtest.c | 2 + 6 files changed, 143 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.xml diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index efd9222..b57b967 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -484,3 +484,20 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps) return 0; } + + +/* qemuDomainGetMasterKeyAlias: + * + * Generate and return the masterKey alias + * + * Returns NULL or a string containing the master key alias + */ +char * +qemuDomainGetMasterKeyAlias(void) +{ + char *alias; + + ignore_value(VIR_STRDUP(alias, "masterKey0")); + + return alias; +} diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index a2eaa27..299a6d4 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -61,4 +61,7 @@ int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps); int qemuDomainDeviceAliasIndex(const virDomainDeviceInfo *info, const char *prefix); + +char *qemuDomainGetMasterKeyAlias(void); + #endif /* __QEMU_ALIAS_H__*/ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0331789..2b1dc93 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -151,6 +151,71 @@ VIR_ENUM_IMPL(qemuNumaPolicy, VIR_DOMAIN_NUMATUNE_MEM_LAST, "interleave"); /** + * qemuBuildHasMasterKey: + * @qemuCaps: QEMU binary capabilities + * + * Return true if this binary supports the secret -object, false otherwise. + */ +static bool +qemuBuildHasMasterKey(virQEMUCapsPtr qemuCaps) +{ + return virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_SECRET); +} + + +/** + * qemuBuildMasterKeyCommandLine: + * @cmd: the command to modify + * @qemuCaps qemu capabilities object + * @domainLibDir: location to find the master key + + * Formats the command line for a master key if available + * + * Returns 0 on success, -1 w/ error message on failure + */ +static int +qemuBuildMasterKeyCommandLine(virCommandPtr cmd, + virQEMUCapsPtr qemuCaps, + const char *domainLibDir) +{ + int ret = -1; + char *alias = NULL; + char *path = NULL; + + /* If the -object secret does not exist, then just return. This just + * means the domain won't be able to use a secret master key and is + * not a failure. + */ + if (!qemuBuildHasMasterKey(qemuCaps)) { + VIR_INFO("secret object is not supported by this QEMU binary"); + return 0; + } + + if (!(alias = qemuDomainGetMasterKeyAlias())) + return -1; + + /* Get the path. NB, the mocked test will not have the created + * file so we cannot check for existence, which is no different + * than other command line options which do not check for the + * existence of socket files before using. + */ + if (!(path = qemuDomainGetMasterKeyFilePath(domainLibDir))) + goto cleanup; + + virCommandAddArg(cmd, "-object"); + virCommandAddArgFormat(cmd, "secret,id=%s,format=base64,file=%s", + alias, path); + + ret = 0; + + cleanup: + VIR_FREE(alias); + VIR_FREE(path); + return ret; +} + + +/** * qemuVirCommandGetFDSet: * @cmd: the command to modify * @fd: fd to reassign to the child @@ -9224,6 +9289,9 @@ qemuBuildCommandLine(virConnectPtr conn, if (!standalone) virCommandAddArg(cmd, "-S"); /* freeze CPU */ + if (qemuBuildMasterKeyCommandLine(cmd, qemuCaps, domainLibDir) < 0) + goto error; + if (enableFips) virCommandAddArg(cmd, "-enable-fips"); diff --git a/tests/qemuxml2argvdata/qemuxml2argv-master-key.args b/tests/qemuxml2argvdata/qemuxml2argv-master-key.args new file mode 100644 index 0000000..14a3597 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-master-key.args @@ -0,0 +1,23 @@ +LC_ALL=C \ +PATH=/bin \ +HOME=/home/test \ +USER=test \ +LOGNAME=test \ +QEMU_AUDIO_DRV=none \ +/usr/bin/qemu \ +-name QEMUGuest1 \ +-S \ +-object secret,id=masterKey0,format=base64,\ +file=/tmp/lib/domain--1-QEMUGuest1/master.key \ +-M pc \ +-m 214 \ +-smp 2 \ +-uuid c7a5fdbd-edaf-9455-926a-d65c16db1809 \ +-nographic \ +-nodefaults \ +-monitor unix:/tmp/lib/domain--1-QEMUGuest1/monitor.sock,server,nowait \ +-no-acpi \ +-boot c \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,format=raw,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-master-key.xml b/tests/qemuxml2argvdata/qemuxml2argv-master-key.xml new file mode 100644 index 0000000..f2adb84 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-master-key.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>2</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <input type='mouse' bus='ps2'/> + <input type='keyboard' bus='ps2'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index e9b8d64..5f492cb 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1864,6 +1864,8 @@ mymain(void) DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS); + DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET); + qemuTestDriverFree(&driver); return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; -- 2.5.0

On Thu, Mar 24, 2016 at 01:53:21PM -0400, John Ferlan wrote:
If the -object secret capability exists, then get the path to the base64 encoded masterKey file and provide that to qemu. Checking for the existence of the file before passing to qemu could be done, but causes issues in mock test environment.
Since the qemuDomainObjPrivate is not available when building the command line, the qemuBuildHasMasterKey API will have to suffice as the primary arbiter for whether the capability exists in order to find/return the path to the master key for usage.
Created the qemuDomainGetMasterKeyAlias API which will be used by later patches to define the 'keyid' (eg, masterKey) to be used by other secrets to provide the id to qemu for the master key.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_alias.c | 17 ++++++ src/qemu/qemu_alias.h | 3 + src/qemu/qemu_command.c | 68 ++++++++++++++++++++++ .../qemuxml2argvdata/qemuxml2argv-master-key.args | 23 ++++++++ tests/qemuxml2argvdata/qemuxml2argv-master-key.xml | 30 ++++++++++ tests/qemuxml2argvtest.c | 2 + 6 files changed, 143 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.xml
diff --git a/src/qemu/qemu_alias.c b/src/qemu/qemu_alias.c index efd9222..b57b967 100644 --- a/src/qemu/qemu_alias.c +++ b/src/qemu/qemu_alias.c @@ -484,3 +484,20 @@ qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps)
return 0; } + + +/* qemuDomainGetMasterKeyAlias: + * + * Generate and return the masterKey alias + * + * Returns NULL or a string containing the master key alias + */ +char * +qemuDomainGetMasterKeyAlias(void) +{ + char *alias; + + ignore_value(VIR_STRDUP(alias, "masterKey0")); + + return alias; +} diff --git a/src/qemu/qemu_alias.h b/src/qemu/qemu_alias.h index a2eaa27..299a6d4 100644 --- a/src/qemu/qemu_alias.h +++ b/src/qemu/qemu_alias.h @@ -61,4 +61,7 @@ int qemuAssignDeviceAliases(virDomainDefPtr def, virQEMUCapsPtr qemuCaps);
int qemuDomainDeviceAliasIndex(const virDomainDeviceInfo *info, const char *prefix); + +char *qemuDomainGetMasterKeyAlias(void); + #endif /* __QEMU_ALIAS_H__*/ diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 0331789..2b1dc93 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -151,6 +151,71 @@ VIR_ENUM_IMPL(qemuNumaPolicy, VIR_DOMAIN_NUMATUNE_MEM_LAST, "interleave");
/** + * qemuBuildHasMasterKey: + * @qemuCaps: QEMU binary capabilities + * + * Return true if this binary supports the secret -object, false otherwise. + */ +static bool +qemuBuildHasMasterKey(virQEMUCapsPtr qemuCaps) +{ + return virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_SECRET); +} + + +/** + * qemuBuildMasterKeyCommandLine: + * @cmd: the command to modify + * @qemuCaps qemu capabilities object + * @domainLibDir: location to find the master key + + * Formats the command line for a master key if available + * + * Returns 0 on success, -1 w/ error message on failure + */ +static int +qemuBuildMasterKeyCommandLine(virCommandPtr cmd, + virQEMUCapsPtr qemuCaps, + const char *domainLibDir) +{ + int ret = -1; + char *alias = NULL; + char *path = NULL; + + /* If the -object secret does not exist, then just return. This just + * means the domain won't be able to use a secret master key and is + * not a failure. + */ + if (!qemuBuildHasMasterKey(qemuCaps)) { + VIR_INFO("secret object is not supported by this QEMU binary"); + return 0; + } + + if (!(alias = qemuDomainGetMasterKeyAlias())) + return -1; + + /* Get the path. NB, the mocked test will not have the created + * file so we cannot check for existence, which is no different + * than other command line options which do not check for the + * existence of socket files before using. + */ + if (!(path = qemuDomainGetMasterKeyFilePath(domainLibDir))) + goto cleanup; + + virCommandAddArg(cmd, "-object"); + virCommandAddArgFormat(cmd, "secret,id=%s,format=base64,file=%s",
Reference my question in previous patch about whether we should just use format=raw instead of base64
+ alias, path); + + ret = 0; + + cleanup: + VIR_FREE(alias); + VIR_FREE(path); + return ret; +}
ACK in general though 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 :|
participants (3)
-
Daniel P. Berrange
-
John Ferlan
-
Peter Krempa