Re: Support for NSS as a libpq TLS backend

> On 22 Mar 2021, at 00:49, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
> Greetings,

Thanks for the review! Below is a partial response, I haven't had time to
address all your review comments yet but I wanted to submit a rebased patchset
directly since the current version doesn't work after recent changes in the
tree. I will address the remaining comments tomorrow or the day after.

This rebase also includes a fix for pgtls_init which was sent offlist by Jacob.
The changes in pgtls_init can potentially be used to initialize the crypto
context for NSS to clean up this patch, Jacob is currently looking at that.

>> Subject: [PATCH v30 1/9] nss: Support libnss as TLS library in libpq
>>
>> This commit contains the frontend and backend portion of TLS support
>> in libpq to allow encrypted connections. The implementation is done
>
> maybe add 'using NSS' to that first sentence. ;)

Fixed.

>> +++ b/src/backend/libpq/auth.c
>> @@ -2849,7 +2849,14 @@ CheckCertAuth(Port *port)
>> {
>> int status_check_usermap = STATUS_ERROR;
>>
>> +#if defined(USE_OPENSSL)
>> Assert(port->ssl);
>> +#elif defined(USE_NSS)
>> + /* TODO: should we rename pr_fd to ssl, to keep consistency? */
>> + Assert(port->pr_fd);
>> +#else
>> + Assert(false);
>> +#endif
>
> Having thought about this TODO item for a bit, I tend to think it's
> better to keep them distinct.

I agree, which is why the TODO comment was there in the first place. I've
removed the comment now.

> They aren't the same and it might not be
> clear what's going on if one was to somehow mix them (at least if pr_fd
> continues to sometimes be a void*, but I wonder why that's being
> done..? more on that later..).

To paraphrase from a later in this email, there are collisions between nspr and
postgres on things like BITS_PER_BYTE, and there were also collisions on basic
types until I learned about NO_NSPR_10_SUPPORT. By moving the juggling of this
into common/nss.h we can use proper types without introducing that pollution
everywhere. I will address these places.

>> +++ b/src/backend/libpq/be-secure-nss.c
> [...]
>> +/* default init hook can be overridden by a shared library */
>> +static void default_nss_tls_init(bool isServerStart);
>> +nss_tls_init_hook_type nss_tls_init_hook = default_nss_tls_init;
>
>> +static PRDescIdentity pr_id;
>> +
>> +static PRIOMethods pr_iomethods;
>
> Happy to be told I'm missing something, but the above two variables seem
> to only be used in init_iolayer.. is there a reason they're declared
> here instead of just being declared in that function?

They must be there since NSPR doesn't copy these but reference them.

>> + /*
>> + * Set the fallback versions for the TLS protocol version range to a
>> + * combination of our minimal requirement and the library maximum. Error
>> + * messages should be kept identical to those in be-secure-openssl.c to
>> + * make translations easier.
>> + */
>
> Should we pull these error messages out into another header so that
> they're in one place to make sure they're kept consistent, if we really
> want to put the effort in to keep them the same..? I'm not 100% sure
> that it's actually necessary to do so, but defining these in one place
> would help maintain this if we want to. Also alright with just keeping
> the comment, not that big of a deal.

It might make sense to pull them into common/nss.h, but seeing the error
message right there when reading the code does IMO make it clearer so it's a
doubleedged sword. Not sure what is the best option, but I'm not married to
the current solution so if there is consensus to pull them out somewhere I'm
happy to do so.

