From 3ff199bc5a9c22e05ff0bb8b9227933217a26553 Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Wed, 28 Jul 2021 09:49:56 -0700
Subject: [PATCH v5 08/12] Handle non-superuser subscription owners sensibly

Stop pretending that subscriptions are always owned by superusers
and instead fix security problems that arise as a consequence of
them not being superuser.  Specifically, disallow a non-superuser
changing the connection, the list of publications, or the options
for their subscription.

The prior behavior violated the principle of least surprise,
allowing a non-superuser in possession of a subscription to change
all aspects of the subscription, connecting it to a different server
and subscribing a different set of publications, effectively
amounting to a non-superuser creating a new subscription.

The new behavior restricts the non-superuser owner to enabling,
disabling, refreshing, renaming, and further assigning ownership of
the subscription.
---
 doc/src/sgml/ref/alter_subscription.sgml   | 11 ++++-
 src/backend/commands/subscriptioncmds.c    | 20 +++++++++
 src/test/regress/expected/subscription.out | 51 ++++++++++++++++++++++
 src/test/regress/sql/subscription.sql      | 35 +++++++++++++++
 4 files changed, 115 insertions(+), 2 deletions(-)

diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index a6f994450d..1cf2bee0a8 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -47,8 +47,6 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
    You must own the subscription to use <command>ALTER SUBSCRIPTION</command>.
    To alter the owner, you must also be a direct or indirect member of the
    new owning role. The new owner has to be a superuser.
-   (Currently, all subscription owners must be superusers, so the owner checks
-   will be bypassed in practice.  But this might change in the future.)
   </para>
   
   <para>
@@ -96,6 +94,9 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
       <xref linkend="sql-createsubscription"/>.  See there for more
       information.
      </para>
+     <para>
+      Only superusers may change the connection property.
+     </para>
     </listitem>
    </varlistentry>
 
@@ -137,6 +138,9 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
       under <literal>REFRESH PUBLICATION</literal> may be specified,
       except in the case of <literal>DROP PUBLICATION</literal>.
      </para>
+     <para>
+      Only superusers may change the list of subscribed publications.
+     </para>
     </listitem>
    </varlistentry>
 
@@ -202,6 +206,9 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
       <literal>binary</literal>, and
       <literal>streaming</literal>.
      </para>
+     <para>
+      Only superusers may alter subscription parameters.
+     </para>
     </listitem>
    </varlistentry>
 
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5157f44058..24364544bd 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -868,6 +868,11 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 	{
 		case ALTER_SUBSCRIPTION_OPTIONS:
 			{
+				if (!superuser())
+					ereport(ERROR,
+							(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+							 errmsg("must be superuser to alter the options for a subscription")));
+
 				supported_opts = (SUBOPT_SLOT_NAME |
 								  SUBOPT_SYNCHRONOUS_COMMIT | SUBOPT_BINARY |
 								  SUBOPT_STREAMING);
@@ -946,6 +951,11 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 			}
 
 		case ALTER_SUBSCRIPTION_CONNECTION:
+			if (!superuser())
+				ereport(ERROR,
+						(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+						 errmsg("must be superuser to alter the connection for a subscription")));
+
 			/* Load the library providing us libpq calls. */
 			load_file("libpqwalreceiver", false);
 			/* Check the connection info string. */
@@ -959,6 +969,11 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 
 		case ALTER_SUBSCRIPTION_SET_PUBLICATION:
 			{
+				if (!superuser())
+					ereport(ERROR,
+							(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+							 errmsg("must be superuser to alter publications for a subscription")));
+
 				supported_opts = SUBOPT_COPY_DATA | SUBOPT_REFRESH;
 				parse_subscription_options(pstate, stmt->options,
 										   supported_opts, &opts);
@@ -1006,6 +1021,11 @@ AlterSubscription(ParseState *pstate, AlterSubscriptionStmt *stmt,
 				List	   *publist;
 				bool		isadd = stmt->kind == ALTER_SUBSCRIPTION_ADD_PUBLICATION;
 
+				if (!superuser())
+					ereport(ERROR,
+							(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+							 errmsg("must be superuser to alter publications for a subscription")));
+
 				supported_opts = SUBOPT_REFRESH;
 				if (isadd)
 					supported_opts |= SUBOPT_COPY_DATA;
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 77b4437b69..f61096bf89 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -144,6 +144,57 @@ HINT:  The owner of a subscription must be a superuser.
 ALTER ROLE regress_subscription_user2 SUPERUSER;
 -- now it works
 ALTER SUBSCRIPTION regress_testsub OWNER TO regress_subscription_user2;
+-- revoke superuser from new owner
+ALTER ROLE regress_subscription_user2 NOSUPERUSER;
+SET SESSION AUTHORIZATION regress_subscription_user2;
+-- fail - non-superuser owner cannot change connection parameter
+ALTER SUBSCRIPTION regress_testsub CONNECTION 'dbname=somethingelse';
+ERROR:  must be superuser to alter the connection for a subscription
+-- fail - non-superuser owner cannot alter the publications list
+ALTER SUBSCRIPTION regress_testsub ADD PUBLICATION somepub;
+ERROR:  must be superuser to alter publications for a subscription
+ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION otherpub;
+ERROR:  must be superuser to alter publications for a subscription
+ALTER SUBSCRIPTION regress_testsub SET PUBLICATION somepub, otherpub;
+ERROR:  must be superuser to alter publications for a subscription
+-- fail - non-superuser owner cannot change subscription parameters
+ALTER SUBSCRIPTION regress_testsub SET (copy_data = true);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (copy_data = false);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (create_slot = true);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (create_slot = false);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (slot_name = 'somethingelse');
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = on);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = off);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = local);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = remote_write);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = remote_apply);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (binary = on);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (binary = off);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (connect = on);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (connect = off);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (streaming = on);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (streaming = off);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (two_phase = on);
+ERROR:  must be superuser to alter the options for a subscription
+ALTER SUBSCRIPTION regress_testsub SET (two_phase = off);
+ERROR:  must be superuser to alter the options for a subscription
+SET SESSION AUTHORIZATION 'regress_subscription_user';
 -- fail - cannot do DROP SUBSCRIPTION inside transaction block with slot name
 BEGIN;
 DROP SUBSCRIPTION regress_testsub;
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index d42104c191..863217f298 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -105,6 +105,41 @@ ALTER ROLE regress_subscription_user2 SUPERUSER;
 -- now it works
 ALTER SUBSCRIPTION regress_testsub OWNER TO regress_subscription_user2;
 
+-- revoke superuser from new owner
+ALTER ROLE regress_subscription_user2 NOSUPERUSER;
+
+SET SESSION AUTHORIZATION regress_subscription_user2;
+
+-- fail - non-superuser owner cannot change connection parameter
+ALTER SUBSCRIPTION regress_testsub CONNECTION 'dbname=somethingelse';
+
+-- fail - non-superuser owner cannot alter the publications list
+ALTER SUBSCRIPTION regress_testsub ADD PUBLICATION somepub;
+ALTER SUBSCRIPTION regress_testsub DROP PUBLICATION otherpub;
+ALTER SUBSCRIPTION regress_testsub SET PUBLICATION somepub, otherpub;
+
+-- fail - non-superuser owner cannot change subscription parameters
+ALTER SUBSCRIPTION regress_testsub SET (copy_data = true);
+ALTER SUBSCRIPTION regress_testsub SET (copy_data = false);
+ALTER SUBSCRIPTION regress_testsub SET (create_slot = true);
+ALTER SUBSCRIPTION regress_testsub SET (create_slot = false);
+ALTER SUBSCRIPTION regress_testsub SET (slot_name = 'somethingelse');
+ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = on);
+ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = off);
+ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = local);
+ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = remote_write);
+ALTER SUBSCRIPTION regress_testsub SET (synchronous_commit = remote_apply);
+ALTER SUBSCRIPTION regress_testsub SET (binary = on);
+ALTER SUBSCRIPTION regress_testsub SET (binary = off);
+ALTER SUBSCRIPTION regress_testsub SET (connect = on);
+ALTER SUBSCRIPTION regress_testsub SET (connect = off);
+ALTER SUBSCRIPTION regress_testsub SET (streaming = on);
+ALTER SUBSCRIPTION regress_testsub SET (streaming = off);
+ALTER SUBSCRIPTION regress_testsub SET (two_phase = on);
+ALTER SUBSCRIPTION regress_testsub SET (two_phase = off);
+
+SET SESSION AUTHORIZATION 'regress_subscription_user';
+
 -- fail - cannot do DROP SUBSCRIPTION inside transaction block with slot name
 BEGIN;
 DROP SUBSCRIPTION regress_testsub;
-- 
2.21.1 (Apple Git-122.3)

