Re: running logical replication as the subscription owner
On Thu, Jun 8, 2023 at 7:29 PM Amit Kapila wrote: > > On Thu, Jun 8, 2023 at 6:32 AM Masahiko Sawada wrote: > > > > On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila wrote: > > > > > > On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada > > > wrote: > > > > > > > > On Thu, May 25, 2023 at 5:41 PM Amit Kapila > > > > wrote: > > > > > > > > I've attached the updated patch. Please review it. > > > > > > > > > > Few comments: > > > 1. > > > + /* get the owner for ACL and RLS checks */ > > > + run_as_owner = MySubscription->runasowner; > > > + checkowner = run_as_owner ? MySubscription->owner : > > > rel->rd_rel->relowner; > > > + > > > /* > > > * Check that our table sync worker has permission to insert into the > > > * target table. > > > */ > > > - aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(), > > > + aclresult = pg_class_aclcheck(RelationGetRelid(rel), checkowner, > > > > > > One thing that slightly worries me about this change is that we > > > started to check the permission for relowner before even ensuring that > > > we can switch to relowner. See checks in SwitchToUntrustedUser(). If > > > we want to first ensure that we can switch to relowner then I think we > > > should move this permission-checking code before we try to copy the > > > table. > > > > Agreed. I thought it's better to do ACL and RLS checks before creating > > the replication slot but it's not important. Rather checking them > > after switching user would make sense since we do the same in > > worker.c. > > > > LGTM. Thanks, pushed. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: running logical replication as the subscription owner
On Thu, Jun 8, 2023 at 6:32 AM Masahiko Sawada wrote: > > On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila wrote: > > > > On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada > > wrote: > > > > > > On Thu, May 25, 2023 at 5:41 PM Amit Kapila > > > wrote: > > > > > > I've attached the updated patch. Please review it. > > > > > > > Few comments: > > 1. > > + /* get the owner for ACL and RLS checks */ > > + run_as_owner = MySubscription->runasowner; > > + checkowner = run_as_owner ? MySubscription->owner : rel->rd_rel->relowner; > > + > > /* > > * Check that our table sync worker has permission to insert into the > > * target table. > > */ > > - aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(), > > + aclresult = pg_class_aclcheck(RelationGetRelid(rel), checkowner, > > > > One thing that slightly worries me about this change is that we > > started to check the permission for relowner before even ensuring that > > we can switch to relowner. See checks in SwitchToUntrustedUser(). If > > we want to first ensure that we can switch to relowner then I think we > > should move this permission-checking code before we try to copy the > > table. > > Agreed. I thought it's better to do ACL and RLS checks before creating > the replication slot but it's not important. Rather checking them > after switching user would make sense since we do the same in > worker.c. > LGTM. -- With Regards, Amit Kapila.
Re: running logical replication as the subscription owner
On Mon, Jun 5, 2023 at 3:15 AM Amit Kapila wrote: > > On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada wrote: > > > > On Thu, May 25, 2023 at 5:41 PM Amit Kapila wrote: > > > > I've attached the updated patch. Please review it. > > > > Few comments: > 1. > + /* get the owner for ACL and RLS checks */ > + run_as_owner = MySubscription->runasowner; > + checkowner = run_as_owner ? MySubscription->owner : rel->rd_rel->relowner; > + > /* > * Check that our table sync worker has permission to insert into the > * target table. > */ > - aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(), > + aclresult = pg_class_aclcheck(RelationGetRelid(rel), checkowner, > > One thing that slightly worries me about this change is that we > started to check the permission for relowner before even ensuring that > we can switch to relowner. See checks in SwitchToUntrustedUser(). If > we want to first ensure that we can switch to relowner then I think we > should move this permission-checking code before we try to copy the > table. Agreed. I thought it's better to do ACL and RLS checks before creating the replication slot but it's not important. Rather checking them after switching user would make sense since we do the same in worker.c. > > 2. In the commit message, the link for discussion > "https://postgr.es/m/CAA4eK1KfZcRq7hUqQ7WknP+u=08+6mevvm+2w5rrab+dtxr...@mail.gmail.com"; > is slightly misleading. Can we instead use > "https://www.postgresql.org/message-id/CAA4eK1L%3DqzRHPEn%2BqeMoKQGFBzqGoLBzt_ov0A89iFFiut%2BppA%40mail.gmail.com";? Agreed. I've attached the updated patch. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com v4-0001-Honor-run_as_owner-option-in-tablesync-worker.patch Description: Binary data
Re: running logical replication as the subscription owner
On Fri, May 26, 2023 at 6:18 PM Masahiko Sawada wrote: > > On Thu, May 25, 2023 at 5:41 PM Amit Kapila wrote: > > I've attached the updated patch. Please review it. > Few comments: 1. + /* get the owner for ACL and RLS checks */ + run_as_owner = MySubscription->runasowner; + checkowner = run_as_owner ? MySubscription->owner : rel->rd_rel->relowner; + /* * Check that our table sync worker has permission to insert into the * target table. */ - aclresult = pg_class_aclcheck(RelationGetRelid(rel), GetUserId(), + aclresult = pg_class_aclcheck(RelationGetRelid(rel), checkowner, One thing that slightly worries me about this change is that we started to check the permission for relowner before even ensuring that we can switch to relowner. See checks in SwitchToUntrustedUser(). If we want to first ensure that we can switch to relowner then I think we should move this permission-checking code before we try to copy the table. 2. In the commit message, the link for discussion "https://postgr.es/m/CAA4eK1KfZcRq7hUqQ7WknP+u=08+6mevvm+2w5rrab+dtxr...@mail.gmail.com"; is slightly misleading. Can we instead use "https://www.postgresql.org/message-id/CAA4eK1L%3DqzRHPEn%2BqeMoKQGFBzqGoLBzt_ov0A89iFFiut%2BppA%40mail.gmail.com";? -- With Regards, Amit Kapila.
Re: running logical replication as the subscription owner
On Thu, May 25, 2023 at 5:41 PM Amit Kapila wrote: > > On Thu, May 25, 2023 at 12:33 PM Masahiko Sawada > wrote: > > > > On Tue, May 23, 2023 at 8:21 PM Amit Kapila wrote: > > > > > > On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada > > > wrote: > > > > > > > > Thank you for updating the patch! Here are review comments: > > > > > > > > + /* > > > > +* Make sure that the copy command runs as the table owner, > > > > unless > > > > +* the user has opted out of that behaviour. > > > > +*/ > > > > + run_as_owner = MySubscription->runasowner; > > > > + if (!run_as_owner) > > > > + SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt); > > > > + > > > > /* Now do the initial data copy */ > > > > PushActiveSnapshot(GetTransactionSnapshot()); > > > > > > > > I think we should switch users before the acl check in > > > > LogicalRepSyncTableStart(). > > > > > > > > > > Agreed, we should check acl with the user that is going to perform > > > operations on the target table. BTW, is it okay to perform an > > > operation on the system table with the changed user as that would be > > > possible with your suggestion (see replorigin_create())? > > > > Do you see any problem in particular? > > > > As per the documentation, pg_replication_origin_create() is only > > allowed to the superuser by default, but in CreateSubscription() a > > non-superuser (who has pg_create_subscription privilege) can call > > replorigin_create(). > > Nothing in particular but it seems a bit odd to perform operations on > catalog tables with some other user table owners when that was not the > actual intent of this option. > > > OTOH, we don't necessarily need to switch to the > > table owner user for checking ACL and RLS. We can just pass either > > table owner OID or subscription owner OID to pg_class_aclcheck() and > > check_enable_rls() without actually switching the user. > > > > I think that would be better. Agreed. I've attached the updated patch. Please review it. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com v3-0001-Fix-tablesync-worker-missed-using-run_as_owner-op.patch Description: Binary data
Re: running logical replication as the subscription owner
On Thu, May 25, 2023 at 12:33 PM Masahiko Sawada wrote: > > On Tue, May 23, 2023 at 8:21 PM Amit Kapila wrote: > > > > On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada > > wrote: > > > > > > Thank you for updating the patch! Here are review comments: > > > > > > + /* > > > +* Make sure that the copy command runs as the table owner, unless > > > +* the user has opted out of that behaviour. > > > +*/ > > > + run_as_owner = MySubscription->runasowner; > > > + if (!run_as_owner) > > > + SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt); > > > + > > > /* Now do the initial data copy */ > > > PushActiveSnapshot(GetTransactionSnapshot()); > > > > > > I think we should switch users before the acl check in > > > LogicalRepSyncTableStart(). > > > > > > > Agreed, we should check acl with the user that is going to perform > > operations on the target table. BTW, is it okay to perform an > > operation on the system table with the changed user as that would be > > possible with your suggestion (see replorigin_create())? > > Do you see any problem in particular? > > As per the documentation, pg_replication_origin_create() is only > allowed to the superuser by default, but in CreateSubscription() a > non-superuser (who has pg_create_subscription privilege) can call > replorigin_create(). Nothing in particular but it seems a bit odd to perform operations on catalog tables with some other user table owners when that was not the actual intent of this option. > OTOH, we don't necessarily need to switch to the > table owner user for checking ACL and RLS. We can just pass either > table owner OID or subscription owner OID to pg_class_aclcheck() and > check_enable_rls() without actually switching the user. > I think that would be better. -- With Regards, Amit Kapila.
Re: running logical replication as the subscription owner
On Tue, May 23, 2023 at 8:21 PM Amit Kapila wrote: > > On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada wrote: > > > > Thank you for updating the patch! Here are review comments: > > > > + /* > > +* Make sure that the copy command runs as the table owner, unless > > +* the user has opted out of that behaviour. > > +*/ > > + run_as_owner = MySubscription->runasowner; > > + if (!run_as_owner) > > + SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt); > > + > > /* Now do the initial data copy */ > > PushActiveSnapshot(GetTransactionSnapshot()); > > > > I think we should switch users before the acl check in > > LogicalRepSyncTableStart(). > > > > Agreed, we should check acl with the user that is going to perform > operations on the target table. BTW, is it okay to perform an > operation on the system table with the changed user as that would be > possible with your suggestion (see replorigin_create())? Do you see any problem in particular? As per the documentation, pg_replication_origin_create() is only allowed to the superuser by default, but in CreateSubscription() a non-superuser (who has pg_create_subscription privilege) can call replorigin_create(). OTOH, we don't necessarily need to switch to the table owner user for checking ACL and RLS. We can just pass either table owner OID or subscription owner OID to pg_class_aclcheck() and check_enable_rls() without actually switching the user. > > > > > While this approach works, I'm not sure we really need a trigger for > > this test. I've attached a patch for discussion that doesn't use > > triggers for the regression tests. We create a new subscription owned > > by a user who doesn't have the permission to the target table. The > > test passes only if run_as_owner = true works. > > > > Why in the test do you need to give additional permissions to > regress_admin2 when the actual operation has to be performed by the > table owner? Good point. We need to give the ability to SET ROLE to regress_admin2 but other permissions are unnecessary. > > +# Because the initial data sync is working as the table owner, all > +# dat should be copied. > > Typo. /dat/data Will fix. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: running logical replication as the subscription owner
On Mon, May 22, 2023 at 10:36 PM Masahiko Sawada wrote: > > On Wed, May 17, 2023 at 10:10 AM Ajin Cherian wrote: > > > > On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada > > wrote: > > > > > > On Mon, May 15, 2023 at 5:44 PM Ajin Cherian wrote: > > > > > > > > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian wrote: > > > > > > > > > > If nobody else is working on this, I can come up with a patch to fix > > > > > this > > > > > > > > > > > > > Attaching a patch which attempts to fix this. > > > > > > > > > > Thank you for the patch! I think we might want to have tests for it. > > > > > I have updated the patch with a test case as well. > > Thank you for updating the patch! Here are review comments: > > + /* > +* Make sure that the copy command runs as the table owner, unless > +* the user has opted out of that behaviour. > +*/ > + run_as_owner = MySubscription->runasowner; > + if (!run_as_owner) > + SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt); > + > /* Now do the initial data copy */ > PushActiveSnapshot(GetTransactionSnapshot()); > > I think we should switch users before the acl check in > LogicalRepSyncTableStart(). > > --- > +# Create a trigger on table alice.unpartitioned that writes > +# to a table that regress_alice does not have permission. > +$node_subscriber->safe_psql( > + 'postgres', qq( > +SET SESSION AUTHORIZATION regress_alice; > +CREATE OR REPLACE FUNCTION alice.alice_audit() > +RETURNS trigger AS > +\$\$ > + BEGIN > + insert into public.admin_audit values(2); > + RETURN NEW; > + END; > +\$\$ > +LANGUAGE 'plpgsql'; > +CREATE TRIGGER ALICE_TRIGGER AFTER INSERT ON alice.unpartitioned FOR EACH ROW > +EXECUTE PROCEDURE alice.alice_audit(); > +ALTER TABLE alice.unpartitioned ENABLE ALWAYS TRIGGER ALICE_TRIGGER; > +)); > > While this approach works, I'm not sure we really need a trigger for > this test. I've attached a patch for discussion that doesn't use > triggers for the regression tests. We create a new subscription owned > by a user who doesn't have the permission to the target table. The > test passes only if run_as_owner = true works. > this is better, thanks. Since you are testing run_as_owner = false behaviour during table copy phase, you might as well add a test case that it correctly behaves during insert replication as well. regards, Ajin Cherian Fujitsu Australia
Re: running logical replication as the subscription owner
On Mon, May 22, 2023 at 6:06 PM Masahiko Sawada wrote: > > Thank you for updating the patch! Here are review comments: > > + /* > +* Make sure that the copy command runs as the table owner, unless > +* the user has opted out of that behaviour. > +*/ > + run_as_owner = MySubscription->runasowner; > + if (!run_as_owner) > + SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt); > + > /* Now do the initial data copy */ > PushActiveSnapshot(GetTransactionSnapshot()); > > I think we should switch users before the acl check in > LogicalRepSyncTableStart(). > Agreed, we should check acl with the user that is going to perform operations on the target table. BTW, is it okay to perform an operation on the system table with the changed user as that would be possible with your suggestion (see replorigin_create())? > > While this approach works, I'm not sure we really need a trigger for > this test. I've attached a patch for discussion that doesn't use > triggers for the regression tests. We create a new subscription owned > by a user who doesn't have the permission to the target table. The > test passes only if run_as_owner = true works. > Why in the test do you need to give additional permissions to regress_admin2 when the actual operation has to be performed by the table owner? +# Because the initial data sync is working as the table owner, all +# dat should be copied. Typo. /dat/data -- With Regards, Amit Kapila.
Re: running logical replication as the subscription owner
On Wed, May 17, 2023 at 10:10 AM Ajin Cherian wrote: > > On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada > wrote: > > > > On Mon, May 15, 2023 at 5:44 PM Ajin Cherian wrote: > > > > > > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian wrote: > > > > > > > > If nobody else is working on this, I can come up with a patch to fix > > > > this > > > > > > > > > > Attaching a patch which attempts to fix this. > > > > > > > Thank you for the patch! I think we might want to have tests for it. > > > I have updated the patch with a test case as well. Thank you for updating the patch! Here are review comments: + /* +* Make sure that the copy command runs as the table owner, unless +* the user has opted out of that behaviour. +*/ + run_as_owner = MySubscription->runasowner; + if (!run_as_owner) + SwitchToUntrustedUser(rel->rd_rel->relowner, &ucxt); + /* Now do the initial data copy */ PushActiveSnapshot(GetTransactionSnapshot()); I think we should switch users before the acl check in LogicalRepSyncTableStart(). --- +# Create a trigger on table alice.unpartitioned that writes +# to a table that regress_alice does not have permission. +$node_subscriber->safe_psql( + 'postgres', qq( +SET SESSION AUTHORIZATION regress_alice; +CREATE OR REPLACE FUNCTION alice.alice_audit() +RETURNS trigger AS +\$\$ + BEGIN + insert into public.admin_audit values(2); + RETURN NEW; + END; +\$\$ +LANGUAGE 'plpgsql'; +CREATE TRIGGER ALICE_TRIGGER AFTER INSERT ON alice.unpartitioned FOR EACH ROW +EXECUTE PROCEDURE alice.alice_audit(); +ALTER TABLE alice.unpartitioned ENABLE ALWAYS TRIGGER ALICE_TRIGGER; +)); While this approach works, I'm not sure we really need a trigger for this test. I've attached a patch for discussion that doesn't use triggers for the regression tests. We create a new subscription owned by a user who doesn't have the permission to the target table. The test passes only if run_as_owner = true works. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com fix_run_as_owner.patch Description: Binary data
Re: running logical replication as the subscription owner
On Mon, May 15, 2023 at 10:47 PM Masahiko Sawada wrote: > > On Mon, May 15, 2023 at 5:44 PM Ajin Cherian wrote: > > > > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian wrote: > > > > > > If nobody else is working on this, I can come up with a patch to fix this > > > > > > > Attaching a patch which attempts to fix this. > > > > Thank you for the patch! I think we might want to have tests for it. > I have updated the patch with a test case as well. regards, Ajin Cherian Fujitsu Australia v2-0001-Fix-issue-where-the-copy-command-does-not-adhere-.patch Description: Binary data
Re: running logical replication as the subscription owner
On Tue, May 16, 2023 at 2:39 AM Amit Kapila wrote: > Agreed with you and Sawada-San about having a test. BTW, shall we > slightly tweak the documentation [1]: "The subscription apply process > will, at a session level, run with the privileges of the subscription > owner. However, when performing an insert, update, delete, or truncate > operation on a particular table, it will switch roles to the table > owner and perform the operation with the table owner's privileges." to > be bit more specific about initial sync process as well? It doesn't seem entirely necessary to me because the initial sync is in effect a bunch of inserts. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Mon, May 15, 2023 at 7:24 PM Jelte Fennema wrote: > > On Mon, 15 May 2023 at 14:47, Masahiko Sawada wrote: > > Thank you for the patch! I think we might want to have tests for it. > > Yes, code looks good. But indeed some tests would be great. It seems > we forgot to actually do: > Agreed with you and Sawada-San about having a test. BTW, shall we slightly tweak the documentation [1]: "The subscription apply process will, at a session level, run with the privileges of the subscription owner. However, when performing an insert, update, delete, or truncate operation on a particular table, it will switch roles to the table owner and perform the operation with the table owner's privileges." to be bit more specific about initial sync process as well? [1] - https://www.postgresql.org/docs/devel/logical-replication-security.html -- With Regards, Amit Kapila.
Re: running logical replication as the subscription owner
On Fri, May 12, 2023 at 5:25 PM Ajin Cherian wrote: > > On Fri, May 12, 2023 at 1:49 PM Amit Kapila wrote: > > > > I tried the following test: > > > Repeat On the publisher and subscriber: > /* Create role regress_alice with NOSUPERUSER on >publisher and subscriber and a table for replication */ > > CREATE ROLE regress_alice NOSUPERUSER LOGIN; > CREATE ROLE regress_admin SUPERUSER LOGIN; > GRANT CREATE ON DATABASE postgres TO regress_alice; > SET SESSION AUTHORIZATION regress_alice; > CREATE SCHEMA alice; > GRANT USAGE ON SCHEMA alice TO regress_admin; > CREATE TABLE alice.test (i INTEGER); > ALTER TABLE alice.test REPLICA IDENTITY FULL; > Why do we need a schema and following grant statement for this test? > On the publisher: > postgres=> insert into alice.test values(1); > postgres=> insert into alice.test values(2); > postgres=> insert into alice.test values(3); > postgres=> CREATE PUBLICATION alice FOR TABLE alice.test > WITH (publish_via_partition_root = true); > Again, 'publish_via_partition_root' doesn't seem to be required. Let's try to write a minimal test for the initial sync behaviour. -- With Regards, Amit Kapila.
Re: running logical replication as the subscription owner
On Fri, 24 Mar 2023 at 19:37, Robert Haas wrote: > > > > I think there's some important tests missing related to this: > > > > 1. Ensuring that SECURITY_RESTRICTED_OPERATION things are enforced > > > > when the user **does not** have SET ROLE permissions to the > > > > subscription owner, e.g. don't allow SET ROLE from a trigger. > > > > 2. Ensuring that SECURITY_RESTRICTED_OPERATION things are not enforced > > > > when the user **does** have SET ROLE permissions to the subscription > > > > owner, e.g. allows SET ROLE from trigger. > > > Yeah, if we stick with the current approach we should probably add > > > tests for that stuff. > > > > Even if we don't, we should still have tests showing that the security > > restrictions that we intend to put in place actually do their job. > > Yeah, I just don't want to write the tests and then decide to change > the behavior and then have to write them over again. It's not so much > fun that I'm yearning to do it twice. I forgot to follow up on this before, but based on the bug found by Amit. I think it would be good to still add these tests.
Re: running logical replication as the subscription owner
On Mon, 15 May 2023 at 14:47, Masahiko Sawada wrote: > Thank you for the patch! I think we might want to have tests for it. Yes, code looks good. But indeed some tests would be great. It seems we forgot to actually do: On Fri, 12 May 2023 at 13:55, Ajin Cherian wrote: > CREATE ROLE regress_alice NOSUPERUSER LOGIN; > CREATE ROLE regress_admin SUPERUSER LOGIN; > ... > > What I see is that as part of tablesync, the trigger invokes an > updates admin_audit which it shouldn't, as the table owner > of alice.test should not have access to the > table admin_audit. This means the table copy is being invoked as the > subscription owner and not the table owner. I think having this as a tap/regress test would be very useful.
Re: running logical replication as the subscription owner
On Mon, May 15, 2023 at 5:44 PM Ajin Cherian wrote: > > On Fri, May 12, 2023 at 9:55 PM Ajin Cherian wrote: > > > > If nobody else is working on this, I can come up with a patch to fix this > > > > Attaching a patch which attempts to fix this. > Thank you for the patch! I think we might want to have tests for it. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: running logical replication as the subscription owner
On Fri, May 12, 2023 at 9:55 PM Ajin Cherian wrote: > > If nobody else is working on this, I can come up with a patch to fix this > Attaching a patch which attempts to fix this. regards, Ajin Cherian Fujitsu Australia v1-0001-Fix-issue-where-the-copy-command-does-not-adhere-.patch Description: Binary data
Re: running logical replication as the subscription owner
On Fri, May 12, 2023 at 1:49 PM Amit Kapila wrote: > > On Fri, May 12, 2023 at 9:10 AM Masahiko Sawada wrote: > > > > On Fri, May 12, 2023 at 1:12 AM Robert Haas wrote: > > > > > > On Thu, May 11, 2023 at 7:38 AM Amit Kapila > > > wrote: > > > > Do we want the initial sync to also respect 'run_as_owner' option? I > > > > might be missing something but I don't see anything in the docs about > > > > initial sync interaction with this option. In the commit a2ab9c06ea, > > > > we did the permission checking during the initial sync so I thought we > > > > should do it here as well. > > > > > > It definitely should work that way. lf it doesn't, that's a bug. > > > > After some tests, it seems that the initial sync worker respects > > 'run_as_owner' during catching up but not during COPYing. > > > > Yeah, I was worried during copy phase only. During catchup, the code > is common with apply worker code, so it will work. > I tried the following test: Repeat On the publisher and subscriber: /* Create role regress_alice with NOSUPERUSER on publisher and subscriber and a table for replication */ CREATE ROLE regress_alice NOSUPERUSER LOGIN; CREATE ROLE regress_admin SUPERUSER LOGIN; GRANT CREATE ON DATABASE postgres TO regress_alice; SET SESSION AUTHORIZATION regress_alice; CREATE SCHEMA alice; GRANT USAGE ON SCHEMA alice TO regress_admin; CREATE TABLE alice.test (i INTEGER); ALTER TABLE alice.test REPLICA IDENTITY FULL; On the publisher: postgres=> insert into alice.test values(1); postgres=> insert into alice.test values(2); postgres=> insert into alice.test values(3); postgres=> CREATE PUBLICATION alice FOR TABLE alice.test WITH (publish_via_partition_root = true); On the subscriber: /* create table admin_audit which regress_alice does not have access to */ SET SESSION AUTHORIZATION regress_admin; create table admin_audit (i integer); On the subscriber: /* Create a trigger for table alice.test which inserts on table admin_audit which the table owner of alice.test does not have access to */ SET SESSION AUTHORIZATION regress_alice; CREATE OR REPLACE FUNCTION alice.alice_audit() RETURNS trigger AS $$ BEGIN insert into public.admin_audit values(2); RETURN NEW; END; $$ LANGUAGE 'plpgsql'; create trigger test_alice after insert on alice.test for each row execute procedure alice.alice_audit(); alter table alice.test enable always trigger test_alice; On the subscriber: /* Create a subscription with run_as_owner = false */ CREATE SUBSCRIPTION admin_sub CONNECTION 'dbname=postgres host=localhost port=6972' PUBLICATION alice WITH (run_as_owner = false); === What I see is that as part of tablesync, the trigger invokes an updates admin_audit which it shouldn't, as the table owner of alice.test should not have access to the table admin_audit. This means the table copy is being invoked as the subscription owner and not the table owner. However, I see subsequent inserts fail on replication with permission denied error, so the apply worker correctly applies the inserts as the table owner. If nobody else is working on this, I can come up with a patch to fix this regards, Ajin Cherian Fujitsu Australia
Re: running logical replication as the subscription owner
On Fri, May 12, 2023 at 9:10 AM Masahiko Sawada wrote: > > On Fri, May 12, 2023 at 1:12 AM Robert Haas wrote: > > > > On Thu, May 11, 2023 at 7:38 AM Amit Kapila wrote: > > > Do we want the initial sync to also respect 'run_as_owner' option? I > > > might be missing something but I don't see anything in the docs about > > > initial sync interaction with this option. In the commit a2ab9c06ea, > > > we did the permission checking during the initial sync so I thought we > > > should do it here as well. > > > > It definitely should work that way. lf it doesn't, that's a bug. > > After some tests, it seems that the initial sync worker respects > 'run_as_owner' during catching up but not during COPYing. > Yeah, I was worried during copy phase only. During catchup, the code is common with apply worker code, so it will work. -- With Regards, Amit Kapila.
Re: running logical replication as the subscription owner
On Fri, May 12, 2023 at 1:12 AM Robert Haas wrote: > > On Thu, May 11, 2023 at 7:38 AM Amit Kapila wrote: > > Do we want the initial sync to also respect 'run_as_owner' option? I > > might be missing something but I don't see anything in the docs about > > initial sync interaction with this option. In the commit a2ab9c06ea, > > we did the permission checking during the initial sync so I thought we > > should do it here as well. > > It definitely should work that way. lf it doesn't, that's a bug. After some tests, it seems that the initial sync worker respects 'run_as_owner' during catching up but not during COPYing. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Re: running logical replication as the subscription owner
On Thu, May 11, 2023 at 7:38 AM Amit Kapila wrote: > Do we want the initial sync to also respect 'run_as_owner' option? I > might be missing something but I don't see anything in the docs about > initial sync interaction with this option. In the commit a2ab9c06ea, > we did the permission checking during the initial sync so I thought we > should do it here as well. It definitely should work that way. lf it doesn't, that's a bug. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Tue, Apr 4, 2023 at 9:40 PM Robert Haas wrote: > > On Mon, Apr 3, 2023 at 10:09 PM Noah Misch wrote: > > I gather we agree on what to do for v16, which is good. > > I have committed the patches. > Do we want the initial sync to also respect 'run_as_owner' option? I might be missing something but I don't see anything in the docs about initial sync interaction with this option. In the commit a2ab9c06ea, we did the permission checking during the initial sync so I thought we should do it here as well. -- With Regards, Amit Kapila.
Re: running logical replication as the subscription owner
On Mon, Apr 10, 2023 at 10:09 PM Shinoda, Noriyoshi (PN Japan FSIP) wrote: > Hi hackers, > Thank you for developing a great feature. > The following commit added a column to the pg_subscription catalog. > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=482675987bcdffb390ae735cfd5f34b485ae97c6 > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c3afe8cf5a1e465bd71e48e4bc717f5bfdc7a7d6 > > I found that the documentation of the pg_subscription catalog was missing an > explanation of the columns subrunasowner and subpasswordrequired, so I > attached a patch. Please fix the patch if you have a better explanation. Thank you. Committed. -- Robert Haas EDB: http://www.enterprisedb.com
RE: running logical replication as the subscription owner
Hi hackers, Thank you for developing a great feature. The following commit added a column to the pg_subscription catalog. https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=482675987bcdffb390ae735cfd5f34b485ae97c6 https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=c3afe8cf5a1e465bd71e48e4bc717f5bfdc7a7d6 I found that the documentation of the pg_subscription catalog was missing an explanation of the columns subrunasowner and subpasswordrequired, so I attached a patch. Please fix the patch if you have a better explanation. Regards, Noriyoshi Shinoda -Original Message- From: Robert Haas Sent: Wednesday, April 5, 2023 1:10 AM To: Noah Misch Cc: Jeff Davis ; Jelte Fennema ; pgsql-hack...@postgresql.org; Andres Freund Subject: Re: running logical replication as the subscription owner On Mon, Apr 3, 2023 at 10:09 PM Noah Misch wrote: > I gather we agree on what to do for v16, which is good. I have committed the patches. -- Robert Haas EDB: http://www.enterprisedb.com pg_subscription_doc_v1.diff Description: pg_subscription_doc_v1.diff
Re: running logical replication as the subscription owner
On Mon, Apr 3, 2023 at 10:09 PM Noah Misch wrote: > I gather we agree on what to do for v16, which is good. I have committed the patches. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Mon, Apr 03, 2023 at 12:05:29PM -0700, Jeff Davis wrote: > On Sun, 2023-04-02 at 20:21 -0700, Noah Misch wrote: > > The most-plausible-to-me attack involves an ENABLE ALWAYS trigger > > that logs > > CURRENT_USER to an audit table. > > How does requiring that the subscription owner have SET ROLE privileges > on the table owner help that case? As Robert pointed out, users coming > from v15 will have superuser subscription owners anyway, so the change > will be silent for them. For subscriptions upgraded from v15, it doesn't matter. Requiring SET ROLE prevents this sequence: - Make a table with such an audit trigger. Grant INSERT and UPDATE to Alice. - Upgrade to v15. - Grant pg_create_subscription and database-level CREATE to Alice. - Alice creates a subscription as a tool to impersonate the table owner, bypassing audit. To put it another way, the benefit of the SET ROLE requirement is not really making subscriptions more secure. The benefit of the requirement is pg_create_subscription not becoming a tool for bypassing audit. I gather we agree on what to do for v16, which is good. > But I feel like we can do better in version 17 when we have time to > actually work through common use cases and the exceptional cases and > weight them appropriately. Like, how common is it to want to get the > user from a trigger on the subscriber side? Fair. I don't think the community has arrived at a usable approach for answering questions like that. It would be valuable to have an approach. > Should that trigger be > using SESSION_USER instead of CURRENT_USER? Apart from evaluating the argument of SET ROLE, I've not heard of a valid use case for SESSION_USER.
Re: running logical replication as the subscription owner
On Mon, 2023-04-03 at 10:26 -0400, Robert Haas wrote: > Not very much. I think the biggest risk is user confusion, but I > don't > think that's a huge risk because I don't think this scenario will > come > up very often. Also, it's kind of hard to imagine that there's a > security model here which never does anything potentially surprising. Alright, let's just proceed as-is then. I believe these patches are a major improvement to the usability of logical replication and will put up with the weirdness. I wanted to understand better why it's there, and I'm not sure I fully do, but we'll have more time to discuss later. Regards, Jeff Davis
Re: running logical replication as the subscription owner
On Sun, 2023-04-02 at 20:21 -0700, Noah Misch wrote: > The most-plausible-to-me attack involves an ENABLE ALWAYS trigger > that logs > CURRENT_USER to an audit table. How does requiring that the subscription owner have SET ROLE privileges on the table owner help that case? As Robert pointed out, users coming from v15 will have superuser subscription owners anyway, so the change will be silent for them. We need support to apply changes as the table owner, and we need that to be the default; and this patch provides those things, and almost all users of logical replication will be better off after this is committed. The small number of users for whom this new model is not good still need the right documentation in front of them to understand the consequences, so that they can opt out one way or another (as 0002 offers). Release notes are probably the most powerful tool we have for notifying users, unfortunately. Requiring SET ROLE for users that are almost certainly superusers doesn't give an opportunity to educate people about the change in behavior. As I said before, I'm fine with requiring that the subscription owner can SET ROLE to the table owner for v16. It's the most conservative choice and the most "correct" (in that no lesser privilege we have today is a perfect match). But I feel like we can do better in version 17 when we have time to actually work through common use cases and the exceptional cases and weight them appropriately. Like, how common is it to want to get the user from a trigger on the subscriber side? Should that trigger be using SESSION_USER instead of CURRENT_USER? Security is best when it takes into account what people actually want to do and makes it easy to do that securely. Regards, Jeff Davis
Re: running logical replication as the subscription owner
Thank you for this email. It's very helpful to get your opinion on this. On Sun, Apr 2, 2023 at 11:21 PM Noah Misch wrote: > On Wed, Mar 29, 2023 at 04:00:45PM -0400, Robert Haas wrote: > > > The dangerous cases seem to be something along the lines of a security- > > > invoker trigger function that builds and executes arbirary SQL based on > > > the row contents. And then the table owner would then still need to set > > > ENABLE ALWAYS TRIGGER. > > > > > > Do we really want to take that case on as our security responsibility? > > > > That's something about which I would like to get more opinions. > > The most-plausible-to-me attack involves an ENABLE ALWAYS trigger that logs > CURRENT_USER to an audit table. The "SQL based on the row contents" scenario > feels remote. Another remotely-possible attack involves a trigger that > internally queries some other table having RLS. (Switching to the table owner > can change the rows visible to that query.) I had thought of the first of these cases, but not the second one. > If having INSERT/UPDATE privileges on the table were enough to make a > subscription that impersonates the table owner, then relatively-unprivileged > roles could make a subscription to bypass the aforementioned auditing. Commit > c3afe8c has imposed weighty requirements beyond I/U privileges, namely holding > the pg_create_subscription role and database-level CREATE privilege. Since > database-level CREATE is already powerful, it would be plausible to drop the > SET ROLE requirement and add this audit bypass to its powers. The SET ROLE > requirement is nice for keeping the powers disentangled. One drawback is > making people do GRANTs regardless of whether a relevant audit trigger exists. > Another drawback is the subscription role having more privileges than ideally > needed. I do like keeping strong privileges orthogonal, so I lean toward > keeping the SET ROLE requirement. The orthogonality argument weighs extremely heavily with me in this case. As I said to Jeff, I would not mind having a more granular way to control which tables a user can replicate into; e.g. a grantable REPLICAT{E,ION} privilege, or we want something global we could have a predefined role for it, e.g. pg_replicate_into_any_table. But I think any such thing should definitely be separate from pg_create_subscription. I'll fix the typos. Thanks for reporting them. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Fri, Mar 31, 2023 at 6:46 PM Jeff Davis wrote: > I guess the "more convenient" is where I'm confused, because the "grant > subscription_owner to table owner with set role true" is not likely to > be conveniently already present; it would need to be issued manually to > take advantage of this special case. You and I disagree about the likelihood of that, but I could well be wrong. > Do you have any concern about the weirdness where assigning the > subscription to a higher-privilege user Z would cause B's trigger to > fail? Not very much. I think the biggest risk is user confusion, but I don't think that's a huge risk because I don't think this scenario will come up very often. Also, it's kind of hard to imagine that there's a security model here which never does anything potentially surprising. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Thu, Mar 30, 2023 at 7:12 PM Robert Haas wrote: > > I don't think run_as_owner is terrible, despite the ambiguity. It's > talking about the owner of the object on which the property is being > set. > I find this justification quite reasonable to keep the option name as run_as_owner. So, +1 to use the new option name as run_as_owner. -- With Regards, Amit Kapila.
Re: running logical replication as the subscription owner
On Mon, Mar 27, 2023 at 04:47:22PM -0400, Robert Haas wrote: > So that's a grey area, at least IMHO. The patch could be revised in > some way, and the permissions requirements downgraded. However, if we > do that, I think we're going to have to document that although in > theory table owners can make themselves against subscription owners, > in practice they probably won't be. That approach has some advantages, > and I don't think it's insane. However, I am not convinced that it is > the best idea, either, and I had the impression based on > pgsql-security discussion that Andres and Noah thought this way was > better. I might have misinterpreted their position, and they might > have changed their minds, and they might have been wrong. But that's > how we got here. ["this way" = requirement for SET ROLE] On Wed, Mar 29, 2023 at 04:00:45PM -0400, Robert Haas wrote: > > The dangerous cases seem to be something along the lines of a security- > > invoker trigger function that builds and executes arbirary SQL based on > > the row contents. And then the table owner would then still need to set > > ENABLE ALWAYS TRIGGER. > > > > Do we really want to take that case on as our security responsibility? > > That's something about which I would like to get more opinions. The most-plausible-to-me attack involves an ENABLE ALWAYS trigger that logs CURRENT_USER to an audit table. The "SQL based on the row contents" scenario feels remote. Another remotely-possible attack involves a trigger that internally queries some other table having RLS. (Switching to the table owner can change the rows visible to that query.) If having INSERT/UPDATE privileges on the table were enough to make a subscription that impersonates the table owner, then relatively-unprivileged roles could make a subscription to bypass the aforementioned auditing. Commit c3afe8c has imposed weighty requirements beyond I/U privileges, namely holding the pg_create_subscription role and database-level CREATE privilege. Since database-level CREATE is already powerful, it would be plausible to drop the SET ROLE requirement and add this audit bypass to its powers. The SET ROLE requirement is nice for keeping the powers disentangled. One drawback is making people do GRANTs regardless of whether a relevant audit trigger exists. Another drawback is the subscription role having more privileges than ideally needed. I do like keeping strong privileges orthogonal, so I lean toward keeping the SET ROLE requirement. On Thu, Mar 30, 2023 at 02:17:31PM -0400, Robert Haas wrote: > --- a/doc/src/sgml/logical-replication.sgml > +++ b/doc/src/sgml/logical-replication.sgml > @@ -1774,6 +1774,23 @@ CONTEXT: processing remote data for replication > origin "pg_16395" during "INSER > SET ROLE to each role that owns a replicated table. > > > + > + If the subscription has been configued with Typo. > Subject: [PATCH v3 1/2] Perform logical replication actions as the table > owner. > Since this involves switching the active user frequently within > a session that is authenticated as the subscription user, also > impose SECURITY_RESTRICTED_OPEATION restrictions on logical s/OPEATION/OPERATION/ > replication code. As an exception, if the table owner can SET > ROLE to the subscription owner, these restrictions have no > security value, so don't impose them in that case. > > Subscription owners are now required to have the ability to > SET ROLE to every role that owns a table that the subscription > is replicating. If they don't, replication will fail. Superusers, > who normally own subscriptions, satisfy this property by default. > Non-superusers users who own subscriptions will needed to be > granted the roles that own relevant tables. s/will needed/will need/ (I did not read the patches in their entirety.)
Re: running logical replication as the subscription owner
On Fri, 2023-03-31 at 15:17 -0400, Robert Haas wrote: > I think that's too Boolean. The special case in 0001 is a better > solution for the cases where it works. It's both more granular and > more convenient. I guess the "more convenient" is where I'm confused, because the "grant subscription_owner to table owner with set role true" is not likely to be conveniently already present; it would need to be issued manually to take advantage of this special case. Do you have any concern about the weirdness where assigning the subscription to a higher-privilege user Z would cause B's trigger to fail? Regards, Jeff Davis
Re: running logical replication as the subscription owner
On Fri, Mar 31, 2023 at 3:36 AM Jeff Davis wrote: > That's moving the goalposts a little, though: > > "Some concern was expressed ... might break things that are currently > working..." > > https://www.postgresql.org/message-id/CA+TgmoaE35kKS3-zSvGiZszXP9Tb9rNfYzT=+fo8ehk5edk...@mail.gmail.com > > If the original use case was "don't break stuff", I think patch 0002 > solves that, and we don't need this special case in 0001. Would you > agree with that statement? I think that's too Boolean. The special case in 0001 is a better solution for the cases where it works. It's both more granular and more convenient. The fact that you might be able to get by with 0002 doesn't negate that. > > But I imagine CREATE SUBSCRIPTION being used either by > > superusers or by people who already have those role grants anyway, > > because I imagine replication as something that a highly privileged > > user configures on behalf of everyone who uses the system. And in > > that > > case those role grants aren't something new that you do specifically > > for logical replication - they're already there because you need them > > to administer stuff. Or you're the superuser and don't need them > > anyway. > > Did the discussion drift back towards the SET ROLE in the other > direction? I thought we had settled that in v16 we would require that > the subscription owner can SET ROLE to the table owner (as in your > current 0001), but that we could revisit it later. Yeah, I think that's what we agreed. I'm just saying that I'm not as concerned about that design as you are, and encouraging you to maybe not be quite so dismayed by it. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Thu, 2023-03-30 at 16:08 -0400, Robert Haas wrote: > But the run_as_owner option applies to the entire subscription. > If A activates that option, then B's hypothetical triggers that run > afoul of the SECURITY_RESTRICTED_OPERATION restrictions start working > again (woohoo!) but they're now vulnerable to attacks from C. With > the > patch as coded, A doesn't need to use run_as_owner, everything still > just works for B, and A is still protected against C. That's moving the goalposts a little, though: "Some concern was expressed ... might break things that are currently working..." https://www.postgresql.org/message-id/CA+TgmoaE35kKS3-zSvGiZszXP9Tb9rNfYzT=+fo8ehk5edk...@mail.gmail.com If the original use case was "don't break stuff", I think patch 0002 solves that, and we don't need this special case in 0001. Would you agree with that statement? Hypothetically, if 0001 (without the special case) along with 0002 were already in 16, and then there was some hypothetical 0003 that introduced the special case to solve the problem described above with the bidirectional trust relationship, I'm not sure I'd be sold on 0003. First, the problem seems fairly minor to me, at least in comparison to the main problem you are solving in this thread. Second, it seems like you could work around it by having two subscriptions. Third, it's a bit unintuitive at least to me: if you introduce a new user Z that can SET ROLE to any of A, B, or C, and then Z reassigns the subscription to themselves, then B's trigger will break because B can't SET ROLE to Z. Others seem to like it, so don't take that as a hard objection. > > But I imagine CREATE SUBSCRIPTION being used either by > superusers or by people who already have those role grants anyway, > because I imagine replication as something that a highly privileged > user configures on behalf of everyone who uses the system. And in > that > case those role grants aren't something new that you do specifically > for logical replication - they're already there because you need them > to administer stuff. Or you're the superuser and don't need them > anyway. Did the discussion drift back towards the SET ROLE in the other direction? I thought we had settled that in v16 we would require that the subscription owner can SET ROLE to the table owner (as in your current 0001), but that we could revisit it later. Regards, Jeff Davis
Re: running logical replication as the subscription owner
On Thu, Mar 30, 2023 at 2:52 PM Jeff Davis wrote: > > Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET > > ROLE to A. With the patch as written, actions on B's tables are not > > confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on > > C's > > tables are. > > It's interesting that it's not transitive, but I'm not sure whether > that's an argument for or against the current approach, or where it > fits (or doesn't fit) with my suggestion. Why do you consider it > important that C's actions are SECURITY_RESTRICTED_OPERATIONs? So that C can't try to hack into A's account. I mean the point here is that B already has permissions to get into A's account whenever they like, without any hacking. So we don't need to impose SECURITY_RESTRICTED_OPERATION when running as B, because the only purpose of SECURITY_RESTRICTED_OPERATION is to prevent the role to which we're switching from attacking the role from which we're switching. And that's how the patch is currently coded. You proposed removing that behavior, on the theory that if the SECURITY_RESTRICTED_OPERATION restrictions were a problem, someone could activate the run_as_owner option (or whatever we end up calling it). But the run_as_owner option applies to the entire subscription. If A activates that option, then B's hypothetical triggers that run afoul of the SECURITY_RESTRICTED_OPERATION restrictions start working again (woohoo!) but they're now vulnerable to attacks from C. With the patch as coded, A doesn't need to use run_as_owner, everything still just works for B, and A is still protected against C. > In version 16, the subscription owner is almost certainly a superuser, > and the table owner almost certainly is not, so there's little chance > that it just happens that the table owner has that privilege already. > > I don't think we want to encourage such grants to proliferate any more > than we want the option you introduce in 0002 to proliferate. Arguably, > it's worse. I don't necessarily find those role grants to be a problem. Obviously it depends on the use case. If you're hoping to be able to set up an account whose only purpose in life is to own subscriptions and which should have as few permissions as possible, then those role grants suck, and a hypothetical future feature where you can GRANT REPLICATION ON TABLE t1 TO subscription_owning_user will be far better. But I imagine CREATE SUBSCRIPTION being used either by superusers or by people who already have those role grants anyway, because I imagine replication as something that a highly privileged user configures on behalf of everyone who uses the system. And in that case those role grants aren't something new that you do specifically for logical replication - they're already there because you need them to administer stuff. Or you're the superuser and don't need them anyway. > And let's say a user says "I upgraded and my trigger broke logical > replication with message about a security-restricted operation... how > do I get up and running again?". With the patches as-written, we will > have two answers to that question: > > * GRANT subscription_owner TO table_owner WITH SET TRUE > * ALTER SUBSCRIPTION ... ($awesome_option_name=false) > > Under what circumstances would we recommend the former vs. the latter? Well, the latter is clearly better because it has such an awesome option name, right? More seriously, my theory is that there's very little use case for having a replication trigger, default expression, etc. that is performing a security restricted operation. And if someone does have a use case, and it's between users that can't already SET ROLE back and forth, then the setup is pretty dubious from a security perspective and maybe the user ought to rethink it. And if they don't want to rethink it, then they need to throw security out the window, and I don't really care which of those commands they use to do it, but the second one would probably break less other stuff for them, so I'd likely recommend that one. > > I don't think run_as_owner is terrible, despite the ambiguity. > > I won't object but I'm not thrilled. Let's see if anyone else weighs in. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Thu, 2023-03-30 at 09:41 -0400, Robert Haas wrote: > On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis wrote: > > I say just take the special case out of 0001. If the trigger > > doesn't > > work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED > > ALWAYS, > > then the user can just use the new option in 0002 to get the old > > behavior. I don't see a reason to implicitly give them the old > > behavior, as 0001 does. > > Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET > ROLE to A. With the patch as written, actions on B's tables are not > confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on > C's > tables are. It's interesting that it's not transitive, but I'm not sure whether that's an argument for or against the current approach, or where it fits (or doesn't fit) with my suggestion. Why do you consider it important that C's actions are SECURITY_RESTRICTED_OPERATIONs? > I think we want to do everything possible to avoid people feeling > like > they need to turn on this new option. I'm not sure we'll ever be able > to get rid of it, but we certainly should avoid doing things that > make > it more likely that it will be needed. I don't think it helps much, though. While I previously said that the special-case behavior is implicit (which is true), it still almost certainly requires a manual step: GRANT subscription_owner TO table_owner WITH SET; In version 16, the subscription owner is almost certainly a superuser, and the table owner almost certainly is not, so there's little chance that it just happens that the table owner has that privilege already. I don't think we want to encourage such grants to proliferate any more than we want the option you introduce in 0002 to proliferate. Arguably, it's worse. And let's say a user says "I upgraded and my trigger broke logical replication with message about a security-restricted operation... how do I get up and running again?". With the patches as-written, we will have two answers to that question: * GRANT subscription_owner TO table_owner WITH SET TRUE * ALTER SUBSCRIPTION ... ($awesome_option_name=false) Under what circumstances would we recommend the former vs. the latter? > > > I agree that the naming is somewhat problematic, but I don't like > trust_table_owners. It's not clear enough about what actually > happens. > I want the name to describe behavior, not sentiment. On reflection, I agree here. We want it to communicate something about the behavior or mechanism. > run_as_subscription_owner removes the ambiguity, but is long. Then fewer people will use it, which might be a good thing. > I can think of other alternatives, like user_switching or > switch_to_table_owner or no_user_switching or various other things, > but none of them seem very good to me. I like the idea of using "switch" (or some synonym) because it's technically more correct. The subscription always runs as the subscription owner; we are just switching temporarily while applying a change. > Another idea could be to make the option non-Boolean. This is > comically long and I can't seriously recommend it, but just to > illustrate the point, if you type CREATE SUBSCRIPTION ... WITH > (execute_code_as_owner_of_which_object = subscription) then you > certainly should know what you've signed up for! If there were a > shorter version that were still clear, I could go for that, but I'm > having trouble coming up with exact wording. I don't care for that -- it communicates the options as co-equal and maybe something that would live forever (or even have more options in the future). I'd prefer that nobody uses the non-switching behavior except for migration purposes or weird use cases we don't really understand. > I don't think run_as_owner is terrible, despite the ambiguity. I won't object but I'm not thrilled. > Regards, Jeff Davis
Re: running logical replication as the subscription owner
On Thu, Mar 30, 2023 at 11:11 AM Jelte Fennema wrote: > Regarding the actual patch. I think the code looks good. Mainly the > tests and docs are lacking for the new option. Like I said for the > tests you can borrow the tests I updated for my v2 patch, I think > those should work fine for the new option. I took a look at that but I didn't really feel like that was quite the direction I wanted to go. I'd actually like to separate the tests of the new option out into their own file, so that if for some reason we decide we want to remove it in the future, it's easier to nuke all the associated tests. Also, quite frankly, I think we've gone way overboard in terms of loading too many tests into a single file, with the result that it's very hard to understand exactly what and all the file is actually testing and what it's intended to be testing. So the attached 0002 does it that way. I've also amended 0001 and 0002 with documentation changes that I hope are appropriate. I noticed along the way that my earlier commits had missed one place that needed to be updated by the pg_create_subscription patch I created earlier. A fix for that is included in 0001, but it can be broken out and committed separately if somebody feels strongly about it. I personally don't think it's worth it. -- Robert Haas EDB: http://www.enterprisedb.com v3-0002-Add-a-run_as_owner-option-to-subscriptions.patch Description: Binary data v3-0001-Perform-logical-replication-actions-as-the-table-.patch Description: Binary data
Re: running logical replication as the subscription owner
On Thu, 30 Mar 2023 at 15:42, Robert Haas wrote: > On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis wrote: > > I say just take the special case out of 0001. If the trigger doesn't > > work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED ALWAYS, > > then the user can just use the new option in 0002 to get the old > > behavior. I don't see a reason to implicitly give them the old > > behavior, as 0001 does. > > Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET > ROLE to A. With the patch as written, actions on B's tables are not > confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on C's > tables are. I think that's fair. There's no need to set SECURITY_RESTRICTED_OPERATION if it doesn't protect you anyway, and indeed it might break things. To be clear I do think it's important to still switch to the table owner, simply for consistency. But that's done in the patch so that's fine. > I agree that the naming is somewhat problematic, but I don't like > trust_table_owners. It's not clear enough about what actually happens. > I want the name to describe behavior, not sentiment. For security related things, I think sentiment is often just as important as the actual behaviour. It's not without reason that newer javascript frameworks have things like dangerouslySetInnerHTML, to scare people away from using it unless they know what they are doing. > I don't think run_as_owner is terrible, despite the ambiguity. It's > talking about the owner of the object on which the property is being > set. Isn't that the most natural interpretation? I'd be pretty > surprised if I set a property called run_as_owner on object A and it > ran as the owner of some other object B. I think that's fair and I'd be happy with run_as_owner. If someone doesn't understand which owner, they should probably check the documentation anyways to understand the implications. Regarding the actual patch. I think the code looks good. Mainly the tests and docs are lacking for the new option. Like I said for the tests you can borrow the tests I updated for my v2 patch, I think those should work fine for the new option. On Thu, 30 Mar 2023 at 15:42, Robert Haas wrote: > > On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis wrote: > > I say just take the special case out of 0001. If the trigger doesn't > > work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED ALWAYS, > > then the user can just use the new option in 0002 to get the old > > behavior. I don't see a reason to implicitly give them the old > > behavior, as 0001 does. > > Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET > ROLE to A. With the patch as written, actions on B's tables are not > confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on C's > tables are. > > I think we want to do everything possible to avoid people feeling like > they need to turn on this new option. I'm not sure we'll ever be able > to get rid of it, but we certainly should avoid doing things that make > it more likely that it will be needed. > > > > 0002 adds a run_as_owner option to subscriptions. This doesn't make > > > the updated regression tests fail, because they don't use it. If you > > > revert the changes to 027_nosuperuser.pl then you get failures (as > > > one > > > would hope) and if you then add WITH (run_as_owner = true) to the > > > CREATE SUBSCRIPTION command in 027_nosuperuser.pl then it passes > > > again. I need to spend some more time thinking about what the tests > > > actually ought to look like if we go this route -- I haven't looked > > > through what Jelte proposed yet -- and also the documentation would > > > need a bunch of updating. > > > > "run_as_owner" is ambiguous -- subscription owner or table owner? > > > > I would prefer something like "trust_table_owners". That would > > communicate that the user shouldn't choose it unless they know what > > they're doing. > > I agree that the naming is somewhat problematic, but I don't like > trust_table_owners. It's not clear enough about what actually happens. > I want the name to describe behavior, not sentiment. > > run_as_subscription_owner removes the ambiguity, but is long. > > run_as_table_owner is a bit shorter, and we could do that with the > sense reversed. Is that equally clear? I'm not sure. > > I can think of other alternatives, like user_switching or > switch_to_table_owner or no_user_switching or various other things, > but none of them seem very good to me. > > Another idea could be to make the option non-Boolean. This is > comically long and I can't seriously recommend it, but just to > illustrate the point, if you type CREATE SUBSCRIPTION ... WITH > (execute_code_as_owner_of_which_object = subscription) then you > certainly should know what you've signed up for! If there were a > shorter version that were still clear, I could go for that, but I'm > having trouble coming up with exact wording. > > I don't think run_as_owner is terrible, despite the ambiguity. It
Re: running logical replication as the subscription owner
On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis wrote: > I say just take the special case out of 0001. If the trigger doesn't > work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED ALWAYS, > then the user can just use the new option in 0002 to get the old > behavior. I don't see a reason to implicitly give them the old > behavior, as 0001 does. Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET ROLE to A. With the patch as written, actions on B's tables are not confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on C's tables are. I think we want to do everything possible to avoid people feeling like they need to turn on this new option. I'm not sure we'll ever be able to get rid of it, but we certainly should avoid doing things that make it more likely that it will be needed. > > 0002 adds a run_as_owner option to subscriptions. This doesn't make > > the updated regression tests fail, because they don't use it. If you > > revert the changes to 027_nosuperuser.pl then you get failures (as > > one > > would hope) and if you then add WITH (run_as_owner = true) to the > > CREATE SUBSCRIPTION command in 027_nosuperuser.pl then it passes > > again. I need to spend some more time thinking about what the tests > > actually ought to look like if we go this route -- I haven't looked > > through what Jelte proposed yet -- and also the documentation would > > need a bunch of updating. > > "run_as_owner" is ambiguous -- subscription owner or table owner? > > I would prefer something like "trust_table_owners". That would > communicate that the user shouldn't choose it unless they know what > they're doing. I agree that the naming is somewhat problematic, but I don't like trust_table_owners. It's not clear enough about what actually happens. I want the name to describe behavior, not sentiment. run_as_subscription_owner removes the ambiguity, but is long. run_as_table_owner is a bit shorter, and we could do that with the sense reversed. Is that equally clear? I'm not sure. I can think of other alternatives, like user_switching or switch_to_table_owner or no_user_switching or various other things, but none of them seem very good to me. Another idea could be to make the option non-Boolean. This is comically long and I can't seriously recommend it, but just to illustrate the point, if you type CREATE SUBSCRIPTION ... WITH (execute_code_as_owner_of_which_object = subscription) then you certainly should know what you've signed up for! If there were a shorter version that were still clear, I could go for that, but I'm having trouble coming up with exact wording. I don't think run_as_owner is terrible, despite the ambiguity. It's talking about the owner of the object on which the property is being set. Isn't that the most natural interpretation? I'd be pretty surprised if I set a property called run_as_owner on object A and it ran as the owner of some other object B. I think our notion of how ambiguous this is may be somewhat inflated by the fact that we've just had a huge conversation about whether it should be the table owner or the subscription owner, so those possibilities are etched in our mind in a way that maybe they won't be for people coming at this fresh. But it's hard to be sure what other people will think about something, and I don't want to be too optimistic about the name I picked, either. > If you are worried that *I* think 0002 would be a poor call, then no, I > don't. Initially I didn't like the idea of supporting two behaviors > (and who would?), but we probably can't avoid it at least for right > now. OK, good. Then we have a way forward that nobody's too upset about. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Wed, 2023-03-29 at 16:00 -0400, Robert Haas wrote: > Here's a new patch set to show how this would work. 0001 is as > before. The following special case ("unless they can SET ROLE..."): /* * Try to prevent the user to which we're switching from assuming the * privileges of the current user, unless they can SET ROLE to that * user anyway. */ which turns SwitchToUntrustedUser() into a no-op, is conceptually redundant with patch 0002. The only difference is that the former (in 0001) is implicit and the latter (in 0002) is declared. I say just take the special case out of 0001. If the trigger doesn't work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED ALWAYS, then the user can just use the new option in 0002 to get the old behavior. I don't see a reason to implicitly give them the old behavior, as 0001 does. > 0002 adds a run_as_owner option to subscriptions. This doesn't make > the updated regression tests fail, because they don't use it. If you > revert the changes to 027_nosuperuser.pl then you get failures (as > one > would hope) and if you then add WITH (run_as_owner = true) to the > CREATE SUBSCRIPTION command in 027_nosuperuser.pl then it passes > again. I need to spend some more time thinking about what the tests > actually ought to look like if we go this route -- I haven't looked > through what Jelte proposed yet -- and also the documentation would > need a bunch of updating. "run_as_owner" is ambiguous -- subscription owner or table owner? I would prefer something like "trust_table_owners". That would communicate that the user shouldn't choose it unless they know what they're doing. > And either way, at least PostgreSQL > hacker who has taken the time to write about this topic is going to > think we've made a poor call, or at best a dubious one, and either > way, If you are worried that *I* think 0002 would be a poor call, then no, I don't. Initially I didn't like the idea of supporting two behaviors (and who would?), but we probably can't avoid it at least for right now. Regards, Jeff Davis
Re: running logical replication as the subscription owner
On Tue, Mar 28, 2023 at 6:48 PM Jeff Davis wrote: > That's not strictly true. See the example at the bottom of this email. Yeah, we're very casual about repeated user switching in all kinds of contexts, and that's really pretty scary. I don't know how to fix that without removing capabilities that people rely on. > The dangerous cases seem to be something along the lines of a security- > invoker trigger function that builds and executes arbirary SQL based on > the row contents. And then the table owner would then still need to set > ENABLE ALWAYS TRIGGER. > > Do we really want to take that case on as our security responsibility? That's something about which I would like to get more opinions. > I believe switching to the table owner (as done in your patch) is the > right best practice, and should be the default. > > I'm fine with an escape hatch here to ease migrations or whatever. But > please do it in an unobtrusive way such that we (as hackers) can mostly > forget that the non-switching behavior exists. At least for me, our > system is already pretty complex without two kinds of subscription > security models. Here's a new patch set to show how this would work. 0001 is as before. 0002 adds a run_as_owner option to subscriptions. This doesn't make the updated regression tests fail, because they don't use it. If you revert the changes to 027_nosuperuser.pl then you get failures (as one would hope) and if you then add WITH (run_as_owner = true) to the CREATE SUBSCRIPTION command in 027_nosuperuser.pl then it passes again. I need to spend some more time thinking about what the tests actually ought to look like if we go this route -- I haven't looked through what Jelte proposed yet -- and also the documentation would need a bunch of updating. Now, backing up a minute to address your other comments here, I'm not particularly dedicated to this new option. But the problem is that you can't please all the people all the time. When I proposed to unconditionally change the behavior, people said "what if SECURITY_RESTRICTED_OPERATION hoses people, or they just don't like the new behavior?" and "what if I don't trust the publisher and want to use table permissions on the subscriber side to minimize my exposure?". Well, this option provides a way out of those problems by allowing you to get the current behavior, but that gives rise to the complaint you raise here: it's nicer to have one behavior than two. To be honest, I'm pretty sympathetic to that complaint: i think the problems if we don't add this switch are probably not going to affect many people at all, and the new switch will therefore be of very little value but will be something we continue to maintain forever. However, I could be wrong and there's certainly something to be said for having escape hatches that let people un-break stuff that core patches break. But you know we can't have it both ways. We either add an escape hatch to make sure that people have a way out if they run into trouble and incur the risk and complexity of carrying it probably forever, OR we suck it up and make it a hard behavior change and tell people who are upset "too bad, we changed it." And either way, at least PostgreSQL hacker who has taken the time to write about this topic is going to think we've made a poor call, or at best a dubious one, and either way, there will probably be at least one user who cusses the PostgreSQL development process out under their breath. I don't know what to do about that. There's not a fix for this gaping security problem that doesn't involve the potential of some people being inconvenienced. -- Robert Haas EDB: http://www.enterprisedb.com v2-0002-Add-a-run_as_owner-option-to-subscriptions.patch Description: Binary data v2-0001-Perform-logical-replication-actions-as-the-table-.patch Description: Binary data
Re: running logical replication as the subscription owner
On Tue, 2023-03-28 at 13:55 -0400, Robert Haas wrote: > Oh, interesting. I hadn't realized we were doing that, and I do think > that narrows the vulnerability quite a bit. It's good to know I wasn't missing something obvious. > bsent logical replication, you don't really need to worry > about whether that function is written securely -- it will run with > privileges of the person performing the DML, and not impact your > account at all. That's not strictly true. See the example at the bottom of this email. The second trigger is SECURITY INVOKER, and it captures the leaked secret number that was stored in the tuple by an earlier SECURITY DEFINER trigger. Perhaps I'm being pedantic, but my point is that SECURITY INVOKER is not a magical shield that protects users from themselves without bound. It protects users against some kinds of attacks, sure, but there's a limit to what it has to offer. > They have reason to be scared of you, but not the > reverse. However, if the people doing DML on the table can arrange to > perform that DML as you, then any security vulnerabilities in your > function potentially allow them to compromise your account. OK, but I'd like to be clear that you've moved from your prior statement: "in theory they might be able to protect themselves, but in practice they are unlikely to be careful enough" To something very different, where it seems that we can't think of a concrete problem even for not-so-careful users. The dangerous cases seem to be something along the lines of a security- invoker trigger function that builds and executes arbirary SQL based on the row contents. And then the table owner would then still need to set ENABLE ALWAYS TRIGGER. Do we really want to take that case on as our security responsibility? > It would also affect someone who uses a default > expression or other means of associating executable code with the > table, and something like a default expression doesn't require any > explicit setting to make it apply in case of replication, so maybe > the > risk of someone messing up is a bit higher. Perhaps, but I still don't understand that case. Unless I'm missing something, the table owner would have to write a pretty weird default expression or check constraint or whatever to end up with something dangerous. > But this definitely makes it more of a judgment call than I thought > initially. And I'm fine if the judgement is that we just require SET ROLE to be conservative and make sure we don't over-promise in 16. That's a totally reasonable thing: it's easier to loosen restrictions later than to tighten them. Furthermore, just because you and I can't think of exploitable problems doesn't mean they don't exist. I have found this discussion enlightening, so thank you for going through these details with me. > I'm still inclined to leave the patch checking for SET ROLE +0.5. I want the patch to go in either way, and can carry on the discussion later and improve things more for 17. But I want to say generally that I believe we spend too much effort trying to protect unreasonable users from themselves, which interferes with our ability to protect reasonable users and solve real use cases. > However, there might be an argument > that we ought to do something else, like have a REPLICATE privilege Sounds cool. Not sure I have an opinion yet, but probably 17 material. > What I would like to change in the > patch in this release is to add a new subscription property along the > lines of what I proposed to Jelte in my earlier email: let's provide > an escape hatch that turns off the user-switching behavior. I believe switching to the table owner (as done in your patch) is the right best practice, and should be the default. I'm fine with an escape hatch here to ease migrations or whatever. But please do it in an unobtrusive way such that we (as hackers) can mostly forget that the non-switching behavior exists. At least for me, our system is already pretty complex without two kinds of subscription security models. Regards, Jeff Davis - example follows -- -- user foo CREATE TABLE secret(I INT); INSERT INTO secret VALUES(42); CREATE TABLE r(i INT, secret INT); CREATE OR REPLACE FUNCTION a_func() RETURNS TRIGGER SECURITY DEFINER LANGUAGE plpgsql AS $$ BEGIN SELECT i INTO NEW.secret FROM secret; RETURN NEW; END; $$; CREATE OR REPLACE FUNCTION z_func() RETURNS TRIGGER SECURITY INVOKER LANGUAGE plpgsql AS $$ BEGIN IF NEW.secret <> abs(NEW.secret) THEN RAISE EXCEPTION 'no negative secret numbers allowed'; END IF; RETURN NEW; END; $$; CREATE TRIGGER a_trigger BEFORE INSERT ON r FOR EACH ROW EXECUTE PROCEDURE a_func(); CREATE TRIGGER z_trigger BEFORE INSERT ON r FOR EACH ROW EXECUTE PROCEDURE z_func(); GRANT INSERT ON r TO bar; GRANT USAGE ON SCHEMA foo TO bar; -- user bar CREATE OR REPLACE FUNCTION bar.abs(i INT4) RETURNS INT4 LA
Re: running logical replication as the subscription owner
On Tue, 28 Mar 2023 at 18:13, Robert Haas wrote: > > On Mon, Mar 27, 2023 at 6:08 PM Jelte Fennema wrote: > > For high privileged subscription owners, > > we downgrade to the permissions of the table owner, but for low > > privileged ones we care about permissions of the subscription owner > > itself. > > Hmm. This is an interesting idea. A variant on this theme could be: > what if we made this an explicit configuration option? Sounds perfect to me. Let's do that. As long as the old no-superuser tests continue to pass when disabling the new switch-to-table-owner behaviour, that sounds totally fine to me. I think it's probably easiest if you use the tests from my v2 patch when you add that option, since that was by far the thing I spent most time on to get right in the v2 patch. On Tue, 28 Mar 2023 at 18:13, Robert Haas wrote: > > On Mon, Mar 27, 2023 at 6:08 PM Jelte Fennema wrote: > > Attached is an updated version of your patch with what I had in mind > > (admittedly it needed one more line than "just" the return to make it > > work). But as you can see all previous tests for a lowly privileged > > subscription owner that **cannot** SET ROLE to the table owner > > continue to work as they did before. While still downgrading to the > > table owners role when the subscription owner **can** SET ROLE to the > > table owner. > > > > Obviously this needs some comments explaining what's going on and > > probably some code refactoring and/or variable renaming, but I hope > > it's clear what I meant now: For high privileged subscription owners, > > we downgrade to the permissions of the table owner, but for low > > privileged ones we care about permissions of the subscription owner > > itself. > > Hmm. This is an interesting idea. A variant on this theme could be: > what if we made this an explicit configuration option? > > I'm worried that if we just try to switch users and silently fall back > to not doing so when we don't have enough permissions, the resulting > behavior is going to be difficult to understand and troubleshoot. I'm > thinking that maybe if you let people pick the behavior they want that > becomes more comprehensible. It's also a nice insurance policy: say > for the sake of argument we make switch-to-table-owner the new > default. If that new behavior causes something to happen to somebody > that they don't like, they can always turn it off, even if they are a > highly privileged user who doesn't "need" to turn it off. > > -- > Robert Haas > EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Tue, Mar 28, 2023 at 1:38 AM Jeff Davis wrote: > > If they do have those things then in > > theory they might be able to protect themselves, but in practice they > > are unlikely to be careful enough. I judge that practically every > > installation where table owners use triggers would be easily > > compromised. Only the most security-conscious of table owners are > > going to get this right. > > This is the interesting part. > > Commit 11da97024ab set the subscription process's search path as empty. > It seems it was done to protect the subscription owner against users > writing into the public schema. But after your apply-as-table-owner > patch, it seems to also offer some protection for table owners against > a malicious subscription owner, too. > > But I must be missing something obvious here -- if the subscription > process has an empty search_path, what can an attacker do to trick it? Oh, interesting. I hadn't realized we were doing that, and I do think that narrows the vulnerability quite a bit. But I still feel pretty uncomfortable with the idea of requiring only INSERT/UPDATE/DELETE permissions on the target table. Let's suppose that you create a table and attach a trigger to it which is SECURITY INVOKER. Absent logical replication, you don't really need to worry about whether that function is written securely -- it will run with privileges of the person performing the DML, and not impact your account at all. They have reason to be scared of you, but not the reverse. However, if the people doing DML on the table can arrange to perform that DML as you, then any security vulnerabilities in your function potentially allow them to compromise your account. Now, there might not be any, but there also might be some, depending on exactly what your function does. And if logical replication into a table requires only I/U/D permission then it's basically a back-door way to run functions that someone could otherwise execute only as themselves as the table owner, and that's scary. Now, I don't know how much to worry about that. As you just pointed out to me in some out-of-band discussion, this is only going to affect a table owner who writes a trigger, makes it insecure in some way other than failure to sanitize the search_path, and sets it ENABLE ALWAYS TRIGGER or ENABLE REPLICA TRIGGER. And maybe we could say that if you do that last part, you kind of need to think about the consequences for logical replication. If so, we could document that problem away. It would also affect someone who uses a default expression or other means of associating executable code with the table, and something like a default expression doesn't require any explicit setting to make it apply in case of replication, so maybe the risk of someone messing up is a bit higher. But this definitely makes it more of a judgment call than I thought initially. Functions that are vulnerable to search_path exploits are a dime a dozen; other kinds of exploits are going to be less common, and more dependent on exactly what the function is doing. I'm still inclined to leave the patch checking for SET ROLE, because after all, we're thinking of switching user identities to the table owner, and checking whether we can SET ROLE is the most natural way of doing that, and definitely doesn't leave room to escalate to any privilege you don't already have. However, there might be an argument that we ought to do something else, like have a REPLICATE privilege on the table that the owner can grant to users that they trust to replicate into that table. Because time is short, I'd like to leave that idea for a future release. What I would like to change in the patch in this release is to add a new subscription property along the lines of what I proposed to Jelte in my earlier email: let's provide an escape hatch that turns off the user-switching behavior. If we do that, then anyone who feels themselves worse off after this patch can switch back to the old behavior. Most people will be better off, I believe, and the opportunity to make things still better in the future is not foreclosed. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Mon, Mar 27, 2023 at 6:08 PM Jelte Fennema wrote: > Attached is an updated version of your patch with what I had in mind > (admittedly it needed one more line than "just" the return to make it > work). But as you can see all previous tests for a lowly privileged > subscription owner that **cannot** SET ROLE to the table owner > continue to work as they did before. While still downgrading to the > table owners role when the subscription owner **can** SET ROLE to the > table owner. > > Obviously this needs some comments explaining what's going on and > probably some code refactoring and/or variable renaming, but I hope > it's clear what I meant now: For high privileged subscription owners, > we downgrade to the permissions of the table owner, but for low > privileged ones we care about permissions of the subscription owner > itself. Hmm. This is an interesting idea. A variant on this theme could be: what if we made this an explicit configuration option? I'm worried that if we just try to switch users and silently fall back to not doing so when we don't have enough permissions, the resulting behavior is going to be difficult to understand and troubleshoot. I'm thinking that maybe if you let people pick the behavior they want that becomes more comprehensible. It's also a nice insurance policy: say for the sake of argument we make switch-to-table-owner the new default. If that new behavior causes something to happen to somebody that they don't like, they can always turn it off, even if they are a highly privileged user who doesn't "need" to turn it off. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Tue, 28 Mar 2023 at 11:15, Jelte Fennema wrote: > Which can currently only be addressed by having 1 > subscription/publication pair for every table owner. Oops, moving sentences around in my email made me not explicitly reference escalation 1 anymore. The above should have been: 1 can currently only be addressed by having 1 subscription/publication pair for every table owner.
Re: running logical replication as the subscription owner
On Mon, 27 Mar 2023 at 22:47, Robert Haas wrote: > Can a table owner defend themselves > against a subscription owner who wants to usurp their privileges? I have a hard time following the discussion. And I think it's because there's lots of different possible privilege escalations to think about. Below is the list of escalations I've gathered and how I think they interact with the different patches: 1. Table owner escalating to a high-privileged subscription owner. i.e. the subscription owner is superuser, or has SET ROLE privileges for all owners of tables in the subscription. 2. Table owner escalating to a low-privileged subscription owner. e.g. the subscription owner can only insert into the tables in its subscription a. The subscription owner only has insert permissions for a tables owned by a single other user b. The subscription owner has insert permissions for tables owned by multiple other users (but still does not have SET ROLE, or possibly even select/update/delete) 3. Publisher applying into tables that the subscriber side doesn't want it to 4. Subscription-owner escalating to table owner a. The subscription owner is highly privileged (allows SET ROLE to table owner) b. The subscription owner is lowly privileged Which can currently only be addressed by having 1 subscription/publication pair for every table owner. This has the big issue that WAL needs to be decoded multiple times on the publisher. This patch would make escalation 1 impossible without having to do anything special when setting up the subscription. With Citus we only run into this escalation issue with logical replication at the moment. We want to replicate lots of different tables, possibly owned by multiple users from one node to another. We trust the publisher and we trust the subscription owner, but we don't trust the table owners at all. This is why I'm very much in favor of a version of this patch. 2.a seems a non-issue, because in this case the table owner can easily give itself the same permissions as the subscription owner (if it didn't have them yet). So by "escalating" to the subscription owner the table owner only gets fewer permissions. 2.b is actually interesting from a security perspective, because by escalating to the subscription owner, the table owner might be able to insert into tables that it normally can't. Patch v1 would "solve" both these issues by simply not supporting these scenarios. My patch v2 keeps the existing behaviour, where both "escalations" are possible and who-ever sets up the replication should create a dedicated subscriber for each table owner to make sure that only 2.a ever occurs and 2.b does not. 3 is something I have not run into. But I can easily imagine a case where a subscriber connects to a (somewhat) untrusted publisher for the purpose of replicating changes from a single table, e.g. some events table. But you don't want to allow replication into any other tables, even if the publisher tells you to. Patch v1 would force you to have SET ROLE privileges on the target events table its owner. So if that owner owns other tables too, then effectively you'd allow the publisher to write into those tables too. The current behaviour (without any patch) supports protecting against this escalation, by giving only INSERT permissions on a single table without the need for SET ROLE. My v2 patch preserves that ability. 4.a again seems like an obvious non-issue to me because the subscription owner can already "escalate" to table owner using SET ROLE. 4.b seems like it's pretty much the same as 3, afaict all the same arguments apply. And I honestly can't think of a real situation where you would not trust the subscription owner (as opposed to the publisher). If any of you have an example of such a situation I'd love to understand this one better. All in all, I think patch v2 is the right direction here, because it covers all these escalations to some extent.
Re: running logical replication as the subscription owner
On Mon, 2023-03-27 at 16:47 -0400, Robert Haas wrote: > No, not really. It's pretty common for a lot of things to be in the > public schema, and the public schema is likely to be in the search > path of every user involved. Consider this trigger function which uses an unqualified reference to pfunc(), therefore implicitly depending on the public schema: CREATE FUNCTION public.pfunc() RETURNS INT4 LANGUAGE plpgsql AS $$ BEGIN RETURN 42; END; $$; CREATE FUNCTION foo.tfunc() RETURNS TRIGGER LANGUAGE plpgsql AS $$ BEGIN NEW.i = pfunc(); RETURN NEW; END; $$; CREATE TABLE foo.a(i INT); CREATE TRIGGER a_trigger BEFORE INSERT ON foo.a FOR EACH ROW EXECUTE PROCEDURE foo.tfunc(); ALTER TABLE foo.a ENABLE ALWAYS TRIGGER a_trigger; But that trigger function is broken today for logical replication -- pfunc() isn't found by the subscription process, which has an empty search_path. > Let's back up a minute here. Suppose someone were to request a new > command, ALTER TABLE DO NOT LET THE SUPERUSER ACCESS THIS. We > would reject that proposal. The reason we would reject it is because > it wouldn't actually work as documented. We know that the superuser > has the power to access that account and reverse that command, either > by SET ROLE or by changing the account password or by changing > pg_hba.conf or by shelling out to the filesystem and doing whatever. > The feature purports to do something that it actually cannot do. No > one can defend themselves against the superuser. Not even another > superuser can defend themselves against a superuser. Pretending > otherwise would be confusing and have no security benefits. Good framing. With you so far... > Now let's think about this case. Can a table owner defend themselves > against a subscription owner who wants to usurp their privileges? If > they cannot, then I think that what the patch does is correct for the > same reason that I think we would correctly reject the hypothetical > command proposed above. Agreed... [ Aside: A user foo who accesses a table owned by bar has no means to defend themselves against SECURITY INVOKER code that usurps their privileges. Applying your logic above, foo should have to grant SET ROLE privileges to bar before accessing the table. ] > If > the table owner has no executable code at all attached to the table - > - > not just triggers, but also expression indexes and default > expressions > and so forth -- then they can. Right... > If they do have those things then in > theory they might be able to protect themselves, but in practice they > are unlikely to be careful enough. I judge that practically every > installation where table owners use triggers would be easily > compromised. Only the most security-conscious of table owners are > going to get this right. This is the interesting part. Commit 11da97024ab set the subscription process's search path as empty. It seems it was done to protect the subscription owner against users writing into the public schema. But after your apply-as-table-owner patch, it seems to also offer some protection for table owners against a malicious subscription owner, too. But I must be missing something obvious here -- if the subscription process has an empty search_path, what can an attacker do to trick it? Regards, Jeff Davis
Re: running logical replication as the subscription owner
> I don't get it. If we just return, that would result in skipping > changes rather than erroring out on changes, but it wouldn't preserve > the current behavior, because we'd still care about the table owner's > permissions rather than, as now, the subscription owner's permissions. Attached is an updated version of your patch with what I had in mind (admittedly it needed one more line than "just" the return to make it work). But as you can see all previous tests for a lowly privileged subscription owner that **cannot** SET ROLE to the table owner continue to work as they did before. While still downgrading to the table owners role when the subscription owner **can** SET ROLE to the table owner. Obviously this needs some comments explaining what's going on and probably some code refactoring and/or variable renaming, but I hope it's clear what I meant now: For high privileged subscription owners, we downgrade to the permissions of the table owner, but for low privileged ones we care about permissions of the subscription owner itself. v2-0001-Perform-logical-replication-actions-as-the-table-.patch Description: Binary data
Re: running logical replication as the subscription owner
On Mon, Mar 27, 2023 at 3:04 PM Jeff Davis wrote: > On Mon, 2023-03-27 at 12:53 -0400, Robert Haas wrote: > > We do know that an empty search_path is secure, > > but it's probably also going to cause any code we run to fail, unless > > that code schema-qualifies all references outside of pg_catalog, or > > unless it sets search_path itself. > > I am confused. > > If the trigger function requires schema "xyz" to be in the search_path, > and the function itself doesn't set it, how will it ever get set in the > subscription process? Won't such a function be simply broken for all > logical subscriptions (in current code or with any of the proposals > active right now)? > > And if the trigger function requires object "abc" (regardless of > schema) to be somehow accessible without qualification, and if it > doesn't set the search path itself, and it's not in pg_catalog; then > again I think that function would be broken today. No, not really. It's pretty common for a lot of things to be in the public schema, and the public schema is likely to be in the search path of every user involved. > It feels like we are reaching to say that a trigger function might be > broken based on some contrived cases, but we can already contrive cases > that are broken for logical replication today. It might not be exactly > the same set, but unless I'm missing something it would be a very > similar set. No, it's not a contrived case, and it's not the same set of cases, not even close. Running functions with a different search path than the author intended is a really common cause of all kinds of things breaking. See for example commit 582edc369cdbd348d68441fc50fa26a84afd0c1a, which certainly did break things for some users. > Are you suggesting that we require the subscription owner to have SET > ROLE on everybody so that everyone is equally insecure and there's no > way to ever fix it even if you don't have any triggers on your tables > at all? I certainly am not. I don't even know how to respond to this. I want to make it secure, not insecure. But we don't gain any security by pretending that certain exploits don't exist or aren't problems when they do and are. Quite the contrary. > And all of this is to protect a user that writes a trigger function > that executes arbitrary SQL based on the input data? Or is insecure in any other way, and there are quite a few ways. If you don't think that this is a problem in reality, I really don't know how to carry this conversation forward. The idea that the average trigger function is safe if it can be unexpectedly called by someone other than the table owner with arguments and GUC settings chosen by the caller doesn't seem remotely correct to me. It matches no part of my experience with user-defined functions, either written by me or by EDB customers. Every database I've ever seen that used triggers at all would be vulnerable to the subscription owner compromising the table owner's account. > > Well, I continue to feel that if you can compromise the subscription > > owner's account, we haven't really fixed anything yet. > > I'm trying to reconcile the following two points: > > A. That you are proposing a patch to allow non-superuser > subscriptions; and > B. That you are arguing that the subscription owner basically needs > to be superuser and we should just accept that. > > Granted, there's some nuance here and I won't say that those two are > mutually exclusive. But I think it would be helpful to explain how > those two ideas fit together. I do think that what this patch does is tantamount to B, because you can have SET ROLE to some users without having SET ROLE to all users. That's a big part of the point of the CREATEROLE and createrole_self_grant work. > But I think you're saying something slightly different about the > subscription process compromising the table owner, and assuming that > problem exists, your patch doesn't do anything to stop it. In fact, > your patch requires the subscription owner can SET ROLE to each of the > table owners, guaranteeing that the table owners can never do anything > at all to protect themselves from the subscription owner. Yeah, that's true, and like I said, if there's a way to avoid that, great. But wishing it were so is not that way. Let's back up a minute here. Suppose someone were to request a new command, ALTER TABLE DO NOT LET THE SUPERUSER ACCESS THIS. We would reject that proposal. The reason we would reject it is because it wouldn't actually work as documented. We know that the superuser has the power to access that account and reverse that command, either by SET ROLE or by changing the account password or by changing pg_hba.conf or by shelling out to the filesystem and doing whatever. The feature purports to do something that it actually cannot do. No one can defend themselves against the superuser. Not even another superuser can defend themselves against a superuser. Pretending otherwise would be confusing and have no se
Re: running logical replication as the subscription owner
On Mon, 2023-03-27 at 12:53 -0400, Robert Haas wrote: > We do know that an empty search_path is secure, > but it's probably also going to cause any code we run to fail, unless > that code schema-qualifies all references outside of pg_catalog, or > unless it sets search_path itself. I am confused. If the trigger function requires schema "xyz" to be in the search_path, and the function itself doesn't set it, how will it ever get set in the subscription process? Won't such a function be simply broken for all logical subscriptions (in current code or with any of the proposals active right now)? And if the trigger function requires object "abc" (regardless of schema) to be somehow accessible without qualification, and if it doesn't set the search path itself, and it's not in pg_catalog; then again I think that function would be broken today. It feels like we are reaching to say that a trigger function might be broken based on some contrived cases, but we can already contrive cases that are broken for logical replication today. It might not be exactly the same set, but unless I'm missing something it would be a very similar set. > search_path also isn't necessarily > the only problem. As a hypothetical example, suppose I create a table > with one text column, revoke all public access to that table, and > then > create an on-insert trigger that executes as an SQL command any text > value inserted into the table. This is perfectly secure as long as > I'm > the only one who can access the table, but if someone else gets > access > to insert things into that table using my credentials then they can > easily break into my account. Real examples aren't necessarily that > dramatically bad, but that doesn't mean they don't exist. Are you suggesting that we require the subscription owner to have SET ROLE on everybody so that everyone is equally insecure and there's no way to ever fix it even if you don't have any triggers on your tables at all? And all of this is to protect a user that writes a trigger function that executes arbitrary SQL based on the input data? > > Well, I continue to feel that if you can compromise the subscription > owner's account, we haven't really fixed anything yet. I'm trying to reconcile the following two points: A. That you are proposing a patch to allow non-superuser subscriptions; and B. That you are arguing that the subscription owner basically needs to be superuser and we should just accept that. Granted, there's some nuance here and I won't say that those two are mutually exclusive. But I think it would be helpful to explain how those two ideas fit together. > We just ... fixed it so that no > compromise was possible. And I think that's also the right approach > here. If you are saying that we fix the subscription process so that it can't easily be compromised by the table owner, then that appears to be the entire purpose of this patch and I agree. But I think you're saying something slightly different about the subscription process compromising the table owner, and assuming that problem exists, your patch doesn't do anything to stop it. In fact, your patch requires the subscription owner can SET ROLE to each of the table owners, guaranteeing that the table owners can never do anything at all to protect themselves from the subscription owner. > I do agree with you that allowing SET ROLE on tons of accounts is not > great, though. Regardless of security concerns, the UI is bad. Regards, Jeff Davis
Re: running logical replication as the subscription owner
On Sat, Mar 25, 2023 at 5:24 AM Jelte Fennema wrote: > For my purposes I always trust the publisher, what I don't trust is > the table owners. But I can indeed imagine scenarios where that's the > other way around, and indeed you can protect against that currently, > but not with your new patch. That seems fairly easily solvable though. > > + if (!member_can_set_role(context->save_userid, userid)) > + ereport(ERROR, > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > +errmsg("role \"%s\" cannot SET ROLE to \"%s\"", > + GetUserNameFromId(context->save_userid, false), > + GetUserNameFromId(userid, false; > > If we don't throw an error here, but instead simply return, then the > current behaviour is preserved and people can manually configure > permissions to protect against an untrusted publisher. This would > still mean that the table owners can escalate privileges to the > subscription owner, but if that subscription owner actually has fewer > privileges than the table owner then you don't have that issue. I don't get it. If we just return, that would result in skipping changes rather than erroring out on changes, but it wouldn't preserve the current behavior, because we'd still care about the table owner's permissions rather than, as now, the subscription owner's permissions. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Sat, Mar 25, 2023 at 12:01 PM Noah Misch wrote: > (One might allow temp > tables by introducing NewTempSchemaNestLevel(), called whenever we call > NewGUCNestLevel(). The transaction would then proceed as though it has no > temp schema, allocating an additional schema if creating a temp object.) Neat idea. That would require some adjustments, I believe, because of the way that temp schemas are named. But it sounds doable. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Fri, Mar 24, 2023 at 8:02 PM Jeff Davis wrote: > Without a reasonable example, we should probably be on some kind of > path to disallowing crazy stuff in triggers that poses only risks and > no benefits. Not the job of this patch, but perhaps it can be seen as a > step in that direction? Possibly, but it's a little harder to say what's crazy in a trigger than in some other contexts. I feel like it would be fine to say that your index expression should probably not be doing "ALTER USER somebody SUPERUSER" really ever, but it's not quite so clear in case of a trigger. And the stuff that's prevented by SECURITY_RESTRICTED_OPERATION isn't that sort of thing, but has rather to do with stuff that messes with the session state, maybe leaving booby-traps behind for the next user. For instance, if user A runs some code as user B and user B closes a cursor opened by A and opens a new one with the same name, that has a rather good chance of making A do something they didn't intend to do. SECURITY_RESTRICTED_OPERATION is aimed at preventing that sort of attack. > > I am not thrilled with this either, but if you can arrange to run > > code > > as a certain user without the ability to SET ROLE to that user then > > there is, by definition, a potential privilege escalation. > > I don't think that's "by definition". > > The code is being executed with the privileges of the person who wrote > it. I don't see anything inherently insecure about that. There could be > incidental or practical risks, but it's a pretty reasonable thing to do > at a high level. Not really. My home directory on my laptop is full of Perl and sh scripts that I wouldn't want someone else to execute as me. They don't have any defenses against malicious use because I don't expect anyone else has access to run them, especially as me. If someone got access to run them as me, they'd compromise my laptop account in no time at all. I don't see any reason to believe the situation is any different inside of a database. People have no reason to harden code unless someone else is going to have access to run it. > > I don't > > think we can just dismiss that as a non-issue. If the ability of one > > user to potentially become some other user weren't a problem, we > > wouldn't need any patch at all. Imagine for example that the table > > owner has a trigger which doesn't sanitize search_path. The > > subscription owner can potentially leverage that to get the table > > owner's privileges. > > Can you explain? Couldn't we control the subscription process's search > path? There's no place in the code right now where when we switch user identities we also change the search_path. There is nothing to prevent us from writing such code, but we have no place from which to obtain a search_path that will cause the called code to behave properly. We don't have access to the search_path that would prevail at the time the target user logged in, and even if we did, we don't know that that search_path is secure. We do know that an empty search_path is secure, but it's probably also going to cause any code we run to fail, unless that code schema-qualifies all references outside of pg_catalog, or unless it sets search_path itself. search_path also isn't necessarily the only problem. As a hypothetical example, suppose I create a table with one text column, revoke all public access to that table, and then create an on-insert trigger that executes as an SQL command any text value inserted into the table. This is perfectly secure as long as I'm the only one who can access the table, but if someone else gets access to insert things into that table using my credentials then they can easily break into my account. Real examples aren't necessarily that dramatically bad, but that doesn't mean they don't exist. > The benefit of delegating to a non-superuser is to contain the risk if > that account is compromised. Allowing SET ROLE on tons of accounts > diminishes that benefit, a lot. Well, I continue to feel that if you can compromise the subscription owner's account, we haven't really fixed anything yet. I mean, it used to be that autovacuum could compromise the superuser's account, and we fixed that. If we find more ways for that same thing to happen, we would presumably fix those too. We would never accept a situation where autovacuum can compromise the superuser's account. And we shouldn't accept a situation where either the table owner can compromise the subscription owner's account, either. And similarly nobody ever proposed that that issue should be fixed by running the autovacuum worker process as some kind of low-privileged user that we created specially for that purpose. We just ... fixed it so that no compromise was possible. And I think that's also the right approach here. I do agree with you that allowing SET ROLE on tons of accounts is not great, though. I don't really think it matters very much today, because basically all subscriptions today are owned by superus
Re: running logical replication as the subscription owner
On Fri, Mar 24, 2023 at 4:11 PM Mark Dilger wrote: > > > Imagine for example that the table > > > owner has a trigger which doesn't sanitize search_path. The > > > subscription owner can potentially leverage that to get the table > > > owner's privileges. > > I don't find that terribly convincing. First, there's no reason a > subscription owner should be an ordinary user able to volitionally do > anything. The subscription owner should just be a role that the subscription > runs under, as a means of superuser dropping privileges before applying > changes. So the only real problem would be that the changes coming from the > publisher might, upon application, hack the table owner. But if that's the > case, the table owner's vulnerability on the subscription-database side is > equal to their vulnerability on the publication-database side (assuming equal > schemas on both). Flagging this vulnerability as being logical replication > related seems a category error. Instead, it's a schema vulnerability. I think there are actually a number of reasons why the subscription owner should be a regular user account rather than a special low-privilege account. First, it's only barely possible to do anything else. As of today, you can't create a subscription owned by a non-superuser except by making the subscription first and then removing superuser from the account. You can't even do this: rhaas=# alter subscription s1 owner to alice; ERROR: permission denied to change owner of subscription "s1" HINT: The owner of a subscription must be a superuser. That hint is actually a lie because of the loophole mentioned above, but even if making the subscription owner a low-privilege account were the right model (which I don't believe) we've got error messages in the current source code saying that you're not allowed to even do it. Second, even if this kind of setup were fully supported and all the stuff worked as you expect, it's not very convenient. It requires you to create this extra dummy account that doesn't otherwise need to exist. I don't see a good reason to impose that requirement on everybody. Given that subscriptions initially could only be owned by superusers, and that's still mostly true, it seems to me that the feature was intended to be used with the superuser as the subscription owner, and I think that's what most people must be doing now and will probably want to continue to do, and we should try to make it safe instead of back-pedaling and saying, hey, do it this totally other way instead. More discussion of problems below. > > So I think that if we allow user A to replicate into user B's > > table with fewer privileges than A-can-set-role-to-B, we're building a > > privilege-escalation attack into the system. But if we do require > > A-can-set-role-to-B, then things change as described above. > > I don't understand the direction this patch is going. I'm emphatically not > objecting to it, merely expressing my confusion about it. > > I had imagined the solution to the replication security problem was to stop > running the replication as superuser, and instead as a trivial user. Imagine > that superuser creates roles "deadhead_bob" and "deadhead_alice" which cannot > log in, are not members of any groups nor have any other roles as members of > themselves, and have no privileges beyond begin able to replicate into bob's > and alice's tables, respectively. The superuser sets up the subscriptions > disabled, transfers ownership to deadhead_bob and deadhead_alice, and only > then enables the subscriptions. > > Since deadhead_bob and deadhead_alice cannot log in, and nobody can set role > to them, I don't see what the vulnerability is. Sure, maybe alice can attack > deadhead_alice, or bob can attack deadhead_bob, but that's more of a > privilege deescalation than a privilege escalation, so where's the risk? > That's not a rhetorical question. Is there a risk here? Or are we just > concerned that most users will set up replication with superuser or some > other high-privilege user, and we're trying to protect them from the > consequences of that choice? Having a separate subscription for each different table owner is extremely undesirable from a performance perspective, because it means that the WAL on the origin server has to be decoded once per table owner. That's not very much fun even if the number of table owners is only two, as in your example, and there could be many more than two users who own tables. In addition, it breaks the transactional consistency of replication. If there's any single transaction that modifies both a table owned by alice and a table owned by bob, we lose transactional consistency on the subscriber side unless both of those transactions are replicated in a single transaction, which means they also need to be part of a single subscription. I admit that hacking into deadhead_alice or deadhead_bob is probably only minimally interesting. There a
Re: running logical replication as the subscription owner
On Fri, Mar 24, 2023 at 10:00:36AM -0400, Robert Haas wrote: > On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis wrote: > > That's a little confusing, why not just always use the > > SECURITY_RESTRICTED_OPERATION? Is there a use case I'm missing? > > Some concern was expressed -- not sure exactly where the email is > exactly, and it might've been on pgsql-security -- that doing that > categorically might break things that are currently working. The > people who were concerned included Andres and I forget who else. My SECURITY_RESTRICTED_OPERATION blocks deferred triggers and CREATE TEMP TABLE. If you create a DEFERRABLE INITIALLY DEFERRED fk constraint and replicate to the constraint's table in a SECURITY_RESTRICTED_OPERATION, I expect an error. > gut reaction was the same as yours, just do it always and don't worry > about it. But if people think that users are likely to run afoul of Hard to know. It's the sort of thing that I model as creating months of work for epsilon users to adapt their applications. Epsilon could be zero. Most users don't notice. > the SECURITY_RESTRICTED_OPERATION restrictions in practice, then this > is better, and the implementation complexity isn't high. We could even > think of extending this kind of logic to other places where > SECURITY_RESTRICTED_OPERATION is enforced, if desired. Firing a trigger in an index expression or materialized view query is not reasonable, so today's uses of SECURITY_RESTRICTED_OPERATION would not benefit from this approach. Being able to create temp tables in those places has some value, but I would allow temp tables in SECURITY_RESTRICTED_OPERATION instead of proliferating the check on the ability to SET ROLE. (One might allow temp tables by introducing NewTempSchemaNestLevel(), called whenever we call NewGUCNestLevel(). The transaction would then proceed as though it has no temp schema, allocating an additional schema if creating a temp object.)
Re: running logical replication as the subscription owner
> As things stand today, if you think somebody's going to send you > changes to tables other than the ones you intend to replicate, you > could handle that by making sure that the user that owns the > subscription only has permission to write to the tables that are > expected to receive replicated data. It's a bit awkward to set up > because you have to initially make the subscription owner a superuser > and then later remove the superuser bit, so I think this is another > argument for the pg_create_subscription patch posted elsewhere, but if > you're a superuser yourself, you can do it. However, with this patch, > that wouldn't work any more, because the permissions checks don't > happen until after we've switched to the target role. For my purposes I always trust the publisher, what I don't trust is the table owners. But I can indeed imagine scenarios where that's the other way around, and indeed you can protect against that currently, but not with your new patch. That seems fairly easily solvable though. + if (!member_can_set_role(context->save_userid, userid)) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), +errmsg("role \"%s\" cannot SET ROLE to \"%s\"", + GetUserNameFromId(context->save_userid, false), + GetUserNameFromId(userid, false; If we don't throw an error here, but instead simply return, then the current behaviour is preserved and people can manually configure permissions to protect against an untrusted publisher. This would still mean that the table owners can escalate privileges to the subscription owner, but if that subscription owner actually has fewer privileges than the table owner then you don't have that issue.
Re: running logical replication as the subscription owner
On Fri, 2023-03-24 at 14:35 -0400, Robert Haas wrote: > That > actually wouldn't work today, and with the patch it would start > working, so basically the effect of the patch on the problem that you > mention would be to remove the ability to filter by specific table > and > add the ability to filter by owning role. That's certainly a loss. It makes a lot of sense to want to limit a subscription to only write to certain tables. If you want to filter by role, you can do that today by granting the role, or some role that has the necessary privileges. It takes me a while to re-learn the problems of poorly-written trigger functions, malicious trigger functions, search paths, etc., each time I start working in this area again. Can you include an example of such a trigger function that we're worried about? Can the subscription owner change the search path in the subscription process, and if so, why? The doc here: https://www.postgresql.org/docs/devel/sql-createfunction.html#SQL-CREATEFUNCTION-SECURITY mentions search_path, but other hazards don't really seem applicable. (Is the trigger creating new roles?) Regards, Jeff Davis
Re: running logical replication as the subscription owner
On Fri, 2023-03-24 at 10:00 -0400, Robert Haas wrote: > On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis wrote: > > That's a little confusing, why not just always use the > > SECURITY_RESTRICTED_OPERATION? Is there a use case I'm missing? > > Some concern was expressed -- not sure exactly where the email is > exactly, and it might've been on pgsql-security -- that doing that > categorically might break things that are currently working. The > people who were concerned included Andres and I forget who else. My > gut reaction was the same as yours, just do it always and don't worry > about it. But if people think that users are likely to run afoul of > the SECURITY_RESTRICTED_OPERATION restrictions in practice, then this > is better, and the implementation complexity isn't high. We could > even > think of extending this kind of logic to other places where > SECURITY_RESTRICTED_OPERATION is enforced, if desired. Without a reasonable example, we should probably be on some kind of path to disallowing crazy stuff in triggers that poses only risks and no benefits. Not the job of this patch, but perhaps it can be seen as a step in that direction? > > I am not thrilled with this either, but if you can arrange to run > code > as a certain user without the ability to SET ROLE to that user then > there is, by definition, a potential privilege escalation. I don't think that's "by definition". The code is being executed with the privileges of the person who wrote it. I don't see anything inherently insecure about that. There could be incidental or practical risks, but it's a pretty reasonable thing to do at a high level. > I don't > think we can just dismiss that as a non-issue. If the ability of one > user to potentially become some other user weren't a problem, we > wouldn't need any patch at all. Imagine for example that the table > owner has a trigger which doesn't sanitize search_path. The > subscription owner can potentially leverage that to get the table > owner's privileges. Can you explain? Couldn't we control the subscription process's search path? > In cases where the subscription owner isn't the > superuser, I think the next most likely possibility is that the > subscription owner is some kind of almost-super-user The benefit of delegating to a non-superuser is to contain the risk if that account is compromised. Allowing SET ROLE on tons of accounts diminishes that benefit, a lot. Regards, Jeff Davis
Re: running logical replication as the subscription owner
> On Mar 24, 2023, at 11:35 AM, Robert Haas wrote: > > I don't know how bad that sounds to you, and if it does sound bad, I > don't immediately see how to mitigate it. As I said to Jeff, if you > can replicate into a table that has a casually-written SECURITY > INVOKER trigger on it, you can probably hack into the table owner's > account. I assume you mean this bit: > > Imagine for example that the table > > owner has a trigger which doesn't sanitize search_path. The > > subscription owner can potentially leverage that to get the table > > owner's privileges. I don't find that terribly convincing. First, there's no reason a subscription owner should be an ordinary user able to volitionally do anything. The subscription owner should just be a role that the subscription runs under, as a means of superuser dropping privileges before applying changes. So the only real problem would be that the changes coming from the publisher might, upon application, hack the table owner. But if that's the case, the table owner's vulnerability on the subscription-database side is equal to their vulnerability on the publication-database side (assuming equal schemas on both). Flagging this vulnerability as being logical replication related seems a category error. Instead, it's a schema vulnerability. > So I think that if we allow user A to replicate into user B's > table with fewer privileges than A-can-set-role-to-B, we're building a > privilege-escalation attack into the system. But if we do require > A-can-set-role-to-B, then things change as described above. I don't understand the direction this patch is going. I'm emphatically not objecting to it, merely expressing my confusion about it. I had imagined the solution to the replication security problem was to stop running the replication as superuser, and instead as a trivial user. Imagine that superuser creates roles "deadhead_bob" and "deadhead_alice" which cannot log in, are not members of any groups nor have any other roles as members of themselves, and have no privileges beyond begin able to replicate into bob's and alice's tables, respectively. The superuser sets up the subscriptions disabled, transfers ownership to deadhead_bob and deadhead_alice, and only then enables the subscriptions. Since deadhead_bob and deadhead_alice cannot log in, and nobody can set role to them, I don't see what the vulnerability is. Sure, maybe alice can attack deadhead_alice, or bob can attack deadhead_bob, but that's more of a privilege deescalation than a privilege escalation, so where's the risk? That's not a rhetorical question. Is there a risk here? Or are we just concerned that most users will set up replication with superuser or some other high-privilege user, and we're trying to protect them from the consequences of that choice? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: running logical replication as the subscription owner
On Fri, Mar 24, 2023 at 2:14 PM Jelte Fennema wrote: > Yes, I totally agree. I now realise that wasn't clear at all from the wording > in my previous email. I'm fine with both behaviours. I mainly meant that if > we actually think the new behaviour is better (which honestly I'm not > convinced of yet), then some follow up patch would probably be good. I > definitely don't want to block this patch on any of that though. Both > behaviors would be vastly better than the current one in my opinion. So if > others wanted the behaviour in your patch, I'm completely fine with that. Makes sense. I hope a few more people will comment on what they think we should do here, especially Andres and Noah. > > Yeah, if we stick with the current approach we should probably add > > tests for that stuff. > > Even if we don't, we should still have tests showing that the security > restrictions that we intend to put in place actually do their job. Yeah, I just don't want to write the tests and then decide to change the behavior and then have to write them over again. It's not so much fun that I'm yearning to do it twice. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Fri, Mar 24, 2023 at 12:58 PM Mark Dilger wrote: > I also think the subscription owner should be a low-privileged user, owing to > the risk of the publisher injecting malicious content into the publication. > I think you are focused on all the bad actors on the subscription-side > database and what they can do to each other. That's also valid, but I get > the impression that you're losing sight of the risk posed by malicious > publishers. Or maybe you aren't, and can explain? You have a point. As things stand today, if you think somebody's going to send you changes to tables other than the ones you intend to replicate, you could handle that by making sure that the user that owns the subscription only has permission to write to the tables that are expected to receive replicated data. It's a bit awkward to set up because you have to initially make the subscription owner a superuser and then later remove the superuser bit, so I think this is another argument for the pg_create_subscription patch posted elsewhere, but if you're a superuser yourself, you can do it. However, with this patch, that wouldn't work any more, because the permissions checks don't happen until after we've switched to the target role. You could alternatively set up a user to own the subscription who has the ability to SET ROLE to some users and not others, but that only lets you restrict replication based on which user owns the tables, rather than which specific tables are getting data replicated into them. That actually wouldn't work today, and with the patch it would start working, so basically the effect of the patch on the problem that you mention would be to remove the ability to filter by specific table and add the ability to filter by owning role. I don't know how bad that sounds to you, and if it does sound bad, I don't immediately see how to mitigate it. As I said to Jeff, if you can replicate into a table that has a casually-written SECURITY INVOKER trigger on it, you can probably hack into the table owner's account. So I think that if we allow user A to replicate into user B's table with fewer privileges than A-can-set-role-to-B, we're building a privilege-escalation attack into the system. But if we do require A-can-set-role-to-B, then things change as described above. I suppose in theory we could check both A-can-set-role-to-B and A-can-modify-this-table-as-A, but that feels pretty unprincipled. I can't particularly see why A should need permission to perform an action that is actually going to be performed as B; what makes sense is to check that A can become B, and that B has permission to perform the action, which is the state that this patch would create. I guess there are other things we could do, too, like add replication restriction or filtering capabilities specifically to address this problem, or devise some other kind of new kind of permission system around this, but I don't have a specific idea what that would look like. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
> I don't think it makes sense for this patch to run around and try to > adjust all of those other pages. We have to pick between doing it this > way (and thus being inconsistent with what we do elsewhere) or taking > that logic out (and taking our chances that something will break for > some users). I'm OK with either of those, but I'm not OK with going > and changing the way this works in all of the other cases first and > only then coming back to this problem. This problem is WAY more > important than fiddling with the details of how > SECURITY_RESTRICTED_OPERATION is applied. Yes, I totally agree. I now realise that wasn't clear at all from the wording in my previous email. I'm fine with both behaviours. I mainly meant that if we actually think the new behaviour is better (which honestly I'm not convinced of yet), then some follow up patch would probably be good. I definitely don't want to block this patch on any of that though. Both behaviors would be vastly better than the current one in my opinion. So if others wanted the behaviour in your patch, I'm completely fine with that. > Yeah, if we stick with the current approach we should probably add > tests for that stuff. Even if we don't, we should still have tests showing that the security restrictions that we intend to put in place actually do their job.
Re: running logical replication as the subscription owner
> On Mar 24, 2023, at 7:00 AM, Robert Haas wrote: > > More generally, Stephen Frost has elsewhere argued that we should want > the subscription owner to be a very low-privilege user, so that if > their privileges get stolen, it's no big deal. I disagree with that. I > think it's always a problem if one user can get unauthorized access to > another user's account, regardless of exactly what those accounts can > do. I think our goal should be to make it safe for the subscription > owner to be a very high-privilege user, because you're going to need > to be a very high-privilege user to set up replication. And if you do > have that level of privilege, it's more convenient and simpler if you > can just own the subscription yourself, rather than having to make a > dummy account to own it. To put that another way, I think that what > people are going to want to do in a lot of cases is have the superuser > own the subscription, so I think we need to make that case safe, > whatever it takes. I also think the subscription owner should be a low-privileged user, owing to the risk of the publisher injecting malicious content into the publication. I think you are focused on all the bad actors on the subscription-side database and what they can do to each other. That's also valid, but I get the impression that you're losing sight of the risk posed by malicious publishers. Or maybe you aren't, and can explain? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: running logical replication as the subscription owner
On Fri, Mar 24, 2023 at 12:17 PM Jelte Fennema wrote: > I personally cannot think of a reasonable example that would be > broken, but I agree the code is simple enough. I do think that if > there is an actual reason to do this, we'd probably want it in other > places where SECURITY_RESTRICTED_OPERATION is enforced too. I don't think it makes sense for this patch to run around and try to adjust all of those other pages. We have to pick between doing it this way (and thus being inconsistent with what we do elsewhere) or taking that logic out (and taking our chances that something will break for some users). I'm OK with either of those, but I'm not OK with going and changing the way this works in all of the other cases first and only then coming back to this problem. This problem is WAY more important than fiddling with the details of how SECURITY_RESTRICTED_OPERATION is applied. > I think there's some important tests missing related to this: > 1. Ensuring that SECURITY_RESTRICTED_OPERATION things are enforced > when the user **does not** have SET ROLE permissions to the > subscription owner, e.g. don't allow SET ROLE from a trigger. > 2. Ensuring that SECURITY_RESTRICTED_OPERATION things are not enforced > when the user **does** have SET ROLE permissions to the subscription > owner, e.g. allows SET ROLE from trigger. Yeah, if we stick with the current approach we should probably add tests for that stuff. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
> Some concern was expressed -- not sure exactly where the email is > exactly, and it might've been on pgsql-security -- that doing that > categorically might break things that are currently working. The > people who were concerned included Andres and I forget who else. My > gut reaction was the same as yours, just do it always and don't worry > about it. But if people think that users are likely to run afoul of > the SECURITY_RESTRICTED_OPERATION restrictions in practice, then this > is better, and the implementation complexity isn't high. We could even > think of extending this kind of logic to other places where > SECURITY_RESTRICTED_OPERATION is enforced, if desired. I personally cannot think of a reasonable example that would be broken, but I agree the code is simple enough. I do think that if there is an actual reason to do this, we'd probably want it in other places where SECURITY_RESTRICTED_OPERATION is enforced too. I think there's some important tests missing related to this: 1. Ensuring that SECURITY_RESTRICTED_OPERATION things are enforced when the user **does not** have SET ROLE permissions to the subscription owner, e.g. don't allow SET ROLE from a trigger. 2. Ensuring that SECURITY_RESTRICTED_OPERATION things are not enforced when the user **does** have SET ROLE permissions to the subscription owner, e.g. allows SET ROLE from trigger. Finally a small typo in the one of the comments: > + * If we created a new GUC nest level, also role back any changes that were s/role/roll/
Re: running logical replication as the subscription owner
On Fri, Mar 24, 2023 at 3:59 AM Jeff Davis wrote: > That's a little confusing, why not just always use the > SECURITY_RESTRICTED_OPERATION? Is there a use case I'm missing? Some concern was expressed -- not sure exactly where the email is exactly, and it might've been on pgsql-security -- that doing that categorically might break things that are currently working. The people who were concerned included Andres and I forget who else. My gut reaction was the same as yours, just do it always and don't worry about it. But if people think that users are likely to run afoul of the SECURITY_RESTRICTED_OPERATION restrictions in practice, then this is better, and the implementation complexity isn't high. We could even think of extending this kind of logic to other places where SECURITY_RESTRICTED_OPERATION is enforced, if desired. > It would be really nice if this could be done with some kind of > ordinary privilege rather than requiring SET ROLE. A user might expect > that INSERT/UPDATE/DELETE/TRUNCATE privileges are enough. Or > pg_write_all_data. > > I can see theoretically that a table owner might write some dangerous > code and attach it to their table. But I don't quite understand why > they would do that. If the code was vulnerable to attack, would that > mean that they couldn't even insert into their own table safely? > > Requiring SET ROLE seems like it makes the subscription role into > something very close to a superuser. And that takes away some of the > benefits of delegating to non-superusers. I am not thrilled with this either, but if you can arrange to run code as a certain user without the ability to SET ROLE to that user then there is, by definition, a potential privilege escalation. I don't think we can just dismiss that as a non-issue. If the ability of one user to potentially become some other user weren't a problem, we wouldn't need any patch at all. Imagine for example that the table owner has a trigger which doesn't sanitize search_path. The subscription owner can potentially leverage that to get the table owner's privileges. More generally, Stephen Frost has elsewhere argued that we should want the subscription owner to be a very low-privilege user, so that if their privileges get stolen, it's no big deal. I disagree with that. I think it's always a problem if one user can get unauthorized access to another user's account, regardless of exactly what those accounts can do. I think our goal should be to make it safe for the subscription owner to be a very high-privilege user, because you're going to need to be a very high-privilege user to set up replication. And if you do have that level of privilege, it's more convenient and simpler if you can just own the subscription yourself, rather than having to make a dummy account to own it. To put that another way, I think that what people are going to want to do in a lot of cases is have the superuser own the subscription, so I think we need to make that case safe, whatever it takes. In cases where the subscription owner isn't the superuser, I think the next most likely possibility is that the subscription owner is some kind of almost-super-user, like a CREATEROLE user or someone running with rds_superuser or similar on some PG fork. So that needs to be safe too, and I think this does that. Having the subscription owner be some random user that doesn't have a lot of privileges doesn't seem particularly useful to me. If it were unproblematic to allow that, sure, but considering how easy it would be for that low-privilege user to steal table owner privileges, I don't think it makes sense. > > It is unclear to me whether we should try to back-port this. It's > > definitely a behavior change, and changing the behavior in the back > > branches is not a nice thing to do. On the other hand, at least in my > > opinion, the security consequences of the status quo are pretty dire. > > I tend to suspect that a really high percentage of people who are > > using logical replication at all are vulnerable to this, and lots of > > people having a way to escalate to superuser isn't good. > > It's worth considering given that most subscription owners are > superusers anyway. What plausible cases might it break? AFAIU, the main concern is about the SECURITY_RESTRICTED_OPERATION flag interacting badly with things people are already doing. Other problems seem possible, e.g. if you're doing something that gets the current user name and does something with it, the answer's going to change, and you might like the new answer more or less than the old one. It's a little hard to predict who will be inconvenienced in what ways when you change behavior, but problems are certainly possible. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
On Fri, 2023-03-03 at 11:02 -0500, Robert Haas wrote: > Hi, > > Here's a patch against master for $SUBJECT. It lacks documentation > changes and might have bugs, so please review if you're concerned > about this issue. I think the subject has a typo, you mean "as the table owner", right? > However, I'm unsatisfied with just > documenting the hazard, because I feel like almost everyone who uses > logical replication wants to do the exact thing that this > documentation says you shouldn't do. +1 > The proposed fix is to perform logical replication actions (SELECT, > INSERT, UPDATE, DELETE, and TRUNCATE) as the user who owns the table > rather than as the owner of the subscription. The session still runs > as the subscription owner, but the active user ID is switched to the > table owner for the duration of each operation. To prevent table > owners from doing tricky things to attack the subscription owner, we > impose SECURITY_RESTRICTED_OPERATION while running as the table > owner. +1 > To avoid inconveniencing users when this restriction adds no > meaningful security, we refrain from imposing > SECURITY_RESTRICTED_OPERATION when the table owner can SET ROLE to > the > subscription owner anyway. That's a little confusing, why not just always use the SECURITY_RESTRICTED_OPERATION? Is there a use case I'm missing? > There is also a possibility of an attack in the other direction. > Maybe > the subscription owner would like to obtain the table owner's > permissions, or at the very least, use logical replication as a > vehicle to perform operations they can't perform directly. A > malicious > subscription owner could hook up logical replication to a table into > which the table owner doesn't want replication to occur. To block > such > attacks, the patch requires that the subscription owner have the > ability to SET ROLE to each table owner. It would be really nice if this could be done with some kind of ordinary privilege rather than requiring SET ROLE. A user might expect that INSERT/UPDATE/DELETE/TRUNCATE privileges are enough. Or pg_write_all_data. I can see theoretically that a table owner might write some dangerous code and attach it to their table. But I don't quite understand why they would do that. If the code was vulnerable to attack, would that mean that they couldn't even insert into their own table safely? Requiring SET ROLE seems like it makes the subscription role into something very close to a superuser. And that takes away some of the benefits of delegating to non-superusers. > If the subscription owner is > a superuser, which is usual, this will be automatic. Otherwise, the > superuser will need to grant to the subscription owner the roles that > own relevant tables. This can usefully serve as a kind of access > control to make sure that the subscription doesn't touch any tables > other than the ones it's supposed to be touching: just make those > tables owned by a different user and don't grant them to the > subscription owner. Previously, we provided no way at all of > controlling the tables that replication can target. We check for ordinary access privileges, which I think is your next point, so the above paragraph confuses me a bit. > This fix interacts in an interesting way with Mark Dilger's work, > committed by Jeff Davis, to make logical replication respect table > permissions. I initially thought that with this change, that commit > would end up just being reverted, with the permissions scheme > described above replacing the existing one. However, I then realized > that it's still good to perform those checks. Normally, a table owner > can do any DML operation on a table they own, so those checks will > never fail. However, if a table owner has revoked their ability to, > say, INSERT into one of their own tables, then logical replication > shouldn't bypass that and perform the INSERT anyway. So I now think > that the checks added by that commit complement the ones added by > this > proposed patch, rather than competing with them. That's an interesting case. > It is unclear to me whether we should try to back-port this. It's > definitely a behavior change, and changing the behavior in the back > branches is not a nice thing to do. On the other hand, at least in my > opinion, the security consequences of the status quo are pretty dire. > I tend to suspect that a really high percentage of people who are > using logical replication at all are vulnerable to this, and lots of > people having a way to escalate to superuser isn't good. It's worth considering given that most subscription owners are superusers anyway. What plausible cases might it break? Regards, Jeff Davis
Re: running logical replication as the subscription owner
> Yeah. As Andres pointed out somewhere or other, that also means you're > decoding the WAL once per user instead of just once. I'm surprised > that hasn't been cost-prohibitive. We'd definitely prefer to have one subscription and do the decoding only once. But we haven't run into big perf issues with the current setup so far. We use it for non-blocking copying of shards (regular PG tables under the hood). Most of the time is usually spent in the initial copy phase, not the catchup. And also in practice our users often only have one table owning user (and more than 5 table owning users is extremely rare).
Re: running logical replication as the subscription owner
On Fri, Mar 3, 2023 at 6:57 PM Jelte Fennema wrote: > I'm definitely in favor of making it easier to use logical replication > in a safe manner. Cool. > In Citus we need to logically replicate and we're > currently using quite some nasty and undocumented hacks to do so: > We're creating a subscription per table owner, where each subscription > is owned by a temporary user that has the same permissions as the > table owner. These temporary users were originally superusers, because > otherwise we cannot make them subscription owners, but once assigning > a subscription to them we take away the superuser permissions from > them[1]. And we also need to hook into ALTER/DELETE subscription > commands to make sure that these temporary owners cannot edit their > own subscription[2]. > > Getting this right was not easy. And even it has the serious downside > that we need multiple subscriptions/replication slots which causes > extra complexity in various ways and it eats much more aggressively > into the replication slot limits than we'd like. Having one > subscription that could apply into tables that were owned by multiple > users in a safe way would make this sooo much easier. Yeah. As Andres pointed out somewhere or other, that also means you're decoding the WAL once per user instead of just once. I'm surprised that hasn't been cost-prohibitive. -- Robert Haas EDB: http://www.enterprisedb.com
Re: running logical replication as the subscription owner
I'm definitely in favor of making it easier to use logical replication in a safe manner. In Citus we need to logically replicate and we're currently using quite some nasty and undocumented hacks to do so: We're creating a subscription per table owner, where each subscription is owned by a temporary user that has the same permissions as the table owner. These temporary users were originally superusers, because otherwise we cannot make them subscription owners, but once assigning a subscription to them we take away the superuser permissions from them[1]. And we also need to hook into ALTER/DELETE subscription commands to make sure that these temporary owners cannot edit their own subscription[2]. Getting this right was not easy. And even it has the serious downside that we need multiple subscriptions/replication slots which causes extra complexity in various ways and it eats much more aggressively into the replication slot limits than we'd like. Having one subscription that could apply into tables that were owned by multiple users in a safe way would make this sooo much easier. [1]: https://github.com/citusdata/citus/blob/main/src/backend/distributed/replication/multi_logical_replication.c#L1487-L1573 [2]: https://github.com/citusdata/citus/blob/main/src/backend/distributed/commands/utility_hook.c#L455-L487