[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/3] qemu: Create domain master key



On Thu, Mar 24, 2016 at 13:53:20 -0400, John Ferlan wrote:
> Add a masterKey to _qemuDomainObjPrivate to store a random domain master
> key in order to support the ability to encrypt/decrypt sensitive data
> shared between libvirt and qemu. The key will be base64 encoded and written
> to a file to be used by the command line building code to share with qemu.
> 
> New API's from this patch:
> 
>   qemuDomainGetMasterKeyFilePath:
>     Return a path to where the key is located
> 
>   qemuDomainWriteMasterKeyFile: (private)
>     Base64 the masterKey and write to the *KeyFilePath
> 
>   qemuDomainMasterKeyReadFile:
>     Using the *KeyFilePath, open/read the file, base64 decode, and
>     store in masterKey. Expected use only from qemuProcessReconnect
> 
>   qemuDomainGenerateRandomKey: (private)
>     Generate a random key using available algorithms
> 
>     The key is generated either from the gnutls_rnd function if it
>     exists or a less cryptographically strong mechanisum as a series of 8
>     bit random numbers from virRandomBits stored as a byte string.
> 
>   qemuDomainMasterKeyRemove:
>     Remove traces of the master key, remove the *KeyFilePath
> 
>   qemuDomainMasterKeyCreate:
>     Generate the key and save a base64 encoded value of the key
>     in the location returned by qemuDomainGetMasterKeyFilePath.
> 
>     This API will first ensure the QEMU_CAPS_OBJECT_SECRET is set
>     in the capabilities. If not, then there's no need to generate
>     the secret or file.
> 
> The creation of the key will be attempted from qemuProcessPrepareHost
> once the libDir directory structure exists.
> 
> The removal of the key will handled from qemuProcessStop just prior
> to deleting the libDir tree.
> 
> Since the key will not be written out to the domain object XML file,
> the qemuProcessReconnect will read the saved, decode, and restore
> the masterKey value after a series of checks.
> 
> Signed-off-by: John Ferlan <jferlan redhat com>
> ---
>  src/qemu/qemu_domain.c  | 269 ++++++++++++++++++++++++++++++++++++++++++++++++
>  src/qemu/qemu_domain.h  |  13 +++
>  src/qemu/qemu_process.c |  11 ++
>  3 files changed, 293 insertions(+)
> 
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 53cb2b6..fecf668 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c

[...]

> @@ -465,6 +471,269 @@ qemuDomainJobInfoToParams(qemuDomainJobInfoPtr jobInfo,
>  }
>  
>  
> +/* qemuDomainGetMasterKeyFilePath:
> + * @libDir: Directory path to domain lib files
> + *
> + * This API will generate a path of the domain's libDir (e.g.
> + * (/var/lib/libvirt/qemu/domain-#-$NAME/) and a name 'master.key'.
> + *
> + * This API will check and fail if the libDir is NULL as that results
> + * in an invalid path generated.
> + *
> + * This API does not check if the resulting path exists, that is left
> + * up to the caller.
> + *
> + * Returns path to memory containing the name of the file. It is up to the
> + * caller to free; otherwise, NULL on failure.

Whoah, docs longer than the code itself.

> + */
> +char *
> +qemuDomainGetMasterKeyFilePath(const char *libDir)
> +{
> +    if (!libDir) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("invalid path for master key file"));
> +        return NULL;

And the code is rather self-explanatory.

