Re: running logical replication as the subscription owner - Mailing list pgsql-hackers

On Thu, 30 Mar 2023 at 15:42, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis <pgsql@j-davis.com> wrote:
> > I say just take the special case out of 0001. If the trigger doesn't
> > work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED ALWAYS,
> > then the user can just use the new option in 0002 to get the old
> > behavior. I don't see a reason to implicitly give them the old
> > behavior, as 0001 does.
>
> Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET
> ROLE to A. With the patch as written, actions on B's tables are not
> confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on C's
> tables are.

I think that's fair. There's no need to set
SECURITY_RESTRICTED_OPERATION if it doesn't protect you anyway, and
indeed it might break things. To be clear I do think it's important to
still switch to the table owner, simply for consistency. But that's
done in the patch so that's fine.

> I agree that the naming is somewhat problematic, but I don't like
> trust_table_owners. It's not clear enough about what actually happens.
> I want the name to describe behavior, not sentiment.

For security related things, I think sentiment is often just as
important as the actual behaviour. It's not without reason that newer
javascript frameworks have things like dangerouslySetInnerHTML, to
scare people away from using it unless they know what they are doing.

> I don't think run_as_owner is terrible, despite the ambiguity. It's
> talking about the owner of the object on which the property is being
> set. Isn't that the most natural interpretation? I'd be pretty
> surprised if I set a property called run_as_owner on object A and it
> ran as the owner of some other object B.

I think that's fair and I'd be happy with run_as_owner. If someone
doesn't understand which owner, they should probably check the
documentation anyways to understand the implications.

Regarding the actual patch. I think the code looks good. Mainly the
tests and docs are lacking for the new option. Like I said for the
tests you can borrow the tests I updated for my v2 patch, I think
those should work fine for the new option.

On Thu, 30 Mar 2023 at 15:42, Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Mar 30, 2023 at 1:19 AM Jeff Davis <pgsql@j-davis.com> wrote:
> > I say just take the special case out of 0001. If the trigger doesn't
> > work as a SECURITY_RESTRICTED_OPERATION, and is also ENABLED ALWAYS,
> > then the user can just use the new option in 0002 to get the old
> > behavior. I don't see a reason to implicitly give them the old
> > behavior, as 0001 does.
>
> Mmm, I don't agree. Suppose A can SET ROLE to B or C, and B can SET
> ROLE to A. With the patch as written, actions on B's tables are not
> confined by the SECURITY_RESTRICTED_OPERATION flag, but actions on C's
> tables are.
>
> I think we want to do everything possible to avoid people feeling like
> they need to turn on this new option. I'm not sure we'll ever be able
> to get rid of it, but we certainly should avoid doing things that make
> it more likely that it will be needed.
>
> > > 0002 adds a run_as_owner option to subscriptions. This doesn't make
> > > the updated regression tests fail, because they don't use it. If you
> > > revert the changes to 027_nosuperuser.pl then you get failures (as
> > > one
> > > would hope) and if you then add WITH (run_as_owner = true) to the
> > > CREATE SUBSCRIPTION command in 027_nosuperuser.pl then it passes
> > > again. I need to spend some more time thinking about what the tests
> > > actually ought to look like if we go this route -- I haven't looked
> > > through what Jelte proposed yet -- and also the documentation would
> > > need a bunch of updating.
> >
> > "run_as_owner" is ambiguous -- subscription owner or table owner?
> >
> > I would prefer something like "trust_table_owners". That would
> > communicate that the user shouldn't choose it unless they know what
> > they're doing.
>
> I agree that the naming is somewhat problematic, but I don't like
> trust_table_owners. It's not clear enough about what actually happens.
> I want the name to describe behavior, not sentiment.
>
> run_as_subscription_owner removes the ambiguity, but is long.
>
> run_as_table_owner is a bit shorter, and we could do that with the
> sense reversed. Is that equally clear? I'm not sure.
>
> I can think of other alternatives, like user_switching or
> switch_to_table_owner or no_user_switching or various other things,
> but none of them seem very good to me.
>
> Another idea could be to make the option non-Boolean. This is
> comically long and I can't seriously recommend it, but just to
> illustrate the point, if you type CREATE SUBSCRIPTION ... WITH
> (execute_code_as_owner_of_which_object = subscription) then you
> certainly should know what you've signed up for! If there were a
> shorter version that were still clear, I could go for that, but I'm
> having trouble coming up with exact wording.
>
> I don't think run_as_owner is terrible, despite the ambiguity. It's
> talking about the owner of the object on which the property is being
> set. Isn't that the most natural interpretation? I'd be pretty
> surprised if I set a property called run_as_owner on object A and it
> ran as the owner of some other object B. I think our notion of how
> ambiguous this is may be somewhat inflated by the fact that we've just
> had a huge conversation about whether it should be the table owner or
> the subscription owner, so those possibilities are etched in our mind
> in a way that maybe they won't be for people coming at this fresh. But
> it's hard to be sure what other people will think about something, and
> I don't want to be too optimistic about the name I picked, either.
>
> > If you are worried that *I* think 0002 would be a poor call, then no, I
> > don't. Initially I didn't like the idea of supporting two behaviors
> > (and who would?), but we probably can't avoid it at least for right
> > now.
>
> OK, good. Then we have a way forward that nobody's too upset about.
>
> --
> Robert Haas
> EDB: http://www.enterprisedb.com
>
>