>> +int
>> +be_tls_open_server(Port *port)
>> +{
>> + SECStatus status;
>> + PRFileDesc *model;
>> + PRFileDesc *pr_fd;
>
> pr_fd here is materially different from port->pr_fd, no? As in, one is
> the NSS raw TCP fd while the other is the SSL fd, right? Maybe we
> should use two different variable names to try and make sure they don't
> get confused? Might even set this to NULL after we are done with it
> too.. Then again, I see later on that when we do the dance with the
> 'model' PRFileDesc that we just use the same variable- maybe we should
> do that? That is, just get rid of this 'pr_fd' and use port->pr_fd
> always?

Hmm, I think you're right. I will try that for the next patchset version.

>> + /*
>> + * The NSPR documentation states that runtime initialization via PR_Init
>> + * is no longer required, as the first caller into NSPR will perform the
>> + * initialization implicitly. The documentation doesn't however clarify
>> + * from which version this is holds true, so let's perform the potentially
>> + * superfluous initialization anyways to avoid crashing on older versions
>> + * of NSPR, as there is no difference in overhead. The NSS documentation
>> + * still states that PR_Init must be called in some way (implicitly or
>> + * explicitly).
>> + *
>> + * The below parameters are what the implicit initialization would've done
>> + * for us, and should work even for older versions where it might not be
>> + * done automatically. The last parameter, maxPTDs, is set to various
>> + * values in other codebases, but has been unused since NSPR 2.1 which was
>> + * released sometime in 1998. In current versions of NSPR all parameters
>> + * are ignored.
>> + */
>> + PR_Init(PR_USER_THREAD, PR_PRIORITY_NORMAL, 0 /* maxPTDs */ );
>> +
>> + /*
>> + * The certificate path (configdir) must contain a valid NSS database. If
>> + * the certificate path isn't a valid directory, NSS will fall back on the
>> + * system certificate database. If the certificate path is a directory but
>> + * is empty then the initialization will fail. On the client side this can
>> + * be allowed for any sslmode but the verify-xxx ones.
>> + * https://bugzilla.redhat.com/show_bug.cgi?id=728562 For the server side
>> + * we won't allow this to fail however, as we require the certificate and
>> + * key to exist.
>> + *
>> + * The original design of NSS was for a single application to use a single
>> + * copy of it, initialized with NSS_Initialize() which isn't returning any
>> + * handle with which to refer to NSS. NSS initialization and shutdown are
>> + * global for the application, so a shutdown in another NSS enabled
>> + * library would cause NSS to be stopped for libpq as well. The fix has
>> + * been to introduce NSS_InitContext which returns a context handle to
>> + * pass to NSS_ShutdownContext. NSS_InitContext was introduced in NSS
>> + * 3.12, but the use of it is not very well documented.
>> + * https://bugzilla.redhat.com/show_bug.cgi?id=738456
>
> The above seems to indicate that we will be requiring at least 3.12,
> right? Yet above we have code to work with NSPR versions before 2.1?

Well, not really. The comment tries to explain the rationale for the
parameters passed. Clearly the comment could be improved to make that point
clearer.

> Maybe we should put a stake in the ground that says "we only support
> back to version X of NSS", test with that and a few more recent versions
> and the most recent, and then rip out anything that's needed for
> versions which are older than that?

Yes, right now there is very little in the patch which caters for old versions,
the PR_Init call might be one of the few offenders. There has been discussion
upthread about settling for a required version, combining the insights learned
there with a survey of which versions are commonly available packaged.

Once we settle on a version we can confirm if PR_Init is/isn't needed and
remove all traces of it if not.

> I have a pretty hard time imagining that someone is going to want to build PG
> v14 w/ NSS 2.0 ...

Let alone compiling 2.0 at all on a recent system..

>> + {
>> + char *ciphers,
>> + *c;
>> +
>> + char *sep = ":;, ";
>> + PRUint16 ciphercode;
>> + const PRUint16 *nss_ciphers;
>> +
>> + /*
>> + * If the user has specified a set of preferred cipher suites we start
>> + * by turning off all the existing suites to avoid the risk of down-
>> + * grades to a weaker cipher than expected.
>> + */
>> + nss_ciphers = SSL_GetImplementedCiphers();
>> + for (int i = 0; i < SSL_GetNumImplementedCiphers(); i++)
>> + SSL_CipherPrefSet(model, nss_ciphers[i], PR_FALSE);
>> +
>> + ciphers = pstrdup(SSLCipherSuites);
>> +
>> + for (c = strtok(ciphers, sep); c; c = strtok(NULL, sep))
>> + {
>> + if (!pg_find_cipher(c, &ciphercode))
>> + {
>> + status = SSL_CipherPrefSet(model, ciphercode, PR_TRUE);
>> + if (status != SECSuccess)
>> + {
>> + ereport(COMMERROR,
>> + (errmsg("invalid cipher-suite specified: %s", c)));
>> + return -1;
>> + }
>> + }
>> + }
>
> Maybe I'm a bit confused, but doesn't pg_find_cipher return *true* when
> a cipher is found, and therefore the '!' above is saying "if we don't
> find a matching cipher, then run the code to set the cipher ...".

Hmm, yes thats broken. Fixed.

> Also- we don't seem to complain at all about a cipher being specified that we
> don't find? Guess I would think that we might want to throw a WARNING in such
> a case, but I could possibly be convinced otherwise.

No, I think you're right, we should throw WARNING there or possibly even a
higher elevel. Should that be a COMMERROR even?

> Kind of wonder just what happens with the current code, I'm guessing ciphercode
> is zero and therefore doesn't complain but also doesn't do what we want. I
> wonder if there's a way to test this?

We could extend the test suite to set ciphers in postgresql.conf, I'll give it
a go.

> I do think we should probably throw an error if we end up with *no*
> ciphers being set, which doesn't seem to be happening here..?

Yeah, that should be a COMMERROR. Fixed.

>> + /*
>> + * Set up the custom IO layer.
>> + */
>
> Might be good to mention that the IO Layer is what sets up the
> read/write callbacks to be used.

Good point, will do in the next version of the patchset.

>> + port->pr_fd = SSL_ImportFD(model, pr_fd);
>> + if (!port->pr_fd)
>> + {
>> + ereport(COMMERROR,
>> + (errmsg("unable to initialize")));
>> + return -1;
>> + }
>
> Maybe a comment and a better error message for this?

Will do.

>
>> + PR_Close(model);
>
> This might deserve one also, the whole 'model' construct is a bit
> different. :)

