Re: Support for NSS as a libpq TLS backend

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. [...]
> > 
> > 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.
> 
> Yes, but see below.
> 
> > > 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.
> 
> 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.
> 
> 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 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.
> > 
> > [...]
> > 
> > > 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.
> 
> 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