Re: Privileges and inheritance

Lists: pgsql-hackers
From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Privileges and inheritance
Date: 2009-10-03 06:45:55
Message-ID: 1254552355.10783.17.camel@fsopti579.F-Secure.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

I would like to propose a change in the way privilege checking is done
with inheritance hierarchies. Currently, selecting from a table that
has children requires explicit privileges on all the children. This is
inconsistent with all other commands, which treat children as implicitly
part of the parent table. (Arguably, it exposes an implementation
detail, since you could just as well implement inheritance by keeping
all the children's data for the inherited columns in the parent's heap.)

As inheritance has now found new popularity as a partitioning mechanism,
this exacerbates the annoyance because you have to copy the privilege
sets to possibly dozens or hundreds of subtables in cumbersome ways for
really no good reason.

If you use inheritance for data modeling (the original purpose), you
face another problem. Either you grant table privileges on all the
child tables, thus giving users access to more information than they
were supposed to have, or you grant column privileges on only those
columns that were inherited, being careful to keep that set updated
whenever table definitions are altered. (Before 8.4 you couldn't even
do that.) It's messy.

So let's get rid of that. Selecting (or in general, operating) on a
table with children only checks the privileges on that table, not the
children. Is there any use case where the current behavior is useful at
all? (What I gather from history and the code, I think it was just
forgotten to change this when we switched away from the table* syntax
way back when.) FWIW, changing this behavior would also be more
SQL-conforming.

We could use a GUC variable to ease the transition, perhaps like
sql_inheritance = no | yes_without_privileges | yes

Comments?


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
Subject: Re: Privileges and inheritance
Date: 2009-10-03 14:45:22
Message-ID: 102.1254581122@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:
> So let's get rid of that. Selecting (or in general, operating) on a
> table with children only checks the privileges on that table, not the
> children.

+1

> We could use a GUC variable to ease the transition, perhaps like
> sql_inheritance = no | yes_without_privileges | yes

If we're gonna do it, let's just do it. I think adding a GUC just
complicates matters, especially since it would have to be superuser-only
(and thus effectively installation-wide). There would also be issues
with when it takes effect. The only simple way to implement this is
going to be to modify the planner's expansion of the range table, but
privilege checks should happen at execution; so the GUC would take
effect at the wrong time.

regards, tom lane


