Re: Support for NSS as a libpq TLS backend - Mailing list pgsql-hackers

On Tue, Jan 19, 2021 at 09:21:41PM +0100, Daniel Gustafsson wrote:
>> In order to
>> move on with this set, I would suggest to extract some parts of the
>> patch set independently of the others and have two buildfarm members
>> for the MSVC and non-MSVC cases to stress the parts that can be
>> committed.  Just seeing the size, we could move on with:
>> - The ./configure set, with the change to introduce --with-ssl=openssl.
>> - 0004 for strong randoms.
>> - Support for cryptohashes.
>
> I will leave it to others to decide the feasibility of this, I'm happy to slice
> and dice the commits into smaller bits to for example separate out the
> --with-ssl autoconf change into a non NSS dependent commit, if that's wanted.

IMO it makes sense to extract the independent pieces and build on top
of them.  The bulk of the changes is likely going to have a bunch of
comments if reviewed deeply, so I think that we had better remove from
the stack the small-ish problems to ease the next moves.  The
./configure part and replacement of with_openssl by with_ssl is mixed
in 0001 and 0002, which is actually confusing.  And, FWIW, I would be
fine with applying a patch that introduces a --with-ssl with a
compatibility kept for --with-openssl.  This is what 0001 is doing,
actually, similarly to the past switches for --with-uuid.

A point that has been mentioned offline by you, but not mentioned on
this list.  The structure of the modules in src/test/ssl/ could be
refactored to help with an easier integration of more SSL libraries.
This makes sense taken independently.

> Based on an offlist discussion I believe this was a misunderstanding, but if I
> instead misunderstood that feel free to correct me with how you think this
> should be done.

The point would be to rename BITS_PER_BYTE to PG_BITS_PER_BYTE in the
code and avoid conflicts.  I am not completely sure if others would
agree here, but this would remove quite some ifdef/undef stuff from
the code dedicated to NSS.

> > src/sgml/libpq.sgml needs to document PQdefaultSSLKeyPassHook_nss, no?
>
> Good point, fixed.

Please note that patch 0001 is failing to apply after the recent
commit b663a41.  There are conflicts in postgres_fdw.out.

Also, what's the minimum version of NSS that would be supported?  It
would be good to define an acceptable older version, to keep that
documented and to track that perhaps with some configure checks (?),
similarly to what is done for OpenSSL.

Patch 0006 has three trailing whitespaces (git diff --check
complains).  Running the regression tests of pgcrypto, I think that
the SHA2 implementation is not completely right.  Some SHA2 encoding
reports results from already-freed data.  I have spotted a second
issue within scram_HMAC_init(), where pg_cryptohash_create() remains
stuck inside NSS_InitContext(), freezing the regression tests where
password hashed for SCRAM are created.

+   ResourceOwnerEnlargeCryptoHash(CurrentResourceOwner);
+   ctx = MemoryContextAlloc(TopMemoryContext, sizeof(pg_cryptohash_ctx));
+#else
+   ctx = pg_malloc(sizeof(pg_cryptohash_ctx));
+#endif
cryptohash_nss.c cannot use pg_malloc() for frontend allocations.  On
OOM, your patch would call exit() directly, even within libpq.  But
shared library callers need to know about the OOM failure.

+   explicit_bzero(ctx, sizeof(pg_cryptohash_ctx));
+   pfree(ctx);
For similar reasons, pfree should not be used for the frontend code in
cryptohash_nss.c.  The fallback should be just a malloc/free set.

+   status = PK11_DigestBegin(ctx->pk11_context);
+
+   if (status != SECSuccess)
+       return 1;
+   return 0;
This needs to return -1 on failure, not 1.

I really need to study more the choide of the options chosen for
NSS_InitContext()...  But based on the docs I can read on the matter I
think that saving nsscontext in pg_cryptohash_ctx is right for each
cryptohash built.

src/tools/msvc/ is missing an update for cryptohash_nss.c.
--
Michael