[libvirt] [PATCH v2 0/4] Add a domain masterKey secret for qemu,

v1: http://www.redhat.com/archives/libvir-list/2016-March/msg01206.html Patch 1 is already ACK'd. I assume this code won't go into 1.3.3, but would hopefully be early in 1.3.4 and I didn't want to break up the capability bits across releases... Differences to v1 - Patch 2 is new - it's taking the virUUIDGenerateRandomBytes and making it generic since we'll use it in Patch 3 (it already opens/reads from /dev/urandom, so I figured it'd be better to share than cut, copy, paste). - Patch 3 has changes from review: * Less comments in qemuDomainGetMasterKeyFilePath * Master key no longer base64 encoded to be written (or read). Instead the Write code will open, truncate, and write the secret directly. The Read code will read the secret directly * The fallback algorithm for key generation uses virGenerateRandomBytes * Changed 'masterKey' from "char *" to "uint8_t *" and added the masterKeyLen - Patch 4 changes in order to tell qemu the format of the file is 'raw'. Also affects test .args file Removed references to encode/decode, adjusted commit messages. Ran through Coverity checker... happy... Created a domain that would pass/read the file... Killed libvirtd, restarted and read the masterKey file properly. Also ensured the #else of the secret generation compiled... John Ferlan (4): qemu: Add capability bit for qemu secret object util: Introduce virGenerateRandomBytes qemu: Create domain master key qemu: Introduce qemuBuildMasterKeyCommandLine src/libvirt_private.syms | 1 + 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 | 252 +++++++++++++++++++++ src/qemu/qemu_domain.h | 15 ++ src/qemu/qemu_process.c | 11 + src/util/virutil.c | 36 +++ src/util/virutil.h | 3 + src/util/viruuid.c | 30 +-- 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 + 17 files changed, 469 insertions(+), 29 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.xml -- 2.5.5

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 2823843..5d09dc8 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.5

On Tue, Mar 29, 2016 at 07:11:33PM -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 :|

