Re: Support for NSS as a libpq TLS backend

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?
> > 
> > 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.
> > 
> > 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.
> 
> Some additional findings (NSS 3.63), please correct me if I've made any mistakes:
> 
> The very first NSSInitContext created is special. If it contains a database, that database will be considered part of the "internal" slot and its certificates can be referenced directly by nickname. If it doesn't have a database, 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.
> 
> Databases that are opened *after* the first one are given their own separate slots. Any certificates that are part of those databases seemingly can't be referenced directly by nickname. They have to be prefixed by their token name -- a name which you don't have if you used NSS_InitContext() to create the database. You have to use SECMOD_OpenUserDB() instead. This explains some strange failures I was seeing in local testing, where the order of InitContext 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=blah and sslcert=whatever, 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 (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.

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