[libvirt] [PATCH v3 1/2] qemu: Introduce new Secret IV API's

[Daniel P. Berrange]

On Wed, May 11, 2016 at 02:47:21PM +0200, Peter Krempa wrote:
> On Thu, May 05, 2016 at 15:00:40 -0400, John Ferlan wrote:
> > New APIs:
> > 
> >   qemuDomainGetIVKeyAlias:
> >     Generate/return the secret object alias for an initialization
> >     vector (IV) secret info type. This will be saved in the secret
> >     info block. This will be called from qemuDomainSecretIVSetup.
> > 
> >   qemuDomainSecretHaveEncrypt: (private)
> >     Boolean function to determine whether the underly encryption
> >     API is available. This function will utilize a similar mechanism
> >     as the 'gnutls_rnd' did in configure.ac.
> > 
> >   qemuDomainSecretIVSetup: (private)
> >     This API handles the details of the generation of the IV secret
> >     and saves the pieces that need to be passed to qemu in order for
> >     the secret to be decrypted. The encrypted secret based upon the
> >     domain master key, an initialization vector (16 byte random value),
> >     and the stored secret. Finally, the requirement from qemu is the IV
> >     and encrypted secret are to be base64 encoded. They can be passed
> >     either directly or within a file. This implementation chooses
> >     to pass directly rather than a file.
> > 
> > Signed-off-by: John Ferlan <jferlan at redhat.com>
> > ---
> >  configure.ac           |   1 +
> >  src/qemu/qemu_alias.c  |  23 +++++++++
> >  src/qemu/qemu_alias.h  |   2 +
> >  src/qemu/qemu_domain.c | 137 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 163 insertions(+)
> > 

> > +static int ATTRIBUTE_UNUSED
> > +qemuDomainSecretIVSetup(virConnectPtr conn,
> > +                        qemuDomainObjPrivatePtr priv,
> > +                        qemuDomainSecretInfoPtr secinfo,
> > +                        const char *srcalias,
> > +                        virStorageNetProtocol protocol,
> > +                        virStorageAuthDefPtr authdef)
> > +{
> > +    int ret = -1;
> > +    int rc;
> > +    uint8_t *raw_iv = NULL;
> > +    char *secret = NULL;
> > +    uint8_t *ciphertext = NULL;
> > +    size_t secretlen = 0, ciphertextlen = 0, paddinglen;
> 
> Please one variable per line.
> 
> > +    int secretType = VIR_SECRET_USAGE_TYPE_ISCSI;
> 
> Rather than changing to RBD below I think the decision should be done in
> a single swithch.
> 
> > +    const char *protocolstr = virStorageNetProtocolTypeToString(protocol);
> > +    gnutls_cipher_hd_t handle = NULL;
> > +    gnutls_datum_t enc_key;
> > +    gnutls_datum_t iv_key;
> 
> This function is compiled even without gnutls.
> 
> > +
> > +    secinfo->type = VIR_DOMAIN_SECRET_INFO_TYPE_IV;
> > +    if (VIR_STRDUP(secinfo->s.iv.username, authdef->username) < 0)
> > +        return -1;
> > +
> > +    if (protocol == VIR_STORAGE_NET_PROTOCOL_RBD)
> > +        secretType = VIR_SECRET_USAGE_TYPE_CEPH;
> > +
> > +    if (!(secinfo->s.iv.alias = qemuDomainGetIVKeyAlias(srcalias)))
> > +        return -1;
> > +
> > +    /* Create a random initialization vector */
> > +    if (!(raw_iv = qemuDomainGenerateRandomKey(QEMU_DOMAIN_IV_KEY_LEN)))
> > +        return -1;
> > +
> > +    /* Encode the IV and save that since qemu will need it */
> 
> So the initialization vector or IV is a specific of the used cipher. I
> don't think we should use it in the names for the secret type.
> 
> Either the name of the cipher should be used or just "secret" if it's
> unlikey that qemu would add new ciphers.
> 
> > +    base64_encode_alloc((const char *)raw_iv, QEMU_DOMAIN_IV_KEY_LEN,
> > +                        &secinfo->s.iv.iv);
> > +    if (!secinfo->s.iv.iv) {
> > +        virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> > +                       _("failed to encode initialization vector"));
> > +        goto cleanup;
> > +    }
> > +
> > +    /* Grab the unencoded secret */
> > +    if (!(secret = virSecretGetSecretString(conn, protocolstr, false,
> 
> In qemuDomainSecretPlainSetup the secret is requested encoded for RBD,
> thus it was possible to use non ascii keys ...
> 
> > +                                            authdef, secretType)))
> > +        goto cleanup;
> > +
> > +    /* Allocate a padded buffer */
> > +    secretlen = strlen(secret);
> 
> ... which won't work with this.

Yep, since we're treating the secret as raw bytes, we should really
use  'uint8_t *' for 'secret' instead of 'char*' to make it obvious
you can't strlen() it. This means virSecretGetSecretString should
probably return uint8_t too, and have an size_t return for the length


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 :|