From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47242) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YzSuj-0007oj-V9 for qemu-devel@nongnu.org; Mon, 01 Jun 2015 12:50:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YzSug-00043n-LH for qemu-devel@nongnu.org; Mon, 01 Jun 2015 12:50:13 -0400 Received: from mx1.redhat.com ([209.132.183.28]:34014) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YzSug-00043Y-Cl for qemu-devel@nongnu.org; Mon, 01 Jun 2015 12:50:10 -0400 Date: Mon, 1 Jun 2015 17:50:04 +0100 From: "Daniel P. Berrange" Message-ID: <20150601165004.GB17374@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> <20150522091002.GC14428@redhat.com> <5567D14C.8010706@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <5567D14C.8010706@huawei.com> 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: Gonglei Cc: Kevin Wolf , Paolo Bonzini , Gerd Hoffmann , qemu-devel@nongnu.org, Richard Henderson On Fri, May 29, 2015 at 10:39:08AM +0800, Gonglei wrote: > On 2015/5/22 17:10, Daniel P. Berrange wrote: > > 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 > > > > Form your code, we can see that exists many duplicate code about encryption and > decryption, which have the same arguments, such as qcrypto_cipher_encrypt() > and qcrypto_cipher_decrypt(). Can we just use an argument to check the operation > is encryption or decryption, then invoke corresponding functions? In this > way, it will decrease lots of duplicate code. IIRC OpenSSL EVP api do this work > using this way. What you describe was done with the qcow2_encrypt_sectors() method, where it accepts an 'int enc' parameter to indicate whether it does encryption or decryption. In looking at the qcow2 code for encryption this is one of the things that I found rather unhelpful, as it makes it less obvious to the casual reader what is going on. Having the explicitly _encrypt() and _decrypt() APIs makes the code clearer to follow IMHO, and I don't think size difference in code is appreciable enough to counteract this benefit. 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 :|