Re: [PATCH v2 1/2] wrapper: add a helper to generate numbers from a CSPRNG
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 4, 2022, 21:01 UTC
- Message-ID
- <xmqqsfu3b4gw.fsf@gitster.g>
- In-Reply-To
- <20220104015555.3387101-2-sandals@crustytoothpaste.net>
"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 7 quoted lines
> +# 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.
Show 24 quoted lines
> +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...
Show 40 quoted lines
> 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?
Show 12 quoted lines
> 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.
Show 62 quoted lines
> + /* 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.