[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: |
Mon, 1 Jun 2015 17:50:04 +0100 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
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 :|
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] [PATCH 04/10] crypto: introduce generic cipher API & built-in implementation,
Daniel P. Berrange <=