From: | Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com> |
---|---|
To: | Andreas Karlsson <andreas(at)proxel(dot)se>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com> |
Cc: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Bruce Momjian <bruce(at)momjian(dot)us>, Magnus Hagander <magnus(at)hagander(dot)net>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
Subject: | Re: ssl passphrase callback |
Date: | 2020-03-21 13:15:53 |
Message-ID: | a5a7dd7c-3edf-5e68-2b1c-be935a447c6c@2ndQuadrant.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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
From | Date | Subject | |
---|---|---|---|
Next Message | Andrew Dunstan | 2020-03-21 13:18:26 | Re: ssl passphrase callback |
Previous Message | Amit Kapila | 2020-03-21 12:26:53 | Re: backup manifests |