Re: [Qemu-devel] [PATCH v2 4/5] crypto: add QCryptoSecret object class for password/key handling

[Daniel P. Berrange]

On Tue, Nov 24, 2015 at 11:29:55AM -0700, Eric Blake wrote:
> > +static void
> > +qcrypto_secret_load_data(QCryptoSecret *secret,
> > +                         uint8_t **output,
> > +                         size_t *outputlen,
> > +                         Error **errp)
> > +{
> > +    int fd;
> > +    char *data = NULL;
> > +    size_t offset = 0;
> > +    size_t length = 0;
> > +
> > +    *output = NULL;
> > +    *outputlen = 0;
> > +
> > +    if (secret->file) {
> > +        if (secret->data) {
> > +            error_setg(errp,
> > +                       "'file' and 'data' are mutually exclusive");
> 
> Is it worth trying to use a qapi flat union to make the mutual exclusion
> inherent in the type definition, rather than something we have to
> enforce manually?  (I've got more experience with qapi than with Object,
> so my question may be nonsensical)

Not ensure sure how you'd wire up a qapi type to a QOM property. It
is probably possible in some manner, but I not sure its particularly
compelling to do it for this.

> 
> > +            return;
> > +        }
> > +        fd = qemu_open(secret->file, O_RDONLY);
> > +        if (fd < 0) {
> > +            error_setg_errno(errp, errno,
> > +                             "Unable to open %s", secret->file);
> 
> Using error_setg_file_open() makes for a consistent message on open()
> failure.

Yep

> 
> > +            return;
> > +        }
> > +        while (length < (1024 * 1024)) { /* Limit secrets to 1 MB */
> > +            if ((length - offset) < 1024) {
> > +                length += 1024;
> > +                data = g_renew(char, data, length);
> > +            }
> > +            ssize_t ret = read(fd, data + offset, length - offset);
> > +            if (ret == 0) {
> > +                break;
> > +            }
> > +            if (ret < 0) {
> > +                error_setg_errno(errp, errno,
> > +                                 "Unable to read from %s", secret->file);
> 
> Does glib have a convenience function for reading contents of a file?

Of course, I completely forgot about g_file_get_contents() which
works just fine.


> > +static void qcrypto_secret_decrypt(QCryptoSecret *secret,
> > +                                   const uint8_t *input,
> > +                                   size_t inputlen,
> > +                                   uint8_t **output,
> > +                                   size_t *outputlen,
> > +                                   Error **errp)
> > +{
> > +    uint8_t *key = NULL, *ciphertext = NULL, *iv = NULL;
> > +    size_t keylen, ciphertextlen, ivlen;
> > +    QCryptoCipher *aes = NULL;
> > +    uint8_t *plaintext = NULL;
> > +
> > +    *output = NULL;
> > +    *outputlen = 0;
> > +
> > +    if (qcrypto_secret_lookup(secret->keyid,
> > +                              &key, &keylen,
> > +                              errp) < 0) {
> > +        goto cleanup;
> > +    }
> > +
> > +    if (keylen != 32) {
> > +        error_setg(errp, "Key should be 32 bytes in length");
> > +        goto cleanup;
> > +    }
> > +
> > +    if (!secret->iv) {
> > +        error_setg(errp, "IV is required to decrypt secret");
> > +        goto cleanup;
> > +    }
> > +
> > +    iv = (uint8_t *)g_base64_decode(secret->iv, &ivlen);
> 
> Shouldn't this be using qbase64_decode()?

Yeah, it really should

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