Re: [PATCH] test/ssl: rework the sslfiles Makefile target - Mailing list pgsql-hackers
From | Daniel Gustafsson |
---|---|
Subject | Re: [PATCH] test/ssl: rework the sslfiles Makefile target |
Date | |
Msg-id | 7532A831-76D5-4A45-A63F-B215AC9CE93D@yesql.se Whole thread Raw |
In response to | Re: [PATCH] test/ssl: rework the sslfiles Makefile target (Michael Paquier <michael@paquier.xyz>) |
Responses |
Re: [PATCH] test/ssl: rework the sslfiles Makefile target
(Jacob Champion <pchampion@vmware.com>)
|
List | pgsql-hackers |
> On 9 Sep 2021, at 03:32, Michael Paquier <michael@paquier.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/
Attachment
pgsql-hackers by date: