[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 3/3] util: Introduce encryption APIs




On 05/19/2016 05:00 AM, Peter Krempa wrote:
> On Wed, May 18, 2016 at 19:52:32 -0400, John Ferlan wrote:
>> Introduce virCryptoHaveEncrypt and virCryptoEncryptSecret to handle
>> performing encryption.
>>
>>  virCryptoHaveEncrypt:
>>    Boolean function to determine whether the requested encryption
>>    API is available. It's expected this API will be called prior to
>>    virCryptoEncryptSecret. It will return true/false.
>>
>>  virCryptoEncryptSecret:
>>    Based on the requested encryption type, call the specific encryption
>>    API to encrypt the data.
>>
>> Currently the only algorithm support is the AES 256 CBC encryption.
>>
>> Adjust tests for the API's
>>
>> Signed-off-by: John Ferlan <jferlan redhat com>
>> ---
>>  configure.ac             |   1 +
>>  src/libvirt_private.syms |   2 +
>>  src/util/vircrypto.c     | 189 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  src/util/vircrypto.h     |  20 ++++-
>>  tests/vircryptotest.c    |  86 +++++++++++++++++++++
>>  5 files changed, 296 insertions(+), 2 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/src/util/vircrypto.c b/src/util/vircrypto.c
>> index 39a479a..05bd167 100644
>> --- a/src/util/vircrypto.c
>> +++ b/src/util/vircrypto.c
> 
> [...]
> 
>> @@ -76,3 +85,181 @@ virCryptoHashString(virCryptoHash hash,
>>  
>>      return 0;
>>  }
>> +
>> +
>> +/* virCryptoHaveEncrypt:
>> + * @algorithm: Specific encryption algorithm desired
>> + *
>> + * Expected to be called prior to virCryptoEncryptData in order
>> + * to determine whether the requested encryption option is available,
>> + * so that "other" alternatives can be taken if the algorithm is
>> + * not available.
>> + *
>> + * Returns true if we can support the encryption.
>> + */
>> +bool
>> +virCryptoHaveEncrypt(virCryptoEncrypt algorithm)
> 
> Cipher rather than Encrypt
> 

Fine -

