Subject: Non-superuser subscription owners

From: Mark Dilger <[email-masked]>
Date: Wed, 20 Oct 2021 11:40:39 -0700

These patches have been split off the now deprecated monolithic "Delegating superuser tasks to new security roles" thread at [1].

The purpose of these patches is to allow non-superuser subscription owners without risk of them overwriting tables they lack privilege to write directly. This both allows subscriptions to be managed by non-superusers, and protects servers with subscriptions from malicious activity on the publisher side.

[1] https://www.postgresql.org/message-id/flat/F9408A5A-B20B-42D2-9E7F-49CD3D1547BC%40enterprisedb.com
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

---

From: Ronan Dunklau <[email-masked]>
Date: Mon, 25 Oct 2021 09:26:30 +0200

Le mercredi 20 octobre 2021, 20:40:39 CEST Mark Dilger a écrit :
> These patches have been split off the now deprecated monolithic "Delegating
> superuser tasks to new security roles" thread at [1].
> 
> The purpose of these patches is to allow non-superuser subscription owners
> without risk of them overwriting tables they lack privilege to write
> directly. This both allows subscriptions to be managed by non-superusers,
> and protects servers with subscriptions from malicious activity on the
> publisher side.

Thank you Mark for splitting this.

This patch looks good to me, and provides both better security (by closing the 
"dropping superuser role" loophole) and usefule features. 

-- 
Ronan Dunklau

---

From: Andrew Dunstan <[email-masked]>
Date: Mon, 1 Nov 2021 10:18:24 -0400

> [1] https://www.postgresql.org/message-id/flat/F9408A5A-B20B-42D2-9E7F-49CD3D1547BC%40enterprisedb.com

These patches look good on their face. The code changes are very
straightforward.

w.r.t. this:

+   On the subscriber, the subscription owner's privileges are
re-checked for
+   each change record when applied, but beware that a change of
ownership for a
+   subscription may not be noticed immediately by the replication workers.
+   Changes made on the publisher may be applied on the subscriber as
+   the old owner.  In such cases, the old owner's privileges will be
the ones
+   that matter.  Worse still, it may be hard to predict when replication
+   workers will notice the new ownership.  Subscriptions created
disabled and
+   only enabled after ownership has been changed will not be subject to
this
+   race condition.

maybe we should disable the subscription before making such a change and
then re-enable it?

cheers

andrew

--
Andrew Dunstan
EDB: https://www.enterprisedb.com

---

From: Mark Dilger <[email-masked]>
Date: Mon, 1 Nov 2021 10:58:25 -0700

> On Nov 1, 2021, at 7:18 AM, Andrew Dunstan <[email-masked]> wrote:
> 
> w.r.t. this:
> 

Right.  I commented the code that way because there is a clear concern, but I was uncertain which way around the problem was best.

ALTER SUBSCRIPTION..[ENABLE | DISABLE] do not synchronously start or stop subscription workers.  The ALTER command updates the catalog's subenabled field, but workers only lazily respond to that.  Disabling and enabling the subscription as part of the OWNER TO would not reliably accomplish anything.

The attached patch demonstrates the race condition.  It sets up a publisher and subscriber, and toggles the subscription on and off on the subscriber node, interleaved with inserts and deletes on the publisher node.  If the ALTER SUBSCRIPTION commands were synchronous, the test results would be deterministic, with only the inserts performed while the subscription is enabled being replicated, but because the ALTER commands are asynchronous, the results are nondeterministic.

It is unclear that I can make ALTER SUBSCRIPTION..OWNER TO synchronous without redesigning the way workers respond to pg_subscription catalog updates generally.  That may be a good project to eventually tackle, but I don't see that it is more important to close the race condition in an OWNER TO than for a DISABLE.

Thoughts?

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Mon, 1 Nov 2021 15:44:32 -0700

> On Nov 1, 2021, at 10:58 AM, Mark Dilger <[email-masked]> wrote:
> 

Having discussed this with Andrew off-list, we've concluded that updating the documentation for logical replication to make this point more clear is probably sufficient, but I wonder if anyone thinks otherwise?

—
Mark Dilger

---

From: Robert Haas <[email-masked]>
Date: Tue, 2 Nov 2021 11:17:48 -0400

> Having discussed this with Andrew off-list, we've concluded that updating the documentation for logical replication to make this point more clear is probably sufficient, but I wonder if anyone thin…

The question in my mind is whether there's some reasonable amount of
time that a user should expect to have to wait for the changes to take
effect. If it could easily happen that the old permissions are still
in use a month after the change is made, I think that's probably not
good. If there's reason to think that, barring unusual circumstances,
changes will be noticed within a few minutes, I think that's fine.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Mark Dilger <[email-masked]>
Date: Wed, 3 Nov 2021 12:50:28 -0700

> ALTER SUBSCRIPTION..[ENABLE | DISABLE] do not synchronously start or stop subscription workers.  The ALTER command updates the catalog's subenabled field, but workers only lazily respond to that.…

I have rethought my prior analysis.  The problem in the previous patch was that the subscription apply workers did not check for a change in ownership the way they checked for other changes, instead only picking up the new ownership information when the worker restarted for some other reason.  This next patch set fixes that.  The application of a change record may continue under the old ownership permissions when a concurrent command changes the ownership of the subscription, but the worker will pick up the new permissions before applying the next record.  I think that is consistent enough with reasonable expectations.

The first two patches are virtually unchanged.  The third updates the behavior of the apply workers, and updates the documentation to match.

—
Mark Dilger

---

From: Jeff Davis <[email-masked]>
Date: Tue, 16 Nov 2021 10:08:06 -0800

On Mon, 2021-11-01 at 10:58 -0700, Mark Dilger wrote:
> It is unclear that I can make ALTER SUBSCRIPTION..OWNER TO
> synchronous without redesigning the way workers respond to
> pg_subscription catalog updates generally.  That may be a good
> project to eventually tackle, but I don't see that it is more
> important to close the race condition in an OWNER TO than for a
> DISABLE.
> 
> Thoughts?

What if we just say that OWNER TO must be done by a superuser, changing
from one superuser to another, just like today? That would preserve
backwards compatibility, but people with non-superuser subscriptions
would need to drop/recreate them.

When we eventually do tackle the problem, we can lift the restriction.

Regards,
	Jeff Davis

---

From: Mark Dilger <[email-masked]>
Date: Tue, 16 Nov 2021 10:12:26 -0800

> On Nov 16, 2021, at 10:08 AM, Jeff Davis <[email-masked]> wrote:
> 
>> It is unclear .....

The paragraph I wrote on 11/01 and you are responding to is no longer relevant.  The patch submission on 11/03 tackled the problem.  Have you had a chance to take a look at the new design?

—
Mark Dilger

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 16 Nov 2021 15:06:23 -0500

> The first two patches are virtually unchanged.  The third updates the behavior of the apply workers, and updates the documentation to match.

I'm generally happier about this than the previous patch set. With the
exception of some slight documentation modifications I think it's
basically committable. There doesn't seem to be a CF item for it but I'm
inclined to commit it in a couple of days time.

cheers

andrew

---

From: Mark Dilger <[email-masked]>
Date: Tue, 16 Nov 2021 12:08:04 -0800

> On Nov 16, 2021, at 12:06 PM, Andrew Dunstan <[email-masked]> wrote:
> 
> There doesn't seem to be a CF item for it but I'm

https://commitfest.postgresql.org/36/3414/

—
Mark Dilger

---

From: Andrew Dunstan <[email-masked]>
Date: Tue, 16 Nov 2021 15:40:54 -0500

> https://commitfest.postgresql.org/36/3414/

OK, got it, thanks.

cheers

andrew

---

From: Jeff Davis <[email-masked]>
Date: Tue, 16 Nov 2021 20:11:59 -0800

On Wed, 2021-11-03 at 12:50 -0700, Mark Dilger wrote:
> The first two patches are virtually unchanged.  The third updates the
> behavior of the apply workers, and updates the documentation to
> match.

v2-0001 corrects some surprises, but may create others. Why is renaming
allowed, but not changing the options? What if we add new options, and
some of them seem benign for a non-superuser to change?

The commit message part of the patch says that it's to prevent non-
superusers from being able to (effectively) create subscriptions, but
don't we want privileged non-superusers to be able to create
subscriptions?

