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

On Fri, May 28, 2021 at 1:42 PM Mark Dilger
<mark(dot)dilger(at)enterprisedb(dot)com> wrote:
> > pg_logical_replication would not be safe to delegate that way:
> > https://postgr.es/m/flat/CACqFVBbx6PDq%2B%3DvHM0n78kHzn8tvOM-kGO_2q_q0zNAMT%2BTzdA%40mail.gmail.com
>
> Oh, I agree that this patch set does not go the extra step to make it safe. You are quite right to push back, as my email was poorly worded. I should have said "intended to be eventually made safe to delegate". The idea is that the patch set addresses most places in the sources where we test for superuser and tests instead for (superuser || <SOME_ROLE>), and then uses that same set of roles to control who has sufficient privileges to set GUCs. The pg_host_security and pg_network_security roles are not intended to eventually be safe to delegate. Or at least, I can't see any clear path to getting there. The pg_database_security and pg_logical_replication roles should be ones we can make safe. If we can agree as a community which set of roles are appropriate, then we can have separate patches as needed for tightening the security around them.

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. 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. 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.

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.

--
Robert Haas
EDB: http://www.enterprisedb.com