Validate subscription conninfo on owner change

For subscriptions using SERVER, changing the owner can change the
effective connection string. However, ALTER SUBSCRIPTION ... OWNER TO
did not validate the generated conninfo for the new owner.

As a result, ownership could be transferred to a non-superuser whose
generated connection string did not satisfy password_required=true.
The ownership change succeeded, but the subscription would fail later
when the worker or another command tried to connect.

Fix this by making ALTER SUBSCRIPTION ... OWNER TO validate the new
owner's generated conninfo with walrcv_check_conninfo().

Backpatch to v19, where SERVER subscriptions were introduced.

Author: Fujii Masao <masao.fujii@gmail.com>
Reviewed-by: Yuanchao Zhang <145zhangyc@gmail.com>
Reviewed-by: Hayato Kuroda <kuroda.hayato@fujitsu.com>
Discussion: https://postgr.es/m/CAHGQGwFGa6+wWVgUmZPFwN=fBY59mYPkMK3=TxT=Pv5C1mNNRQ@mail.gmail.com
Backpatch-through: 19

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index 6fc3e07a2d5028a72fd1067295d92a4b2196bc95..0f81af5608bf29a10e6fcf6e263a712659625904 100644 (file)
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -53,6 +53,13 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
    to alter the owner, you must be able to <literal>SET ROLE</literal> to the
    new owning role. If the subscription has
    <literal>password_required=false</literal>, only superusers can modify it.
+   If the subscription uses a foreign server, the new owner must have
+   <literal>USAGE</literal> privilege on the foreign server, a user mapping
+   for the new owner or for <literal>PUBLIC</literal> must exist, and the
+   connection string generated for the new owner must be valid.  If the new
+   owner is not a superuser and the subscription has
+   <literal>password_required=true</literal>, the generated connection string
+   must include a password.
   </para>
 
   <para>

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 7f946c5b454578adcaa5249a1a20bb20768af08f..d4504b4a0c6fb90bda3c20b4445d1d009ffab4b6 100644 (file)
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -2949,11 +2949,12 @@ AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
 
        /*
         * If the subscription uses a server, check that the new owner has USAGE
-        * privileges on the server and that a user mapping exists. Note: does not
-        * re-check the resulting connection string.
+        * privileges on the server, that a user mapping exists, and that the
+        * resulting connection string is valid for the new owner.
         */
        if (OidIsValid(form->subserver))
        {
+               char       *conninfo;
                ForeignServer *server = GetForeignServer(form->subserver);
 
                aclresult = object_aclcheck(ForeignServerRelationId, server->serverid, newOwnerId, ACL_USAGE);
@@ -2966,6 +2967,15 @@ AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
 
                /* make sure a user mapping exists */
                GetUserMapping(newOwnerId, server->serverid);
+
+               conninfo = ForeignServerConnectionString(newOwnerId, server);
+
+               /* Load the library providing us libpq calls. */
+               load_file("libpqwalreceiver", false);
+               /* Check the connection info string. */
+               walrcv_check_conninfo(conninfo,
+                                                         form->subpasswordrequired &&
+                                                         !superuser_arg(newOwnerId));
        }
 
        form->subowner = newOwnerId;

diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index d201ad764f05adcc7d42a5d52b83ba8785e6c89d..1bb785f4f9f5fb3b1989d54261427906cc90224f 100644 (file)
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -9,6 +9,10 @@ CREATE FUNCTION test_fdw_connection(oid, oid, internal)
     RETURNS text
     AS :'regresslib', 'test_fdw_connection'
     LANGUAGE C;
+CREATE FUNCTION test_fdw_connection_no_password(oid, oid, internal)
+    RETURNS text
+    AS :'regresslib', 'test_fdw_connection_no_password'
+    LANGUAGE C;
 CREATE ROLE regress_subscription_user LOGIN SUPERUSER;
 CREATE ROLE regress_subscription_user2;
 CREATE ROLE regress_subscription_user3 IN ROLE pg_create_subscription;
@@ -189,6 +193,18 @@ CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
 WARNING:  subscription was created, but is not connected
 HINT:  To initiate replication, you must manually create the replication slot, enable the subscription, and alter the subscription to refresh publications.
 RESET SESSION AUTHORIZATION;
+GRANT USAGE ON FOREIGN SERVER test_server TO regress_subscription_user2;
+CREATE USER MAPPING FOR regress_subscription_user2 SERVER test_server OPTIONS(user 'foo');
+ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection_no_password;
+WARNING:  changing the foreign-data wrapper connection function can cause the options for dependent objects to become invalid
+-- fail, new owner's generated conninfo must satisfy password_required
+ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user2;
+ERROR:  password is required
+DETAIL:  Non-superusers must provide a password in the connection string.
+ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
+WARNING:  changing the foreign-data wrapper connection function can cause the options for dependent objects to become invalid
+DROP USER MAPPING FOR regress_subscription_user2 SERVER test_server;
+REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user2;
 REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
 SET SESSION AUTHORIZATION regress_subscription_user3;
 -- ok, lacks USAGE on test_server, but replacing connection anyway
@@ -231,6 +247,7 @@ HINT:  Use DROP ... CASCADE to drop the dependent objects too.
 ALTER FOREIGN DATA WRAPPER test_fdw NO CONNECTION;
 WARNING:  removing the foreign-data wrapper connection function will cause dependent subscriptions to fail
 DROP FUNCTION test_fdw_connection(oid, oid, internal);
+DROP FUNCTION test_fdw_connection_no_password(oid, oid, internal);
 DROP FOREIGN DATA WRAPPER test_fdw;
 -- fail - invalid connection string during ALTER
 ALTER SUBSCRIPTION regress_testsub CONNECTION 'foobar';

diff --git a/src/test/regress/regress.c b/src/test/regress/regress.c
index 9801cdd1d8c3ea1c45d53f22c08115041a9c173e..14d301b3499ecf8d46ccb085d7b73d2ddb2a5e07 100644 (file)
--- a/src/test/regress/regress.c
+++ b/src/test/regress/regress.c
@@ -742,6 +742,15 @@ test_fdw_connection(PG_FUNCTION_ARGS)
        PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist user=doesnotexist password=secret"));
 }
 
+PG_FUNCTION_INFO_V1(test_fdw_connection_no_password);
+Datum
+test_fdw_connection_no_password(PG_FUNCTION_ARGS)
+{
+       /* Ensure the test fails if no valid user mapping exists. */
+       GetUserMapping(PG_GETARG_OID(0), PG_GETARG_OID(1));
+       PG_RETURN_TEXT_P(cstring_to_text("dbname=regress_doesnotexist user=doesnotexist"));
+}
+
 PG_FUNCTION_INFO_V1(is_catalog_text_unique_index_oid);
 Datum
 is_catalog_text_unique_index_oid(PG_FUNCTION_ARGS)

diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 86c402c59aa6d7162802d0069b6ee75b02857c3b..f19740fdfb8381a1ca6bbfbcad0e7d6d2a4d1341 100644 (file)
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -12,6 +12,10 @@ CREATE FUNCTION test_fdw_connection(oid, oid, internal)
     RETURNS text
     AS :'regresslib', 'test_fdw_connection'
     LANGUAGE C;
+CREATE FUNCTION test_fdw_connection_no_password(oid, oid, internal)
+    RETURNS text
+    AS :'regresslib', 'test_fdw_connection_no_password'
+    LANGUAGE C;
 
 CREATE ROLE regress_subscription_user LOGIN SUPERUSER;
 CREATE ROLE regress_subscription_user2;
@@ -136,6 +140,17 @@ CREATE SUBSCRIPTION regress_testsub6 SERVER test_server
   PUBLICATION testpub WITH (slot_name = 'dummy', connect = false);
 
 RESET SESSION AUTHORIZATION;
+GRANT USAGE ON FOREIGN SERVER test_server TO regress_subscription_user2;
+CREATE USER MAPPING FOR regress_subscription_user2 SERVER test_server OPTIONS(user 'foo');
+ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection_no_password;
+
+-- fail, new owner's generated conninfo must satisfy password_required
+ALTER SUBSCRIPTION regress_testsub6 OWNER TO regress_subscription_user2;
+
+ALTER FOREIGN DATA WRAPPER test_fdw CONNECTION test_fdw_connection;
+DROP USER MAPPING FOR regress_subscription_user2 SERVER test_server;
+REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user2;
+
 REVOKE USAGE ON FOREIGN SERVER test_server FROM regress_subscription_user3;
 SET SESSION AUTHORIZATION regress_subscription_user3;
 
@@ -182,6 +197,7 @@ DROP FUNCTION test_fdw_connection(oid, oid, internal);
 ALTER FOREIGN DATA WRAPPER test_fdw NO CONNECTION;
 
 DROP FUNCTION test_fdw_connection(oid, oid, internal);
+DROP FUNCTION test_fdw_connection_no_password(oid, oid, internal);
 
 DROP FOREIGN DATA WRAPPER test_fdw;