Re: Support for NSS as a libpq TLS backend
Daniel Gustafsson <daniel@yesql.se>
From: Daniel Gustafsson <daniel@yesql.se>
To: Jacob Champion <pchampion@vmware.com>
Cc: "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
"hlinnaka@iki.fi" <hlinnaka@iki.fi>,
"andrew.dunstan@2ndquadrant.com" <andrew.dunstan@2ndquadrant.com>,
"andres@anarazel.de" <andres@anarazel.de>,
"thomas.munro@gmail.com" <thomas.munro@gmail.com>,
"sfrost@snowman.net" <sfrost@snowman.net>,
"michael@paquier.xyz" <michael@paquier.xyz>
Date: 2021-06-16T13:31:42Z
Lists: pgsql-hackers
> On 16 Jun 2021, at 01:50, Jacob Champion <pchampion@vmware.com> wrote:
> I've been tracking down reference leaks in the client. These open
> references prevent NSS from shutting down cleanly, which then makes it
> impossible to open a new context in the future. This probably affects
> other libpq clients more than it affects psql.
Ah, nice catch, that's indeed a bug in the frontend implementation. The
problem is that the NSS trustdomain cache *must* be empty before shutting down
the context, else this very issue happens. Note this in be_tls_destroy():
/*
* It reads a bit odd to clear a session cache when we are destroying the
* context altogether, but if the session cache isn't cleared before
* shutting down the context it will fail with SEC_ERROR_BUSY.
*/
SSL_ClearSessionCache();
Calling SSL_ClearSessionCache() in pgtls_close() fixes the error.
There is another resource leak left (visible in one test after the above is
added), the SECMOD module needs to be unloaded in case it's been loaded.
Implementing that with SECMOD_UnloadUserModule trips a segfault in NSS which I
have yet to figure out (when acquiring a lock with NSSRWLock_LockRead).
> The first step to fixing that is not ignoring failures during NSS
> shutdown, so I've tried a patch to pgtls_close() that pushes any
> failures through the pqInternalNotice(). That seems to be working well.
I'm keeping these in during hacking, with a comment that they need to be
revisited during review since they are mainly useful for debugging.
> The tests were still mostly green, so I taught connect_ok() to fail if
> any stderr showed up, and that exposed quite a few failures.
With your patches I'm seeing a couple of these:
SSL error: The one-time function was previously called and failed. Its error code is no longer available
This is an error from NSPR, but it's not clear to me which PR_CallOnce call
it's coming from. It seems to be hitting in the SAN and CRL tests, so it
smells of some form of caching implemented with NSPR API's to me but thats a
mere hunch.
> I am currently stuck on one last failing test. This leak seems to only
> show up when using TLSv1.2 or below.
AFAICT the session cache is avoided for TLSv1.3 due to 1.3 not supporting
renegotiation.
--
Daniel Gustafsson https://vmware.com/
Commits
-
Add tab-completion for CREATE FOREIGN TABLE.
- 74527c3e022d 15.0 cited
-
Add tap tests for the schema publications.
- 6b0f6f79eef2 15.0 cited
-
Move Perl test modules to a better namespace
- b3b4d8e68ae8 15.0 cited
-
Adjust configure to insist on Perl version >= 5.8.3.
- 92e6a98c3636 15.0 cited
-
Simplify code related to compilation of SSL and OpenSSL
- 092b785fad3d 14.0 landed
-
Introduce --with-ssl={openssl} as a configure option
- fe61df7f82aa 14.0 landed
-
Implement support for bulk inserts in postgres_fdw
- b663a4136331 14.0 cited
-
Fix redundant error messages in client tools
- 6be725e70161 14.0 cited
-
doc: Apply more consistently <productname> markup for OpenSSL
- 089da3c4778f 14.0 landed
-
Check ssl_in_use flag when reporting statistics
- 6a5c750f3f72 14.0 cited