Re: [PATCH] test/ssl: rework the sslfiles Makefile target

From: Daniel Gustafsson <daniel(at)yesql(dot)se>
To: Michael Paquier <michael(at)paquier(dot)xyz>, Jacob Champion <pchampion(at)vmware(dot)com>
Cc: "andrew(at)dunslane(dot)net" <andrew(at)dunslane(dot)net>, "pgsql-hackers(at)lists(dot)postgresql(dot)org" <pgsql-hackers(at)lists(dot)postgresql(dot)org>, "tgl(at)sss(dot)pgh(dot)pa(dot)us" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: [PATCH] test/ssl: rework the sslfiles Makefile target
Date: 2021-09-13 13:04:28
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

> On 9 Sep 2021, at 03:32, Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> On Wed, Sep 08, 2021 at 07:32:03PM +0000, Jacob Champion wrote:
>> On Fri, 2021-09-03 at 23:21 +0000, Jacob Champion wrote:
>>>> One small-ish comment that I have is about all the .config files we
>>>> have at the root of src/test/ssl/, bloating the whole. I think that
>>>> it would be a bit cleaner to put all of them in a different
>>>> sub-directory, say just config/ or conf/.
>>> That sounds reasonable. I won't be able to get to it before the holiday
>>> weekend, but I can put up a patch sometime next week.
>> Done in v6, a three-patch squashable set. I chose conf/ as the
>> directory.
> Looks sensible to me.

I concur, I like this more readable approach and it makes adding new keys and
certificates easier.

> One thing I can see, while poking at it, is
> that the README mentions sslfiles to recreate the set of files. But
> it is necessary to do sslfiles-clean once, as sslfiles is a no-op if
> the set of files exists.

A few things noted (and fixed as per the attached, which is v6 squashed and
rebased on HEAD; commitmessage hasn't been addressed yet) while reviewing:

* Various comment reflowing to fit within 79 characters

* Pass through the clean targets into rather than rewrite them to
make clean (even if they are the same in

* Moved the lists of keys/certs to be one object per line to match the style
introduced in 01368e5d9. The previous Makefile was violating that as well, but
when we're fixing this file for other things we might as well fix that too.

* Bumped the password protected key output to AES256 to match the server files,
since it's more realistic to see than AES128 (and I was fiddling around here
anyways testing different things, see below).

> I have not been able to check that this is compatible across all the
> versions of OpenSSL we support on HEAD. By looking at the code, that
> should be fine but it would be good to be sure.

The submitted patch works for 1.0.1, 1.0.2 and 1.1.1 when running the below

make check
make ssfiles-clean
make sslfiles
make check

Testing what's in the tree, recreating the keys/certs and testing against the
new ones.

In OpenSSL 3.0.0, the final make check on the generated files breaks on the
encrypted DER key. The key generates fine, and running "openssl rsa ..
-check" validates it, but it fails to load into postgres. Removing the
explicit selection of cipher makes the test pass again (also included in the
attached). I haven't been able to track down exactly what the issue is, but I
have a suspicion that it's in OpenSSL rather than libpq. This issue is present
in master too, so fixing it is orthogonal to this work, but it needs to be
figured out.

Another point of interest here is that 3.0.0 put "openssl rsa" in maintenance
mode, so maybe we'll have to look at supporting "openssl pkeyutl" as well for
some parts should future bugs remain unfixed in the rsa command.

> Daniel, you are registered as a reviewer of this thread
> ( So I guess that you
> would prefer to look at that by yourself and perhaps take care of the
> commit?

Sure, I can take care of it.

Daniel Gustafsson

Attachment Content-Type Size
v7-0001-test-ssl-rework-the-sslfiles-Makefile-target.patch application/octet-stream 99.4 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Amul Sul 2021-09-13 13:09:13 Re: TAP test for recovery_end_command
Previous Message Daniel Gustafsson 2021-09-13 13:01:56 Re: proposal: possibility to read dumped table's name from file