Обсуждение: pg_dump needs SELECT privileges on irrelevant extension table

Hi all,

We have a situation where we need to revoke SELECT on a table that
belongs to our extension, and we also need to let less privileged users
dump the extension's external config tables. (The restricted table's
contents are exposed through a security_barrier view, and it's a cloud
environment where "admin" users don't necessarily have true superuser
access.)

Since the restricted table is internal, its contents aren't included in
dumps anyway, so we expected to be able to meet both use cases at once.
Unfortunately:

    $ pg_dump -U unprivileged_user -d postgres
    pg_dump: error: query failed: ERROR:  permission denied for relation
ext_table
    pg_dump: error: query was: LOCK TABLE public.ext_table IN ACCESS
SHARE MODE

...and there appears to be no way to work around this with
--exclude-table, since the table is part of the extension.

It looks like the only reason pg_dump locks this particular table is
because it's been marked with DUMP_COMPONENT_POLICY, which needs a lock
to ensure the consistency of later pg_get_expr() calls. That stings for
two reasons: 1) it doesn't seem like you need SELECT access on a table
to see its policies, and 2) we have no policies on the table anyway;
there are no pg_get_expr() calls to protect.

So I've attached the simplest backportable workaround I could think of:
unmark DUMP_COMPONENT_POLICY for a table that has no policies at the
time of the getTables() query. This is similar to the ACL optimization
that back branches do; it should ensure that there will be no
pg_get_expr() calls on pg_policy for that table later, due to
repeatable-read, and it omits the lock when there's no reason to grab
it. It won't fix the problem for tables that have do policies, but I
don't have any ideas for how to do that safely, unless there's some lock
mode that uses fewer privileges.

I also attached a speculative backport to 11 to illustrate what that
might look like, but first I have to convince you it's a bug. :)

WDYT?

Thanks,
--Jacob

Jacob Champion <jchampion@timescale.com> writes:
> We have a situation where we need to revoke SELECT on a table that
> belongs to our extension, and we also need to let less privileged users
> dump the extension's external config tables.

In general, we don't expect that random minimum-privilege users can do
a database-wide pg_dump, so I'm not entirely sure that I buy that this
is a case we should cater to.  Why shouldn't your dump user have enough
privilege to take this lock?

I'd be more willing to consider the proposed patch if it weren't such
a hack --- as you say, it doesn't fix the problem when the table has
policies, so it's hardly a general-purpose solution.  I fear that it's
also fairly expensive: adding sub-selects to the query we must do
before we can lock any tables is not appetizing, because making that
window wider adds to the risk of deadlocks, dump failures, etc.

            regards, tom lane

On Mon, Mar 20, 2023 at 10:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> In general, we don't expect that random minimum-privilege users can do
> a database-wide pg_dump, so I'm not entirely sure that I buy that this
> is a case we should cater to.

They're neither random nor minimum-privilege -- it's the role with the
most privileges available to our end users. They just can't see the
contents of this table.

> Why shouldn't your dump user have enough
> privilege to take this lock?

The table contains information that's confidential to the superuser.
Other users access it through a view.

> I'd be more willing to consider the proposed patch if it weren't such
> a hack --- as you say, it doesn't fix the problem when the table has
> policies, so it's hardly a general-purpose solution.

Right. Does a more general fix exist?

> I fear that it's
> also fairly expensive: adding sub-selects to the query we must do
> before we can lock any tables is not appetizing, because making that
> window wider adds to the risk of deadlocks, dump failures, etc.

I was hoping an EXISTS subselect would be cheap enough, but maybe I
don't have enough entries in pg_policy to see a slowdown. Any
suggestions on an order of magnitude so I can characterize it? Or
would you just like to know at what point I start seeing slower
behavior? (Alternatively: are there cheaper ways to write this query?)

Thanks!
--Jacob

On Mon, Mar 20, 2023 at 11:23 AM Jacob Champion <jchampion@timescale.com> wrote:
> On Mon, Mar 20, 2023 at 10:43 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I fear that it's
> > also fairly expensive: adding sub-selects to the query we must do
> > before we can lock any tables is not appetizing, because making that
> > window wider adds to the risk of deadlocks, dump failures, etc.
>
> I was hoping an EXISTS subselect would be cheap enough, but maybe I
> don't have enough entries in pg_policy to see a slowdown. Any
> suggestions on an order of magnitude so I can characterize it? Or
> would you just like to know at what point I start seeing slower
> behavior? (Alternatively: are there cheaper ways to write this query?)

As a smoke test, I have 10M policies spread across 100k tables on my
laptop (that is, 100 policies each). I also have 100k more empty
tables with no policies on them, to try to stress both sides of the
EXISTS. On PG11, the baseline query duration is roughly 20s; with the
patch, it increases to roughly 22s (~10% slowdown). Setup SQL
attached.

This appears to be tied to the number of policies more than the number
of tables; if I reduce it to "only" 1M policies, the slowdown drops to
~400ms (2%), and at 10k policies any difference is lost in noise. That
doesn't seem unreasonable to me, but I don't know what a worst-case
pg_policy catalog looks like.

--Jacob