Regards,
	Jeff Davis

---

From: Mark Dilger <[email-masked]>
Date: Wed, 17 Nov 2021 07:44:27 -0800

> On Nov 16, 2021, at 8:11 PM, Jeff Davis <[email-masked]> wrote:
> 

The patch cannot anticipate which logical replication options may be added to the project in some later commit.  We can let that commit adjust the behavior to allow the option if we agree it is sensible for non-superusers to do so.

> don't we want privileged non-superusers to be able to create

Perhaps, but I don't think merely owning a subscription should entitle a role to create new subscriptions.  Administrators may quite intentionally create low-power users, ones without access to anything but a single table, or a single schema, as a means of restricting the damage that a subscription might do (or more precisely, what the publisher might do via the subscription.)  It would be surprising if that low-power user was then able to recreate the subscription into something different.

We should probably come back to this topic in a different patch, perhaps a patch that introduces a new pg_manage_subscriptions role or such.

—
Mark Dilger

---

From: Jeff Davis <[email-masked]>
Date: Wed, 17 Nov 2021 09:33:24 -0800

On Wed, 2021-11-17 at 07:44 -0800, Mark Dilger wrote:
> Administrators may quite
> intentionally create low-power users, ones without access to anything
> but a single table, or a single schema, as a means of restricting the
> damage that a subscription might do (or more precisely, what the
> publisher might do via the subscription.)  It would be surprising if
> that low-power user was then able to recreate the subscription into
> something different.

I am still trying to understand this use case. It doesn't feel like
"ownership" to me, it feels more like some kind of delegation.

Is GRANT a better fit here? That would allow more than one user to
REFRESH, or ENABLE/DISABLE the same subscription. It wouldn't allow
RENAME, but I don't see why we'd separate privileges for
CREATE/DROP/RENAME anyway.

This would not address the weirdness of the existing code where a
superuser loses their superuser privileges but still owns a
subscription. But perhaps we can solve that a different way, like just
performing a check when someone loses their superuser privileges that
they don't own any subscriptions.

Regards,
	Jeff Davis

---

From: Mark Dilger <[email-masked]>
Date: Wed, 17 Nov 2021 10:25:50 -0800

> On Nov 17, 2021, at 9:33 AM, Jeff Davis <[email-masked]> wrote:
> 

We may eventually allow non-superusers to create subscriptions, but there are lots of details to work out.  Should there be limits on how many subscriptions they can create?  Should there be limits to the number of simultaneously open connections they can create out to other database servers (publishers)?  Should they need to be granted USAGE on a database publisher in order to use the connection string for that publisher in a subscription they create?  Should they need to be granted USAGE on a publication in order to replicate it?  Yes, there may be restrictions on the publisher side, too, but the user model on subscriber and publisher might differ, and the connection string used might not match the subscription owner, so some restriction on the subscriber side may be needed.

The implementation of [CREATE | ALTER] SUBSCRIPTION was designed at a time when only superusers could execute them, and as far as I can infer from the design, no effort to constrain the effects of those commands was made.  Since we're trying to make subscriptions into things that non-superusers can use, we have to deal with some things in those functions.  For example, ALTER SUBSCRIPTION can change the database connection parameter, or the publication subscribed, or whether synchronous_commit is used.  I don't see that a subscription owner should necessarily be allowed to mess with that, at least not without some other privilege checks beyond mere ownership.

I think this is pretty analogous to how security definer functions work.  You might call those "delegation" also, but the basic idea is that the function will run under the privileges of the function's owner, who might be quite privileged if you want the function to do highly secure things for you, but who could also intentionally be limited in privilege.  It wouldn't make much sense to say the owner of a security definer function can arbitrarily escalate their privileges to do things like open connections to other database servers, or have the transactions in which they run have a different setting of synchronous_commit.  Yet with subscriptions, if the subscription owner can run all forms of ALTER SUBSCRIPTION, that's what they can do.

I took a conservative position in the design of the patch to avoid giving away too much.  I suspect that we'll come back to these design decisions and relax them at some point, but the exact way in which we relax them is not obvious.  We could just agree to remove them (as you seem to propose), or we might agree to create predefined roles and say that the subscription owner can change certain aspects of the subscription if and only if they are members of one or more of those roles, or we may create new grantable privileges.  Each of those debates may be long and hard fought, so I don't want to invite that as part of this thread, or this patch will almost surely miss the cutoff for v15.

> they don't own any subscriptions.

I gave that a slight amount of thought during the design of this patch, but didn't think we could refuse to revoke superuser on such a basis, and didn't see what we should do with the subscription other than have it continue to be owned by the recently-non-superuser.  If you have a better idea, we can discuss it, but to some degree I think that is also orthogonal to the purpose of this patch.  The only sense in which this patch depends on that issue is that this patch proposes that non-superuser subscription owners are already an issue, and therefore that this patch isn't creating a new issue, but rather making more sane something that already can happen.

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Wed, 17 Nov 2021 10:48:44 -0800

> CREATE/DROP/RENAME anyway.

I don't think I answered this directly in my last reply.

GRANT *might* be part of some solution, but it is unclear to me how best to do it.  The various configuration parameters on subscriptions entail different security concerns.  We might take a fine-grained approach and create a predefined role for each, or we might take a course-grained approach and create a single pg_manage_subscriptions role which can set any parameter on any subscription, or maybe just parameters on subscriptions that the role also owns, or we might do something else, like burn some privilege bits and define new privileges that can be granted per subscription rather than globally.  (I think that last one is a non-starter, but just mention it as an example of another approach.)

—
Mark Dilger

---

From: Andrew Dunstan <[email-masked]>
Date: Wed, 17 Nov 2021 15:26:41 -0500

> inclined to commit it in a couple of days time.

Given there is some debate about the patch set I will hold off any
action for the time being.

cheers

andrew

---

From: Jeff Davis <[email-masked]>
Date: Wed, 17 Nov 2021 13:06:32 -0800

On Wed, 2021-11-17 at 10:25 -0800, Mark Dilger wrote:
> We may eventually allow non-superusers to create subscriptions, but
> there are lots of details to work out.

I am setting aside the idea of subscriptions created by non-superusers.

My comments were about your idea for "low-power users" that can still
do things with subscriptions. And for that, GRANT seems like a better
fit than ownership.

With v2-0001, there are several things that seem weird to me:

  * Why can there be only one low-power user per subscription?
  * Why is RENAME a separate capability from CREATE/DROP?
  * What if you want to make the privileges more fine-grained, or make
changes in the future? Ownership is a single bit, so it requires that
everyone agree. Maybe some people want RENAME to be a part of that, and
others don't.

GRANT seems to provide better answers here.

> Since we're trying to make subscriptions into things that non-
> superusers can use, we have to deal with some things in those
> functions.

I understand the use case where a superuser isn't required anywhere in
the process, and some special users can create and own subscriptions. I
also understand that's not what these patches are trying to accomplish
(though v2-0003 seems like a good step in that direction).

I don't understand the use case as well where a non-superuser can
merely "use" a subscription. I'm sure such use cases exist and I'm fine
to go along with that idea, but I'd like to understand why ownership
(partial ownership?) is the right way to do this and GRANT is the wrong
way.

> For example, ALTER SUBSCRIPTION can change the database connection
> parameter, or the publication subscribed, or whether
> synchronous_commit is used.  I don't see that a subscription owner
> should necessarily be allowed to mess with that, at least not without
> some other privilege checks beyond mere ownership.

That violates my expectations of what "ownership" means.

> I think this is pretty analogous to how security definer functions
> work.

The analogy to SECURITY DEFINER functions seems to support my
suggestion for GRANT at least as much as your modified definition of
ownership.

> I gave that a slight amount of thought during the design of this
> patch, but didn't think we could refuse to revoke superuser on such a
> basis,

I don't necessarily see a problem there, but I could be missing
something.

>  and didn't see what we should do with the subscription other than
> have it continue to be owned by the recently-non-superuser.  If you
> have a better idea, we can discuss it, but to some degree I think
> that is also orthogonal to the purpose of this patch.  The only sense
> in which this patch depends on that issue is that this patch proposes
> that non-superuser subscription owners are already an issue, and
> therefore that this patch isn't creating a new issue, but rather
> making more sane something that already can happen.

