Re: Extend argument of OAT_POST_CREATE

Thanks for your reviews.

2012/10/10 Alvaro Herrera <alvherre(at)2ndquadrant(dot)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(at)kaigai(dot)gr(dot)jp>