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@xxxxxxxxxx>
>> ---
>> 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);
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list