By introducing and documenting a way to get non-superusers to own a
subscription, it makes it more likely that people will do it, and
harder for us to change. That means the standard should be "this is
what we really want", rather than just "more sane than before".

Regards,
	Jeff Davis

---

From: Jeff Davis <[email-masked]>
Date: Wed, 17 Nov 2021 13:10:20 -0800

On Wed, 2021-11-17 at 10:48 -0800, Mark Dilger wrote:
> GRANT *might* be part of some solution, but it is unclear to me how
> best to do it.  The various configuration parameters on subscriptions
> entail different security concerns.  We might take a fine-grained
> approach and create a predefined role for each

I think you misunderstood the idea: not using predefined roles, just
plain old ordinary GRANT on a subscription object to ordinary roles.

   GRANT REFRESH ON SUBSCRIPTION sub1 TO nonsuper;

This should be easy enough because the subscription is a real object,
right?

Regards,
	Jeff Davis

---

From: Mark Dilger <[email-masked]>
Date: Wed, 17 Nov 2021 15:07:10 -0800

> On Nov 17, 2021, at 1:10 PM, Jeff Davis <[email-masked]> wrote:
> 

/*
 * Grantable rights are encoded so that we can OR them together in a bitmask.
 * The present representation of AclItem limits us to 16 distinct rights,
 * even though AclMode is defined as uint32.  See utils/acl.h.
 *
 * Caution: changing these codes breaks stored ACLs, hence forces initdb.
 */
typedef uint32 AclMode;         /* a bitmask of privilege bits */

#define ACL_INSERT      (1<<0)  /* for relations */
#define ACL_SELECT      (1<<1)
#define ACL_UPDATE      (1<<2)
#define ACL_DELETE      (1<<3)
#define ACL_TRUNCATE    (1<<4)
#define ACL_REFERENCES  (1<<5)
#define ACL_TRIGGER     (1<<6)
#define ACL_EXECUTE     (1<<7)  /* for functions */
#define ACL_USAGE       (1<<8)  /* for languages, namespaces, FDWs, and
                                 * servers */
#define ACL_CREATE      (1<<9)  /* for namespaces and databases */
#define ACL_CREATE_TEMP (1<<10) /* for databases */
#define ACL_CONNECT     (1<<11) /* for databases */

We only have 4 values left in the bitmask, and I doubt that burning those slots for multiple new types of rights that only have meaning for subscriptions is going to be accepted.  For full disclosure, I'm proposing adding ACL_SET and ACL_ALTER_SYSTEM in another patch and my proposal there could get shot down for the same reasons, but I think your argument would be even harder to defend.  Maybe others feel differently.

—
Mark Dilger

---

From: Jeff Davis <[email-masked]>
Date: Wed, 17 Nov 2021 15:46:55 -0800

On Wed, 2021-11-17 at 15:07 -0800, Mark Dilger wrote:
> We only have 4 values left in the bitmask, and I doubt that burning
> those slots for multiple new types of rights that only have meaning
> for subscriptions is going to be accepted.  For full disclosure, I'm
> proposing adding ACL_SET and ACL_ALTER_SYSTEM in another patch and my
> proposal there could get shot down for the same reasons, but I think
> your argument would be even harder to defend.  Maybe others feel
> differently.

Why not overload ACL_USAGE again, and say:

    GRANT USAGE ON SUBSCRIPTION sub1 TO nonsuper;

would allow ENABLE/DISABLE and REFRESH.

Again, I don't really understand the use case behind "can use a
subscription but not create one", so I'm not making a proposal. But
assuming that the use case exists, GRANT seems like a much better
approach.

(Aside: for me to commit something like this I'd want to understand the
"can use a subscription but not create one" use case better.)

Regards,
	Jeff Davis

---

From: Mark Dilger <[email-masked]>
Date: Wed, 17 Nov 2021 16:17:42 -0800

> On Nov 17, 2021, at 1:06 PM, Jeff Davis <[email-masked]> wrote:
> 

Ok, fair enough.  I think eventually we'll want that, but I'm also setting that aside for this patch.

> do things with subscriptions. And for that, GRANT seems like a better

This patch has basically no value beyond the fact that it allows the replication to be *applied* as a user other than superuser.  Throw that out, and there isn't any point.  Everything else is window dressing.  The real security problem with subscriptions is that they act with superuser power. That naturally means that they must be owned and operated by superuser, too, otherwise they serve as a privilege escalation attack vector.  It really doesn't make any sense to think of subscriptions as operating under the permissions of multiple non-superusers.  You must choose a single role you want the subscription to run under.  What purpose would be served by GRANTing privileges on a subscription to more than one non-superuser?  It still operates as just the one user.  I agree you *could* give multiple users privileges to mess with it, but you'd still need to assign a single role as the one whose privileges matter for the purpose of applying replication changes.  I'm using "owner" for that purpose, and I think that is consistent with how security definer functions work.  They run as the owner, too.  It's perfectly well-precedented to use "owner" for this.

I think the longer term plan is that non-superusers who have some privileged role will be allowed to create subscriptions, and naturally they will own the subscriptions that they create, at least until an ALTER SUBSCRIPTION..OWNER TO is successfully executed to transfer ownership.  Once that longer term plan is complete, non-superusers will be able to create publications of their tables on one database, and subscriptions to those publications on another database, all without needing the help of a superuser.  This patch doesn't get us all the way there, but it heads directly toward that goal.

>  * Why can there be only one low-power user per subscription?

Because the apply workers run as only one user.  Currently it is always superuser.  After this patch, it is always the owner, which amounts to the same thing for legacy subscriptions created and owned by superuser prior to upgrading to v15, but not necessarily for new ones or ones that have ownership transferred after upgrade.

We could think about subscriptions that act under multiple roles, perhaps taking role information as part of the data-stream from the publisher, but that's a pretty complicated proposal, and it is far from clear that we want it anyway.  There is a security case to be made for *not* allowing the publisher to call all the shots, so such a proposal would at best be an alternate mode of operation, not the one and only mode.

>  * Why is RENAME a separate capability from CREATE/DROP?

I don't care enough to argue this point.  If you want me to remove RENAME privilege from the owner, I can resubmit with it removed.  It just doesn't seem like it's dangerous to allow a non-superuser to rename their subscriptions, so I saw no compelling reason to disallow it.

CREATE clearly must be disallowed since it gives the creator the ability to form network connections, set fsync modes, etc., and there is no reason to assume arbitrary non-superusers should be able to do that.

The argument against DROP is a bit weaker.  It doesn't seem like a user who cannot bring subscriptions into existence should be able to drop them either.  I was expecting to visit that issue in a follow-on patch which deals with non-superuser predefined roles that have some power to create and drop subscriptions.  What that patch will propose to do is not obvious, since some of what you can do with subscriptions is so powerful we may not want non-superusers doing it, even with a privileged role.  If you can't picture what I mean, consider that you might use a connection parameter that connects outside and embeds data into the connection string, with a server listening on the other end, not really to publish data, but to harvest the secret data that you are embedding in the network connection attempt.

> everyone agree.

We can modify the patch to have the subscription owner have zero privileges on the subscription, not even the ability to see how it is defined, and just have "owner" mean the role under whose privileges the logical replication workers apply changes.  Would that be better?  I would expect people to find that odd.

The problem is that we want a setuid/setgid type behavior.  Actual setuid/setgid programs act as the user/group of the executable.  There's no reason that user/group needs to be one that any real human uses to log into the system.  Likewise, we need the subscription to act under a role, and we're establishing which role that is by having that role own the subscription.  That is like how setuid/setgid programs work by executing as the user/group that owns the executable, except that postgres doesn't have separate user/group concepts, just roles.  Isn't this design pattern completely commonplace?

> Maybe some people want RENAME to be a part of that, and
> others don't.

Fair enough.  Should I remove RENAME from what the patch allows the owner to do?  On this particular point, I genuinely don't care.  I think it can be reasonably argued either way.

> GRANT seems to provide better answers here.

No, because we don't have infinite privilege bits to burn.

> (though v2-0003 seems like a good step in that direction).