Agreed. will do.

>> + port->ssl_in_use = true;
>> +
>> + /* Register out shutdown callback */
>
> *our

Fixed.

>> +int
>> +be_tls_get_cipher_bits(Port *port)
>> +{
>> + SECStatus status;
>> + SSLChannelInfo channel;
>> + SSLCipherSuiteInfo suite;
>> +
>> + status = SSL_GetChannelInfo(port->pr_fd, &channel, sizeof(channel));
>> + if (status != SECSuccess)
>> + goto error;
>> +
>> + status = SSL_GetCipherSuiteInfo(channel.cipherSuite, &suite, sizeof(suite));
>> + if (status != SECSuccess)
>> + goto error;
>> +
>> + return suite.effectiveKeyBits;
>> +
>> +error:
>> + ereport(WARNING,
>> + (errmsg("unable to extract TLS session information: %s",
>> + pg_SSLerrmessage(PR_GetError()))));
>> + return 0;
>> +}
>
> It doesn't have to be much, but I, at least, do prefer to see
> function-header comments. :) Not that the OpenSSL code has them
> consistently, so obviously not that big of a deal. Goes for a number of
> the functions being added.

No disagreement from me, I've added comments on a few more functions and will
continue to go over the patchset to add them everywhere. Some of these
comments are pretty uninteresting and could do with some wordsmithing.

>> + /* Found a CN, ecode and copy it into a newly allocated buffer */
>
> *decode

Fixed.

>> +static PRInt32
>> +pg_ssl_read(PRFileDesc *fd, void *buf, PRInt32 amount, PRIntn flags,
>> + PRIntervalTime timeout)
>> +{
>> + PRRecvFN read_fn;
>> + PRInt32 n_read;
>> +
>> + read_fn = fd->lower->methods->recv;
>> + n_read = read_fn(fd->lower, buf, amount, flags, timeout);
>> +
>> + return n_read;
>> +}
>> +
>> +static PRInt32
>> +pg_ssl_write(PRFileDesc *fd, const void *buf, PRInt32 amount, PRIntn flags,
>> + PRIntervalTime timeout)
>> +{
>> + PRSendFN send_fn;
>> + PRInt32 n_write;
>> +
>> + send_fn = fd->lower->methods->send;
>> + n_write = send_fn(fd->lower, buf, amount, flags, timeout);
>> +
>> + return n_write;
>> +}
>> +
>> +static PRStatus
>> +pg_ssl_close(PRFileDesc *fd)
>> +{
>> + /*
>> + * Disconnect our private Port from the fd before closing out the stack.
>> + * (Debug builds of NSPR will assert if we do not.)
>> + */
>> + fd->secret = NULL;
>> + return PR_GetDefaultIOMethods()->close(fd);
>> +}
>
> Regarding these, I find myself wondering how they're different from the
> defaults..? I mean, the above just directly called
> PR_GetDefaultIOMethods() to then call it's close() function- are the
> fd->lower_methods->recv/send not the default methods? I don't quite get
> what the point is from having our own callbacks here if they just do
> exactly what the defaults would do (or are there actually no defined
> defaults and you have to provide these..?).

It's really just to cope with debug builds of NSPR which assert that fd->secret
is null before closing.

>> +/*
>> + * ssl_protocol_version_to_nss
>> + * Translate PostgreSQL TLS version to NSS version
>> + *
>> + * Returns zero in case the requested TLS version is undefined (PG_ANY) and
>> + * should be set by the caller, or -1 on failure.
>> + */
>> +static uint16
>> +ssl_protocol_version_to_nss(int v, const char *guc_name)
>
> guc_name isn't actually used in this function..? Is there some reason
> to keep it or is it leftover?

It's a leftover from when the function was doing error reporting, fixed.

> Also, I get that they do similar jobs and that one is in the frontend
> and the other is in the backend, but I'm not a fan of having two
> 'ssl_protocol_version_to_nss()'s functions that take different argument
> types but have exact same name and do functionally different things..

Good point, I'll change that.