> +    }
> +    return virFileBuildPath(libDir, "master.key", NULL);
> +}
> +
> +
> +/* qemuDomainWriteMasterKeyFile:
> + * @priv: pointer to domain private object
> + *
> + * Get the desired path to the masterKey file, base64 encode the masterKey,
> + * and store it in the file.
> + *
> + * Returns 0 on success, -1 on failure with error message indicating failure
> + */
> +static int
> +qemuDomainWriteMasterKeyFile(qemuDomainObjPrivatePtr priv)
> +{
> +    char *path;
> +    char *base64Key = NULL;
> +    int ret = -1;
> +
> +    if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir)))
> +        return -1;
> +
> +    /* base64 encode the key to store it */
> +    base64_encode_alloc(priv->masterKey, QEMU_DOMAIN_MASTER_KEY_LEN,
> +                        &base64Key);
> +    if (!base64Key) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("failed to encode master key"));
> +        goto cleanup;
> +    }
> +
> +    if (virFileWriteStr(path, base64Key, 0600) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("failed to write master key file for domain"));
> +        goto cleanup;
> +    }
> +
> +    ret = 0;
> +
> + cleanup:
> +    if (base64Key) {
> +       /* clear heap, free memory */
> +        memset(base64Key, 0, strlen(base64Key));
> +        VIR_FREE(base64Key);
> +    }
> +
> +    VIR_FREE(path);
> +
> +    return ret;
> +}
> +
> +
> +/* qemuDomainMasterKeyReadFile:
> + * @priv: pointer to domain private object
> + *
> + * Expected to be called during qemuProcessReconnect once the domain
> + * libDir has been generated through qemuStateInitialize calling
> + * virDomainObjListLoadAllConfigs which will restore the libDir path
> + * to the domain private object.
> + *
> + * This function will get the path to the master key file and if it
> + * exists, it will read the file, base64 decode the read value, and
> + * save it in priv->masterKey.
> + *
> + * Once the file exists, the validity checks may cause failures; however,
> + * if the file doesn't exist or the capability doesn't exist, we just
> + * return (mostly) quietly.
> + *
> + * Returns 0 on success or lack of capability
> + *        -1 on failure with error message indicating failure
> + */
> +int
> +qemuDomainMasterKeyReadFile(qemuDomainObjPrivatePtr priv)
> +{
> +    char *path;
> +    char *base64Key = NULL;
> +    int base64KeySize = 0;
> +    char *masterKey = NULL;
> +    size_t masterKeySize = 0;
> +    int ret = -1;
> +
> +    /* If we don't have the capability, then do nothing. */
> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET))
> +        return 0;
> +
> +    if (!(path = qemuDomainGetMasterKeyFilePath(priv->libDir)))
> +        return -1;
> +
> +    if (!virFileExists(path)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("domain master key file doesn't exist in %s"),
> +                       priv->libDir);
> +        goto cleanup;
> +    }
> +
> +    /* Read the base64 encooded secret from the file, decode it, and
> +     * store in the domain private object.
> +     */
> +    if ((base64KeySize = virFileReadAll(path, 1024, &base64Key)) < 0)
> +        goto cleanup;
> +
> +    if (!base64_decode_alloc(base64Key, base64KeySize,
> +                             &masterKey, &masterKeySize)) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> +                       _("invalid base64 in domain master key file"));
> +        goto cleanup;
> +    }
> +
> +    if (!masterKey || masterKeySize != QEMU_DOMAIN_MASTER_KEY_LEN) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("invalid encoded master key read, size=%zd"),
> +                       masterKeySize);
> +        goto cleanup;
> +    }
> +
> +    priv->masterKey = masterKey;
> +    masterKey = NULL;

so this is NULL now and masterKeySize > 0

> +
> +    ret = 0;
> +
> + cleanup:
> +    if (masterKeySize > 0)
> +        memset(masterKey, 0, masterKeySize);

... memset(NULL, 0, 32);

This crashes on success path.

