Date: Fri, 26 Mar 2021 15:33:38 -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: <20210326193337.GW20766@tamriel.snowman.net>
References: <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>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="xqsKhBVyaL6VuhYV"
Content-Disposition: inline
In-Reply-To: <50c5a423329ea9996a2a2b167931711189ed5d1b.camel@vmware.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: 
 <https://www.postgresql.org/message-id/20210326193337.GW20766%40tamriel.snowman.net>
Precedence: bulk


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

Greetings,

* Jacob Champion (pchampion@vmware.com) wrote:
> On Wed, 2021-03-24 at 14:10 -0400, Stephen Frost wrote:
> > * Jacob Champion (pchampion@vmware.com) wrote:
> > > I could see this being a problem if two client certificate nicknames
> > > collide across multiple in-use databases, maybe?
> >=20
> > Right, in such a case either cert might get returned and it's possible
> > that the "wrong" one is returned and therefore the connection would end
> > up failing, assuming that they aren't actually the same and just happen
> > to be in both.
> >=20
> > Seems like we could use SECMOD_OpenUserDB() and then pass the result
> > from that into PK11_ListCertsInSlot() and scan through the certs in just
> > the specified database to find the one we're looking for if we really
> > feel compelled to try and address this risk.  I've reached out to the
> > NSS folks to see if they have any thoughts about the best way to address
> > this.
>=20
> Some additional findings (NSS 3.63), please correct me if I've made any m=
istakes:
>=20
> The very first NSSInitContext created is special. If it contains a databa=
se, that database will be considered part of the "internal" slot and its ce=
rtificates can be referenced directly by nickname. If it doesn't have a dat=
abase, the internal slot has no certificates, and it will continue to have =
zero certificates until NSS is completely shut down and reinitialized with =
a new "first" context.
>=20
> Databases that are opened *after* the first one are given their own separ=
ate slots. Any certificates that are part of those databases seemingly can'=
t be referenced directly by nickname. They have to be prefixed by their tok=
en name -- a name which you don't have if you used NSS_InitContext() to cre=
ate the database. You have to use SECMOD_OpenUserDB() instead. This explain=
s some strange failures I was seeing in local testing, where the order of I=
nitContext determined whether our client certificate selection succeeded or=
 failed.

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.

> If you SECMOD_OpenUserDB() a database that is identical to the first (int=
ernal) database, NSS deduplicates for you and just returns the internal slo=
t. Which seems like it's helpful, except you're not allowed to close that d=
atabase, and you have to know not to close it by checking to see whether th=
at slot is the "internal key slot". It appears to remain open until NSS is =
shut down entirely.

Seems like we shouldn't do that and should just use SECMOD_OpenUserDB()
for opening databases.

> 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 slot
> 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 parent
> application will do before loading us up.

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.

We could also just generally caution users in our documentation against
using multiple databases.  The NSS folks discourage doing so and it
doesn't strike me as being a terribly useful thing to do anyway, at
least from within one invocation of an application.  Still, if we could
make it work reasonably well, then I'd say we should go ahead and do so.

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

That's unfortuante but solvable by using our own locks, similar
to what's done in fe-secure-openssl.c.

Thanks!

Stephen

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

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

iQIcBAEBCgAGBQJgXjcRAAoJEO1sijiDR2RVxOkQAIHDTUxZ99aOMwWebMiGLiGM
mtDupxOAKoBQIWY50u0SpJywoG4Z6XJK72vzHbLWref2JqpnJZ/H/9HuY2omPJge
pIlgQA4yonmlAofMjvebf3CfX1xnQEBxGjAAPB3xJ5HIhK2gbPSGKtRv+b88kfos
ywLFe0t6/Wfqk64MgMBU59LNN+8cJFA/CfESw0BnPG22PCG1JwzihcIDru+E4W27
YTsT1ZVctGfx0cKaVzVYTvyjTAAVAuRGJsBlB4p+NfO82r95SZuvSjQuDqx91tkN
85EbpjfscBjiKNKZGJEErjo6VDm68h/T1xfcOivZ3bmhiL50Vr/Te7aEebn12hCS
XIjc+6um6+/FwjkRVHQG4lSNKd24+MdKR646q/9ANkhxSu4AM4jHqsgysmJ4abKf
2GVZOm2mhXSACJsR0Fl94ver1D2dVh0BLOsJnriCDSoyeMr1z1pdRWLYA4a4O9ip
QtGlGJzJ9VPELmVFBnqPjkHDKF0PbkE4dO5DM/J1VJYAz8C7jsVLKF9kCfKhsbTu
OBkmre9sYwg2qoddVU2w6K97SXliWHyrRAwVXYXkqEl4AHhZCwTxTXDDEVn6gaMu
CJpioqO8tZ+SxQH/eW2afvAKD0XnXzbn0gRqVla/8BjLLClnN50J1owPIkJ+1f0S
DvVRt00jMpAaYI5Nn+2s
=YCuW
-----END PGP SIGNATURE-----

--xqsKhBVyaL6VuhYV--