From: Josh Berkus <josh(at)agliodbs(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-04 18:56:01
Message-ID: 4AC8EFC1.1050209@agliodbs.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


> So let's get rid of that. Selecting (or in general, operating) on a
> table with children only checks the privileges on that table, not the
> children. Is there any use case where the current behavior is useful at
> all?

In theory, someone out there may be using privs to restrict access to
child tables. In practice, this would be unmanageable enough that I
doubt anyone is doing it intentionally.

Except ... I can imagine a multi-tenant setup where certain ROLEs only
have permissions on some child relations, but not others. So we'd want
to still enable a permissions check on a child when the child is called
directly rather than through the parent.

And we'd want to hammer this to death looking for ways it can be a
security exploit. Like, could you make a table into the parent of an
existing table you didn't have permissions on?

> We could use a GUC variable to ease the transition, perhaps like
> sql_inheritance = no | yes_without_privileges | yes

no | without_privileges | yes

Mind you, this is a boolean now, isn't it?

--Josh Berkus


From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: Josh Berkus <josh(at)agliodbs(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-04 19:57:30
Message-ID: 1254686250.13655.7.camel@vanquo.pezone.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sun, 2009-10-04 at 11:56 -0700, Josh Berkus wrote:
> Except ... I can imagine a multi-tenant setup where certain ROLEs only
> have permissions on some child relations, but not others. So we'd want
> to still enable a permissions check on a child when the child is called
> directly rather than through the parent.

Well, when you access the child, it doesn't care whether it has a
parent. So this is equivalent to checking permissions before accessing
a table, period. I think we'll keep that. ;-)

> And we'd want to hammer this to death looking for ways it can be a
> security exploit. Like, could you make a table into the parent of an
> existing table you didn't have permissions on?

I don't think so, but you're free to hammer. ;-)


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: Josh Berkus <josh(at)agliodbs(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 01:54:13
Message-ID: 17029.1254707653@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:
> On Sun, 2009-10-04 at 11:56 -0700, Josh Berkus wrote:
>> And we'd want to hammer this to death looking for ways it can be a
>> security exploit. Like, could you make a table into the parent of an
>> existing table you didn't have permissions on?

> I don't think so, but you're free to hammer. ;-)

I believe you have to be owner of both tables to do an ALTER INHERIT.
So you would have the right to make the child more accessible than it
had been. Whether you realized you were doing that might be a bit
debatable ... but I don't seriously think this is a problem.

regards, tom lane


From: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 03:15:02
Message-ID: 4AC964B6.1020609@ak.jp.nec.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Peter Eisentraut wrote:
> I would like to propose a change in the way privilege checking is done
> with inheritance hierarchies. Currently, selecting from a table that
> has children requires explicit privileges on all the children. This is
> inconsistent with all other commands, which treat children as implicitly
> part of the parent table. (Arguably, it exposes an implementation
> detail, since you could just as well implement inheritance by keeping
> all the children's data for the inherited columns in the parent's heap.)
>
> As inheritance has now found new popularity as a partitioning mechanism,
> this exacerbates the annoyance because you have to copy the privilege
> sets to possibly dozens or hundreds of subtables in cumbersome ways for
> really no good reason.

I think it is a matter of perspectives.
(So, we will not have a perfectly correct answer.)

If we consider that inherited columns are a part of parent table,
it is odd to apply checks on both of parent and child tables when
we select data from a table with its children, as you mentioned.

On the other hand, it also needs to check permission both of child
table and its parents when we select data from a table with its
parents, because the selected columns are inherited from other tables.

The current implementation handles the parent/children as an individual
relations. It does not consider the inherited columns are a part of
parent tables.
For example, ALTER TABLE ADD COLUMN statement checks ownership on
the target table and all the children to add a new column. It is
equivalent to the iteration of ALTER TABLE on the children.

> If you use inheritance for data modeling (the original purpose), you
> face another problem. Either you grant table privileges on all the
> child tables, thus giving users access to more information than they
> were supposed to have, or you grant column privileges on only those
> columns that were inherited, being careful to keep that set updated
> whenever table definitions are altered. (Before 8.4 you couldn't even
> do that.) It's messy.

I think we should check permission on the parent tables/columns when
a user tries to select inherited columns on the child table.
It stands on the perspective that the inherited columns are owned by
the parent (source) table.

The matter is a case when we cannot identify where is the source of
the inherited column if the child table has multiple parents.
An idea is to check permissions on all the parents in this case.

> We could use a GUC variable to ease the transition, perhaps like
> sql_inheritance = no | yes_without_privileges | yes

I think the GUC should toggle which table owns the inherited columns.

If DBA considers the inherited columns are a part of the parent table,
individual checks can be bypassed. Otherwise, we can keep the compatible
bahavior.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>


From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 07:01:48
Message-ID: 1254726108.25576.5.camel@fsopti579.F-Secure.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, 2009-10-05 at 12:15 +0900, KaiGai Kohei wrote:
> On the other hand, it also needs to check permission both of child
> table and its parents when we select data from a table with its
> parents,

You can't do that anyway.


From: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 07:27:35
Message-ID: 4AC99FE7.7030408@ak.jp.nec.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Peter Eisentraut wrote:
> On Mon, 2009-10-05 at 12:15 +0900, KaiGai Kohei wrote:
>> On the other hand, it also needs to check permission both of child
>> table and its parents when we select data from a table with its
>> parents,
>
> You can't do that anyway.

Sorry, I'm not clear why it is impossible.

If we adopt the perspective that inherited columns are a part of
the parent tables, it is quite natural to bypass permisson checks
on the child tables, when we select data from the parent table.
However, we also can select data from the child table which contains
inherited columns from the parent table. In this case, it seems to me
unnatural to bypass permission checks on the parent tables/columns,
although it adopts the perspective.

What I wanted to say is...

For example)

