Re: Support for NSS as a libpq TLS backend

> On 24 Mar 2021, at 04:54, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
>
> On Tue, Mar 23, 2021 at 12:38:50AM +0100, Daniel Gustafsson wrote:
>> 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.
>
> I have been looking at the infrastructure of the tests, patches 0002
> (some refactoring) and 0003 (more refactoring with tests for NSS), and
> I am a bit confused by its state.
>
> First, I think that the split is not completely clear. For example,
> patch 0003 has changes for OpenSSL.pm and Server.pm, but wouldn't it
> be better to have all the refactoring infrastructure only in 0002,
> with 0003 introducing only the NSS pieces for its internal data and
> NSS.pm?

Yes. Juggling a patchset of this size is errorprone. This is why I opened the
separate thread for this where the patch can be held apart cleaner, so let's
take this discussion over there. I will post an updated patch there shortly.

> + keyfile => 'server-password',
> + nssdatabase => 'server-cn-only.crt__server-password.key.db',
> + passphrase_cmd => 'echo secret1',
> 001_ssltests.pl and 002_scram.pl have NSS-related parameters, which
> does not look like a clean separation to me as there are OpenSSL tests
> that use some NSS parts, and the main scripts should remain neutral in
> terms setting contents, including only variables and callbacks that
> should be filled specifically for each SSL implementation, no? Aren't
> we missing a second piece here with a set of callbacks for the
> per-library test paths then?

Well, then again, keyfile is an OpenSSL specific parameter, it just happens to
be named quite neutrally. I'm not sure how to best express the certificate and
key requirements of a test since the testcase is the source of truth in terms
of what it requires. If we introduce a standard set of cert/keys which all
backends are required to supply, we could refer to those. Tests that need
something more specific can then go into 00X_<library>.pl. There is a balance
to strike though, there is a single backend now with at most one on the horizon
which is yet to be decided upon, making it too generic may end up making test
writing overcomplicated. Do you have any concretee ideas?

> + if (defined($openssl))
> + {
> + copy_files("ssl/server-*.crt", $pgdata);
> + copy_files("ssl/server-*.key", $pgdata);
> + chmod(0600, glob "$pgdata/server-*.key") or die $!;
> + copy_files("ssl/root+client_ca.crt", $pgdata);
> + copy_files("ssl/root_ca.crt", $pgdata);
> + copy_files("ssl/root+client.crl", $pgdata);
> + mkdir("$pgdata/root+client-crldir");
> + copy_files("ssl/root+client-crldir/*",
> "$pgdata/root+client-crldir/");
> + }
> + elsif (defined($nss))
> + {
> + RecursiveCopy::copypath("ssl/nss", $pgdata . "/nss") if -e
> "ssl/nss";
> + }
> This had better be in its own callback, for example.

Yes, this one is a clearer case, fixed in the v2 patch which will be posted on
the separate thread.

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