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

[Jakub Jelen <jjelen@xxxxxxxxxx>]

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


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



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?

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

> +
> +    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()?

> @@ -759,6 +1007,8 @@ int ssh_crypto_init(void)
>  {
>      size_t i;
>  
> +    (void)i;
> +

I think you should use UNUSED_VAR() here

Regards,
Jakub