Re: Delegating superuser tasks to new security roles (Was: Granting control of SUSET gucs to non-superusers) - Mailing list pgsql-hackers

> On Jul 22, 2021, at 8:29 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> I don't think that we want to commit a patch to add a
> pg_logical_replication role that can "eventually" be made staff to
> delegate to non-superusers.

Certainly not.  What I meant on May 28 by "eventually" was that the patch set (posted May 25 and named "v3") had not
yetimplemented such security, as I was fishing for comments from the community about whether the basic division of
superuserinto these new roles was the right division.  Having gotten little feedback on that, on June 29 I posted
anotherpatch set (confusingly also named "v3", my apologies) in which patch 0001 had expanded to include new security
restrictions.

> Whatever issues need to be fixed should be
> fixed first, and then this change can be considered afterwards. It
> seems like you try to fix at least some of the issues in the patch,
> because I see permission checks being added in
> src/backend/replication/logical/worker.c, and I don't think that
> should happen in the same patch that adds the new predefined role.

Prior to this patch, the logical replication workers run under the userid of the owner of the subscription.  This is
unchangedafter the patch.  The real difference is that prior to the patch, only superusers can own subscriptions, so
checkingpermissions on tables during replication would be silly (though not harmful).  The worker is assured of passing
allsuch permission checks by virtue of being a superuser.  After the patch, since subscription owners need not be
superusers,the permission checks are no longer silly.  There is no assurance that they have permission to apply changes
toa table, so naturally that has to be checked, and it is.  

I don't really see this as two separate patches, since the addition of the permissions checks without the addition of
non-superusersas logical replication workers is silly.  But I don't mind that much, either.  I'll break them in two for
thenext patch set. 

> I
> also think it should be accompanied not only by new test cases (which
> you seem to have added, though I have not reviewed them in detail) but
> also documentation changes (which seem to be missing, since the doc
> changes are all about the new predefined role). This is a really
> significant behavior change to logical replication IMV and shouldn't
> just be slipped into some other patch.

I'm not sure what is meant by "slipped into some other patch", but I *think* you mean that the documentation changes
shouldnot be in a separate patch from the behavioral changes.  I agree with that.  I'll add documentation of the
changesto logical replication in the same patch as the changes themselves. 

> It also seems based on Noah's comments and your response that there
> might be some other issue here, and I haven't understood what that is,
> but I think that should also be fixed separately, and first.
> Considering all this, I would suggest not having this be patch #1 in
> your series; make something come first that doesn't have
> prerequisites.

The issue that gets thrown around in the email archive is that "arbitrary code" can be made to run on the subscriber
side. As I understand the problem, this is because trigger functions can be created on tables with arbitrary code in
them,and that code will be executed under the userid of the user who causes the trigger to fire during an
insert/update/deleterather than as the user who created the trigger.  This of course is not peculiar to logical
replication;it is how triggers work generally.  What is peculiar is that a non-superuser who can create tables,
triggers,publications and subscriptions can get the logical replication worker to perform inserts/updates/deletes on
thosetables, thereby firing those triggers, and executing the trigger code as superuser.  That is ordinarily not
somethingthat a user can do simply by creating a table with a trigger, since there would be no mechanism to force the
superuserto perform operations on the table. 

After patch 0001 (which will be split in the next patch set, but hasn't been split yet) the user who creates the
subscriptionis also the user whose permissions are checked when operating on the table and executing the trigger.  This
closesthe security hole, so far as I am aware.  I would very much like more eyeballs on this patch, and if anybody sees
whythis is an insufficient solution, please speak up.  But it's not as if I punted the security issue down the road to
someill-defined future patch.  On the contrary, this patch both creates the ability to delegate subscription creation
authorityto a non-superuser and closes the security hole which that would otherwise entail, or at least, that is the
intent.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company