Re: [Qemu-devel] [PATCH 04/10] crypto: introduce generic cipher API & built-in implementation

[Daniel P. Berrange]

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 :|