> Date: Fri, 5 Nov 2021 16:25:05 +0000
> From: nia <n...@netbsd.org>
>
> This patch adds an "argon2id" password-based key generation method
> to cgdconfig(8).
Cool, thanks for working on this!
(For future patches: please use the `-p' option with cvs diff!)
> +++ lib/Makefile 5 Nov 2021 15:41:41 -0000
> @@ -54,6 +54,10 @@
> SUBDIR+= libnvmm
> .endif
>
> +.if (${MKARGON2} != "no")
> +SUBDIR+= ../external/apache2/argon2/lib/libargon2
> +.endif
> [...]
> --- external/apache2/Makefile 12 Oct 2021 17:24:36 -0000 1.4
> +++ external/apache2/Makefile 5 Nov 2021 15:41:41 -0000
> @@ -2,6 +2,10 @@
>
> .include <bsd.own.mk>
>
> +.if (${MKARGON2} != "no")
> +SUBDIR+= argon2
> +.endif
> +
It looks like we'll descend into external/apache2/argon2 twice this
way. Am I mistaken? Is this intentional?
> --- sbin/cgdconfig/Makefile 1 Jul 2016 22:50:09 -0000 1.15
> +++ sbin/cgdconfig/Makefile 5 Nov 2021 15:41:43 -0000
> [...]
> +ARGON2DIR= ${NETBSDSRCDIR}/external/apache2/argon2
> +ARGON2OBJDIR!= cd ${ARGON2DIR}/lib/libargon2 && ${PRINTOBJDIR}
> +CPPFLAGS+=
> -I${NETBSDSRCDIR}/external/apache2/argon2/dist/phc-winner-argon2/include
> +CPPFLAGS+= -DHAVE_ARGON2
> +LDADD+= -Wl,-Bstatic
> +LDADD+= -L${ARGON2OBJDIR} -largon2
> +LDADD+= -Wl,-Bdynamic
> +LDADD+= -pthread
> +DPADD+= ${ARGON2OBJDIR}/libargon2.a ${LIBPTHREAD}
Can this be made to use LIBDPLIBS, or are there too many moving parts
here?
> RCS file: sbin/cgdconfig/argon2_utils.c
> diff -N sbin/cgdconfig/argon2_utils.c
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ sbin/cgdconfig/argon2_utils.c 5 Nov 2021 15:41:43 -0000
> @@ -0,0 +1,165 @@
> +/* $NetBSD$ */
Missing __RCSID in body of .c file?
> + uint8_t tmp_pwd[17];
> + char tmp_encoded[512];
Where do these magic constants come from? Can they be named or made
more obvious?
> + char encoded[512];
Is it necessary to pass this in? We're not using it; can we pass in
null instead so argon2 doesn't bother to encode it?
> RCS file: sbin/cgdconfig/argon2_utils.h
> diff -N sbin/cgdconfig/argon2_utils.h
> --- /dev/null 1 Jan 1970 00:00:00 -0000
> +++ sbin/cgdconfig/argon2_utils.h 5 Nov 2021 15:41:43 -0000
Missing include guard.
> --- sbin/cgdconfig/cgdconfig.8 30 Apr 2021 21:07:34 -0000 1.50
> +++ sbin/cgdconfig/cgdconfig.8 5 Nov 2021 15:41:43 -0000
> [...]
> +.It memory Ar integer
> +Memory consumption in kilobytes.
I wonder whether this can nicely be made a little more obvious, like
memory 123M;
or something.
I wonder whether cgdconfig -g/-G could have an option to specify the
percentage of RAM.
> --- sbin/cgdconfig/cgdconfig.c 16 Jun 2021 23:22:08 -0000 1.52
> +++ sbin/cgdconfig/cgdconfig.c 5 Nov 2021 15:41:43 -0000
> [...]
> + ret = bits_new(raw, keylen);
> + kg->kg_key = bits_dup(ret);
Why dup what you just created? Why not kg->kg_key = bits_new(raw,
keylen)? This looks like a (minor) memory leak.
Would be nice if we had automatic tests for cgdconfig -g/-G.
P.S. Would also be nice if cgdconfig(8) had a way to enter a single
password from which multiple keys are derived, so you can do a single
password entry (and, perhaps, a shell command for a U2F/FIDO button
press interaction like fidocrypt) and use the result for multiple
disks encrypted with separate keys. Not part of argon2 support, of
course, just musing...
