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

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

> Date: Fri, 5 Nov 2021 16:25:05 +0000
> From: nia <nia%NetBSD.org@localhost>
> 
> 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...