On Thu, Jul 22, 2021 at 1:29 PM Mark Dilger <mark.dil...@enterprisedb.com> wrote: > 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.
Oh. > 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. Aren't you supposing that the set of superusers never changes? Unless we have some code for this that we don't have elsewhere, a superuser could create a subscription and then be de-superuser'd, or the subscription's owner could be altered. > > 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. I just meant that I think the behavioral change to logical replication is significant in its own right and should be a separate patch. Perhaps it's not as significant as I thought, but I still think it should be made separately and likely documented as an incompatibility with previous releases, unless I'm still confused. > > 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. OK. I thought Noah must be talking about some other problem, because on May 28th you wrote "Oh, I agree that this patch set does not go the extra step to make it safe" and I failed to understand that you thought you'd addressed this in v4. -- Robert Haas EDB: http://www.enterprisedb.com
