On 2/18/20 11:39 PM, Andrew Dunstan wrote:
> This should fix the issue, it happened when I switched to using a
> pre-generated cert/key.
# Review
The patch still applies and passes the test suite, both with openssl
enabled and with it disabled.
As for the feature I agree that it is nice to expose this callback to
extension writers and I agree with the approach taken. The other
proposals up-thread seem over engineered to me. Maybe if it was a
general feature used in many places those proposals would be worth it,
but I am still skeptical even then. This approach is so much simpler.
The only real risk I see is that if people install multiple libraries
for this they will overwrite the hook for each other but we have other
cases like that already so I think that is fine.
The patch moves secure_initialize() to after
process_shared_preload_libraries() which in theory could break some
extension but it does not seem like a likely thing for extensions to
rely on. Or is it?
An idea would be to have the code in ssl_passphrase_func.c to warn if
the ssl_passphrase_command GUC is set to make it more useful as example
code for people implementing this hook.
# Nitpicking
The certificate expires in 2030 while all other certificates used in
tests expires in 2046. Should we be consistent?
There is text in server.crt and server.key, while other certificates and
keys used in the tests do not have this. Again, should we be consistent?
Empty first line in
src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl which should
probably just be removed or replaced with a shebang.
There is an extra space between the parentheses in the line below. Does
that follow our code style for Perl?
+unless ( ($ENV{with_openssl} || 'no') eq 'yes')
Missing space after comma in:
+ok(-e "$ddir/postmaster.pid","postgres started");
Missing space after comma in:
+ok(! -e "$ddir/postmaster.pid","postgres not started with bad passphrase");
Andreas