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

[Gonglei <arei.gonglei@huawei.com>]

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.

Regards,
-Gonglei