Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-11-08 Thread Robert Haas
On Mon, Nov 7, 2011 at 12:20 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 If sepgsql would apply permission checks db_procedure:{install} on the
 OAT_POST_CREATE hook based on the funcion-oid within new entry of
 system catalog, we can relocate OAT_PREP_CREATE hook more conceptually
 right place, such as just after the pg_namespace_aclcheck() of DefineType().
 On the other hand, we may need to answer why these information are NOT
 delivered on the OAT_PREP_CREATE hook without knowledge of sepgsql
 internal.

I'm having a hard time understanding this.

Can you strip this patch down so it just applies to a single object
type (tables, maybe, or functions, or whatever you like) and then
submit the corresponding sepgsql changes with it?  Just as a demo
patch, so I can understand where you're trying to go with this.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-11-01 Thread Kohei KaiGai
2011/10/21 Robert Haas robertmh...@gmail.com:
 On Fri, Oct 21, 2011 at 12:44 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I had checked my older implementation based on 8.4.x or 9.0.x that
 includes all the features that I want to implement.
 At least, it does not require so much different information from ones
 needed by DAC model, although SELECT INTO was an exception.
 It might be quite natural because both works similar things.

 OK.  It seems like it might be helpful to put together a list of all
 of the things you want to check permissions on, maybe on a wiki page
 somewhere, and indicate in there which ones are done and which ones
 are not done, and what additional information you think is needed in
 each case, and flag any MAC/DAC discrepancies that you are concerned
 about.  I think that might help us reach agreement on the best way
 forward.  Also, then, as we get things committed, we can track
 progress versus that roadmap.

I tried to summarize permission checks of DAC/MAC on several object classes
that are allowed to assign security label right now.
http://wiki.postgresql.org/index.php?title=SEPostgreSQL/Permissions

In most of checks, required contextual information by SELinux are commonly
used to DAC also, as listed.

On the creation of relations, SELinux requires relkind and TupleDesc to
identify relation type and columns. Unlike a flag of select-into, I'm not sure
whether it is a model specific and hard to manage without knowledge about
sepgsql internal.

In some cases, DAC uses superuser privilege as a substitute for individual
permission checks, such as DefineType().
It seems to me its original implementation checked CREATE permission of
the namespace and ownership of the functions that implements type input,
output or others, then, we commented out these code because superuser
is obviously allowed these checks.
However, MAC does not have concept of superuser, so, SELinux wants to
check db_procedure:{install} for each functions, thus, it requires oid of
the functions to be used for the new type.
Of course, we can see here is no difference between DAC and MAC, if we
consider DAC implicitly allows these checks by superuser().

I think it is a good start to implement first ddl permissions on creation of
the seven object classes as listed; also to proof the concept; 1) we put
permission checks around existing DAC checks, 2) we deliver contextual
data (basically) used in existing DAC also.

I guess DROP or some of ALTER code reworking should be done prior to
deploy object_access_hook around their permission checks, to minimize
maintain efforts.

Thanks,
-- 
KaiGai Kohei kai...@kaigai.gr.jp

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-11-01 Thread Robert Haas
On Tue, Nov 1, 2011 at 1:32 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I tried to summarize permission checks of DAC/MAC on several object classes
 that are allowed to assign security label right now.
 http://wiki.postgresql.org/index.php?title=SEPostgreSQL/Permissions

 In most of checks, required contextual information by SELinux are commonly
 used to DAC also, as listed.

What's up with this:

a flag to inform whether CASCADE or RESTRICT

That doesn't seem like it should be needed.

We should consider whether CREATE TABLE should be considered to
consist of creating a table and then n attributes, rather than trying
to shove the attribute information wholesale into the create table
check.

 I guess DROP or some of ALTER code reworking should be done prior to
 deploy object_access_hook around their permission checks, to minimize
 maintain efforts.

+1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-11-01 Thread Kohei KaiGai
2011/11/1 Robert Haas robertmh...@gmail.com:
 On Tue, Nov 1, 2011 at 1:32 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I tried to summarize permission checks of DAC/MAC on several object classes
 that are allowed to assign security label right now.
 http://wiki.postgresql.org/index.php?title=SEPostgreSQL/Permissions

 In most of checks, required contextual information by SELinux are commonly
 used to DAC also, as listed.

 What's up with this:

 a flag to inform whether CASCADE or RESTRICT

 That doesn't seem like it should be needed.

Ah, it is needed to determine the scope of objects to be deleted.
If a table being referenced by views, deletion of this table with cascade
involves the views, even if these are owned by others.
DAC does not check permissions to drop on depending objects,
so, it is a headache for me.

However, the worth of drop permission checks on involved objects
is not perfectly clear, because the user is allowed to drop the table
being reference by views at least in above example. If so, views to
reference non-existent table is nonsense.
I'm not 100% sure why existing DAC does not check permissions
on depending objects even if these are owned by other users.
I'd like to know the reason of this behavior.

 We should consider whether CREATE TABLE should be considered to
 consist of creating a table and then n attributes, rather than trying
 to shove the attribute information wholesale into the create table
 check.

I don't see tangible difference between them except for simpleness of
implementation. One point we should consider is the timing to store
security label of table and columns.

If creation checks of table/columns are consolidated into one hook,
we can check permissions to create them and write-back default
security labels of them into one private datum to be delivered to
post-creation hook.

If creation checks of table/columns are separated into individual
invocation, the later column-checks needs security label of the
new table as contextual information, however, it requires us to
know internals of sepgsql why it needs table's label to check
permission to create a column, because both of checks shall
be done DefineRelation / OpenIntoRel prior to heap_create_with_catalog
that invokes post-creation hook.

So, I think it works better with the approach that also delivers
TupleDesc on creation of relation checks, rather than separated
invocations.