CREATE TABLE tbl_p (int a, int b);
CREATE TABLE tbl_c (int x) INHERITS(tbl_p);

SELECT a,b FROM tbl_p; --> It selects data from only tbl_p.
It is reasonable to bypass checks on tbl_c.
SELECT b,x FROM tbl_c; --> It selects data from tbl_p and tbl_c concurrently,
if we consider the inherited columns are a part of
the parent table.
SELECT x FROM tbl_c; --> ???
In this case, I don't think it is necessary to check
permissions on the parent table.

Am I missing something?

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>


From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 08:22:16
Message-ID: 1254730936.4691.120.camel@ebony.2ndQuadrant
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On Sat, 2009-10-03 at 09:45 +0300, Peter Eisentraut wrote:

> We could use a GUC variable to ease the transition, perhaps like
> sql_inheritance = no | yes_without_privileges | yes

The original way of doing things was quite useful if you wanted some
people to be able to see history and others just see recent data. I
don't think many people are aware of or take advantage of that, so your
proposal does simplify things for many people.

Would it not be better to offer this as a table-level option, with
default of check-permission-on-parent-only?

--
Simon Riggs www.2ndQuadrant.com


From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 08:22:17
Message-ID: 1254730937.25576.8.camel@fsopti579.F-Secure.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, 2009-10-05 at 16:27 +0900, KaiGai Kohei wrote:
> CREATE TABLE tbl_p (int a, int b);
> CREATE TABLE tbl_c (int x) INHERITS(tbl_p);
>
> SELECT a,b FROM tbl_p; --> It selects data from only tbl_p.
> It is reasonable to bypass checks on tbl_c.
> SELECT b,x FROM tbl_c; --> It selects data from tbl_p and tbl_c concurrently,
> if we consider the inherited columns are a part of
> the parent table.

I think you need to distinguish between the definition of columns and
the data in the columns. tbl_c has inherited the definition of the
columns from tbl_p, but the data is part of tbl_c, not tbl_p. So there
is not reason for this second query to ask tbl_p for permission, because
it does not touch data in tbl_p at all.


From: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 08:59:31
Message-ID: 4AC9B573.9060002@ak.jp.nec.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Peter Eisentraut wrote:
> On Mon, 2009-10-05 at 16:27 +0900, KaiGai Kohei wrote:
>> CREATE TABLE tbl_p (int a, int b);
>> CREATE TABLE tbl_c (int x) INHERITS(tbl_p);
>>
>> SELECT a,b FROM tbl_p; --> It selects data from only tbl_p.
>> It is reasonable to bypass checks on tbl_c.
>> SELECT b,x FROM tbl_c; --> It selects data from tbl_p and tbl_c concurrently,
>> if we consider the inherited columns are a part of
>> the parent table.
>
> I think you need to distinguish between the definition of columns and
> the data in the columns. tbl_c has inherited the definition of the
> columns from tbl_p, but the data is part of tbl_c, not tbl_p. So there
> is not reason for this second query to ask tbl_p for permission, because
> it does not touch data in tbl_p at all.

Yes, I can understand the second query selects data stored within only
tbl_c in this case, not tbl_p, even if tbl_c inherits its definitions
from the parent.
However, this perspective may be inconsistent to the idea to bypass
permission checks on the child (tbl_c) when we select data from the
parent (tbl_p), because the first query also fetches data stored
within the tbl_c, not only the tbl_p.

