> Date: Mon, 8 Nov 2021 13:33:27 +0000
> From: nia <n...@netbsd.org>
>
> 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!)