>> +++ b/src/backend/utils/misc/guc.c
>> @@ -4377,6 +4381,18 @@ static struct config_string ConfigureNamesString[] =
>> check_canonical_path, assign_pgstat_temp_directory, NULL
>> },
>>
>> +#ifdef USE_NSS
>> + {
>> + {"ssl_database", PGC_SIGHUP, CONN_AUTH_SSL,
>> + gettext_noop("Location of the NSS certificate database."),
>> + NULL
>> + },
>> + &ssl_database,
>> + "",
>> + NULL, NULL, NULL
>> + },
>> +#endif
>
> We don't #ifdef out the various GUCs even if SSL isn't compiled in, so
> it doesn't seem quite right to be doing so here? Generally speaking,
> GUCs that we expect people to use (rather than debugging ones and such)
> are typically always built, even if we don't build support for that
> capability, so we can throw a better error message than just some ugly
> syntax or parsing error if we come across one being set in a non-enabled
> build.

Of course, fixed.

>> +++ b/src/common/cipher_nss.c
>> @@ -0,0 +1,192 @@
>> +/*-------------------------------------------------------------------------
>> + *
>> + * cipher_nss.c
>> + * NSS functionality shared between frontend and backend for working
>> + * with ciphers
>> + *
>> + * This should only bse used if code is compiled with NSS support.
>
> *be

Fixed.

>> +++ b/src/include/libpq/libpq-be.h
>> @@ -200,6 +200,10 @@ typedef struct Port
>> SSL *ssl;
>> X509 *peer;
>> #endif
>> +
>> +#ifdef USE_NSS
>> + void *pr_fd;
>> +#endif
>> } Port;
>
> Given this is under a #ifdef USE_NSS, does it need to be / should it
> really be a void*?

It's to avoid the same BITS_PER_BYTE collision discussed elsewhere in this
email.

>> +++ b/src/interfaces/libpq/fe-connect.c
>> @@ -359,6 +359,10 @@ static const internalPQconninfoOption PQconninfoOptions[] = {
>> "Target-Session-Attrs", "", 15, /* sizeof("prefer-standby") = 15 */
>> offsetof(struct pg_conn, target_session_attrs)},
>>
>> + {"cert_database", NULL, NULL, NULL,
>> + "CertificateDatabase", "", 64,
>> + offsetof(struct pg_conn, cert_database)},
>
> I mean, maybe nitpicking here, but all the other SSL stuff is
> 'sslsomething' and the backend version of this is 'ssl_database', so
> wouldn't it be more consistent to have this be 'ssldatabase'?

Thats a good point, I was clearly Stockholm syndromed since I hadn't reflected
on that but it's clearly wrong. Will fix.

>> +++ b/src/interfaces/libpq/fe-secure-nss.c
>> + * This logic exist in NSS as well, but it's only available for when there is
>
> *exists

Fixed.

>> + /*
>> + * The NSPR documentation states that runtime initialization via PR_Init
>> + * is no longer required, as the first caller into NSPR will perform the
>> + * initialization implicitly. See be-secure-nss.c for further discussion
>> + * on PR_Init.
>> + */
>> + PR_Init(PR_USER_THREAD, PR_PRIORITY_NORMAL, 0);
>
> See same comment I made above- and also there's a comment earlier in
> this file that we don't need to PR_Init() even ...

Right, once we can confirm that the minimum required versions are past the
PR_Init dependency then we should remove all of these calls. If we can't
remove the calls, the comments should be updated to reflect why they are there.

>> + {
>> + conn->nss_context = NSS_InitContext("", "", "", "", &params,
>> + NSS_INIT_READONLY | NSS_INIT_NOCERTDB |
>> + NSS_INIT_NOMODDB | NSS_INIT_FORCEOPEN |
>> + NSS_INIT_NOROOTINIT | NSS_INIT_PK11RELOAD);
>> + if (!conn->nss_context)
>> + {
>> + printfPQExpBuffer(&conn->errorMessage,
>> + libpq_gettext("unable to create certificate database: %s"),
>> + pg_SSLerrmessage(PR_GetError()));
>> + return PGRES_POLLING_FAILED;
>> + }
>> + }
>
> That error message seems a bit ... off? Surely we aren't trying to
> actually create a certificate database here?

Not really no, it does set up a transient database structure for the duration
of the connection AFAIK but thats clearly not the level of detail we should be
giving users. I've reworded to indicate that NSS init failed, and ideally the
pg_SSLerrmessage call will provide appropriate detail.

