From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39810) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1IhV-0000pk-9R for qemu-devel@nongnu.org; Tue, 24 Nov 2015 13:52:26 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1a1IhQ-0002Vb-Op for qemu-devel@nongnu.org; Tue, 24 Nov 2015 13:52:25 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40976) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1a1IhQ-0002VX-HB for qemu-devel@nongnu.org; Tue, 24 Nov 2015 13:52:20 -0500 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (Postfix) with ESMTPS id 449D76566B for ; Tue, 24 Nov 2015 18:52:19 +0000 (UTC) Date: Tue, 24 Nov 2015 18:52:15 +0000 From: "Daniel P. Berrange" Message-ID: <20151124185215.GA5793@redhat.com> References: <1448377362-18117-1-git-send-email-berrange@redhat.com> <1448377362-18117-5-git-send-email-berrange@redhat.com> <5654ACA3.6030907@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5654ACA3.6030907@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 4/5] crypto: add QCryptoSecret object class for password/key handling Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake Cc: qemu-devel@nongnu.org, Markus Armbruster 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 :|