From: | Jacob Champion <pchampion(at)vmware(dot)com> |
---|---|
To: | "daniel(at)yesql(dot)se" <daniel(at)yesql(dot)se>, "michael(at)paquier(dot)xyz" <michael(at)paquier(dot)xyz> |
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-14 22:14:16 |
Message-ID: | b762e81b418166ad399fbf2e963609f2212f41e7.camel@vmware.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2021-09-14 22:18:48 | Re: Remove duplicate static function check_permissions in slotfuncs.c and logicalfuncs.c |
Previous Message | Tom Lane | 2021-09-14 21:56:58 | Re: BUG #15293: Stored Procedure Triggered by Logical Replication is Unable to use Notification Events |