IMO, if we adopt the perspective which considers the access control
depends on the physical location, the current implementation works fine.
However, this idea proposed a different perspective.
It allows to bypass permission checks on the child tables, because
the child has identical definition with its parent and these are a part
of the parent table.
If so, I think this perspective should be ensured without any exception.

BTW, I basically think this perspective change is better.

Thanks,
--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>


From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 09:30:19
Message-ID: 1254735019.25576.14.camel@fsopti579.F-Secure.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, 2009-10-05 at 09:22 +0100, Simon Riggs wrote:
> On Sat, 2009-10-03 at 09:45 +0300, Peter Eisentraut wrote:
>
> > We could use a GUC variable to ease the transition, perhaps like
> > sql_inheritance = no | yes_without_privileges | yes
>
> The original way of doing things was quite useful if you wanted some
> people to be able to see history and others just see recent data. I
> don't think many people are aware of or take advantage of that, so your
> proposal does simplify things for many people.

Wouldn't that look something like

data -- empty
data_recent INHERITS (data)
data_old INHERITS (data)
data_ancient INHERITS (data)

GRANT ... ON data_recent TO A
GRANT ... ON data_old TO B

I guess you could also do

data -- recent data
data_old INHERITS (data)
data_ancient INHERITS (data)

GRANT ... ON data TO A
GRANT ... ON data_old TO B

And then A, who has only access to the recent data, would always have to
use ONLY data to be able to do anything. That would be a pretty weird
setup. The workaround is to change it to the setup above, which you can
do with a few renames.


From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 09:47:54
Message-ID: 1254736074.4691.132.camel@ebony.2ndQuadrant
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On Mon, 2009-10-05 at 12:30 +0300, Peter Eisentraut wrote:
> On Mon, 2009-10-05 at 09:22 +0100, Simon Riggs wrote:
> > On Sat, 2009-10-03 at 09:45 +0300, Peter Eisentraut wrote:
> >
> > > We could use a GUC variable to ease the transition, perhaps like
> > > sql_inheritance = no | yes_without_privileges | yes
> >
> > The original way of doing things was quite useful if you wanted some
> > people to be able to see history and others just see recent data. I
> > don't think many people are aware of or take advantage of that, so your
> > proposal does simplify things for many people.
>
> Wouldn't that look something like
>
> data -- empty
> data_recent INHERITS (data)
> data_old INHERITS (data)
> data_ancient INHERITS (data)
>
> GRANT ... ON data_recent TO A
> GRANT ... ON data_old TO B
>
> I guess you could also do
>
> data -- recent data
> data_old INHERITS (data)
> data_ancient INHERITS (data)
>
> GRANT ... ON data TO A
> GRANT ... ON data_old TO B
>
> And then A, who has only access to the recent data, would always have to
> use ONLY data to be able to do anything. That would be a pretty weird
> setup. The workaround is to change it to the setup above, which you can
> do with a few renames.

If you use multiple inheritance it all works as I described.

top level: data-template
main tables: data, data-recent both inherit from data-template
all partitions inherit from data
only recent partitions inherit from data-recent
grants are issued on data and data-recent

Now that I think about it more, I want the change you describe but don't
think its a system-wide setting. You may have PostgreSQL inheritance
apps next door to partitioning apps. The right place to fix this is when
we implement partitioning syntax, so we can set a flag saying "make
permissions easier for partitions".

--
Simon Riggs www.2ndQuadrant.com


From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 10:06:40
Message-ID: 1254737200.25576.15.camel@fsopti579.F-Secure.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, 2009-10-05 at 10:47 +0100, Simon Riggs wrote:
> top level: data-template
> main tables: data, data-recent both inherit from data-template
> all partitions inherit from data
> only recent partitions inherit from data-recent
> grants are issued on data and data-recent

I don't see where the problem is here.