Thanks,
-- 
KaiGai Kohei kai...@kaigai.gr.jp

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-10-21 Thread Kohei KaiGai
 When someone comes along in another year or two and adds materialized
 views, will they need to pass some additional data to the object
 access hook?  Probably, but I bet you're the only one who can quickly
 figure out what it is.  That's no good.  We're not going to make
 changes to PostgreSQL core that only you can maintain, and that are
 likely to be broken by future commits.  At this point I feel pretty
 good that someone can look at the stuff that we've done so far with
 SECURITY LABEL and the object access hooks, and if they add a new
 object type, they can make those things apply to the new object type
 too by copying what's already there, without making any reference to
 the sepgsql code.  There's a clear abstraction boundary, and people
 who are working on one side of the boundary do not need to understand
 the details of what happens on the other side of the boundary.

I had checked my older implementation based on 8.4.x or 9.0.x that
includes all the features that I want to implement.
At least, it does not require so much different information from ones
needed by DAC model, although SELECT INTO was an exception.
It might be quite natural because both works similar things.

For example, sepgsql required Oid of source database to compute
default security label on new database at createdb(). It was used to
permission checks in DAC model also.
For another example, sepgsql also required Oids of type-functions
to check permissions on them at DefineType(). It was also used to
DAC model except for these checks were commented out by
#ifdef NOT_USED because of superuser() was already checked.


So, how do we launch this efforts according to the principles:
- Hooks being used to security checks also should be deployed
  around existing DAC checks.
- The delivered arguments should not be model specific.

I don't have clear idea to rework existing routines like as I proposed
long before; that wrap-up a series of DAC checks and entrypoint of
MAC hooks into a single function.

A straightforward idea is to deploy object-access-hook around existing
DAC checks with new OAT_* label, such as OAT_CREATE.
In the case of relation creation, it shall be DefineRelation() and OpenIntoRel()
to bypass internal invocation of heap_create_with_catalog().

Please tell me if we have different idea of code reworking to simplify
deployment of the hooks.

 In this particular case, I think it might be reasonable to change the
 DAC behavior, so that a CREATE TABLE AS SELECT or SELECT INTO requires
 insert privileges on the new table as well as permission to create it
 in the first place.  I don't particularly see any reason to require
 different privileges for CREATE TABLE followed by INSERT .. SELECT
 than what we require when the two commands are rolled into one.  Prior
 to 9.0, this case couldn't arise, because we didn't have default
 privileges, so I'm inclined to think that the way it works now is a
 historical accident rather than a deliberate design decision.

It will help this mismatch between DAC and MAC.
I'll submit it as a separate patch to handle this behavior.
Probably, all we need to do here is invocation of ExecCheckRTPerms().

Thanks,
-- 
KaiGai Kohei kai...@kaigai.gr.jp

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-10-21 Thread Robert Haas
On Fri, Oct 21, 2011 at 12:44 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I had checked my older implementation based on 8.4.x or 9.0.x that
 includes all the features that I want to implement.
 At least, it does not require so much different information from ones
 needed by DAC model, although SELECT INTO was an exception.
 It might be quite natural because both works similar things.

OK.  It seems like it might be helpful to put together a list of all
of the things you want to check permissions on, maybe on a wiki page
somewhere, and indicate in there which ones are done and which ones
are not done, and what additional information you think is needed in
each case, and flag any MAC/DAC discrepancies that you are concerned
about.  I think that might help us reach agreement on the best way
forward.  Also, then, as we get things committed, we can track
progress versus that roadmap.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-10-19 Thread Kohei KaiGai
2011/10/18 Robert Haas robertmh...@gmail.com:
 In the example table creation, heap_create_with_catalog() is invoked
 by 5 routines, however, 3 of them are just internal usages, so it is not
 preferable to apply permission checks on table creation

 Some wit once made the remark that if a function has 10 arguments, it
 has 11 arguments, meaning that functions with very large numbers of
 arguments typically indicate a poor choice of abstraction boundary.
 That's pretty much what I think is going on with
 heap_create_with_catalog().  I think maybe some refactoring is in
 order there, though I am not sure quite what.

Sorry, are you complained about the number of arguments in
heap_create_with_catalog? or hooks of security checks?

If we try to rework access control logic around DefineRelation and
OpenIntoRel to host both of DAC and MAC, it will takes a long
arguments list because an entry of pg_class catalog is not
constructed at the timing, so we need to inform the routine
all the parameters of new relation required to both DAC and MAC.
Thus, it takes a long argument list, however, number of arguments
of heap_create_with_catalog will not increased so much. (Maybe,
it additionally requires a private data to deliver security labels to be
assigned on the new relation.)

If we relocate logics of DAC into heap_create_with_catalog(), it will
need additional two arguments, though it has 19 arguments right now.
It is not a strange idea to inform routines that modified catalogs
whether this context needs permission checks, or not, because
CreateTrigger has a flag of isInternal to skip permission check.

 BTW, it seems to me SELECT INTO should also check insert permission
 on DAC level, because it become an incorrect assumption that owner of
 table has insert permission on its creation time.
   :
 You could make the argument that there's no real security hole there,
 because the user could give himself those same privileges right back.
 You could also make the argument that a SELECT .. INTO is not the same
 as an INSERT and therefore INSERT privileges shouldn't be checked.  I
 think there are valid arguments on both sides, but my main point is
 that we shouldn't have core do it one way and sepgsql do it the other
 way.  That's going to be impossible to maintain and doesn't really
 make any logical sense anyway.

My point is that we should minimize the code complexity, redundancy
or others between DAC and MAC implementation, however, it is not
possible to get their different to zero due to the differences of their
standpoint.

Yes, I never say SELECT INTO without DAC checks cause actual
security hole, because owner can change its access permissions by
himself, unlike MAC.
Please suppose that there are two security labels: confidential table
(TC) and public table (PT). Typical MAC policy (including selinux)
does not allow users who can read from tables with TC to write out
data into tables with PT, because PT is readable for public as literal,
so confidential data may be leaked to public domain in the result.

It is a significant characteristic of MAC; called as data-flow-control.
So, it damages significant part of its worth, if MAC could not distinct
CREATE TABLE from SELECT INTO in PostgreSQL, and it is the
reason why I strongly requires a flag of contextual information here.