>> +{
>> +    switch (algorithm) {
>> +
>> +    case VIR_CRYPTO_ENCRYPT_AES256CBC:
>> +#ifdef HAVE_GNUTLS_CIPHER_ENCRYPT
>> +    return true;
>> +#else
>> +    return false;
>> +#endif
>> +
>> +    case VIR_CRYPTO_ENCRYPT_NONE:
>> +    case VIR_CRYPTO_ENCRYPT_LAST:
>> +        break;
>> +    };
>> +
>> +    return false;
>> +}
>> +
>> +
>> +/* virCryptoEncryptDataAESgntuls:
>> + *
>> + * Performs the AES gnutls encryption - parameters essentially the
>> + * same as virCryptoEncryptSecret, except the libvirt algorithm is
>> + * converted to the gnutls_cipher_algorithm_t
>> + *
>> + * Same input as virCryptoEncryptData, but ensures encryption key and
>> + * initialization vector lengths are correct.
>> + *
>> + * The data buffer will be cleared as soon as it has been prepared for
>> + * encryption.
>> + *
>> + * Encrypts the @data buffer using the @enckey and if available the @iv
>> + *
>> + * Returns 0 on success with the ciphertext being filled. It is the
>> + * caller's responsibility to clear and free it. Returns -1 on failure
>> + * w/ error set.
>> + */
>> +static int
>> +virCryptoEncryptDataAESgnutls(gnutls_cipher_algorithm_t gnutls_enc_alg,
>> +                              uint8_t *enckey,
>> +                              size_t enckeylen,
>> +                              uint8_t *iv,
>> +                              size_t ivlen,
>> +                              uint8_t *data,
>> +                              size_t datalen,
>> +                              uint8_t **ciphertextret,
>> +                              size_t *ciphertextlenret)
>> +{
>> +    int rc;
>> +    size_t i;
>> +    gnutls_cipher_hd_t handle = NULL;
>> +    gnutls_datum_t enc_key;
>> +    gnutls_datum_t iv_key;
> 
> As pointed out last time, even here it won't compile with gnutls.
> 

I was more focused on getting the framework right. I'll add #ifdef
HAVE_GNUTLS_CIPHER_ENCRYPT around the virCryptoEncryptDataAESgnutls
function and around the calling spot where the #else will be a
VIR_ERR_OPERATION_UNSUPPORTED error. I did point out in my cover that
this probably should have been an RFC, but seeing as it's the setup for
the other AES patches - I just went with a v1.

>> +    uint8_t *ciphertext;
>> +    size_t ciphertextlen;
>> +
>> +    /* Allocate a padded buffer, copy in the data, padding the buffer with
                                                       ^^^^^^^^^^^^^^^^^
>> +     * the size of the padding size which is required for decryption, then
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> +     * clear the data buffer when we're done. */
>> +    ciphertextlen = VIR_ROUND_UP(datalen, 16);
>> +    if (VIR_ALLOC_N(ciphertext, ciphertextlen) < 0)
>> +        return -1;
>> +    memcpy(ciphertext, data, datalen);
>> +    for (i = datalen; i < ciphertextlen; i++)
> 
> And this still isn't documented.
> 

It was, but I'll move it to make it more obvious.

>> +        ciphertext[i] = ciphertextlen - datalen;
>> +    memset(data, 0, datalen);
> 
> The caller should ensure sanitization. Especially sinc its _NOT_ done if
> the above allocation fails. In such case the caller still needs to do
> it.
> 

Fine

>> +
>> +    /* Initialize the gnutls cipher */
>> +    enc_key.size = enckeylen;
>> +    enc_key.data = enckey;
>> +    if (iv) {
>> +        iv_key.size = ivlen;
>> +        iv_key.data = iv;
>> +    }
>> +    if ((rc = gnutls_cipher_init(&handle, gnutls_enc_alg,
>> +                                 &enc_key, &iv_key)) < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("failed to initialize cipher: '%s'"),
>> +                       gnutls_strerror(rc));
>> +        goto error;
>> +    }
>> +
>> +    /* Encrypt the data and free the memory for cipher operations */
>> +    rc = gnutls_cipher_encrypt(handle, ciphertext, ciphertextlen);
>> +    gnutls_cipher_deinit(handle);
>> +    if (rc < 0) {
>> +        virReportError(VIR_ERR_INTERNAL_ERROR,
>> +                       _("failed to encrypt the data: '%s'"),
>> +                       gnutls_strerror(rc));
>> +        goto error;
>> +    }
>> +
>> +    *ciphertextret = ciphertext;
>> +    *ciphertextlenret = ciphertextlen;
>> +    return 0;
>> +
>> + error:
>> +    VIR_DISPOSE_N(ciphertext, ciphertextlen);
>> +    return -1;
>> +}
>> +
>> +
>> +/* virCryptoEncryptData:
>> + * @algorithm: algoritm desired for encryption
>> + * @enckey: encryption key
>> + * @enckeylen: encription key length
>> + * @iv: initialization vector
>> + * @ivlen: length of initialization vector
>> + * @data: data to encrypt
>> + * @datalen: length of data
>> + * @ciphertext: stream of bytes allocated to store ciphertext
>> + * @ciphertextlen: size of the stream of bytes
>> + *
>> + * If available, attempt and return the requested encryption type
>> + * using the parameters passed.
>> + *
>> + * Returns 0 on success, -1 on failure with error set
>> + */
>> +int
>> +virCryptoEncryptData(virCryptoEncrypt algorithm,
>> +                     uint8_t *enckey,
>> +                     size_t enckeylen,
>> +                     uint8_t *iv,
>> +                     size_t ivlen,
>> +                     uint8_t *data,
>> +                     size_t datalen,
>> +                     uint8_t **ciphertext,
>> +                     size_t *ciphertextlen)
>> +{
>> +    switch (algorithm) {
>> +    case VIR_CRYPTO_ENCRYPT_AES256CBC:
>> +        if (enckeylen != 32) {
>> +            virReportError(VIR_ERR_INVALID_ARG,
>> +                           _("AES256CBC encryption invalid keylen=%zu"),
>> +                           enckeylen);
>> +            return -1;
>> +        }
>> +
>> +        if (ivlen != 16) {
>> +            virReportError(VIR_ERR_INVALID_ARG,
>> +                           _("AES246CBC initialization vector invalid len=%zu"),
> 
> AES256CBC
> 

OK

>> +                           ivlen);
>> +            return -1;
>> +        }
>> +
>> +        /*
>> +         * Encrypt the data buffer using an encryption key and
>> +         * initialization vector via the gnutls_cipher_encrypt API
>> +         * for GNUTLS_CIPHER_AES_256_CBC.
>> +         */
>> +        return virCryptoEncryptDataAESgnutls(GNUTLS_CIPHER_AES_256_CBC,
>> +                                             enckey, enckeylen, iv, ivlen,
>> +                                             data, datalen,
>> +                                             ciphertext, ciphertextlen);
>> +
>> +    case VIR_CRYPTO_ENCRYPT_NONE:
>> +    case VIR_CRYPTO_ENCRYPT_LAST:
>> +        break;
>> +    }
>> +
>> +    virReportError(VIR_ERR_INVALID_ARG,
>> +                   _("algorithm=%d is not supported"), algorithm);
>> +    return -1;
>> +}
>> diff --git a/src/util/vircrypto.h b/src/util/vircrypto.h
>> index f67d49d..7d0829e 100644
>> --- a/src/util/vircrypto.h
>> +++ b/src/util/vircrypto.h
> 
> [...]
> 
>> @@ -30,6 +30,14 @@ typedef enum {
>>      VIR_CRYPTO_HASH_LAST
>>  } virCryptoHash;
>>  
>> +
>> +typedef enum {
>> +    VIR_CRYPTO_ENCRYPT_NONE = 0,
>> +    VIR_CRYPTO_ENCRYPT_AES256CBC,
>> +
>> +    VIR_CRYPTO_ENCRYPT_LAST
>> +} virCryptoEncrypt;
> 
> This can be used for decryption too. Thus virCryptoCipher.
> 