There is a cart-before-the-horse problem here.  If I propose a patch with a privileged role for creating and owning subscriptions *before* I tighten down how non-superuser-owned subscriptions work, that patch would surely be rejected.  So I either propose this first, and only if/when it gets accepted, propose the other, or I propose them together.  That's a damned-if-you-do--damned-if-you-dont situation, because if I propose them together, I'll get arguments that they are clearly separable and should be proposed separately, and if I do them one before the other, I'll get the argument that you are making now.  I fully expect the privileged role proposal to be made (possibly by me), though it is unclear if there will be time left to do it in v15.

> (partial ownership?) is the right way to do this and GRANT is the wrong

Even if we had the privilege bits to burn, no spelling of that GRANT idea sounds all that great:

	GRANT RUN AS ON subscription TO role;
	GRANT RUN AS ON role TO subscription;
	GRANT SUDO ON subscription TO role;
	GRANT SETUID ON role TO subscription;
	...

I just don't see how that really works.  I'm not inclined to spend time being more clever, since I already know that privilege bits are in short supply, but if you want to propose something, go ahead.  Elsewhere you proposed GRANT REFRESH or something, not looking at that email just now, but that's not the same thing as GRANT RUN AS, and burns another privilege bit, and still doesn't get us all the way there, because you presumably also want GRANT RENAME, GRANT ALTER CONNECTION SETTING, GRANT ALTER FSYNC SETTING, ..., and we're out of privilege bits before we're done.

> That violates my expectations of what "ownership" means.

I think that's because you're thinking of these settings as properties of the subscription.  You may *own* the subscription, but the subscription doesn't *own* the right to make connections to arbitrary databases, nor *own* the right to change buffer cache settings, nor *own* the right to bring data from a publication on some other server which, if it existed on the local server, would violate site policy and possibly constitute a civil or criminal violation of data privacy laws.  I may own my house, and the land it sits on, and my driveway, but that doesn't mean I own the ability to make my driveway go across my neighbor's field, down through town, and to the waterfront.  But that's the kind of ownership definition you seem to be defending.

Some of what I perceive as the screwiness of your argument I must admin is not your fault.  The properties of subscriptions are defined in ways that don't make sense to me.  It would be far more sensible if connection strings were objects in their own right, and you could grant USAGE on a connection string to a role, and USAGE on a subscription to a role, and only if the subscription worker's role had privileges on the connection string could they use it as part of fulfilling their task of replicating the data, and otherwise they'd error out in the attempt.  Likewise, fsync modes could be proper objects, and only if the subscription's role had privileges on the fsync mode they wanted to use would they be able to use it.  But we don't have these things as proper objects, with acl lists on them, so we're stuck trying to design around that.  To my mind, that means subscription owners *do not own* properties associated with the subscription.  To your mind, that's not what "ownership" means.  What to do?

> suggestion for GRANT at least as much as your modified definition of

I don't see how.  Can you please explain?

> I don't necessarily see a problem there, but I could be missing

Close your eyes and imagine that I have superuser on your database... really picture it in your mind.  Now, do you want the revoke command you are issuing to work?

> what we really want", rather than just "more sane than before".

Ok, I'll wait to hear back from you on the points above.

—
Mark Dilger

---

From: Amit Kapila <[email-masked]>
Date: Thu, 18 Nov 2021 16:20:01 +0530

> I gave that a slight amount of thought during the design of this patch, but didn't think we could refuse to revoke superuser on such a basis, and didn't see what we should do with the subscription…

Don't we want to close this gap irrespective of the other part of the
feature? I mean if we take out the part of your 0003 patch that checks
whether the current user has permission to perform a particular
operation on the target table then the gap related to the owner losing
superuser privileges should be addressed.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Thu, 18 Nov 2021 17:07:47 +0530

On Thu, Nov 4, 2021 at 1:20 AM Mark Dilger <[email-masked]> wrote:
> I have rethought my prior analysis.  The problem in the previous patch was that the subscription apply workers did not check for a change in ownership the way they checked for other changes, instead only picking up the new ownership information when the worker restarted for some other reason.  This next patch set fixes that.  The application of a change record may continue under the old ownership permissions when a concurrent command changes the ownership of the subscription, but the worker will pick up the new permissions before applying the next record.
>

Are you talking about the below change in the above paragraph?

@@ -2912,6 +2941,7 @@ maybe_reread_subscription(void)
  strcmp(newsub->slotname, MySubscription->slotname) != 0 ||
  newsub->binary != MySubscription->binary ||
  newsub->stream != MySubscription->stream ||
