Re: SSL passphrase prompt external command

First: thanks a lot for hacking on the SSL code, I might be biased but I really
appreciate it!

The patch no longer applies due to ff18115ae9 and f96f48113f, but the conflicts
are trivial so nothing to worry about there. Documentation exist and reads
well, the added tests pass and seem quite reasonable.

A few comments:

* In src/backend/libpq/be-secure-common.c:

+int
+run_ssl_passphrase_command(const char *prompt, bool is_server_start, char *buf, int size)
+{
[..]
+ size_t len = 0;
[..]
+ return len;
+}

run_ssl_passphrase_command() is defined to return a signed int but returns
size_t which is unsigned. It’s obviously a remote cornercase to hit, but
wouldn’t it be better to return the same signedness?

* The documentation for the passphrase command format states:

"If the setting starts with -, then it will be ignored during a reload and
the SSL configuration will not be reloaded if a passphrase is needed."

In run_ssl_passphrase_command() the leading ‘-‘ is however just shifted away
regardless of the state of is_server_start.

+ p = ssl_passphrase_command;
+
+ if (p[0] == '-')
+ p++;
+

The OpenSSL code is instead performing this in be_tls_init() in the below hunk:

+ if (ssl_passphrase_command[0] && ssl_passphrase_command[0] != '-')

In order to make this generic, shouldn’t we in run_ssl_passphrase_command() do
something along the lines of the below to ensure we aren’t executing the
passphrase callback during reloads?

if (p[0] == '-')
{
/*
* passphrase commands with a leading '-' are only invoked on server
* start, and are ignored on reload.
*/
if (!is_server_start)
goto error;
p++;
}

Unless I’m misunderstanding how the leading dash is supposed to work.

* In src/tools/msvc/Mkvcbuild.pm

# if building without OpenSSL
if (!$solution->{options}->{openssl})
{
+ $postgres->RemoveFile('src/backend/libpq/be-secure-common.c');
$postgres->RemoveFile('src/backend/libpq/be-secure-openssl.c');
}

Is this because run_ssl_passphrase_command() would otherwise generate a warning
due to not being used? Since we will need the file for future SChannel support
(should such a patch materialize), we might as well build it if we can, or at
least add a comment about it needing to be removed from this block.

This patch is further removing our dependency on OpenSSL for SSL code, which is
a direction I support. Putting on my Secure Transport patch-author hat, I
definitely want this feature to be able to use passphrase protected Keychains.

I’m marking this as waiting on author due to my question above, with that
cleared up I am very much +1’ing this to go in.

cheers ./daniel