Re: [PATCH v2 1/2] wrapper: add a helper to generate numbers from a CSPRNG

[Junio C Hamano <gitster@pobox.com>]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> +# Define CSPRNG_METHOD to "arc4random" if your system has arc4random and
> +# arc4random_buf, "arc4random-libbsd" if your system has those functions from
> +# libbsd, "getrandom" if your system has getrandom, "getentropy" if your
> +# system has getentropy, "rtlgenrandom" for RtlGenRandom (Windows only), or
> +# "openssl" if you'd want to use the OpenSSL CSPRNG.  If unset or set to
> +# anything else, defaults to using "/dev/urandom".
> +#

OK.

> +ifeq ($(strip $(CSPRNG_METHOD)),arc4random)
> +	BASIC_CFLAGS += -DHAVE_ARC4RANDOM
> +endif
> +
> +ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd)
> +	BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD
> +	EXTLIBS += -lbsd
> +endif
> +
> +ifeq ($(strip $(CSPRNG_METHOD)),getrandom)
> +	BASIC_CFLAGS += -DHAVE_GETRANDOM
> +endif
> +
> +ifeq ($(strip $(CSPRNG_METHOD)),getentropy)
> +	BASIC_CFLAGS += -DHAVE_GETENTROPY
> +endif
> +
> +ifeq ($(strip $(CSPRNG_METHOD)),rtlgenrandom)
> +	BASIC_CFLAGS += -DHAVE_RTLGENRANDOM
> +endif
> +
> +ifeq ($(strip $(CSPRNG_METHOD)),openssl)
> +	BASIC_CFLAGS += -DHAVE_OPENSSL_CSPRNG
> +endif

Use of $(strip ($VAR)) looks a bit different from what everybody
else does with ifeq in this Makefile.  Was there a particular reason
to use it that I am missing?

When we see something unrecognized in CSPRNG_METHOD, we do not touch
BASIC_CFLAGS (or EXTLIBS) here.  I wonder how easy a clue we would
have to decide that we need to fall back to urandom.  IOW, I would
have expected a if/else if/... cascade that has "no we do not have
anything else and need to fall back to urandom" at the end.

But that's OK, as long as the fallback logic is cleanly done.  Let's
keep reading...

> diff --git a/git-compat-util.h b/git-compat-util.h
> index 5fa54a7afe..50597c76be 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1421,4 +1433,11 @@ static inline void *container_of_or_null_offset(void *ptr, size_t offset)
>  
>  void sleep_millisec(int millisec);
>  
> +/*
> + * Generate len bytes from the system cryptographically secure PRNG.
> + * Returns 0 on success and -1 on error, setting errno.  The inability to
> + * satisfy the full request is an error.
> + */
> +int csprng_bytes(void *buf, size_t len);
> +
>  #endif
> diff --git a/t/helper/test-csprng.c b/t/helper/test-csprng.c
> new file mode 100644
> index 0000000000..65d14973c5
> --- /dev/null
> +++ b/t/helper/test-csprng.c
> @@ -0,0 +1,29 @@
> +#include "test-tool.h"
> +#include "git-compat-util.h"
> +
> +
> +int cmd__csprng(int argc, const char **argv)
> +{
> +	unsigned long count;
> +	unsigned char buf[1024];
> +
> +	if (argc > 2) {
> +		fprintf(stderr, "usage: %s [<size>]\n", argv[0]);
> +		return 2;
> +	}
> +
> +	count = (argc == 2) ? strtoul(argv[1], NULL, 0) : -1L;
> +
> +	while (count) {
> +		unsigned long chunk = count < sizeof(buf) ? count : sizeof(buf);

"chunk" should be of type "size_t", no?

> diff --git a/wrapper.c b/wrapper.c
> index 36e12119d7..1052356703 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -702,3 +702,69 @@ int open_nofollow(const char *path, int flags)
>  	return open(path, flags);
>  #endif
>  }
> +
> +int csprng_bytes(void *buf, size_t len)
> +{
> +#if defined(HAVE_ARC4RANDOM) || defined(HAVE_ARC4RANDOM_LIBBSD)

Shouldn't HAVE_ARC4RANDOM mean "we have arc4random_buf() function
available; please use that.", i.e. wouldn't this give us cleaner
result in the change to the Makefile?

 ifeq ($(strip $(CSPRNG_METHOD)),arc4random)
 	BASIC_CFLAGS += -DHAVE_ARC4RANDOM
 endif
 
 ifeq ($(strip $(CSPRNG_METHOD)),arc4random-libbsd)
-	BASIC_CFLAGS += -DHAVE_ARC4RANDOM_LIBBSD
+	BASIC_CFLAGS += -DHAVE_ARC4RANDOM
 	EXTLIBS += -lbsd
 endif

To put it differently, C source, via BASIC_CFLAGS, would not have to
care where the function definition comes from.  It is linker's job
to care and we are already telling it via EXTLIBS, so I am not sure
the value of having HAVE_ARC4RANDOM_LIBBSD as a separate symbol.

> +	/* This function never returns an error. */
> +	arc4random_buf(buf, len);
> +	return 0;
> +#elif defined(HAVE_GETRANDOM)
> +	ssize_t res;
> +	char *p = buf;
> +	while (len) {
> +		res = getrandom(p, len, 0);
> +		if (res < 0)
> +			return -1;
> +		len -= res;
> +		p += res;
> +	}
> +	return 0;
> +#elif defined(HAVE_GETENTROPY)
> +	int res;
> +	char *p = buf;
> +	while (len) {
> +		/* getentropy has a maximum size of 256 bytes. */
> +		size_t chunk = len < 256 ? len : 256;
> +		res = getentropy(p, chunk);
> +		if (res < 0)
> +			return -1;
> +		len -= chunk;
> +		p += chunk;
> +	}
> +	return 0;
> +#elif defined(HAVE_RTLGENRANDOM)
> +	if (!RtlGenRandom(buf, len))
> +		return -1;
> +	return 0;
> +#elif defined(HAVE_OPENSSL_CSPRNG)
> +	int res = RAND_bytes(buf, len);
> +	if (res == 1)
> +		return 0;
> +	if (res == -1)
> +		errno = ENOTSUP;
> +	else
> +		errno = EIO;
> +	return -1;
> +#else
> +	ssize_t res;
> +	char *p = buf;
> +	int fd, err;
> +	fd = open("/dev/urandom", O_RDONLY);
> +	if (fd < 0)
> +		return -1;
> +	while (len) {
> +		res = xread(fd, p, len);
> +		if (res < 0) {
> +			err = errno;
> +			close(fd);
> +			errno = err;
> +			return -1;
> +		}
> +		len -= res;
> +		p += res;
> +	}
> +	close(fd);
> +	return 0;
> +#endif
> +}

OK, I earlier worried about the lack of explicit "we are using
urandom" at the Makefile level, but as long as this will remain the
only place that needs to care about the fallback, the above is
perfectly fine.

Thanks.