> On 27 Nov 2021, at 20:27, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I don't have any problem with this structurally, but I do have a
> few nitpicks:
Thanks for reviewing!
> * I think the error message added in 0001 should complain about
> missing password "encryption" not "encoding", no?
Doh, of course.
> * 0002 hasn't been updated for the great PostgresNode renaming.
Fixed.
> * 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.
Good point, I copied over the wording from recovery/README and adapted for SSL
since I think it was well written as is. (Consistency is also a good benefit.)
> * 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.
Fixed, and see below.
> * 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.
Well spotted, I hadn't thought about that but in hindsight it's quite obviously
bad. I've done this in a 0003 patch in this series which also comes with the
IMO benefit of a tighter coupling between the key filename used in the test
with what's in the repo by removing the _tmp suffix. To avoid concatenating
with the long tmp_check path variable everywhere, I went with a lookup HASH to
make it easier on the eye and harder to mess up should we change tmp path at
some point. There might be ways which are more like modern Perl, but I wasn't
able to think of one off the bat.
> * 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.
Agreed. The attached v3 covers the issuer and extension function to at least
some degree. In order to reliably test the extension I added a new cert with a
CA extension.
--
Daniel Gustafsson https://vmware.com/