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

[Daniel P. Berrange]

On Tue, Mar 29, 2016 at 10:33:34AM -0400, John Ferlan wrote:
> 
> 
> [...]
> 
> >>  
> >> +/* qemuDomainGetMasterKeyFilePath:
> >> + * @libDir: Directory path to domain lib files
> >> + *
> >> + * This API will generate a path of the domain's libDir (e.g.
> >> + * (/var/lib/libvirt/qemu/domain-#-$NAME/) and a name 'master.key'.
> >> + *
> >> + * This API will check and fail if the libDir is NULL as that results
> >> + * in an invalid path generated.
> >> + *
> >> + * This API does not check if the resulting path exists, that is left
> >> + * up to the caller.
> >> + *
> >> + * Returns path to memory containing the name of the file. It is up to the
> >> + * caller to free; otherwise, NULL on failure.
> >> + */
> >> +char *
> >> +qemuDomainGetMasterKeyFilePath(const char *libDir)
> >> +{
> >> +    if (!libDir) {
> >> +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> >> +                       _("invalid path for master key file"));
> >> +        return NULL;
> >> +    }
> >> +    return virFileBuildPath(libDir, "master.key", NULL);
> >> +}
> > 
> > How about calling this master-key.aes to make it explicit what this
> > key is intended to be used with.
> > 
> > 
> 
> OK
> 
> >> +/* qemuDomainWriteMasterKeyFile:
> >> + * @priv: pointer to domain private object
> >> + *
> >> + * Get the desired path to the masterKey file, base64 encode the masterKey,
> >> + * and store it in the file.
> > 
> > The QEMU secrets code is happy to get data in either raw or base64
> > format. I wonder if there's a compelling reason to use base64 instead
> > of just writing it out as raw bytes.
> > 
> 
> No compelling reason comes to mind - one extra level of obscurity
> mostly. One less place to have an error in the Write/Read functions for
> base64 conversions.
> 
> [...]
> 
> >> +static char *
> >> +qemuDomainGenerateRandomKey(size_t nbytes)
> >> +{
> >> +    char *key;
> >> +#if HAVE_GNUTLS_CRYPTO_H
> >> +    int ret;
> >> +#else
> >> +    size_t i;
> >> +#endif
> >> +
> >> +    if (VIR_ALLOC_N(key, nbytes) < 0)
> >> +        return NULL;
> >> +
> >> +#if HAVE_GNUTLS_CRYPTO_H
> >> +    /* Generate a master key using gnutls if possible */
> >> +    if ((ret = gnutls_rnd(GNUTLS_RND_RANDOM, key, nbytes)) < 0) {
> >> +        virReportError(VIR_ERR_INTERNAL_ERROR,
> >> +                       _("failed to generate master key, ret=%d"), ret);
> >> +        VIR_FREE(key);
> >> +        return NULL;
> >> +    }
> >> +#else
> >> +    /* Generate a less cryptographically strong master key */
> >> +    for (i = 0; i < nbytes; i++)
> >> +        key[i] = virRandomBits(8);
> > 
> > It is probably better to just read nbytes from /dev/urandom
> > directly, as that's much closer to what gnutls_rnd would
> > do as compared to virRandomBits.
> > 
> 
> OK
> 
> >> +#endif
> >> +
> >> +    return key;
> >> +}
> >> +
> >> +
> >> +/* qemuDomainMasterKeyRemove:
> >> + * @priv: Pointer to the domain private object
> >> + *
> >> + * Remove the traces of the master key, clear the heap, clear the file,
> >> + * delete the file.
> >> + */
> >> +void
> >> +qemuDomainMasterKeyRemove(qemuDomainObjPrivatePtr priv)
> >> +{
> >> +    char *path = NULL;
> >> +
> >> +    if (!priv->masterKey)
> >> +        return;
> >> +
> >> +    /* Clear the heap */
> >> +    memset(priv->masterKey, 0, QEMU_DOMAIN_MASTER_KEY_LEN);
> >> +    VIR_FREE(priv->masterKey);
> >> +
> >> +    /* Clear and remove the master key file */
> >> +    path = qemuDomainGetMasterKeyFilePath(priv->libDir);
> >> +    if (path && virFileExists(path)) {
> >> +        /* Write a "0" to the file even though we're about to delete it */
> >> +        ignore_value(virFileWriteStr(path, "0", 0600));
> > 
> > In the src/storage/storage_backend.c we have a fnuction that can use
> > scrub to wipe out a file. We should probably put that function into
> > src/util/virfile.c as virFileWipe() or something like that.
> 
> Given Peter's continued objection, should we just skip this. IDC either
> way, but don't
> 
> If it was desired, then I assume you are looking for just something that
> will move/rename virStorageBackendWipeLocal (adjusting params), right?

Ok lets just ignore it - we can easily add it later if desired.


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|