Re: Support for NSS as a libpq TLS backend

> On 23 Mar 2021, at 00:38, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>> On 22 Mar 2021, at 00:49, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

Attached is a rebase on top of the recent SSL related commits with a few more
fixes from previous reviews.

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

Fixed

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

Fixed.

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

Actually, it turns out that NSS 3.12.6 introduced the serverside SNI handling
by providing callbacks to respond to hostname verification. There was no
mention of clientside SNI in the NSS documentation that I could find, reading
the code however SSL_SetURL does actually set the SNI extension in the
ClientHello. So, clientsidee SNI (which is what is proposed for the OpenSSL
backend) is already in.

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

Thinking more about this, I don't think we should have the loop at all in the
frontend either. The reason it was added was to cover cases where we're
confused about blocking but I can't actually see the case I was worried about
in the code so I think it's useless. Removed.

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

Moved this into a shared function as it's used by both frontend and backend.
It's moved mostly verbatim as it seemed simple enough to not warrant much
complication.

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