>> + /*
>> + * Configure cipher policy.
>> + */
>> + status = NSS_SetDomesticPolicy();
>> + if (status != SECSuccess)
>> + {
>> + printfPQExpBuffer(&conn->errorMessage,
>> + libpq_gettext("unable to configure cipher policy: %s"),
>> + pg_SSLerrmessage(PR_GetError()));
>> +
>> + return PGRES_POLLING_FAILED;
>> + }
>
> Probably good to pull over at least some parts of the comments made in
> the backend code about SetDomesticPolicy() actually enabling everything
> (just like all the policies apparently do)...

Good point, will do.

>> + /*
>> + * If we don't have a certificate database, the system trust store is the
>> + * fallback we can use. If we fail to initialize that as well, we can
>> + * still attempt a connection as long as the sslmode isn't verify*.
>> + */
>> + if (!conn->cert_database && conn->sslmode[0] == 'v')
>> + {
>> + status = pg_load_nss_module(&ca_trust, ca_trust_name, "\"Root Certificates\"");
>> + if (status != SECSuccess)
>> + {
>> + printfPQExpBuffer(&conn->errorMessage,
>> + libpq_gettext("WARNING: unable to load NSS trust module \"%s\" : %s"),
>> + ca_trust_name,
>> + pg_SSLerrmessage(PR_GetError()));
>> +
>> + return PGRES_POLLING_FAILED;
>> + }
>> + }
>
> Maybe have something a bit more here about "maybe you should specifify a
> cert_database" or such?

Good point, will expand with more detail.

>> + if (conn->ssl_max_protocol_version && strlen(conn->ssl_max_protocol_version) > 0)
>> + {
>> + int ssl_max_ver = ssl_protocol_version_to_nss(conn->ssl_max_protocol_version);
>> +
>> + if (ssl_max_ver == -1)
>> + {
>> + printfPQExpBuffer(&conn->errorMessage,
>> + libpq_gettext("invalid value \"%s\" for maximum version of SSL protocol\n"),
>> + conn->ssl_max_protocol_version);
>> + return -1;
>> + }
>> +
>> + desired_range.max = ssl_max_ver;
>> + }
>
> In the backend code, we have an additional check to make sure they
> didn't set the min version higher than the max.. should we have that
> here too? Either way, seems like we should be consistent.

We already test that in src/interfaces/libpq/fe-connect.c.

>> + * The model can now we closed as we've applied the settings of the model
>
> *be

Fixed.

>> + * onto the real socket. From hereon we should only use conn->pr_fd.
>
> *here on

Fixed.

> Similar comments to the backend code- should we just always use
> conn->pr_fd? Or should we rename pr_fd to something else?

Renaming is probably not a bad idea, will fix.

>> + /*
>> + * Specify which hostname we are expecting to talk to. This is required,
>> + * albeit mostly applies to when opening a connection to a traditional
>> + * http server it seems.
>> + */
>> + SSL_SetURL(conn->pr_fd, (conn->connhost[conn->whichhost]).host);
>
> We should probably also set SNI, if available (NSS 3.12.6 it seems?),
> since it looks like that's going to be added to the OpenSSL code.

Good point, will do.

>> + do
>> + {
>> + status = SSL_ForceHandshake(conn->pr_fd);
>> + }
>> + while (status != SECSuccess && PR_GetError() == PR_WOULD_BLOCK_ERROR);
>
> We don't seem to have this loop in the backend code.. Is there some
> reason that we don't? Is it possible that we need to have a loop here
> too? I recall in the GSS encryption code there were definitely things
> during setup that had to be looped back over on both sides to make sure
> everything was finished ...

Off the cuff I can't remember, will look into it.

>> + if (conn->sslmode[0] == 'v')
>> + return SECFailure;
>
> Seems a bit grotty to do this (though I see that the OpenSSL code does
> too ... at least there we have a comment though, maybe add one here?).
> I would have thought we'd actually do strcmp()'s like above.

That's admittedly copied from the OpenSSL code, and I agree that it's a bit too
clever. Replaced with plain strcmp's to improve readability in both places it
occurred.

>> + /*
>> + * Return the underlying PRFileDesc which can be used to access
>> + * information on the connection details. There is no SSL context per se.
>> + */
>> + if (strcmp(struct_name, "NSS") == 0)
>> + return conn->pr_fd;
>> + return NULL;
>> +}
>
> Is there never a reason someone might want the pointer returned by
> NSS_InitContext? I don't know that there is but it might be something
> to consider (we could even possibly have our own structure returned by
> this function which includes both, maybe..?). Not sure if there's a
> sensible use-case for that or not just wanted to bring it up as it's
> something I asked myself while reading through this patch.

Not sure I understand what you're asking for here, did you mean "is there ever
a reason"?

>> + if (strcmp(attribute_name, "protocol") == 0)
>> + {
>> + switch (channel.protocolVersion)
>> + {
>> +#ifdef SSL_LIBRARY_VERSION_TLS_1_3
>> + case SSL_LIBRARY_VERSION_TLS_1_3:
>> + return "TLSv1.3";
>> +#endif
>> +#ifdef SSL_LIBRARY_VERSION_TLS_1_2
>> + case SSL_LIBRARY_VERSION_TLS_1_2:
>> + return "TLSv1.2";
>> +#endif
>> +#ifdef SSL_LIBRARY_VERSION_TLS_1_1
>> + case SSL_LIBRARY_VERSION_TLS_1_1:
>> + return "TLSv1.1";
>> +#endif
>> + case SSL_LIBRARY_VERSION_TLS_1_0:
>> + return "TLSv1.0";
>> + default:
>> + return "unknown";
>> + }
>> + }
>
> Not sure that it really matters, but this seems like it might be useful
> to have as its own function... Maybe even a data structure that both
> functions use just in oppostie directions. Really minor tho. :)

I suppose that wouldn't be a bad thing, will fix.

>> diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
>> index c601071838..7f10da3010 100644
>> --- a/src/interfaces/libpq/fe-secure.c
>> +++ b/src/interfaces/libpq/fe-secure.c
>> @@ -448,6 +448,27 @@ PQdefaultSSLKeyPassHook_OpenSSL(char *buf, int size, PGconn *conn)
>> }
>> #endif /* USE_OPENSSL */
>>
>> +#ifndef USE_NSS
>> +
>> +PQsslKeyPassHook_nss_type
>> +PQgetSSLKeyPassHook_nss(void)
>> +{
>> + return NULL;
>> +}
>> +
>> +void
>> +PQsetSSLKeyPassHook_nss(PQsslKeyPassHook_nss_type hook)
>> +{
>> + return;
>> +}
>> +
>> +char *
>> +PQdefaultSSLKeyPassHook_nss(PK11SlotInfo * slot, PRBool retry, void *arg)
>> +{
>> + return NULL;
>> +}
>> +#endif /* USE_NSS */
>
> Isn't this '!USE_NSS'?

Technically it is, but using just /* USE_NSS */ is consistent with the rest of
blocks in the file.

>> diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h
>> index 0c9e95f1a7..f15af39222 100644
>> --- a/src/interfaces/libpq/libpq-int.h
>> +++ b/src/interfaces/libpq/libpq-int.h
>> @@ -383,6 +383,7 @@ struct pg_conn
>> char *sslrootcert; /* root certificate filename */
>> char *sslcrl; /* certificate revocation list filename */
>> char *sslcrldir; /* certificate revocation list directory name */
>> + char *cert_database; /* NSS certificate/key database */
>> char *requirepeer; /* required peer credentials for local sockets */
>> char *gssencmode; /* GSS mode (require,prefer,disable) */
>> char *krbsrvname; /* Kerberos service name */
>> @@ -507,6 +508,28 @@ struct pg_conn
>> * OpenSSL version changes */
>> #endif
>> #endif /* USE_OPENSSL */
>> +
>> +/*
>> + * The NSS/NSPR specific types aren't used to avoid pulling in the required
>> + * headers here, as they are causing conflicts with PG definitions.
>> + */
>
> I'm a bit confused- what are the conflicts being caused here..?
> Certainly under USE_OPENSSL we use the actual OpenSSL types..

It's referring to collisions with for example BITS_PER_BYTE which is defined
both by postgres and nspr. Since writing this I've introduced src/common/nss.h
to handle it in a single place, so we can indeed use the proper types without
polluting the file. Fixed.

>> Subject: [PATCH v30 2/9] Refactor SSL testharness for multiple library
>>
>> The SSL testharness was fully tied to OpenSSL in the way the server was
>> set up and reconfigured. This refactors the SSLServer module into a SSL
>> library agnostic SSL/Server module which in turn use SSL/Backend/<lib>
>> modules for the implementation details.
>>
>> No changes are done to the actual tests, this only change how setup and
>> teardown is performed.
>
> Presumably this could be committed ahead of the main NSS support?

Correct, I think this has merits even if NSS support is ultimately rejected.

>> Subject: [PATCH v30 4/9] nss: pg_strong_random support
>> +++ b/src/port/pg_strong_random.c
>> +bool
>> +pg_strong_random(void *buf, size_t len)
>> +{
>> + NSSInitParameters params;
>> + NSSInitContext *nss_context;
>> + SECStatus status;
>> +
>> + memset(&params, 0, sizeof(params));
>> + params.length = sizeof(params);
>> + nss_context = NSS_InitContext("", "", "", "", &params,
>> + NSS_INIT_READONLY | NSS_INIT_NOCERTDB |
>> + NSS_INIT_NOMODDB | NSS_INIT_FORCEOPEN |
>> + NSS_INIT_NOROOTINIT | NSS_INIT_PK11RELOAD);
>> +
>> + if (!nss_context)
>> + return false;
>> +
>> + status = PK11_GenerateRandom(buf, len);
>> + NSS_ShutdownContext(nss_context);
>> +
>> + if (status == SECSuccess)
>> + return true;
>> +
>> + return false;
>> +}
>> +
>> +#else /* not USE_OPENSSL, USE_NSS or WIN32 */
>
> I don't know that it's an issue, but do we actually need to init the NSS
> context and shut it down every time..?

