[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 4/5] crypto: add QCryptoSecret object class f
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [PATCH v3 4/5] crypto: add QCryptoSecret object class for password/key handling |
Date: |
Tue, 8 Dec 2015 09:49:40 -0700 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 |
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
> + *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.
> + 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);
> +
> +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 found some style or efficiency things you might want to touch up, but
no actual bugs that would prevent this patch from being usable as-is.
Reviewed-by: Eric Blake <address@hidden>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
- Re: [Qemu-devel] [PATCH v3 4/5] crypto: add QCryptoSecret object class for password/key handling,
Eric Blake <=