Re: Per-function search_path => per-function GUC settings

Lists: pgsql-hackers
From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)postgreSQL(dot)org
Subject: Per-function search_path => per-function GUC settings
Date: 2007-09-01 16:41:28
Message-ID: 20583.1188664888@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

I believe we had consensus that 8.3 needs to include an easier way for a
function to set a local value of search_path, as proposed here:
http://archives.postgresql.org/pgsql-hackers/2007-03/msg01717.php
I've been holding off actually implementing that because adding a column
to pg_proc would create merge problems for pending patches. But tsearch
was the last one with any major changes to pg_proc.h, so it's probably
time to get on with it.

A few days ago, Simon suggested that we should generalize this notion
to allow per-function settings of any GUC variable:
http://archives.postgresql.org/pgsql-hackers/2007-08/msg01155.php
My reaction to that was more or less "D'oh, of course!" Stuff like
regex_flavor can easily break a function. So rather than thinking
only about search_path, it seems to me we should implement a facility
that allows function-local settings of any USERSET GUC variable, and
probably also SUSET ones if the function is SECURITY DEFINER and owned by
a superuser.

The most straightforward way to support this syntactically seems to
be to follow the per-user and per-database GUC setting features:

ALTER FUNCTION func(args) SET var = value
ALTER FUNCTION func(args) RESET var

The RESET alternative is a lot cleaner than my previous suggestion of
"PATH NONE" to remove a non-default path setting, anyway.

I thought about ways to include GUC settings directly into CREATE
FUNCTION, but it seemed pretty ugly and inconsistent with the
existing syntax. So I'm thinking of supporting only the above
syntaxes, meaning it'll take at least two commands to create a secure
SECURITY DEFINER function.

Comments?

regards, tom lane


From: "Brendan Jurd" <direvus(at)gmail(dot)com>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 17:36:41
Message-ID: 37ed240d0709011036g2e56925bvece78a3de0bf638c@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 9/2/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I thought about ways to include GUC settings directly into CREATE
> FUNCTION, but it seemed pretty ugly and inconsistent with the
> existing syntax. So I'm thinking of supporting only the above
> syntaxes, meaning it'll take at least two commands to create a secure
> SECURITY DEFINER function.

There's a niceness to being able to tell Postgres everything it needs
to know about a function in the one CREATE FUNCTION command.

So if we integrated the GUC settings into CREATE FUNCTION, we'd end up
writing something like

CREATE FUNCTION foo(int) RETURNS int AS $$
...
$$
LANGUAGE plpgsql
STABLE
STRICT
SECURITY DEFINER
RESET search_path
SET regex_flavor = 'cinnamon';

That doesn't seem especially horrible. In what way do you feel it is
inconsistent with existing syntax?

And ... although I'll admit this is a paranoid thing to mention, if
you have to fix the search_path setting *after* creating a function as
SECURITY DEFINER, then there is necessarily a short period of time
where the function exists and is insecure.

Cheers,
BJ


From: Gregory Stark <stark(at)enterprisedb(dot)com>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: <pgsql-hackers(at)postgreSQL(dot)org>
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 17:48:55
Message-ID: 87ps12jmyg.fsf@oxford.xeocode.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> I thought about ways to include GUC settings directly into CREATE
> FUNCTION, but it seemed pretty ugly and inconsistent with the
> existing syntax. So I'm thinking of supporting only the above
> syntaxes, meaning it'll take at least two commands to create a secure
> SECURITY DEFINER function.

I think security definer functions should automatically inherit their
search_path. The whole "secure by default" thing.

