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

[Daniel P. Berrange]

On Tue, Dec 08, 2015 at 09:49:40AM -0700, Eric Blake wrote:
> On 11/27/2015 09:30 AM, Daniel P. Berrange wrote:
> > Introduce a new QCryptoSecret object class which will be used
> > for providing passwords and keys to other objects which need
> > sensitive credentials.
> > 
> 
> > More examples are shown in the updated docs.
> > 
> > Signed-off-by: Daniel P. Berrange <address@hidden>
> > ---
> 
> > +++ b/crypto/secret.c
> 
> > +static void
> > +qcrypto_secret_load_data(QCryptoSecret *secret,
> > +                         uint8_t **output,
> > +                         size_t *outputlen,
> > +                         Error **errp)
> > +{
> 
> > +        if (!g_file_get_contents(secret->file, &data, &length, &gerr)) {
> > +            error_setg(errp,
> > +                       "Unable to read %s: %s",
> > +                       secret->file, gerr->message);
> > +            g_error_free(gerr);
> > +            return;
> > +        }
> > +        if (length) {
> > +            /* Even though data is raw 8-bit, so may contain
> > +             * arbitrary NULs, ensure it is explicitly NUL
> > +             * terminated */
> > +            *output = g_renew(uint8_t, data, length + 1);
> > +            (*output)[length] = '\0';
> 
> These two lines are dead code. g_file_get_contents() guarantees that on
> success, contents is malloc'd large enough and that contents[length] == 0.
> 
> https://developer.gnome.org/glib/stable/glib-File-Utilities.html#g-file-get-contents

Yep, I just checked the source too and it matches the docs

> 
> > +            *outputlen = length;
> > +        } else {
> > +            error_setg(errp, "Secret file %s is empty",
> > +                       secret->file);
> 
> Is there any technical reason why we must forbid a 0-length password?
> (Sometimes, having the empty string as a password can be a useful for
> development tests).  I'm not opposed to rejecting it, especially if
> doing so now avoids a more cryptic error message later because there is
> indeed a technical reason; but just want to make sure it is not an
> arbitrary limitation.

It was fairly arbitrary, motivated by a desire to give an error
message if user accidentally pointed to a bad file, but based
on your point about empty passwords I'll remove this and leave
it upto the callers to decide if they'll accept empty passwords.

> 
> > +            g_free(data);
> > +        }
> > +    } else if (secret->data) {
> > +        *outputlen = strlen(secret->data);
> > +        *output = g_new(uint8_t, *outputlen + 1);
> > +        memcpy(*output, secret->data, *outputlen + 1);
> 
> These two lines could be shortened to:
> *output = g_strdup(secret->data);

Yes, indeed, I'll fix that.

> 
> > +
> > +static void qcrypto_secret_decrypt(QCryptoSecret *secret,
> > +                                   const uint8_t *input,
> > +                                   size_t inputlen,
> > +                                   uint8_t **output,
> > +                                   size_t *outputlen,
> > +                                   Error **errp)
> > +{
> 
> > +    if (secret->format == QCRYPTO_SECRET_FORMAT_BASE64) {
> > +        ciphertext = qbase64_decode((const gchar*)input,
> > +                                    inputlen,
> > +                                    &ciphertextlen,
> > +                                    errp);
> > +        if (!ciphertext) {
> > +            goto cleanup;
> > +        }
> > +        plaintext = g_new0(uint8_t, ciphertextlen + 1);
> > +    } else {
> > +        ciphertextlen = inputlen;
> > +        plaintext = g_new0(uint8_t, inputlen + 1);
> 
> g_new0(uint8_t, value) is the same as g_malloc0(value); I don't know if
> it is worth the distinction.  But not worth a respin on its own.

I just prefer to always use g_new0 so it is explicit what type we're
using, though I wish glib's allocators worked like libvirt's so it
got the compiler to provide the correct type.


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