Re: Support for NSS as a libpq TLS backend

> On 10 Jul 2020, at 07:10, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> wrote:
>
> On Fri, Jul 3, 2020 at 11:51 PM Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>>> On 25 Jun 2020, at 17:39, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>>>> On 15 May 2020, at 22:46, Daniel Gustafsson <daniel(at)yesql(dot)se> wrote:
>>>> The 0001 patch contains the full NSS support, and 0002 is a fix for the pgstat
>>>> abstraction which IMO leaks backend implementation details. This needs to go
>>>> on it's own thread, but since 0001 fails without it I've included it here for
>>>> simplicity sake for now.
>>>
>>> The attached 0001 and 0002 are the same patchseries as before, but with the
>>> OpenSSL test module fixed and a rebase on top of the current master.
>>
>> Another rebase to resolve conflicts with the recent fixes in the SSL tests, as
>> well as some minor cleanup.
>
> Hi Daniel,
>
> Thanks for blazing the trail for other implementations to coexist in
> the tree. I see that cURL (another project Daniel works on)
> supports a lot of TLS implementations[1].

The list on that URL is also just a selection, the total count is 10 (not
counting OpenSSL forks) IIRC, after axing support for a few lately. OpenSSL
clearly has a large mindshare but the gist of it is that there exist quite a
few alternatives each with their own strengths.

> I recognise 4 other library
> names from that table as having appeared on this mailing list as
> candidates for PostgreSQL support complete with WIP patches, including
> another one from you (Apple Secure Transport). I don't have strong
> views on how many and which libraries we should support,

I think it's key to keep in mind *why* it's relevant to provide options in the
first place, after all, as they must be 100% interoperable one can easily argue
for a single one being enough. We need to to look at what they offer users on
top of just a TLS connection, like: managed certificate storage like for
example macOS Keychains, FIPS certification, good platform availability and/or
OS integration etc. If all a library offers is "not being OpenSSL" then it's
not clear that we're adding much value by spending the cycles to support it.

My personal opinion is that we should keep it pretty limited, not least to
lessen the burden of testing and during feature development. Supporting a new
library comes with requirements on both the CFBot as well as the buildfarm, not
to mention on developers who dabble in that area of the code. The goal should
IMHO be to make it trivial for every postgres installation to use TLS
regardless of platform and experience level with the person installing it.

The situation is a bit different for curl where we have as a goal to provide
enough alternatives such that every platform can have a libcurl/curl more or
less regardless of what it contains. As a consequence, we have around 80 CI
jobs to test each pull request to provide ample coverage. Being a kitchen-
sink is really hard work.

> but I was
> curious how many packages depend on libss1.1, libgnutls30 and libnss3
> in the Debian package repos in my sources.list, and I came up with
> OpenSSL = 820, GnuTLS = 342, and NSS = 87.

I don't see a lot of excitement over GnuTLS lately, but Debian shipping it due
to (I believe) licensing concerns with OpenSSL does help it along. In my
experience, platforms with GnuTLS easily available also have OpenSSL easily
available.

> I guess Solution.pm needs at least USE_NSS => undef for this not to
> break the build on Windows.

Thanks, I'll fix (I admittedly haven't tried this at all on Windows yet).

> Obviously cfbot is useless for testing this code, since its build
> script does --with-openssl and you need --with-nss,

Right, this is a CFBot problem with any patch that require specific autoconf
flags to be excercised. I wonder if we can make something when we do CF app
integration which can inject flags to a Travis pipeline in a safe manner?

> but it still shows
> us one thing: with your patch, a --with-openssl build is apparently
> broken:
>
> /001_ssltests.pl .. 1/93 Bailout called. Further testing stopped:
> system pg_ctl failed

Humm .. I hate to say "it worked on my machine" but it did, but my TLS
environment is hardly standard. Sorry for posting breakage, most likely this
is a bug in the new test module structure that the patch introduce in order to
support multiple backends for src/tests/ssl. I'll fix.

> There are some weird configure-related hunks in the patch:
>
> + -runstatedir | --runstatedir | --runstatedi | --runstated \
> ...[more stuff like that]...
>
> -#define LARGE_OFF_T (((off_t) 1 << 62) - 1 + ((off_t) 1 << 62))
> +#define LARGE_OFF_T ((((off_t) 1 << 31) << 31) - 1 + (((off_t) 1 << 31) << 31))
>
> I see the same when I use Debian's autoconf, but not FreeBSD's or
> MacPorts', despite all being version 2.69. That seems to be due to
> non-upstreamed changes added by the Debian maintainers (I see the
> off_t thing mentioned in /usr/share/doc/autoconf/changelog.Debian.gz).
> I think you need to build a stock autoconf 2.69 or run autoconf on a
> non-Debian system.

Sigh, yes that's a Debianism that slipped through, again sorry about that.

> I installed libnss3-dev on my Debian box and then configure had
> trouble locating and understanding <ssl.h>, until I added
> --with-includes=/usr/include/nss:/usr/include/nspr. I suspect this is
> supposed to be done with pkg-config nss --cflags somewhere in
> configure (or alternatively nss-config --cflags, nspr-config --cflags,
> I don't know, but we're using pkg-config for other stuff).

Yeah, that's a good point, I should fix that. Having a metric ton of TLS
libraries in various versions around in my environment I've been Stockholm
Syndromed to --with-includes to the point where I didn't even think to run
without it. It should clearly be as easy to use as OpenSSL wrt autoconf.

> I installed the Debian package libnss3-tools (for certutil) and then,
> in src/test/ssl, I ran make nssfiles (I guess that should be
> automatic?)

Yes, it needs to run automatically for NSS builds on make check.

> , and make check, and I got this far:
>
> Test Summary Report
> -------------------
> t/001_ssltests.pl (Wstat: 3584 Tests: 93 Failed: 14)
> Failed tests: 14, 16, 18-20, 24, 27-28, 54-55, 78-80
> 91
> Non-zero exit status: 14
>
> You mentioned some were failing in this WIP -- are those results you expect?

I'm not on my dev box at the moment, and I don't remember off the cuff, but
that sounds higher than I remember. I wonder if I fat-fingered the regexes in
the last version?

Thanks for taking a look at the patch, I'll fix up the reported issues Monday
at the latest.

cheers ./daniel