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

On Wed, Oct 20, 2021 at 11:27:11AM -0700, Jeff Davis wrote:
> On Wed, 2021-10-20 at 10:32 -0700, Mark Dilger wrote:
> > I'd like to have a much clearer understanding of Noah's complaint
> > first. There are multiple things to consider: (1) the role which
> > owns the trigger, (2) the role which is performing an action which
> > would cause the trigger to fire, (3) the set of roles role #1 belongs
> > to, (4) the set of roles role #1 has ADMIN privilege on, (5) the set
> > of roles that role #2 belongs to, and (6) the set of roles that role
> > #2 has ADMIN privilege on. Maybe more?

> > I'd like to know precisely which combinations of these six things are
> > objectionable, and why. There may be a way around the objections
> > without needing to create new user options or new privileged roles.
>
> I can't speak for Noah, but my interpretation is that it would be
> surprising if GRANT/REVOKE or membership in an ordinary role had
> effects other than "permission denied" errors. It might make sense for
> event trigger firing in all the cases we can think of, but it would
> certainly be strange if we started accumulating a collection of
> behaviors that implicitly change when you move in or out of a role.
>
> That's pretty general, so to answer your question: it seems like a
> problem to use #3-6 in the calculation about whether to fire an event
> trigger.

Exactly. That's the main point. Also, it's currently a best practice for
only non-LOGIN roles to have members. The proposed approach invites folks to
abandon that best practice. I take the two smells as a sign that we'll regret
adopting the proposal, despite not knowing how it will go seriously wrong.

On Wed, Oct 20, 2021 at 12:09:08PM -0700, Jeff Davis wrote:
> On Thu, 2021-05-27 at 23:06 -0700, Noah Misch 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
>
> What do you mean "that way"? Do you mean it's not safe to delegate
> subscription creation to non-superusers at all?

I meant "pg_logical_replication would not be safe to delegate to the tenant of
a database provided as a service." It's not safe today, but it can be made
safe:

> From the thread above, I don't see anything so dangerous that it can't
> be delegated:
>
> * persistent background workers on subscriber
> - still seems reasonable to delegate to a privileged user

Agreed, I don't have a problem with pg_logical_replication implying that
ability. If you can create this worker, you can bypass ADMIN OPTION by
running the GRANT/REVOKE inside a subscription. That's probably fine if
documented, or else is_admin_of_role() could prevent it.

> * arbitrary code execution by the apply worker on subscriber
> - apply worker runs as subscription owner, so doesn't seem
> like a problem?

Sounds right. I think Mark Dilger drafted a patch to add ACL checks and a TAP
test confirming that the worker does get permission denied. That change has
no disadvantage, so this problem is on the way to getting solved.

> * connection info may be visible to non-superusers
> - seems either solvable or not necessarily a problem

Yes.

The other matter from the thread you linked is "the connection to the
publisher must enforce the equivalent of dblink_security_check()". I think
Mark Dilger drafted a patch for that, too.