[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 04/10] crypto: introduce generic cipher API & bu
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH 04/10] crypto: introduce generic cipher API & built-in implementation |
Date: |
Fri, 22 May 2015 10:10:02 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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 :|
- [Qemu-devel] [PATCH 00/10] Consolidate crypto APIs & implementations, Daniel P. Berrange, 2015/05/21
- [Qemu-devel] [PATCH 07/10] block: convert quorum blockdrv to use crypto APIs, Daniel P. Berrange, 2015/05/21
- [Qemu-devel] [PATCH 05/10] crypto: add a gcrypt cipher implementation, Daniel P. Berrange, 2015/05/21
- [Qemu-devel] [PATCH 08/10] ui: convert VNC websockets to use crypto APIs, Daniel P. Berrange, 2015/05/21
- [Qemu-devel] [PATCH 06/10] crypto: add a nettle cipher implementation, Daniel P. Berrange, 2015/05/21