Re: Password identifiers, protocol aging and SCRAM protocol

On Fri, Oct 14, 2016 at 9:08 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi> wrote:
> On 10/12/2016 11:11 AM, Michael Paquier wrote:
> * 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.

Okay for this one. Indeed that's a cleaner interface.

> This is pretty much ready for commit now, IMO, but please do review one more
> time.

OK, I had an extra lookup and the patch looks in pretty good shape
seen from here.

- MyCancelKey = PostmasterRandom();
+ if (!pg_strong_random(&MyCancelKey, sizeof(MyCancelKey)))
+ {
+ rw->rw_crashed_at = GetCurrentTimestamp();
+ return false;
+ }
It would be nice to LOG an entry here for bgworkers.

+ /*
+ * fork failed, fall through to report -- actual error
message was
+ * logged by StartAutoVacWorker
+ */
Since you created a new block, the first line gets longer than 80 characters.

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

Actually I have hacked up a small program that can be used to compare
using /dev/urandom with random() calls (this emulates RandomSalt), and
opening/closing /dev/urandom causes a performance hit, but the
difference becomes noticeable with loop calls higher than 10k on my
Linux laptop. I recall that /dev/urandom is quite slow on Linux
compared to other platforms still... So for a single call per
connection attempt we won't actually notice it much. I am just
attaching that if you want to play with it, and you can use it as
follows:
./calc [dev|random] nbytes loops
That's really a quick hack but it does the job if you worry about the
performance.

> * 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.)

That's the business of the maintainers such modules, so my heart is
telling me to rip it off, but my mind tells me that there is no point
in making them unhappy either if they rely on it. I'd trust my mind on
this one, other opinions are welcome.

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

I think that we discussed quickly the point at last PGCon during the
SCRAM-committee-unofficial meeting, and that we talked about doing
that only for HEAD.
--
Michael