Although you say it is not possible to maintain, doesn't the above
introduction help us to understand nothing why MAC needs to
distinct SELECT INTO from CREATE TABLE?, and why it needs
a flag to distinct two cases?

Thanks,
-- 
KaiGai Kohei kai...@kaigai.gr.jp

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-10-19 Thread Robert Haas
On Wed, Oct 19, 2011 at 6:18 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 2011/10/18 Robert Haas robertmh...@gmail.com:
 In the example table creation, heap_create_with_catalog() is invoked
 by 5 routines, however, 3 of them are just internal usages, so it is not
 preferable to apply permission checks on table creation

 Some wit once made the remark that if a function has 10 arguments, it
 has 11 arguments, meaning that functions with very large numbers of
 arguments typically indicate a poor choice of abstraction boundary.
 That's pretty much what I think is going on with
 heap_create_with_catalog().  I think maybe some refactoring is in
 order there, though I am not sure quite what.

 Sorry, are you complained about the number of arguments in
 heap_create_with_catalog? or hooks of security checks?

I'm just saying that heap_create_with_catalog() is big and complex and
does a lot of different things.  The fact it does security checks even
though it's also sometimes called as an internal function strikes me
as a modularity violation.  Normally I would expect to have a
high-level routine (which is invoked more or less directly from SQL)
that does security checks and then calls a low-level routine (that
actually does the work) if they pass.  Other parts of the code that
want to perform the same operation without the security checks can
call the low-level function directly.  But that's not the way it's set
up here.

 Yes, I never say SELECT INTO without DAC checks cause actual
 security hole, because owner can change its access permissions by
 himself, unlike MAC.
 Please suppose that there are two security labels: confidential table
 (TC) and public table (PT). Typical MAC policy (including selinux)
 does not allow users who can read from tables with TC to write out
 data into tables with PT, because PT is readable for public as literal,
 so confidential data may be leaked to public domain in the result.

 It is a significant characteristic of MAC; called as data-flow-control.
 So, it damages significant part of its worth, if MAC could not distinct
 CREATE TABLE from SELECT INTO in PostgreSQL, and it is the
 reason why I strongly requires a flag of contextual information here.

 Although you say it is not possible to maintain, doesn't the above
 introduction help us to understand nothing why MAC needs to
 distinct SELECT INTO from CREATE TABLE?, and why it needs
 a flag to distinct two cases?

Sure.  But what is going to happen when someone needs to modify that
code in a year or two?  In order to understand what to do with the
object access hook, they're going to need to understand all those
details you just mentioned.  And those details do not exist in the
patch as submitted.  Worse, let's suppose we'd committed a patch like
the one you have here before foreign tables went in.  Then, whoever
added foreign tables would have needed to know to add the foreign
table server name to the object access hook invocation, because
apparently sepgsql needs that.  How would they know they needed to do
that?  I've committed practically all of the sepgsql-related patches,
and I would not have known that, so it seems likely to me that other
people adding new functionality to the server wouldn't know it either.

When someone comes along in another year or two and adds materialized
views, will they need to pass some additional data to the object
access hook?  Probably, but I bet you're the only one who can quickly
figure out what it is.  That's no good.  We're not going to make
changes to PostgreSQL core that only you can maintain, and that are
likely to be broken by future commits.  At this point I feel pretty
good that someone can look at the stuff that we've done so far with
SECURITY LABEL and the object access hooks, and if they add a new
object type, they can make those things apply to the new object type
too by copying what's already there, without making any reference to
the sepgsql code.  There's a clear abstraction boundary, and people
who are working on one side of the boundary do not need to understand
the details of what happens on the other side of the boundary.

In this particular case, I think it might be reasonable to change the
DAC behavior, so that a CREATE TABLE AS SELECT or SELECT INTO requires
insert privileges on the new table as well as permission to create it
in the first place.  I don't particularly see any reason to require
different privileges for CREATE TABLE followed by INSERT .. SELECT
than what we require when the two commands are rolled into one.  Prior
to 9.0, this case couldn't arise, because we didn't have default
privileges, so I'm inclined to think that the way it works now is a
historical accident rather than a deliberate design decision.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-10-18 Thread Kohei KaiGai
2011/10/18 Robert Haas robertmh...@gmail.com:
 On Thu, Oct 13, 2011 at 6:48 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
    struct ObjectAccessInfoData {
        ObjectAccessType   oa_type;
        ObjectAddress         oa_address;
        union {
            struct {
                HeapTuple       newtuple;
                TupleDesc       tupdesc;  /* only if create a new relation */
                    :
            } post_create;      /* additional info for OAT_POST_CREATE event 
 */
        }
    }

 That's possibly an improvement over just passing everything opaquely,
 but it still doesn't seem very good.  I mean, this is C, not Perl or
 Python.  Anything where you pass a bunch of arguments of indeterminate
 type and hope that the person you're calling can figure it out is
 inherently pretty dangerous.  I had it in mind that the object access
 hook mechanism could serve as a simple and generic way of letting an
 external module know that an event of a certain type has occurred on a
 certain object, and to let that module gain control.  But if we have
 to pass a raft of arguments in, then it's not generic any more - it's
 just a bunch of things that are probably really need to be separate
 hooks shoved into a single function.

