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

> 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 sslfiles.mk rather than rewrite them to
make clean (even if they are the same in sslfiles.mk).

* 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
sequence:

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
> (https://commitfest.postgresql.org/34/3029/). 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 https://vmware.com/