Using the existing virUUIDGenerateRandomBytes, move API to virutil.c and add it to libvirt_private.syms. This will be used as a fallback for generating a domain master key. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virutil.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 3 +++ src/util/viruuid.c | 30 +----------------------------- 4 files changed, 41 insertions(+), 29 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7c44047..3d54c39 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2422,6 +2422,7 @@ virEnumToString; virFindFCHostCapableVport; virFindSCSIHostByPCI; virFormatIntDecimal; +virGenerateRandomBytes; virGetDeviceID; virGetDeviceUnprivSGIO; virGetEnvAllowSUID; diff --git a/src/util/virutil.c b/src/util/virutil.c index b401f8d..c55f6f6 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2669,3 +2669,39 @@ virMemoryMaxValue(bool capped) else return LLONG_MAX; } + + +/** + * virGenerateRandomBytes + * @buf: Pointer to location to store bytes + * @buflen: Number of bytes to store + * + * Generate a stream of random bytes into @buf of size @buflen + */ +int +virGenerateRandomBytes(unsigned char *buf, + size_t buflen) +{ + int fd; + + if ((fd = open("/dev/urandom", O_RDONLY)) < 0) + return errno; + + while (buflen > 0) { + ssize_t n; + + if ((n = read(fd, buf, buflen)) <= 0) { + if (errno == EINTR) + continue; + VIR_FORCE_CLOSE(fd); + return n < 0 ? errno : ENODATA; + } + + buf += n; + buflen -= n; + } + + VIR_FORCE_CLOSE(fd); + + return 0; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index b121de0..a398b38 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -254,6 +254,9 @@ unsigned long long virMemoryLimitTruncate(unsigned long long value); bool virMemoryLimitIsSet(unsigned long long value); unsigned long long virMemoryMaxValue(bool ulong); +int virGenerateRandomBytes(unsigned char *buf, size_t buflen) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; + /** * VIR_ASSIGN_IS_OVERFLOW: * @rvalue: value that is checked (evaluated twice) diff --git a/src/util/viruuid.c b/src/util/viruuid.c index 615d419..e2d9fbf 100644 --- a/src/util/viruuid.c +++ b/src/util/viruuid.c @@ -53,34 +53,6 @@ VIR_LOG_INIT("util.uuid"); static unsigned char host_uuid[VIR_UUID_BUFLEN]; static int -virUUIDGenerateRandomBytes(unsigned char *buf, - int buflen) -{ - int fd; - - if ((fd = open("/dev/urandom", O_RDONLY)) < 0) - return errno; - - while (buflen > 0) { - int n; - - if ((n = read(fd, buf, buflen)) <= 0) { - if (errno == EINTR) - continue; - VIR_FORCE_CLOSE(fd); - return n < 0 ? errno : ENODATA; - } - - buf += n; - buflen -= n; - } - - VIR_FORCE_CLOSE(fd); - - return 0; -} - -static int virUUIDGeneratePseudoRandomBytes(unsigned char *buf, int buflen) { @@ -108,7 +80,7 @@ virUUIDGenerate(unsigned char *uuid) if (uuid == NULL) return -1; - if ((err = virUUIDGenerateRandomBytes(uuid, VIR_UUID_BUFLEN))) { + if ((err = virGenerateRandomBytes(uuid, VIR_UUID_BUFLEN))) { char ebuf[1024]; VIR_WARN("Falling back to pseudorandom UUID," " failed to generate random bytes: %s", -- 2.5.5

On Tue, Mar 29, 2016 at 07:11:34PM -0400, John Ferlan wrote:
Using the existing virUUIDGenerateRandomBytes, move API to virutil.c and add it to libvirt_private.syms.
This will be used as a fallback for generating a domain master key.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virutil.c | 36 ++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 3 +++ src/util/viruuid.c | 30 +----------------------------- 4 files changed, 41 insertions(+), 29 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 7c44047..3d54c39 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2422,6 +2422,7 @@ virEnumToString; virFindFCHostCapableVport; virFindSCSIHostByPCI; virFormatIntDecimal; +virGenerateRandomBytes; virGetDeviceID; virGetDeviceUnprivSGIO; virGetEnvAllowSUID; diff --git a/src/util/virutil.c b/src/util/virutil.c index b401f8d..c55f6f6 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2669,3 +2669,39 @@ virMemoryMaxValue(bool capped) else return LLONG_MAX; } + + +/** + * virGenerateRandomBytes + * @buf: Pointer to location to store bytes + * @buflen: Number of bytes to store + * + * Generate a stream of random bytes into @buf of size @buflen + */ +int +virGenerateRandomBytes(unsigned char *buf, + size_t buflen) +{ + int fd; + + if ((fd = open("/dev/urandom", O_RDONLY)) < 0) + return errno; + + while (buflen > 0) { + ssize_t n; + + if ((n = read(fd, buf, buflen)) <= 0) { + if (errno == EINTR) + continue; + VIR_FORCE_CLOSE(fd); + return n < 0 ? errno : ENODATA; + } + + buf += n; + buflen -= n; + } + + VIR_FORCE_CLOSE(fd); + + return 0; +} diff --git a/src/util/virutil.h b/src/util/virutil.h index b121de0..a398b38 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -254,6 +254,9 @@ unsigned long long virMemoryLimitTruncate(unsigned long long value); bool virMemoryLimitIsSet(unsigned long long value); unsigned long long virMemoryMaxValue(bool ulong);
+int virGenerateRandomBytes(unsigned char *buf, size_t buflen) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +
Please call this virRandomBytes() and put it in virrandom.{c,h} ACK if you make that change before applying. 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 and masterKeyLen to _qemuDomainObjPrivate to store a random domain master key and its length 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) Open (create/trunc) the masterKey path and write the masterKey qemuDomainMasterKeyReadFile: Using the master key path, open/read the file, and store the masterKey and masterKeyLen. 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 mechanism using virGenerateRandomBytes qemuDomainMasterKeyRemove: Remove traces of the master key, remove the *KeyFilePath qemuDomainMasterKeyCreate: Generate the domain master key and save 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 file and restore the masterKey and masterKeyLen. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 252 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 15 +++ src/qemu/qemu_process.c | 11 +++ 3 files changed, 278 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 53cb2b6..99a91d2 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,252 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, } +/* qemuDomainGetMasterKeyFilePath: + * @libDir: Directory path to domain lib files + * + * Generate a path to the domain master key file for libDir. + * It's up to the caller to handle checking if path exists. + * + * 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.aes", NULL); +} + + +/* qemuDomainWriteMasterKeyFile: + * @priv: pointer to domain private object + * + * Get the desired path to the masterKey file and store it in the path. + * + * Returns 0 on success, -1 on failure with error message indicating failure + */ +static int +qemuDomainWriteMasterKeyFile(qemuDomainObjPrivatePtr priv) +{ + char *path; + int fd = -1; + int ret = -1; + + if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir))) + return -1; + + if ((fd = open(path, O_WRONLY|O_TRUNC|O_CREAT, 0600)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to open domain master key file for write")); + goto cleanup; + } + + if (safewrite(fd, priv->masterKey, priv->masterKeyLen) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to write master key file for domain")); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FORCE_CLOSE(fd); + 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 contents of the file saving 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; + int fd = -1; + uint8_t *masterKey = NULL; + ssize_t masterKeyLen = 0; + + /* 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 error; + } + + if ((fd = open(path, O_RDONLY)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to open domain master key file for read")); + goto error; + } + + if (VIR_ALLOC_N(masterKey, 1024) < 0) + goto error; + + if ((masterKeyLen = saferead(fd, masterKey, 1024)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to read domain master key file")); + goto error; + } + + if (masterKeyLen != QEMU_DOMAIN_MASTER_KEY_LEN) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid master key read, size=%zd"), masterKeyLen); + goto error; + } + + masterKey[masterKeyLen] = '\0'; + ignore_value(VIR_REALLOC_N_QUIET(masterKey, masterKeyLen + 1)); + + priv->masterKey = masterKey; + priv->masterKeyLen = masterKeyLen; + + VIR_FORCE_CLOSE(fd); + VIR_FREE(path); + + return 0; + + error: + if (masterKeyLen > 0) + memset(masterKey, 0, masterKeyLen); + VIR_FREE(masterKey); + + VIR_FORCE_CLOSE(fd); + VIR_FREE(path); + + return -1; +} + + +/* 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 uint8_t * +qemuDomainGenerateRandomKey(size_t nbytes) +{ + uint8_t *key; + int ret; + + 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 + /* If we don't have gnutls, we will generate a less cryptographically + * strong master key from /dev/urandom. + */ + if ((ret = virGenerateRandomBytes(key, nbytes)) < 0) { + virReportSystemError(ret, "%s", _("failed to generate master key")); + VIR_FREE(key); + return NULL; + } +#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 contents */ + memset(priv->masterKey, 0, priv->masterKeyLen); + VIR_FREE(priv->masterKey); + priv->masterKeyLen = 0; + + /* Delete the master key file */ + path = qemuDomainGetMasterKeyFilePath(priv->libDir); + 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 'raw' in a file 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) +{ + /* If we don't have the capability, then do nothing. */ + if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET)) + return 0; + + if (!(priv->masterKey = + qemuDomainGenerateRandomKey(QEMU_DOMAIN_MASTER_KEY_LEN))) + goto error; + + priv->masterKeyLen = QEMU_DOMAIN_MASTER_KEY_LEN; + + 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 4d581d7..0e82a7d 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -148,6 +148,7 @@ struct qemuDomainJobObj { typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver, virDomainObjPtr vm); +# define QEMU_DOMAIN_MASTER_KEY_LEN 32 /* 32 bytes for 256 bit random key */ typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate { @@ -215,6 +216,11 @@ struct _qemuDomainObjPrivate { char *machineName; char *libDir; /* base path for per-domain files */ char *channelTargetDir; /* base path for per-domain channel targets */ + + /* random masterKey and length for encryption (not to be saved in our */ + /* private XML) - need to restore at process reconnect */ + uint8_t *masterKey; + size_t masterKeyLen; }; # define QEMU_DOMAIN_DISK_PRIVATE(disk) \ @@ -549,4 +555,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 205d9ae..6da7360 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -3509,6 +3509,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); @@ -5117,6 +5121,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); @@ -5803,6 +5811,9 @@ void qemuProcessStop(virQEMUDriverPtr driver, priv->monConfig = NULL; } + /* Remove the master key */ + qemuDomainMasterKeyRemove(priv); + virFileDeleteTree(priv->libDir); virFileDeleteTree(priv->channelTargetDir); -- 2.5.5

