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

On Mon, 2021-09-13 at 15:04 +0200, Daniel Gustafsson wrote:
> 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.

Looks good, thanks!

> * 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).

Few thoughts about this part of the diff:

> -# Convert client.key to encrypted PEM (X.509 text) and DER (X.509 ASN.1) formats
> -# to test libpq's support for the sslpassword= option.
> -ssl/client-encrypted-pem.key: outform := PEM
> -ssl/client-encrypted-der.key: outform := DER
> +# Convert client.key to encrypted PEM (X.509 text) and DER (X.509 ASN.1)
> +# formats to test libpq's support for the sslpassword= option.
>  ssl/client-encrypted-pem.key ssl/client-encrypted-der.key: ssl/client.key
> -       openssl rsa -in $< -outform $(outform) -aes128 -passout 'pass:dUmmyP^#+' -out $@
> +       openssl rsa -in $< -outform PEM -aes256 -passout 'pass:dUmmyP^#+' -out $@
> +ssl/client-encrypted-der.key: ssl/client.key
> +       openssl rsa -in $< -outform DER -passout 'pass:dUmmyP^#+' -out $@

1. Should the DER key be AES256 as well?
2. The ssl/client-encrypted-der.key target for the first recipe should
be removed; I get a duplication warning from Make.
3. The new client key will need to be included in the patch; the one
there now is still the AES128 version.

And one doc comment:

>  ssl/ subdirectory. The Makefile also contains a rule, "make sslfiles", to
> -recreate them if you need to make changes.
> +recreate them if you need to make changes. "make sslfiles-clean" is required
> +in order to recreate.

This is only true if you need to rebuild the entire tree; if you just
want to recreate a single cert pair, you can just touch the config file
for it (or remove the key, if you want to regenerate the pair) and
`make sslfiles` again.

> 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.

Great, thanks!

> 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.

Good to know. Agreed that it should be a separate patch.

--Jacob