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

["Daniel P. Berrange" <berrange@redhat.com>]

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