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

[Jakub Jelen <jjelen@xxxxxxxxxx>]

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.

Regards,
-- 
Jakub Jelen
Senior Software Engineer
Security Technologies
Red Hat, Inc.