Re: [PATCH] pgcrypto: implement gen_random_uuid

The only useful feature of the uuid-ossp module in my opinion is the
uuid_generate_v4 function and as uuid-ossp is more or less abandonware
people have had trouble building and installing it. This patch
implements an alternative uuid v4 generation function in pgcrypto which
could be moved to core once there's a core PRNG with large enough
internal state.

On my test system it took 3796 msec to generate a million UUIDs with
pgcrypto while uuid-ossp took 20375 msec.

https://github.com/saaros/postgres/compare/pgcrypto-uuid-v4

contrib/pgcrypto/Makefile | 2 +-
contrib/pgcrypto/pgcrypto--1.0--1.1.sql | 8 ++++++++
contrib/pgcrypto/{pgcrypto--1.0.sql => pgcrypto--1.1.sql} | 7 ++++++-
contrib/pgcrypto/pgcrypto.c | 22
++++++++++++++++++++++
contrib/pgcrypto/pgcrypto.control | 2 +-
contrib/pgcrypto/pgcrypto.h | 1 +
doc/src/sgml/pgcrypto.sgml | 11 +++++++++++

/ Oskari

One comment, this:

> /* get 128 random bits */
> int err = px_get_random_bytes(buf, 16);

might be better to use px_get_pseudo_random_bytes(). UUIDs don't
need to be unguessable or have perfect entropy; they just need to
be collision-resistant. RFC4122 mentions this I think, and if you
look at the ossp-uuid function that this is replacing, it also uses
its internal PRNG for v4 UUIDs rather than strong high-entropy
randomness.

(The downside of requesting strong randomness when you don't need
it is that it can potentially cause the server to block while the
system gathers entropy.)

13.01.2014 04:35, Wim Lewis kirjoitti:
> One comment, this:
>
>> /* get 128 random bits */
>> int err = px_get_random_bytes(buf, 16);
>
> might be better to use px_get_pseudo_random_bytes(). UUIDs don't
> need to be unguessable or have perfect entropy; they just need to
> be collision-resistant. RFC4122 mentions this I think, and if you
> look at the ossp-uuid function that this is replacing, it also uses
> its internal PRNG for v4 UUIDs rather than strong high-entropy
> randomness.
>
> (The downside of requesting strong randomness when you don't need
> it is that it can potentially cause the server to block while the
> system gathers entropy.)

pgcrypto's px_get_pseudo_random_bytes is just a wrapper for
px_get_random_bytes which itself calls system_reseed and
fortuna_get_bytes. system_reseed function tries to read from
/dev/urandom, and only uses /dev/random if reading urandom fails, so it
should never block on systems which have urandom.

That said, it may still make sense to use px_get_pseudo_random_bytes
instead just in case it ever gets modified to do something lighter than
px_get_random_bytes.

Thanks for the review,
Oskari

2014/1/9 Oskari Saarenmaa <os(at)ohmu(dot)fi>:
> The only useful feature of the uuid-ossp module in my opinion is the
> uuid_generate_v4 function and as uuid-ossp is more or less abandonware
> people have had trouble building and installing it. This patch implements
> an alternative uuid v4 generation function in pgcrypto which could be moved
> to core once there's a core PRNG with large enough internal state.

It is a small but very useful patch. Installing uuid-ossp can be very hard
on some systems. There is not much to review. The patch applies cleanly to
HEAD. The function is generating valid UUID version 4. The code and
the documentation style seems to fit in the pgcrypto extension. I am marking
it as "Ready for Commiter".

The problem is users probably would not look pgcrypto extension for
UUID generator, especially when there is another extension with uuid in
it's name. Also, UUID generator does not sound like a cryptographic function.
It would be much better, if this would be in core with the UUID type. There
is a reference on the UUID Type documentation page to the uuid-ossp
extension. We can add a reference to pgcrypro extension in that page and
consider adding a deprecation note to the uuid-ossp extension, if is is not
possible to add the function to the core, for now.

Emre Hasegeli <emre(at)hasegeli(dot)com> writes:
> 2014/1/9 Oskari Saarenmaa <os(at)ohmu(dot)fi>:
>> The only useful feature of the uuid-ossp module in my opinion is the
>> uuid_generate_v4 function and as uuid-ossp is more or less abandonware
>> people have had trouble building and installing it. This patch implements
>> an alternative uuid v4 generation function in pgcrypto which could be moved
>> to core once there's a core PRNG with large enough internal state.

> It is a small but very useful patch. Installing uuid-ossp can be very hard
> on some systems. There is not much to review. The patch applies cleanly to
> HEAD. The function is generating valid UUID version 4. The code and
> the documentation style seems to fit in the pgcrypto extension. I am marking
> it as "Ready for Commiter".

> The problem is users probably would not look pgcrypto extension for
> UUID generator, especially when there is another extension with uuid in
> it's name. Also, UUID generator does not sound like a cryptographic function.
> It would be much better, if this would be in core with the UUID type. There
> is a reference on the UUID Type documentation page to the uuid-ossp
> extension. We can add a reference to pgcrypro extension in that page and
> consider adding a deprecation note to the uuid-ossp extension, if is is not
> possible to add the function to the core, for now.

Well, we're not pulling pgcrypto into core in the foreseeable future;
there are legal (export control) issues that make that too risky.
Even aside from that, there was general consensus when type uuid went
in that the various generation algorithms were, how shall I say it, too
intellectually unsatisfying to be part of the core code. So I think from
a code standpoint this solution is just fine. I agree that we need some
extra work on the documentation to point people towards this approach
instead of uuid-ossp, though. I'll take care of that and commit.

regards, tom lane