Re-validate connection string in libpqrcv_connect().

A superuser may create a subscription with password_required=true, but
which uses a connection string without a password.

Previously, if the owner of such a subscription was changed to a
non-superuser, the non-superuser was able to utilize a password from
another source (like a password file or the PGPASSWORD environment
variable), which should not have been allowed.

This commit adds a step to re-validate the connection string before
connecting.

Reported-by: Jeff Davis
Author: Vignesh C
Reviewed-by: Peter Smith, Robert Haas, Amit Kapila
Discussion: https://www.postgresql.org/message-id/flat/e5892973ae2a80a1a3e0266806640dae3c428100.camel%40j-davis.com
Backpatch-through: 16

2 files changed, 89 insertions, 0 deletions

diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
index 77812f12a9b..a5b1507559f 100644
--- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
+++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
@@ -138,6 +138,15 @@ libpqrcv_connect(const char *conninfo, bool logical, bool must_use_password,
 	int			i = 0;
 
 	/*
+	 * Re-validate connection string. The validation already happened at DDL
+	 * time, but the subscription owner may have changed. If we don't recheck
+	 * with the correct must_use_password, it's possible that the connection
+	 * will obtain the password from a different source, such as PGPASSFILE or
+	 * PGPASSWORD.
+	 */
+	libpqrcv_check_conninfo(conninfo, must_use_password);
+
+	/*
 	 * We use the expand_dbname parameter to process the connection string (or
 	 * URI), and pass some extra options.
 	 */
diff --git a/src/test/subscription/t/027_nosuperuser.pl b/src/test/subscription/t/027_nosuperuser.pl
index d7a7e3ef5bb..525ef7917f7 100644
--- a/src/test/subscription/t/027_nosuperuser.pl
+++ b/src/test/subscription/t/027_nosuperuser.pl
@@ -303,4 +303,84 @@ GRANT SELECT ON alice.unpartitioned TO regress_alice;
 expect_replication("alice.unpartitioned", 3, 17, 21,
 	"restoring SELECT permission permits replication to continue");
 
+# If the subscription connection requires a password ('password_required'
+# is true) then a non-superuser must specify that password in the connection
+# string.
+$ENV{"PGPASSWORD"} = 'secret';
+
+my $node_publisher1  = PostgreSQL::Test::Cluster->new('publisher1');
+my $node_subscriber1 = PostgreSQL::Test::Cluster->new('subscriber1');
+$node_publisher1->init(allows_streaming => 'logical');
+$node_subscriber1->init;
+$node_publisher1->start;
+$node_subscriber1->start;
+my $publisher_connstr1 =
+  $node_publisher1->connstr . ' user=regress_test_user dbname=postgres';
+my $publisher_connstr2 =
+  $node_publisher1->connstr
+  . ' user=regress_test_user dbname=postgres password=secret';
+
+for my $node ($node_publisher1, $node_subscriber1)
+{
+	$node->safe_psql(
+		'postgres', qq(
+  CREATE ROLE regress_test_user PASSWORD 'secret' LOGIN REPLICATION;
+  GRANT CREATE ON DATABASE postgres TO regress_test_user;
+  GRANT PG_CREATE_SUBSCRIPTION TO regress_test_user;
+  ));
+}
+
+$node_publisher1->safe_psql(
+	'postgres', qq(
+SET SESSION AUTHORIZATION regress_test_user;
+CREATE PUBLICATION regress_test_pub;
+));
+$node_subscriber1->safe_psql(
+	'postgres', qq(
+CREATE SUBSCRIPTION regress_test_sub CONNECTION '$publisher_connstr1' PUBLICATION regress_test_pub;
+));
+
+# Wait for initial sync to finish
+$node_subscriber1->wait_for_subscription_sync($node_publisher1,
+	'regress_test_sub');
+
+# Setup pg_hba configuration so that logical replication connection without
+# password is not allowed.
+unlink($node_publisher1->data_dir . '/pg_hba.conf');
+$node_publisher1->append_conf('pg_hba.conf',
+	qq{local all 				regress_test_user 	md5});
+$node_publisher1->reload;
+
+# Change the subscription owner to a non-superuser
+$node_subscriber1->safe_psql(
+	'postgres', qq(
+ALTER SUBSCRIPTION regress_test_sub OWNER TO regress_test_user;
+));
+
+# Non-superuser must specify password in the connection string
+my ($ret, $stdout, $stderr) = $node_subscriber1->psql(
+	'postgres', qq(
+SET SESSION AUTHORIZATION regress_test_user;
+ALTER SUBSCRIPTION regress_test_sub REFRESH PUBLICATION;
+));
+isnt($ret, 0,
+	"non zero exit for subscription whose owner is a non-superuser must specify password parameter of the connection string"
+);
+ok( $stderr =~ m/DETAIL:  Non-superusers must provide a password in the connection string./,
+	'subscription whose owner is a non-superuser must specify password parameter of the connection string'
+);
+
+delete $ENV{"PGPASSWORD"};
+
+# It should succeed after including the password parameter of the connection
+# string.
+($ret, $stdout, $stderr) = $node_subscriber1->psql(
+	'postgres', qq(
+SET SESSION AUTHORIZATION regress_test_user;
+ALTER SUBSCRIPTION regress_test_sub CONNECTION '$publisher_connstr2';
+ALTER SUBSCRIPTION regress_test_sub REFRESH PUBLICATION;
+));
+is($ret, 0,
+	"Non-superuser will be able to refresh the publication after specifying the password parameter of the connection string"
+);
 done_testing();