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

> On Jul 22, 2021, at 8:29 AM, Robert Haas <robertmhaas(at)gmail(dot)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