Content-Type: text/plain;
	charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.80.0.2.43\))
Subject: Re: Support for NSS as a libpq TLS backend
From: Daniel Gustafsson <daniel@yesql.se>
In-Reply-To: <14b33300304507db6f4e4d8a77da105a8d18ea1a.camel@vmware.com>
Date: Wed, 16 Jun 2021 21:52:47 +0200
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>,
 "michael@paquier.xyz" <michael@paquier.xyz>,
 "thomas.munro@gmail.com" <thomas.munro@gmail.com>,
 "andres@anarazel.de" <andres@anarazel.de>,
 "sfrost@snowman.net" <sfrost@snowman.net>
Content-Transfer-Encoding: quoted-printable
Message-Id: <FBBFDB5D-644C-45E7-B8F0-016068464B0C@yesql.se>
References: <1b16041447d57b45ca273f41d26f6e298d756f99.camel@vmware.com>
 <04E81B50-ACE9-40D3-9C66-7041F969A958@yesql.se>
 <21FABD55-5F80-4D8B-B994-3DB81D8742EE@yesql.se>
 <20210321234950.GQ20766@tamriel.snowman.net>
 <F25D6A6A-8FCB-4188-B6E5-5587BABE4D3E@yesql.se>
 <743a11e6668bef648c7a53f862038e6b2fad3755.camel@vmware.com>
 <YFqHmspKASMHcH7V@paquier.xyz>
 <05da9d530ab01e6959bd50c993d590ad73575c21.camel@vmware.com>
 <20210324170036.GS20766@tamriel.snowman.net>
 <8f741a425fd135ca38a28063730715499f7f6445.camel@vmware.com>
 <20210324181016.GU20766@tamriel.snowman.net>
 <50c5a423329ea9996a2a2b167931711189ed5d1b.camel@vmware.com>
 <2FD5BB88-595A-47A0-8F51-0A1B5EEC93FA@yesql.se>
 <F1975F74-DC71-4036-99B4-CBB2D5085EB4@yesql.se>
 <4a15bbab42cd85b6bec83c2609305552d246f68e.camel@vmware.com>
 <8335133C-920E-438D-9CEB-DEF4C1533F73@yesql.se>
 <4ec146256e31afa0542f9fa970ec258c5f1a5f98.camel@vmware.com>
 <70A12E33-90B8-4761-8FA6-8DDB9EEA4D3E@yesql.se>
 <14b33300304507db6f4e4d8a77da105a8d18ea1a.camel@vmware.com>
To: Jacob Champion <pchampion@vmware.com>
Archived-At: 
 <https://www.postgresql.org/message-id/FBBFDB5D-644C-45E7-B8F0-016068464B0C%40yesql.se>
Precedence: bulk

> On 16 Jun 2021, at 18:15, Jacob Champion <pchampion@vmware.com> wrote:
>=20
> On Wed, 2021-06-16 at 15:31 +0200, Daniel Gustafsson wrote:
>>> 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.
>>=20
>> 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():
>>=20
>>    /*
>>     * 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();
>>=20
>> Calling SSL_ClearSessionCache() in pgtls_close() fixes the error.
>=20
> That's unfortunate. The session cache is global, right? So I'm =
guessing
> we'll need to refcount and lock that call, to avoid cleaning up out
> from under a thread that's actively using the the cache?

I'm not sure, the documentation doesn't give any answers and =
implementations of
libnss tend to just clear the cache without consideration.  In libcurl =
we do
just that, and haven't had any complaints - which doesn't mean it's =
correct but
it's a datapoint.

>> 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).
>>=20
>> [...]
>>=20
>> With your patches I'm seeing a couple of these:
>>=20
>>  SSL error: The one-time function was previously called and failed. =
Its error code is no longer available
>=20
> Hmm. Adding SSL_ClearSessionCache() (without thread-safety at the
> moment) fixes all of the SSL tests for me, and I don't see either the
> SECMOD leak or the "one-time function" error that you've mentioned.

Reading the code I don't think a loaded user module is considered a =
resource
that must've been released prior to closing the context.  I will dig for =
what
showed up in my tests, but I don't think it was caused by this.

> What version of NSS are you running? I'm on 3.63.

Right now I'm using what Debian 10 is packaging which is 3.42.  =
Admittedly not
hot off the press but I've been trying to develop off a packaged version =
which
we might see users wanting to deploy against should this get shipped.

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