From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 10:45:44
Message-ID: 1254739544.4691.142.camel@ebony.2ndQuadrant
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On Mon, 2009-10-05 at 13:06 +0300, Peter Eisentraut wrote:
> On Mon, 2009-10-05 at 10:47 +0100, Simon Riggs wrote:
> > top level: data-template
> > main tables: data, data-recent both inherit from data-template
> > all partitions inherit from data
> > only recent partitions inherit from data-recent
> > grants are issued on data and data-recent
>
> I don't see where the problem is here.

In your last post you said it was necessary to use ONLY to address the
required partitions and so setup would be weird. I am showing that this
is not required and the setup is smooth.

The main point though is that this should not be a system-wide setting.
I agree with your overall intention, just not your specific solution.

We need improvements in partitioning, not minor tweaks. It seems much
better to me to hack out the portion of the last partitioning patch
(Kedar's) so that we just support new syntax without any underlying
changes (yet). We can mark a table as being partitioned and for such
tables skip the permission checks - no new GUC.

If you can push that change through as initial infrastructure then
others can work on internals - which were not close in that last patch.

--
Simon Riggs www.2ndQuadrant.com


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 14:14:36
Message-ID: 6874.1254752076@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> On Mon, 2009-10-05 at 13:06 +0300, Peter Eisentraut wrote:
>> I don't see where the problem is here.

> In your last post you said it was necessary to use ONLY to address the
> required partitions and so setup would be weird. I am showing that this
> is not required and the setup is smooth.

Peter is right and you are wrong: this setup STILL needs ONLY, unless
permissions are in sync with inheritance, ie, every child has the union
of its parents' permissions. It would work at least as well under
Peter's proposal as with the existing behavior.

> The main point though is that this should not be a system-wide setting.

No, it should be a flat-out behavioral change, no "setting" anywhere.
I have never seen an example where the current behavior is actually
useful, because of precisely the point that you'd have to use ONLY to
avoid permissions errors unless you have granted permissions on all
children of each parent.

regards, tom lane


From: Simon Riggs <simon(at)2ndQuadrant(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
Subject: Re: Privileges and inheritance
Date: 2009-10-05 14:31:45
Message-ID: 1254753105.4691.163.camel@ebony.2ndQuadrant
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On Mon, 2009-10-05 at 10:14 -0400, Tom Lane wrote:
> Simon Riggs <simon(at)2ndQuadrant(dot)com> writes:
> > On Mon, 2009-10-05 at 13:06 +0300, Peter Eisentraut wrote:
> >> I don't see where the problem is here.
>
> > In your last post you said it was necessary to use ONLY to address the
> > required partitions and so setup would be weird. I am showing that this
> > is not required and the setup is smooth.
>
> Peter is right and you are wrong: this setup STILL needs ONLY, unless
> permissions are in sync with inheritance, ie, every child has the union
> of its parents' permissions. It would work at least as well under
> Peter's proposal as with the existing behavior.

What I proposed works, so perhaps we are talking about different things.

If you wish to see all data you grant access to parent_full, if you wish
to see recent data you grant access to parent_partial. The partitions
can then be given access to the various users.

--
Simon Riggs www.2ndQuadrant.com


From: Josh Berkus <josh(at)agliodbs(dot)com>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 17:27:40
Message-ID: 4ACA2C8C.7000004@agliodbs.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Simon,

>> We could use a GUC variable to ease the transition, perhaps like
>> sql_inheritance = no | yes_without_privileges | yes
>
> The original way of doing things was quite useful if you wanted some
> people to be able to see history and others just see recent data. I
> don't think many people are aware of or take advantage of that, so your
> proposal does simplify things for many people.
>
> Would it not be better to offer this as a table-level option, with
> default of check-permission-on-parent-only?

No, I don't think so. The original way *wasn't* actually useful if you
wanted to differentiate between which partitions people could see. Example:

You partition the sales table geographically. You want salespeople to
only be able to see their own geo, but management to be able to see all:

role staff
manager inherits staff
sales_USA inherits staff
sales_CAN inherits staff
sales_EUR inherits staff

master table sales grant SELECT: staff
sales_CAN inherits sales grant SELECT: manager, sales_CAN
sales_USA inherits sales grant SELECT: manager, sales_USA
sales_EUR inherits sales grant SELECT: manager, sales_EUR

So, then a USA-role salesperson does "SELECT sum(gross) FROM sales;".
What happens? A permissions error. *Not* the desired seeing only the
USA data.

In order for a user which privs on only some partitions to see the data
in those partitions, that user needs to query the partitions directly.
The proposed patch would not change that. The only thing it would
change is that DBAs would need to be careful to be restrictive about
permissions granted on the master table.

--Josh Berkus


From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 18:23:43
Message-ID: 1254767023.22144.0.camel@vanquo.pezone.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Mon, 2009-10-05 at 11:45 +0100, Simon Riggs wrote:
> On Mon, 2009-10-05 at 13:06 +0300, Peter Eisentraut wrote:
> > On Mon, 2009-10-05 at 10:47 +0100, Simon Riggs wrote:
> > > top level: data-template
> > > main tables: data, data-recent both inherit from data-template
> > > all partitions inherit from data
> > > only recent partitions inherit from data-recent
> > > grants are issued on data and data-recent
> >
> > I don't see where the problem is here.
>
> In your last post you said it was necessary to use ONLY to address the
> required partitions and so setup would be weird. I am showing that this
> is not required and the setup is smooth.

Well, you posted this:

top level: data-template
main tables: data, data-recent both inherit from data-template
all partitions inherit from data
only recent partitions inherit from data-recent
grants are issued on data and data-recent

But this is too vague for me to make out who can read what and what
would change under the proposed change and why that would be a problem.


From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Josh Berkus <josh(at)agliodbs(dot)com>
Cc: Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 18:42:42
Message-ID: 1254768162.4691.241.camel@ebony.2ndQuadrant
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On Mon, 2009-10-05 at 10:27 -0700, Josh Berkus wrote:
> Simon,
>
> >> We could use a GUC variable to ease the transition, perhaps like
> >> sql_inheritance = no | yes_without_privileges | yes
> >
> > The original way of doing things was quite useful if you wanted some
> > people to be able to see history and others just see recent data. I
> > don't think many people are aware of or take advantage of that, so your
> > proposal does simplify things for many people.
> >
> > Would it not be better to offer this as a table-level option, with
> > default of check-permission-on-parent-only?
>
> No, I don't think so. The original way *wasn't* actually useful if you
> wanted to differentiate between which partitions people could see. Example:
>
> You partition the sales table geographically. You want salespeople to
> only be able to see their own geo, but management to be able to see all:
>
> role staff
> manager inherits staff
> sales_USA inherits staff
> sales_CAN inherits staff
> sales_EUR inherits staff
>
> master table sales grant SELECT: staff
> sales_CAN inherits sales grant SELECT: manager, sales_CAN
> sales_USA inherits sales grant SELECT: manager, sales_USA
> sales_EUR inherits sales grant SELECT: manager, sales_EUR
>
> So, then a USA-role salesperson does "SELECT sum(gross) FROM sales;".
> What happens? A permissions error. *Not* the desired seeing only the
> USA data.

> In order for a user which privs on only some partitions to see the data
> in those partitions, that user needs to query the partitions directly.
> The proposed patch would not change that. The only thing it would
> change is that DBAs would need to be careful to be restrictive about
> permissions granted on the master table.

OK, make the change.

A small addition though, please. This functionality has been available
since 8.1 and changing things could cause existing people's scripts to
fail when they upgrade. If we make this change then we should make sure
that explicitly GRANTing a permission on the child tables does not fail.

--
Simon Riggs www.2ndQuadrant.com


From: Josh Berkus <josh(at)agliodbs(dot)com>
To: Simon Riggs <simon(at)2ndQuadrant(dot)com>
Cc: Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 18:58:20
Message-ID: 4ACA41CC.1050804@agliodbs.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Simon,

> A small addition though, please. This functionality has been available
> since 8.1 and changing things could cause existing people's scripts to
> fail when they upgrade. If we make this change then we should make sure
> that explicitly GRANTing a permission on the child tables does not fail.

You seem to have misunderstood the patch. We're not disabling the
ability to GRANT permissions on individual child tables. We're just
disabling the child table permissions check if someone comes in through
the master.

And we'll *definitely* need to warn people about the security implications.

--Josh Berkus


From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Josh Berkus <josh(at)agliodbs(dot)com>
Cc: Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-05 19:51:04
Message-ID: 1254772264.4691.255.camel@ebony.2ndQuadrant
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On Mon, 2009-10-05 at 11:58 -0700, Josh Berkus wrote:
> Simon,
>
> > A small addition though, please. This functionality has been available
> > since 8.1 and changing things could cause existing people's scripts to
> > fail when they upgrade. If we make this change then we should make sure
> > that explicitly GRANTing a permission on the child tables does not fail.

> We're not disabling the
> ability to GRANT permissions on individual child tables.

Until I raised it, that subject had not been mentioned at all, so I've
no idea how you know that either was or was not intended. I wish to make
sure we don't make that error by explicitly stating requirements.

--
Simon Riggs www.2ndQuadrant.com


From: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
To: Peter Eisentraut <peter_e(at)gmx(dot)net>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-06 08:18:39
Message-ID: 4ACAFD5F.1010504@ak.jp.nec.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Was it uncertain what I wanted to say for the proposition?

In my understanding, here are two different perspectives to handle
table inheritances.

The one handles the inherited definitions of child tables as a part
of the child table. The current implementation uses this perspective.
Thus, it checks user's privilege on all the children when he tries to
select data from the parent table. In constrast, it does not need to
check permission on the parent tables, when he tries to select data
from the child table, because the child table does not contain any
part of the parent table.

The other is newly proposed one. It handles the inherited definitions
of child tables as a part of parent table. Thus, it does not need to
check user's privilege on the children when he tries to select data
from the parent table, because this query can select only columns
inherited from the parent table and we can consider these are a part
of the parent table in this perspective.
In constrast, if we consider inherited columns are a part of the parent
tables, it should check permission on the parent relation and columns
when he tries to select data from the child table.

If a table tbl_c(c int) inherits a table tbl_p(a int, b int), ...

The first perspective considers as follows:

+--- physical location of the data
|
--V---+---------------+
tbl_p | data stored |
| within tbl_p |
------+---------------+-------+
tbl_c | data stored |
| within tbl_c |
------+-------+-------+-------|
| a | b | c |

In this perspective, "SELECT * FROM tbl_p" accesses data within both of
tbl_p and tbl_c, so it needs to check privileges to tbl_p and tbl_c.
But "SELECT * FROM tbl_c" does not see any data within tbl_p.

The later perspective considers as follows:

+--- physical location of the data
|
--V---+---------------+
tbl_p | | +----- data stored within tbl_c
| | |
------+ data stored +---|---+
tbl_c | within tbl_p | V |
| | |
------+-------+-------+-------+
| a | b | c |

In this perspective, "SELECT * FROM tbl_p" access data within only tbl_p,
so it does not needs to check privilege on the tbl_c.
But, "SELECT * FROM tbl_c" also means accesses data within both of tbl_p
and tbl_c concurrently in this perspective, so I think it is quite natural
to check privileges to both of tbl_p and tbl_c.

In my understanding, your proposition enables DBA to choose which perspective
can be applied on his system. It seems to me quite cool.
However, the behavior when we select from a child table seems to me inconsistent.
If it does not check privileges on the parent table when selecting from the child,
it means we can adopt different perspectives concurrently.

Am I missing something?
I'm still unclear which perspective does it tries to provide.

Thanks,

BTW, I uses the term of "perspective" to mean "point of view" or "philosophy".
Is it confusable? Is there any more appropriate terms?

KaiGai Kohei wrote:
> Peter Eisentraut wrote:
>> On Mon, 2009-10-05 at 16:27 +0900, KaiGai Kohei wrote:
>>> CREATE TABLE tbl_p (int a, int b);
>>> CREATE TABLE tbl_c (int x) INHERITS(tbl_p);
>>>
>>> SELECT a,b FROM tbl_p; --> It selects data from only tbl_p.
>>> It is reasonable to bypass checks on tbl_c.
>>> SELECT b,x FROM tbl_c; --> It selects data from tbl_p and tbl_c concurrently,
>>> if we consider the inherited columns are a part of
>>> the parent table.
>> I think you need to distinguish between the definition of columns and
>> the data in the columns. tbl_c has inherited the definition of the
>> columns from tbl_p, but the data is part of tbl_c, not tbl_p. So there
>> is not reason for this second query to ask tbl_p for permission, because
>> it does not touch data in tbl_p at all.
>
> Yes, I can understand the second query selects data stored within only
> tbl_c in this case, not tbl_p, even if tbl_c inherits its definitions
> from the parent.
> However, this perspective may be inconsistent to the idea to bypass
> permission checks on the child (tbl_c) when we select data from the
> parent (tbl_p), because the first query also fetches data stored
> within the tbl_c, not only the tbl_p.
>
> IMO, if we adopt the perspective which considers the access control
> depends on the physical location, the current implementation works fine.
> However, this idea proposed a different perspective.
> It allows to bypass permission checks on the child tables, because
> the child has identical definition with its parent and these are a part
> of the parent table.
> If so, I think this perspective should be ensured without any exception.
>
> BTW, I basically think this perspective change is better.
>
> Thanks,

--
OSS Platform Development Division, NEC
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>


From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>
Cc: Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-06 14:00:53
Message-ID: 770.1254837653@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
> Was it uncertain what I wanted to say for the proposition?

No: it's that nobody sees any particular value in making a fundamental
restructuring of the permissions system like that. What you propose
would be far harder/more complicated to implement, to understand,
or to use.

regards, tom lane


From: KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>, Peter Eisentraut <peter_e(at)gmx(dot)net>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-06 14:46:23
Message-ID: 4ACB583F.3040401@kaigai.gr.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

Tom Lane wrote:
> KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com> writes:
>> Was it uncertain what I wanted to say for the proposition?
>
> No: it's that nobody sees any particular value in making a fundamental
> restructuring of the permissions system like that. What you propose
> would be far harder/more complicated to implement, to understand,
> or to use.

Yes, I agree.
If we actually implement the new perspective perfectly, it needs complex
implementation due to the differences from physical structure.

However, it seems to me the current proposition tries to adopt a mixed
perspectives depending on the situation....

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>


From: Peter Eisentraut <peter_e(at)gmx(dot)net>
To: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Privileges and inheritance
Date: 2009-10-20 19:57:28
Message-ID: 1256068648.12727.1.camel@vanquo.pezone.net
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

On Sat, 2009-10-03 at 09:45 +0300, I wrote:
> So let's get rid of that. Selecting (or in general, operating) on a
> table with children only checks the privileges on that table, not the
> children.

If I'm seeing this right, it's literally a one-line fix. Patch attached
with documentation and test updates.

Attachment Content-Type Size
grant-recursive.patch text/x-patch 3.7 KB

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
Subject: Re: Privileges and inheritance
Date: 2009-10-20 20:10:12
Message-ID: 4645.1256069412@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:
> If I'm seeing this right, it's literally a one-line fix.

At least a two-line fix, please: that needs a comment. But yeah,
I think that's basically all that would have to change.

regards, tom lane