From: Tom Lane <tgl@sss.pgh.pa.us>
To: Daniel Gustafsson <daniel@yesql.se>
cc: Michael Paquier <michael@paquier.xyz>,
        Andrew Dunstan <andrew@dunslane.net>,
        =?utf-8?Q?Dagfinn_Ilmari_Manns=C3=A5ker?= <ilmari@ilmari.org>,
        PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: SSL Tests for sslinfo extension
In-reply-to: <3B87BBC9-706E-4698-8636-30403D21F9B9@yesql.se>
References: <E23F9811-0C77-45DA-912F-D809AB140741@yesql.se>
 <87v97emzcp.fsf@wibble.ilmari.org>
 <cef59c00-8136-157f-5401-2add87c6e014@dunslane.net>
 <09FAE16B-DDE3-4EB3-AEBA-7E31279AA173@yesql.se>
 <YMr5w68Yxf5lF69u@paquier.xyz>
 <3B87BBC9-706E-4698-8636-30403D21F9B9@yesql.se>
Comments: In-reply-to Daniel Gustafsson <daniel@yesql.se>
	message dated "Wed, 23 Jun 2021 16:25:52 +0200"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <599243.1638041239.1@sss.pgh.pa.us>
Date: Sat, 27 Nov 2021 14:27:19 -0500
Message-ID: <599244.1638041239@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/599244.1638041239%40sss.pgh.pa.us>
Precedence: bulk

Daniel Gustafsson <daniel@yesql.se> writes:
> On 17 Jun 2021, at 09:29, Michael Paquier <michael@paquier.xyz> wrote:
>> Hmm.  Adding internal dependencies between the tests should be avoided
>> if we can.  What would it take to move those TAP tests to
>> contrib/sslinfo instead?  This is while keeping in mind that there was
>> a patch aimed at refactoring the SSL test suite for NSS.

> It would be quite invasive as we currently don't provide the SSLServer test
> harness outside of src/test/ssl, and given how tailored it is today I'm not
> sure doing so without a rewrite would be a good idea.

I think testing sslinfo in src/test/ssl is fine, while putting its test
inside contrib/ would be dangerous, because then the test would be run
by default.  src/test/ssl is not run by default because the server it
starts is potentially accessible by other local users, and AFAICS the
same has to be true for an sslinfo test.

So I don't have any problem with this structurally, but I do have a
few nitpicks:

* I think the error message added in 0001 should complain about
missing password "encryption" not "encoding", no?

* 0002 hasn't been updated for the great PostgresNode renaming.

* 0002 needs to extend src/test/ssl/README to mention that
"make installcheck" requires having installed contrib/sslinfo,
analogous to similar comments in (eg) src/test/recovery/README.

* 0002 writes a temporary file in the source tree.  This is bad;
for one thing I bet it fails under VPATH, but in any case there
is no reason to risk it.  Put it in the tmp_check directory instead
(cf temp kdc files in src/test/kerberos/t/001_auth.pl).  That's
safer and you needn't worry about cleaning it up.

* Hmm ... now I notice that you borrowed the key-file-copying logic
from the 001 and 002 tests, but it's just as bad practice there.
We should fix them too.

* I ran a code-coverage check and it shows that this doesn't test
ssl_issuer_field() or any of the following functions in sslinfo.c.
I think at least ssl_extension_info() is complicated enough to
deserve a test.

			regards, tom lane