On Fri, May 28, 2021 at 1:42 PM Mark Dilger
<mark.dilger@enterprisedb.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
emailwas poorly worded. I should have said "intended to be eventually made safe to delegate". The idea is that the
patchset 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_securityand pg_network_security roles are not intended to eventually be safe to delegate. Or at least, I can't
seeany clear path to getting there. The pg_database_security and pg_logical_replication roles should be ones we can
makesafe. If we can agree as a community which set of roles are appropriate, then we can have separate patches as
neededfor 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