+ newsub->owner != MySubscription->owner ||
  !equal(newsub->publications, MySubscription->publications))
  {

If so, I am not sure how it will ensure that we check the ownership
change before applying each change? I think this will be invoked at
each transaction boundary, so, if there is a transaction with a large
number of changes, all the changes will be processed under the
previous owner.

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Thu, 18 Nov 2021 07:33:53 -0800

> On Nov 18, 2021, at 2:50 AM, Amit Kapila <[email-masked]> wrote:
> 

I don't think there is a gap.  The patch does the right thing, causing the subscription whose owner has had superuser revoked to itself no longer function with superuser privileges.  Whether that causes the subscription to fail depends on whether the previously-superuser now non-superuser owner now lacks sufficient privileges on the target relation(s).  I think removing that part of the patch would be a regression.

Let's compare two scenarios.  In the first, we have a regular user "alice" who owns a subscription which replicates into table "accounting.receipts" for which she has been granted privileges by the table's owner.  What would you expect to happen after the table's owner revokes privileges from alice?  I would expect that the subscription can no longer function, and periodic attempts to replicate into that table result in permission denied errors in the logs.

In the second, we have a superuser "alice" who owns a subscription that replicates into table "accounting.receipts", and she only has sufficient privileges to modify "accounting.receipts" by virtue of being superuser.  I would expect that when she has superuser revoked, the subscription can likewise no longer function.  

Now, maybe I'm wrong in both cases, and both should continue to function.  But I would find it really strange if the first situation behaved differently from the second.

I think intuitions about how subscriptions behave differ depending on the reason you expect the subscription to be owned by a particular user.  If the reason the user owns the subscription is that the user just happens to be the user who created it, but isn't in your mind associated with the subscription, then having the subscription continue to function regardless of what happens to the user, even the user being dropped, is probably consistent with your expectations.  In a sense, you think of the user who creates the subscription as having gifted it to the universe rather than continuing to own it.  Or perhaps you think of the creator of the subscription as a solicitor/lawyer/agent working on behalf of client, and once that legal transaction is completed, you don't expect the lawyer being disbarred should impact the subscription which exists for the benefit of the client.

If instead you think about the subscription owner as continuing to be closely associated with the subscription (as I do), then you expect changes in the owner's permissions to impact the subscription.

I think the "gifted to the universe"/"lawyer" mental model is not consistent with how the system is already designed to work.  You can't drop the subscription's owner without first running REASSIGN OWNED, or ALTER SUBSCRIPTION..OWNER TO, or simply dropping the subscription:

  DROP ROLE regress_subscription_user;
  ERROR:  role "regress_subscription_user" cannot be dropped because some objects depend on it
  DETAIL:  owner of subscription regress_testsub

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Thu, 18 Nov 2021 07:45:13 -0800

> On Nov 18, 2021, at 3:37 AM, Amit Kapila <[email-masked]> wrote:
> 

Yes, your analysis appears correct.  I was sloppy to say "before applying the next record".  It will pick up the change before applying the next transaction.

The prior version of the patch only picked up the change if it happened to start a new worker, but could process multiple transactions without noticing the change.  Now, it is limited to finishing the current transaction.  Would you prefer that the worker noticed the change in ownership and aborted the transaction on the subscriber side?  Or should the ALTER SUBSCRIPTION..OWNER TO block?  I don't see much advantage to either of those options, but I also don't think I have any knock-down argument for my approach either.  What do you think?

—
Mark Dilger

---

From: Jeff Davis <[email-masked]>
Date: Thu, 18 Nov 2021 10:29:26 -0800

On Wed, 2021-11-17 at 16:17 -0800, Mark Dilger wrote:
>  You must choose a single role you want the subscription to run
> under.

I think that was the source of a lot of my confusion: 

Your patches are creating the notion of a "run as" user by assigning
ownership-that-isn't-really-ownership. I got distracted wondering why
you would really care if some user could enable/disable/refresh/rename
a subscription, but the main point was to change who the subscription
runs as.

That's a more general idea: I could see how "run as" could apply to
subscriptions as well as functions (right now it can only run as the
owner or the invoker, not an arbitrary role). And I better understand
your analogy to security definer now.

But it's also not exactly a simple idea, and I think the current
patches oversimplify it and conflate it with ownership. 

> I think the longer term plan is that non-superusers who have some
> privileged role will be allowed to create subscriptions,

You earlier listed some challenges with that:

https://postgr.es/m/[email-masked]

But it seems like it's really the right direction to go. Probably the
biggest concern is connection strings that read server files, but
dblink solved that by requiring password auth.

What are the reasonable steps to get there? Do you think anything is
doable for v15?

> There is a cart-before-the-horse problem here.

I don't think we need to hold up v2-0003. It seems like a step in the
right direction, though I haven't looked closely yet.

Regards,
	Jeff Davis

---

From: Jeff Davis <[email-masked]>
Date: Thu, 18 Nov 2021 10:50:29 -0800

On Wed, 2021-11-17 at 16:17 -0800, Mark Dilger wrote:
> Some of what I perceive as the screwiness of your argument I must
> admin is not your fault.  The properties of subscriptions are defined
> in ways that don't make sense to me.  It would be far more sensible
> if connection strings were objects in their own right, and you could
> grant USAGE on a connection string to a role,

We sort of have that with CREATE SERVER, in fact dblink can use a
server instead of a string. 

Regards,
	Jeff Davis

---

From: Amit Kapila <[email-masked]>
Date: Fri, 19 Nov 2021 15:14:40 +0530

> I don't think there is a gap.  The patch does the right thing, causing the subscription whose owner has had superuser revoked to itself no longer function with superuser privileges.  Whether that c…

I think we are saying the same thing. I intend to say that your 0003*
patch closes the current gap in the code and we should consider
applying it irrespective of what we do with respect to changing the
... OWNER TO .. behavior. Is there a reason why 0003* patch (or
something on those lines) shouldn't be considered to be applied?

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Fri, 19 Nov 2021 15:26:09 +0530

> The prior version of the patch only picked up the change if it happened to start a new worker, but could process multiple transactions without noticing the change.  Now, it is limited to finishing…

How about allowing to change ownership only for disabled
subscriptions? Basically, users need to first disable the subscription
and then change its ownership. Now, disabling is an asynchronous
operation but we won't allow the ownership change command to proceed
unless the subscription is marked disabled and all the apply/sync
workers are not running. After the ownership is changed, users can
enable it. We already have 'slot_name' parameter's dependency on
whether the subscription is marked enabled or not.

This will add some steps in changing the ownership of a subscription
but I think it will be predictable.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Fri, 19 Nov 2021 15:53:08 +0530

> your analogy to security definer now.

I was thinking why not separate the ownership and "run as" privileges
for the subscriptions? We can introduce a new syntax in addition to
the current syntax for "Owner" for this as:

Create Subscription sub RUNAS <role_name> ...;
Alter Subscription sub RUNAS <role_name>

Now, RUNAS role will be used to apply changes and perform the initial
table sync. The owner will be tied to changing some of the other
properties (enabling, disabling, etc.) of the subscription. Now, we
still need a superuser to create subscription and change properties
like CONNECTION for a subscription but we can solve that by having
roles specific to it as being indicated by Mark in some of his
previous emails.

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Fri, 19 Nov 2021 07:25:49 -0800

> On Nov 19, 2021, at 1:44 AM, Amit Kapila <[email-masked]> wrote:
> 

Jeff Davis and I had a long conversation off-list yesterday and reached the same conclusion.  I will be submitting a version of 0003 which does not depend on the prior two patches.

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Fri, 19 Nov 2021 07:47:06 -0800

> On Nov 19, 2021, at 1:56 AM, Amit Kapila <[email-masked]> wrote:
> 
> and then change its ownership.

There are some open issues about non-superuser owners that Jeff would like to address before allowing transfers of ownership to non-superusers.  Your proposal about requiring the subscription to be disabled seems reasonable to me, but I'd like to see how it would interact with whatever Jeff proposes.  So I think I will change the patch as you suggest, but consider it a WIP patch until then.

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Fri, 19 Nov 2021 08:12:27 -0800

> On Nov 19, 2021, at 2:23 AM, Amit Kapila <[email-masked]> wrote:
> 

I feel disquieted about the "runas" idea.  I can't really say why yet.  Maybe it is ok, but it feels like a larger design decision than just an implementation detail about how subscriptions work.  We should consider if we won't soon be doing the same thing for other parts of the system.  If so, we should choose a solution that makes sense when applied more broadly.

Security definer functions could benefit from splitting the owner from the runas role.

Event triggers might benefit from having a runas role.  Currently, event triggers are always owned by superusers, but we've discussed allowing non-superuser owners.  That proposal still has outstanding issues to be resolved, so I can't be sure if runas would be helpful, but it might.

Table triggers might benefit from having a runas role.  I don't have a proposal here, just an intuition that we should think about this before designing how "runas" works.

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Fri, 19 Nov 2021 16:45:50 -0800

> On Nov 19, 2021, at 7:25 AM, Mark Dilger <[email-masked]> wrote:
> 

Renamed as 0001 in version 3, as it is the only remaining patch.  For anyone who reviewed the older patch set, please note that I made some changes to the src/test/subscription/t/026_nosuperuser.pl test case relative to the prior version.

—
Mark Dilger

---

From: Jeff Davis <[email-masked]>
Date: Wed, 24 Nov 2021 16:30:06 -0800

On Fri, 2021-11-19 at 16:45 -0800, Mark Dilger wrote:
> Renamed as 0001 in version 3, as it is the only remaining patch.  For
> anyone who reviewed the older patch set, please note that I made some
> changes to the src/test/subscription/t/026_nosuperuser.pl test case
> relative to the prior version.

We need to do permission checking for WITH CHECK OPTION and RLS. The
patch right now allows the subscription to write data that an RLS
policy forbids.

A couple other points:

 * We shouldn't refer to the behavior of previous versions in the docs
unless there's a compelling reason
 * Do we need to be smarter about partitioned tables, where an insert
can turn into an update?
 * Should we refactor to borrow logic from ExecInsert so that it's less
likely that we miss something in the future?

Regards,
	Jeff Davis

---

From: Amit Kapila <[email-masked]>
Date: Thu, 25 Nov 2021 09:51:22 +0530

> patch right now allows the subscription to write data that an RLS

Won't it be better to just check if the current user is superuser
before applying each change as a matter of this first patch? Sorry, I
was under impression that first, we want to close the current gap
where we allow to proceed with replication if the user's superuser
privileges were revoked during replication. To allow non-superusers
owners, I thought it might be better to first try to detect the change
of ownership as soon as possible instead of at the transaction
boundary.

-- 
With Regards,
Amit Kapila.

---

From: Jeff Davis <[email-masked]>
Date: Thu, 25 Nov 2021 12:06:36 -0800

On Thu, 2021-11-25 at 09:51 +0530, Amit Kapila wrote:
> privileges were revoked during replication.

That could be a first step, and I don't oppose it. But it seems like a
very small first step that would be made obsolete when v3-0001 is
ready, which I think will be very soon.

>  To allow non-superusers
> of ownership

In the case of revoked superuser privileges, there's no change in
ownership, just a change of privileges (SUPERUSER -> NOSUPERUSER). And
if we're detecting a change of privileges, why not just do it in
something closer to the right way, which is what v3-0001 is attempting
to do.

>  as soon as possible instead of at the transaction
> boundary.

I don't understand why it's important to detect a loss of privileges
faster than a transaction boundary. Can you elaborate?

Regards,
	Jeff Davis

---

From: Mark Dilger <[email-masked]>
Date: Sat, 27 Nov 2021 10:05:16 -0800

> On Nov 24, 2021, at 4:30 PM, Jeff Davis <[email-masked]> wrote:
> 

Thanks for reviewing and for this observation!  I can verify that RLS is not being honored on the subscriber side.  I agree this is a problem for subscriptions owned by non-superusers.

The implementation of the table sync worker uses COPY FROM, which makes this problem hard to fix, because COPY FROM does not support row level security.  We could do some work to honor the RLS policies during the apply workers' INSERT statements, but then some data would circumvent RLS during table sync and other data would honor RLS during worker apply, which would make the implementation not only wrong but inconsistently so.

I think a more sensible approach for v15 is to raise an ERROR if a non-superuser owned subscription is trying to replicate into a table which has RLS enabled.  We might try to be more clever and check whether the RLS policies could possibly reject the operation (by comparing the TO and FOR clauses of the policies against the role and operation type) but that seems like a partial re-implementation of RLS.  It would be simpler and more likely correct if we just unconditionally reject replicating into tables which have RLS enabled.

What do you think?

> unless there's a compelling reason

Fair enough.

> * Do we need to be smarter about partitioned tables, where an insert

Do you mean an INSERT statement with an ON CONFLICT DO UPDATE clause that is running against a partitioned table?  If so, I don't think we need to handle that on the subscriber side under the current logical replication design.  I would expect the plain INSERT or UPDATE that ultimately executes on the publisher to be what gets replicated to the subscriber, and not the original INSERT .. ON CONFLICT DO UPDATE statement.

> likely that we miss something in the future?

Hooking into the executor at a higher level, possibly ExecInsert or ExecModifyTable would do a lot more than what logical replication currently does.  If we also always used INSERT/UPDATE/DELETE statements and never COPY FROM statements, we might solve several problems at once, including honoring RLS policies and honoring rules defined for the target table on the subscriber side.

Doing this would clearly be a major design change, and possibly one we do not want.  Can we consider this out of scope?

—
Mark Dilger

---

From: Amit Kapila <[email-masked]>
Date: Mon, 29 Nov 2021 09:43:41 +0530

> faster than a transaction boundary. Can you elaborate?

The first reason is that way it would be consistent with what we can
see while doing the operations from the backend. For example, if we
revoke privileges from the user during the transaction, the results
will be reflected.
postgres=> Begin;
BEGIN
postgres=*> insert into t1 values(1);
INSERT 0 1
postgres=*> insert into t1 values(2);
ERROR:  permission denied for table t1

In this case, after the first insert, I have revoked the privileges of
the user from table t1 and the same is reflected in the very next
operation. Another reason is to make behavior predictable as users can
always expect when exactly the privilege change will be reflected and
it won't depend on the number of changes in the transaction.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Mon, 29 Nov 2021 11:26:01 +0530

...
> Do you mean an INSERT statement with an ON CONFLICT DO UPDATE clause that is running against a partitioned table?  If so, I don't think we need to handle that on the subscriber side under the curre…

Yeah, that is correct but I think the update case is more relevant
here. In ExecUpdate(), we convert Update to DELETE+INSERT when the
partition constraint is failed whereas, on the subscriber-side, it
will simply fail in this case. It is not clear to me how that is
directly related to this patch but surely it will be a good
improvement on its own and might help if that requires us to change
some infrastructure here like hooking into executor at a higher level.

> Doing this would clearly be a major design change, and possibly one we do not want.  Can we consider this out of scope?

I agree that if we want to do all of this then that would require a
lot of changes. However, giving an error for RLS-enabled tables might
also be too restrictive. The few alternatives could be that (a) we
allow subscription owners to be either have "bypassrls" attribute or
they could be superusers. (b) don't allow initial table_sync for rls
enabled tables. (c) evaluate/analyze what is required to allow Copy

---

From: Mark Dilger <[email-masked]>
Date: Mon, 29 Nov 2021 08:26:38 -0800

> On Nov 28, 2021, at 9:56 PM, Amit Kapila <[email-masked]> wrote:
> 
> In ExecUpdate(), we convert Update to DELETE+INSERT when the

I would rather get a fix for non-superuser subscription owners committed than expand the scope of work and have this patch linger until the v16 development cycle.  This particular DELETE+INSERT problem sounds important but unrelated and out of scope.

> From to start respecting RLS policies. (d) reject replicating any
> changes to tables that have RLS enabled.
> 
> I see that you are favoring (d) which clearly has merits like lesser
> code/design change but not sure if that is the best way forward or we
> can do something better than that either by following one of (a), (b),
> (c), or something less restrictive than (d).

I was favoring option (d) only when RLS policies exist for one or more of the target relations.

Skipping the table_sync step while replicating tables that have RLS policies for subscriptions that are owned by users who lack bypassrls is interesting.  If we make that work, it will be a more complete solution than option (d).

—
Mark Dilger

---

From: Jeff Davis <[email-masked]>
Date: Mon, 29 Nov 2021 10:22:47 -0800

On Mon, 2021-11-29 at 08:26 -0800, Mark Dilger wrote:
> > will simply fail in this case.

Thank you, yes, that's the more important case.

> This particular DELETE+INSERT problem sounds important but unrelated
> and out of scope.

+1

> > changes to tables that have RLS enabled.

Maybe a combination?

Allow subscriptions with copy_data=true iff the subscription owner is
bypassrls or superuser. And then enforce RLS+WCO during
insert/update/delete.

I don't think it's a big change (correct me if I'm wrong), and it
allows good functionality now, and room to improve in the future if we
want to bring in more of ExecInsert into logical replication.

Regards,
	Jeff Davis

---

From: Jeff Davis <[email-masked]>
Date: Mon, 29 Nov 2021 11:26:01 -0800

On Mon, 2021-11-29 at 09:43 +0530, Amit Kapila wrote:
> see while doing the operations from the backend.

Logical replication is not interactive, so it doesn't seem quite the
same.

If you have a long running INSERT INTO SELECT or COPY FROM, the
permission checks just happen at the beginning. As a user, it wouldn't
surprise me if logical replication was similar.

> it won't depend on the number of changes in the transaction.

This patch does detect ownership changes more quickly (at the
transaction boundary) than the current code (only when it reloads for
some other reason). Transaction boundary seems like a reasonable time
to detect the change to me.

Detecting faster might be nice, but I don't have a strong opinion about
it and I don't see why it necessarily needs to happen before this patch
goes in.

Also, do you think the cost of doing maybe_reread_subscription() per-
tuple instead of per-transaction would be detectable? If we lock
ourselves into semantics that detect changes quickly, it will be harder
to optimize the per-tuple path later.

Regards,
	Jeff Davis

---

From: Amit Kapila <[email-masked]>
Date: Tue, 30 Nov 2021 17:19:13 +0530

> bypassrls or superuser. And then enforce RLS+WCO during

Yeah, that sounds reasonable to me.

> I don't think it's a big change (correct me if I'm wrong),
>

Yeah, I also don't think it should be a big change.

-- 
With Regards,
Amit Kapila.

---

From: Amit Kapila <[email-masked]>
Date: Tue, 30 Nov 2021 17:25:40 +0530

> it and I don't see why it necessarily needs to happen before this patch

I think it would be better to do it before we allow subscription
owners to be non-superusers.

> tuple instead of per-transaction would be detectable?
>

Yeah, it is possible that is why I suggested in one of the emails
above to allow changing the owners only for disabled subscriptions.

-- 
With Regards,
Amit Kapila.

---

From: Jeff Davis <[email-masked]>
Date: Tue, 30 Nov 2021 12:42:13 -0800

> owners to be non-superusers.

There are a couple other things to consider before allowing non-
superusers to create subscriptions anyway. For instance, a non-
superuser shouldn't be able to use a connection string that reads the
certificate file from the server unless they also have
pg_read_server_files privs.

> above to allow changing the owners only for disabled subscriptions.

The current patch detects the following cases at the transaction
boundary:

 * ALTER SUBSCRIPTION ... OWNER TO ...
 * ALTER ROLE ... NOSUPERUSER
 * privileges revoked one way or another (aside from the RLS/WCO
problems, which will be fixed)

If we want to detect at row boundaries we need to capture all of those
cases too, or else we're being inconsistent. The latter two cannot be
tied to whether the subscription is disabled or not, so I don't think
that's a complete solution.

How about (as a separate patch) we just do maybe_reread_subscription()
every K operations within a transaction? That would speed up
permissions errors if a revoke happens.

Regards,
	Jeff Davis

---

From: Amit Kapila <[email-masked]>
Date: Wed, 1 Dec 2021 19:06:17 +0530

> pg_read_server_files privs.

Isn't allowing to create subscriptions via non-superusers and allowing
to change the owner two different things? I am under the impression
that the latter one is more towards allowing the workers to apply
changes with a non-superuser role.

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Wed, 1 Dec 2021 11:21:35 -0800

> On Dec 1, 2021, at 5:36 AM, Amit Kapila <[email-masked]> wrote:
> 

The short-term goal is to have logical replication workers respect the privileges of the role which owns the subscription.

The long-term work probably includes creating a predefined role with permission to create subscriptions, and the ability to transfer those subscriptions to roles who might be neither superuser nor members of any particular predefined role; the idea being that logical replication subscriptions can be established without any superuser involvement, and may thereafter run without any special privilege.

The more recent patches on this thread are not as ambitious as the earlier patch-sets.  We are no longer trying to support transferring subscriptions to non-superusers.

Right now, on HEAD, if a subscription owner has superuser revoked, the subscription can continue to operate as superuser in so far as its replication actions are concerned.  That seems like a pretty big security hole.

This patch mostly plugs that hole by adding permissions checks, so that a subscription owned by a role who has privileges revoked cannot (for the most part) continue to act under the old privileges.

There are two problematic edge cases that can occur after transfer of ownership.  Remember, the new owner is required to be superuser for the transfer of ownership to occur.

1) A subscription is transferred to a new owner, and the new owner then has privilege revoked.

2) A subscription is transferred to a new owner, and then the old owner has privileges increased.

In both cases, a currently running logical replication worker may finish a transaction in progress acting with the current privileges of the old owner.  The clearest solution is, as you suggest, to refuse transfer of ownership of subscriptions that are enabled.

Doing so will create a failure case for REASSIGN OWNED BY.  Will that be ok?

—
Mark Dilger

---

From: Amit Kapila <[email-masked]>
Date: Thu, 2 Dec 2021 14:59:46 +0530

> This patch mostly plugs that hole by adding permissions checks, so that a subscription owned by a role who has privileges revoked cannot (for the most part) continue to act under the old privileges.

If we want to maintain the property that subscriptions can only be
owned by superuser for your first version then isn't a simple check
like ((!superuser()) for each of the operations is sufficient?

> 2) A subscription is transferred to a new owner, and then the old owner has privileges increased.

In (2), I am not clear what do you mean by "the old owner has
privileges increased"? If the owners can only be superusers then what
does it mean to increase the privileges.

> Doing so will create a failure case for REASSIGN OWNED BY.  Will that be ok?

I think so. Do we see any problem with that? I think we have some
failure cases currently as well like "All Tables Publication" can only
be owned by superusers whereas ownership for others can be to
non-superusers and similarly we can't change ownership for pinned
objects. I think the case being discussed is not exactly the same but
I am not able to see a problem with it.

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Fri, 3 Dec 2021 05:31:46 -0800

> On Dec 2, 2021, at 1:29 AM, Amit Kapila <[email-masked]> wrote:
> 

As things stand today, nothing prevents a superuser subscription owner from having superuser revoked.  The patch does nothing to change this.

> does it mean to increase the privileges.

The old owner may have had privileges reduced (no superuser, only permission to write into a specific schema, etc.) and the subscription enabled only after those privilege reductions were put in place.  This is a usage pattern this patch is intended to support, by honoring those privilege restrictions.

—
Mark Dilger

---

From: Amit Kapila <[email-masked]>
Date: Mon, 6 Dec 2021 15:49:52 +0530

> As things stand today, nothing prevents a superuser subscription owner from having superuser revoked.  The patch does nothing to change this.

I understand that but won't that get verified when we look up the
information in pg_authid as part of superuser() check?

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Mon, 6 Dec 2021 07:56:56 -0800

> On Dec 6, 2021, at 2:19 AM, Amit Kapila <[email-masked]> wrote:
> 
>>> owned by superuser

We don't want to maintain such a property, or at least, that's not what I want.  I don't think that's what Jeff wants, either.

To clarify, I'm not entirely sure how to interpret the verb "maintain" in your question, since before the patch the property does not exist, and after the patch, it continues to not exist.  We could *add* such a property, of course, though this patch does not attempt any such thing.

> information in pg_authid as part of superuser() check?

If we added a superuser() check, then yes, but that would take things in a direction I do not want to go.

As I perceive the roadmap:

1) Fix the current bug wherein subscription changes are applied with superuser force after the subscription owner has superuser privileges revoked.
2) Allow the transfer of subscriptions to non-superuser owners.
3) Allow the creation of subscriptions by non-superusers who are members of some as yet to be created predefined role, say "pg_create_subscriptions"

