Re: [PATCH 2/2] libgcrypt: Implement chacha20-poly1305@xxxxxxxxxxx cipher using libgcrypt

[Jussi Kivilinna <jussi.kivilinna@xxxxxx>]

Hello,

On 10.12.2019 16.47, Jakub Jelen wrote:
> On Mon, 2019-12-09 at 19:49 +0200, Jussi Kivilinna wrote:
>> Hello,
>>
>> On 9.12.2019 15.20, Jakub Jelen wrote:
>>> Hello.
>>> Thank you for the good work. I ran your commits through CI, but
>>> there
>>> seems to be some issues with the new test you added not only with
>>> other
>>> crypto backends. Can you check the results and see why it does not
>>> work?
>>>
>>> https://gitlab.com/jjelen/libssh-mirror/pipelines/101684690
>>>
>>
>> New test tried to reused existing key from aes256-cbc test, but that 
>> obviously was not going to work. I've fixed test to use proper size
>> key.
>>
>>> I also added few minor comments inline below. Throughout the code I
>>> also noticed that some of the one-line blocks are not in the
>>> braces,
>>> which is also recommended by our coding guidelines:
>>>
>>> https://gitlab.com/libssh/libssh-mirror/blob/master/README.CodingStyle#L254
>>>
>>
>> Ok, fixed for v2.
>>
>> I see that libgcrypt.c has other coding style mismatches that could
>> be 
>> fixed. Would coding style fixing patches be accepted, or is old code
>> cleaned only as part of other changes?
> 
> Hello,
> unfortunately, old code does not always follow the same style.
> Reformatting whole files does not make sense, but when touching some
> functions, it makes sense to reformat it in separate commit before
> doing your change, such as:
> 
> https://gitlab.com/libssh/libssh-mirror/commit/23c837f4
> 
>>>
>>> On Sat, 2019-12-07 at 19:17 +0200, Jussi Kivilinna wrote:
>>>> @@ -278,6 +278,9 @@ if (GCRYPT_FOUND)
>>>>          set(HAVE_GCRYPT_ECC 1)
>>>>          set(HAVE_ECC 1)
>>>>      endif (GCRYPT_VERSION VERSION_GREATER "1.4.6")
>>>> +    if (NOT GCRYPT_VERSION VERSION_LESS "1.7.0")
>>>
>>> Would not it be more readable to write GCRYPT_VERSION
>>> VERSION_GREATER_EQUAL?
>>
>> Yes, it would. However VERSION_GREATER_EQUAL is available only in 
>> CMake 3.7 or later. That's newer than CMake version listed in
>> current 
>> 'INSTALL'.
> 
> Thank you for clarification. In that case, it is fine.
> 
>>>> +static int chacha20_set_encrypt_key(struct ssh_cipher_struct
>>>> *cipher,
>>>> +                                    void *key,
>>>> +                                    void *IV)
>>>> +{
>>>> +    struct chacha20_poly1305_keysched *ctx;
>>>> +    uint8_t *u8key = key;
>>>> +    (void)IV;
>>>
>>> Please, use UNUSED_PARAM() macro rather than the void cast here.
>>>
>>
>> Ok, fixed for v2.
>>
>>>> +
>>>> +    if (cipher->chacha20_schedule == NULL) {
>>>> +        ctx = malloc(sizeof *ctx);
>>>> +        if (ctx == NULL){
>>>> +            return -1;
>>>> +        }
>>>> +        memset(ctx, 0, sizeof *ctx);
>>>
>>> Can this be simplified using calloc()?
>>
>> Yes, it can; fixed for v2.
>>
>>>> @@ -759,6 +1007,8 @@ int ssh_crypto_init(void)
>>>>  {
>>>>      size_t i;
>>>>  
>>>> +    (void)i;
>>>> +
>>>
>>> I think you should use UNUSED_VAR() here
>>
>> Ok, fixed for v2.
> 
> Hello,
> did you send the v2 patch or would you like to open a MR directly on
> gitlab to check the CI results directly?> 
> https://gitlab.com/libssh/libssh-mirror/merge_requests
> 
> I would like to work on the implementation for OpenSSL once this will
> be in.
I decided to wait for feedback on CMake/VERSION_GREATER_EQUAL so did 
not send v2 yet. 

I've opened merge request with updated commits,
   https://gitlab.com/libssh/libssh-mirror/merge_requests/75

-Jussi