Sure and the symbols change toe _CIPHER_ as well. That's fine.

>> +
>>  int
>>  virCryptoHashString(virCryptoHash hash,
>>                      const char *input,
> 
> [...]
> 
>> diff --git a/tests/vircryptotest.c b/tests/vircryptotest.c
>> index bfc47db..038eb7a 100644
>> --- a/tests/vircryptotest.c
>> +++ b/tests/vircryptotest.c
> 
> [...]
> 
>> @@ -56,10 +57,74 @@ testCryptoHash(const void *opaque)
>>  }
>>  
>>  
>> +struct testCryptoEncryptData {
>> +    virCryptoEncrypt algorithm;
>> +    uint8_t *input;
>> +    size_t inputlen;
>> +    const char *base64;
>> +};
>> +
>> +static int
>> +testCryptoEncrypt(const void *opaque)
>> +{
>> +    const struct testCryptoEncryptData *data = opaque;
>> +    size_t i;
>> +    uint8_t *enckey = NULL;
>> +    size_t enckeylen = 32;
>> +    uint8_t *iv = NULL;
>> +    size_t ivlen = 16;
>> +    uint8_t *ciphertext = NULL;
>> +    size_t ciphertextlen = 0;
>> +    char *actual = NULL;
>> +    int ret = -1;
>> +
>> +    if (!virCryptoHaveEncrypt(data->algorithm)) {
>> +        fprintf(stderr, "Invalid encryption algorithm=%d\n", data->algorithm);
>> +        return -1;
>> +    }
>> +
>> +    if (VIR_ALLOC_N(enckey, enckeylen) < 0 ||
>> +        VIR_ALLOC_N(iv, ivlen) < 0)
>> +        goto cleanup;
>> +
>> +    /* To be replaced by mock if I can get it to work */
> 
> Why not keep it this way? This is testing the encryption not the key
> generation.
> 

Well I was *hoping* to get the mock to work... Something covered between
patch 2 and the cover (which IIRC you don't read covers).

>> +    for (i = 0; i < enckeylen; i++)
>> +        enckey[i] = i;
>> +    for (i = 0; i < ivlen; i++)
>> +        iv[i] = i;
>> +
>> +    if (virCryptoEncryptData(data->algorithm, enckey, enckeylen, iv, ivlen,
>> +                             data->input, data->inputlen,
>> +                             &ciphertext, &ciphertextlen) < 0)
>> +        goto cleanup;
>> +
>> +    /* Comparing the encoded ciphertext would be what is desired in
>> +     * the long run and easier to read/format the expected ciphertext */
> 
> Read? nobody is going to read it if it doesn't match.
> 

OK - so I took the other route - passing in an expected cipher value.

>> +    if (!(actual = virStringEncodeBase64(ciphertext, ciphertextlen)))
>> +        goto cleanup;
>> +
>> +    if (STRNEQ(data->base64, actual)) {
>> +        fprintf(stderr, "Expected encoded encryption '%s' but got '%s'\n",
>> +                data->base64, NULLSTR(actual));
> 
> and it definitely doesn't make sense to print it. It's just binary
> garbage.
> 

I forgot the STRNEQ_NULLABLE too.. hrmph.

>> +        goto cleanup;
>> +    }
>> +
>> +    ret = 0;
>> + cleanup:
>> +    VIR_DISPOSE_N(enckey, enckeylen);
>> +    VIR_DISPOSE_N(iv, ivlen);
>> +    VIR_DISPOSE_N(ciphertext, ciphertextlen);
> 
> This isn't really necessary for demo keys. You can also lose the lenght
> variables.
> 

OK

>> +    VIR_FREE(actual);
>> +
> 
> I won't review further versions until you fix the issue with compiling
> without gnutls. I've pointed it out far too many times.
> 

I didn't expect v1 to be the last - figured it was more important to get
the structure as expected.

John


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]