Re: CREATE FUNCTION ... SEARCH { DEFAULT | SYSTEM | SESSION }

On Tue, 2023-09-19 at 20:23 -0700, Maciek Sakrejda wrote:
> On Tue, Sep 19, 2023 at 5:56 PM Jeff Davis <pgsql(at)j-davis(dot)com> wrote:
> > ...
> > On Tue, 2023-09-19 at 11:41 -0400, Robert Haas wrote:
> > > That leads to a second idea, which is having it continue
> > > to be a GUC but only affect directly-entered SQL, with all
> > > indirectly-entered SQL either being stored as a node tree or
> > > having a
> > > search_path property attached somewhere.
> >
> > That's not too far from the proposed patch and I'd certainly be
> > interested to hear more and/or adapt my patch towards this idea.
>
> As an interested bystander, that's the same thing I was thinking when
> reading this. I reread your original e-mail, Jeff, and I still think
> that.

I have attached an updated patch. Changes:

* Syntax is now: SEARCH FROM { DEFAULT | TRUSTED | SESSION }
- added "FROM" to suggest that it's the source, and only a starting
place, rather than a specific and final setting. I don't feel strongly
about the FROM one way or another, so I can take it out if it's not
helpful.
- changed "SYSTEM" to "TRUSTED", which better reflects the purpose,
and doesn't suggest any connection to ALTER SYSTEM.
* Removed GUC -- we can reconsider this kind of thing later.
* ERROR if IMMUTABLE is combined with SEARCH FROM SESSION
* pg_dump support. Emits "SEARCH FROM SESSION" or "SEARCH FROM
TRUSTED" only if explicitly specified; otherwise emits no SEARCH
clause. Differentiating the unspecified cases may be useful for
migration purposes later.
* psql support.
* Updated docs to try to better present the concept, and document
CREATE PROCEDURE as well.

The SEARCH clause declares a new property that will be useful to both
enforce safety and also to guide users to migrate in a safe direction
over time.

For instance, the current patch prohibits the combination of IMMUTABLE
and SEARCH FROM SESSION; but allows IMMUTABLE if no SEARCH clause is
specified at all (to avoid breaking anything). We could extend that
slowly over several releases ratchet up the pressure (with warnings or
changing defaults) until all IMMUTABLE functions require SEARCH FROM
TRUSTED. Perhaps IMMUTABLE would even imply SEARCH FROM TRUSTED.

The search property is consistent with other properties, like
IMMUTABLE, which is both a marker and also enforces some restrictions
(e.g. you can't CREATE TABLE). It's also a lot nicer to use than a SET
clause, and provides a nice place to document certain behaviors.

(Aside: the concept of IMMUTABLE is basically broken today, due to
search_path problems.)

SEARCH FROM DEFAULT is just a way to get an object back to the
"unspecified search clause" state. It has the same behavior as SEARCH
FROM SESSION, except that the former will cause a hard error when
combined with IMMUTABLE. I think it's worth differentiating the
unspecified search clause from the explicit SEARCH FROM SESSION clause
for the purposes of migration.

There were three main complaints:

Comaplaint A: That it creates a new mechanism[1].

The patch doesn't create a new internal mechanism, it almost entirely
reuses the existing SET clause mechanism. I think complaint A is really
about the user-facing mechanics, which is essentially the same as the
complaint B.

Complaint B: That it's overlapping in functionality with the SET
clause[2][3]. In other words:

CREATE FUNCTION ... SEARCH FROM TRUSTED ...;
CREATE FUNCTION ... SET search_path = pg_catalog, pg_temp ...;

do similar things. But the latter is much worse:

* it's user-unfriendly (requiring pg_temp is highly unintuitive)
* it doesn't allow Postgres to warn if the function is being used in
an unsafe context
* there's no way to explicitly declare that you want the search path
to come from the session instead (either to be more clear about your
intentions, or to be forward-compatible)

In my opinion, the "SET search_path = ..." form should be used when you
actually want the search_path to contain some specific schema, not in
cases where you're just using built-in objects.

Complaint C: search_path is hopeless[4].

I think we can make meaningful improvements to the status quo, like
with the attached patch, that will reduce a lot of the surface area for
security risks. Right now our privilege model breaks down very quickly
even with trivial and typical use cases and we can do better.

--
Jeff Davis
PostgreSQL Contributor Team - AWS

[1]
https://www.postgresql.org/message-id/CA%2BTgmoaRPJJN%3DAOqC4b9t90vFQX81hKiXNPNhbxR0-Sm8F8nCA%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CA%2BTgmoah_bTjUFng-vZnivPQs0kQWUaSwAu49SU5M%2BzTxA%2B3Qw%40mail.gmail.com
[3]
https://www.postgresql.org/message-id/15464811-18fb-c7d4-4620-873366d367d6%40eisentraut.org
[4]
https://www.postgresql.org/message-id/20230812182559.d7plqwx3p65ys4i7%40awork3.anarazel.de