Privileges and inheritance

Peter Eisentraut <peter_e@gmx.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

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

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. ;-)

Peter Eisentraut <peter_e@gmx.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

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@ak.jp.nec.com>

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.

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@ak.jp.nec.com>

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

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.

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@ak.jp.nec.com>

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.

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

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.

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

Simon Riggs <simon@2ndQuadrant.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

On Mon, 2009-10-05 at 10:14 -0400, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.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

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

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.

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