> On Jul 22, 2021, at 8:29 AM, Robert Haas <robertmh...@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 yet implemented such security, as I was 
fishing for comments from the community about whether the basic division of 
superuser into these new roles was the right division.  Having gotten little 
feedback on that, on June 29 I posted another patch 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 unchanged after the patch.  The real 
difference is that prior to the patch, only superusers can own subscriptions, 
so checking permissions on tables during replication would be silly (though not 
harmful).  The worker is assured of passing all such 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 to a 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-superusers as logical 
replication workers is silly.  But I don't mind that much, either.  I'll break 
them in two for the next 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 should not be in a separate patch from 
the behavioral changes.  I agree with that.  I'll add documentation of the 
changes to 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/delete rather 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 those tables, thereby 
firing those triggers, and executing the trigger code as superuser.  That is 
ordinarily not something that a user can do simply by creating a table with a 
trigger, since there would be no mechanism to force the superuser to 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 subscription is also the user whose 
permissions are checked when operating on the table and executing the trigger.  
This closes the security hole, so far as I am aware.  I would very much like 
more eyeballs on this patch, and if anybody sees why this is an insufficient 
solution, please speak up.  But it's not as if I punted the security issue down 
the road to some ill-defined future patch.  On the contrary, this patch both 
creates the ability to delegate subscription creation authority to 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





Reply via email to