Re: Support for NSS as a libpq TLS backend

> On 21 Jan 2021, at 06:21, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> 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.

This has been discussed elsewhere in the thread, so let's continue that there.
The attached v23 does however split off --with-ssl for OpenSSL in 0001, adding
the nss option in 0002.

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

This has been submitted in F513E66A-E693-4802-9F8A-A74C1D0E3D10(at)yesql(dot)se(dot)

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

Aha, now I see what you mean, sorry for the confusion. That can certainly be
done (and done so outside of this patchset), but it admittedly feels a bit
intrusive. If there is consensus that we should namespace our version like
this I'll go ahead and do that.

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

Fixed.

> Patch 0006 has three trailing whitespaces (git diff --check complains).

Fixed.

> 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've been unable to reproduce, can you shed some light on this?

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

I think the freezing you saw comes from opening and closing NSS contexts per
cryptohash op (some patience on my part runs the test Ok in ~30s which is
clearly not in the wheelhouse of acceptable), more on that below.

> + 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.

Of course, fixed.

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

Doh, fixed.

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

It's a safe but slow option, NSS wasn't really made for running a single crypto
operation. Since we are opening a context which isn't backed by an NSS
database we could have a static context, which indeed speeds up processing a
lot. The problem with that is that there is no good callsite for closing the
context as the backend is closing down. Since you are kneedeep in the
cryptohash code, do you have any thoughts on this? I've included 0008 which
implements this, with a commented out dummy stub for cleaning up.

Making nss_context static in cryptohash_nss.c is
appealing but there is no good option for closing it there. Any thoughts on
how to handle global contexts like this?

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

Fixed.

--
Daniel Gustafsson https://vmware.com/