public inbox for [email protected]
help / color / mirror / Atom feedFrom: Mark Dilger <[email protected]>
To: PostgreSQL-development <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Subject: Non-superuser subscription owners
Date: Wed, 20 Oct 2021 11:40:39 -0700
Message-ID: <[email protected]> (raw)
These patches have been split off the now deprecated monolithic "Delegating superuser tasks to new security roles" thread at [1].
The purpose of these patches is to allow non-superuser subscription owners without risk of them overwriting tables they lack privilege to write directly. This both allows subscriptions to be managed by non-superusers, and protects servers with subscriptions from malicious activity on the publisher side.
[1] https://www.postgresql.org/message-id/flat/F9408A5A-B20B-42D2-9E7F-49CD3D1547BC%40enterprisedb.com
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachments:
[application/octet-stream] v1-0001-Handle-non-superuser-subscription-owners-sensibly.patch (11.0K, ../[email protected]/2-v1-0001-Handle-non-superuser-subscription-owners-sensibly.patch)
download | inline diff:
From c2a9a709b70d47b1ede7dfc3bcb58328fe1fce53 Mon Sep 17 00:00:00 2001
From: Mark Dilger <[email protected]>
Date: Sun, 26 Sep 2021 18:21:26 -0700
Subject: [PATCH v1 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] v1-0002-Allow-subscription-ownership-by-non-superusers.patch (4.9K, ../[email protected]/3-v1-0002-Allow-subscription-ownership-by-non-superusers.patch)
download | inline diff:
From 9b1be64005e0e78694f9b6965075b35cfa77072e Mon Sep 17 00:00:00 2001
From: Mark Dilger <[email protected]>
Date: Sun, 26 Sep 2021 18:24:07 -0700
Subject: [PATCH v1 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] v1-0003-Respect-permissions-within-logical-replication.patch (14.4K, ../[email protected]/4-v1-0003-Respect-permissions-within-logical-replication.patch)
download | inline diff:
From cf0e0575e0a97f0b3a1aebee2d2fc4d3822daaeb Mon Sep 17 00:00:00 2001
From: Mark Dilger <[email protected]>
Date: Sun, 26 Sep 2021 18:25:32 -0700
Subject: [PATCH v1 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 | 49 +++++---
src/backend/replication/logical/tablesync.c | 13 +++
src/backend/replication/logical/worker.c | 29 +++++
src/test/subscription/t/025_nosuperuser.pl | 118 ++++++++++++++++++++
4 files changed, 196 insertions(+), 13 deletions(-)
create mode 100644 src/test/subscription/t/025_nosuperuser.pl
diff --git a/doc/src/sgml/logical-replication.sgml b/doc/src/sgml/logical-replication.sgml
index 88646bc859..d6c7698847 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,13 +539,15 @@
<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
- publications that list tables explicitly. That is to say, create a
- subscription <literal>FOR ALL TABLES</literal> only when superusers trust
- every user permitted to create a non-temp table on the publisher or the
- subscriber.
+ 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. Likewise, limit privileges of the subscription
+ owner to only the privileges needed to modify the tables that the
+ subscription should be able to change. 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> only when
+ superusers trust every user permitted to create a non-temp table on the
+ publisher or the subscriber.
</para>
<para>
@@ -569,18 +580,30 @@
</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, but beware that a change of ownership for a
+ subscription may not be noticed immediately by the replication workers.
+ Changes made on the publisher may be applied on the subscriber as
+ the old owner. In such cases, the old owner's privileges will be the ones
+ that matter. Worse still, it may be hard to predict when replication
+ workers will notice the new ownership. Subscriptions created disabled and
+ only enabled after ownership has been changed will not be subject to this
+ race condition.
</para>
</sect1>
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..459d9b5ae1 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);
diff --git a/src/test/subscription/t/025_nosuperuser.pl b/src/test/subscription/t/025_nosuperuser.pl
new file mode 100644
index 0000000000..93c830a0ed
--- /dev/null
+++ b/src/test/subscription/t/025_nosuperuser.pl
@@ -0,0 +1,118 @@
+
+# Copyright (c) 2021, PostgreSQL Global Development Group
+
+# Test subscriptions owned by non-superusers
+use strict;
+use warnings;
+use PostgresNode;
+use TestLib;
+use Test::More tests => 3;
+
+# Setup
+
+my $node_publisher = PostgresNode->new('publisher');
+$node_publisher->init(allows_streaming => 'logical');
+$node_publisher->start;
+
+my $node_subscriber = PostgresNode->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 alice NOSUPERUSER NOLOGIN;
+GRANT CREATE ON DATABASE postgres TO alice;
+SET ROLE alice;
+CREATE SCHEMA alice;
+CREATE TABLE alice.tbl1 (i1 INTEGER);
+ALTER TABLE alice.tbl1 REPLICA IDENTITY FULL;
+RESET ROLE;
+
+CREATE ROLE bob NOSUPERUSER NOLOGIN;
+GRANT CREATE ON DATABASE postgres TO bob;
+SET 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 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 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 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 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 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 = TestLib::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]
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