We need to have a context, and we should be able to set it like how the WIN32
code sets hProvider. I don't remember if there was a reason against that, will
revisit.

>> /*
>> * Without OpenSSL or Win32 support, just read /dev/urandom ourselves.
>
> *or NSS

Fixed.

>> Subject: [PATCH v30 5/9] nss: Documentation
>> +++ b/doc/src/sgml/acronyms.sgml
>> @@ -684,6 +717,16 @@
>> </listitem>
>> </varlistentry>
>>
>> + <varlistentry>
>> + <term><acronym>TLS</acronym></term>
>> + <listitem>
>> + <para>
>> + <ulink url="https://en.wikipedia.org/wiki/Transport_Layer_Security">
>> + Transport Layer Security</ulink>
>> + </para>
>> + </listitem>
>> + </varlistentry>
>
> We don't have this already..? Surely we should..

We really should, especially since we've had <acronym>TLS</acronym> in
config.sgml since 2014 (c6763156589). That's another small piece that could be
committed on it's own to cut down the size of this patchset (even if only by a
tiny amount).

>> diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
>> index 967de73596..1608e9a7c7 100644
>> --- a/doc/src/sgml/config.sgml
>> +++ b/doc/src/sgml/config.sgml
>> @@ -1272,6 +1272,23 @@ include_dir 'conf.d'
>> </listitem>
>> </varlistentry>
>>
>> + <varlistentry id="guc-ssl-database" xreflabel="ssl_database">
>> + <term><varname>ssl_database</varname> (<type>string</type>)
>> + <indexterm>
>> + <primary><varname>ssl_database</varname> configuration parameter</primary>
>> + </indexterm>
>> + </term>
>> + <listitem>
>> + <para>
>> + Specifies the name of the file containing the server certificates and
>> + keys when using <productname>NSS</productname> for <acronym>SSL</acronym>
>> + connections. This parameter can only be set in the
>> + <filename>postgresql.conf</filename> file or on the server command
>> + line.
>
> *SSL/TLS maybe?

Fixed.

>> @@ -1288,7 +1305,9 @@ include_dir 'conf.d'
>> connections using TLS version 1.2 and lower are affected. There is
>> currently no setting that controls the cipher choices used by TLS
>> version 1.3 connections. The default value is
>> - <literal>HIGH:MEDIUM:+3DES:!aNULL</literal>. The default is usually a
>> + <literal>HIGH:MEDIUM:+3DES:!aNULL</literal> for servers which have
>> + been built with <productname>OpenSSL</productname> as the
>> + <acronym>SSL</acronym> library. The default is usually a
>> reasonable choice unless you have specific security requirements.
>> </para>
>
> Shouldn't we say something here wrt NSS?

We should, but I'm not entirely what just yet. Need to revisit that.

>> @@ -1490,8 +1509,11 @@ include_dir 'conf.d'
>> <para>
>> Sets an external command to be invoked when a passphrase for
>> decrypting an SSL file such as a private key needs to be obtained. By
>> - default, this parameter is empty, which means the built-in prompting
>> - mechanism is used.
>> + default, this parameter is empty. When the server is using
>> + <productname>OpenSSL</productname>, this means the built-in prompting
>> + mechanism is used. When using <productname>NSS</productname>, there is
>> + no default prompting so a blank callback will be used returning an
>> + empty password.
>> </para>
>
> Maybe we should point out here that this requires the database to not
> require a password..? So if they have one, they need to set this, or
> maybe we should provide a default one..

I've added a sentence on not using a password for the cert database. I'm not
sure if providing a default one is a good idea but it's no less insecure than
having no password really..

>> +++ b/doc/src/sgml/libpq.sgml
>> +<synopsis>
>> +PQsslKeyPassHook_nss_type PQgetSSLKeyPassHook_nss(void);
>> +</synopsis>
>> + </para>
>> +
>> + <para>
>> + <function>PQgetSSLKeyPassHook_nss</function> has no effect unless the
>> + server was compiled with <productname>nss</productname> support.
>> + </para>
>
> We should try to be consistent- above should be NSS, not nss.

Fixed.

>> + <listitem>
>> + <para>
>> + <productname>NSS</productname>: specifying the parameter is required
>> + in case any password protected items are referenced in the
>> + <productname>NSS</productname> database, or if the database itself
>> + is password protected. If multiple different objects are password
>> + protected, the same password is used for all.
>> + </para>
>> + </listitem>
>> + </itemizedlist>
>
> Is this a statement about NSS databases (which I don't think it is) or
> about the fact that we'll just use the password provided for all
> attempts to decrypt something we need in the database?

Correct.

> Assuming the
> latter, seems like we could reword this to be a bit more clear.
>
> Maybe:
>
> All attempts to decrypt objects which are password protected in the
> database will use this password.

Agreed, fixed.

>> @@ -2620,9 +2791,14 @@ void *PQsslStruct(const PGconn *conn, const char *struct_name);
>> + For <productname>NSS</productname>, there is one struct available under
>> + the name "NSS", and it returns a pointer to the
>> + <productname>NSS</productname> <literal>PRFileDesc</literal>.
>
> ... SSL PRFileDesc associated with the connection, no?

I was trying to be specific that it's an NSS-defined structure and not a
PostgreSQL one which is returned. Fixed.

>> +++ b/doc/src/sgml/runtime.sgml
>> @@ -2552,6 +2583,89 @@ openssl x509 -req -in server.csr -text -days 365 \
>> </para>
>> </sect2>
>>
>> + <sect2 id="nss-certificate-database">
>> + <title>NSS Certificate Databases</title>
>> +
>> + <para>
>> + When using <productname>NSS</productname>, all certificates and keys must
>> + be loaded into an <productname>NSS</productname> certificate database.
>> + </para>
>> +
>> + <para>
>> + To create a new <productname>NSS</productname> certificate database and
>> + load the certificates created in <xref linkend="ssl-certificate-creation" />,
>> + use the following <productname>NSS</productname> commands:
>> +<programlisting>
>> +certutil -d "sql:server.db" -N --empty-password
>> +certutil -d "sql:server.db" -A -n server.crt -i server.crt -t "CT,C,C"
>> +certutil -d "sql:server.db" -A -n root.crt -i root.crt -t "CT,C,C"
>> +</programlisting>
>> + This will give the certificate the filename as the nickname identifier in
>> + the database which is created as <filename>server.db</filename>.
>> + </para>
>> + <para>
>> + Then load the server key, which require converting it to
>
> *requires

Fixed.

>> Subject: [PATCH v30 6/9] nss: Support NSS in pgcrypto
>> +++ b/doc/src/sgml/pgcrypto.sgml
>> <row>
>> <entry>Blowfish</entry>
>> <entry>yes</entry>
>> <entry>yes</entry>
>> + <entry>yes</entry>
>> </row>
>
> Maybe this should mention that it's with the built-in implementation as
> blowfish isn't available from NSS?

Fixed by adding a Note item.

>> <row>
>> <entry>DES/3DES/CAST5</entry>
>> <entry>no</entry>
>> <entry>yes</entry>
>> + <entry>yes</entry>
>> + </row>
>
> Surely CAST5 from the above should be removed, since it's given its own
> entry now?

Indeed, fixed.

>> @@ -1241,7 +1260,8 @@ gen_random_uuid() returns uuid
>> <orderedlist>
>> <listitem>
>> <para>
>> - Any digest algorithm <productname>OpenSSL</productname> supports
>> + Any digest algorithm <productname>OpenSSL</productname> and
>> + <productname>NSS</productname> supports
>> is automatically picked up.
>
> *or? Maybe something more specific though- "Any digest algorithm
> included with the library that PostgreSQL is compiled with is
> automatically picked up." ?

Good point, thats better. Fixed.

>> Subject: [PATCH v30 7/9] nss: Support NSS in sslinfo
>>
>> Since sslinfo to a large extent use the be_tls_* API this mostly
>
> *uses

Fixed.

>> Subject: [PATCH v30 8/9] nss: Support NSS in cryptohash
>> +++ b/src/common/cryptohash_nss.c
>> + /*
>> + * Initialize our own NSS context without a database backing it.
>> + */
>> + memset(&params, 0, sizeof(params));
>> + params.length = sizeof(params);
>> + status = NSS_NoDB_Init(".");
>
> We take some pains to use NSS_InitContext elsewhere.. Are we sure that
> we should be using NSS_NoDB_Init here..?

No, we should probably be using NSS_InitContext. Will fix.

> Just a, well, not so quick read-through. Generally it's looking pretty
> good to me. Will see about playing with it this week.

Thanks again for reviewing, another version which addresses the remaining
issues will be posted soon but I wanted to get this out to give further reviews
something that properly works.

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