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

[Richard Henderson <rth@twiddle.net>]

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.

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


r~