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

[Jussi Kivilinna <jussi.kivilinna@xxxxxx>]

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?

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

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

> 
> Regards,
> Jakub
> 

Thanks,
-Jussi