On 10/12/2016 11:11 AM, Michael Paquier wrote:
And so we are back on that, with a new set:

Great! I'm looking at this first one for now:

- 0001, introducing pg_strong_random() in src/port/ to have the
backend portion of SCRAM use it instead of random(). This patch is
from Magnus who has kindly sent is to me, so the authorship goes to
him. This patch replaces at the same time PostmasterRandom() with it,
this way once SCRAM gets integrated both the frontend and the backend
finish using the same facility. I think that's good for consistency.
Compared to the version Magnus has sent me, I have changed two things:
-- Reading from /dev/urandom and /dev/random is not influenced by
EINTR. read() handling is also made better in case of partial reads
from a given source.
-- Win32 Crypto routines use MS_DEF_PROV instead of NULL. I think
that's a better idea to not let the user the choice of the encryption
source here.

I spent some time whacking that around:

* Renamed the file to src/port/pg_strong_random.c "pgsrandom" makes me think of srandom(), which this isn't.

* Changed pg_strong_random() to return false on error, and let the callers handle errors. That's more error-prone than throwing an error in the function itself, as it's an easy mistake to forget to check for the return value, but we can't just "exit(1)" if called in the frontend. If it gets called from libpq during authentication, as it will with SCRAM, we want to close the connection and report an error, not exit the whole user application. Likewise, in postmaster, if we fail to generate a query cancel key when forking a backend, we don't want to FATAL and shut down the whole postmaster.

* There used to be this:

-        * Precompute password salt values to use for this connection. It's
-        * slightly annoying to do this long in advance of knowing whether we'll
-        * need 'em or not, but we must do the random() calls before we fork, 
-        * after.  Else the postmaster's random sequence won't get advanced, and
-        * all backends would end up using the same salt...
-        */
-       RandomSalt(port->md5Salt, sizeof(port->md5Salt));

But that whole business of advancing postmaster's random sequence is moot now. So I moved the generation of md5 salt from postmaster to where MD5 authentication is performed.

* This comment in postmaster.c was wrong:

@@ -581,7 +571,7 @@ PostmasterMain(int argc, char *argv[])
         * Note: the seed is pretty predictable from externally-visible facts 
         * as postmaster start time, so avoid using random() for 
         * random values during postmaster startup.  At the time of first
-        * connection, PostmasterRandom will select a hopefully-more-random 
+        * connection, pg_strong_random will select a hopefully-more-random 
        srandom((unsigned int) (MyProcPid ^ MyStartTime));

We don't use pg_strong_random() for that, the same PID+timestamp method is still used as before. Adjusted the comment to reflect reality.

* Added "#include <Wincrypt.h>", for the CryptAcquireContext and CryptGenRandom functions? It compiled OK without that, so I guess it got pulled in via some other header file, but seems more clear and future-proof to #include it directly.

* random comment kibitzing (no pun intended).

This is pretty much ready for commit now, IMO, but please do review one more time. And I do have some small questions still:

* We now open and close /dev/(u)random on every pg_strong_random() call. Should we be worried about performance of that?

* Now that we don't call random() in postmaster anymore, is there any point in calling srandom() there (i.e. where the above incorrect comment was)? Should we remove it? random() might be used by pre-loaded extensions, though. (Hopefully not for cryptographic purposes.)

* Should we backport this? Sorry if we discussed that already, but I don't remember.

- Heikki

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to