[libvirt] [RFC 0/3] Master secret key support
Posted as RFC primarily to make sure there's buyin for something that will become the basis for more patches to add support for using the master secret to encrypt sensitive data using objects. This work is related to the QEMU 2.6 commit 'ac1d8878' (not a libvirt commit, rather a qemu.git commit id). Hopefully I've properly read the qemu checkin notes as they relate to key generation. One thing of note that differs from other descriptions for a master secret. Most bz's where it's described indicate using /var/lib/libvirt/qemu/ $GUEST-master.key; whereas, these patches chose a slightly different tact using the generated libDir (/var/lib/libvirt/qemu/domain-#-$GUEST/ master.key). This file will be generated in qemu process launch regardless of whether the emulator supports it or not (because we have the domain private object to access the masterKey, but not in qemuBuildCommandLine). The only odd part for me was the realizing that libDir is created after successfully completing qemuBuildCommandLine - so checking if the file exists before adding it to the command line wasn't possible, but it seems that's no different to other libDir usages. Internally, the I chose to store the secret to be used as a base64 value since 1. it's going to be saved in the domain objects XML output (for libvirtd restart) and 2. it's easy enough to decode if we do need to later on. Not sure it's appropriate to store the non encoded secret in the domain object XML file. One extra area I need help on is the capabilitiesdata setup... That is how to generate the tests/qemucapabilitiesdata/caps_2.6.0-1.replies so that the 'secret' object exists and so that if I add "<flag name='secret'>" to the .caps file I won't get a test failure since the existing one doesn't list the secrets object. I've done a bit of testing locally... Starting with running domains (both persistent and transient) then restarting libvirtd and with starting the same domains with the code running. I didn't update my qemu locally to see the -object on the command line, but the test added does show the -object added (although I it took a double take and some thinking whether /tmp/lib/domain--1-$NAME/ should be used - note the /tmp and the domain--# prefix). John Ferlan (3): qemu: Create domain master key qemu: Add capability bit for qemu secret object qemu: Introduce qemuBuildMasterKeyCommandLine src/qemu/qemu_capabilities.c | 2 + src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 67 ++++++++ src/qemu/qemu_domain.c | 175 +++++++++++++++++++++ src/qemu/qemu_domain.h | 11 ++ src/qemu/qemu_process.c | 13 ++ .../qemuxml2argvdata/qemuxml2argv-master-key.args | 23 +++ tests/qemuxml2argvdata/qemuxml2argv-master-key.xml | 30 ++++ tests/qemuxml2argvtest.c | 2 + 9 files changed, 324 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-master-key.xml -- 2.5.0
Add a masterKey to _qemuDomainObjPrivate to store a base64 encoded domain master key in order to support the ability to encrypt/decrypt sensitive data shared between libvirt and qemu. The base64 encoded value will be written to the domain XML file for consistency between domain restarts. Two APIs qemuDomainWriteMasterKeyFile and qemuDomainGetMasterKeyFilePath will manage the details of the file manipulation. The Get*Path API can be used in order to return a path to the master secret key file, which will be a combination of the domain's libDir (e.g. /var/lib/libvirt/qemu/domain-#-$NAME/) and a name 'master.key'. Use of the domain's libDir was chosen as opposed to a more generic /var/lib/libvirt/qemu/$NAME-master.key since it's a domain specific area rather than repeating issues found as a result of using the domain name in a file. The Get*Path API doesn't check if the libDir exists, it just generates the path. The Write*File API will be used to create the on disk master secret file once the domain lib infrastructure is generated during qemuProcessLaunch. The masterKey is generated as a series of 8 bit random numbers stored as a 32 byte string and then base64 encoded before saving in the domain private object. A separate function will generate the key as it's expected to be utilized by future patches to support the generation of the initialization vector. Object removal will clear and free the secret as well as check for the presence of the master key file and remove it if necessary (although it shouldn't be necessary by this point in time, but being extra safe). During process launch, the key value will additionally be stored in the domain libDir. This is what will be used to share the secret with qemu via a secret object. The secret file will only present while the domain is active (e.g. create at Launch, delete at Stop). During process stop, logic is added to check if the path to the domain libDir secret exists and then to clear the contents of the file before the directory tree is removed. The path will not exist for a domain already running prior to support being added for the master key and it'd be annoying to get errors indicating it doesn't exist when it was never created. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 9 +++ src/qemu/qemu_process.c | 13 ++++ 3 files changed, 180 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9f9fae3..507ae9e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -23,6 +23,7 @@ #include <config.h> +#include <assert.h> #include "qemu_domain.h" #include "qemu_alias.h" #include "qemu_command.h" @@ -44,6 +45,8 @@ #include "virthreadjob.h" #include "viratomic.h" #include "virprocess.h" +#include "virrandom.h" +#include "base64.h" #include "logging/log_manager.h" #include "storage/storage_driver.h" @@ -465,6 +468,151 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, } +/* qemuDomainGetMasterKeyFilePath: + * @libDir: Directory path to domain lib files + * + * Build the name of the master key file based on valid libDir path + * + * 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: + * @libDir: Directory path to domain lib files + * @masterKey: Key to write to the file + * + * Using the passed libDir and masterKey write the key to the master + * key file for the domain. + * + * Returns 0 on success, -1 on failure with error message indicating failure + */ +int +qemuDomainWriteMasterKeyFile(const char *libDir, + const char *masterKey) +{ + char *path; + int ret = -1; + + if (!(path = qemuDomainGetMasterKeyFilePath(libDir))) + return -1; + + if (virFileWriteStr(path, masterKey, 0600) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to write master key file for domain")); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(path); + return ret; +} + + +/* qemuDomainGenerateRandomKey + * @nbytes: Size in bytes of random key to generate - expect multiple of 8 + * + * Generate a random key of nbytes length and return it. + * + * Returns pointer memory containing key on success, NULL on failure + */ +static unsigned char * +qemuDomainGenerateRandomKey(int nbytes) +{ + unsigned char *key; + size_t i; + + assert((nbytes % 8) == 0); + + if (VIR_ALLOC_N(key, nbytes) < 0) + return NULL; + + /* Generate a random master key based on the nbytes passed */ + for (i = 0; i < nbytes; i++) + key[i] = virRandomBits(8); + + 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. + */ +static 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 file, if not already handled during process stop */ + if (priv->libDir && virFileExists(priv->libDir)) { + path = qemuDomainGetMasterKeyFilePath(priv->libDir); + if (path && virFileExists(path)) { + ignore_value(qemuDomainWriteMasterKeyFile(priv->libDir, "0")); + unlink(path); + } + } + VIR_FREE(path); +} + + +/* qemuDomainMasterKeyCreate: + * @priv: Pointer to the domain private object + * + * Generate and store as a base64 encoded value a random 32-byte key + * to be used as a secret shared with qemu to share sensative data. + * + * Returns: 0 on success, -1 w/ error message on failure + */ +static int +qemuDomainMasterKeyCreate(qemuDomainObjPrivatePtr priv) +{ + unsigned char *key = NULL; + + if (!(key = qemuDomainGenerateRandomKey(QEMU_DOMAIN_MASTER_KEY_LEN))) + goto error; + + /* base64 encode the key */ + base64_encode_alloc((const char *)key, QEMU_DOMAIN_MASTER_KEY_LEN, + &priv->masterKey); + memset(key, 0, QEMU_DOMAIN_MASTER_KEY_LEN); + VIR_FREE(key); + if (!priv->masterKey) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to encode master key")); + goto error; + } + + return 0; + + error: + qemuDomainMasterKeyRemove(priv); + return -1; +} + + static virClassPtr qemuDomainDiskPrivateClass; static int @@ -574,6 +722,9 @@ qemuDomainObjPrivateAlloc(void) if (!(priv->devs = virChrdevAlloc())) goto error; + if (qemuDomainMasterKeyCreate(priv) < 0) + goto error; + priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX; return priv; @@ -590,6 +741,8 @@ qemuDomainObjPrivateFree(void *data) virObjectUnref(priv->qemuCaps); + qemuDomainMasterKeyRemove(priv); + virCgroupFree(&priv->cgroup); virDomainPCIAddressSetFree(priv->pciaddrs); virDomainCCWAddressSetFree(priv->ccwaddrs); @@ -746,6 +899,9 @@ qemuDomainObjPrivateXMLFormat(virBufferPtr buf, virBufferEscapeString(buf, "<channelTargetDir path='%s'/>\n", priv->channelTargetDir); + if (priv->masterKey) + virBufferAsprintf(buf, "<masterKey>%s</masterKey>\n", priv->masterKey); + return 0; } @@ -959,6 +1115,8 @@ qemuDomainObjPrivateXMLParse(xmlXPathContextPtr ctxt, if (qemuDomainSetPrivatePathsOld(driver, vm) < 0) goto error; + priv->masterKey = virXPathString("string(./masterKey)", ctxt); + return 0; error: diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 573968c..b24acdf 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 AES key */ typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate { @@ -212,6 +213,8 @@ struct _qemuDomainObjPrivate { char *machineName; char *libDir; /* base path for per-domain files */ char *channelTargetDir; /* base path for per-domain channel targets */ + + char *masterKey; /* base64 encoded random key */ }; # define QEMU_DOMAIN_DISK_PRIVATE(disk) \ @@ -546,4 +549,10 @@ int qemuDomainSetPrivatePaths(char **domainLibDir, const char *confChannelDir, const char *domainName, int domainId); + +char *qemuDomainGetMasterKeyFilePath(const char *libDir); + +int qemuDomainWriteMasterKeyFile(const char *libDir, const char *masterKey) + ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + #endif /* __QEMU_DOMAIN_H__ */ diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c332747..0784f1c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5141,6 +5141,10 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessMakeDir(driver, vm, priv->channelTargetDir) < 0) goto cleanup; + /* Write the masterKey to the file in libDir */ + if (qemuDomainWriteMasterKeyFile(priv->libDir, priv->masterKey) < 0) + goto cleanup; + /* now that we know it is about to start call the hook if present */ if (qemuProcessStartHook(driver, vm, VIR_HOOK_QEMU_OP_START, @@ -5583,6 +5587,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, virNetDevVPortProfilePtr vport = NULL; size_t i; char *timestamp; + char *masterKeyPath = NULL; virQEMUDriverConfigPtr cfg = virQEMUDriverGetConfig(driver); qemuDomainLogContextPtr logCtxt = NULL; @@ -5668,6 +5673,13 @@ void qemuProcessStop(virQEMUDriverPtr driver, priv->monConfig = NULL; } + /* If we have a masterKeyPath, then clear the masterKey from the file + * even though it's about to be deleted, let's not leave any traces. + */ + masterKeyPath = qemuDomainGetMasterKeyFilePath(priv->libDir); + if (virFileExists(masterKeyPath)) + ignore_value(qemuDomainWriteMasterKeyFile(priv->libDir, "0")); + virFileDeleteTree(priv->libDir); virFileDeleteTree(priv->channelTargetDir); @@ -5867,6 +5879,7 @@ void qemuProcessStop(virQEMUDriverPtr driver, qemuDomainObjEndJob(driver, vm); cleanup: + VIR_FREE(masterKeyPath); if (orig_err) { virSetError(orig_err); virFreeError(orig_err); -- 2.5.0
On Mon, Mar 21, 2016 at 14:29:00 -0400, John Ferlan wrote:
Add a masterKey to _qemuDomainObjPrivate to store a base64 encoded domain master key in order to support the ability to encrypt/decrypt sensitive data shared between libvirt and qemu. The base64 encoded value will be written to the domain XML file for consistency between domain restarts.
Two APIs qemuDomainWriteMasterKeyFile and qemuDomainGetMasterKeyFilePath will manage the details of the file manipulation.
The Get*Path API can be used in order to return a path to the master secret key file, which will be a combination of the domain's libDir (e.g. /var/lib/libvirt/qemu/domain-#-$NAME/) and a name 'master.key'. Use of the domain's libDir was chosen as opposed to a more generic /var/lib/libvirt/qemu/$NAME-master.key since it's a domain specific area rather than repeating issues found as a result of using the domain name in a file. The Get*Path API doesn't check if the libDir exists, it just generates the path.
The Write*File API will be used to create the on disk master secret file once the domain lib infrastructure is generated during qemuProcessLaunch.
The masterKey is generated as a series of 8 bit random numbers stored as a 32 byte string and then base64 encoded before saving in the domain private object.
A separate function will generate the key as it's expected to be utilized by future patches to support the generation of the initialization vector.
Object removal will clear and free the secret as well as check for the presence of the master key file and remove it if necessary (although it shouldn't be necessary by this point in time, but being extra safe).
During process launch, the key value will additionally be stored in the domain libDir. This is what will be used to share the secret with qemu via a secret object. The secret file will only present while the domain is active (e.g. create at Launch, delete at Stop).
During process stop, logic is added to check if the path to the domain libDir secret exists and then to clear the contents of the file before the directory tree is removed. The path will not exist for a domain already running prior to support being added for the master key and it'd be annoying to get errors indicating it doesn't exist when it was never created.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 9 +++ src/qemu/qemu_process.c | 13 ++++ 3 files changed, 180 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9f9fae3..507ae9e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c
[...]
@@ -465,6 +468,151 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, }
+/* qemuDomainGetMasterKeyFilePath: + * @libDir: Directory path to domain lib files + * + * Build the name of the master key file based on valid libDir path + * + * 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: + * @libDir: Directory path to domain lib files + * @masterKey: Key to write to the file + * + * Using the passed libDir and masterKey write the key to the master + * key file for the domain. + * + * Returns 0 on success, -1 on failure with error message indicating failure + */ +int +qemuDomainWriteMasterKeyFile(const char *libDir,
Won't it be better to pass the domain object instead of having to extract libDir explicitly ...
+ const char *masterKey)
And the key.
+{ + char *path; + int ret = -1; + + if (!(path = qemuDomainGetMasterKeyFilePath(libDir))) + return -1; + + if (virFileWriteStr(path, masterKey, 0600) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to write master key file for domain")); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(path); + return ret; +} + + +/* qemuDomainGenerateRandomKey + * @nbytes: Size in bytes of random key to generate - expect multiple of 8 + * + * Generate a random key of nbytes length and return it. + * + * Returns pointer memory containing key on success, NULL on failure + */ +static unsigned char * +qemuDomainGenerateRandomKey(int nbytes)
size_t ? negative key lengths don't make much sense
+{ + unsigned char *key; + size_t i; + + assert((nbytes % 8) == 0);
Why? The algorithm below does not require it in any way.
+ + if (VIR_ALLOC_N(key, nbytes) < 0) + return NULL; + + /* Generate a random master key based on the nbytes passed */ + for (i = 0; i < nbytes; i++) + key[i] = virRandomBits(8); + + 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. + */ +static 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 file, if not already handled during process stop */ + if (priv->libDir && virFileExists(priv->libDir)) { + path = qemuDomainGetMasterKeyFilePath(priv->libDir); + if (path && virFileExists(path)) { + ignore_value(qemuDomainWriteMasterKeyFile(priv->libDir, "0")); + unlink(path); + } + } + VIR_FREE(path); +} + + +/* qemuDomainMasterKeyCreate: + * @priv: Pointer to the domain private object + * + * Generate and store as a base64 encoded value a random 32-byte key + * to be used as a secret shared with qemu to share sensative data. + * + * Returns: 0 on success, -1 w/ error message on failure + */ +static int +qemuDomainMasterKeyCreate(qemuDomainObjPrivatePtr priv) +{ + unsigned char *key = NULL; + + if (!(key = qemuDomainGenerateRandomKey(QEMU_DOMAIN_MASTER_KEY_LEN))) + goto error; + + /* base64 encode the key */ + base64_encode_alloc((const char *)key, QEMU_DOMAIN_MASTER_KEY_LEN, + &priv->masterKey); + memset(key, 0, QEMU_DOMAIN_MASTER_KEY_LEN); + VIR_FREE(key);
Won't this require us to decode the base64 representation every time when we will be about to encrypt any secret stuff to be passed to qemu?
+ if (!priv->masterKey) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to encode master key")); + goto error; + } + + return 0; + + error: + qemuDomainMasterKeyRemove(priv); + return -1; +} + + static virClassPtr qemuDomainDiskPrivateClass;
static int @@ -574,6 +722,9 @@ qemuDomainObjPrivateAlloc(void) if (!(priv->devs = virChrdevAlloc())) goto error;
+ if (qemuDomainMasterKeyCreate(priv) < 0) + goto error;
I think we should generate the key when we are starting the process and only if qemu supports it. That way we can check whether the key is present rather than having to check that the key is present AND that qemu supports the key feature before using it every time.
+ priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
return priv; @@ -590,6 +741,8 @@ qemuDomainObjPrivateFree(void *data)
virObjectUnref(priv->qemuCaps);
+ qemuDomainMasterKeyRemove(priv);
This should be done when we stop the process.
+ virCgroupFree(&priv->cgroup); virDomainPCIAddressSetFree(priv->pciaddrs); virDomainCCWAddressSetFree(priv->ccwaddrs);
[...]
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 573968c..b24acdf 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 AES key */
Key lenghts are usually expressed in bits.
typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate {
[...]
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c332747..0784f1c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5141,6 +5141,10 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessMakeDir(driver, vm, priv->channelTargetDir) < 0) goto cleanup;
Shouldn't we generate the key here? So that it isn't the same during the lifetime of the libvirtd process?
+ /* Write the masterKey to the file in libDir */ + if (qemuDomainWriteMasterKeyFile(priv->libDir, priv->masterKey) < 0) + goto cleanup; + /* now that we know it is about to start call the hook if present */ if (qemuProcessStartHook(driver, vm, VIR_HOOK_QEMU_OP_START,
@@ -5668,6 +5673,13 @@ void qemuProcessStop(virQEMUDriverPtr driver, priv->monConfig = NULL; }
+ /* If we have a masterKeyPath, then clear the masterKey from the file + * even though it's about to be deleted, let's not leave any traces. + */
Why?
+ masterKeyPath = qemuDomainGetMasterKeyFilePath(priv->libDir); + if (virFileExists(masterKeyPath)) + ignore_value(qemuDomainWriteMasterKeyFile(priv->libDir, "0")); + virFileDeleteTree(priv->libDir); virFileDeleteTree(priv->channelTargetDir);
On 03/22/2016 08:56 AM, Peter Krempa wrote:
On Mon, Mar 21, 2016 at 14:29:00 -0400, John Ferlan wrote:
Add a masterKey to _qemuDomainObjPrivate to store a base64 encoded domain master key in order to support the ability to encrypt/decrypt sensitive data shared between libvirt and qemu. The base64 encoded value will be written to the domain XML file for consistency between domain restarts.
Two APIs qemuDomainWriteMasterKeyFile and qemuDomainGetMasterKeyFilePath will manage the details of the file manipulation.
The Get*Path API can be used in order to return a path to the master secret key file, which will be a combination of the domain's libDir (e.g. /var/lib/libvirt/qemu/domain-#-$NAME/) and a name 'master.key'. Use of the domain's libDir was chosen as opposed to a more generic /var/lib/libvirt/qemu/$NAME-master.key since it's a domain specific area rather than repeating issues found as a result of using the domain name in a file. The Get*Path API doesn't check if the libDir exists, it just generates the path.
The Write*File API will be used to create the on disk master secret file once the domain lib infrastructure is generated during qemuProcessLaunch.
The masterKey is generated as a series of 8 bit random numbers stored as a 32 byte string and then base64 encoded before saving in the domain private object.
A separate function will generate the key as it's expected to be utilized by future patches to support the generation of the initialization vector.
Object removal will clear and free the secret as well as check for the presence of the master key file and remove it if necessary (although it shouldn't be necessary by this point in time, but being extra safe).
During process launch, the key value will additionally be stored in the domain libDir. This is what will be used to share the secret with qemu via a secret object. The secret file will only present while the domain is active (e.g. create at Launch, delete at Stop).
During process stop, logic is added to check if the path to the domain libDir secret exists and then to clear the contents of the file before the directory tree is removed. The path will not exist for a domain already running prior to support being added for the master key and it'd be annoying to get errors indicating it doesn't exist when it was never created.
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_domain.c | 158 ++++++++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.h | 9 +++ src/qemu/qemu_process.c | 13 ++++ 3 files changed, 180 insertions(+)
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9f9fae3..507ae9e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c
[...]
@@ -465,6 +468,151 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, }
+/* qemuDomainGetMasterKeyFilePath: + * @libDir: Directory path to domain lib files + * + * Build the name of the master key file based on valid libDir path + * + * 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: + * @libDir: Directory path to domain lib files + * @masterKey: Key to write to the file + * + * Using the passed libDir and masterKey write the key to the master + * key file for the domain. + * + * Returns 0 on success, -1 on failure with error message indicating failure + */ +int +qemuDomainWriteMasterKeyFile(const char *libDir,
Won't it be better to pass the domain object instead of having to extract libDir explicitly ...
+ const char *masterKey)
And the key.
Oh right - seems so easy to think this way in retrospect... When I first started it was Write the file only if the object caps existed and write it to libDir. Chosen because qemuBuildCommandLine doesn't have the priv object, rather it seems it was preferred to pass multiple elements (monConfig, monJSON, qemuCaps, audoNodeset, libDir, channelTargetDir) instead. Even vm->def is passed instead of 'vm'. Having not always been successful banging on the door of changing what was passed to functions and just passing 'vm' instead, I chose to use what I had (at least for starters). After generating and testing code, I realized libDir wasn't created until after qemuBuildCommandLine returned successfully, so I moved the Write code to Launch (of course not even thinking I had the priv object, so pass it). Initially only the libDir was passed. I added the 2nd argument when I added the write "0" on Stop, which you question below and *KeyRemove. Since this pass, I see Pavel has made other changes to the libDir processing which means I could now do the Write in qemu_command.c and I won't have issues because libDir is NULL or the directory doesn't exist (a check I had at one time before writing - does libDir exist, if not then error).
+{ + char *path; + int ret = -1; + + if (!(path = qemuDomainGetMasterKeyFilePath(libDir))) + return -1; + + if (virFileWriteStr(path, masterKey, 0600) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to write master key file for domain")); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(path); + return ret; +} + + +/* qemuDomainGenerateRandomKey + * @nbytes: Size in bytes of random key to generate - expect multiple of 8 + * + * Generate a random key of nbytes length and return it. + * + * Returns pointer memory containing key on success, NULL on failure + */ +static unsigned char * +qemuDomainGenerateRandomKey(int nbytes)
size_t ? negative key lengths don't make much sense
Sure - that's fine.
+{ + unsigned char *key; + size_t i; + + assert((nbytes % 8) == 0);
Why? The algorithm below does not require it in any way.
I think a relic of my first thoughts on how this would work, but neglected when I finalized things - consider it gone...
+ + if (VIR_ALLOC_N(key, nbytes) < 0) + return NULL; + + /* Generate a random master key based on the nbytes passed */ + for (i = 0; i < nbytes; i++) + key[i] = virRandomBits(8); + + 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. + */ +static 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 file, if not already handled during process stop */ + if (priv->libDir && virFileExists(priv->libDir)) { + path = qemuDomainGetMasterKeyFilePath(priv->libDir); + if (path && virFileExists(path)) { + ignore_value(qemuDomainWriteMasterKeyFile(priv->libDir, "0")); + unlink(path); + } + } + VIR_FREE(path); +} + + +/* qemuDomainMasterKeyCreate: + * @priv: Pointer to the domain private object + * + * Generate and store as a base64 encoded value a random 32-byte key + * to be used as a secret shared with qemu to share sensative data. + * + * Returns: 0 on success, -1 w/ error message on failure + */ +static int +qemuDomainMasterKeyCreate(qemuDomainObjPrivatePtr priv) +{ + unsigned char *key = NULL; + + if (!(key = qemuDomainGenerateRandomKey(QEMU_DOMAIN_MASTER_KEY_LEN))) + goto error; + + /* base64 encode the key */ + base64_encode_alloc((const char *)key, QEMU_DOMAIN_MASTER_KEY_LEN, + &priv->masterKey); + memset(key, 0, QEMU_DOMAIN_MASTER_KEY_LEN); + VIR_FREE(key);
Won't this require us to decode the base64 representation every time when we will be about to encrypt any secret stuff to be passed to qemu?
True - it was a coin flip - base64 encode it now or later just in the file. I can leave it un-encoded... I think I was also considering the save/restore XML logic for libvirtd restarts, but it seems I don't have to consider that any more.
+ if (!priv->masterKey) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("failed to encode master key")); + goto error; + } + + return 0; + + error: + qemuDomainMasterKeyRemove(priv); + return -1; +} + + static virClassPtr qemuDomainDiskPrivateClass;
static int @@ -574,6 +722,9 @@ qemuDomainObjPrivateAlloc(void) if (!(priv->devs = virChrdevAlloc())) goto error;
+ if (qemuDomainMasterKeyCreate(priv) < 0) + goto error;
I think we should generate the key when we are starting the process and only if qemu supports it. That way we can check whether the key is present rather than having to check that the key is present AND that qemu supports the key feature before using it every time.
Fair enough - simplifies some things. It might mean checking the caps bit twice though. I guess I saw no harm in generating the secretKey (although I do now realize this is much too early).
+ priv->migMaxBandwidth = QEMU_DOMAIN_MIG_BANDWIDTH_MAX;
return priv; @@ -590,6 +741,8 @@ qemuDomainObjPrivateFree(void *data)
virObjectUnref(priv->qemuCaps);
+ qemuDomainMasterKeyRemove(priv);
This should be done when we stop the process.
It is. Since I had allocation in *Alloc, I figured there needed to be something in *Free. It's one of those automatic reflexes. This was here before it became more apparent that the object lives for as long as the domain is defined.
+ virCgroupFree(&priv->cgroup); virDomainPCIAddressSetFree(priv->pciaddrs); virDomainCCWAddressSetFree(priv->ccwaddrs);
[...]
diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index 573968c..b24acdf 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 AES key */
Key lenghts are usually expressed in bits.
And masterKey is supposed as 32 bytes. Using "MASTER_KEY" wasn't meant to imply an sort of 32 or 64 bit "key" stored for an 'uint32' or 'uint64' (e.g. 'SIZE' vs. 'LEN' from my POV). There will eventually be an InitialzationVector which is 16 bytes.
typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate; typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr; struct _qemuDomainObjPrivate {
[...]
diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index c332747..0784f1c 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -5141,6 +5141,10 @@ qemuProcessLaunch(virConnectPtr conn, qemuProcessMakeDir(driver, vm, priv->channelTargetDir) < 0) goto cleanup;
Shouldn't we generate the key here? So that it isn't the same during the lifetime of the libvirtd process?
yeah right - I guess first pass I wasn't thinking in terms of how long the priv object lives. Pavel's changes make it a bit easier now too.
+ /* Write the masterKey to the file in libDir */ + if (qemuDomainWriteMasterKeyFile(priv->libDir, priv->masterKey) < 0) + goto cleanup; + /* now that we know it is about to start call the hook if present */ if (qemuProcessStartHook(driver, vm, VIR_HOOK_QEMU_OP_START,
@@ -5668,6 +5673,13 @@ void qemuProcessStop(virQEMUDriverPtr driver, priv->monConfig = NULL; }
+ /* If we have a masterKeyPath, then clear the masterKey from the file + * even though it's about to be deleted, let's not leave any traces. + */
Why?
When we free a key, we memset 0 on it to clear the heap. I guess I just figured writing a "0" to the file that's about to be deleted will (more or less) match that logic. You'll note the qemuDomainMasterKeyRemove also writes the "0". Thanks for the feedback - John
+ masterKeyPath = qemuDomainGetMasterKeyFilePath(priv->libDir); + if (virFileExists(masterKeyPath)) + ignore_value(qemuDomainWriteMasterKeyFile(priv->libDir, "0")); + virFileDeleteTree(priv->libDir); virFileDeleteTree(priv->channelTargetDir);
On Mon, Mar 21, 2016 at 02:29:00PM -0400, John Ferlan wrote:
Add a masterKey to _qemuDomainObjPrivate to store a base64 encoded domain master key in order to support the ability to encrypt/decrypt sensitive data shared between libvirt and qemu. The base64 encoded value will be written to the domain XML file for consistency between domain restarts.
Ohh, no, we don't want the master key to ever appear in any XML file, because that in turn leads to compromise of user data when reporting bugs. For example if the user provides the CLI args + runtime XML then you can decrypt their passwords from the CLI args. The master key must only ever be in its own file, which minimises the chance of the user ever uploading the master key for their VM with bug reports.
Two APIs qemuDomainWriteMasterKeyFile and qemuDomainGetMasterKeyFilePath will manage the details of the file manipulation.
The Get*Path API can be used in order to return a path to the master secret key file, which will be a combination of the domain's libDir (e.g. /var/lib/libvirt/qemu/domain-#-$NAME/) and a name 'master.key'. Use of the domain's libDir was chosen as opposed to a more generic /var/lib/libvirt/qemu/$NAME-master.key since it's a domain specific area rather than repeating issues found as a result of using the domain name in a file. The Get*Path API doesn't check if the libDir exists, it just generates the path.
The Write*File API will be used to create the on disk master secret file once the domain lib infrastructure is generated during qemuProcessLaunch.
The masterKey is generated as a series of 8 bit random numbers stored as a 32 byte string and then base64 encoded before saving in the domain private object.
A separate function will generate the key as it's expected to be utilized by future patches to support the generation of the initialization vector.
Object removal will clear and free the secret as well as check for the presence of the master key file and remove it if necessary (although it shouldn't be necessary by this point in time, but being extra safe).
During process launch, the key value will additionally be stored in the domain libDir. This is what will be used to share the secret with qemu via a secret object. The secret file will only present while the domain is active (e.g. create at Launch, delete at Stop).
Process launch is the place where we should *first* generate the key and store it directly into the domain libDir. Also we should be generating a new key every time the guest starts. There is no reason we need to persist the same key forever, as it only needs to be valid for lifetime of a single QEMU process. This further mimizes risks of actual user passwords leaking, as they'll be encrypted with a new throw-away key every time QEMU is started
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9f9fae3..507ae9e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -23,6 +23,7 @@
#include <config.h>
+#include <assert.h>
We have a general rule that libvirt should never assert() in its code, so don't add this. Errors should always propagate back to a virErrorPtr.
+/* qemuDomainGenerateRandomKey + * @nbytes: Size in bytes of random key to generate - expect multiple of 8 + * + * Generate a random key of nbytes length and return it. + * + * Returns pointer memory containing key on success, NULL on failure + */ +static unsigned char * +qemuDomainGenerateRandomKey(int nbytes) +{ + unsigned char *key; + size_t i; + + assert((nbytes % 8) == 0); + + if (VIR_ALLOC_N(key, nbytes) < 0) + return NULL; + + /* Generate a random master key based on the nbytes passed */ + for (i = 0; i < nbytes; i++) + key[i] = virRandomBits(8); + + return key; +}
The virRandomBits API doesn't provide cryptographically strong random data. Since we already link to gnutls, we should call out to ret = gnutls_rnd(GNUTLS_RND_RANDOM, key, nbytes); to generate the data 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/22/2016 10:08 AM, Daniel P. Berrange wrote:
On Mon, Mar 21, 2016 at 02:29:00PM -0400, John Ferlan wrote:
Add a masterKey to _qemuDomainObjPrivate to store a base64 encoded domain master key in order to support the ability to encrypt/decrypt sensitive data shared between libvirt and qemu. The base64 encoded value will be written to the domain XML file for consistency between domain restarts.
Ohh, no, we don't want the master key to ever appear in any XML file, because that in turn leads to compromise of user data when reporting bugs. For example if the user provides the CLI args + runtime XML then you can decrypt their passwords from the CLI args. The master key must only ever be in its own file, which minimises the chance of the user ever uploading the master key for their VM with bug reports.
OK - well that simplifies certain things; however, I would think that means on libvirtd restart we would then have to read the master key file in order to repopulate the priv->masterKey, right?
Two APIs qemuDomainWriteMasterKeyFile and qemuDomainGetMasterKeyFilePath will manage the details of the file manipulation.
The Get*Path API can be used in order to return a path to the master secret key file, which will be a combination of the domain's libDir (e.g. /var/lib/libvirt/qemu/domain-#-$NAME/) and a name 'master.key'. Use of the domain's libDir was chosen as opposed to a more generic /var/lib/libvirt/qemu/$NAME-master.key since it's a domain specific area rather than repeating issues found as a result of using the domain name in a file. The Get*Path API doesn't check if the libDir exists, it just generates the path.
The Write*File API will be used to create the on disk master secret file once the domain lib infrastructure is generated during qemuProcessLaunch.
The masterKey is generated as a series of 8 bit random numbers stored as a 32 byte string and then base64 encoded before saving in the domain private object.
A separate function will generate the key as it's expected to be utilized by future patches to support the generation of the initialization vector.
Object removal will clear and free the secret as well as check for the presence of the master key file and remove it if necessary (although it shouldn't be necessary by this point in time, but being extra safe).
During process launch, the key value will additionally be stored in the domain libDir. This is what will be used to share the secret with qemu via a secret object. The secret file will only present while the domain is active (e.g. create at Launch, delete at Stop).
Process launch is the place where we should *first* generate the key and store it directly into the domain libDir.
Also we should be generating a new key every time the guest starts. There is no reason we need to persist the same key forever, as it only needs to be valid for lifetime of a single QEMU process. This further mimizes risks of actual user passwords leaking, as they'll be encrypted with a new throw-away key every time QEMU is started
It didn't dawn on me when first going through this that the priv was only generated once at define time. But yeah, now that that realization is fresh in my mind, I certainly see the point. I guess I was initially thinking that the object was generated later, but I never revisited once it became more obvious that the private object was generated when the domain was defined. I will move it...
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9f9fae3..507ae9e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -23,6 +23,7 @@
#include <config.h>
+#include <assert.h>
We have a general rule that libvirt should never assert() in its code, so don't add this. Errors should always propagate back to a virErrorPtr.
OK - although it is used today in virsh/vsh and remote_driver...
+/* qemuDomainGenerateRandomKey + * @nbytes: Size in bytes of random key to generate - expect multiple of 8 + * + * Generate a random key of nbytes length and return it. + * + * Returns pointer memory containing key on success, NULL on failure + */ +static unsigned char * +qemuDomainGenerateRandomKey(int nbytes) +{ + unsigned char *key; + size_t i; + + assert((nbytes % 8) == 0); + + if (VIR_ALLOC_N(key, nbytes) < 0) + return NULL; + + /* Generate a random master key based on the nbytes passed */ + for (i = 0; i < nbytes; i++) + key[i] = virRandomBits(8); + + return key; +}
The virRandomBits API doesn't provide cryptographically strong random data. Since we already link to gnutls, we should call out to
ret = gnutls_rnd(GNUTLS_RND_RANDOM, key, nbytes);
Ahhh.. I wasn't aware of that API in gnutls - I was searching on "other" strings using cscope. Thanks for the feedback - John
On Wed, Mar 23, 2016 at 08:36:30AM -0400, John Ferlan wrote:
On 03/22/2016 10:08 AM, Daniel P. Berrange wrote:
On Mon, Mar 21, 2016 at 02:29:00PM -0400, John Ferlan wrote:
Add a masterKey to _qemuDomainObjPrivate to store a base64 encoded domain master key in order to support the ability to encrypt/decrypt sensitive data shared between libvirt and qemu. The base64 encoded value will be written to the domain XML file for consistency between domain restarts.
Ohh, no, we don't want the master key to ever appear in any XML file, because that in turn leads to compromise of user data when reporting bugs. For example if the user provides the CLI args + runtime XML then you can decrypt their passwords from the CLI args. The master key must only ever be in its own file, which minimises the chance of the user ever uploading the master key for their VM with bug reports.
OK - well that simplifies certain things; however, I would think that means on libvirtd restart we would then have to read the master key file in order to repopulate the priv->masterKey, right?
Yes, that's correct.
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 9f9fae3..507ae9e 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -23,6 +23,7 @@
#include <config.h>
+#include <assert.h>
We have a general rule that libvirt should never assert() in its code, so don't add this. Errors should always propagate back to a virErrorPtr.
OK - although it is used today in virsh/vsh and remote_driver...
Using it in virsh is ok as that's a client app. We shouldn't use it in the remote_driver though - I'd not noticed that actually. 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 capability bit for the qemu secret object Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + 2 files changed, 3 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; -- 2.5.0
Using the masterKey and if the capability exists build an -object secret command line to pass a masterKey file to qemu. The qemuBuildHasMasterKey is expected to be reused by other objects which would need to know not only if the masterKey has been provided, but if the secret object can be used for their own purposes. Additionally, generate qemuDomainGetMasterKeyAlias 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. Although the path to the domain libDir and the masterKey file are created after qemuBuildCommandLine completes successfully, it's still possible to generate the path and use it. No different than code paths that use the domain libDir to create socket files (e.g. monitor.sock). Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 67 ++++++++++++++++++++++ src/qemu/qemu_domain.c | 17 ++++++ src/qemu/qemu_domain.h | 2 + .../qemuxml2argvdata/qemuxml2argv-master-key.args | 23 ++++++++ tests/qemuxml2argvdata/qemuxml2argv-master-key.xml | 30 ++++++++++ tests/qemuxml2argvtest.c | 2 + 6 files changed, 141 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_command.c b/src/qemu/qemu_command.c index eb02553..04e75fd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -151,6 +151,70 @@ 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 path will not exist yet as domainLibDir + * and the master secret file gets created after we've successfully + * built the command line + */ + 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 @@ -9220,6 +9284,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/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 507ae9e..53d6705 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -468,6 +468,23 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, } +/* 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; +} + + /* qemuDomainGetMasterKeyFilePath: * @libDir: Directory path to domain lib files * diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h index b24acdf..60dbe82 100644 --- a/src/qemu/qemu_domain.h +++ b/src/qemu/qemu_domain.h @@ -550,6 +550,8 @@ int qemuDomainSetPrivatePaths(char **domainLibDir, const char *domainName, int domainId); +char *qemuDomainGetMasterKeyAlias(void); + char *qemuDomainGetMasterKeyFilePath(const char *libDir); int qemuDomainWriteMasterKeyFile(const char *libDir, const char *masterKey) 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 4fac77d..a9d5975 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1897,6 +1897,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 Mon, Mar 21, 2016 at 14:29:02 -0400, John Ferlan wrote:
Using the masterKey and if the capability exists build an -object secret command line to pass a masterKey file to qemu. The qemuBuildHasMasterKey is expected to be reused by other objects which would need to know not only if the masterKey has been provided, but if the secret object can be used for their own purposes.
Additionally, generate qemuDomainGetMasterKeyAlias 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.
Although the path to the domain libDir and the masterKey file are created after qemuBuildCommandLine completes successfully, it's still possible to generate the path and use it. No different than code paths that use the domain libDir to create socket files (e.g. monitor.sock).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 67 ++++++++++++++++++++++ src/qemu/qemu_domain.c | 17 ++++++ src/qemu/qemu_domain.h | 2 + .../qemuxml2argvdata/qemuxml2argv-master-key.args | 23 ++++++++ tests/qemuxml2argvdata/qemuxml2argv-master-key.xml | 30 ++++++++++ tests/qemuxml2argvtest.c | 2 + 6 files changed, 141 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_command.c b/src/qemu/qemu_command.c index eb02553..04e75fd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -151,6 +151,70 @@ 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); +}
This doesn't seem to be terribly useful. Especially since you have to check for priv->masterKey anyways.
+ + +/** + * 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;
Here is the correct place to generate the key. I don't really see the benefit of doing it even for qemu that does not support this feature.
+ } + + if (!(alias = qemuDomainGetMasterKeyAlias())) + return -1; + + /* Get the path... NB, the path will not exist yet as domainLibDir + * and the master secret file gets created after we've successfully + * built the command line + */ + 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
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 507ae9e..53d6705 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -468,6 +468,23 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, }
+/* qemuDomainGetMasterKeyAlias: + * + * Generate and return the masterKey alias + * + * Returns NULL or a string containing the master key alias + */ +char * +qemuDomainGetMasterKeyAlias(void)
You've added src/qemu/qemu_alias.c, so this belongs there.
+{ + char *alias; + + ignore_value(VIR_STRDUP(alias, "masterKey0")); + + return alias; +} + + /* qemuDomainGetMasterKeyFilePath: * @libDir: Directory path to domain lib files *
Peter
On 03/22/2016 09:01 AM, Peter Krempa wrote:
On Mon, Mar 21, 2016 at 14:29:02 -0400, John Ferlan wrote:
Using the masterKey and if the capability exists build an -object secret command line to pass a masterKey file to qemu. The qemuBuildHasMasterKey is expected to be reused by other objects which would need to know not only if the masterKey has been provided, but if the secret object can be used for their own purposes.
Additionally, generate qemuDomainGetMasterKeyAlias 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.
Although the path to the domain libDir and the masterKey file are created after qemuBuildCommandLine completes successfully, it's still possible to generate the path and use it. No different than code paths that use the domain libDir to create socket files (e.g. monitor.sock).
Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/qemu/qemu_command.c | 67 ++++++++++++++++++++++ src/qemu/qemu_domain.c | 17 ++++++ src/qemu/qemu_domain.h | 2 + .../qemuxml2argvdata/qemuxml2argv-master-key.args | 23 ++++++++ tests/qemuxml2argvdata/qemuxml2argv-master-key.xml | 30 ++++++++++ tests/qemuxml2argvtest.c | 2 + 6 files changed, 141 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_command.c b/src/qemu/qemu_command.c index eb02553..04e75fd 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -151,6 +151,70 @@ 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); +}
This doesn't seem to be terribly useful. Especially since you have to check for priv->masterKey anyways.
I thought it might be useful for other CommmandLine functions that may someday need to determine if the secret -object is supported before making decisions whether to use it or go with the existing mechanism. But given other feedback, this'll probably be moot anyway.
+ + +/** + * 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;
Here is the correct place to generate the key. I don't really see the benefit of doing it even for qemu that does not support this feature.
Cannot disagree, but my conundrum was not having the priv object in qemuBuildCommandLine, so choose something (libDir) that I can use. This'll all get flipped in the next pass, but figured I'd at least try to explain what was rattling through the rafters while making an initial pass.
+ } + + if (!(alias = qemuDomainGetMasterKeyAlias())) + return -1; + + /* Get the path... NB, the path will not exist yet as domainLibDir + * and the master secret file gets created after we've successfully + * built the command line + */ + 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
[...]
diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 507ae9e..53d6705 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -468,6 +468,23 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo, }
+/* qemuDomainGetMasterKeyAlias: + * + * Generate and return the masterKey alias + * + * Returns NULL or a string containing the master key alias + */ +char * +qemuDomainGetMasterKeyAlias(void)
You've added src/qemu/qemu_alias.c, so this belongs there.
Coin flip - I chose here mainly because qemu_alias.c is primarily dealing with Device aliases, but also because qemuDomainStorageAlias is here. IDC either way and will move it there. Tks - John
+{ + char *alias; + + ignore_value(VIR_STRDUP(alias, "masterKey0")); + + return alias; +} + + /* qemuDomainGetMasterKeyFilePath: * @libDir: Directory path to domain lib files *
Peter
participants (3)
-
Daniel P. Berrange -
John Ferlan -
Peter Krempa