It might be best to have a guc variable which controls the variables which are
automatically saved. regexp_flavour and maybe a handful of others could be in
it by default. But that might depend on how expensive it is at run-time. I
wouldn't want trivial SQL functions to no longer be inline-able because one
might one day use a regexp for example.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: "Brendan Jurd" <direvus(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 17:55:06
Message-ID: 22560.1188669306@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

"Brendan Jurd" <direvus(at)gmail(dot)com> writes:
> CREATE FUNCTION foo(int) RETURNS int AS $$
> ...
> $$
> LANGUAGE plpgsql
> STABLE
> STRICT
> SECURITY DEFINER
> RESET search_path
> SET regex_flavor = 'cinnamon';

> That doesn't seem especially horrible. In what way do you feel it is
> inconsistent with existing syntax?

Hmm ... I hadn't thought of including SET in the syntax, so I was
running into problems with distingushing GUC variable names from the
keywords that are already in the syntax. That way would work from a
grammar point of view. It still seems a bit inconsistent to me, but
we could live with it. Comments anyone?

> And ... although I'll admit this is a paranoid thing to mention, if
> you have to fix the search_path setting *after* creating a function as
> SECURITY DEFINER, then there is necessarily a short period of time
> where the function exists and is insecure.

You already have that issue with respect to the default public execute
permissions on the function. The standard solution is to do it in a
transaction block --- then no one can even see the function until you
commit.

regards, tom lane


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Gregory Stark <stark(at)enterprisedb(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 18:05:50
Message-ID: 22752.1188669950@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> I think security definer functions should automatically inherit their
> search_path. The whole "secure by default" thing.

This assumes that the search path at creation time has something to do
with the path you'd like to use at execution, which is unlikely to be
the case in existing pg_dump output, to name one example. I don't
really want to get into doing the above.

> It might be best to have a guc variable which controls the variables which are
> automatically saved. regexp_flavour and maybe a handful of others could be in
> it by default. But that might depend on how expensive it is at run-time. I
> wouldn't want trivial SQL functions to no longer be inline-able because one
> might one day use a regexp for example.

Well, security definer functions can't be inlined anyway, so as long as
you only try to do this for sec-def functions it wouldn't be an issue.
I just think it's too big a departure from existing behavior, and will
probably break more things than it fixes.

regards, tom lane


From: Gregory Stark <stark(at)enterprisedb(dot)com>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 18:31:38
Message-ID: 87lkbqjkz9.fsf@oxford.xeocode.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

"Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:

> Gregory Stark <stark(at)enterprisedb(dot)com> writes:
>> I think security definer functions should automatically inherit their
>> search_path. The whole "secure by default" thing.
>
> This assumes that the search path at creation time has something to do
> with the path you'd like to use at execution, which is unlikely to be
> the case in existing pg_dump output, to name one example. I don't
> really want to get into doing the above.

pg_dump will have to do a ALTER FUNCTION SET command anyways, no? So the
default search_path that gets saved doesn't really matter. In general if it's
not the search path you want at run-time you just have to change it, but you
should always have *something* set or else it's a wide open security hole.

I'm not clear why the search path at creation time is such a bad choice
anyways, it is security "definer", what's the difference between taking the
userid from the defining environment and taking the search path from the
defining environment?

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com


From: Jeff Davis <pgsql(at)j-davis(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgreSQL(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 18:40:04
Message-ID: 1188672004.4144.43.camel@jdavis
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sat, 2007-09-01 at 12:41 -0400, Tom Lane wrote:
> A few days ago, Simon suggested that we should generalize this notion
> to allow per-function settings of any GUC variable:
> http://archives.postgresql.org/pgsql-hackers/2007-08/msg01155.php
> My reaction to that was more or less "D'oh, of course!" Stuff like
> regex_flavor can easily break a function. So rather than thinking
> only about search_path, it seems to me we should implement a facility
> that allows function-local settings of any USERSET GUC variable, and
> probably also SUSET ones if the function is SECURITY DEFINER and owned by
> a superuser.
>
> The most straightforward way to support this syntactically seems to
> be to follow the per-user and per-database GUC setting features:
>
> ALTER FUNCTION func(args) SET var = value
>

I was hoping this feature would make it easier for modules to install
into any schema. Right now a module can either preprocess a SQL install
script to install it into the schema you want, or hard-code the schema
into the file and you're stuck with whatever schema the module authors
chose (usually either the module name, or "public").

Can we also provide syntax which would be equivalent to setting "var"
for the function to be whatever the current value happens to be when the
ALTER FUNCTION is run? Possible syntax might be something like:

ALTER FUNCTION func(args) SET var TO CURRENT;

> I thought about ways to include GUC settings directly into CREATE
> FUNCTION, but it seemed pretty ugly and inconsistent with the
> existing syntax. So I'm thinking of supporting only the above
> syntaxes, meaning it'll take at least two commands to create a secure
> SECURITY DEFINER function.

That seems a little awkward, because to avoid a security race condition,
you'd have to wrap the CREATE/ALTER in a transaction block. However, we
already have a similar situation with creating a security definer
function and then revoking access, so maybe it's already expected.

I don't have a strong opinion, I just wanted to bring that up.

Regards,
Jeff Davis


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Jeff Davis <pgsql(at)j-davis(dot)com>
Cc: pgsql-hackers(at)postgreSQL(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 19:03:14
Message-ID: 24343.1188673394@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> Can we also provide syntax which would be equivalent to setting "var"
> for the function to be whatever the current value happens to be when the
> ALTER FUNCTION is run? Possible syntax might be something like:

> ALTER FUNCTION func(args) SET var TO CURRENT;

Hmmm ... that's certainly do-able, though I'm not sure how much it helps
the use-case you suggest. The search path still has to be set at the
top of the module script, no?

However, I like an explicit option of this sort a lot better than the
automatic version Greg was suggesting ... I'm willing to do it if people
want it.

One problem is that we'd have to make CURRENT a reserved word to make it
work exactly like that. Can anyone think of a variant syntax that
doesn't need a new reserved word?

regards, tom lane


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Gregory Stark <stark(at)enterprisedb(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 19:12:44
Message-ID: 24544.1188673964@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Gregory Stark <stark(at)enterprisedb(dot)com> writes:
> "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us> writes:
>> This assumes that the search path at creation time has something to do
>> with the path you'd like to use at execution, which is unlikely to be
>> the case in existing pg_dump output, to name one example. I don't
>> really want to get into doing the above.

> pg_dump will have to do a ALTER FUNCTION SET command anyways, no?

You're missing the point: this change will break existing pg_dump
output, because pg_dump feels free to set the search_path for its own
purposes. The same is true of other GUC variables that might be
automatically absorbed into CREATE FUNCTION: there is not any very good
reason to suppose that their values when the dump is restored are really
what should be used. The argument is slightly more credible with
respect to interactively-issued commands, but for pg_dump it's simply
wrong.

What we might change pg_dump to do in future is a separate topic,
but we can't make that kind of change in the semantics of existing
dumps.

regards, tom lane


From: "Josh Tolley" <eggyknap(at)gmail(dot)com>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: "Jeff Davis" <pgsql(at)j-davis(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 19:17:11
Message-ID: e7e0a2570709011217v5abffd5eqe5b5f9124baa650@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 9/1/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> > Can we also provide syntax which would be equivalent to setting "var"
> > for the function to be whatever the current value happens to be when the
> > ALTER FUNCTION is run? Possible syntax might be something like:
>
> > ALTER FUNCTION func(args) SET var TO CURRENT;
>
> Hmmm ... that's certainly do-able, though I'm not sure how much it helps
> the use-case you suggest. The search path still has to be set at the
> top of the module script, no?
>
> However, I like an explicit option of this sort a lot better than the
> automatic version Greg was suggesting ... I'm willing to do it if people
> want it.
>
> One problem is that we'd have to make CURRENT a reserved word to make it
> work exactly like that. Can anyone think of a variant syntax that
> doesn't need a new reserved word?
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 2: Don't 'kill -9' the postmaster
>

+1 for being able to define the entire function in one command, +1 for
not inheriting a bunch of GUC settings without the definer's explicit
say-so.

- Josh/Eggyknap


From: John DeSoi <desoi(at)pgedit(dot)com>
To: Brendan Jurd <direvus(at)gmail(dot)com>
Cc: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 19:56:29
Message-ID: 0FDD47E1-D9F7-4479-B829-32947EBC7DB4@pgedit.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On Sep 1, 2007, at 1:36 PM, Brendan Jurd wrote:

> So if we integrated the GUC settings into CREATE FUNCTION, we'd end up
> writing something like
>
> CREATE FUNCTION foo(int) RETURNS int AS $$
> ...
> $$
> LANGUAGE plpgsql
> STABLE
> STRICT
> SECURITY DEFINER
> RESET search_path
> SET regex_flavor = 'cinnamon';
>
> That doesn't seem especially horrible. In what way do you feel it is
> inconsistent with existing syntax?

I like this and would really like to see way to set everything using
CREATE FUNCTION. Using ALTER only requires maintaining a separate
copy of of function arguments which can be a hassle for large
argument lists.

John DeSoi, Ph.D.
http://pgedit.com/
Power Tools for PostgreSQL


From: Decibel! <decibel(at)decibel(dot)org>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Jeff Davis <pgsql(at)j-davis(dot)com>, pgsql-hackers(at)postgreSQL(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 22:23:55
Message-ID: 20070901222355.GT38801@decibel.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sat, Sep 01, 2007 at 03:03:14PM -0400, Tom Lane wrote:
> Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> > Can we also provide syntax which would be equivalent to setting "var"
> > for the function to be whatever the current value happens to be when the
> > ALTER FUNCTION is run? Possible syntax might be something like:
>
> > ALTER FUNCTION func(args) SET var TO CURRENT;
>
> Hmmm ... that's certainly do-able, though I'm not sure how much it helps
> the use-case you suggest. The search path still has to be set at the
> top of the module script, no?

Better one place than scores of places...

> However, I like an explicit option of this sort a lot better than the
> automatic version Greg was suggesting ... I'm willing to do it if people
> want it.
>
> One problem is that we'd have to make CURRENT a reserved word to make it
> work exactly like that. Can anyone think of a variant syntax that
> doesn't need a new reserved word?

I'm not good at the synonym game, but I did have an idea related to the
SET syntax, so I'll just post it here...

If you needed to set a bunch of GUCs on a function, having a load of SET
statements might be a bit tedious... I'm wondering if there's some way
to specify them like an array?
SET {'search_path=CURRENT', 'enable_seqscan=false', ...}

Or now that we have arrays of complex types, perhaps an array of type
GUC...

Either case would at least take care of CURRENT...
--
Decibel!, aka Jim Nasby decibel(at)decibel(dot)org
EnterpriseDB http://enterprisedb.com 512.569.9461 (cell)


From: "Marko Kreen" <markokr(at)gmail(dot)com>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: "Jeff Davis" <pgsql(at)j-davis(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 22:37:35
Message-ID: e51f66da0709011537g34bc45c5g5169ffe49c92f7c7@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 9/1/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Jeff Davis <pgsql(at)j-davis(dot)com> writes:
> > Can we also provide syntax which would be equivalent to setting "var"
> > for the function to be whatever the current value happens to be when the
> > ALTER FUNCTION is run? Possible syntax might be something like:
>
> > ALTER FUNCTION func(args) SET var TO CURRENT;
>
> Hmmm ... that's certainly do-able, though I'm not sure how much it helps
> the use-case you suggest. The search path still has to be set at the
> top of the module script, no?
>
> However, I like an explicit option of this sort a lot better than the
> automatic version Greg was suggesting ... I'm willing to do it if people
> want it.
>
> One problem is that we'd have to make CURRENT a reserved word to make it
> work exactly like that. Can anyone think of a variant syntax that
> doesn't need a new reserved word?

SET var FROM CURRENT SESSION

--
marko


From: David Fetter <david(at)fetter(dot)org>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgreSQL(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 22:52:29
Message-ID: 20070901225229.GA19705@fetter.org
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sat, Sep 01, 2007 at 12:41:28PM -0400, Tom Lane wrote:
> I believe we had consensus that 8.3 needs to include an easier way for a
> function to set a local value of search_path, as proposed here:
> http://archives.postgresql.org/pgsql-hackers/2007-03/msg01717.php
> I've been holding off actually implementing that because adding a column
> to pg_proc would create merge problems for pending patches. But tsearch
> was the last one with any major changes to pg_proc.h, so it's probably
> time to get on with it.
>
> A few days ago, Simon suggested that we should generalize this notion
> to allow per-function settings of any GUC variable:
> http://archives.postgresql.org/pgsql-hackers/2007-08/msg01155.php
> My reaction to that was more or less "D'oh, of course!" Stuff like
> regex_flavor can easily break a function. So rather than thinking
> only about search_path, it seems to me we should implement a facility
> that allows function-local settings of any USERSET GUC variable, and
> probably also SUSET ones if the function is SECURITY DEFINER and owned by
> a superuser.
>
> The most straightforward way to support this syntactically seems to
> be to follow the per-user and per-database GUC setting features:
>
> ALTER FUNCTION func(args) SET var = value

Would it be hard to extend this into this?

ALTER FUNCTION func(args) SET var = value [, var = value ...]

> ALTER FUNCTION func(args) RESET var
>
> The RESET alternative is a lot cleaner than my previous suggestion
> of "PATH NONE" to remove a non-default path setting, anyway.
>
> I thought about ways to include GUC settings directly into CREATE
> FUNCTION, but it seemed pretty ugly and inconsistent with the
> existing syntax. So I'm thinking of supporting only the above
> syntaxes, meaning it'll take at least two commands to create a
> secure SECURITY DEFINER function.

With the extended syntax I proposed it could take just one command to
create such a function :)

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
phone: +1 415 235 3778 AIM: dfetter666
Skype: davidfetter

Remember to vote!
Consider donating to PostgreSQL: http://www.postgresql.org/about/donate


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: David Fetter <david(at)fetter(dot)org>
Cc: pgsql-hackers(at)postgreSQL(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 23:44:26
Message-ID: 18607.1188690266@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

David Fetter <david(at)fetter(dot)org> writes:
> On Sat, Sep 01, 2007 at 12:41:28PM -0400, Tom Lane wrote:
>> The most straightforward way to support this syntactically seems to
>> be to follow the per-user and per-database GUC setting features:
>>
>> ALTER FUNCTION func(args) SET var = value

> Would it be hard to extend this into this?
> ALTER FUNCTION func(args) SET var = value [, var = value ...]

Actually, it would be hard *not* to, because of the way the CREATE/ALTER
FUNCTION syntax is already set up --- but without the commas. I've got
it working now with SET and RESET as alternatives for
common_func_opt_item.

regards, tom lane


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: "Marko Kreen" <markokr(at)gmail(dot)com>
Cc: "Jeff Davis" <pgsql(at)j-davis(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-01 23:47:20
Message-ID: 18674.1188690440@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

"Marko Kreen" <markokr(at)gmail(dot)com> writes:
> On 9/1/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> One problem is that we'd have to make CURRENT a reserved word to make it
>> work exactly like that. Can anyone think of a variant syntax that
>> doesn't need a new reserved word?

> SET var FROM CURRENT SESSION

Seems a little verbose, but maybe we could do "SET var FROM CURRENT"
or "SET var FROM SESSION"?

One point worth noting here is that this'd more or less automatically
apply to ALTER USER SET and ALTER DATABASE SET as well ... not sure
how much use-case there is for those, but it'd fall out ...

regards, tom lane


From: "Marko Kreen" <markokr(at)gmail(dot)com>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: "Jeff Davis" <pgsql(at)j-davis(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-02 15:42:08
Message-ID: e51f66da0709020842l1ee5040er317a0eb525e3a74c@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 9/2/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Marko Kreen" <markokr(at)gmail(dot)com> writes:
> > On 9/1/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> One problem is that we'd have to make CURRENT a reserved word to make it
> >> work exactly like that. Can anyone think of a variant syntax that
> >> doesn't need a new reserved word?
>
> > SET var FROM CURRENT SESSION
>
> Seems a little verbose, but maybe we could do "SET var FROM CURRENT"
> or "SET var FROM SESSION"?

I'd prefer FROM SESSION then. FROM CURRENT seems unclear.

> One point worth noting here is that this'd more or less automatically
> apply to ALTER USER SET and ALTER DATABASE SET as well ... not sure
> how much use-case there is for those, but it'd fall out ...

Does not seem to be a problem.

--
marko


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: "Marko Kreen" <markokr(at)gmail(dot)com>
Cc: "Jeff Davis" <pgsql(at)j-davis(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-02 16:11:19
Message-ID: 7939.1188749479@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

"Marko Kreen" <markokr(at)gmail(dot)com> writes:
> On 9/2/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Seems a little verbose, but maybe we could do "SET var FROM CURRENT"
>> or "SET var FROM SESSION"?

> I'd prefer FROM SESSION then. FROM CURRENT seems unclear.

Actually, I think FROM SESSION is unclear, as it opens the question
whether the value to be applied is the session-wide setting or the
currently active one. Inside a transaction that has done SET LOCAL,
these are different things.

I think we pretty clearly want to have it take the currently active
setting, and I'd vote for FROM CURRENT as the best way of expressing
that.

regards, tom lane


From: Jeff Davis <pgsql(at)j-davis(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Marko Kreen <markokr(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-02 17:54:52
Message-ID: 1188755692.4144.48.camel@jdavis
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sun, 2007-09-02 at 12:11 -0400, Tom Lane wrote:
> I think we pretty clearly want to have it take the currently active
> setting, and I'd vote for FROM CURRENT as the best way of expressing
> that.

FROM CURRENT sounds good to me.

Another idea (just brainstorming): SET var AS CURRENT.

Regards,
Jeff Davis


From: Jeff Davis <pgsql(at)j-davis(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgreSQL(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-02 18:03:42
Message-ID: 1188756222.4144.56.camel@jdavis
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sat, 2007-09-01 at 15:03 -0400, Tom Lane wrote:
> > ALTER FUNCTION func(args) SET var TO CURRENT;
>
> Hmmm ... that's certainly do-able, though I'm not sure how much it helps
> the use-case you suggest. The search path still has to be set at the
> top of the module script, no?

It gives some better options for module authors and people installing
those modules:

1. Set it at the top of the file, in one place

2. Connect as the user whose schema you want to install into, i.e.:
$ psql my_db jdavis < module_install.sql

3. prepend the "SET search_path=foo" to the input of psql, i.e.:
$ echo "SET search_path=foo;" | cat module_install.sql | psql my_db
..or
$ psql my_db
=> SET search_path=foo;
=> \i module_install.sql

Regards,
Jeff Davis


From: Jeff Davis <pgsql(at)j-davis(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Brendan Jurd <direvus(at)gmail(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-02 18:29:06
Message-ID: 1188757746.4144.74.camel@jdavis
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sat, 2007-09-01 at 13:55 -0400, Tom Lane wrote:
> You already have that issue with respect to the default public execute
> permissions on the function. The standard solution is to do it in a
> transaction block --- then no one can even see the function until you
> commit.

It might be a good idea to have the ability to revoke privileges at
CREATE FUNCTION time also.

That could clutter up the CREATE FUNCTION syntax, but would offer an
opportunity to document the danger of default public execute in a
SECURITY DEFINER function.

Something like:
CREATE FUNCTION ...
LANGUAGE plpgsql
SECURITY DEFINER
REVOKE EXECUTE FROM PUBLIC;

Even if we don't do that, we should at least document your standard
solution here:
http://www.postgresql.org/docs/8.2/static/sql-createfunction.html

It is already documented here:
http://www.postgresql.org/docs/8.2/static/sql-grant.html

But the CREATE FUNCTION page has a section titled "Writing SECURITY
DEFINER Functions Safely", so I think it's useful to have it there, too.

Regards,
Jeff Davis


From: "Marko Kreen" <markokr(at)gmail(dot)com>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: "Jeff Davis" <pgsql(at)j-davis(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-03 07:55:24
Message-ID: e51f66da0709030055n3cfbc9a4qe00097fc097f6ddc@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 9/2/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Marko Kreen" <markokr(at)gmail(dot)com> writes:
> > On 9/2/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> >> Seems a little verbose, but maybe we could do "SET var FROM CURRENT"
> >> or "SET var FROM SESSION"?
>
> > I'd prefer FROM SESSION then. FROM CURRENT seems unclear.
>
> Actually, I think FROM SESSION is unclear, as it opens the question
> whether the value to be applied is the session-wide setting or the
> currently active one. Inside a transaction that has done SET LOCAL,
> these are different things.

I did not consider that.

> I think we pretty clearly want to have it take the currently active
> setting, and I'd vote for FROM CURRENT as the best way of expressing
> that.

Ok.

--
marko


From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-11 09:07:21
Message-ID: 46E65AC9.5050101@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Tom Lane wrote:

>
> I thought about ways to include GUC settings directly into CREATE
> FUNCTION, but it seemed pretty ugly and inconsistent with the
> existing syntax. So I'm thinking of supporting only the above
> syntaxes, meaning it'll take at least two commands to create a secure
> SECURITY DEFINER function.
>
> Comments?

I have a question about what does happen if search path is not defined
for SECURITY DEFINER function. My expectation is that SECURITY DEFINER
function should defined empty search patch in this case. This behavior
is similar to how dynamic linker processes setuid binaries - (ignoring
LD_LIBRARY_PATH and so on).

Zdenek


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-11 13:53:24
Message-ID: 27428.1189518804@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM> writes:
> I have a question about what does happen if search path is not defined
> for SECURITY DEFINER function. My expectation is that SECURITY DEFINER
> function should defined empty search patch in this case.

Your expectation is incorrect. We are not in the business of breaking
every application in sight, which is what that would do. Nor do I think
it's a good plan to try to be smarter than the programmer.

regards, tom lane


From: Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-11 14:57:01
Message-ID: 46E6ACBD.4030707@sun.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Tom Lane wrote:
> Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM> writes:
>> I have a question about what does happen if search path is not defined
>> for SECURITY DEFINER function. My expectation is that SECURITY DEFINER
>> function should defined empty search patch in this case.
>
> Your expectation is incorrect. We are not in the business of breaking
> every application in sight, which is what that would do.

Oh. I see. In this point of view I suggest to add some warning about
potential security issue if SECURITY DEFINER function will create
without preset search_path. I'm aware that a lot of developer forget to
modify their application.

Zdenek


From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: pgsql-hackers(at)postgresql(dot)org
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Zdenek Kotala <Zdenek(dot)Kotala(at)sun(dot)com>
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-11 16:22:34
Message-ID: 200709111822.35061.peter_e@gmx.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Am Dienstag, 11. September 2007 15:53 schrieb Tom Lane:
> Zdenek Kotala <Zdenek(dot)Kotala(at)Sun(dot)COM> writes:
> > I have a question about what does happen if search path is not defined
> > for SECURITY DEFINER function. My expectation is that SECURITY DEFINER
> > function should defined empty search patch in this case.
>
> Your expectation is incorrect. We are not in the business of breaking
> every application in sight, which is what that would do.

Well, a SECURITY DEFINER function either sets its own search path, in which
case a default search path would have no effect, or it doesn't set its own
search path, in which case it's already broken (albeit in a different way).
So setting a default search path can only be a net gain.

--
Peter Eisentraut
http://developer.postgresql.org/~petere/


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers(at)postgresql(dot)org, Zdenek Kotala <Zdenek(dot)Kotala(at)sun(dot)com>
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-11 17:06:47
Message-ID: 13610.1189530407@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Peter Eisentraut <peter_e(at)gmx(dot)net> writes:
> Well, a SECURITY DEFINER function either sets its own search path, in which
> case a default search path would have no effect, or it doesn't set its own
> search path, in which case it's already broken (albeit in a different way).
> So setting a default search path can only be a net gain.

It would break functions that actually want to use a caller-specified
search path, and protect themselves by explicitly schema-qualifying
every other reference than one to some caller-specified object. Which
admittedly is notationally a pain in the neck, but it's possible to do.
I do not think that we should foreclose potentially useful behavior
*and* make a major break in backward compatibility in order to make
a very small improvement in security.

regards, tom lane


From: "Brendan Jurd" <direvus(at)gmail(dot)com>
To: "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: "Peter Eisentraut" <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org, "Zdenek Kotala" <Zdenek(dot)Kotala(at)sun(dot)com>
Subject: Re: Per-function search_path => per-function GUC settings
Date: 2007-09-12 03:29:31
Message-ID: 37ed240d0709112029l15222ed7vde06094d9bbff01b@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On 9/12/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> It would break functions that actually want to use a caller-specified
> search path, and protect themselves by explicitly schema-qualifying
> every other reference than one to some caller-specified object. Which
> admittedly is notationally a pain in the neck, but it's possible to do.
> I do not think that we should foreclose potentially useful behavior
> *and* make a major break in backward compatibility in order to make
> a very small improvement in security.

In that case, is there anything wrong with Zdenek's suggestion to add
a warning on SECURITY DEFINER functions that do not set a search_path?

Something to the tune of

WARNING: "Your function is defined with SECURITY DEFINER but does not
specify a local search path. This is potentially a serious security
vulnerability."
HINT: "Use the SET clause in CREATE FUNCTION to set a safe search path
which is specific to your function."