Thread: Extend argument of OAT_POST_CREATE

The attached patch adds argument of OAT_POST_CREATE hook;
to inform extensions type of the context of this object creation. It allows
extensions to know whether the new object is indirectly created apart
from user's operations, or not.

I found out this flag is necessary to add feature to support selinux
checks on ALTER statement (with reasonably simple code) during
my investigation.

A table has various kind of properties; some of them are inlined in
pg_class but others are stored in extra catalogs such as pg_trigger,
pg_constraint and so on.
It might take an extra discussion whether trigger or constraint is
an independent object or an attribute of table. But, anyway, the
default permission checks table's ownership or ACLs when we
create or drop them. I don't think sepgsql should establish its own
object model here.

So, I want sepgsql to check table's "setattr" permission when user
create, drop or alter these objects.

In case of index creation, here are two cases a) user's operation
intend to create index, thus, checks permission of the table being
indexed on b) index is indirectly created as a result of other
operations like change of column's data type.
Due to same reason why we don't check permissions for cleanup
of temporary object, I don't want to apply checks on the later case.

Right now, sepgsql determines the current context using command
tag being saved at ProceddUtility_hook; to avoid permission checks
on table creation due to CLUSTER command for example.
But, it is not easy to apply this approach for the case of index
creation because it can be defined as a part of ALTER TABLE
which may have multiple sub-commands.

So, I want OAT_POST_CREATE hook to inform the current context
of the object creation; whether it is internal / indirect creation, or not.

This patch includes hook enhancement and "setattr" permission checks
on index creation / deletion.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Kohei KaiGai escribió:
> The attached patch adds argument of OAT_POST_CREATE hook;
> to inform extensions type of the context of this object creation. It allows
> extensions to know whether the new object is indirectly created apart
> from user's operations, or not.

Can we add Assert(!is_internal) to the ProcedureRelationId case in
sepgsql_object_access() too?  I don't see any caller that would set it
true anywhere, but maybe you have a good reason for omitting it.

I'm not clear on what's sepgsql_relation_setattr for; it doesn't seem to
be called anywhere (other than sepgsql_relation_setattr_extra, but
that's static, so why isn't sepgsql_relation_setattr also static?).  But
I notice that it calls sepgsql_index_modify without first checking for
the toast namespace like the other callers do.  Is this okay or an
oversight?

I admit the new RELKIND_INDEX cases in various places make for strange
flow.  Not sure how it can be improved though.

I didn't find anything wrong with the changes to src/backend.  One thing
that I noticed is that when bootstrapping, all relation creation is
considered internal.  I am sure this is okay fo the normal case, but I
wonder if a malicious superuser could get a hold of things that he
shouldn't by starting a bootstrapping backend and run relation creation
there.

Note: I can compile sepgsql but not run the regression tests.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Thanks for your reviews.

2012/10/10 Alvaro Herrera <alvherre@2ndquadrant.com>:
> Kohei KaiGai escribió:
>> The attached patch adds argument of OAT_POST_CREATE hook;
>> to inform extensions type of the context of this object creation. It allows
>> extensions to know whether the new object is indirectly created apart
>> from user's operations, or not.
>
> Can we add Assert(!is_internal) to the ProcedureRelationId case in
> sepgsql_object_access() too?  I don't see any caller that would set it
> true anywhere, but maybe you have a good reason for omitting it.
>
No, I just missed to add Assert() here.

> I'm not clear on what's sepgsql_relation_setattr for; it doesn't seem to
> be called anywhere (other than sepgsql_relation_setattr_extra, but
> that's static, so why isn't sepgsql_relation_setattr also static?).  But
> I notice that it calls sepgsql_index_modify without first checking for
> the toast namespace like the other callers do.  Is this okay or an
> oversight?
>
I assume sepgsql_relation_setattr is also called on ALTER TABLE
command; to check privilege to modify properties of the target table.
Entrypoint of the object_access_hook is at sepgsql/hooks.c, so this
function was declared without static for (near) future usage.
Regarding to toast relation/index, as default permission mechanism
doing, sepgsql handles toast is a pure-internal semantics, thus, no
security label is assigned and no permission checks are applied.
(Also, please check check_relation_privileges at sepgsql/dml.c.
 It does not allow to reference toast relation using regular SQL
 with hardwired rule, because of the nature of "internal stuff".)

> I admit the new RELKIND_INDEX cases in various places make for strange
> flow.  Not sure how it can be improved though.
>
The idea is not so complicated. This code considers index is an
property of a certain table like as individual field of pg_class.
Relation is the most complex object in PostgreSQL. Its property
is not only ones in pg_class, but some extra catalogs such as
pg_trigger, pg_rewrite and so on.
The default permission checks ownership of the table, instead
of triggers, rules or indexes, when user tries to alter them.
Here is no good reason why sepgsql needs to give its own
definition of relation, so I just followed this manner.

> I didn't find anything wrong with the changes to src/backend.  One thing
> that I noticed is that when bootstrapping, all relation creation is
> considered internal.  I am sure this is okay fo the normal case, but I
> wonder if a malicious superuser could get a hold of things that he
> shouldn't by starting a bootstrapping backend and run relation creation
> there.
>
Yes, you are right. Even though it is harmless right now because we have
no way to load extension prior to initdb, it makes confusion if some built-in
feature used object access hook. The "internal" flag means the given SQL
statement does not intend creation itself of the target object, but bootstrap
command definitely intend to create initial objects.

On the other hand, I don't care about the scenario with malicious superuser
that runs initdb, because it is an assumption of sepgsql to set up initial
database on environment without something malicious.
If we try to prevent to create an initial database by malicious users, it should
be a responsibility of operating system and its policy.

Thanks,
--
KaiGai Kohei <kaigai@kaigai.gr.jp>

Kohei KaiGai escribió:

> 2012/10/10 Alvaro Herrera <alvherre@2ndquadrant.com>:

> > I admit the new RELKIND_INDEX cases in various places make for strange
> > flow.  Not sure how it can be improved though.
> >
> The idea is not so complicated. This code considers index is an
> property of a certain table like as individual field of pg_class.
> Relation is the most complex object in PostgreSQL. Its property
> is not only ones in pg_class, but some extra catalogs such as
> pg_trigger, pg_rewrite and so on.

I have pushed this patch with some slight changes in the way
the RELKIND_INDEX case is handled in various places, to make it
clearer.  I may have broken some cases; please have a look.

Thanks.

--
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services