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

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

> Date: Mon, 8 Nov 2021 13:33:27 +0000
> From: nia <nia%NetBSD.org@localhost>
> 
> On Sat, Nov 06, 2021 at 09:42:04AM +0000, Taylor R Campbell wrote:
> > 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?
> 
> I decided I don't want to add a new library dependency to libcrypt
> because external software will be linking against it and it's
> surprising for those use cases.
> 
> Do we want to use libcrypt here, though? It would add extra
> string processing and it also stores hashes secrets in a static
> variable, which may be a problem for cgd because we need the hash
> to be secret.

What I had in mind was linking against a common libargon2 in /lib.
But maybe the engineering cost isn't worth however many hundred
kilobytes or so the extra copy of libargon2 incurs.


The PBKDF2 calibration code does a second run to verify the timing,
and starts over if it isn't reproducible.  Maybe argon2id_calibrate
should too?  (Not a blocker.)


Have you tested a release build, and maybe running through sysinst?
LGTM by code inspection other than some minor nits.


> +	if (sysctl(mib, __arraycount(mib),
> +	    &ncpuonline, &ncpuonline_len, NULL, 0) < 0) {

sysctl(...) == -1, not sysctl(...) < 0

> +	if (getrlimit(RLIMIT_AS, &rlim) < 0)
> +		return usermem64;

same

> +	const uint64_t usermem = get_usermem();

This is in units of 2^10 bytes too, right?  Comment, here and on
definition of get_usermem?

> +		if (clock_gettime(CLOCK_MONOTONIC, &tp1) == -1)
> +			goto error;
> [...]
> +error:
> +	errx(EXIT_FAILURE, "failed to calculate hash parameters");

Would be nice to show the errno message, for the branches where errno
is set.

> +error_a2:
> +	errx(EXIT_FAILURE,
> +	    "failed to calculate Argon2 hash, error code %d\n", err);

No \n in err message.

> +		argon2id_calibrate(BITS2BYTES(keylen), DEFAULT_SALTLEN,
> +		    &kg->kg_iterations, &kg->kg_memory, &kg->kg_parallelism);

Might be nice to have some feedback to the console that cgdconfig(8)
is calibrating, maybe if `-v' is passed.  (Same with the PBKDF2
calibration!)