Re: Random number generation, take two

On Wed, Nov 30, 2016 at 8:51 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 11/30/2016 09:01 AM, Michael Paquier wrote:
> I was thinking that with --disable-strong-random, we'd use plain random() in
> libpq as well. I believe SCRAM is analogous to the MD5 salt generation in
> the backend, in its requirement for randomness.

OK. That's fine by me to do so.

>>> As this patch stands, it removes Fortuna, and returns known-weak keys,
>>> but
>>> there's a good argument to be made for throwing an error instead.
>>
>>
>> IMO, leading to an error would make the users aware of them playing
>> with the fire... Now pademelon's owner may likely have a different
>> opinion on the matter :p
>
> Ok, I bit the bullet and modified those pgcrypto functions that truly need
> cryptographically strong random numbers to throw an error with
> --disable-strong-random. Notably, the pgp encrypt functions mostly fail now.

The alternate output looks good to me.

>> +bool
>> +pg_backend_random(char *dst, int len)
>> +{
>> + int i;
>> + char *end = dst + len;
>> +
>> + /* should not be called in postmaster */
>> + Assert (IsUnderPostmaster || !IsPostmasterEnvironment);
>> +
>> + LWLockAcquire(BackendRandomLock, LW_EXCLUSIVE);
>> Shouldn't an exclusive lock be taken only when the initialization
>> phase is called? When reading the value a shared lock would be fine.
>
>
> No, it needs to be exclusive. Each pg_jrand48() call updates the state, aka
> seed.

Do we need to worry about performance in the case of application doing
small transactions and creating new connections for each transaction?
This would become a contention point when calculating cancel keys for
newly-forked backends. It could be rather easy to measure a
concurrency impact with for example pgbench -C with many concurrent
transactions running something as light as SELECT 1.

>> Attached is a patch for MSVC to apply on top of yours to enable the
>> build for strong and weak random functions. Feel free to hack it as
>> needs be, this base implementation works for the current
>> implementation.
>
> Great, thanks! I wonder if this is overly complicated, though. For
> comparison, we haven't bothered to expose --disable-spinlocks in
> config_default.pl either. Perhaps we should just always use the Windows
> native function on MSVC, whether or not configured with OpenSSL, and just
> put USE_WIN32_RANDOM in pg_config.h.win32? See 2nd attached patch
> (untested).

I could live with that. Your patch is not complete though, you need to
add pg_strong_random.c into the array @pgportfiles in Mkvcbuild.pm.
You also need to remove fortuna.c and random.c from the list of files
in $pgcrypto->AddFiles(). After doing so the code is able to compile
properly.

+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("pg_random_bytes() is not supported by this build"),
+ errdetail("This functionality requires a source of
strong random numbers"),
+ errhint("You need to rebuild PostgreSQL using
--enable-strong-random")));
Perhaps this should say "You need to rebuild PostgreSQL without
--disable-strong-random", the docs do not mention
--enable-strong-random nor does ./configure --help.

+/* port/pg_strong_random.c */
+#ifndef USE_WEAK_RANDOM
+extern bool pg_strong_random(void *buf, size_t len);
+#endif
This should be HAVE_STRONG_RANDOM.
--
Michael