Date: Fri, 26 Mar 2021 18:05:40 -0400
From: Stephen Frost <sfrost@snowman.net>
To: Jacob Champion <pchampion@vmware.com>
Cc: "daniel@yesql.se" <daniel@yesql.se>,
	"pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>,
	"hlinnaka@iki.fi" <hlinnaka@iki.fi>,
	"andrew.dunstan@2ndquadrant.com" <andrew.dunstan@2ndquadrant.com>,
	"thomas.munro@gmail.com" <thomas.munro@gmail.com>,
	"michael@paquier.xyz" <michael@paquier.xyz>,
	"andres@anarazel.de" <andres@anarazel.de>
Subject: Re: Support for NSS as a libpq TLS backend
Message-ID: <20210326220540.GZ20766@tamriel.snowman.net>
References: <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>
 <20210326193337.GW20766@tamriel.snowman.net>
 <60817900d55ea2c9b8dd94658efd2d2adb76dd67.camel@vmware.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="/FQiPxeFNO2XLsFV"
Content-Disposition: inline
In-Reply-To: <60817900d55ea2c9b8dd94658efd2d2adb76dd67.camel@vmware.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: 
 <https://www.postgresql.org/message-id/20210326220540.GZ20766%40tamriel.snowman.net>
Precedence: bulk


--/FQiPxeFNO2XLsFV
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Jacob Champion (pchampion@vmware.com) wrote:
> On Fri, 2021-03-26 at 15:33 -0400, Stephen Frost wrote:
> > * Jacob Champion (pchampion@vmware.com) wrote:
> > > Databases that are opened *after* the first one are given their own
> > > separate slots. [...]
> >=20
> > This is more-or-less what we would want though, right..?  If a user asks
> > for a connection with ssl_database=3Dblah and sslcert=3Dwhatever, we'd =
want
> > to open database 'blah' and search (just) that database for cert
> > 'whatever'.  We could possibly offer other options in the future but
> > certainly this would work and be the most straight-forward and expected
> > behavior.
>=20
> Yes, but see below.
>=20
> > > If you SECMOD_OpenUserDB() a database that is identical to the first
> > > (internal) database, NSS deduplicates for you and just returns the
> > > internal slot. Which seems like it's helpful, except you're not
> > > allowed to close that database, and you have to know not to close it
> > > by checking to see whether that slot is the "internal key slot". It
> > > appears to remain open until NSS is shut down entirely.
> >=20
> > Seems like we shouldn't do that and should just use SECMOD_OpenUserDB()
> > for opening databases.
>=20
> We don't have control over whether or not this happens. If the
> application embedding libpq has already loaded the database into the
> internal slot via its own NSS initialization, then when we call
> SECMOD_OpenUserDB() for that same database, the internal slot will be
> returned and we have to handle it accordingly.
>=20
> It's not a huge amount of work, but it is magic knowledge that has to
> be maintained, especially in the absence of specialized clientside
> tests.

Ah..  yeah, fair enough.  We could document that we discourage
applications from doing so, but I agree that we'll need to deal with it
since it could happen.

> > > But if you open a database that is *not* the magic internal database,
> > > and then open a duplicate of that one, NSS creates yet another new sl=
ot
> > > for the duplicate. So SECMOD_OpenUserDB() may or may not be a resource
> > > hog, depending on the global state of the process at the time libpq
> > > opens its first connection. We won't be able to control what the pare=
nt
> > > application will do before loading us up.
> >=20
> > I would think we'd want to avoid re-opening the same database multiple
> > times, to avoid the duplicate slots and such.  If the application code
> > does it themselves, well, there's not much we can do about that, but we
> > could at least avoid doing so in *our* code.  I wouldn't expect us to be
> > opening hundreds of databases either and so keeping a simple list around
> > of what we've opened and scanning it seems like it'd be workable.  Of
> > course, this could likely be improved in the future but I would think
> > that'd be good for an initial implementation.
> >=20
> > [...]
> >=20
> > > It also doesn't look like any of the SECMOD_* machinery that we're
> > > looking at is thread-safe, but I'd really like to be wrong...
> >=20
> > That's unfortuante but solvable by using our own locks, similar
> > to what's done in fe-secure-openssl.c.
>=20
> Yeah. I was hoping to avoid implementing our own locks and refcounts,
> but it seems like it's going to be required.

Yeah, afraid so.

Thanks!

Stephen

--/FQiPxeFNO2XLsFV
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBCgAGBQJgXlqzAAoJEO1sijiDR2RV720P/RzxqRs4wn6Md4iebw8CnTpX
HZeMJ+el4ylUWqZ4w9NGHbTHWVYAHbe6ne8NRrmYhFiv0sONxCYlPgOq/U0otE3W
KOwfMHDEs4KjCRdwvevuM9zj36pyN3keV/9ZNpBqCOQDHbJqJq+aoJ7Buga9vHNY
IBNKjnjI+NeT69r4BDvX7253/DVEDrb885h8+lkgElr9RJxQdU/xvwwUnFY4O8qy
Nee1AnxnY1HEyti+ByzBSGtjpJ/3Zjs2A0Igckc3hJ+1Fc0w6rqvP8Ak6CebTpW9
SDBxd2iupYTG/k5atWqFyez1rzVMkV+vTuOSP8YH/2pbK+g5pd7n5op4yc0heckX
EZPuKltmJAP0HakUZIX1yxIxvdbkuw7r1LQduYEwV2ci7cyJ21nE6uDkOflFDmDs
+L35hkxhBdWPzevsZyYKHSC/J3n/vWdS4dAqpNqCYclUlt+VeQ5LD1zriiAkk096
ddypYSyZJDFAyFfCczp970HHfiUn1QnySd+2oYnTTN/LUlDiZ0W7fTmqhZMKY86f
v0ADPSwnDu/j8pPVGifnbVtOOjNteoqZ2mO8xkR5PWHSvucQrtzK0y0CCipelm4z
lO3DUQiXWKZFhsuAZW/QvfG1rVpjrX7YH094Ct05PNo6jMeD5cErDrdPZB93Sqdd
4dwRjnL0cHC7Zg8dlXes
=cTip
-----END PGP SIGNATURE-----

--/FQiPxeFNO2XLsFV--