ssl passphrase callback

Started by Andrew Dunstanabout 6 years ago48 messages
#1Andrew Dunstan
andrew.dunstan@2ndquadrant.com
1 attachment(s)

This is the first of a number of patches to enhance SSL functionality,
particularly w.r.t. passphrases.

This patch provides a hook for a function that can supply an SSL
passphrase. The hook can be filled in by a shared preloadable module. In
order for that to be effective, the startup order is modified slightly.
There is a test attached that builds and uses one trivial
implementation, which just takes a configuration setting and rot13's it
before supplying the result as the passphrase.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

ssl-passphrase-callback-1.patchtext/x-patch; name=ssl-passphrase-callback-1.patchDownload
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 629919cc6e..bf4493c94a 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -45,6 +45,9 @@
 #include "utils/memutils.h"
 
 
+ssl_passphrase_func_cb ssl_passphrase_function = NULL;
+bool ssl_passphrase_function_supports_reload = false;
+
 static int	my_sock_read(BIO *h, char *buf, int size);
 static int	my_sock_write(BIO *h, const char *buf, int size);
 static BIO_METHOD *my_BIO_s_socket(void);
@@ -124,12 +127,16 @@ be_tls_init(bool isServerStart)
 	 */
 	if (isServerStart)
 	{
-		if (ssl_passphrase_command[0])
+		if (ssl_passphrase_function)
+			SSL_CTX_set_default_passwd_cb(context, ssl_passphrase_function);
+		else if (ssl_passphrase_command[0])
 			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
 	}
 	else
 	{
-		if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
+		if (ssl_passphrase_function && ssl_passphrase_function_supports_reload)
+			SSL_CTX_set_default_passwd_cb(context, ssl_passphrase_function);
+		else if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
 			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
 		else
 
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 5f30359165..18dd8578d9 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -972,17 +972,6 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	LocalProcessControlFile(false);
 
-	/*
-	 * Initialize SSL library, if specified.
-	 */
-#ifdef USE_SSL
-	if (EnableSSL)
-	{
-		(void) secure_initialize(true);
-		LoadedSSL = true;
-	}
-#endif
-
 	/*
 	 * Register the apply launcher.  Since it registers a background worker,
 	 * it needs to be called before InitializeMaxBackends(), and it's probably
@@ -996,6 +985,17 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	process_shared_preload_libraries();
 
+	/*
+	 * Initialize SSL library, if specified.
+	 */
+#ifdef USE_SSL
+	if (EnableSSL)
+	{
+		(void) secure_initialize(true);
+		LoadedSSL = true;
+	}
+#endif
+
 	/*
 	 * Now that loadable modules have had their chance to register background
 	 * workers, calculate MaxBackends.
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 541f970f99..dd2b5fc6c8 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -287,6 +287,17 @@ extern void be_tls_get_peer_serial(Port *port, char *ptr, size_t len);
 extern char *be_tls_get_certificate_hash(Port *port, size_t *len);
 #endif
 
+/*
+ * ssl_passphrase_function can be filled in by a shared preloaded module
+ * to supply a passphrase for a key file, as can the flag noting whether the
+ * function supports reloading.
+ */
+
+typedef int (* ssl_passphrase_func_cb) (char *buf, int size, int rwflag,
+										void *userdata);
+extern ssl_passphrase_func_cb ssl_passphrase_function;
+extern bool ssl_passphrase_function_supports_reload;
+
 #endif							/* USE_SSL */
 
 #ifdef ENABLE_GSS
diff --git a/src/test/modules/ssl_passphrase_callback/.gitignore b/src/test/modules/ssl_passphrase_callback/.gitignore
new file mode 100644
index 0000000000..1dbadf7baf
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/.gitignore
@@ -0,0 +1 @@
+tmp_check
diff --git a/src/test/modules/ssl_passphrase_callback/Makefile b/src/test/modules/ssl_passphrase_callback/Makefile
new file mode 100644
index 0000000000..876ede2f21
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/Makefile
@@ -0,0 +1,22 @@
+# ssl_passphrase Makefile
+
+MODULE_big = ssl_passphrase_func
+OBJS = ssl_passphrase_func.o $(WIN32RES)
+PGFILEDESC = "callback function to provide a passphrase"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/ssl_passphrase_callback
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: prove-check
+
+prove-check: | temp-install
+	@echo running prove ...
+	$(prove_check)
diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
new file mode 100644
index 0000000000..e3e433b6f0
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
@@ -0,0 +1,80 @@
+/*-------------------------------------------------------------------------
+ *
+ * ssl_passphrase_func.c
+ *
+ * Loadable PostgreSQL module fetch an ssl passphrase for the server cert.
+ * instead of calling an external program. This implementation just hands
+ * back the configured password rot13'd.
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include <float.h>
+#include <stdio.h>
+
+#include "libpq/libpq-be.h"
+#include "utils/guc.h"
+
+PG_MODULE_MAGIC;
+
+void		_PG_init(void);
+void		_PG_fini(void);
+
+static char *ssl_passphrase = NULL;
+
+static int rot13_passphrase(char *buf,
+				 int size,
+				 int rwflag,
+				 void *userdata);
+
+/*
+ * Module load callback
+ */
+void
+_PG_init(void)
+{
+	/* Define custom GUC variable. */
+	DefineCustomStringVariable("ssl_passphrase.passphrase",
+							   "passphrase before transformation",
+							   NULL,
+							   &ssl_passphrase,
+							   NULL,
+							   PGC_SIGHUP,
+							   0,	/* no flags required */
+							   NULL,
+							   NULL,
+							   NULL);
+	if (ssl_passphrase)
+	{
+		ssl_passphrase_function = rot13_passphrase;
+		ssl_passphrase_function_supports_reload = true;
+	}
+}
+
+void
+_PG_fini(void)
+{
+	/* do  nothing yet */
+}
+
+static int
+rot13_passphrase(char *buf, int size, int rwflag, void *userdata)
+{
+
+	Assert(sslpassphrase != NULL);
+	StrNCpy(buf, ssl_passphrase, size);
+	for (char *p = buf; *p; p++)
+	{
+		char		c = *p;
+
+		if ((c >= 'a' && c <= 'm') || (c >= 'A' && c <= 'M'))
+			*p = c + 13;
+		else if ((c >= 'n' && c <= 'z') || (c >= 'N' && c <= 'Z'))
+			*p = c - 13;
+	}
+
+	return strlen(buf);
+
+}
diff --git a/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
new file mode 100644
index 0000000000..1226647d3e
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
@@ -0,0 +1,64 @@
+
+use strict;
+use warnings;
+
+use File::Copy;
+
+use TestLib;
+use Test::More;
+use PostgresNode;
+
+my $clearpass = "FooBaR1";
+my $rot13pass = "SbbOnE1";
+
+
+
+# create a self-signed cert
+system('openssl req -new -x509 -days 365 -nodes -text -out server.crt -keyout server.ckey -subj "/CN=localhost"');
+# add the cleartext passphrase to the key, remove the unprotected key
+system("openssl rsa -aes256 -in server.ckey -out server.key -passout pass:$clearpass");
+unlink "server.ckey";
+
+
+my $node = get_new_node('main');
+$node->init;
+$node->append_conf('postgresql.conf',
+				   "ssl_passphrase.passphrase = '$rot13pass'");
+$node->append_conf('postgresql.conf',
+				   "shared_preload_libraries = 'ssl_passphrase_func'");
+$node->append_conf('postgresql.conf',
+				   "listen_addresses = 'localhost'");
+$node->append_conf('postgresql.conf',
+				   "ssl = 'on'");
+
+my $ddir = $node->data_dir;
+
+# install certificate and protected key
+move("server.crt", $ddir);
+move("server.key", $ddir);
+chmod 0600, "$ddir/server.key";
+
+$node->start;
+
+# if the server is running we must had successfully transformed the passphrase
+ok(-e "$ddir/postmaster.pid","postgres started");
+
+$node->stop('fast');
+
+# set the wrong passphrase
+$node->append_conf('postgresql.conf',
+				   "ssl_passphrase.passphrase = 'blurfl'");
+
+# try to start the server again
+my $ret = TestLib::system_log('pg_ctl', '-D', $node->data_dir, '-l',
+							  $node->logfile, 'start');
+
+
+# with a bad passphrase the server should not start
+ok($ret, "pg_ctl fails with bad passphrase");
+ok(! -e "$ddir/postmaster.pid","postgres not started with bad passphrase");
+
+# just in case
+$node->stop('fast');
+
+done_testing();
#2Robert Haas
robertmhaas@gmail.com
In reply to: Andrew Dunstan (#1)
Re: ssl passphrase callback

On Thu, Oct 31, 2019 at 11:37 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

This patch provides a hook for a function that can supply an SSL
passphrase. The hook can be filled in by a shared preloadable module. In
order for that to be effective, the startup order is modified slightly.
There is a test attached that builds and uses one trivial
implementation, which just takes a configuration setting and rot13's it
before supplying the result as the passphrase.

It seems to me that it would be a lot better to have an example in
contrib that does something which might be of actual use to users,
such as running a shell command and reading the passphrase from
stdout.

Features that are only accessible by writing C code are, in general,
not as desirable as features which can be accessed via SQL or
configuration.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#3Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Robert Haas (#2)
Re: ssl passphrase callback

On 11/1/19 11:01 AM, Robert Haas wrote:

On Thu, Oct 31, 2019 at 11:37 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

This patch provides a hook for a function that can supply an SSL
passphrase. The hook can be filled in by a shared preloadable module. In
order for that to be effective, the startup order is modified slightly.
There is a test attached that builds and uses one trivial
implementation, which just takes a configuration setting and rot13's it
before supplying the result as the passphrase.

It seems to me that it would be a lot better to have an example in
contrib that does something which might be of actual use to users,
such as running a shell command and reading the passphrase from
stdout.

Features that are only accessible by writing C code are, in general,
not as desirable as features which can be accessed via SQL or
configuration.

Well, I tried to provide the most trivial and simple test I could come
up with. Running a shell command can already be accomplished via the
ssl_passphrase_command setting.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#4Thomas Munro
thomas.munro@gmail.com
In reply to: Andrew Dunstan (#3)
Re: ssl passphrase callback

On Sat, Nov 2, 2019 at 6:57 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

On 11/1/19 11:01 AM, Robert Haas wrote:

On Thu, Oct 31, 2019 at 11:37 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

This patch provides a hook for a function that can supply an SSL
passphrase. The hook can be filled in by a shared preloadable module. In
order for that to be effective, the startup order is modified slightly.
There is a test attached that builds and uses one trivial
implementation, which just takes a configuration setting and rot13's it
before supplying the result as the passphrase.

It seems to me that it would be a lot better to have an example in
contrib that does something which might be of actual use to users,
such as running a shell command and reading the passphrase from
stdout.

Features that are only accessible by writing C code are, in general,
not as desirable as features which can be accessed via SQL or
configuration.

Well, I tried to provide the most trivial and simple test I could come
up with. Running a shell command can already be accomplished via the
ssl_passphrase_command setting.

It looks like the new declarations in libpq-be.h are ifdef'd out in a
non-USE_SSL build, but then we still try to build the new test module
and it fails:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.64071

#5Bruce Momjian
bruce@momjian.us
In reply to: Andrew Dunstan (#3)
Re: ssl passphrase callback

On Fri, Nov 1, 2019 at 01:57:29PM -0400, Andrew Dunstan wrote:

On 11/1/19 11:01 AM, Robert Haas wrote:

On Thu, Oct 31, 2019 at 11:37 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

This patch provides a hook for a function that can supply an SSL
passphrase. The hook can be filled in by a shared preloadable module. In
order for that to be effective, the startup order is modified slightly.
There is a test attached that builds and uses one trivial
implementation, which just takes a configuration setting and rot13's it
before supplying the result as the passphrase.

It seems to me that it would be a lot better to have an example in
contrib that does something which might be of actual use to users,
such as running a shell command and reading the passphrase from
stdout.

Features that are only accessible by writing C code are, in general,
not as desirable as features which can be accessed via SQL or
configuration.

Well, I tried to provide the most trivial and simple test I could come
up with. Running a shell command can already be accomplished via the
ssl_passphrase_command setting.

What is the value of a shared library over a shell command? We had this
discussion in relation to archive_command years ago, and decided on a
shell command as the best API.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +
#6Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Thomas Munro (#4)
1 attachment(s)
Re: ssl passphrase callback

On 11/4/19 4:43 PM, Thomas Munro wrote:

It looks like the new declarations in libpq-be.h are ifdef'd out in a
non-USE_SSL build, but then we still try to build the new test module
and it fails:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.64071

I think this updated patch should fix things.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

ssl-passphrase-callback-2.patchtext/x-patch; name=ssl-passphrase-callback-2.patchDownload
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 629919cc6e..bf4493c94a 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -45,6 +45,9 @@
 #include "utils/memutils.h"
 
 
+ssl_passphrase_func_cb ssl_passphrase_function = NULL;
+bool ssl_passphrase_function_supports_reload = false;
+
 static int	my_sock_read(BIO *h, char *buf, int size);
 static int	my_sock_write(BIO *h, const char *buf, int size);
 static BIO_METHOD *my_BIO_s_socket(void);
@@ -124,12 +127,16 @@ be_tls_init(bool isServerStart)
 	 */
 	if (isServerStart)
 	{
-		if (ssl_passphrase_command[0])
+		if (ssl_passphrase_function)
+			SSL_CTX_set_default_passwd_cb(context, ssl_passphrase_function);
+		else if (ssl_passphrase_command[0])
 			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
 	}
 	else
 	{
-		if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
+		if (ssl_passphrase_function && ssl_passphrase_function_supports_reload)
+			SSL_CTX_set_default_passwd_cb(context, ssl_passphrase_function);
+		else if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
 			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
 		else
 
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 5f30359165..18dd8578d9 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -972,17 +972,6 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	LocalProcessControlFile(false);
 
-	/*
-	 * Initialize SSL library, if specified.
-	 */
-#ifdef USE_SSL
-	if (EnableSSL)
-	{
-		(void) secure_initialize(true);
-		LoadedSSL = true;
-	}
-#endif
-
 	/*
 	 * Register the apply launcher.  Since it registers a background worker,
 	 * it needs to be called before InitializeMaxBackends(), and it's probably
@@ -996,6 +985,17 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	process_shared_preload_libraries();
 
+	/*
+	 * Initialize SSL library, if specified.
+	 */
+#ifdef USE_SSL
+	if (EnableSSL)
+	{
+		(void) secure_initialize(true);
+		LoadedSSL = true;
+	}
+#endif
+
 	/*
 	 * Now that loadable modules have had their chance to register background
 	 * workers, calculate MaxBackends.
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 541f970f99..dd2b5fc6c8 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -287,6 +287,17 @@ extern void be_tls_get_peer_serial(Port *port, char *ptr, size_t len);
 extern char *be_tls_get_certificate_hash(Port *port, size_t *len);
 #endif
 
+/*
+ * ssl_passphrase_function can be filled in by a shared preloaded module
+ * to supply a passphrase for a key file, as can the flag noting whether the
+ * function supports reloading.
+ */
+
+typedef int (* ssl_passphrase_func_cb) (char *buf, int size, int rwflag,
+										void *userdata);
+extern ssl_passphrase_func_cb ssl_passphrase_function;
+extern bool ssl_passphrase_function_supports_reload;
+
 #endif							/* USE_SSL */
 
 #ifdef ENABLE_GSS
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b2eaef3bff..5f975ebcba 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -25,4 +25,9 @@ SUBDIRS = \
 		  unsafe_tests \
 		  worker_spi
 
+ifeq ($(with_openssl),yes)
+SUBDIRS += ssl_passphrase_callback
+endif
+
+
 $(recurse)
diff --git a/src/test/modules/ssl_passphrase_callback/.gitignore b/src/test/modules/ssl_passphrase_callback/.gitignore
new file mode 100644
index 0000000000..1dbadf7baf
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/.gitignore
@@ -0,0 +1 @@
+tmp_check
diff --git a/src/test/modules/ssl_passphrase_callback/Makefile b/src/test/modules/ssl_passphrase_callback/Makefile
new file mode 100644
index 0000000000..685b3aed74
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/Makefile
@@ -0,0 +1,25 @@
+# ssl_passphrase Makefile
+
+export with_openssl
+
+MODULE_big = ssl_passphrase_func
+OBJS = ssl_passphrase_func.o $(WIN32RES)
+PGFILEDESC = "callback function to provide a passphrase"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/ssl_passphrase_callback
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: prove-check
+
+prove-check: ssl_passphrase_func$(DLSUFFIX) | temp-install
+	@echo running prove ...
+	$(prove_check)
+
diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
new file mode 100644
index 0000000000..e3e433b6f0
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
@@ -0,0 +1,80 @@
+/*-------------------------------------------------------------------------
+ *
+ * ssl_passphrase_func.c
+ *
+ * Loadable PostgreSQL module fetch an ssl passphrase for the server cert.
+ * instead of calling an external program. This implementation just hands
+ * back the configured password rot13'd.
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include <float.h>
+#include <stdio.h>
+
+#include "libpq/libpq-be.h"
+#include "utils/guc.h"
+
+PG_MODULE_MAGIC;
+
+void		_PG_init(void);
+void		_PG_fini(void);
+
+static char *ssl_passphrase = NULL;
+
+static int rot13_passphrase(char *buf,
+				 int size,
+				 int rwflag,
+				 void *userdata);
+
+/*
+ * Module load callback
+ */
+void
+_PG_init(void)
+{
+	/* Define custom GUC variable. */
+	DefineCustomStringVariable("ssl_passphrase.passphrase",
+							   "passphrase before transformation",
+							   NULL,
+							   &ssl_passphrase,
+							   NULL,
+							   PGC_SIGHUP,
+							   0,	/* no flags required */
+							   NULL,
+							   NULL,
+							   NULL);
+	if (ssl_passphrase)
+	{
+		ssl_passphrase_function = rot13_passphrase;
+		ssl_passphrase_function_supports_reload = true;
+	}
+}
+
+void
+_PG_fini(void)
+{
+	/* do  nothing yet */
+}
+
+static int
+rot13_passphrase(char *buf, int size, int rwflag, void *userdata)
+{
+
+	Assert(sslpassphrase != NULL);
+	StrNCpy(buf, ssl_passphrase, size);
+	for (char *p = buf; *p; p++)
+	{
+		char		c = *p;
+
+		if ((c >= 'a' && c <= 'm') || (c >= 'A' && c <= 'M'))
+			*p = c + 13;
+		else if ((c >= 'n' && c <= 'z') || (c >= 'N' && c <= 'Z'))
+			*p = c - 13;
+	}
+
+	return strlen(buf);
+
+}
diff --git a/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
new file mode 100644
index 0000000000..ffdc9f1562
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
@@ -0,0 +1,67 @@
+
+use strict;
+use warnings;
+
+use File::Copy;
+
+use TestLib;
+use Test::More;
+use PostgresNode;
+
+unless ( ($ENV{with_openssl} || 'no') eq 'yes')
+{
+	plan skip_all => 'SSL not supported by this build';
+}
+
+my $clearpass = "FooBaR1";
+my $rot13pass = "SbbOnE1";
+
+# create a self-signed cert
+system('openssl req -new -x509 -days 365 -nodes -text -out server.crt -keyout server.ckey -subj "/CN=localhost"');
+# add the cleartext passphrase to the key, remove the unprotected key
+system("openssl rsa -aes256 -in server.ckey -out server.key -passout pass:$clearpass");
+unlink "server.ckey";
+
+
+my $node = get_new_node('main');
+$node->init;
+$node->append_conf('postgresql.conf',
+				   "ssl_passphrase.passphrase = '$rot13pass'");
+$node->append_conf('postgresql.conf',
+				   "shared_preload_libraries = 'ssl_passphrase_func'");
+$node->append_conf('postgresql.conf',
+				   "listen_addresses = 'localhost'");
+$node->append_conf('postgresql.conf',
+				   "ssl = 'on'");
+
+my $ddir = $node->data_dir;
+
+# install certificate and protected key
+move("server.crt", $ddir);
+move("server.key", $ddir);
+chmod 0600, "$ddir/server.key";
+
+$node->start;
+
+# if the server is running we must had successfully transformed the passphrase
+ok(-e "$ddir/postmaster.pid","postgres started");
+
+$node->stop('fast');
+
+# set the wrong passphrase
+$node->append_conf('postgresql.conf',
+				   "ssl_passphrase.passphrase = 'blurfl'");
+
+# try to start the server again
+my $ret = TestLib::system_log('pg_ctl', '-D', $node->data_dir, '-l',
+							  $node->logfile, 'start');
+
+
+# with a bad passphrase the server should not start
+ok($ret, "pg_ctl fails with bad passphrase");
+ok(! -e "$ddir/postmaster.pid","postgres not started with bad passphrase");
+
+# just in case
+$node->stop('fast');
+
+done_testing();
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 9a0963a050..0fd887b778 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -429,7 +429,7 @@ sub mkvcbuild
 
 	if (!$solution->{options}->{openssl})
 	{
-		push @contrib_excludes, 'sslinfo';
+		push @contrib_excludes, 'sslinfo', 'ssl_passphrase_callback';
 	}
 
 	if (!$solution->{options}->{uuid})
#7Simon Riggs
simon@2ndquadrant.com
In reply to: Bruce Momjian (#5)
Re: ssl passphrase callback

On Thu, 7 Nov 2019 at 10:24, Bruce Momjian <bruce@momjian.us> wrote:

What is the value of a shared library over a shell command? We had this
discussion in relation to archive_command years ago, and decided on a
shell command as the best API.

I don't recall such a discussion, but I can give perspective:

* shell command offered the widest and simplest API for integration, which
was the most important consideration for a backup API. That choice caused
difficulty with the need to pass information to the external command, e.g.
%f %p

* shared library is more appropriate for a security-related module, so
users can't see how it is configured, as well as being more
tightly integrated so it can be better tailored to various uses

Summary is that the choice is not random, nor mere preference

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Solutions for the Enterprise

#8Tomas Vondra
tomas.vondra@2ndquadrant.com
In reply to: Simon Riggs (#7)
Re: ssl passphrase callback

On Fri, Nov 08, 2019 at 11:12:08PM +0900, Simon Riggs wrote:

On Thu, 7 Nov 2019 at 10:24, Bruce Momjian <bruce@momjian.us> wrote:

What is the value of a shared library over a shell command? We had
this discussion in relation to archive_command years ago, and decided
on a shell command as the best API.

I don't recall such a discussion, but I can give perspective:

* shell command offered the widest and simplest API for integration,
which was the most important consideration for a backup API. That
choice caused difficulty with the need to pass information to the
external command, e.g. %f %p

It's not clear to me why simple API for integration would be less
valuable for this feature. Also, I'm sure passing data to/from shell
command may be tricky, but presumably we have figured how to do that.

* shared library is more appropriate for a security-related module, so
users can't see how it is configured, as well as being more
tightly integrated so it can be better tailored to various uses

I don't follow. Why would there be a significant difference between a
shell command/script and shared library in this respect? If you don't
want the users to see the config, just store it in a separate file and
it's about the same as storing it in the .so library.

Is there something that can be done with a .so library but can't be done
with a shell command (which may just call a binary, with all the config
included, making it equal to the .so solution)?

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#9Magnus Hagander
magnus@hagander.net
In reply to: Bruce Momjian (#5)
Re: ssl passphrase callback

On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian <bruce@momjian.us> wrote:

On Fri, Nov 1, 2019 at 01:57:29PM -0400, Andrew Dunstan wrote:

On 11/1/19 11:01 AM, Robert Haas wrote:

On Thu, Oct 31, 2019 at 11:37 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

This patch provides a hook for a function that can supply an SSL
passphrase. The hook can be filled in by a shared preloadable module.

In

order for that to be effective, the startup order is modified

slightly.

There is a test attached that builds and uses one trivial
implementation, which just takes a configuration setting and rot13's

it

before supplying the result as the passphrase.

It seems to me that it would be a lot better to have an example in
contrib that does something which might be of actual use to users,
such as running a shell command and reading the passphrase from
stdout.

Features that are only accessible by writing C code are, in general,
not as desirable as features which can be accessed via SQL or
configuration.

Well, I tried to provide the most trivial and simple test I could come
up with. Running a shell command can already be accomplished via the
ssl_passphrase_command setting.

What is the value of a shared library over a shell command?

For one, platforms where shell commands are a lot less convenient, such as
Windows.

We had this
discussion in relation to archive_command years ago, and decided on a
shell command as the best API.

I don't recall that from back then, but that was a long time ago.

But it's interesting that you mention it, given the number of people I have
been discussing that with recently, under the topic of changing it from a
shell command into a shared library API (with there being a shell command
as one possible implementation of course).

One of the main reasons there being to be easily able to transfer more
state and give results other than just an exit code, no need to deal with
parameter escaping etc. Which probably wouldn't matter as much to an SSL
passphrase command, but still.

//Magnus

#10Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Andrew Dunstan (#6)
1 attachment(s)
Re: ssl passphrase callback

On 11/7/19 12:30 PM, Andrew Dunstan wrote:

On 11/4/19 4:43 PM, Thomas Munro wrote:

It looks like the new declarations in libpq-be.h are ifdef'd out in a
non-USE_SSL build, but then we still try to build the new test module
and it fails:

https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.64071

I think this updated patch should fix things.

This time with a typo fixed to keep the cfbot happy.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

ssl-passphrase-callback-3.patchtext/x-patch; name=ssl-passphrase-callback-3.patchDownload
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 629919cc6e..bf4493c94a 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -45,6 +45,9 @@
 #include "utils/memutils.h"
 
 
+ssl_passphrase_func_cb ssl_passphrase_function = NULL;
+bool ssl_passphrase_function_supports_reload = false;
+
 static int	my_sock_read(BIO *h, char *buf, int size);
 static int	my_sock_write(BIO *h, const char *buf, int size);
 static BIO_METHOD *my_BIO_s_socket(void);
@@ -124,12 +127,16 @@ be_tls_init(bool isServerStart)
 	 */
 	if (isServerStart)
 	{
-		if (ssl_passphrase_command[0])
+		if (ssl_passphrase_function)
+			SSL_CTX_set_default_passwd_cb(context, ssl_passphrase_function);
+		else if (ssl_passphrase_command[0])
 			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
 	}
 	else
 	{
-		if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
+		if (ssl_passphrase_function && ssl_passphrase_function_supports_reload)
+			SSL_CTX_set_default_passwd_cb(context, ssl_passphrase_function);
+		else if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
 			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
 		else
 
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 5f30359165..18dd8578d9 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -972,17 +972,6 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	LocalProcessControlFile(false);
 
-	/*
-	 * Initialize SSL library, if specified.
-	 */
-#ifdef USE_SSL
-	if (EnableSSL)
-	{
-		(void) secure_initialize(true);
-		LoadedSSL = true;
-	}
-#endif
-
 	/*
 	 * Register the apply launcher.  Since it registers a background worker,
 	 * it needs to be called before InitializeMaxBackends(), and it's probably
@@ -996,6 +985,17 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	process_shared_preload_libraries();
 
+	/*
+	 * Initialize SSL library, if specified.
+	 */
+#ifdef USE_SSL
+	if (EnableSSL)
+	{
+		(void) secure_initialize(true);
+		LoadedSSL = true;
+	}
+#endif
+
 	/*
 	 * Now that loadable modules have had their chance to register background
 	 * workers, calculate MaxBackends.
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 541f970f99..dd2b5fc6c8 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -287,6 +287,17 @@ extern void be_tls_get_peer_serial(Port *port, char *ptr, size_t len);
 extern char *be_tls_get_certificate_hash(Port *port, size_t *len);
 #endif
 
+/*
+ * ssl_passphrase_function can be filled in by a shared preloaded module
+ * to supply a passphrase for a key file, as can the flag noting whether the
+ * function supports reloading.
+ */
+
+typedef int (* ssl_passphrase_func_cb) (char *buf, int size, int rwflag,
+										void *userdata);
+extern ssl_passphrase_func_cb ssl_passphrase_function;
+extern bool ssl_passphrase_function_supports_reload;
+
 #endif							/* USE_SSL */
 
 #ifdef ENABLE_GSS
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b2eaef3bff..5f975ebcba 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -25,4 +25,9 @@ SUBDIRS = \
 		  unsafe_tests \
 		  worker_spi
 
+ifeq ($(with_openssl),yes)
+SUBDIRS += ssl_passphrase_callback
+endif
+
+
 $(recurse)
diff --git a/src/test/modules/ssl_passphrase_callback/.gitignore b/src/test/modules/ssl_passphrase_callback/.gitignore
new file mode 100644
index 0000000000..1dbadf7baf
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/.gitignore
@@ -0,0 +1 @@
+tmp_check
diff --git a/src/test/modules/ssl_passphrase_callback/Makefile b/src/test/modules/ssl_passphrase_callback/Makefile
new file mode 100644
index 0000000000..685b3aed74
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/Makefile
@@ -0,0 +1,25 @@
+# ssl_passphrase Makefile
+
+export with_openssl
+
+MODULE_big = ssl_passphrase_func
+OBJS = ssl_passphrase_func.o $(WIN32RES)
+PGFILEDESC = "callback function to provide a passphrase"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/ssl_passphrase_callback
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: prove-check
+
+prove-check: ssl_passphrase_func$(DLSUFFIX) | temp-install
+	@echo running prove ...
+	$(prove_check)
+
diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
new file mode 100644
index 0000000000..bdbf3a3671
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
@@ -0,0 +1,80 @@
+/*-------------------------------------------------------------------------
+ *
+ * ssl_passphrase_func.c
+ *
+ * Loadable PostgreSQL module fetch an ssl passphrase for the server cert.
+ * instead of calling an external program. This implementation just hands
+ * back the configured password rot13'd.
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include <float.h>
+#include <stdio.h>
+
+#include "libpq/libpq-be.h"
+#include "utils/guc.h"
+
+PG_MODULE_MAGIC;
+
+void		_PG_init(void);
+void		_PG_fini(void);
+
+static char *ssl_passphrase = NULL;
+
+static int rot13_passphrase(char *buf,
+				 int size,
+				 int rwflag,
+				 void *userdata);
+
+/*
+ * Module load callback
+ */
+void
+_PG_init(void)
+{
+	/* Define custom GUC variable. */
+	DefineCustomStringVariable("ssl_passphrase.passphrase",
+							   "passphrase before transformation",
+							   NULL,
+							   &ssl_passphrase,
+							   NULL,
+							   PGC_SIGHUP,
+							   0,	/* no flags required */
+							   NULL,
+							   NULL,
+							   NULL);
+	if (ssl_passphrase)
+	{
+		ssl_passphrase_function = rot13_passphrase;
+		ssl_passphrase_function_supports_reload = true;
+	}
+}
+
+void
+_PG_fini(void)
+{
+	/* do  nothing yet */
+}
+
+static int
+rot13_passphrase(char *buf, int size, int rwflag, void *userdata)
+{
+
+	Assert(ssl_passphrase != NULL);
+	StrNCpy(buf, ssl_passphrase, size);
+	for (char *p = buf; *p; p++)
+	{
+		char		c = *p;
+
+		if ((c >= 'a' && c <= 'm') || (c >= 'A' && c <= 'M'))
+			*p = c + 13;
+		else if ((c >= 'n' && c <= 'z') || (c >= 'N' && c <= 'Z'))
+			*p = c - 13;
+	}
+
+	return strlen(buf);
+
+}
diff --git a/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
new file mode 100644
index 0000000000..ffdc9f1562
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
@@ -0,0 +1,67 @@
+
+use strict;
+use warnings;
+
+use File::Copy;
+
+use TestLib;
+use Test::More;
+use PostgresNode;
+
+unless ( ($ENV{with_openssl} || 'no') eq 'yes')
+{
+	plan skip_all => 'SSL not supported by this build';
+}
+
+my $clearpass = "FooBaR1";
+my $rot13pass = "SbbOnE1";
+
+# create a self-signed cert
+system('openssl req -new -x509 -days 365 -nodes -text -out server.crt -keyout server.ckey -subj "/CN=localhost"');
+# add the cleartext passphrase to the key, remove the unprotected key
+system("openssl rsa -aes256 -in server.ckey -out server.key -passout pass:$clearpass");
+unlink "server.ckey";
+
+
+my $node = get_new_node('main');
+$node->init;
+$node->append_conf('postgresql.conf',
+				   "ssl_passphrase.passphrase = '$rot13pass'");
+$node->append_conf('postgresql.conf',
+				   "shared_preload_libraries = 'ssl_passphrase_func'");
+$node->append_conf('postgresql.conf',
+				   "listen_addresses = 'localhost'");
+$node->append_conf('postgresql.conf',
+				   "ssl = 'on'");
+
+my $ddir = $node->data_dir;
+
+# install certificate and protected key
+move("server.crt", $ddir);
+move("server.key", $ddir);
+chmod 0600, "$ddir/server.key";
+
+$node->start;
+
+# if the server is running we must had successfully transformed the passphrase
+ok(-e "$ddir/postmaster.pid","postgres started");
+
+$node->stop('fast');
+
+# set the wrong passphrase
+$node->append_conf('postgresql.conf',
+				   "ssl_passphrase.passphrase = 'blurfl'");
+
+# try to start the server again
+my $ret = TestLib::system_log('pg_ctl', '-D', $node->data_dir, '-l',
+							  $node->logfile, 'start');
+
+
+# with a bad passphrase the server should not start
+ok($ret, "pg_ctl fails with bad passphrase");
+ok(! -e "$ddir/postmaster.pid","postgres not started with bad passphrase");
+
+# just in case
+$node->stop('fast');
+
+done_testing();
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 9a0963a050..0fd887b778 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -429,7 +429,7 @@ sub mkvcbuild
 
 	if (!$solution->{options}->{openssl})
 	{
-		push @contrib_excludes, 'sslinfo';
+		push @contrib_excludes, 'sslinfo', 'ssl_passphrase_callback';
 	}
 
 	if (!$solution->{options}->{uuid})
#11Bruce Momjian
bruce@momjian.us
In reply to: Magnus Hagander (#9)
Re: ssl passphrase callback

On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:

On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian <bruce@momjian.us> wrote:
� We had this
discussion in relation to archive_command years ago, and decided on a
shell command as the best API.

I don't recall that from back then, but that was a long time ago.

But it's interesting that you mention it, given the number of people I have
been discussing that with recently, under the topic of changing it from a shell
command into a shared library API (with there being a shell command as one
possible implementation of course).�

One of the main reasons there being to be easily able to transfer more state
and give results other than just an exit code, no need to deal with parameter
escaping etc. Which probably wouldn't matter as much to an SSL passphrase
command, but still.

I get the callback-is-easier issue with shared objects, but are we
expecting to pass in more information here than we do for
archive_command? I would think not. What I am saying is that if we
don't think passing things in works, we should fix all these external
commands, or something. I don't see why ssl_passphrase_command is
different, except that it is new. Or is it related to _securely_
passing something?

Also, why was this patch posted without any discussion of these issues?
Shouldn't we ideally discuss the API first?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +
#12Bruce Momjian
bruce@momjian.us
In reply to: Bruce Momjian (#11)
Re: ssl passphrase callback

On Tue, Nov 12, 2019 at 09:51:33PM -0500, Bruce Momjian wrote:

On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:

On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian <bruce@momjian.us> wrote:
� We had this
discussion in relation to archive_command years ago, and decided on a
shell command as the best API.

I don't recall that from back then, but that was a long time ago.

But it's interesting that you mention it, given the number of people I have
been discussing that with recently, under the topic of changing it from a shell
command into a shared library API (with there being a shell command as one
possible implementation of course).�

One of the main reasons there being to be easily able to transfer more state
and give results other than just an exit code, no need to deal with parameter
escaping etc. Which probably wouldn't matter as much to an SSL passphrase
command, but still.

I get the callback-is-easier issue with shared objects, but are we
expecting to pass in more information here than we do for
archive_command? I would think not. What I am saying is that if we
don't think passing things in works, we should fix all these external
commands, or something. I don't see why ssl_passphrase_command is
different, except that it is new. Or is it related to _securely_
passing something?

Also, why was this patch posted without any discussion of these issues?
Shouldn't we ideally discuss the API first?

I wonder if every GUC that takes an OS command should allow a shared
object to be specified --- maybe control that if the command string
starts with a # or something.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +
#13Simon Riggs
simon@2ndquadrant.com
In reply to: Bruce Momjian (#12)
Re: ssl passphrase callback

On Wed, 13 Nov 2019 at 13:08, Bruce Momjian <bruce@momjian.us> wrote:

On Tue, Nov 12, 2019 at 09:51:33PM -0500, Bruce Momjian wrote:

On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:

On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian <bruce@momjian.us> wrote:

One of the main reasons there being to be easily able to transfer more

state

and give results other than just an exit code, no need to deal with

parameter

escaping etc. Which probably wouldn't matter as much to an SSL

passphrase

command, but still.

I get the callback-is-easier issue with shared objects, but are we
expecting to pass in more information here than we do for
archive_command? I would think not. What I am saying is that if we
don't think passing things in works, we should fix all these external
commands, or something. I don't see why ssl_passphrase_command is
different, except that it is new.

Or is it related to _securely_passing something?

Yes

Also, why was this patch posted without any discussion of these issues?
Shouldn't we ideally discuss the API first?

I wonder if every GUC that takes an OS command should allow a shared
object to be specified --- maybe control that if the command string
starts with a # or something.

Very good idea

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Solutions for the Enterprise

#14Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Bruce Momjian (#12)
Re: ssl passphrase callback

On 11/13/19 8:08 AM, Bruce Momjian wrote:

Also, why was this patch posted without any discussion of these issues?
Shouldn't we ideally discuss the API first?

This is a very tiny patch. I don't think it's unusual to post such
things without prior discussion. I would agree with your point if it
were thousands of lines instead of 20 or so lines of core code.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#15Tomas Vondra
tomas.vondra@2ndquadrant.com
In reply to: Simon Riggs (#13)
Re: ssl passphrase callback

On Wed, Nov 13, 2019 at 01:20:43PM +0000, Simon Riggs wrote:

On Wed, 13 Nov 2019 at 13:08, Bruce Momjian <bruce@momjian.us> wrote:

On Tue, Nov 12, 2019 at 09:51:33PM -0500, Bruce Momjian wrote:

On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:

On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian <bruce@momjian.us> wrote:

One of the main reasons there being to be easily able to transfer more

state

and give results other than just an exit code, no need to deal with

parameter

escaping etc. Which probably wouldn't matter as much to an SSL

passphrase

command, but still.

I get the callback-is-easier issue with shared objects, but are we
expecting to pass in more information here than we do for
archive_command? I would think not. What I am saying is that if we
don't think passing things in works, we should fix all these external
commands, or something. I don't see why ssl_passphrase_command is
different, except that it is new.

Or is it related to _securely_passing something?

Yes

I think it would be beneficial to explain why shared object is more
secure than an OS command. Perhaps it's common knowledge, but it's not
quite obvious to me.

Also, why was this patch posted without any discussion of these issues?
Shouldn't we ideally discuss the API first?

I wonder if every GUC that takes an OS command should allow a shared
object to be specified --- maybe control that if the command string
starts with a # or something.

Very good idea

If it's about securely passing sensitive information (i.e. passphrase)
as was suggested, then I think that only applies to fairly small number
of GUCs.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#16Tomas Vondra
tomas.vondra@2ndquadrant.com
In reply to: Andrew Dunstan (#14)
Re: ssl passphrase callback

On Wed, Nov 13, 2019 at 02:48:01PM -0500, Andrew Dunstan wrote:

On 11/13/19 8:08 AM, Bruce Momjian wrote:

Also, why was this patch posted without any discussion of these issues?
Shouldn't we ideally discuss the API first?

This is a very tiny patch. I don't think it's unusual to post such
things without prior discussion. I would agree with your point if it
were thousands of lines instead of 20 or so lines of core code.

Not sure that's really true. I think patches should provide some basic
explanation why the feature is desirable, no matter the number of lines.

Also, there were vague references to issues with passing parameters to
archive_command. A link to details, past discussion, or brief
explanation would be nice.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#17Magnus Hagander
magnus@hagander.net
In reply to: Tomas Vondra (#15)
Re: ssl passphrase callback

On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra <tomas.vondra@2ndquadrant.com>
wrote:

On Wed, Nov 13, 2019 at 01:20:43PM +0000, Simon Riggs wrote:

On Wed, 13 Nov 2019 at 13:08, Bruce Momjian <bruce@momjian.us> wrote:

On Tue, Nov 12, 2019 at 09:51:33PM -0500, Bruce Momjian wrote:

On Sun, Nov 10, 2019 at 01:01:17PM -0600, Magnus Hagander wrote:

On Wed, Nov 6, 2019 at 7:24 PM Bruce Momjian <bruce@momjian.us>

wrote:

One of the main reasons there being to be easily able to transfer

more

state

and give results other than just an exit code, no need to deal with

parameter

escaping etc. Which probably wouldn't matter as much to an SSL

passphrase

command, but still.

I get the callback-is-easier issue with shared objects, but are we
expecting to pass in more information here than we do for
archive_command? I would think not. What I am saying is that if we
don't think passing things in works, we should fix all these external
commands, or something. I don't see why ssl_passphrase_command is
different, except that it is new.

Or is it related to _securely_passing something?

Yes

I think it would be beneficial to explain why shared object is more
secure than an OS command. Perhaps it's common knowledge, but it's not
quite obvious to me.

Yeah, that probably wouldn't hurt. It's also securely passing from more
than one perspective -- both from the "cannot be eavesdropped" (like
putting the password on the commandline for example) and the requirement
for escaping.

Also, why was this patch posted without any discussion of these

issues?

Shouldn't we ideally discuss the API first?

I wonder if every GUC that takes an OS command should allow a shared
object to be specified --- maybe control that if the command string
starts with a # or something.

Very good idea

If it's about securely passing sensitive information (i.e. passphrase)
as was suggested, then I think that only applies to fairly small number
of GUCs.

There aren't exactly a large number of GUCs that take OS commands in total.
Consistency itself certainly has some value.

--
Magnus Hagander
Me: https://www.hagander.net/ <http://www.hagander.net/&gt;
Work: https://www.redpill-linpro.com/ <http://www.redpill-linpro.com/&gt;

#18Sehrope Sarkuni
sehrope@jackdb.com
In reply to: Tomas Vondra (#15)
Re: ssl passphrase callback

On Wed, Nov 13, 2019 at 3:23 PM Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:

I think it would be beneficial to explain why shared object is more
secure than an OS command. Perhaps it's common knowledge, but it's not
quite obvious to me.

External command args can be viewed by other OS users (not just the
postgres user). For non-sensitive arguments (ex: WAL filename?) that's
not an issue but if you plan on passing in something potentially
secret value from the backend to the external OS command, that value
would be exposed:

Terminal 1 (run a command as some other user):
$ sudo -u nobody sleep 5

Terminal 2 (view command args as a different non-super user):
$ ps -u nobody -o args
COMMAND
sleep 5

A shared library would not have this problem as the backend directly
executes the library in the same process.

Has the idea of using environment variables (rather than command line
args) for external commands been brought up before? I couldn't find
anything in the mailing list archives.

Environment variables have the advantage of only being readable by the
process owner and super user. They also naturally have a "name" and do
not have escaping or quoting issues.

For example, archive_command %p could be exposed as
PG_ARG_ARCHIVE_PATH and %f could be exposed as
PG_ARG_ARCHIVE_FILENAME. Then you could have a script use them via:

#!/usr/bin/env bash
set -euo pipefail
main () {
local archive_dir="/path/to/archive_dir/"
local archive_file="${archive_dir}${PG_ARG_ARCHIVE_FILENAME}"
test ! -f "${archive_file}" && cp -- "${PG_ARG_ARCHIVE_PATH}"
"${archive_file}"
}
main "$@"

It's not particularly useful for that basic archive case but if
there's something like PG_ARG_SUPER_SECRET then the executed command
could receive that value without it being exposed. That'd be useful
for something like a callout to an external KMS (key management
system).

Nothing stops something like this with being used in tandem with
string substitution to create the full commands. That'd give backwards
compatibility too. The main limitation compared to a shared library is
that you'd still have to explicitly pick and name the exposed argument
environment variables (i.e. like picking the set of %? substitutions).
If there's a generic shared-library-for-external-commands approach
then there could be a built-in library that provides this type of
"expose as env vars" functionality.

Regards,
-- Sehrope Sarkuni
Founder & CEO | JackDB, Inc. | https://www.jackdb.com/

#19Bruce Momjian
bruce@momjian.us
In reply to: Magnus Hagander (#17)
Re: ssl passphrase callback

On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:

On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra <tomas.vondra@2ndquadrant.com>
I think it would be beneficial to explain why shared object is more
secure than an OS command. Perhaps it's common knowledge, but it's not
quite obvious to me.

Yeah, that probably wouldn't hurt. It's also securely passing from more than
one perspective -- both from the "cannot be eavesdropped" (like putting the
password on the commandline for example) and the requirement for escaping.

I think a bigger issue is that if you want to give people the option of
using a shell command or a shared object, and if you use two commands to
control it, it isn't clear what happens if both are defined. By using
some character prefix to control if a shared object is used, you can use
a single variable and there is no confusion over having two variables
and their conflicting behavior.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +
#20Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Bruce Momjian (#19)
Re: ssl passphrase callback

On 11/14/19 11:07 AM, Bruce Momjian wrote:

On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:

On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra <tomas.vondra@2ndquadrant.com>
I think it would be beneficial to explain why shared object is more
secure than an OS command. Perhaps it's common knowledge, but it's not
quite obvious to me.

Yeah, that probably wouldn't hurt. It's also securely passing from more than
one perspective -- both from the "cannot be eavesdropped" (like putting the
password on the commandline for example) and the requirement for escaping.

I think a bigger issue is that if you want to give people the option of
using a shell command or a shared object, and if you use two commands to
control it, it isn't clear what happens if both are defined. By using
some character prefix to control if a shared object is used, you can use
a single variable and there is no confusion over having two variables
and their conflicting behavior.

I'm  not sure how that would work in the present instance. The shared
preloaded module installs a function and defines the params it wants. If
we somehow unify the params with ssl_passphrase_command that could look
icky, and the module would have to parse the settings string. That's not
a problem for the sample module which only needs one param, but it will
be for other more complex implementations.

I'm quite open to suggestions, but I want things to be tolerably clean.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#21Tomas Vondra
tomas.vondra@2ndquadrant.com
In reply to: Andrew Dunstan (#20)
Re: ssl passphrase callback

On Thu, Nov 14, 2019 at 11:34:24AM -0500, Andrew Dunstan wrote:

On 11/14/19 11:07 AM, Bruce Momjian wrote:

On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:

On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra <tomas.vondra@2ndquadrant.com>
I think it would be beneficial to explain why shared object is more
secure than an OS command. Perhaps it's common knowledge, but it's not
quite obvious to me.

Yeah, that probably wouldn't hurt. It's also securely passing from more than
one perspective -- both from the "cannot be eavesdropped" (like putting the
password on the commandline for example) and the requirement for escaping.

I think a bigger issue is that if you want to give people the option of
using a shell command or a shared object, and if you use two commands to
control it, it isn't clear what happens if both are defined. By using
some character prefix to control if a shared object is used, you can use
a single variable and there is no confusion over having two variables
and their conflicting behavior.

I'm� not sure how that would work in the present instance. The shared
preloaded module installs a function and defines the params it wants. If
we somehow unify the params with ssl_passphrase_command that could look
icky, and the module would have to parse the settings string. That's not
a problem for the sample module which only needs one param, but it will
be for other more complex implementations.

I'm quite open to suggestions, but I want things to be tolerably clean.

I agree it's better to have two separate GUCs - one for command, one for
shared object, and documented order of precedence. I suppose we may log
a warning when both are specified, or perhaps "reset" the value with
lower order of precedence.

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#22Bruce Momjian
bruce@momjian.us
In reply to: Andrew Dunstan (#20)
Re: ssl passphrase callback

On Thu, Nov 14, 2019 at 11:34:24AM -0500, Andrew Dunstan wrote:

On 11/14/19 11:07 AM, Bruce Momjian wrote:

On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:

On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra <tomas.vondra@2ndquadrant.com>
I think it would be beneficial to explain why shared object is more
secure than an OS command. Perhaps it's common knowledge, but it's not
quite obvious to me.

Yeah, that probably wouldn't hurt. It's also securely passing from more than
one perspective -- both from the "cannot be eavesdropped" (like putting the
password on the commandline for example) and the requirement for escaping.

I think a bigger issue is that if you want to give people the option of
using a shell command or a shared object, and if you use two commands to
control it, it isn't clear what happens if both are defined. By using
some character prefix to control if a shared object is used, you can use
a single variable and there is no confusion over having two variables
and their conflicting behavior.

I'm� not sure how that would work in the present instance. The shared
preloaded module installs a function and defines the params it wants. If
we somehow unify the params with ssl_passphrase_command that could look
icky, and the module would have to parse the settings string. That's not
a problem for the sample module which only needs one param, but it will
be for other more complex implementations.

I'm quite open to suggestions, but I want things to be tolerably clean.

I was assuming if the variable starts with a #, it is a shared object,
if not, it is a shell command:

ssl_passphrase_command='#/lib/x.so'
ssl_passphrase_command='my_command a b c'

Can you show what you are talking about?

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +
#23Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Bruce Momjian (#22)
Re: ssl passphrase callback

On 2019-Nov-14, Bruce Momjian wrote:

I was assuming if the variable starts with a #, it is a shared object,
if not, it is a shell command:

ssl_passphrase_command='#/lib/x.so'
ssl_passphrase_command='my_command a b c'

Note that the proposed patch doesn't use a separate GUC -- it just uses
shared_preload_libraries, and then it is the library that's in charge of
setting up the function. We probably wouldn't like to have multiple
settings that all do the same thing, such as recovery target (which
seems to be a plentiful source of confusion).

Changing the interface so that the user has to specify the function name
(not the library name) in ssl_passphrase_command closes that ambiguity
hole.

Note that if you specify only the library name, it becomes redundant
w.r.t. shared_preload_libraries; you could have more than one library
setting the function callback and it's hard to see which one wins.

I think something like this would do it:
ssl_passphrase_command='#superlib.so,my_rot13_passphrase'

This way, the library can still create any custom GUCs it pleases/needs,
but there's no possible confusion as to the function that's going to be
called.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#24Bruce Momjian
bruce@momjian.us
In reply to: Alvaro Herrera (#23)
Re: ssl passphrase callback

On Thu, Nov 14, 2019 at 02:07:58PM -0300, Alvaro Herrera wrote:

On 2019-Nov-14, Bruce Momjian wrote:

I was assuming if the variable starts with a #, it is a shared object,
if not, it is a shell command:

ssl_passphrase_command='#/lib/x.so'
ssl_passphrase_command='my_command a b c'

Note that the proposed patch doesn't use a separate GUC -- it just uses
shared_preload_libraries, and then it is the library that's in charge of
setting up the function. We probably wouldn't like to have multiple
settings that all do the same thing, such as recovery target (which
seems to be a plentiful source of confusion).

Changing the interface so that the user has to specify the function name
(not the library name) in ssl_passphrase_command closes that ambiguity
hole.

Note that if you specify only the library name, it becomes redundant
w.r.t. shared_preload_libraries; you could have more than one library
setting the function callback and it's hard to see which one wins.

I think something like this would do it:
ssl_passphrase_command='#superlib.so,my_rot13_passphrase'

This way, the library can still create any custom GUCs it pleases/needs,
but there's no possible confusion as to the function that's going to be
called.

Yeah, I was unclear how the function name would be specified. I thought
it would just be hard-coded, but I like the above better. I am still
unclear how parameters are passed.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+                      Ancient Roman grave inscription +
#25Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Alvaro Herrera (#23)
Re: ssl passphrase callback

On 11/14/19 12:07 PM, Alvaro Herrera wrote:

On 2019-Nov-14, Bruce Momjian wrote:

I was assuming if the variable starts with a #, it is a shared object,
if not, it is a shell command:

ssl_passphrase_command='#/lib/x.so'
ssl_passphrase_command='my_command a b c'

Note that the proposed patch doesn't use a separate GUC -- it just uses
shared_preload_libraries, and then it is the library that's in charge of
setting up the function. We probably wouldn't like to have multiple
settings that all do the same thing, such as recovery target (which
seems to be a plentiful source of confusion).

Changing the interface so that the user has to specify the function name
(not the library name) in ssl_passphrase_command closes that ambiguity
hole.

Note that if you specify only the library name, it becomes redundant
w.r.t. shared_preload_libraries; you could have more than one library
setting the function callback and it's hard to see which one wins.

I think something like this would do it:
ssl_passphrase_command='#superlib.so,my_rot13_passphrase'

This way, the library can still create any custom GUCs it pleases/needs,
but there's no possible confusion as to the function that's going to be
called.

I guess this would work. There would have to be a deal of code to load
the library and lookup the symbol. Do we really think it's worth it?
Leveraging shared_preload_libraries makes this comparatively simple.

Also, calling this 'ssl_passphrase_command' seems a little odd.

A simpler way to handle it might be simply to error out and refuse to
start if both ssl_passphrase_function is set and ssl_passphrase_command
is set.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#26Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Andrew Dunstan (#25)
Re: ssl passphrase callback

On 2019-Nov-14, Andrew Dunstan wrote:

I guess this would work. There would have to be a deal of code to load
the library and lookup the symbol. Do we really think it's worth it?
Leveraging shared_preload_libraries makes this comparatively simple.

Using the generic interface has the drawback that the user can make more
mistakes. I think that's part of Bruce's issue with it (although I may
misinterpret.)

I think if you add most of it as a new entry point in dfmgr.c (where you
can leverage internal_library_load) and returns a function pointer to
the user specified function, it's all that much additional code.

(I don't think you can use load_external_function as is, because it
assumes fmgr V1 calling convention, which I'm not sure serves your case.
But then maybe it does. And if not, then those 10 lines should be very
similar to the code you'd need to add.)

A simpler way to handle it might be simply to error out and refuse to
start if both ssl_passphrase_function is set and ssl_passphrase_command
is set.

Yeah, you can do that too I guess, but I'm not sure I see that as simpler.

Also, calling this 'ssl_passphrase_command' seems a little odd.

We could just rename ssl_passphrase_command to something more
generic, and add the existing name to map_old_guc_names to preserve
compatibility with pg12. Maybe the new name could be simply
ssl_passphrase or perhaps ssl_passphrase_{reader,getter,pinentry}.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#27Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Alvaro Herrera (#26)
Re: ssl passphrase callback

On 11/14/19 3:21 PM, Alvaro Herrera wrote:

On 2019-Nov-14, Andrew Dunstan wrote:

I guess this would work. There would have to be a deal of code to load
the library and lookup the symbol. Do we really think it's worth it?
Leveraging shared_preload_libraries makes this comparatively simple.

Using the generic interface has the drawback that the user can make more
mistakes. I think that's part of Bruce's issue with it (although I may
misinterpret.)

I think if you add most of it as a new entry point in dfmgr.c (where you
can leverage internal_library_load) and returns a function pointer to
the user specified function, it's all that much additional code.

(I don't think you can use load_external_function as is, because it
assumes fmgr V1 calling convention, which I'm not sure serves your case.
But then maybe it does. And if not, then those 10 lines should be very
similar to the code you'd need to add.)

In the absence of further comment I will try to code up something along
these lines.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#28Craig Ringer
craig@2ndquadrant.com
In reply to: Andrew Dunstan (#20)
Re: ssl passphrase callback

On Fri, 15 Nov 2019 at 00:34, Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
wrote:

On 11/14/19 11:07 AM, Bruce Momjian wrote:

On Thu, Nov 14, 2019 at 11:42:05AM +0100, Magnus Hagander wrote:

On Wed, Nov 13, 2019 at 9:23 PM Tomas Vondra <

tomas.vondra@2ndquadrant.com>

I think it would be beneficial to explain why shared object is more
secure than an OS command. Perhaps it's common knowledge, but it's

not

quite obvious to me.

Yeah, that probably wouldn't hurt. It's also securely passing from more

than

one perspective -- both from the "cannot be eavesdropped" (like putting

the

password on the commandline for example) and the requirement for

escaping.

I think a bigger issue is that if you want to give people the option of
using a shell command or a shared object, and if you use two commands to
control it, it isn't clear what happens if both are defined. By using
some character prefix to control if a shared object is used, you can use
a single variable and there is no confusion over having two variables
and their conflicting behavior.

I'm not sure how that would work in the present instance. The shared
preloaded module installs a function and defines the params it wants. If
we somehow unify the params with ssl_passphrase_command that could look
icky, and the module would have to parse the settings string. That's not
a problem for the sample module which only needs one param, but it will
be for other more complex implementations.

I'm quite open to suggestions, but I want things to be tolerably clean.

If someone wants a shell command wrapper, they can load a contrib that
delegates the hook to a shell command. So we can just ship a contrib, which
acts both as test coverage for the feature, and a shell-command-support
wrapper for anyone who desires that.

--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise

#29Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Andrew Dunstan (#27)
Re: ssl passphrase callback

On 11/15/19 8:59 AM, Andrew Dunstan wrote:

On 11/14/19 3:21 PM, Alvaro Herrera wrote:

On 2019-Nov-14, Andrew Dunstan wrote:

I guess this would work. There would have to be a deal of code to load
the library and lookup the symbol. Do we really think it's worth it?
Leveraging shared_preload_libraries makes this comparatively simple.

Using the generic interface has the drawback that the user can make more
mistakes. I think that's part of Bruce's issue with it (although I may
misinterpret.)

I think if you add most of it as a new entry point in dfmgr.c (where you
can leverage internal_library_load) and returns a function pointer to
the user specified function, it's all that much additional code.

(I don't think you can use load_external_function as is, because it
assumes fmgr V1 calling convention, which I'm not sure serves your case.
But then maybe it does. And if not, then those 10 lines should be very
similar to the code you'd need to add.)

I've just been looking at that. load_external_function() doesn't
actually do anything V1-ish with the value, it just looks up the symbol
using dlsym and returns it cast to a PGFunction. Is there any reason I
can't just use that and cast it again to the callback function type?

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#30Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andrew Dunstan (#29)
Re: ssl passphrase callback

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

I've just been looking at that. load_external_function() doesn't
actually do anything V1-ish with the value, it just looks up the symbol
using dlsym and returns it cast to a PGFunction. Is there any reason I
can't just use that and cast it again to the callback function type?

TBH, I think this entire discussion has gone seriously off into the
weeds. The original design where we just let a shared_preload_library
function get into a hook is far superior to any of the overcomplicated
kluges that are being discussed now. Something like this, for instance:

ssl_passphrase_command='#superlib.so,my_rot13_passphrase'

makes me positively ill. It introduces problems that we don't need,
like how to parse out the sub-parts of the string, and the
quoting/escaping issues that will come along with that; while from
the user's perspective it replaces a simple and intellectually-coherent
variable definition with an unintelligible mess.

regards, tom lane

#31Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Tom Lane (#30)
Re: ssl passphrase callback

On 12/6/19 6:20 PM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

I've just been looking at that. load_external_function() doesn't
actually do anything V1-ish with the value, it just looks up the symbol
using dlsym and returns it cast to a PGFunction. Is there any reason I
can't just use that and cast it again to the callback function type?

TBH, I think this entire discussion has gone seriously off into the
weeds. The original design where we just let a shared_preload_library
function get into a hook is far superior to any of the overcomplicated
kluges that are being discussed now. Something like this, for instance:

ssl_passphrase_command='#superlib.so,my_rot13_passphrase'

makes me positively ill. It introduces problems that we don't need,
like how to parse out the sub-parts of the string, and the
quoting/escaping issues that will come along with that; while from
the user's perspective it replaces a simple and intellectually-coherent
variable definition with an unintelligible mess.

Yeah, you have a point.

Bruce was worried about what would happen if we defined both
ssl_passphrase_command and ssl_passphrase_callback. The submitted patch
let's the callback have precedence, but it might be cleaner to error out
with such a config. OTOH, that wouldn't be so nice on a reload, so it
might be better just to document the behaviour.

He was also worried that multiple shared libraries might try to provide
the hook. I think that's fairly fanciful, TBH. It comes into the
category of "Don't do that."

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#32Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andrew Dunstan (#31)
Re: ssl passphrase callback

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

Bruce was worried about what would happen if we defined both
ssl_passphrase_command and ssl_passphrase_callback. The submitted patch
let's the callback have precedence, but it might be cleaner to error out
with such a config. OTOH, that wouldn't be so nice on a reload, so it
might be better just to document the behaviour.

I think it would be up to the extension that's using the hook to
decide what to do if ssl_passphrase_command is set. It would not
be our choice, and it would certainly not fall to us to document it.

He was also worried that multiple shared libraries might try to provide
the hook. I think that's fairly fanciful, TBH. It comes into the
category of "Don't do that."

Again, it's somebody else's problem. We have plenty of hooks that
are of dubious use for multiple extensions, so why should this one be
held to a higher standard?

regards, tom lane

#33Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Tom Lane (#32)
Re: ssl passphrase callback

On 12/7/19 12:16 PM, Tom Lane wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

Bruce was worried about what would happen if we defined both
ssl_passphrase_command and ssl_passphrase_callback. The submitted patch
let's the callback have precedence, but it might be cleaner to error out
with such a config. OTOH, that wouldn't be so nice on a reload, so it
might be better just to document the behaviour.

I think it would be up to the extension that's using the hook to
decide what to do if ssl_passphrase_command is set. It would not
be our choice, and it would certainly not fall to us to document it.

He was also worried that multiple shared libraries might try to provide
the hook. I think that's fairly fanciful, TBH. It comes into the
category of "Don't do that."

Again, it's somebody else's problem. We have plenty of hooks that
are of dubious use for multiple extensions, so why should this one be
held to a higher standard?

Well that pretty much brings us back to the patch as submitted :-)

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#34Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andrew Dunstan (#33)
Re: ssl passphrase callback

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

Well that pretty much brings us back to the patch as submitted :-)

Yeah, pretty nearly. Taking a quick look over the v3 patch, my
only quibble is that it doesn't provide any convenient way for the
external module to make decisions about how to interact with
ssl_passphrase_command --- in particular, if it would like to allow
that to take precedence, it can't because there's no way for it to
invoke the static function ssl_external_passwd_cb.

But rather than expose that globally, maybe the theory ought to be
"set up the state as we'd normally do, then let loadable modules
choose to override it". So I'm tempted to propose a hook function
with the signature

void openssl_tls_init_hook(SSL_CTX *context, bool isServerStart);

and invoke that somewhere in be_tls_init --- maybe fairly late,
so that it can override other settings if it wants, not only the
SSL_CTX_set_default_passwd_cb setting.

regards, tom lane

#35Craig Ringer
craig@2ndquadrant.com
In reply to: Tom Lane (#30)
Re: ssl passphrase callback

On Sat, 7 Dec 2019 at 07:21, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

I've just been looking at that. load_external_function() doesn't
actually do anything V1-ish with the value, it just looks up the symbol
using dlsym and returns it cast to a PGFunction. Is there any reason I
can't just use that and cast it again to the callback function type?

TBH, I think this entire discussion has gone seriously off into the
weeds. The original design where we just let a shared_preload_library
function get into a hook is far superior to any of the overcomplicated
kluges that are being discussed now. Something like this, for instance:

ssl_passphrase_command='#superlib.so,my_rot13_passphrase'

makes me positively ill. It introduces problems that we don't need,
like how to parse out the sub-parts of the string, and the
quoting/escaping issues that will come along with that; while from
the user's perspective it replaces a simple and intellectually-coherent
variable definition with an unintelligible mess.

+1000 from me on that.

--
Craig Ringer http://www.2ndQuadrant.com/
2ndQuadrant - PostgreSQL Solutions for the Enterprise

#36Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Tom Lane (#34)
1 attachment(s)
Re: ssl passphrase callback

On Sun, Dec 8, 2019 at 9:02 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:

Well that pretty much brings us back to the patch as submitted :-)

Yeah, pretty nearly. Taking a quick look over the v3 patch, my
only quibble is that it doesn't provide any convenient way for the
external module to make decisions about how to interact with
ssl_passphrase_command --- in particular, if it would like to allow
that to take precedence, it can't because there's no way for it to
invoke the static function ssl_external_passwd_cb.

But rather than expose that globally, maybe the theory ought to be
"set up the state as we'd normally do, then let loadable modules
choose to override it". So I'm tempted to propose a hook function
with the signature

void openssl_tls_init_hook(SSL_CTX *context, bool isServerStart);

and invoke that somewhere in be_tls_init --- maybe fairly late,
so that it can override other settings if it wants, not only the
SSL_CTX_set_default_passwd_cb setting.

Not sure if the placement is what you want, but maybe something like this?

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

ssl-passphrase-callback-4.patchtext/x-patch; charset=US-ASCII; name=ssl-passphrase-callback-4.patchDownload
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 0cc59f1be1..af9f1f71d5 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -45,6 +45,9 @@
 #include "tcop/tcopprot.h"
 #include "utils/memutils.h"
 
+/* default init hook can be overridden by a shared library */
+static void  default_openssl_tls_init(SSL_CTX *context, bool isServerStart);
+openssl_tls_init_hook_typ openssl_tls_init_hook = default_openssl_tls_init;
 
 static int	my_sock_read(BIO *h, char *buf, int size);
 static int	my_sock_write(BIO *h, const char *buf, int size);
@@ -116,27 +119,10 @@ be_tls_init(bool isServerStart)
 	SSL_CTX_set_mode(context, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
 
 	/*
-	 * Set password callback
+	 * Call init hook (usually to set password callback)
 	 */
-	if (isServerStart)
-	{
-		if (ssl_passphrase_command[0])
-			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
-	}
-	else
-	{
-		if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
-			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
-		else
+	(* openssl_tls_init_hook)(context, isServerStart);
 
-			/*
-			 * If reloading and no external command is configured, override
-			 * OpenSSL's default handling of passphrase-protected files,
-			 * because we don't want to prompt for a passphrase in an
-			 * already-running server.
-			 */
-			SSL_CTX_set_default_passwd_cb(context, dummy_ssl_passwd_cb);
-	}
 	/* used by the callback */
 	ssl_is_server_start = isServerStart;
 
@@ -1310,3 +1296,27 @@ ssl_protocol_version_to_openssl(int v, const char *guc_name, int loglevel)
 					GetConfigOption(guc_name, false, false))));
 	return -1;
 }
+
+
+static void
+default_openssl_tls_init(SSL_CTX *context, bool isServerStart)
+{
+	if (isServerStart)
+	{
+		if (ssl_passphrase_command[0])
+			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
+	}
+	else
+	{
+		if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
+			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
+		else
+			/*
+			 * If reloading and no external command is configured, override
+			 * OpenSSL's default handling of passphrase-protected files,
+			 * because we don't want to prompt for a passphrase in an
+			 * already-running server.
+			 */
+			SSL_CTX_set_default_passwd_cb(context, dummy_ssl_passwd_cb);
+	}
+}
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 7a92dac525..9e2777c896 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -972,17 +972,6 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	LocalProcessControlFile(false);
 
-	/*
-	 * Initialize SSL library, if specified.
-	 */
-#ifdef USE_SSL
-	if (EnableSSL)
-	{
-		(void) secure_initialize(true);
-		LoadedSSL = true;
-	}
-#endif
-
 	/*
 	 * Register the apply launcher.  Since it registers a background worker,
 	 * it needs to be called before InitializeMaxBackends(), and it's probably
@@ -996,6 +985,17 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	process_shared_preload_libraries();
 
+	/*
+	 * Initialize SSL library, if specified.
+	 */
+#ifdef USE_SSL
+	if (EnableSSL)
+	{
+		(void) secure_initialize(true);
+		LoadedSSL = true;
+	}
+#endif
+
 	/*
 	 * Now that loadable modules have had their chance to register background
 	 * workers, calculate MaxBackends.
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 82e57afc64..90f80bb33a 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -287,6 +287,10 @@ extern void be_tls_get_peer_serial(Port *port, char *ptr, size_t len);
 extern char *be_tls_get_certificate_hash(Port *port, size_t *len);
 #endif
 
+/* init hook for SSL, the default sets the passwor callback if appropriate */
+typedef void(* openssl_tls_init_hook_typ)(SSL_CTX *context, bool isServerStart);
+extern openssl_tls_init_hook_typ openssl_tls_init_hook;
+
 #endif							/* USE_SSL */
 
 #ifdef ENABLE_GSS
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b2eaef3bff..5f975ebcba 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -25,4 +25,9 @@ SUBDIRS = \
 		  unsafe_tests \
 		  worker_spi
 
+ifeq ($(with_openssl),yes)
+SUBDIRS += ssl_passphrase_callback
+endif
+
+
 $(recurse)
diff --git a/src/test/modules/ssl_passphrase_callback/.gitignore b/src/test/modules/ssl_passphrase_callback/.gitignore
new file mode 100644
index 0000000000..1dbadf7baf
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/.gitignore
@@ -0,0 +1 @@
+tmp_check
diff --git a/src/test/modules/ssl_passphrase_callback/Makefile b/src/test/modules/ssl_passphrase_callback/Makefile
new file mode 100644
index 0000000000..e2d19f131a
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/Makefile
@@ -0,0 +1,24 @@
+# ssl_passphrase Makefile
+
+export with_openssl
+
+MODULE_big = ssl_passphrase_func
+OBJS = ssl_passphrase_func.o $(WIN32RES)
+PGFILEDESC = "callback function to provide a passphrase"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/ssl_passphrase_callback
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: prove-check
+
+prove-check: ssl_passphrase_func$(DLSUFFIX) | temp-install
+	@echo running prove ...
+	$(prove_check)
diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
new file mode 100644
index 0000000000..0964ba3508
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
@@ -0,0 +1,82 @@
+/*-------------------------------------------------------------------------
+ *
+ * ssl_passphrase_func.c
+ *
+ * Loadable PostgreSQL module fetch an ssl passphrase for the server cert.
+ * instead of calling an external program. This implementation just hands
+ * back the configured password rot13'd.
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include <float.h>
+#include <stdio.h>
+
+#include "libpq/libpq-be.h"
+#include "utils/guc.h"
+
+PG_MODULE_MAGIC;
+
+void		_PG_init(void);
+void		_PG_fini(void);
+
+static char *ssl_passphrase = NULL;
+
+/* callback function */
+static int rot13_passphrase(char *buf, int size, int rwflag, void *userdata);
+/* hook function to set the callback */
+static void set_rot13(SSL_CTX *context, bool isServerStart);
+/*
+ * Module load callback
+ */
+void
+_PG_init(void)
+{
+	/* Define custom GUC variable. */
+	DefineCustomStringVariable("ssl_passphrase.passphrase",
+							   "passphrase before transformation",
+							   NULL,
+							   &ssl_passphrase,
+							   NULL,
+							   PGC_SIGHUP,
+							   0,	/* no flags required */
+							   NULL,
+							   NULL,
+							   NULL);
+	if (ssl_passphrase)
+		openssl_tls_init_hook = set_rot13;
+}
+
+void
+_PG_fini(void)
+{
+	/* do  nothing yet */
+}
+
+static void
+set_rot13(SSL_CTX *context, bool isServerStart)
+{
+	SSL_CTX_set_default_passwd_cb(context, rot13_passphrase);
+}
+
+static int
+rot13_passphrase(char *buf, int size, int rwflag, void *userdata)
+{
+
+	Assert(ssl_passphrase != NULL);
+	StrNCpy(buf, ssl_passphrase, size);
+	for (char *p = buf; *p; p++)
+	{
+		char		c = *p;
+
+		if ((c >= 'a' && c <= 'm') || (c >= 'A' && c <= 'M'))
+			*p = c + 13;
+		else if ((c >= 'n' && c <= 'z') || (c >= 'N' && c <= 'Z'))
+			*p = c - 13;
+	}
+
+	return strlen(buf);
+
+}
diff --git a/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
new file mode 100644
index 0000000000..715d635c58
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
@@ -0,0 +1,67 @@
+
+use strict;
+use warnings;
+
+use File::Copy;
+
+use TestLib;
+use Test::More;
+use PostgresNode;
+
+unless ( ($ENV{with_openssl} || 'no') eq 'yes')
+{
+	plan skip_all => 'SSL not supported by this build';
+}
+
+my $clearpass = "FooBaR1";
+my $rot13pass = "SbbOnE1";
+
+# self-signed cert was generated like this:
+# system('openssl req -new -x509 -days 3650 -nodes -text -out server.crt -keyout server.ckey -subj "/CN=localhost"');
+# add the cleartext passphrase to the key, remove the unprotected key
+# system("openssl rsa -aes256 -in server.ckey -out server.key -passout pass:$clearpass");
+# unlink "server.ckey";
+
+
+my $node = get_new_node('main');
+$node->init;
+$node->append_conf('postgresql.conf',
+				   "ssl_passphrase.passphrase = '$rot13pass'");
+$node->append_conf('postgresql.conf',
+				   "shared_preload_libraries = 'ssl_passphrase_func'");
+$node->append_conf('postgresql.conf',
+				   "listen_addresses = 'localhost'");
+$node->append_conf('postgresql.conf',
+				   "ssl = 'on'");
+
+my $ddir = $node->data_dir;
+
+# install certificate and protected key
+move("server.crt", $ddir);
+move("server.key", $ddir);
+chmod 0600, "$ddir/server.key";
+
+$node->start;
+
+# if the server is running we must had successfully transformed the passphrase
+ok(-e "$ddir/postmaster.pid","postgres started");
+
+$node->stop('fast');
+
+# set the wrong passphrase
+$node->append_conf('postgresql.conf',
+				   "ssl_passphrase.passphrase = 'blurfl'");
+
+# try to start the server again
+my $ret = TestLib::system_log('pg_ctl', '-D', $node->data_dir, '-l',
+							  $node->logfile, 'start');
+
+
+# with a bad passphrase the server should not start
+ok($ret, "pg_ctl fails with bad passphrase");
+ok(! -e "$ddir/postmaster.pid","postgres not started with bad passphrase");
+
+# just in case
+$node->stop('fast');
+
+done_testing();
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index c735d529ca..2eb0311a14 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -429,7 +429,7 @@ sub mkvcbuild
 
 	if (!$solution->{options}->{openssl})
 	{
-		push @contrib_excludes, 'sslinfo';
+		push @contrib_excludes, 'sslinfo', 'ssl_passphrase_callback';
 	}
 
 	if (!$solution->{options}->{uuid})
#37Robert Haas
robertmhaas@gmail.com
In reply to: Sehrope Sarkuni (#18)
Re: ssl passphrase callback

On Thu, Nov 14, 2019 at 8:54 AM Sehrope Sarkuni <sehrope@jackdb.com> wrote:

Has the idea of using environment variables (rather than command line
args) for external commands been brought up before? I couldn't find
anything in the mailing list archives.

Passing data through environment variables isn't secure. Try 'ps -E'
on MacOS, or something like 'ps axe' on Linux.

If we want to pass data securely to child processes, the way to do it
is via stdin. Data sent back and forth via file descriptors can't
easily be snooped by other users on the system.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#38Thomas Munro
thomas.munro@gmail.com
In reply to: Andrew Dunstan (#36)
Re: ssl passphrase callback

On Wed, Jan 22, 2020 at 8:02 PM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

Not sure if the placement is what you want, but maybe something like this?

Hi Andrew, FYI this failed here:

t/001_testfunc.pl .. Bailout called. Further testing stopped: pg_ctl
start failed
FAILED--Further testing stopped: pg_ctl start failed
Makefile:23: recipe for target 'prove-check' failed

Unfortunately my robot is poorly trained and does not dump any of the
interesting logs for this case, but it looks like it's failing that
way every time.

https://travis-ci.org/postgresql-cfbot/postgresql/builds/651756191

#39Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Thomas Munro (#38)
Re: ssl passphrase callback

On Tue, Feb 18, 2020 at 2:01 PM Thomas Munro <thomas.munro@gmail.com> wrote:

On Wed, Jan 22, 2020 at 8:02 PM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

Not sure if the placement is what you want, but maybe something like this?

Hi Andrew, FYI this failed here:

t/001_testfunc.pl .. Bailout called. Further testing stopped: pg_ctl
start failed
FAILED--Further testing stopped: pg_ctl start failed
Makefile:23: recipe for target 'prove-check' failed

Unfortunately my robot is poorly trained and does not dump any of the
interesting logs for this case, but it looks like it's failing that
way every time.

https://travis-ci.org/postgresql-cfbot/postgresql/builds/651756191

Thanks for letting me know, I will investigate.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#40Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Andrew Dunstan (#39)
1 attachment(s)
Re: ssl passphrase callback

On Wed, Feb 19, 2020 at 7:10 AM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

On Tue, Feb 18, 2020 at 2:01 PM Thomas Munro <thomas.munro@gmail.com> wrote:

On Wed, Jan 22, 2020 at 8:02 PM Andrew Dunstan
<andrew.dunstan@2ndquadrant.com> wrote:

Not sure if the placement is what you want, but maybe something like this?

Hi Andrew, FYI this failed here:

t/001_testfunc.pl .. Bailout called. Further testing stopped: pg_ctl
start failed
FAILED--Further testing stopped: pg_ctl start failed
Makefile:23: recipe for target 'prove-check' failed

Unfortunately my robot is poorly trained and does not dump any of the
interesting logs for this case, but it looks like it's failing that
way every time.

https://travis-ci.org/postgresql-cfbot/postgresql/builds/651756191

Thanks for letting me know, I will investigate.

This should fix the issue, it happened when I switched to using a
pre-generated cert/key.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

ssl-passphrase-callback-5.patchtext/x-patch; charset=US-ASCII; name=ssl-passphrase-callback-5.patchDownload
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 18d3fff068..b0a8816cff 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -45,6 +45,9 @@
 #include "tcop/tcopprot.h"
 #include "utils/memutils.h"
 
+/* default init hook can be overridden by a shared library */
+static void  default_openssl_tls_init(SSL_CTX *context, bool isServerStart);
+openssl_tls_init_hook_typ openssl_tls_init_hook = default_openssl_tls_init;
 
 static int	my_sock_read(BIO *h, char *buf, int size);
 static int	my_sock_write(BIO *h, const char *buf, int size);
@@ -116,27 +119,10 @@ be_tls_init(bool isServerStart)
 	SSL_CTX_set_mode(context, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
 
 	/*
-	 * Set password callback
+	 * Call init hook (usually to set password callback)
 	 */
-	if (isServerStart)
-	{
-		if (ssl_passphrase_command[0])
-			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
-	}
-	else
-	{
-		if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
-			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
-		else
+	(* openssl_tls_init_hook)(context, isServerStart);
 
-			/*
-			 * If reloading and no external command is configured, override
-			 * OpenSSL's default handling of passphrase-protected files,
-			 * because we don't want to prompt for a passphrase in an
-			 * already-running server.
-			 */
-			SSL_CTX_set_default_passwd_cb(context, dummy_ssl_passwd_cb);
-	}
 	/* used by the callback */
 	ssl_is_server_start = isServerStart;
 
@@ -1313,3 +1299,27 @@ ssl_protocol_version_to_openssl(int v, const char *guc_name, int loglevel)
 					GetConfigOption(guc_name, false, false))));
 	return -1;
 }
+
+
+static void
+default_openssl_tls_init(SSL_CTX *context, bool isServerStart)
+{
+	if (isServerStart)
+	{
+		if (ssl_passphrase_command[0])
+			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
+	}
+	else
+	{
+		if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
+			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
+		else
+			/*
+			 * If reloading and no external command is configured, override
+			 * OpenSSL's default handling of passphrase-protected files,
+			 * because we don't want to prompt for a passphrase in an
+			 * already-running server.
+			 */
+			SSL_CTX_set_default_passwd_cb(context, dummy_ssl_passwd_cb);
+	}
+}
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index b3986bee75..53776d6198 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -972,17 +972,6 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	LocalProcessControlFile(false);
 
-	/*
-	 * Initialize SSL library, if specified.
-	 */
-#ifdef USE_SSL
-	if (EnableSSL)
-	{
-		(void) secure_initialize(true);
-		LoadedSSL = true;
-	}
-#endif
-
 	/*
 	 * Register the apply launcher.  Since it registers a background worker,
 	 * it needs to be called before InitializeMaxBackends(), and it's probably
@@ -996,6 +985,17 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	process_shared_preload_libraries();
 
+	/*
+	 * Initialize SSL library, if specified.
+	 */
+#ifdef USE_SSL
+	if (EnableSSL)
+	{
+		(void) secure_initialize(true);
+		LoadedSSL = true;
+	}
+#endif
+
 	/*
 	 * Now that loadable modules have had their chance to register background
 	 * workers, calculate MaxBackends.
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 82e57afc64..90f80bb33a 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -287,6 +287,10 @@ extern void be_tls_get_peer_serial(Port *port, char *ptr, size_t len);
 extern char *be_tls_get_certificate_hash(Port *port, size_t *len);
 #endif
 
+/* init hook for SSL, the default sets the passwor callback if appropriate */
+typedef void(* openssl_tls_init_hook_typ)(SSL_CTX *context, bool isServerStart);
+extern openssl_tls_init_hook_typ openssl_tls_init_hook;
+
 #endif							/* USE_SSL */
 
 #ifdef ENABLE_GSS
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b2eaef3bff..5f975ebcba 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -25,4 +25,9 @@ SUBDIRS = \
 		  unsafe_tests \
 		  worker_spi
 
+ifeq ($(with_openssl),yes)
+SUBDIRS += ssl_passphrase_callback
+endif
+
+
 $(recurse)
diff --git a/src/test/modules/ssl_passphrase_callback/.gitignore b/src/test/modules/ssl_passphrase_callback/.gitignore
new file mode 100644
index 0000000000..1dbadf7baf
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/.gitignore
@@ -0,0 +1 @@
+tmp_check
diff --git a/src/test/modules/ssl_passphrase_callback/Makefile b/src/test/modules/ssl_passphrase_callback/Makefile
new file mode 100644
index 0000000000..e2d19f131a
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/Makefile
@@ -0,0 +1,24 @@
+# ssl_passphrase Makefile
+
+export with_openssl
+
+MODULE_big = ssl_passphrase_func
+OBJS = ssl_passphrase_func.o $(WIN32RES)
+PGFILEDESC = "callback function to provide a passphrase"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/ssl_passphrase_callback
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: prove-check
+
+prove-check: ssl_passphrase_func$(DLSUFFIX) | temp-install
+	@echo running prove ...
+	$(prove_check)
diff --git a/src/test/modules/ssl_passphrase_callback/server.crt b/src/test/modules/ssl_passphrase_callback/server.crt
new file mode 100644
index 0000000000..292472a041
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/server.crt
@@ -0,0 +1,77 @@
+Certificate:
+    Data:
+        Version: 3 (0x2)
+        Serial Number:
+            64:b5:b2:57:92:2b:e5:86:c6:c8:b1:6b:f5:79:4d:25:b2:05:91:45
+        Signature Algorithm: sha256WithRSAEncryption
+        Issuer: CN = localhost
+        Validity
+            Not Before: Feb 18 22:02:33 2020 GMT
+            Not After : Feb 15 22:02:33 2030 GMT
+        Subject: CN = localhost
+        Subject Public Key Info:
+            Public Key Algorithm: rsaEncryption
+                RSA Public-Key: (2048 bit)
+                Modulus:
+                    00:fb:01:2c:02:cd:15:49:85:ea:e0:95:57:13:7a:
+                    9c:88:ed:93:f8:11:bc:d4:b2:0a:57:8e:ab:b4:5e:
+                    da:a9:24:b8:43:63:35:2c:df:a1:35:8d:49:11:1a:
+                    f0:9c:0a:15:63:0f:4e:53:54:35:9f:8e:2d:0f:57:
+                    13:fd:b8:61:7f:e7:ff:44:aa:23:b9:3e:e6:59:71:
+                    2a:93:84:62:f9:1d:92:37:20:a0:38:c3:a0:bf:09:
+                    28:19:7a:87:90:89:31:b0:84:4f:8f:9e:ae:f5:73:
+                    11:41:ff:ea:12:1b:ab:bb:4c:4e:ba:b1:50:ff:37:
+                    b1:35:3f:0d:aa:bb:a4:93:f1:0c:1d:bd:76:73:32:
+                    2d:22:1b:77:91:e7:c6:d4:b4:5e:d3:bf:02:6f:87:
+                    16:c4:08:6b:3e:a5:17:8a:bf:82:95:42:fb:2e:69:
+                    8f:26:0a:69:7d:c1:f6:8d:a7:ca:69:1d:81:95:ac:
+                    26:d6:b9:75:25:72:ec:13:c9:cf:82:fb:f3:39:90:
+                    22:7e:64:6b:c3:2f:ad:00:0e:e5:27:87:cf:60:aa:
+                    ae:64:0a:e8:04:9b:8f:25:43:cc:9e:a8:10:62:5f:
+                    4c:3a:8d:95:13:bf:18:fa:6c:be:79:be:36:84:1d:
+                    00:ba:35:c6:06:d7:5b:1b:19:b6:ec:88:60:6c:58:
+                    1e:2f
+                Exponent: 65537 (0x10001)
+        X509v3 extensions:
+            X509v3 Subject Key Identifier: 
+                1D:8D:CB:C5:DE:70:22:7C:1F:47:91:6C:61:03:C3:AB:78:CA:66:26
+            X509v3 Authority Key Identifier: 
+                keyid:1D:8D:CB:C5:DE:70:22:7C:1F:47:91:6C:61:03:C3:AB:78:CA:66:26
+
+            X509v3 Basic Constraints: critical
+                CA:TRUE
+    Signature Algorithm: sha256WithRSAEncryption
+         ae:3a:22:a5:bb:95:0f:7f:de:81:58:60:6f:ce:39:f3:39:09:
+         b3:0e:ff:f9:f1:fa:ce:9c:8c:d1:8b:39:b8:ce:ac:d8:b1:63:
+         08:fd:bd:c9:a8:b7:59:68:14:f2:17:23:0a:c8:04:34:85:5f:
+         05:ba:70:7c:b6:56:aa:aa:7b:86:23:93:27:4c:ac:8f:4e:3a:
+         5f:c6:54:74:d7:d4:00:a7:e7:b8:76:6c:6f:02:62:f0:55:4f:
+         49:35:d6:f1:63:ba:e4:3b:90:cd:cc:a1:2c:ca:f6:11:22:e7:
+         23:74:98:ee:8e:35:c0:e7:58:2b:2b:c4:99:82:81:39:17:5b:
+         a5:b5:d8:df:ca:77:9c:f0:34:1e:d5:5b:ea:76:45:b0:0a:53:
+         6d:94:8f:7a:01:b5:83:32:0b:97:75:3a:2f:dd:cb:0c:40:6f:
+         8c:57:17:db:21:04:6b:72:bc:e1:eb:72:8c:d4:25:53:63:bb:
+         f5:22:0b:40:dc:d0:e3:7b:fc:52:da:c4:cd:44:c1:3e:60:ae:
+         88:c1:01:5b:52:71:33:c2:e7:43:c6:bd:92:50:46:50:ea:2e:
+         d7:ae:2f:d4:35:1e:69:e3:75:e1:bc:e7:0a:a2:e5:cb:5b:da:
+         5e:c5:92:55:df:a2:71:44:bc:93:3a:6d:e5:cd:48:40:e2:fb:
+         72:09:c9:e3
+-----BEGIN CERTIFICATE-----
+MIIDCTCCAfGgAwIBAgIUZLWyV5Ir5YbGyLFr9XlNJbIFkUUwDQYJKoZIhvcNAQEL
+BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MB4XDTIwMDIxODIyMDIzM1oXDTMwMDIx
+NTIyMDIzM1owFDESMBAGA1UEAwwJbG9jYWxob3N0MIIBIjANBgkqhkiG9w0BAQEF
+AAOCAQ8AMIIBCgKCAQEA+wEsAs0VSYXq4JVXE3qciO2T+BG81LIKV46rtF7aqSS4
+Q2M1LN+hNY1JERrwnAoVYw9OU1Q1n44tD1cT/bhhf+f/RKojuT7mWXEqk4Ri+R2S
+NyCgOMOgvwkoGXqHkIkxsIRPj56u9XMRQf/qEhuru0xOurFQ/zexNT8Nqrukk/EM
+Hb12czItIht3kefG1LRe078Cb4cWxAhrPqUXir+ClUL7LmmPJgppfcH2jafKaR2B
+lawm1rl1JXLsE8nPgvvzOZAifmRrwy+tAA7lJ4fPYKquZAroBJuPJUPMnqgQYl9M
+Oo2VE78Y+my+eb42hB0AujXGBtdbGxm27IhgbFgeLwIDAQABo1MwUTAdBgNVHQ4E
+FgQUHY3Lxd5wInwfR5FsYQPDq3jKZiYwHwYDVR0jBBgwFoAUHY3Lxd5wInwfR5Fs
+YQPDq3jKZiYwDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEArjoi
+pbuVD3/egVhgb8458zkJsw7/+fH6zpyM0Ys5uM6s2LFjCP29yai3WWgU8hcjCsgE
+NIVfBbpwfLZWqqp7hiOTJ0ysj046X8ZUdNfUAKfnuHZsbwJi8FVPSTXW8WO65DuQ
+zcyhLMr2ESLnI3SY7o41wOdYKyvEmYKBORdbpbXY38p3nPA0HtVb6nZFsApTbZSP
+egG1gzILl3U6L93LDEBvjFcX2yEEa3K84etyjNQlU2O79SILQNzQ43v8UtrEzUTB
+PmCuiMEBW1JxM8LnQ8a9klBGUOou164v1DUeaeN14bznCqLly1vaXsWSVd+icUS8
+kzpt5c1IQOL7cgnJ4w==
+-----END CERTIFICATE-----
diff --git a/src/test/modules/ssl_passphrase_callback/server.key b/src/test/modules/ssl_passphrase_callback/server.key
new file mode 100644
index 0000000000..a8c69d237c
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/server.key
@@ -0,0 +1,30 @@
+-----BEGIN RSA PRIVATE KEY-----
+Proc-Type: 4,ENCRYPTED
+DEK-Info: AES-256-CBC,6C66380BB92F2E7A0B6CE1D5AF290071
+
+7iu0jn3UasRUxYvSzozTybKZg6RBZtclJhSF/kYSrAu36rbPG4mO5lPBZq1wc+0q
+PutAjbZikucjIrU+y8/S5B0Krqmp9HD/oQ9Um5Wrcw3OXSLhLqOBp7l+9qe0Y3BB
+Efo85Ok0maaLfRWGrQ2Y2DNeNmhmVWFDxEGYTIJZQL7jQY2rfPBLA/Yin48upIUJ
+6EtsVuQELwKaerf/PN5Xjig/vx01kR2Lset2uVeiy43ZcPeN6TSwEEj6bC1PFqCR
+5MQTT+u3/HMZE6UKZ1qOuVt0Cm+QqjNWD2y9ueWvyZ2lW3U+tW1SKSbTOdujSysS
+HWF0vwMt0Aeac3SUDAggVfPzrwPYFacKSUE3RQ28+3O8g5F7G6+R9KQ1VMFPuemk
++DSLZDyLkVYnO/iauzfivONOHHRNWaYdZxS+LxLw3PfoPLjSQG7M87asZXX/lSo8
+ka/4qsnBrMb3h0KDAuD6RmbEJNk8RK8a8X7+98weBVOh+IZkp/BH4q83IjiSQhmF
+DZxuU1dH01B/3NXn7Vbk9J3V1v/GoZf5kvCnmmzC7M5ehYSXHY8NqMpM7KxI+vlw
+YZS7HRD5+PHJv4YqlD9RiKAMQfri6GfV1AcVqO8maagmgSLVmPP9tKcKHPRrFpsY
+9cd3hWDBKJ47ZUNPv1C9ibZehMJoHRDwim6KnhT1vY4ikNrlwwIOt7n5OAael3GD
+sP7Oodrgnye8mM0JtET/DVEuBZsT5uS0tfpIQvJKP7gNRLv8sMxF9t2RRgaakryD
+DPD/YbZxBY2qJnvdrLT1Co5MguOvBh+067SRtVvHFOLb4SPiO1wRLy5UvZmjCRJX
+cfCivr4beBr+mN2+1xDVpq2MaEIlT4RQkHNk1odPhof36g16Abe6QVUr/5LSsHvX
++MbgdI0FkveYrF1RdH1/B5BBagDvgA6w3m/PhzdMXJAuElR0FY/Ypow7BP4tJVUt
+wDuB8pQS9gOMxa70RHnGI9GXxpMdiwMtvZOYlw7WmezryEYSNZrBOYl8t9WTB2Dn
+q7TIdRpqE8GMkKE9gurxl0WhN2YEODNICG9luoD0TzVGwVTFOeilANBcfHFqubVs
+Z9fdKHK9Wdr96RQjRzXrCsoMTVoX3A98PGUoDPgd7NPfbH1ybc1775SryzV/C3ii
+yi3hDBBEgxW1ydmSK1PPQdFXGiSM4Tqdg0sqE5vcYhTBe4CjUZaSiW3hrwqnf8cr
+RmSlwSemzZHW3JpcH/F1H+oj2Kz+S/raD9tMVFZuiO4YcZ/1ZEkC3K7jO+G0cc8X
+EJlt1j7Vu2ULECqhHGgd2RVDX06QCwM4ZmZVEmGvtRg9r/OKZhhR4Cjgw0UcFw9Z
+POlSI7GN/Ae0WlmpdC1yj/BUvf99hcUuiPy/eVvEJQIcZPkh7AzmbGMvf6OJSQd+
+TXaDtt/A3khqkQFUBuRkh5OtJaCoh2e7Htsmb0Xf5rWpnBdDY1zs8kwjUaMP0e7r
+IWbsby39Rrwh9nis9f8JwyHg0OZAAkyJPDbkIKOsHfbEWY5zCW54/2vX0S/7rn1u
+kYmrzHRls/lfGI4l6I5zMKb7QHfsKcCRO9NGjNEQPt/q3/Cf9ZjAe2lK2/ROKf/y
+-----END RSA PRIVATE KEY-----
diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
new file mode 100644
index 0000000000..0964ba3508
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
@@ -0,0 +1,82 @@
+/*-------------------------------------------------------------------------
+ *
+ * ssl_passphrase_func.c
+ *
+ * Loadable PostgreSQL module fetch an ssl passphrase for the server cert.
+ * instead of calling an external program. This implementation just hands
+ * back the configured password rot13'd.
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include <float.h>
+#include <stdio.h>
+
+#include "libpq/libpq-be.h"
+#include "utils/guc.h"
+
+PG_MODULE_MAGIC;
+
+void		_PG_init(void);
+void		_PG_fini(void);
+
+static char *ssl_passphrase = NULL;
+
+/* callback function */
+static int rot13_passphrase(char *buf, int size, int rwflag, void *userdata);
+/* hook function to set the callback */
+static void set_rot13(SSL_CTX *context, bool isServerStart);
+/*
+ * Module load callback
+ */
+void
+_PG_init(void)
+{
+	/* Define custom GUC variable. */
+	DefineCustomStringVariable("ssl_passphrase.passphrase",
+							   "passphrase before transformation",
+							   NULL,
+							   &ssl_passphrase,
+							   NULL,
+							   PGC_SIGHUP,
+							   0,	/* no flags required */
+							   NULL,
+							   NULL,
+							   NULL);
+	if (ssl_passphrase)
+		openssl_tls_init_hook = set_rot13;
+}
+
+void
+_PG_fini(void)
+{
+	/* do  nothing yet */
+}
+
+static void
+set_rot13(SSL_CTX *context, bool isServerStart)
+{
+	SSL_CTX_set_default_passwd_cb(context, rot13_passphrase);
+}
+
+static int
+rot13_passphrase(char *buf, int size, int rwflag, void *userdata)
+{
+
+	Assert(ssl_passphrase != NULL);
+	StrNCpy(buf, ssl_passphrase, size);
+	for (char *p = buf; *p; p++)
+	{
+		char		c = *p;
+
+		if ((c >= 'a' && c <= 'm') || (c >= 'A' && c <= 'M'))
+			*p = c + 13;
+		else if ((c >= 'n' && c <= 'z') || (c >= 'N' && c <= 'Z'))
+			*p = c - 13;
+	}
+
+	return strlen(buf);
+
+}
diff --git a/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
new file mode 100644
index 0000000000..e146e09dd7
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
@@ -0,0 +1,67 @@
+
+use strict;
+use warnings;
+
+use File::Copy;
+
+use TestLib;
+use Test::More;
+use PostgresNode;
+
+unless ( ($ENV{with_openssl} || 'no') eq 'yes')
+{
+	plan skip_all => 'SSL not supported by this build';
+}
+
+my $clearpass = "FooBaR1";
+my $rot13pass = "SbbOnE1";
+
+# self-signed cert was generated like this:
+# system('openssl req -new -x509 -days 3650 -nodes -text -out server.crt -keyout server.ckey -subj "/CN=localhost"');
+# add the cleartext passphrase to the key, remove the unprotected key
+# system("openssl rsa -aes256 -in server.ckey -out server.key -passout pass:$clearpass");
+# unlink "server.ckey";
+
+
+my $node = get_new_node('main');
+$node->init;
+$node->append_conf('postgresql.conf',
+				   "ssl_passphrase.passphrase = '$rot13pass'");
+$node->append_conf('postgresql.conf',
+				   "shared_preload_libraries = 'ssl_passphrase_func'");
+$node->append_conf('postgresql.conf',
+				   "listen_addresses = 'localhost'");
+$node->append_conf('postgresql.conf',
+				   "ssl = 'on'");
+
+my $ddir = $node->data_dir;
+
+# install certificate and protected key
+copy("server.crt", $ddir);
+copy("server.key", $ddir);
+chmod 0600, "$ddir/server.key";
+
+$node->start;
+
+# if the server is running we must had successfully transformed the passphrase
+ok(-e "$ddir/postmaster.pid","postgres started");
+
+$node->stop('fast');
+
+# set the wrong passphrase
+$node->append_conf('postgresql.conf',
+				   "ssl_passphrase.passphrase = 'blurfl'");
+
+# try to start the server again
+my $ret = TestLib::system_log('pg_ctl', '-D', $node->data_dir, '-l',
+							  $node->logfile, 'start');
+
+
+# with a bad passphrase the server should not start
+ok($ret, "pg_ctl fails with bad passphrase");
+ok(! -e "$ddir/postmaster.pid","postgres not started with bad passphrase");
+
+# just in case
+$node->stop('fast');
+
+done_testing();
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index a43e31c60e..e1ad4f4052 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -429,7 +429,7 @@ sub mkvcbuild
 
 	if (!$solution->{options}->{openssl})
 	{
-		push @contrib_excludes, 'sslinfo';
+		push @contrib_excludes, 'sslinfo', 'ssl_passphrase_callback';
 	}
 
 	if (!$solution->{options}->{uuid})
#41Andreas Karlsson
andreas@proxel.se
In reply to: Andrew Dunstan (#40)
Re: ssl passphrase callback

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

#42asaba.takanori@fujitsu.com
asaba.takanori@fujitsu.com
In reply to: Andreas Karlsson (#41)
RE: ssl passphrase callback

Hello Andrew,

From: Andreas Karlsson <andreas@proxel.se>

# 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

Trailing space:

220 + X509v3 Subject Key Identifier:
222 + X509v3 Authority Key Identifier:

Missing "d"(password?):

121 +/* init hook for SSL, the default sets the passwor callback if appropriate */

Regards,

--
Takanori Asaba

#43Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Andreas Karlsson (#41)
Re: ssl passphrase callback

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

#44Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: asaba.takanori@fujitsu.com (#42)
Re: ssl passphrase callback

On 3/19/20 4:10 AM, asaba.takanori@fujitsu.com wrote:

Trailing space:

220 + X509v3 Subject Key Identifier:
222 + X509v3 Authority Key Identifier:

We're going to remove all the text, so this becomes moot.

Missing "d"(password?):

121 +/* init hook for SSL, the default sets the passwor callback if appropriate */

Will fix, thanks.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#45Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Andrew Dunstan (#44)
1 attachment(s)
Re: ssl passphrase callback

On 3/21/20 9:18 AM, Andrew Dunstan wrote:

On 3/19/20 4:10 AM, asaba.takanori@fujitsu.com wrote:

Trailing space:

220 + X509v3 Subject Key Identifier:
222 + X509v3 Authority Key Identifier:

We're going to remove all the text, so this becomes moot.

Missing "d"(password?):

121 +/* init hook for SSL, the default sets the passwor callback if appropriate */

Will fix, thanks.

Latest patch attached, I think all comments have been addressed. I
propose to push this later this coming week if there are no more comments.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

ssl-passphrase-callback-6.patchtext/x-patch; charset=UTF-8; name=ssl-passphrase-callback-6.patchDownload
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 18d3fff068..b0a8816cff 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -45,6 +45,9 @@
 #include "tcop/tcopprot.h"
 #include "utils/memutils.h"
 
+/* default init hook can be overridden by a shared library */
+static void  default_openssl_tls_init(SSL_CTX *context, bool isServerStart);
+openssl_tls_init_hook_typ openssl_tls_init_hook = default_openssl_tls_init;
 
 static int	my_sock_read(BIO *h, char *buf, int size);
 static int	my_sock_write(BIO *h, const char *buf, int size);
@@ -116,27 +119,10 @@ be_tls_init(bool isServerStart)
 	SSL_CTX_set_mode(context, SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER);
 
 	/*
-	 * Set password callback
+	 * Call init hook (usually to set password callback)
 	 */
-	if (isServerStart)
-	{
-		if (ssl_passphrase_command[0])
-			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
-	}
-	else
-	{
-		if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
-			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
-		else
+	(* openssl_tls_init_hook)(context, isServerStart);
 
-			/*
-			 * If reloading and no external command is configured, override
-			 * OpenSSL's default handling of passphrase-protected files,
-			 * because we don't want to prompt for a passphrase in an
-			 * already-running server.
-			 */
-			SSL_CTX_set_default_passwd_cb(context, dummy_ssl_passwd_cb);
-	}
 	/* used by the callback */
 	ssl_is_server_start = isServerStart;
 
@@ -1313,3 +1299,27 @@ ssl_protocol_version_to_openssl(int v, const char *guc_name, int loglevel)
 					GetConfigOption(guc_name, false, false))));
 	return -1;
 }
+
+
+static void
+default_openssl_tls_init(SSL_CTX *context, bool isServerStart)
+{
+	if (isServerStart)
+	{
+		if (ssl_passphrase_command[0])
+			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
+	}
+	else
+	{
+		if (ssl_passphrase_command[0] && ssl_passphrase_command_supports_reload)
+			SSL_CTX_set_default_passwd_cb(context, ssl_external_passwd_cb);
+		else
+			/*
+			 * If reloading and no external command is configured, override
+			 * OpenSSL's default handling of passphrase-protected files,
+			 * because we don't want to prompt for a passphrase in an
+			 * already-running server.
+			 */
+			SSL_CTX_set_default_passwd_cb(context, dummy_ssl_passwd_cb);
+	}
+}
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index 2b9ab32293..73d278f3b2 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -972,17 +972,6 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	LocalProcessControlFile(false);
 
-	/*
-	 * Initialize SSL library, if specified.
-	 */
-#ifdef USE_SSL
-	if (EnableSSL)
-	{
-		(void) secure_initialize(true);
-		LoadedSSL = true;
-	}
-#endif
-
 	/*
 	 * Register the apply launcher.  Since it registers a background worker,
 	 * it needs to be called before InitializeMaxBackends(), and it's probably
@@ -996,6 +985,17 @@ PostmasterMain(int argc, char *argv[])
 	 */
 	process_shared_preload_libraries();
 
+	/*
+	 * Initialize SSL library, if specified.
+	 */
+#ifdef USE_SSL
+	if (EnableSSL)
+	{
+		(void) secure_initialize(true);
+		LoadedSSL = true;
+	}
+#endif
+
 	/*
 	 * Now that loadable modules have had their chance to register background
 	 * workers, calculate MaxBackends.
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 82e57afc64..ee57fdc301 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -287,6 +287,10 @@ extern void be_tls_get_peer_serial(Port *port, char *ptr, size_t len);
 extern char *be_tls_get_certificate_hash(Port *port, size_t *len);
 #endif
 
+/* init hook for SSL, the default sets the password callback if appropriate */
+typedef void(* openssl_tls_init_hook_typ)(SSL_CTX *context, bool isServerStart);
+extern openssl_tls_init_hook_typ openssl_tls_init_hook;
+
 #endif							/* USE_SSL */
 
 #ifdef ENABLE_GSS
diff --git a/src/test/modules/Makefile b/src/test/modules/Makefile
index b2eaef3bff..5f975ebcba 100644
--- a/src/test/modules/Makefile
+++ b/src/test/modules/Makefile
@@ -25,4 +25,9 @@ SUBDIRS = \
 		  unsafe_tests \
 		  worker_spi
 
+ifeq ($(with_openssl),yes)
+SUBDIRS += ssl_passphrase_callback
+endif
+
+
 $(recurse)
diff --git a/src/test/modules/ssl_passphrase_callback/.gitignore b/src/test/modules/ssl_passphrase_callback/.gitignore
new file mode 100644
index 0000000000..1dbadf7baf
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/.gitignore
@@ -0,0 +1 @@
+tmp_check
diff --git a/src/test/modules/ssl_passphrase_callback/Makefile b/src/test/modules/ssl_passphrase_callback/Makefile
new file mode 100644
index 0000000000..e2d19f131a
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/Makefile
@@ -0,0 +1,24 @@
+# ssl_passphrase Makefile
+
+export with_openssl
+
+MODULE_big = ssl_passphrase_func
+OBJS = ssl_passphrase_func.o $(WIN32RES)
+PGFILEDESC = "callback function to provide a passphrase"
+
+ifdef USE_PGXS
+PG_CONFIG = pg_config
+PGXS := $(shell $(PG_CONFIG) --pgxs)
+include $(PGXS)
+else
+subdir = src/test/modules/ssl_passphrase_callback
+top_builddir = ../../../..
+include $(top_builddir)/src/Makefile.global
+include $(top_srcdir)/contrib/contrib-global.mk
+endif
+
+check: prove-check
+
+prove-check: ssl_passphrase_func$(DLSUFFIX) | temp-install
+	@echo running prove ...
+	$(prove_check)
diff --git a/src/test/modules/ssl_passphrase_callback/server.crt b/src/test/modules/ssl_passphrase_callback/server.crt
new file mode 100644
index 0000000000..b3c4be4fa5
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/server.crt
@@ -0,0 +1,19 @@
+-----BEGIN CERTIFICATE-----
+MIIDCTCCAfGgAwIBAgIUfHgPLNys4V0d0cWrzRHqfs91LFMwDQYJKoZIhvcNAQEL
+BQAwFDESMBAGA1UEAwwJbG9jYWxob3N0MB4XDTIwMDMyMTE0MDM1OVoXDTQ3MDgw
+NzE0MDM1OVowFDESMBAGA1UEAwwJbG9jYWxob3N0MIIBIjANBgkqhkiG9w0BAQEF
+AAOCAQ8AMIIBCgKCAQEA2j0PZwmeahBC7QpG7i9/VUVJrLzy+b8oVaqZUO6nlPbY
+wuPISYTO/jqc0XDfs/Gb0kccDJ6bPfNfvSnRTG1omE6OO9YjR0u3296l4bWAmYVq
+q4SesgQmm1Wy8ODNpeGaoBUwR51OB/gFHFjUlqAjRwOmrTCbDiAsLt7e+cx+W26r
+2SrJIweiSJsqaQsMMaqlY2qpHnYgWfqRUTqwXqlno0dXuqBt+KKgqeHMY3w3XS51
+8roOI0+Q9KWsexL/aYnLwMRsHRMZcthhzTK6HD/OrLh9CxURImr4ed9TtsNiZltA
+KqLTeGbtS1D2AvFqJU8n5DvtU+26wDrHu6pEM3kSJQIDAQABo1MwUTAdBgNVHQ4E
+FgQUkkfa08hDnxYs1UjG2ydCBJs1b2AwHwYDVR0jBBgwFoAUkkfa08hDnxYs1UjG
+2ydCBJs1b2AwDwYDVR0TAQH/BAUwAwEB/zANBgkqhkiG9w0BAQsFAAOCAQEAjsJh
+p4tCopCA/Pvxupv3VEwGJ+nbH7Zg/hp+o2IWuHBOK1qrkyXBv34h/69bRnWZ5UFV
+HxQwL7CjNtjZu9SbpKkaHbZXPWANC9fbPKdBz9fAEwunf33KbZe3dPv/7xbJirMz
+e+j5V0LE0Spkr/p89LipXfjGw0jLC8VRTx/vKPnmbiBsCKw5SQKh3w7CcBx84Y6q
+Nc27WQ8ReR4W4X1zHGN6kEV4H+yPN2Z9OlSixTiSNvr2mtJQsZa7gK7Wwfm79RN7
+5Kf3l8b6e2BToJwLorpK9mvu41NtwRzl4UoJ1BFJDyhMplFMd8RcwTW6yT2biOFC
+lYCajcBoms3IiyqBog==
+-----END CERTIFICATE-----
diff --git a/src/test/modules/ssl_passphrase_callback/server.key b/src/test/modules/ssl_passphrase_callback/server.key
new file mode 100644
index 0000000000..1475007c73
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/server.key
@@ -0,0 +1,30 @@
+-----BEGIN RSA PRIVATE KEY-----
+Proc-Type: 4,ENCRYPTED
+DEK-Info: AES-256-CBC,DB0E7068D4DCE79FFE63C95B8D8F7CEA
+
+Y4uvnlWX/kyulqsmt8aWI55vKFdfJL4wEZItL8ZKlQFuZuxC9w0OworyjTdqO38R
+v9hwnetZBDgK8kEv6U7wR58mTfwHHCGuxYgSiPZtiW7btS4zu16ePdh8oBEzCxjW
+ALrCFt7uvRu5h2AWy/4BgV4gLNVPNB+lJFABtUoiSnUDr7+bcx7UjZ4An6HriGxC
+Kg/N1pKjT/xiKOy+yHtrp1Jih5HYDE4i99jPtMuTROf8Uyz7ibyrdd/E7QNvANQN
+Cmw4I4Xk4hZ68F0iGU0C0wLND3pWbeYPKorpo3PkI4Du+Aqlg15ae5u8CtU3fXGJ
+mq4/qLGAi1sr/45f5P5a3Q8BQpKkCmGopXMaFYOOiaf3YYgD1eVOxLhsCWqUB+O8
+ygcTNRCoKhzY+ULComXp880J3fFk5b92g4Hm1PAO42uDKzhWSnrmCBJ7ynXvnEc+
+JqhiE8Obrp6FBIHvfN26JtHcXTd/bgUMXSh7AXjsotfvPPV0URve9JJG+RnwckeT
+K3AYDOQK/lbqDGliNqHg1WiMSA2oHSqDhUMB0Sm0jh6+jxCQlsmSDvPvJfWRo5wY
+zbZZZARQnFUaHa9CZVdFxbaPGhYU6vAwxDqi42osSJEdf68Gy2KVXcelqpU/2dKk
+aHfTgAWOsajbgt9p+0369TeZb39+zLODdDJnvZYiu1pTASHP5VrJ2xHhu5zOdjXm
+GafYiPwYBM280wkIVQ0HsTX7BViU2R/7W3FqflXgQvBiraVQVwHyaX4bOU1a3rzg
+emHNLTCpRamT0i/D0tkEPgS42bWSVi9ko5Mn9yb+qToBjAOLVUOAOs9Bv3qxawhI
+XFbBDZ7DS59l2yV6eQkrG7DUCLDf4dv4WZeBnhrPe/Jg8HKcsKcJYV3cejZh8sgu
+XHeCU50+jpJDfTZVPW3TjZWmrTqStGwF1UFpj+tTsTcX+OHAY/shFs3bBZulAsMy
+5UWZWzyWHMWr/wbxW7dbhTb1gNmOgpQQz9dunSgcZ8umzSGLa0ZGmnQj9P/kZkQA
+RenuswH5O7CK/MDmf3J6svwyLt/jULmH26MZTcNu7igT6dj3VMSwkoQQaaQdtmzb
+glzN3uqf8qM+CEjV8dxlt8fv6KJV7gvoYfPAz+1pp5DVJBmRo/+b4e/d4QTV9iWS
+ScBYdonc9WXcrjmExX9+Wf/K/IKfLnKLIi2MZ3pwr1n7yY+dMeF6iREYSjFVIpZd
+MH3G9/SxTrqR7X/eHjwdv1UupYYyaDag8wpVn1RMCb0xYqh2/QP1k0pQycckL0WQ
+lieXibEuQhV/heXcqt83G6pGqLImc6YPYU46jdGpPIMyOK+ZSqJTHUWHfRMQTIMz
+varR2M3uhHvwUFzmvjLh/o6I3r0a0Rl1MztpYfjBV6MS4BKYfraWZ0kxCyV+e6tz
+O7vD0P5W2qm6b89Md3nqjUcbOM8AojcfBl3xpQrpSdgJ25YJBoJ9L2I2pIMNCK/x
+yDNEJl7yP87fdHfXZm2VoUXclDUYHyNys9Rtv9NSr+VNkIMcqrCHEgpAxwQQ5NsO
+/vOZe3wjhXXLyRO7Nh5W8jojw3xcb9c9avFUWUvM2BaS4vEYcItUoF4QuHohrCwk
+-----END RSA PRIVATE KEY-----
diff --git a/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
new file mode 100644
index 0000000000..c95cb50945
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/ssl_passphrase_func.c
@@ -0,0 +1,88 @@
+/*-------------------------------------------------------------------------
+ *
+ * ssl_passphrase_func.c
+ *
+ * Loadable PostgreSQL module fetch an ssl passphrase for the server cert.
+ * instead of calling an external program. This implementation just hands
+ * back the configured password rot13'd.
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres.h"
+
+#include <float.h>
+#include <stdio.h>
+
+#include "libpq/libpq.h"
+#include "libpq/libpq-be.h"
+#include "utils/guc.h"
+
+PG_MODULE_MAGIC;
+
+void		_PG_init(void);
+void		_PG_fini(void);
+
+static char *ssl_passphrase = NULL;
+
+/* callback function */
+static int rot13_passphrase(char *buf, int size, int rwflag, void *userdata);
+/* hook function to set the callback */
+static void set_rot13(SSL_CTX *context, bool isServerStart);
+/*
+ * Module load callback
+ */
+void
+_PG_init(void)
+{
+	/* Define custom GUC variable. */
+	DefineCustomStringVariable("ssl_passphrase.passphrase",
+							   "passphrase before transformation",
+							   NULL,
+							   &ssl_passphrase,
+							   NULL,
+							   PGC_SIGHUP,
+							   0,	/* no flags required */
+							   NULL,
+							   NULL,
+							   NULL);
+	if (ssl_passphrase)
+		openssl_tls_init_hook = set_rot13;
+}
+
+void
+_PG_fini(void)
+{
+	/* do  nothing yet */
+}
+
+static void
+set_rot13(SSL_CTX *context, bool isServerStart)
+{
+	/* warn if the user has set ssl_passphrase_command */
+	if(ssl_passphrase_command[0])
+		ereport(WARNING,
+				(errmsg("ssl_passphrase_command setting ignored by ssl_passphrase_func module")));
+
+	SSL_CTX_set_default_passwd_cb(context, rot13_passphrase);
+}
+
+static int
+rot13_passphrase(char *buf, int size, int rwflag, void *userdata)
+{
+
+	Assert(ssl_passphrase != NULL);
+	StrNCpy(buf, ssl_passphrase, size);
+	for (char *p = buf; *p; p++)
+	{
+		char		c = *p;
+
+		if ((c >= 'a' && c <= 'm') || (c >= 'A' && c <= 'M'))
+			*p = c + 13;
+		else if ((c >= 'n' && c <= 'z') || (c >= 'N' && c <= 'Z'))
+			*p = c - 13;
+	}
+
+	return strlen(buf);
+
+}
diff --git a/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
new file mode 100644
index 0000000000..c052d72f9c
--- /dev/null
+++ b/src/test/modules/ssl_passphrase_callback/t/001_testfunc.pl
@@ -0,0 +1,80 @@
+use strict;
+use warnings;
+
+use File::Copy;
+
+use TestLib;
+use Test::More;
+use PostgresNode;
+
+unless (($ENV{with_openssl} || 'no') eq 'yes')
+{
+	plan skip_all => 'SSL not supported by this build';
+}
+
+my $clearpass = "FooBaR1";
+my $rot13pass = "SbbOnE1";
+
+# self-signed cert was generated like this:
+# system('openssl req -new -x509 -days 10000 -nodes -out server.crt -keyout server.ckey -subj "/CN=localhost"');
+# add the cleartext passphrase to the key, remove the unprotected key
+# system("openssl rsa -aes256 -in server.ckey -out server.key -passout pass:$clearpass");
+# unlink "server.ckey";
+
+
+my $node = get_new_node('main');
+$node->init;
+$node->append_conf('postgresql.conf',
+	"ssl_passphrase.passphrase = '$rot13pass'");
+$node->append_conf('postgresql.conf',
+	"shared_preload_libraries = 'ssl_passphrase_func'");
+$node->append_conf('postgresql.conf', "listen_addresses = 'localhost'");
+$node->append_conf('postgresql.conf', "ssl = 'on'");
+
+my $ddir = $node->data_dir;
+
+# install certificate and protected key
+copy("server.crt", $ddir);
+copy("server.key", $ddir);
+chmod 0600, "$ddir/server.key";
+
+$node->start;
+
+# if the server is running we must have successfully transformed the passphrase
+ok(-e "$ddir/postmaster.pid", "postgres started");
+
+$node->stop('fast');
+
+# should get a warning if ssl_passphrase_command is set
+my $log = $node->rotate_logfile();
+
+$node->append_conf('postgresql.conf',
+	"ssl_passphrase_command = 'echo spl0tz'");
+
+$node->start;
+
+$node->stop('fast');
+
+my $log_contents = slurp_file($log);
+
+like(
+	$log_contents,
+	qr/WARNING.*ssl_passphrase_command setting ignored by ssl_passphrase_func module/,
+	"ssl_passphrase_command set warning");
+
+# set the wrong passphrase
+$node->append_conf('postgresql.conf', "ssl_passphrase.passphrase = 'blurfl'");
+
+# try to start the server again
+my $ret = TestLib::system_log('pg_ctl', '-D', $node->data_dir, '-l',
+	$node->logfile, 'start');
+
+
+# with a bad passphrase the server should not start
+ok($ret,                       "pg_ctl fails with bad passphrase");
+ok(!-e "$ddir/postmaster.pid", "postgres not started with bad passphrase");
+
+# just in case
+$node->stop('fast');
+
+done_testing();
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index f89a8a4fdb..c5a7025d5e 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -429,7 +429,7 @@ sub mkvcbuild
 
 	if (!$solution->{options}->{openssl})
 	{
-		push @contrib_excludes, 'sslinfo';
+		push @contrib_excludes, 'sslinfo', 'ssl_passphrase_callback';
 	}
 
 	if (!$solution->{options}->{uuid})
#46Andreas Karlsson
andreas@proxel.se
In reply to: Andrew Dunstan (#45)
Re: ssl passphrase callback

On 3/22/20 1:08 AM, Andrew Dunstan wrote:

Latest patch attached, I think all comments have been addressed. I
propose to push this later this coming week if there are no more comments.

I do not have any objections.

Andreas

#47Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andreas Karlsson (#46)
Re: ssl passphrase callback

Andreas Karlsson <andreas@proxel.se> writes:

On 3/22/20 1:08 AM, Andrew Dunstan wrote:

Latest patch attached, I think all comments have been addressed. I
propose to push this later this coming week if there are no more comments.

I do not have any objections.

This CF entry is still open, should it not be closed as committed?

https://commitfest.postgresql.org/27/2338/

regards, tom lane

#48Andrew Dunstan
andrew.dunstan@2ndquadrant.com
In reply to: Tom Lane (#47)
Re: ssl passphrase callback

On 3/28/20 1:45 PM, Tom Lane wrote:

Andreas Karlsson <andreas@proxel.se> writes:

On 3/22/20 1:08 AM, Andrew Dunstan wrote:

Latest patch attached, I think all comments have been addressed. I
propose to push this later this coming week if there are no more comments.

I do not have any objections.

This CF entry is still open, should it not be closed as committed?

https://commitfest.postgresql.org/27/2338/

Done, thanks for the reminder.

cheers

andrew

--
Andrew Dunstan https://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services