On 3/15/20 10:14 PM, Andreas Karlsson wrote:
> 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.
Right, me too.
>
> 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?
I don't think so.
>
> 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.
I'll look at that. Should be possible.
>
> # Nitpicking
>
> The certificate expires in 2030 while all other certificates used in
> tests expires in 2046. Should we be consistent?
Sure. will fix.
>
> 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?
Not in server.key, but I will suppress it for the crt file.
>
> 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.
OK
>
> 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");
I'll make sure to run it through our perl indenter.
Thanks for the review.
cheers
andrew
--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
