Horton

Re: MAINTAIN privilege -- what do we need to un-revert it?

Jeff Davis <pgsql@j-davis.com>

View thread View on postgr.es
From: Jeff Davis <pgsql@j-davis.com>
To: Nathan Bossart <nathandbossart@gmail.com>
Cc: pgsql-hackers@postgresql.org, Joe Conway <mail@joeconway.com>, Robert Haas <robertmhaas@gmail.com>
Date: 2024-02-28T00:22:34Z
Lists: pgsql-hackers

Attachments

New version attached.

Do we need a documentation update here? If so, where would be a good
place?

On Fri, 2024-02-23 at 15:30 -0600, Nathan Bossart wrote:
> Why is this change needed?  Is the idea to make amcheck follow the
> same
> rules as maintenance commands to encourage folks to set up index
> functions
> correctly?

amcheck is calling functions it defined, so in order to find those
functions it needs to set the right search path.


> 
> What is the purpose of this [bootstrap-related] change?

DefineIndex() is called during bootstrap, and it's also a maintenance
command. I tried to handle the bootstrapping case, but I think it's
best to just guard it with a conditional. Done.

I also added Assert(!IsBootstrapProcessingMode()) in
assign_search_path().

> > +       SetConfigOption("search_path", GUC_SAFE_SEARCH_PATH,
> > PGC_USERSET,
> > +                                       PGC_S_SESSION);
> 
> I wonder if it's worth using PGC_S_INTERACTIVE or introducing a new
> value
> for these.
> > 

Did you have a particular concern about PGC_S_SESSION?

If it's less than PGC_S_SESSION, it won't work, because the caller's
SET command will override it, and the same manipulation is possible.

And I don't think we want it higher than PGC_S_SESSION, otherwise the
function can't set its own search_path, if needed.

> > +#define GUC_SAFE_SEARCH_PATH "pg_catalog, pg_temp"
> 
> Is including pg_temp actually safe?  I worry that a user could use
> their
> temporary schema to inject objects that would take the place of
> non-schema-qualified stuff in functions.

pg_temp cannot (currently) be excluded. If it is omitted from the
string, it will be placed *first* in the search_path, which is more
dangerous.

pg_temp does not take part in function or operator resolution, which
makes it safer than it first appears. There are potentially some risks
around tables, but it's not typical to access a table in a function
called as part of an index expression.

If we determine that pg_temp is actually unsafe to include, we need to
do something like what I proposed here:

https://www.postgresql.org/message-id/a6865db287596c9c6ea12bdd9de87216cb5e7902.camel@j-davis.com

before this change.

Regards,
	Jeff Davis

Commits

  1. doc: Note that CREATE MATERIALIZED VIEW restricts search_path.

  2. Small refactoring around ExecCreateTableAs().

  3. Add is_create parameter to RefreshMatviewByOid().

  4. Remove unused ParamListInfo argument from ExecRefreshMatView.

  5. When creating materialized views, use REFRESH to load data.

  6. Add missing RestrictSearchPath() calls.

  7. Fix search_path to a safe value during maintenance operations.

  8. Revert MAINTAIN privilege and pg_maintain predefined role.

  9. Avoid repeated name lookups during table and index DDL.

  10. Add a materialized view relations.