Re: Support for NSS as a libpq TLS backend

Greetings,

* Daniel Gustafsson (daniel(at)yesql(dot)se) wrote:
> Attached is a rebase which attempts to fix the cfbot Appveyor failure, there
> were missing HAVE_ defines for MSVC.

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

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

> +++ 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?

> + /*
> + * 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.

> +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?

> + /*
> + * 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?
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? I have a pretty hard time imagining
that someone is going to want to build PG v14 w/ NSS 2.0 ...

> + {
> + 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 ...". 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. 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?

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

> + /*
> + * 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.

> + 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?

> + PR_Close(model);

This might deserve one also, the whole 'model' construct is a bit
different. :)

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

*our

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

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

*decode

> +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..?).

> +/*
> + * 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?

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

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

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

> +++ 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*?

> +++ 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'?

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

> + /*
> + * 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 ...

> + {
> + 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?

> + /*
> + * 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)...

> + /*
> + * 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?

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

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

*be

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

*here on

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

> + /*
> + * 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.

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

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

> + /*
> + * 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.

> + 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. :)

> 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'?

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

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

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

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

*or NSS

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

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

> @@ -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?

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

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

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

?

> @@ -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?

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

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

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

> @@ -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." ?

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

*uses

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

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!

Stephen