Re: ssl passphrase callback

From: Andreas Karlsson <andreas(at)proxel(dot)se>
To: Andrew Dunstan <andrew(dot)dunstan(at)2ndquadrant(dot)com>, 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-16 02:14:57
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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/ 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/","postgres started");

Missing space after comma in:

+ok(! -e "$ddir/","postgres not started with bad passphrase");


In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2020-03-16 02:45:03 Re: [HACKERS] Moving relation extension locks out of heavyweight lock manager
Previous Message Atsushi Torikoshi 2020-03-16 02:09:49 add types to index storage params on doc