On Thu, Dec 2, 2021 at 12:51 AM Mark Dilger <mark.dil...@enterprisedb.com> wrote: > > > > On Dec 1, 2021, at 5:36 AM, Amit Kapila <amit.kapil...@gmail.com> wrote: > > > > On Wed, Dec 1, 2021 at 2:12 AM Jeff Davis <pg...@j-davis.com> wrote: > >> > >> On Tue, 2021-11-30 at 17:25 +0530, Amit Kapila wrote: > >>> I think it would be better to do it before we allow subscription > >>> 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. > >> > > > > 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. > > 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. >
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? > 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 (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. > 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? > 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.
