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
