public inbox for [email protected]
help / color / mirror / Atom feedFrom: Mark Dilger <[email protected]>
To: Andrew Dunstan <[email protected]>
Cc: PostgreSQL-development <[email protected]>
Cc: Robert Haas <[email protected]>
Subject: Re: Non-superuser subscription owners
Date: Wed, 3 Nov 2021 12:50:28 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
> On Nov 1, 2021, at 10:58 AM, Mark Dilger <[email protected]> wrote:
>
> ALTER SUBSCRIPTION..[ENABLE | DISABLE] do not synchronously start or stop subscription workers. The ALTER command updates the catalog's subenabled field, but workers only lazily respond to that. Disabling and enabling the subscription as part of the OWNER TO would not reliably accomplish anything.
I have rethought my prior analysis. The problem in the previous patch was that the subscription apply workers did not check for a change in ownership the way they checked for other changes, instead only picking up the new ownership information when the worker restarted for some other reason. This next patch set fixes that. The application of a change record may continue under the old ownership permissions when a concurrent command changes the ownership of the subscription, but the worker will pick up the new permissions before applying the next record. I think that is consistent enough with reasonable expectations.
The first two patches are virtually unchanged. The third updates the behavior of the apply workers, and updates the documentation to match.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[application/octet-stream] v2-0001-Handle-non-superuser-subscription-owners-sensibly.patch (11.0K, ../[email protected]/2-v2-0001-Handle-non-superuser-subscription-owners-sensibly.patch)
download | inline diff:
From 6225780bd7e4282ec47e153c19ef59fc10648c17 Mon Sep 17 00:00:00 2001
From: Mark Dilger <[email protected]>
Date: Sun, 26 Sep 2021 18:21:26 -0700
Subject: [PATCH v2 1/3] 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 0b027cc346..b1fd73f776 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>
@@ -99,6 +97,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>
@@ -139,6 +140,9 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
<literal>REFRESH PUBLICATION</literal> may be specified, to control the
implicit refresh operation.
</para>
+ <para>
+ Only superusers may change the list of subscribed publications.
+ </para>
</listitem>
</varlistentry>
@@ -204,6 +208,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 c47ba26369..6a5d192128 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 | SUBOPT_COPY_DATA;
parse_subscription_options(pstate, stmt->options,
supported_opts, &opts);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 15a1ac6398..a05e6f1795 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 7faa935a2a..8f66709441 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)
[application/octet-stream] v2-0002-Allow-subscription-ownership-by-non-superusers.patch (4.9K, ../[email protected]/3-v2-0002-Allow-subscription-ownership-by-non-superusers.patch)
download | inline diff:
From f4cae21ee2c7045fe5c89fbba5ecf7a3de3e2283 Mon Sep 17 00:00:00 2001
From: Mark Dilger <[email protected]>
Date: Sun, 26 Sep 2021 18:24:07 -0700
Subject: [PATCH v2 2/3] Allow subscription ownership by non-superusers
Non-superusers can already end up owning subscriptions if superuser
is revoked from a subscription owner, so quit the pretense and
simply allow the ownership transfer overtly by ALTER SUBSCRIPTION.
The prior situation did more to give a false sense that
subscriptions would never be owned by non-superusers than it did to
actually prevent that scenario from arising.
---
doc/src/sgml/ref/alter_subscription.sgml | 2 +-
src/backend/commands/subscriptioncmds.c | 9 ++-------
src/test/regress/expected/subscription.out | 12 ++++--------
src/test/regress/sql/subscription.sql | 11 ++++-------
4 files changed, 11 insertions(+), 23 deletions(-)
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index b1fd73f776..bc52339eba 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -46,7 +46,7 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
<para>
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.
+ new owning role.
</para>
<para>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 6a5d192128..d75183cd12 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1476,13 +1476,8 @@ AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_SUBSCRIPTION,
NameStr(form->subname));
- /* New owner must be a superuser */
- if (!superuser_arg(newOwnerId))
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied to change owner of subscription \"%s\"",
- NameStr(form->subname)),
- errhint("The owner of a subscription must be a superuser.")));
+ /* Must be able to assign ownership to the target role */
+ check_is_member_of_role(GetUserId(), newOwnerId);
form->subowner = newOwnerId;
CatalogTupleUpdate(rel, &tup->t_self, tup);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index a05e6f1795..9b4dac9c0e 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -137,16 +137,12 @@ HINT: Available values: local, remote_write, remote_apply, on, off.
-- rename back to keep the rest simple
ALTER SUBSCRIPTION regress_testsub_foo RENAME TO regress_testsub;
--- fail - new owner must be superuser
+-- superuser can assign the ownership to a non-superuser
ALTER SUBSCRIPTION regress_testsub OWNER TO regress_subscription_user2;
-ERROR: permission denied to change owner of subscription "regress_testsub"
-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 - not a member of the target role
+ALTER SUBSCRIPTION regress_testsub OWNER TO regress_subscription_user;
+ERROR: must be member of role "regress_subscription_user"
-- 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
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 8f66709441..6c68d547d1 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -99,17 +99,14 @@ ALTER SUBSCRIPTION regress_testsub_foo SET (synchronous_commit = foobar);
-- rename back to keep the rest simple
ALTER SUBSCRIPTION regress_testsub_foo RENAME TO regress_testsub;
--- fail - new owner must be superuser
-ALTER SUBSCRIPTION regress_testsub OWNER TO regress_subscription_user2;
-ALTER ROLE regress_subscription_user2 SUPERUSER;
--- now it works
+-- superuser can assign the ownership to a non-superuser
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 - not a member of the target role
+ALTER SUBSCRIPTION regress_testsub OWNER TO regress_subscription_user;
+
-- fail - non-superuser owner cannot change connection parameter
ALTER SUBSCRIPTION regress_testsub CONNECTION 'dbname=somethingelse';
--
2.21.1 (Apple Git-122.3)
[application/octet-stream] v2-0003-Respect-permissions-within-logical-replication.patch (15.5K, ../[email protected]/4-v2-0003-Respect-permissions-within-logical-replication.patch)
download | inline diff:
From cc8e820799d4423f329346e43b82e3bf0216842c Mon Sep 17 00:00:00 2001
From: Mark Dilger <[email protected]>
Date: Tue, 2 Nov 2021 15:08:54 -0700
Subject: [PATCH v2 3/3] Respect permissions within logical replication
Prevent logical replication workers from performing insert, update,
delete, truncate, or copy commands on tables unless the subscription
owner has permission to do so. This makes it much safer to allow
non-superusers to create subscriptions, since they can only cause
changes to replicate into schemas and tables that they would be able
to directly make themselves. This also reduces the amount of trust
that a subscriber must have in a publisher; if the subscription is
set up by a DBA using an account that is intentionally restricted to
only those tables or schemas that the DBA expects the publication to
touch, then the DBA can be create and refresh the subscription
without fear that a malicious publisher will have added other
critical tables to the publication and thereby overwrite them on the
subscriber side. In many setups this is likely a non-issue, as the
same DBA manages both the publisher and the subscriber, but it seems
wiser not to hard-code that assumption into the security model of
logical replication.
---
doc/src/sgml/logical-replication.sgml | 35 ++++--
doc/src/sgml/ref/alter_subscription.sgml | 7 +-
src/backend/commands/subscriptioncmds.c | 2 +
src/backend/replication/logical/tablesync.c | 13 +++
src/backend/replication/logical/worker.c | 30 +++++
src/test/subscription/t/026_nosuperuser.pl | 118 ++++++++++++++++++++
6 files changed, 194 insertions(+), 11 deletions(-)
create mode 100644 src/test/subscription/t/026_nosuperuser.pl
diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml
index 45b2e1e28f..5968958f03 100644
--- a/doc/src/sgml/logical-replication.sgml
+++ b/doc/src/sgml/logical-replication.sgml
@@ -330,6 +330,15 @@
will simply be skipped.
</para>
+ <para>
+ Logical replication operations are performed with the privileges of the role
+ which owns the subscription. Only superusers may create subscriptions, but
+ if the ownership of the subscription is transferred, or if the owner
+ subsequently has superuser privileges revoked, permissions failures can
+ cause conflicts. (Note that <productname>PostgreSQL</productname> prior to
+ version 15.0 did no permissions checking when applying changes.)
+ </para>
+
<para>
A conflict will produce an error and will stop the replication; it must be
resolved manually by the user. Details about the conflict can be found in
@@ -337,7 +346,7 @@
</para>
<para>
- The resolution can be done either by changing data on the subscriber so
+ The resolution can be done either by changing data or permissions on the subscriber so
that it does not conflict with the incoming change or by skipping the
transaction that conflicts with the existing data. The transaction can be
skipped by calling the <link linkend="pg-replication-origin-advance">
@@ -530,9 +539,9 @@
<para>
A user able to modify the schema of subscriber-side tables can execute
- arbitrary code as a superuser. Limit ownership
- and <literal>TRIGGER</literal> privilege on such tables to roles that
- superusers trust. Moreover, if untrusted users can create tables, use only
+ arbitrary code as the role which owns any subscription which modifies those tables. Limit ownership
+ and <literal>TRIGGER</literal> privilege on such tables to trusted roles.
+ Moreover, if untrusted users can create tables, use only
publications that list tables explicitly. That is to say, create a
subscription <literal>FOR ALL TABLES</literal> or
<literal>FOR ALL TABLES IN SCHEMA</literal> only when superusers trust
@@ -571,18 +580,26 @@
</para>
<para>
- To create a subscription, the user must be a superuser.
+ To create a subscription, the user must be a superuser. The ownership can
+ subsequently be transferred or the role have superuser privilege removed.
</para>
<para>
The subscription apply process will run in the local database with the
- privileges of a superuser.
+ privileges of the subscription owner.
+ </para>
+
+ <para>
+ On the publisher, privileges are only checked once at the start of a
+ replication connection and are not re-checked as each change record is read.
</para>
<para>
- Privileges are only checked once at the start of a replication connection.
- They are not re-checked as each change record is read from the publisher,
- nor are they re-checked for each change when applied.
+ On the subscriber, the subscription owner's privileges are re-checked for
+ each change record when applied. If a worker is in the process of applying a
+ change record when the ownership of the subscription is changed by a
+ concurrent transaction, the application of the change record may finish
+ under the old permissions.
</para>
</sect1>
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index bc52339eba..41bf16e740 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -45,8 +45,11 @@ ALTER SUBSCRIPTION <replaceable class="parameter">name</replaceable> RENAME TO <
<para>
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.
+ To alter the owner, you must also be a direct or indirect member of the new
+ owning role. Replicated changes will be applied to the target relations as
+ the subscription owner. If the subscription owner lacks sufficient
+ privileges on the target relations, the replication workers will fail to
+ apply the changes.
</para>
<para>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index d75183cd12..0b17a582dd 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -1489,6 +1489,8 @@ AlterSubscriptionOwner_internal(Relation rel, HeapTuple tup, Oid newOwnerId)
InvokeObjectPostAlterHook(SubscriptionRelationId,
form->oid, 0);
+
+ ApplyLauncherWakeupAtCommit();
}
/*
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index f07983a43c..2400ef8c45 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -111,6 +111,7 @@
#include "replication/origin.h"
#include "storage/ipc.h"
#include "storage/lmgr.h"
+#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/lsyscache.h"
#include "utils/memutils.h"
@@ -924,6 +925,7 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
char relstate;
XLogRecPtr relstate_lsn;
Relation rel;
+ AclResult aclresult;
WalRcvExecResult *res;
char originname[NAMEDATALEN];
RepOriginId originid;
@@ -1042,6 +1044,17 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
*/
rel = table_open(MyLogicalRepWorker->relid, RowExclusiveLock);
+ /*
+ * Check that our table sync worker has permission to insert into the
+ * target table.
+ */
+ aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(),
+ ACL_INSERT);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult,
+ get_relkind_objtype(rel->rd_rel->relkind),
+ RelationGetRelationName(rel));
+
/*
* Start a transaction in the remote node in REPEATABLE READ mode. This
* ensures that both the replication slot we create (see below) and the
diff --git a/src/backend/replication/logical/worker.c b/src/backend/replication/logical/worker.c
index 8d96c926b4..6ae0b3fb21 100644
--- a/src/backend/replication/logical/worker.c
+++ b/src/backend/replication/logical/worker.c
@@ -179,6 +179,7 @@
#include "storage/proc.h"
#include "storage/procarray.h"
#include "tcop/tcopprot.h"
+#include "utils/acl.h"
#include "utils/builtins.h"
#include "utils/catcache.h"
#include "utils/dynahash.h"
@@ -1537,6 +1538,7 @@ apply_handle_insert(StringInfo s)
LogicalRepRelMapEntry *rel;
LogicalRepTupleData newtup;
LogicalRepRelId relid;
+ AclResult aclresult;
ApplyExecutionData *edata;
EState *estate;
TupleTableSlot *remoteslot;
@@ -1559,6 +1561,12 @@ apply_handle_insert(StringInfo s)
end_replication_step();
return;
}
+ aclresult = pg_class_aclcheck(RelationGetRelid(rel->localrel), GetUserId(),
+ ACL_INSERT);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult,
+ get_relkind_objtype(rel->localrel->rd_rel->relkind),
+ get_rel_name(rel->localreloid));
/* Set relation for error callback */
apply_error_callback_arg.rel = rel;
@@ -1659,6 +1667,7 @@ apply_handle_update(StringInfo s)
{
LogicalRepRelMapEntry *rel;
LogicalRepRelId relid;
+ AclResult aclresult;
ApplyExecutionData *edata;
EState *estate;
LogicalRepTupleData oldtup;
@@ -1686,6 +1695,12 @@ apply_handle_update(StringInfo s)
end_replication_step();
return;
}
+ aclresult = pg_class_aclcheck(RelationGetRelid(rel->localrel), GetUserId(),
+ ACL_UPDATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult,
+ get_relkind_objtype(rel->localrel->rd_rel->relkind),
+ get_rel_name(rel->localreloid));
/* Set relation for error callback */
apply_error_callback_arg.rel = rel;
@@ -1826,6 +1841,7 @@ apply_handle_delete(StringInfo s)
LogicalRepRelMapEntry *rel;
LogicalRepTupleData oldtup;
LogicalRepRelId relid;
+ AclResult aclresult;
ApplyExecutionData *edata;
EState *estate;
TupleTableSlot *remoteslot;
@@ -1848,6 +1864,12 @@ apply_handle_delete(StringInfo s)
end_replication_step();
return;
}
+ aclresult = pg_class_aclcheck(RelationGetRelid(rel->localrel), GetUserId(),
+ ACL_DELETE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult,
+ get_relkind_objtype(rel->localrel->rd_rel->relkind),
+ get_rel_name(rel->localreloid));
/* Set relation for error callback */
apply_error_callback_arg.rel = rel;
@@ -2220,6 +2242,7 @@ apply_handle_truncate(StringInfo s)
{
LogicalRepRelId relid = lfirst_oid(lc);
LogicalRepRelMapEntry *rel;
+ AclResult aclresult;
rel = logicalrep_rel_open(relid, lockmode);
if (!should_apply_changes_for_rel(rel))
@@ -2231,6 +2254,12 @@ apply_handle_truncate(StringInfo s)
logicalrep_rel_close(rel, lockmode);
continue;
}
+ aclresult = pg_class_aclcheck(RelationGetRelid(rel->localrel),
+ GetUserId(), ACL_TRUNCATE);
+ if (aclresult != ACLCHECK_OK)
+ aclcheck_error(aclresult,
+ get_relkind_objtype(rel->localrel->rd_rel->relkind),
+ get_rel_name(rel->localreloid));
remote_rels = lappend(remote_rels, rel);
rels = lappend(rels, rel->localrel);
@@ -2912,6 +2941,7 @@ maybe_reread_subscription(void)
strcmp(newsub->slotname, MySubscription->slotname) != 0 ||
newsub->binary != MySubscription->binary ||
newsub->stream != MySubscription->stream ||
+ newsub->owner != MySubscription->owner ||
!equal(newsub->publications, MySubscription->publications))
{
ereport(LOG,
diff --git a/src/test/subscription/t/026_nosuperuser.pl b/src/test/subscription/t/026_nosuperuser.pl
new file mode 100644
index 0000000000..03a5ac8c52
--- /dev/null
+++ b/src/test/subscription/t/026_nosuperuser.pl
@@ -0,0 +1,118 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# Test subscriptions owned by non-superusers
+use strict;
+use warnings;
+use PostgreSQL::Test::Cluster;
+use PostgreSQL::Test::Utils;
+use Test::More tests => 3;
+
+# Setup
+
+my $node_publisher = PostgreSQL::Test::Cluster->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
+$node_subscriber->init(allows_streaming => 'logical');
+$node_subscriber->start;
+
+my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+
+# Create the same schema, roles and permissions on both nodes.
+my $sql = qq(
+CREATE ROLE regress_role_alice NOSUPERUSER NOLOGIN;
+GRANT CREATE ON DATABASE postgres TO regress_role_alice;
+SET ROLE regress_role_alice;
+CREATE SCHEMA alice;
+CREATE TABLE alice.tbl1 (i1 INTEGER);
+ALTER TABLE alice.tbl1 REPLICA IDENTITY FULL;
+RESET ROLE;
+
+CREATE ROLE regress_role_bob NOSUPERUSER NOLOGIN;
+GRANT CREATE ON DATABASE postgres TO regress_role_bob;
+SET ROLE regress_role_bob;
+CREATE SCHEMA bob;
+CREATE TABLE bob.tbl1 (i1 INTEGER);
+CREATE TABLE bob.tbl2 (i2 INTEGER);
+ALTER TABLE bob.tbl1 REPLICA IDENTITY FULL;
+ALTER TABLE bob.tbl2 REPLICA IDENTITY FULL;
+RESET ROLE;
+);
+$node_publisher->safe_psql('postgres', $sql);
+$node_subscriber->safe_psql('postgres', $sql);
+
+# Bob publishes.
+$node_publisher->safe_psql('postgres', qq(
+SET ROLE regress_role_bob;
+CREATE PUBLICATION bob_pub FOR TABLE bob.tbl1;
+INSERT INTO bob.tbl1 VALUES (1);
+));
+
+# Bob gets DBA to create a subscribe for him.
+$node_subscriber->safe_psql('postgres', qq(
+CREATE SUBSCRIPTION bob_sub CONNECTION '$publisher_connstr' PUBLICATION bob_pub;
+ALTER SUBSCRIPTION bob_sub OWNER TO regress_role_bob;
+));
+
+# Wait for initial sync of all subscriptions.
+my $synced_query =
+ "SELECT count(1) = 0 FROM pg_subscription_rel WHERE srsubstate NOT IN ('r', 's');";
+$node_subscriber->poll_query_until('postgres', $synced_query)
+ or die "Timed out while waiting for subscriber to synchronize data";
+
+# Check that the data came through.
+my $result = $node_subscriber->safe_psql('postgres', qq(
+SET ROLE regress_role_bob;
+SELECT i1 FROM bob.tbl1;
+));
+is( $result, '1', 'data from schema bob was replicated');
+
+# A malicious dba on publisher adds alice's tables to publication "bob_pub".
+$node_publisher->safe_psql('postgres', qq(
+ALTER PUBLICATION bob_pub ADD TABLE alice.tbl1;
+INSERT INTO alice.tbl1 VALUES (1);
+));
+
+# Bob adds another table on publisher side to the publication and performs
+# DML on the new table.
+$node_publisher->safe_psql('postgres', qq(
+SET ROLE regress_role_bob;
+ALTER PUBLICATION bob_pub ADD TABLE bob.tbl2;
+INSERT INTO bob.tbl2 VALUES (2);
+));
+
+# Bob refreshes on subscriber side to get the new data from "bob.tbl2", not
+# knowing that alice's tables have been added. An error during tablesync or
+# apply should prevent the data in schema "alice" being replicated.
+#
+$node_subscriber->psql('postgres', qq(
+SET ROLE regress_role_bob;
+ALTER SUBSCRIPTION bob_sub REFRESH PUBLICATION;
+));
+
+# Wait for sync of all subscriptions.
+$node_subscriber->poll_query_until('postgres', $synced_query)
+ or die "Timed out while waiting for subscriber to synchronize data";
+
+# The data for schema "bob" might not be replicated, because the replication
+# may abort first on a permissions error for schema "alice". We insist that
+# data for "alice" not be replicated, but we only insist that if data from
+# "bob" gets replicated that it be the correct new data and not corrupted.
+$result = $node_subscriber->safe_psql('postgres', qq(
+SELECT a.i1::text || '_' ||
+ coalesce(b.i2::text,'null') || '_' ||
+ coalesce(c.i1::text,'null')
+ FROM bob.tbl1 a
+ LEFT JOIN bob.tbl2 b ON TRUE
+ LEFT JOIN alice.tbl1 c ON TRUE;
+));
+like ($result, qr/^(?:1_null_null|1_2_null)$/,
+ 'subscriber side schema bob data is reasonable and alice data is unreplicated');
+
+# check that the logs on the subscriber side contain the expected permissions
+# error associated with a failed attempt to replicate into schema "alice".
+my $subscriber_log = slurp_file($node_subscriber->logfile);
+like ($subscriber_log, qr/ERROR: permission denied for schema alice/msi,
+ 'subscriber lacks permission for schema alice');
--
2.21.1 (Apple Git-122.3)
view thread (68+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Non-superuser subscription owners
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox