From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59390) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yviy2-0008VG-RI for qemu-devel@nongnu.org; Fri, 22 May 2015 05:10:16 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Yvixz-0008V9-Fb for qemu-devel@nongnu.org; Fri, 22 May 2015 05:10:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54770) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Yvixz-0008Rx-51 for qemu-devel@nongnu.org; Fri, 22 May 2015 05:10:07 -0400 Date: Fri, 22 May 2015 10:10:02 +0100 From: "Daniel P. Berrange" Message-ID: <20150522091002.GC14428@redhat.com> References: <1432205817-16414-1-git-send-email-berrange@redhat.com> <1432205817-16414-5-git-send-email-berrange@redhat.com> <555E378B.2070901@twiddle.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <555E378B.2070901@twiddle.net> Subject: Re: [Qemu-devel] [PATCH 04/10] crypto: introduce generic cipher API & built-in implementation Reply-To: "Daniel P. Berrange" List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson Cc: Kevin Wolf , Paolo Bonzini , qemu-devel@nongnu.org, Gerd Hoffmann On Thu, May 21, 2015 at 12:52:43PM -0700, Richard Henderson wrote: > On 05/21/2015 03:56 AM, Daniel P. Berrange wrote: > > +QCryptoCipher *qcrypto_cipher_new(QCryptoCipherAlgorithm alg, > > + QCryptoCipherMode mode, > > + const uint8_t *key, size_t nkey, > > + Error **errp) > > +{ > > + QCryptoCipher *cipher; > > + > > + cipher = g_new0(QCryptoCipher, 1); > > + cipher->alg = alg; > > + cipher->mode = mode; > > + > > + switch (cipher->alg) { > > + case QCRYPTO_CIPHER_ALG_DES_RFB: > > + if (qcrypto_cipher_init_des_rfb(cipher, key, nkey, errp) < 0) { > > + goto error; > > + } > > + break; > > + case QCRYPTO_CIPHER_ALG_AES: > > + if (qcrypto_cipher_init_aes(cipher, key, nkey, errp) < 0) { > > + goto error; > > + } > > + break; > > + default: > > + error_setg(errp, > > + _("Unsupported cipher algorithm %d"), cipher->alg); > > + goto error; > > + } > > + > > + return cipher; > > + > > + error: > > + g_free(cipher); > > + return NULL; > > +} > > Is it really that helpful to have all of these switches, as opposed to having > one function per cipher and calling it directly? Similarly for the hashing. These switches are just an artifact of this default built-in implementation where we're jumping off to one or our two built-in crypto algorithsm. The gcrypt backend of these APIs has no such switch, since there is just a similar looking gcrypt API we directly pass through to. Similarly, if we add a backend that delegates to the Linux kernel crypto API, then we'd just be doing a more or less straight passthrough with none of these switches. > > The uses I pulled out of the later patches are like > > + if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA256, > + qiov->iov, qiov->niov, > + &data, &len, > + NULL) < 0) { > + return -EINVAL; > > + if (qcrypto_hash_base64(QCRYPTO_HASH_ALG_SHA1, > + combined_key, > + WS_CLIENT_KEY_LEN + WS_GUID_LEN, > + &accept, > + &err) < 0) { > > + cipher = qcrypto_cipher_new( > + QCRYPTO_CIPHER_ALG_DES_RFB, > + QCRYPTO_CIPHER_MODE_ECB, > + key, G_N_ELEMENTS(key), > + &err); > > + s->cipher = qcrypto_cipher_new( > + QCRYPTO_CIPHER_ALG_AES, > + QCRYPTO_CIPHER_MODE_CBC, > + keybuf, G_N_ELEMENTS(keybuf), > + &err); > > This one could have explicitly specified AES128, but you hid that behind > G_N_ELEMENTS. Which seems like obfuscation to me, but... In designing the APIs I was looking forward to uses beyond those shown in this current patch series. In particular with full disk encryption there will be a wide selection of algorithms that can be used with the implementation, so the caller of the APIs will not be passing in a fixed algorithm constant, but instead have it vary according to the data format. So on balance I think this current design is more future proof than what you suggest 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 :|