> +    VIR_FREE(masterKey);
> +
> +    if (base64KeySize > 0)
> +        memset(base64Key, 0, base64KeySize);
> +    VIR_FREE(base64Key);
> +
> +    VIR_FREE(path);
> +
> +    return ret;
> +}
> +
> +
> +/* qemuDomainGenerateRandomKey
> + * @nbytes: Size in bytes of random key to generate
> + *
> + * Generate a random key of nbytes length and return it.
> + *
> + * Since the gnutls_rnd could be missing, provide an alternate less
> + * secure mechanism to at least have something.
> + *
> + * Returns pointer memory containing key on success, NULL on failure
> + */
> +static char *
> +qemuDomainGenerateRandomKey(size_t nbytes)
> +{
> +    char *key;
> +#if HAVE_GNUTLS_CRYPTO_H
> +    int ret;
> +#else
> +    size_t i;
> +#endif
> +
> +    if (VIR_ALLOC_N(key, nbytes) < 0)
> +        return NULL;
> +
> +#if HAVE_GNUTLS_CRYPTO_H
> +    /* Generate a master key using gnutls if possible */
> +    if ((ret = gnutls_rnd(GNUTLS_RND_RANDOM, key, nbytes)) < 0) {
> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> +                       _("failed to generate master key, ret=%d"), ret);
> +        VIR_FREE(key);
> +        return NULL;
> +    }
> +#else
> +    /* Generate a less cryptographically strong master key */
> +    for (i = 0; i < nbytes; i++)
> +        key[i] = virRandomBits(8);
> +#endif
> +
> +    return key;
> +}
> +
> +
> +/* qemuDomainMasterKeyRemove:
> + * @priv: Pointer to the domain private object
> + *
> + * Remove the traces of the master key, clear the heap, clear the file,
> + * delete the file.
> + */
> +void
> +qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv)
> +{
> +    char *path = NULL;
> +
> +    if (!priv->masterKey)
> +        return;
> +
> +    /* Clear the heap */
> +    memset(priv->masterKey, 0, QEMU_DOMAIN_MASTER_KEY_LEN);
> +    VIR_FREE(priv->masterKey);
> +
> +    /* Clear and remove the master key file */
> +    path = qemuDomainGetMasterKeyFilePath(priv->libDir);
> +    if (path && virFileExists(path)) {
> +        /* Write a "0" to the file even though we're about to delete it */

This still doesn't make any sense. The filesystem needs to guarantee
this anyways. This is just a false sense of security.

> +        ignore_value(virFileWriteStr(path, "0", 0600));
> +        unlink(path);
> +    }
> +    VIR_FREE(path);
> +}
> +
> +
> +/* qemuDomainMasterKeyCreate:
> + * @priv: Pointer to the domain private object
> + *
> + * As long as the underlying qemu has the secret capability,
> + * generate and store as a base64 encoded value a random 32-byte
> + * key to be used as a secret shared with qemu to share sensitive data.
> + *
> + * Returns: 0 on success, -1 w/ error message on failure
> + */
> +int
> +qemuDomainMasterKeyCreate(qemuDomainObjPrivatePtr priv)
> +{
> +    char *key = NULL;
> +
> +    /* If we don't have the capability, then do nothing. */
> +    if (!virQEMUCapsGet(priv->qemuCaps, QEMU_CAPS_OBJECT_SECRET))
> +        return 0;
> +
> +    if (!(key = qemuDomainGenerateRandomKey(QEMU_DOMAIN_MASTER_KEY_LEN)))
> +        goto error;
> +
> +    priv->masterKey = key;
> +    key = NULL;

key isn't touched after the previous line, thus it doesn't make much
sense to clear that variable.

> +
> +    if (qemuDomainWriteMasterKeyFile(priv) < 0)
> +        goto error;
> +
> +    return 0;
> +
> + error:
> +    qemuDomainMasterKeyRemove(priv);
> +    return -1;
> +}
> +
> +
>  static virClassPtr qemuDomainDiskPrivateClass;
>  
>  static int
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 573968c..f85f17f 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -147,6 +147,7 @@ struct qemuDomainJobObj {
>  typedef void (*qemuDomainCleanupCallback)(virQEMUDriverPtr driver,
>                                            virDomainObjPtr vm);
>  
> +# define QEMU_DOMAIN_MASTER_KEY_LEN 32  /* For a 32-byte random masterKey */

256 bit.

>  typedef struct _qemuDomainObjPrivate qemuDomainObjPrivate;
>  typedef qemuDomainObjPrivate *qemuDomainObjPrivatePtr;
>  struct _qemuDomainObjPrivate {

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]