On Tue, Mar 28, 2023 at 6:48 PM Jeff Davis <pgsql@j-davis.com> wrote:
> That's not strictly true. See the example at the bottom of this email.
Yeah, we're very casual about repeated user switching in all kinds of
contexts, and that's really pretty scary. I don't know how to fix that
without removing capabilities that people rely on.
> The dangerous cases seem to be something along the lines of a security-
> invoker trigger function that builds and executes arbirary SQL based on
> the row contents. And then the table owner would then still need to set
> ENABLE ALWAYS TRIGGER.
>
> Do we really want to take that case on as our security responsibility?
That's something about which I would like to get more opinions.
> I believe switching to the table owner (as done in your patch) is the
> right best practice, and should be the default.
>
> I'm fine with an escape hatch here to ease migrations or whatever. But
> please do it in an unobtrusive way such that we (as hackers) can mostly
> forget that the non-switching behavior exists. At least for me, our
> system is already pretty complex without two kinds of subscription
> security models.
Here's a new patch set to show how this would work. 0001 is as before.
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.
Now, backing up a minute to address your other comments here, I'm not
particularly dedicated to this new option. But the problem is that you
can't please all the people all the time. When I proposed to
unconditionally change the behavior, people said "what if
SECURITY_RESTRICTED_OPERATION hoses people, or they just don't like
the new behavior?" and "what if I don't trust the publisher and want
to use table permissions on the subscriber side to minimize my
exposure?". Well, this option provides a way out of those problems by
allowing you to get the current behavior, but that gives rise to the
complaint you raise here: it's nicer to have one behavior than two. To
be honest, I'm pretty sympathetic to that complaint: i think the
problems if we don't add this switch are probably not going to affect
many people at all, and the new switch will therefore be of very
little value but will be something we continue to maintain forever.
However, I could be wrong and there's certainly something to be said
for having escape hatches that let people un-break stuff that core
patches break.
But you know we can't have it both ways. We either add an escape hatch
to make sure that people have a way out if they run into trouble and
incur the risk and complexity of carrying it probably forever, OR we
suck it up and make it a hard behavior change and tell people who are
upset "too bad, we changed it." And either way, at least PostgreSQL
hacker who has taken the time to write about this topic is going to
think we've made a poor call, or at best a dubious one, and either
way, there will probably be at least one user who cusses the
PostgreSQL development process out under their breath. I don't know
what to do about that. There's not a fix for this gaping security
problem that doesn't involve the potential of some people being
inconvenienced.
--
Robert Haas
EDB: http://www.enterprisedb.com