I may be wrong, but it sounds like you interpret the intent of this patch as enforcing superuserness.  That's not so.  This patch intends to correctly handle the situation where a subscription is owned by a non-superuser (task 1, above) without going so far as creating new paths by which that situation could arise (tasks 2 and 3, above).

—
Mark Dilger

---

From: Ronan Dunklau <[email-masked]>
Date: Tue, 07 Dec 2021 10:39:43 +0100

Le lundi 6 décembre 2021, 16:56:56 CET Mark Dilger a écrit :
> We don't want to maintain such a property, or at least, that's not what I
> want.  I don't think that's what Jeff wants, either.

That's not what I want either: the ability to run and refresh subscriptions as 
a non superuser is a desirable feature. 

The REFRESH part was possible before PG 14, when it was allowed to run REFRESH 
in a function, which could be made to run as security definer. 

> 1) Fix the current bug wherein subscription changes are applied with
> superuser force after the subscription owner has superuser privileges
> revoked. 2) Allow the transfer of subscriptions to non-superuser owners.
> 3) Allow the creation of subscriptions by non-superusers who are members of
> some as yet to be created predefined role, say "pg_create_subscriptions"

This roadmap seems sensible.

-- 
Ronan Dunklau

---

From: Amit Kapila <[email-masked]>
Date: Tue, 7 Dec 2021 15:59:17 +0530