On Tue, Mar 29, 2016 at 07:11:35PM -0400, John Ferlan wrote:
Add a masterKey and masterKeyLen to _qemuDomainObjPrivate to store a random domain master key and its length 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) Open (create/trunc) the masterKey path and write the masterKey
qemuDomainMasterKeyReadFile: Using the master key path, open/read the file, and store the masterKey and masterKeyLen. 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 mechanism using virGenerateRandomBytes
qemuDomainMasterKeyRemove: Remove traces of the master key, remove the *KeyFilePath
qemuDomainMasterKeyCreate: Generate the domain master key and save 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 file and restore the masterKey and masterKeyLen.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 252 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 15 +++ src/qemu/qemu_process.c | 11 +++ 3 files changed, 278 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 53cb2b6..99a91d2 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,252 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, }
+/* qemuDomainGetMasterKeyFilePath: + * @libDir: Directory path to domain lib files + * + * Generate a path to the domain master key file for libDir. + * It's up to the caller to handle checking if path exists. + * + * 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.aes", NULL); +} + + +/* qemuDomainWriteMasterKeyFile: + * @priv: pointer to domain private object + * + * Get the desired path to the masterKey file and store it in the path. + * + * Returns 0 on success, -1 on failure with error message indicating failure + */ +static int +qemuDomainWriteMasterKeyFile(qemuDomainObjPrivatePtr priv) +{ + char *path; + int fd = -1; + int ret = -1; + + if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir))) + return -1; + + if ((fd = open(path, O_WRONLY|O_TRUNC|O_CREAT, 0600)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to open domain master key file for write")); + goto cleanup; + } + + if (safewrite(fd, priv->masterKey, priv->masterKeyLen) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to write master key file for domain")); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FORCE_CLOSE(fd); + 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 contents of the file saving 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; + int fd = -1; + uint8_t *masterKey = NULL; + ssize_t masterKeyLen = 0; + + /* 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 error; + } + + if ((fd = open(path, O_RDONLY)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to open domain master key file for read")); + goto error; + } + + if (VIR_ALLOC_N(masterKey, 1024) < 0) + goto error; + + if ((masterKeyLen = saferead(fd, masterKey, 1024)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("unable to read domain master key file")); + goto error; + } + + if (masterKeyLen != QEMU_DOMAIN_MASTER_KEY_LEN) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("invalid master key read, size=%zd"), masterKeyLen); + goto error; + } + + masterKey[masterKeyLen] = '\0'; + ignore_value(VIR_REALLOC_N_QUIET(masterKey, masterKeyLen + 1));
We're storing the key as raw binary data, so NUL terminating it doesn't make any sense - it may already contain embedded NULs and so nothing should try strlen() on it. So just drop that extra NUL byte,
+ priv->masterKey = masterKey; + priv->masterKeyLen = masterKeyLen; + + VIR_FORCE_CLOSE(fd); + VIR_FREE(path); + + return 0; + + error: + if (masterKeyLen > 0) + memset(masterKey, 0, masterKeyLen); + VIR_FREE(masterKey); + + VIR_FORCE_CLOSE(fd); + VIR_FREE(path); + + return -1; +} +
ACK with that fix above 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 :|

If the -object secret capability exists, then get the path to the 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 45c5398..01b6a26 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=raw,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 @@ -9235,6 +9300,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..de030eb --- /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=raw,\ +file=/tmp/lib/domain--1-QEMUGuest1/master-key.aes \ +-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.5

On Tue, Mar 29, 2016 at 07:11:36PM -0400, John Ferlan wrote:
If the -object secret capability exists, then get the path to the 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
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 :|

On 03/29/2016 07:11 PM, John Ferlan wrote:
v1: http://www.redhat.com/archives/libvir-list/2016-March/msg01206.html
Patch 1 is already ACK'd. I assume this code won't go into 1.3.3, but would hopefully be early in 1.3.4 and I didn't want to break up the capability bits across releases...
Differences to v1
- Patch 2 is new - it's taking the virUUIDGenerateRandomBytes and making it generic since we'll use it in Patch 3 (it already opens/reads from /dev/urandom, so I figured it'd be better to share than cut, copy, paste).
- Patch 3 has changes from review:
* Less comments in qemuDomainGetMasterKeyFilePath
* Master key no longer base64 encoded to be written (or read). Instead the Write code will open, truncate, and write the secret directly. The Read code will read the secret directly
* The fallback algorithm for key generation uses virGenerateRandomBytes
* Changed 'masterKey' from "char *" to "uint8_t *" and added the masterKeyLen
- Patch 4 changes in order to tell qemu the format of the file is 'raw'. Also affects test .args file
Removed references to encode/decode, adjusted commit messages.
Ran through Coverity checker... happy...
Created a domain that would pass/read the file... Killed libvirtd, restarted and read the masterKey file properly. Also ensured the #else of the secret generation compiled...
John Ferlan (4): qemu: Add capability bit for qemu secret object util: Introduce virGenerateRandomBytes qemu: Create domain master key qemu: Introduce qemuBuildMasterKeyCommandLine
src/libvirt_private.syms | 1 + 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 | 252 +++++++++++++++++++++ src/qemu/qemu_domain.h | 15 ++ src/qemu/qemu_process.c | 11 + src/util/virutil.c | 36 +++ src/util/virutil.h | 3 + src/util/viruuid.c | 30 +-- 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 + 17 files changed, 469 insertions(+), 29 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.xml
Made requested adjustments and pushed. Working through the IV support now... Tks for the review, John
participants (2)
-
Daniel P. Berrange
-
John Ferlan