Re: Support for NSS as a libpq TLS backend

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Jacob Champion <pchampion(at)vmware(dot)com>, "hlinnaka(at)iki(dot)fi" <hlinnaka(at)iki(dot)fi>, "andrew(dot)dunstan(at)2ndquadrant(dot)com" <andrew(dot)dunstan(at)2ndquadrant(dot)com>, "thomas(dot)munro(at)gmail(dot)com" <thomas(dot)munro(at)gmail(dot)com>, "andres(at)anarazel(dot)de" <andres(at)anarazel(dot)de>
Subject: Re: Support for NSS as a libpq TLS backend
Date: 2021-03-24 23:00:30
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

> 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 and, 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

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',
> and 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

In response to

Browse pgsql-hackers by date

  From Date Subject
Next Message Fabrízio de Royes Mello 2021-03-24 23:01:04 Re: Minimal logical decoding on standbys
Previous Message Michael Paquier 2021-03-24 22:44:02 Re: pgsql: Move tablespace path re-creation from the makefiles to pg_regres