> To clarify, I'm not entirely sure how to interpret the verb "maintain" in your question, since before the patch the property does not exist, and after the patch, it continues to not exist.  We coul…

Okay, let me try to explain again. Following is the text from docs
[1]: " (a) To create a subscription, the user must be a superuser. (b)
The subscription apply process will run in the local database with the
privileges of a superuser. (c) Privileges are only checked once at the
start of a replication connection. They are not re-checked as each
change record is read from the publisher, nor are they re-checked for
each change when applied.

My understanding is that we want to improve what is written as (c)
which I think is the same as what you mentioned later as "Fix the
current bug wherein subscription changes are applied with superuser
force after the subscription owner has superuser privileges revoked.".
Am I correct till here? If so, I think what I am suggesting should fix
this with the assumption that we still want to follow (b) at least for
the first patch. One possibility is that our understanding of the
first problem is the same but you want to allow apply worker running
even when superuser privileges are revoked provided the user with
which it is running has appropriate privileges on the objects being
accessed by apply worker.

We will talk about other points of the roadmap you mentioned once our
understanding for the first one matches.

[1] - https://www.postgresql.org/docs/devel/logical-replication-security.html

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Tue, 7 Dec 2021 06:55:51 -0800

> On Dec 7, 2021, at 2:29 AM, Amit Kapila <[email-masked]> wrote:
> 
> the first patch.

Ok, that's a point of disagreement.  I was trying to fix both (b) and (c) in the first patch.

> One possibility is that our understanding of the

Correct, that's what I'm trying to make safe.

> understanding for the first one matches.

I am happy to have an off-list phone call with you, if you like.

—
Mark Dilger

---

From: Amit Kapila <[email-masked]>
Date: Thu, 9 Dec 2021 10:28:24 +0530

> Ok, that's a point of disagreement.  I was trying to fix both (b) and (c) in the first patch.

But, I think as soon as we are trying to fix (b), we seem to be
allowing non-superusers to apply changes. If we want to do that then
we should be even allowed to change the owners to non-superusers. I
was thinking of the below order:
1. First fix (c) from the above description "Privileges are only
checked once at the start of a replication connection."
2A. Allow the transfer of subscriptions to non-superuser owners. This
will be allowed only on disabled subscriptions to make this action
predictable.
2B. The apply worker should be able to apply the changes provided the
user has appropriate privileges on the objects being accessed by apply
worker.
3) Allow the creation of subscriptions by non-superusers who are
members of some as yet to be created predefined role, say
"pg_create_subscriptions"

We all seem to agree that (3) can be done later as an independent
project. 2A, 2B can be developed as separate patches but they need to
be considered together for commit. After 2A, 2B, the first one (1)
won't be required so, in fact, we can just ignore (1) but the only
benefit I see is that if we stuck with some design problem during the
development of  2A, 2B, we would have at least something better than
what we have now.

