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 <berra...@redhat.com>
> > ---
>
> > +++ 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 :|
