Re: [PATCH] argon2 key generation method for cgdconfig(8)

[Taylor R Campbell <riastradh%NetBSD.org@localhost>]

> Date: Sat, 6 Nov 2021 00:20:58 +0000
> From: nia <nia%NetBSD.org@localhost>
> 
> > Why dup what you just created?  Why not kg->kg_key = bits_new(raw,
> > keylen)?  This looks like a (minor) memory leak.
> 
> This is what the existing code for pkcs5_pbkdf2/sha1 does. I believe
> that the returned pointer (raw) is immediately freed after being
> compared by the consumer.

Oh, I see -- it saves the copy in kg->kg_key, and returns ret to the
caller; missed the latter part.

> --- sbin/cgdconfig/Makefile	1 Jul 2016 22:50:09 -0000	1.15
> +++ sbin/cgdconfig/Makefile	6 Nov 2021 00:17:47 -0000
> [...]
> +LDADD+=		-Wl,-Bstatic
> +PROGDPLIBS+=	argon2 ${ARGON2DIR}/lib/libargon2
> +LDADD+=		-Wl,-Bdynamic

Err...  I don't think this will do what you want it to do.  The effect
will presumably be to add something like

	-Wl,-Bstatic -Wl,-Bdynamic ... -largon2

to the linker command line eventually, because PROGDPLIBS gets
processed much later on.

That said, since we already argon2 logic as part of libcrypt, does it
make sense to have another copy in cgdconfig?

I guess the main issue is with pthreads.  Maybe we can find a way
around this with non-threaded weak aliases in libargon2 (maybe
argon2_thread_create/join), so applications can override them with
strong symbols that use pthreads but out of the box libcrypt.so
doesn't require libpthread?

> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sbin/cgdconfig/argon2_utils.c	6 Nov 2021 00:17:48 -0000
> [...]
> +	mem = usermem / 100000;

What units are these in?  Maybe add a comment explaining so the number
100000 is a little more obvious?

> +	/* Increase 'time' until we reach a second */
> +
> +	delta.tv_sec = 0;
> +	delta.tv_usec = 0;
> +
> +	if (getrusage(RUSAGE_SELF, &ru1) == -1)
> +		goto error;
> +
> +	for (; delta.tv_sec < 1 && time < ARGON2_MAX_TIME; ++time) {
> +		if ((err = argon2_hash(time, mem, ncpus,
> +		    tmp_pwd, sizeof(tmp_pwd),
> +		    salt, saltlen,
> +		    key, keylen,
> +		    NULL, 0,
> +		    Argon2_id, ARGON2_VERSION_NUMBER)) != ARGON2_OK) {
> +			goto error_a2;
> +		}
> +		if (getrusage(RUSAGE_SELF, &ru2) == -1)
> +			goto error;
> +		timersub(&ru2.ru_utime, &ru1.ru_utime, &delta);
> +	}

Slightly confused by this logic.  It looks like we increase time
linearly, and then stop when the _entire sequence of trials_ has
exceeded 1sec.  Shouldn't we increase time (say) exponentially, and
stop when _one trial_ has exceeded one second?

> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ sbin/cgdconfig/argon2_utils.h	6 Nov 2021 00:17:48 -0000
> [...]
> +void	argon2id_calibrate(size_t, size_t, size_t *, size_t *, size_t *);

Need #include <stddef.h> for size_t.