You seem to be indicating let's do 2B first as that will anyway be
used later after 2A and 1 won't be required if we do that. I see that
but I personally feel either we should follow 1, 2(A, B) or just do
2(A, B).

-- 
With Regards,
Amit Kapila.

---

From: Robert Haas <[email-masked]>
Date: Thu, 9 Dec 2021 10:41:21 -0500

> owners to be non-superusers.

I think it would be better not to ever do it at any time.

It seems like a really bad idea to me to change the run-as user in the
middle of a transaction. That seems prone to producing all sorts of
confusing behavior that's hard to understand, and hard to test. So
what are we to do if a change occurs mid-transaction? I think we can
either finish replicating the current transaction and then switch to
the new owner for the next transaction, or we could abort the current
attempt to replicate the transaction and retry the whole transaction
with the new run-as user. My guess is that most users would prefer the
former behavior to the latter.

---

From: Robert Haas <[email-masked]>
Date: Thu, 9 Dec 2021 10:47:03 -0500

> but I personally feel either we should follow 1, 2(A, B) or just do

1 and 2B seem to require changing the same code, or related code. 1A
seems to require a completely different set of changes. If I'm right
about that, it seems like a good reason for doing 1+2B first and
leaving 2A for a separate patch.

---

From: Mark Dilger <[email-masked]>
Date: Thu, 9 Dec 2021 09:22:07 -0800

> On Dec 9, 2021, at 7:41 AM, Robert Haas <[email-masked]> wrote:
> 
> middle of a transaction.

I agree.  We allow SET ROLE inside transactions, but faking one on the subscriber seems odd.  No such role change was performed on the publisher side, nor is there a principled reason for assuming the old run-as role has membership in the new run-as role, so we'd be pretending to do something that might otherwise be impossible.

There was some discussion off-list about having the apply worker take out a lock on its subscription, thereby blocking ownership changes mid-transaction.  I coded that and it seems to work fine, but I have a hard time seeing how the lock traffic would be worth expending.  Between (a) changing roles mid-transaction, and (b) locking the subscription for each transaction, I'd prefer to do neither, but (b) seems far better than (a).  Thoughts?

—
Mark Dilger

---

From: Mark Dilger <[email-masked]>
Date: Thu, 9 Dec 2021 09:48:59 -0800

> On Dec 9, 2021, at 7:47 AM, Robert Haas <[email-masked]> wrote:
> 

There are unresolved problems with 2A and 3 which were discussed upthread.  I don't want to include fixes for them in this patch, as it greatly expands the scope of this patch, and is a logically separate effort.  We can come back to those problems after this first patch is committed.

Specifically, a non-superuser owner can perform ALTER SUBSCRIPTION and do things that are morally equivalent to creating a new subscription.  This is problematic where things like the connection string are concerned, because it means the non-superuser owner can connect out to entirely different servers, without any access control checks to make sure the owner should be able to connect to these servers.

This problem already exists, right now.  I'm not fixing it in this first patch, but I'm also not making it any worse.

The solution Jeff Davis proposed seems right to me.  We change subscriptions to use a foreign server rather than a freeform connection string.  When creating or altering a subscription, the role performing the action must have privileges on any foreign server they use.

—
Mark Dilger

---

From: Amit Kapila <[email-masked]>
Date: Fri, 10 Dec 2021 09:45:08 +0530

> There was some discussion off-list about having the apply worker take out a lock on its subscription, thereby blocking ownership changes mid-transaction.  I coded that and it seems to work fine, bu…

Yeah, to me also (b) sounds better than (a). However, a few points
that we might want to consider in that regard are as follows: 1.
locking the subscription for each transaction will add new blocking
areas considering we acquire AccessExclusiveLock to change any
property of subscription. But as Alter Subscription won't be that
frequent operation it might be acceptable. 2. It might lead to adding
some cost to small transactions but not sure if that will be
noticeable. 3. Tomorrow, if we want to make the apply-process parallel
(IIRC, we do have the patch for that somewhere in archives) especially
for large in-progress transactions then this locking will have
additional blocking w.r.t Altering Subscription. But again, this also
might be acceptable.

-- 
With Regards,
Amit Kapila.

---

From: Robert Haas <[email-masked]>
Date: Fri, 10 Dec 2021 09:09:26 -0500

On Thu, Dec 9, 2021 at 11:15 PM Amit Kapila <[email-masked]> wrote:
> frequent operation it might be acceptable.

The problem isn't the cost of the locks taken by ALTER SUBSCRIPTION.
It's the cost of locking and unlocking the relation for every
transaction we apply. Suppose it's a pgbench-type workload with a
single UPDATE per transaction. You've just limited the maximum
possible apply speed to about, I think, 30,000 transactions per second
no matter how many parallel workers you use, because that's how fast
the lock manager is (or was, unless newer hardware or newer PG
versions have changed things in a way I don't know about). That seems
like a poor idea. There's nothing wrong with noticing changes at the
next transaction boundary, as long as we document it. So why would we
incur a possibly-significant performance cost to provide a stricter
guarantee?

I bet users wouldn't even like this behavior. It would mean that if
you are replicating a long-running transaction, an ALTER SUBSCRIPTION
command might block for a long time until replication of that
transaction completes. I have a hard time understanding why anyone
would consider that an improvement.

---

From: Andrew Dunstan <[email-masked]>
Date: Fri, 10 Dec 2021 10:20:02 -0500

+1

I think noticing changes at the transaction boundary is perfectly
acceptable.

cheers

andrew

---

From: Amit Kapila <[email-masked]>
Date: Sat, 11 Dec 2021 10:25:36 +0530

On Fri, Dec 10, 2021 at 7:39 PM Robert Haas <[email-masked]> wrote:
> transaction we apply.

This point is not clear to me as we are already locking the relation
while applying changes. I think here additional cost is to lock a
particular subscription as well in addition to the relation on which
we are going to perform apply. I agree that has a cost and that is why
I mentioned it as one of the points above and then also the
concurrency effect as you also noted could make this idea moot.

> Suppose it's a pgbench-type workload with a
> next transaction boundary, as long as we document it.
>

If we want to just document this then I think we should also keep in
mind that these could be N transactions as well if say tomorrow we
have N parallel apply workers applying the N transactions in parallel.
I think it might also be possible that RLS policies won't be applied
for initial table sync whereas those will be applied for later changes
even though the ownership has changed before both operations and one
of those happens to miss it. If that is possible, then it might be
better to avoid the same as it could appear inconsistent as mentioned
by Mark [1] as well. Now, it might be possible to avoid this by
implementation or we can say that we don't care about this or just
document it. But it seems to me that if we have some way to detect the
change of ownership at each operation level then no such possibilities
would arise.

The other alternative we discussed was to allow a change of ownership
on disabled subscriptions that way the apply behavior will always be
predictable.

There is clearly a merit in noticing the change of ownership at
transaction boundary but just wanted to consider other possibilities.
It could be that detecting at transaction-boundary is the best we can
do but I think there is no harm in considering other possibilities.

> So why would we
> transaction completes.

Agreed and if we decide to lock the subscription during the initial
table sync phase then that could also take a long time for which again
users might not be happy.

[1] - https://www.postgresql.org/message-id/FE7D7024-6723-4ACB-82AB-94F6A194BE0D%40enterprisedb.com

-- 
With Regards,
Amit Kapila.

---

From: Mark Dilger <[email-masked]>
Date: Wed, 15 Dec 2021 12:23:02 -0800

> patch right now allows the subscription to write data that an RLS

Version 4 of the patch, attached, no longer allows RLS to be circumvented, but does so in a course-grained fashion.  If the target table has row-level security policies which are enforced against the subscription owner, the replication draws an error, much as with a permissions failure.  This seems sufficient for now, as superusers, roles with bypassrls, and target table owners should be able to replicate as before.  We may want to revisit this later, perhaps if/when we address your ExecInsert question, below.

> unless there's a compelling reason

Fixed.

> * Do we need to be smarter about partitioned tables, where an insert

Indeed, the logic of apply_handle_tuple_routing() required a bit of refactoring.  Fixed in v4.

> likely that we miss something in the future?

Let's just punt on this for now.

—
Mark Dilger