I got an inspiration of this idea from APIs of foreign-data-wrapper that
trusts values of each fields that contains function pointers.
Indeed, we may have a risk of backend crash, if we try to run modules
built towards v9.1 on the pgsql-v9.2devel. However, it is not avoidable,
unless we rewrite whole of the code by Java?, Python? or something? :-(

At least, we don't promise binary compatible across major versions?
If so, I don't think dangerousness is a reasonable reason why the hook
does not take any additional arguments, as long as auther of modules
can be noticed by compiler warnings.

The point to be focused on is how do we implement the target feature
(DDL permissions by sepgsql in my case) with simpleness.

 Moreover, if
 you did document it, I think it would boil down to this is what
 sepgsql happens to need, and I don't think that's an acceptable
 answer. ?We have repeatedly refused to adopt that approach in the
 past.

 Unfortunately, I still don't have a clear answer for this point.
 However, in general terms, it is impossible to design any interface without
 knowledge of its usage more or less.

 Well, that's true.  But the hook also has to be maintainable.  ISTM
 that there's no reasonable way for someone making a modification to
 the code to know whether the additional local variable that they just
 added to the function should be passed to the hook, or not.  Here, at
 least as far as I can see, there's no guiding principle that would
 enable future contributors to make an intelligent decision about that.
  And if someone wanted to write another client for the hook, it's not
 very obvious whether the particular things you've chosen to pass here
 would be relevant or not.  I think if you look over the code you'll
 find that there's nowhere else that we have a hook that looks anything
 like what you're proposing here.

In the case when the second client of the hook is proposed, a straight-
forward approach is to expand the above ObjectAccessInfoData to
satisfy the requirement of new one, if existing arguments are not enough.
Because existing modules ignore the new fields, it is harmless.

Linux adopts this approch to host multiple security modules without
confusion, although one makes decision by security label and one
other makes decision by pathname of files; they require different
information on the point to make its decision, because they ignore
unrelevant arguments; such as pathname in selinux.

I remember you worry about the number of arguments getting increased
infinity, however, it is an extreme situation. We are not an A.I to commit
proposed patches automatically, so auther of modules (including me)
will explain why the new arguments are additionally needed here, like as
pathname based security module did in Linux kernel development.

 At least, this proposition enables modules being informed using
 commonly used data type (such as HeapTuple, TupleDesc), compared
 to the past approach that tried to push all the necessary information
 into argument list individually.

 That does seem like a good direction to go in, but you're still
 passing a lot of other stuff in there.  I guess my feeling is that if
 we can't boil down the argument list to a short list of things that
 are more-or-less common to all the call sites, we probably need to
 rethink the whole design.

Honestly, the arguments of object_access_hook is a method to implement
security model of sepgsql, not a purpose of mine. So, an alternative idea is
quite welcome to implement sepgsql.
However, I think it is less invasive way than other ideas right now.

For example, I hope sepgsql to perform as follows when user create a new table.
- It computes a default security label that needs Oid of the 

Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-10-18 Thread Robert Haas
On Tue, Oct 18, 2011 at 11:25 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 For example, I hope sepgsql to perform as follows when user create a new 
 table.
 - It computes a default security label that needs Oid of the namespace.
 - It checks db_table:{create} permission on the security label being computed.
 - In addition, it checks db_table:{insert} permission when SELECT INTO
 - Also, it checks these permissions on columns being newly created.
 - However, It does not check permissions when the tables are internally 
 created,
  such as toast tables or temporary relations created by make_new_heap().

That's superficially reasonable, but I don't feel good about passing
is_select_info to the object access hook here.  If insert permission
is required there, then it ought to be getting checked by selinux at
the same place that it's getting checked for at the DAC level.  If we
get into a situation where sepgsql is checking permissions using some
system that's totally different from what we do for DAC, I think
that's going to produce confusing and illogical results.  This is
ground we've been over before.  Similarly with fdw_srvname.  The only
excuse for passing that is to handle a permissions check for foreign
data wrappers that isn't being done for DAC: if there is a DAC
permissions check, then the MAC one should be done there also, not
someplace totally different.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-10-18 Thread Kohei KaiGai
2011/10/18 Robert Haas robertmh...@gmail.com:
 On Tue, Oct 18, 2011 at 11:25 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 For example, I hope sepgsql to perform as follows when user create a new 
 table.
 - It computes a default security label that needs Oid of the namespace.
 - It checks db_table:{create} permission on the security label being 
 computed.
 - In addition, it checks db_table:{insert} permission when SELECT INTO
 - Also, it checks these permissions on columns being newly created.
 - However, It does not check permissions when the tables are internally 
 created,
  such as toast tables or temporary relations created by make_new_heap().

 That's superficially reasonable, but I don't feel good about passing
 is_select_info to the object access hook here.  If insert permission
 is required there, then it ought to be getting checked by selinux at
 the same place that it's getting checked for at the DAC level.  If we
 get into a situation where sepgsql is checking permissions using some
 system that's totally different from what we do for DAC, I think
 that's going to produce confusing and illogical results.  This is
 ground we've been over before.  Similarly with fdw_srvname.  The only
 excuse for passing that is to handle a permissions check for foreign
 data wrappers that isn't being done for DAC: if there is a DAC
 permissions check, then the MAC one should be done there also, not
 someplace totally different.

If you are suggesting DAC and MAC permissions should be checked
on the same place like as we already doing at ExecCheckRTPerms(),
I'd like to agree with the suggestion, rather than all the checks within
object_access_hook, although it will take code reworking around
access controls.

In the example table creation, heap_create_with_catalog() is invoked
by 5 routines, however, 3 of them are just internal usages, so it is not
preferable to apply permission checks on table creation

If we may have a hook on table creation next to the place currently
we checks permission on the namespace being created, it is quite
helpful to implement sepgsql.


BTW, it seems to me SELECT INTO should also check insert permission
on DAC level, because it become an incorrect assumption that owner of
table has insert permission on its creation time.

postgres=# ALTER DEFAULT PRIVILEGES FOR USER tak GRANT select ON TABLES TO tak;
ALTER DEFAULT PRIVILEGES
postgres=# \ddp
 Default access privileges
 Owner | Schema | Type  | Access privileges
---++---+---
 tak   || table | tak=r/tak
(1 row)

postgres=# SET SESSION AUTHORIZATION tak;
SET
postgres= SELECT * INTO t1 FROM (VALUES(1,'aaa'), (2,'bbb'), (3,'ccc')) AS v;
SELECT 3
postgres= SELECT * FROM t1;
 column1 | column2
-+-
   1 | aaa
   2 | bbb
   3 | ccc
(3 rows)

postgres= INSERT INTO t1 VALUES (4,'ddd');
ERROR:  permission denied for relation t1

Oops!
-- 
KaiGai Kohei kai...@kaigai.gr.jp

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-10-18 Thread Robert Haas
On Tue, Oct 18, 2011 at 1:23 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 If you are suggesting DAC and MAC permissions should be checked
 on the same place like as we already doing at ExecCheckRTPerms(),
 I'd like to agree with the suggestion, rather than all the checks within
 object_access_hook, although it will take code reworking around
 access controls.

Agreed.

 In the example table creation, heap_create_with_catalog() is invoked
 by 5 routines, however, 3 of them are just internal usages, so it is not
 preferable to apply permission checks on table creation

Some wit once made the remark that if a function has 10 arguments, it
has 11 arguments, meaning that functions with very large numbers of
arguments typically indicate a poor choice of abstraction boundary.
That's pretty much what I think is going on with
heap_create_with_catalog().  I think maybe some refactoring is in
order there, though I am not sure quite what.

 If we may have a hook on table creation next to the place currently
 we checks permission on the namespace being created, it is quite
 helpful to implement sepgsql.

Makes sense.

 BTW, it seems to me SELECT INTO should also check insert permission
 on DAC level, because it become an incorrect assumption that owner of
 table has insert permission on its creation time.

 postgres=# ALTER DEFAULT PRIVILEGES FOR USER tak GRANT select ON TABLES TO 
 tak;
 ALTER DEFAULT PRIVILEGES
 postgres=# \ddp
         Default access privileges
  Owner | Schema | Type  | Access privileges
 ---++---+---
  tak   |        | table | tak=r/tak
 (1 row)

 postgres=# SET SESSION AUTHORIZATION tak;
 SET
 postgres= SELECT * INTO t1 FROM (VALUES(1,'aaa'), (2,'bbb'), (3,'ccc')) AS v;
 SELECT 3
 postgres= SELECT * FROM t1;
  column1 | column2
 -+-
       1 | aaa
       2 | bbb
       3 | ccc
 (3 rows)

 postgres= INSERT INTO t1 VALUES (4,'ddd');
 ERROR:  permission denied for relation t1

 Oops!

You could make the argument that there's no real security hole there,
because the user could give himself those same privileges right back.
You could also make the argument that a SELECT .. INTO is not the same
as an INSERT and therefore INSERT privileges shouldn't be checked.  I
think there are valid arguments on both sides, but my main point is
that we shouldn't have core do it one way and sepgsql do it the other
way.  That's going to be impossible to maintain and doesn't really
make any logical sense anyway.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-10-17 Thread Robert Haas
On Thu, Oct 13, 2011 at 6:48 AM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
    struct ObjectAccessInfoData {
        ObjectAccessType   oa_type;
        ObjectAddress         oa_address;
        union {
            struct {
                HeapTuple       newtuple;
                TupleDesc       tupdesc;  /* only if create a new relation */
                    :
            } post_create;      /* additional info for OAT_POST_CREATE event */
        }
    }

That's possibly an improvement over just passing everything opaquely,
but it still doesn't seem very good.  I mean, this is C, not Perl or
Python.  Anything where you pass a bunch of arguments of indeterminate
type and hope that the person you're calling can figure it out is
inherently pretty dangerous.  I had it in mind that the object access
hook mechanism could serve as a simple and generic way of letting an
external module know that an event of a certain type has occurred on a
certain object, and to let that module gain control.  But if we have
to pass a raft of arguments in, then it's not generic any more - it's
just a bunch of things that are probably really need to be separate
hooks shoved into a single function.

 Moreover, if
 you did document it, I think it would boil down to this is what
 sepgsql happens to need, and I don't think that's an acceptable
 answer. ?We have repeatedly refused to adopt that approach in the
 past.

 Unfortunately, I still don't have a clear answer for this point.
 However, in general terms, it is impossible to design any interface without
 knowledge of its usage more or less.

Well, that's true.  But the hook also has to be maintainable.  ISTM
that there's no reasonable way for someone making a modification to
the code to know whether the additional local variable that they just
added to the function should be passed to the hook, or not.  Here, at
least as far as I can see, there's no guiding principle that would
enable future contributors to make an intelligent decision about that.
 And if someone wanted to write another client for the hook, it's not
very obvious whether the particular things you've chosen to pass here
would be relevant or not.  I think if you look over the code you'll
find that there's nowhere else that we have a hook that looks anything
like what you're proposing here.

 At least, this proposition enables modules being informed using
 commonly used data type (such as HeapTuple, TupleDesc), compared
 to the past approach that tried to push all the necessary information
 into argument list individually.

That does seem like a good direction to go in, but you're still
passing a lot of other stuff in there.  I guess my feeling is that if
we can't boil down the argument list to a short list of things that
are more-or-less common to all the call sites, we probably need to
rethink the whole design.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-10-13 Thread Kohei KaiGai
Robert,

I agree with it is a reasonable argument that compiler cannot raise warnings
if all the arguments are delivered as Datum. In fact, I also tried to implement
this feature with InvokeObjectAccessHook() defined as function.

The first needed point to be improved is that we hope compiler to raise
warnnings if and when number or type of arguments are changed in the
future version.
If so, how about an idea to define data types to inform modules the context
information in, such as:

struct ObjectAccessInfoData {
ObjectAccessType   oa_type;
ObjectAddress oa_address;
union {
struct {
HeapTuple   newtuple;
TupleDesc   tupdesc;  /* only if create a new relation */
:
} post_create;  /* additional info for OAT_POST_CREATE event */
}
}

Even if its invocation shall be wrapped up by macro, compiler can
raise warnings when caller set values on the ObjectAccessInfoData
structure.

It will also help the second points partially. The objectaccess.h will perform
a placeholder to describe specification of the argument. That clearly informs
developers what informations are available on the hook and what was lacked.

 Moreover, if
 you did document it, I think it would boil down to this is what
 sepgsql happens to need, and I don't think that's an acceptable
 answer. ?We have repeatedly refused to adopt that approach in the
 past.

Unfortunately, I still don't have a clear answer for this point.
However, in general terms, it is impossible to design any interface without
knowledge of its usage more or less.

We have several other hooks being available to loadable modules,
but I don't believe that we designed these hooks without knowledge
of use-cases, more or less.

At least, this proposition enables modules being informed using
commonly used data type (such as HeapTuple, TupleDesc), compared
to the past approach that tried to push all the necessary information
into argument list individually.

I think the answer of this matter is on the middle-of-position between
we should not know anything about sepgsql and we should know
everything about sepgsql, neither of them

 (This particular case is worse than average, because new_rel_tup is
 coming from InsertPgClassTuple, which therefore has to be modified,
 along with AddNewRelationTuple and index_create, to get the tuple back
 up to the call site where, apparently, it is needed.)

It might be a wrong design. All we want to inform was stored within
new_rel_desc in heap_create_with_catalog(). So, I should design the
hook to deliver new_rel_desc, instead of HeapTuple itself that need to
pull up from InsertPgClassTuple.

Thanks,

2011/10/12 Robert Haas robertmh...@gmail.com:
 On Thu, Sep 29, 2011 at 4:52 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I noticed that the previous revision does not provide any way to inform
 the modules name of foreign server, even if foreign table was created,
 on the OAT_POST_CREATE hook.
 So, I modified the invocation at heap_create_with_catalog to deliver
 this information to the modules.

 Rest of parts were unchanged, except for rebasing to the latest git
 master.

 I've never really been totally sanguine with the idea of making object
 access hooks take arguments, and all of my general concerns seem to
 apply to the way you've set this patch up.  In particular:

 1. Type safety goes out the window.  What you're essentially proposing
 here is that we should have one hook function can be used for almost
 anything at all and can be getting up to 9 arguments of any type
 whatsoever.  The hook will need to know how to interpret all of its
 arguments depending on the context in which it was called.  The
 compiler will be totally unable to do any static type-checking, so
 there will be absolutely no warning if the interface is changed
 incompatibly on either side.  Instead, you'll probably just crash the
 database server at runtime.

 2. The particular choice of data being passed to the object access
 hooks appears capricious and arbitrary.  Here's an example:

        /* Post creation hook for new relation */
 -       InvokeObjectAccessHook(OAT_POST_CREATE, RelationRelationId, relid, 0);
 +       InvokeObjectAccessHook4(OAT_POST_CREATE,
 +
 RelationRelationId, relid, 0,
 +
 PointerGetDatum(new_rel_tup),
 +
 PointerGetDatum(tupdesc),
 +
 BoolGetDatum(is_select_into),
 +
 CStringGetDatum(fdw_srvname));

 Now, I am sure that you have good reasons for wanting to pass those
 particular things to the object access hook rather than any other
 local variable or argument that might happen to be lying around at
 this point in the code, but they are not documented.  If someone adds
 a new argument to this function, or removes an argument that's being
 passed, they will have no idea what to do about this.  Moreover, if
 you did document it, I think it would boil down to this is what
 sepgsql happens to need, and I don't 

Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-10-12 Thread Robert Haas
On Thu, Sep 29, 2011 at 4:52 PM, Kohei KaiGai kai...@kaigai.gr.jp wrote:
 I noticed that the previous revision does not provide any way to inform
 the modules name of foreign server, even if foreign table was created,
 on the OAT_POST_CREATE hook.
 So, I modified the invocation at heap_create_with_catalog to deliver
 this information to the modules.

 Rest of parts were unchanged, except for rebasing to the latest git
 master.

I've never really been totally sanguine with the idea of making object
access hooks take arguments, and all of my general concerns seem to
apply to the way you've set this patch up.  In particular:

1. Type safety goes out the window.  What you're essentially proposing
here is that we should have one hook function can be used for almost
anything at all and can be getting up to 9 arguments of any type
whatsoever.  The hook will need to know how to interpret all of its
arguments depending on the context in which it was called.  The
compiler will be totally unable to do any static type-checking, so
there will be absolutely no warning if the interface is changed
incompatibly on either side.  Instead, you'll probably just crash the
database server at runtime.

2. The particular choice of data being passed to the object access
hooks appears capricious and arbitrary.  Here's an example:

/* Post creation hook for new relation */
-   InvokeObjectAccessHook(OAT_POST_CREATE, RelationRelationId, relid, 0);
+   InvokeObjectAccessHook4(OAT_POST_CREATE,
+
RelationRelationId, relid, 0,
+
PointerGetDatum(new_rel_tup),
+
PointerGetDatum(tupdesc),
+
BoolGetDatum(is_select_into),
+
CStringGetDatum(fdw_srvname));

Now, I am sure that you have good reasons for wanting to pass those
particular things to the object access hook rather than any other
local variable or argument that might happen to be lying around at
this point in the code, but they are not documented.  If someone adds
a new argument to this function, or removes an argument that's being
passed, they will have no idea what to do about this.  Moreover, if
you did document it, I think it would boil down to this is what
sepgsql happens to need, and I don't think that's an acceptable
answer.  We have repeatedly refused to adopt that approach in the
past.

(This particular case is worse than average, because new_rel_tup is
coming from InsertPgClassTuple, which therefore has to be modified,
along with AddNewRelationTuple and index_create, to get the tuple back
up to the call site where, apparently, it is needed.)

I am not exactly sure what the right way to solve this problem is, but
I don't think this is it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers


Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-10-03 Thread Kohei KaiGai
BTW, I remember that I was suggested the object-access-hooks to acquire
controls around changes of system catalogs are also useful to implement
clustering features, not only enhanced security features, when I had a talk
at PGcon2001.

It might be my mistake that I categorized this patch at the security topic.
If someone volunteers to review this patch from the different viewpoint, not
only security features, it is quite helpful for us.

Thanks,

2011/9/29 Kohei KaiGai kai...@kaigai.gr.jp:
 I noticed that the previous revision does not provide any way to inform
 the modules name of foreign server, even if foreign table was created,
 on the OAT_POST_CREATE hook.
 So, I modified the invocation at heap_create_with_catalog to deliver
 this information to the modules.

 Rest of parts were unchanged, except for rebasing to the latest git
 master.

 2011/8/28 Kohei KaiGai kai...@kaigai.gr.jp:
 The attached patch is a draft to support arguments in addition to
 OAT_* enum and object identifiers.

 The existing object_access_hook enables loadable modules to acquire
 control when objects are referenced. The first guest of this hook is
 contrib/sepgsql for assignment of default security label on newly
 created objects. Right now, OAT_POST_CREATE is the all supported
 object access type. However, we plan to enhance this mechanism onto
 other widespread purpose; such as comprehensive DDL permissions
 supported by loadable modules.

 This patch is a groundwork to utilize this hook for object creation
 permission checks, not only default labeling.
 At the v9.1 development cycle, I proposed an idea that defines both
 OAT_CREATE hook prior to system catalog modification and
 OAT_POST_CREATE hook as currently we have. This design enables to
 check permission next to the existing pg_xxx_aclcheck() or
 pg_xxx_ownercheck(), and raise an error before system catalog updates.
 However, it was painful to deliver private datum set on OAT_CREATE to
 the OAT_POST_CREATE due to the code complexity.

 The other idea tried to do all the necessary things in OAT_POST_CREATE
 hook, and it had been merged, because loadable modules can pull
 properties of the new object from system catalogs by the supplied
 object identifiers. Thus, contrib/sepgsql assigns a default security
 label on new object using OAT_POST_CREATE hook.
 However, I have two concern on the existing hook to implement
 permission check for object creation. The first one is the entry of
 system catalog is not visible using SnaphotNow, so we had to open and
 scan system catalog again, instead of syscache mechanism. The second
 one is more significant. A part of information to make access control
 decision is not available within contents of the system catalog
 entries. For example, we hope to skip permission check when
 heap_create_with_catalog() was launched by make_new_heap() because the
 new relation is just used to swap later.

 Thus, I'd like to propose to support argument of object_access_hook to
 inform the loadable modules additional contextual information on its
 invocations; to solve these concerns.

 Regarding to the first concern, fortunately, most of existing
 OAT_POST_CREATE hook is deployed just after insert or update of system
 catalogs, but before CCI. So, it is helpful for the loadable modules
 to deliver Form_pg_ data to reference properties of the new
 object, instead of open and scan the catalog again.
 In the draft patch, I enhanced OAT_POST_CREATE hook commonly to take
 an argument that points to the Form_pg_ data of the new object.

 Regarding to the second concern, I added a few contextual information
 as second or later arguments in a part of object classes. Right now, I
 hope the following contextual information shall be provided to
 OAT_POST_CREATE hook to implement permission checks of object
 creation.

 * pg_class - TupleDesc structure of the new relation
 I want to reference of pg_attribute, not only pg_class.

 * pg_class - A flag to show whether the relation is defined for
 rebuilding, or not.
 I want not to apply permission check in the case when it is invoked
 from make_new_heap(), because it just create a dummy table as a part
 of internal process. All the necessary permission checks should be
 done at ALTER TABLE or CLUSTER.

 And, name of the foreign server being used by the foreign table
 being created just a moment before.

 * pg_class - A flag to show whether the relation is created with
 SELECT INTO, or not.
 I want to check insert permission of the new table, created by
 SELECT INTO, because DML hook is not available to check this case.

 * pg_type - A flag to show whether the type is defined as implicit
 array, or not.
 I want not to apply permission check on creation of implicit array type.

 * pg_database - Oid of the source (template) database.
 I want to fetch security label of the source database to compute a
 default label of the new database.

 * pg_trigger - A flag to show whether the trigger is used to FK
 

Re: [HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-09-29 Thread Kohei KaiGai
I noticed that the previous revision does not provide any way to inform
the modules name of foreign server, even if foreign table was created,
on the OAT_POST_CREATE hook.
So, I modified the invocation at heap_create_with_catalog to deliver
this information to the modules.

Rest of parts were unchanged, except for rebasing to the latest git
master.

2011/8/28 Kohei KaiGai kai...@kaigai.gr.jp:
 The attached patch is a draft to support arguments in addition to
 OAT_* enum and object identifiers.

 The existing object_access_hook enables loadable modules to acquire
 control when objects are referenced. The first guest of this hook is
 contrib/sepgsql for assignment of default security label on newly
 created objects. Right now, OAT_POST_CREATE is the all supported
 object access type. However, we plan to enhance this mechanism onto
 other widespread purpose; such as comprehensive DDL permissions
 supported by loadable modules.

 This patch is a groundwork to utilize this hook for object creation
 permission checks, not only default labeling.
 At the v9.1 development cycle, I proposed an idea that defines both
 OAT_CREATE hook prior to system catalog modification and
 OAT_POST_CREATE hook as currently we have. This design enables to
 check permission next to the existing pg_xxx_aclcheck() or
 pg_xxx_ownercheck(), and raise an error before system catalog updates.
 However, it was painful to deliver private datum set on OAT_CREATE to
 the OAT_POST_CREATE due to the code complexity.

 The other idea tried to do all the necessary things in OAT_POST_CREATE
 hook, and it had been merged, because loadable modules can pull
 properties of the new object from system catalogs by the supplied
 object identifiers. Thus, contrib/sepgsql assigns a default security
 label on new object using OAT_POST_CREATE hook.
 However, I have two concern on the existing hook to implement
 permission check for object creation. The first one is the entry of
 system catalog is not visible using SnaphotNow, so we had to open and
 scan system catalog again, instead of syscache mechanism. The second
 one is more significant. A part of information to make access control
 decision is not available within contents of the system catalog
 entries. For example, we hope to skip permission check when
 heap_create_with_catalog() was launched by make_new_heap() because the
 new relation is just used to swap later.

 Thus, I'd like to propose to support argument of object_access_hook to
 inform the loadable modules additional contextual information on its
 invocations; to solve these concerns.

 Regarding to the first concern, fortunately, most of existing
 OAT_POST_CREATE hook is deployed just after insert or update of system
 catalogs, but before CCI. So, it is helpful for the loadable modules
 to deliver Form_pg_ data to reference properties of the new
 object, instead of open and scan the catalog again.
 In the draft patch, I enhanced OAT_POST_CREATE hook commonly to take
 an argument that points to the Form_pg_ data of the new object.

 Regarding to the second concern, I added a few contextual information
 as second or later arguments in a part of object classes. Right now, I
 hope the following contextual information shall be provided to
 OAT_POST_CREATE hook to implement permission checks of object
 creation.

 * pg_class - TupleDesc structure of the new relation
 I want to reference of pg_attribute, not only pg_class.

 * pg_class - A flag to show whether the relation is defined for
 rebuilding, or not.
 I want not to apply permission check in the case when it is invoked
 from make_new_heap(), because it just create a dummy table as a part
 of internal process. All the necessary permission checks should be
 done at ALTER TABLE or CLUSTER.

And, name of the foreign server being used by the foreign table
being created just a moment before.

 * pg_class - A flag to show whether the relation is created with
 SELECT INTO, or not.
 I want to check insert permission of the new table, created by
 SELECT INTO, because DML hook is not available to check this case.

 * pg_type - A flag to show whether the type is defined as implicit
 array, or not.
 I want not to apply permission check on creation of implicit array type.

 * pg_database - Oid of the source (template) database.
 I want to fetch security label of the source database to compute a
 default label of the new database.

 * pg_trigger - A flag to show whether the trigger is used to FK
 constraint, or not.
 I want not to apply permission check on creation of FK constraint. It
 should be done in ALTER TABLE level.

 Sorry for this long explanation. Right now, I tend to consider it is
 the best way to implement permission checks on object creation with
 least invasive way.

 Thanks, Any comments welcome.
 --
 KaiGai Kohei kai...@kaigai.gr.jp

-- 
KaiGai Kohei kai...@kaigai.gr.jp


pgsql-v9.2-object-access-hooks-argument.v2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list 

[HACKERS] [v9.2] Object access hooks with arguments support (v1)

2011-08-28 Thread Kohei KaiGai
The attached patch is a draft to support arguments in addition to
OAT_* enum and object identifiers.

The existing object_access_hook enables loadable modules to acquire
control when objects are referenced. The first guest of this hook is
contrib/sepgsql for assignment of default security label on newly
created objects. Right now, OAT_POST_CREATE is the all supported
object access type. However, we plan to enhance this mechanism onto
other widespread purpose; such as comprehensive DDL permissions
supported by loadable modules.

This patch is a groundwork to utilize this hook for object creation
permission checks, not only default labeling.
At the v9.1 development cycle, I proposed an idea that defines both
OAT_CREATE hook prior to system catalog modification and
OAT_POST_CREATE hook as currently we have. This design enables to
check permission next to the existing pg_xxx_aclcheck() or
pg_xxx_ownercheck(), and raise an error before system catalog updates.
However, it was painful to deliver private datum set on OAT_CREATE to
the OAT_POST_CREATE due to the code complexity.

The other idea tried to do all the necessary things in OAT_POST_CREATE
hook, and it had been merged, because loadable modules can pull
properties of the new object from system catalogs by the supplied
object identifiers. Thus, contrib/sepgsql assigns a default security
label on new object using OAT_POST_CREATE hook.
However, I have two concern on the existing hook to implement
permission check for object creation. The first one is the entry of
system catalog is not visible using SnaphotNow, so we had to open and
scan system catalog again, instead of syscache mechanism. The second
one is more significant. A part of information to make access control
decision is not available within contents of the system catalog
entries. For example, we hope to skip permission check when
heap_create_with_catalog() was launched by make_new_heap() because the
new relation is just used to swap later.

Thus, I'd like to propose to support argument of object_access_hook to
inform the loadable modules additional contextual information on its
invocations; to solve these concerns.

Regarding to the first concern, fortunately, most of existing
OAT_POST_CREATE hook is deployed just after insert or update of system
catalogs, but before CCI. So, it is helpful for the loadable modules
to deliver Form_pg_ data to reference properties of the new
object, instead of open and scan the catalog again.
In the draft patch, I enhanced OAT_POST_CREATE hook commonly to take
an argument that points to the Form_pg_ data of the new object.

Regarding to the second concern, I added a few contextual information
as second or later arguments in a part of object classes. Right now, I
hope the following contextual information shall be provided to
OAT_POST_CREATE hook to implement permission checks of object
creation.

* pg_class - TupleDesc structure of the new relation
I want to reference of pg_attribute, not only pg_class.

* pg_class - A flag to show whether the relation is defined for
rebuilding, or not.
I want not to apply permission check in the case when it is invoked
from make_new_heap(), because it just create a dummy table as a part
of internal process. All the necessary permission checks should be
done at ALTER TABLE or CLUSTER.

* pg_class - A flag to show whether the relation is created with
SELECT INTO, or not.
I want to check insert permission of the new table, created by
SELECT INTO, because DML hook is not available to check this case.

* pg_type - A flag to show whether the type is defined as implicit
array, or not.
I want not to apply permission check on creation of implicit array type.

* pg_database - Oid of the source (template) database.
I want to fetch security label of the source database to compute a
default label of the new database.

* pg_trigger - A flag to show whether the trigger is used to FK
constraint, or not.
I want not to apply permission check on creation of FK constraint. It
should be done in ALTER TABLE level.

Sorry for this long explanation. Right now, I tend to consider it is
the best way to implement permission checks on object creation with
least invasive way.

Thanks, Any comments welcome.
-- 
KaiGai Kohei kai...@kaigai.gr.jp


pgsql-v9.2-object-access-hooks-argument.v1.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers